Detecting possible SQL Injection Vulernerabilties

[craigfrancis]

On your Todos, you have "security rule: detect possible sql injections".

May I suggest using the literal-string type, as it's very simple, and is the only way of being sure an Injection Vulnerability cannot exist.

---

It basically says the SQL must be written by the developer (i.e. it cannot use any dangerous user input).

All user input should use parameters, on the basis that it's easy for escaping to be forgotten, or go wrong:

$sql .= ' WHERE id = ' . $mysqli->real_escape_string($_GET['id']);

Notice how the developer forgot to quote the value, so the attacker could easily do example.com/?id=id (or something a bit more complex, like using a UNION)... and that's before we get into character set issues, and the sql_mode option NO_BACKSLASH_ESCAPES.

If a developer wants to use the IN() operator, that's fine, as they should still use parameters, maybe with something safe like:

$sql .= 'id IN (' . join(',', array_fill(0, count($ids), '?')) . ')';

// id IN (?, ?, ?)

or maybe use a function like this, to make their code easier to read:

/** @return literal-string */
function in_sql(int $count) { // Maybe check for 0?
  $sql = '?';
  for ($k = 1; $k < $count; $k++) {
    $sql .= ',?';
  }
  return $sql;
}

$sql .= 'id IN (' . in_sql(count($ids)) . ')';

And if a developer is doing something like allowing the user to choose how the results are sorted, they should limit the user to only those fields they are allowed to sort by (and nothing more), e.g.

$order_fields = ['name', 'created', 'type'];

$order_id = array_search(($_GET['sort'] ?? NULL), $order_fields);

$sql .= ' ORDER BY ' . $order_fields[$order_id];

There may be cases where the field names cannot be defined in the source code (some complex CMS'es do this, for "reasons")... in which case, they can do something like @phpstan-ignore-line... this should be very rare, and will be flagged as something to check during an audit (a good thing).

---

I must admit it's been a while since I've used PDO and MySQLi directly, but I think it's the $sql argument in these functions that will need checking:

pdo::exec($sql);
pdo::query($sql);
pdo::prepare($sql);

mysqli::prepare($sql);
mysqli::query($sql);
mysqli::real_query($sql);
mysqli::multi_query($sql);

[staabm]

I think phpstan-dba will have different views/solutions on this problem

• add a RuntimeConfiguration which allows enforcing literal-string queries
• add a Rule which checks whether proper ' quoting is used arround quoting functions/methods like real_escape_string
• maybe something more I don't have thought about yet

[craigfrancis]

While I'd prefer literal-string to be the primary method (and to make developers aware of it); the real_escape_string() one could work... but some things to keep in mind, where it's not simply surrounded by single quotes, making them risky, but technically valid:

• "WHERE name = '" . $mysqli->real_escape_string($name_first) . " " . $mysqli->real_escape_string($name_last) . "'"
• "WHERE name LIKE '%" . $mysqli->real_escape_string($name) . "%'"
• "WHERE name REGEXP '^" . $mysqli->real_escape_string($name) . "'" (as an aside, it's risky allowing user values in a RegEx, could create a denial of service with an overly complex pattern).
• "WHERE name = '" . $db->escape($name) . "'" (not calling real_escape_string directly)
• "WHERE id = " . intval($id)
• "WHERE id IN (" . implode(",", array_map("intval", $ids)) . ")" (really hope they always remember to call intval, I've seen a few cases where that wasn't the case).
• 'WHERE name = "' . $mysqli->real_escape_string($name) . '"' (a string could use double quotes, even though it's not ideal, e.g. ANSI_QUOTES mode treating double quotes as an identifier).
• Escaping doesn't always play well with mixed character encodings, or NO_BACKSLASH_ESCAPES.