Skip to content

Bugs and security

Rules for code that runs input it shouldn't trust, or reads the wrong value.

drupal/insecure-unserialize

  • Level: error
  • Fix: none
  • Source: Coder's DrupalPractice standard
  • Ports: DrupalPractice.FunctionCalls.InsecureUnserialize.InsecureUnserialize (partly)

DrupalPractice asks an unserialize() call to limit the classes it builds. Without a limit, the payload decides which objects PHP builds, and their destructors run.

Reports:

  • an unserialize() call that doesn't limit allowed_classes. Only a key of the options array itself counts, and (TRUE) counts as TRUE;
  • unserialize(...), which passes the call on with no options.

Not reported:

  • a payload that the same function built with serialize(), which can't hold a foreign class: unserialize(serialize($x)), and a local variable whose every assignment is a serialize() call. That is the shape of a serialization test. A read inside an arrow function sees the variable of the function around it, as it was when the arrow function was created. So an assignment that comes after the arrow function doesn't count. An assignment inside a nested closure or arrow function doesn't count either, because it writes to a variable of its own;
  • a call that spreads an array into its arguments, as in unserialize(...$args), because the spread can hold the options.
Example 
// Reported.
$data = unserialize($value);

// Fine.
$data = unserialize($value, ['allowed_classes' => FALSE]);
Compared with Coder
  • Coder reports a payload built with serialize() and a call that spreads its arguments.
  • Coder takes the second argument for the options, so it reports unserialize(options: ['allowed_classes' => FALSE], data: $d). The rule reads a named argument by its name.
  • The rule also reports \unserialize(), UNSERIALIZE(), \unserialize(...), (TRUE), a comment before TRUE, as in 'allowed_classes' => /* safe */ TRUE, and options whose only allowed_classes key is in a nested array. Coder misses them.

drupal/preg-security

  • Level: error
  • Fix: none
  • Source: Coder's Drupal standard
  • Ports: Drupal.Semantics.PregSecurity.PregEFlag

Coder asks for no e modifier on a preg_* pattern. That modifier evaluates the replacement as PHP.

Reports:

  • a preg_* pattern that uses the e modifier. The rule reads a literal pattern, and the first operand of a concatenation, as in '/a/e' . $flags.

Not reported:

  • a concatenation whose first piece only looks like it ends in modifiers. In '/edit' . $x . '/' the e follows the opening delimiter, and in '/a\/e' . $x . '/' a backslash escapes the delimiter.

The rule closes a bracket delimiter with its counterpart, so it reports '{a}e'.

Example 
// Reported.
$text = preg_replace('/(\w+)/e', 'strtoupper("$1")', $text);

// Fine.
$text = preg_replace_callback('/(\w+)/', fn (array $m): string => strtoupper($m[1]), $text);
Compared with Coder
  • Coder also reports a concatenation whose first piece only looks like it ends in modifiers.
  • Coder doesn't close a bracket delimiter with its counterpart, so it misses '{a}e'.
  • The rule also reports a call with a leading backslash or in other letter case, such as \preg_replace() or PREG_REPLACE(). Coder skips both, though PHP calls the same function.

drupal/remote-address

  • Level: error
  • Fix: none
  • Source: Coder's Drupal standard
  • Ports: Drupal.Semantics.RemoteAddress.RemoteAddress

Coder asks for the request's client IP in place of $_SERVER['REMOTE_ADDR']. Drupal can run behind a reverse proxy, and a read of the key ignores the proxy settings.

Reports:

  • a read of $_SERVER['REMOTE_ADDR'].

Not reported:

  • a write to the key.
Example 
// Reported.
$ip = $_SERVER['REMOTE_ADDR'];

// Fine.
$ip = \Drupal::request()->getClientIp();
Compared with Coder
  • The rule reports every read. Coder misses a read that starts a statement, a read in a call argument, isset(), an array element or an if condition, and a read with a space or a comment inside the brackets.

drupal/weak-hash

  • Level: warning
  • Fix: unsafe. It writes hash('xxh64', ...), which changes the digest.
  • Source: core's phpstan.neon.dist
  • Ports: its disallowed calls of md5(), sha1(), crc32() and hash()

Core's PHPStan configuration disallows the weak hash functions from Drupal 12.0.0, on core's main and 12.0.x branches. Core's 11.x branch doesn't, so the rule also reports Drupal 11 code that core's own checks accept. A module that targets Drupal 11 only can turn it off with "drupal/weak-hash" = { enabled = false }. drupal-core.mago.toml follows Drupal 11.x and turns it off, so a checkout of core's main branch turns it back on. See the change record.

Reports:

  • an md5(), sha1() or crc32() call, and a hash() call with md5, sha1, crc32 or crc32b.

The fix needs --unsafe, because a stored digest must be migrated and not only computed again. An md5() or sha1() call with a second argument doesn't have a fix.