Security review checklist #

This page is a checklist for the security part of a code review. Use it with the general Code review checklist. Each item names the rule it checks; the background is on Security engineering overview.

Contents #

1. Access control
2. Input and output
3. Database and shell
4. Forms, redirects and files
5. Secrets, logs and privacy
6. HTTP layer and browser
7. Runtime and dependencies

Access control #

• To do: Every new view names its policy functions in functions of module.php.
• To do: No new entry in [RoleSettings] PolicyOmitList[] or AnonymousAccessList[] without a reason written in the change.
• To do: Access is decided by policies, never by comparing a user or group id.
• To do: Fetches and lists apply the user's read permission (the content fetches do; custom SQL must not bypass them).
• To do: A new policy function reaches existing installations through an installer or update script.

Source: Modules, views and access control.

Input and output #

• To do: Request input is read through eZHTTPTool, not $_GET or $_POST.
• To do: Input is validated where it enters (a datatype in validateObjectAttributeHTTPInput()).
• To do: Every variable in a template is printed with |wash, or through an operator that escapes.
• To do: HTML built in PHP (an operator, a view) is escaped there.
• To do: Header values carry no CR or LF from input.

Source: Kernel architecture, Datatype and input hardening.

Database and shell #

• To do: Every string in SQL goes through $db->escapeString(); every number is cast with (int).
• To do: ORDER BY columns come from a whitelist.
• To do: Shell arguments go through escapeshellarg(); better, no shell at all.

Source: Security hardening of 6.0.13, Writing portable SQL.

Forms, redirects and files #

• To do: Every state-changing request is a POST with the form token (ezformtoken); no state change on GET.
• To do: Redirect targets go through the redirect manager and the allowed hosts.
• To do: Downloads go through content/download, which checks read access, or check it themselves.
• To do: Uploads are checked for type and content; SVG goes through the sanitizer.
• To do: Paths built from input cannot leave their directory.
• To do: Outgoing requests to URLs from input refuse private, loopback and link-local addresses.

Secrets, logs and privacy #

• To do: No credential, key or host-specific value in committed files; secrets live in settings/override/ or the environment.
• To do: No password, token, key or session id in a log, the audit trail, an error page or a debug message.
• To do: New log lines follow the masking of the log viewer and the request logs (addresses, e-mail, query values).
• To do: Security-relevant actions are recorded in the audit trail (The audit trail).

HTTP layer and browser #

• To do: No inline script without the CSP nonce; no on...= attributes or javascript: URLs.
• To do: No <style> blocks or style="" attributes in administration templates.
• To do: Code that reads the client address or scheme uses eZSys (which respects TrustedProxies[]), not raw headers.
• To do: Responses for signed-in users are not made cacheable by proxies.

Source: HTTP security: headers, CSP, request rules and proxies.

Runtime and dependencies #

• To do: No per-request secret or user data left in statics or globals of a long-lived worker (Concurrency, statics and long-lived processes).
• To do: A new Composer dependency is maintained, licensed compatibly with the GPL, and pinned to a released version (Dependency management with Composer).
• To do: A changed default that affects security is documented with how to undo it deliberately (Backward compatibility policy).
• To do: A vulnerability found during review is reported privately, as SECURITY.md says, not in the pull request.

Related pages #

• Security engineering overview
• Security review checklist for hosts
• Reviewing other people's work
• Free documentation: this page is free to copy, translate and improve under the GNU FDL 1.3.