更安全的PDO ::准备？

Question

Using PDO::prepare potentially greatly reduces the possibility of SQL injection, because it allows parameterized queries. However, as it also allows non-parameterized queries, its safety depends entirely on how it's used.

For my fairly inexperienced programming team, I'm thinking of putting a wrapper over PDO::prepare that has one additional level of filtering: It generates an error if the argument contains any single or double quotes.

Now, during code inspection, all we have to do is check that every query uses the wrapper instead of PDO::prepare (or any other function that does queries). When we discover a violation, it either needs to be fixed, or, in very rare cases, it involves a dynamic query that can't be handled by a parameter, such as a variable column or table name. (Very unusual in the kinds of applications we build.) Those few cases can then be studied very carefully to make sure they're safe.

My question is this: Does ruling out quotes in the statement prevent SQL injection from non-parameterized queries, as I'm assuming it does, and is it a practical tool to make code inspections more reliable and efficient?

Answer 1

No, this is not a practical solution. There are legitimate reasons to use quotes in SQL statements, because string literals and date literals use quotes. There is no valid reason to use query parameters for every string or date constant in an SQL query. It just makes your queries unreadable, and in some cases has a performance cost.

There are also opportunities for SQL injection that have nothing to do with quotes. Any part of dynamic content (i.e. application variables) that is formed into an SQL query has a potential for being insecure if implemented without good practices.

A numeric constant in SQL does not need quotes. Your filter function wouldn't catch these cases.

$sql = "SELECT * FROM MyTable WHERE id = $UnsafeVariable";

I spoke with a PHP security expert and author, and he claimed there are no cases where query parameters aren't the solution. I showed him this query:

$sql = "SELECT * FROM MyTable ORDER BY $SomeColumn $AscOrDesc";

It's a pretty clear example of something you'd like to do in a web application, when you have a user interface that allows the user to choose column by which to sort, or click on an up-arrow or down-arrow to choose the direction of sorting. These are cases that don't involve quotes, and can't be parameterized with prepared statements, but they're still at risk of SQL injection flaws if $SomeColumn or $AscOrDesc contain unvalidated content.

Nor is it practical to make blanket rules like the one you describe. There are always exception cases. You'll spend too much time reviewing these cases and authorizing developers to bypass your filtering function. The problem with one-size-fits-all rules is that they don't.

Code reviews are a cost-effective and practical way to improve code quality.

The better solution is to allow developers to write code in the most practical way, but create a policy that every code commit goes through code review.

We do that in my current company. Even senior developers are required to get another developer to do a code review with them. We use github, and we've started a practice that every nontrivial code change follows these steps:

1. Create a branch from the main development branch.
2. Develop the fix in that branch, committing as many times as needed.
3. The committer issues a pull request. This merges all the commits into one diff that is easily reviewed.
4. A peer performs a code review on the pull request. They can use github to comment on the pull request to ask questions or suggest corrections. This creates a log that the code review happened.
5. The committer makes fixes in response to the code review.
6. The code reviewer does a final review and then approves the pull request, merging the fix into the main branch. The temporary branch can be deleted.

Not only does this catch mistakes (even senior developers can miss something), but showing more code to the junior developers will only improve their skills. That creates a multiplier effect as your junior developers grow, because they make fewer rookie mistakes, and they can even become senior developers to help the next group of juniors.