Remove SQL injections from PHP. Forever.

A little bit of history

SQL injection in one of the top vulnerabilities in web applications for some years now. Once found, it’s not difficult to exploit, the exploitation is reliable and with disastrous results. The least is access to the whole database, depending on the configuration it may escalate to compromise the database host itself.

It’s really surprising to see web applications vulnerable to SQL injection since it’s not difficult to automatically analyze the code and detect them. The current situation is pretty similar to the so called ‘format strings vulnerabilities’. This class of vulnerabilities were first announced around 1999 and for a while they were all the rage. Bugs resulting form format strings vulnerabilities began to collapse rapidly since 2007-2008, because they were easy to detect by static code analysis or by the compiler itself. Currently, for practical exploitation purposes, this class of bugs is extinct.

Today the same situation repeats with SQL injection vulnerabilities and I think they will become extinct in two to four years. The difference with format string vulnerabilities is that not all the languages used for web application development are compiled languages. So you can’t rely on the compiler, the way C/C++ software did for format string vulnerabilities. This means that for the time being, a static code checks for interpreted languages such as PHP, Ruby or Python need to be developed.

What follows is the description of such a tool, that we recently developed for our needs. It’s used in production software - it’s fast and reliable - PHP-Reaper. I assume you remember the three pillars of test automation. This tool belongs to the first pillar.

The problem

We had a fairly large PHP codebase using ADOdb to talk to MySQL. Due to unawareness, carelessness or just plain rush to get a feature out to the customers, it was riddled with lots of SQL injection vulnerabilities. Here is one example of vulnerable code:

$dbConn->GetRow("SELECT * FROM users WHERE id = $user_id");

Note how $user_id variable is inserted directly in the SQL query string. If an attacker controls $user_id, she can cause SQL injection.

The safe way to write this query:

$dbConn->GetRow("SELECT * FROM users WHERE id = ?", array('$user_id'));

The variable needs to be put in the parameters array (the second argument to GetRow() method). It is bound and no SQL injection is possible. The second code snipped requires more typing, it requires extra effort and more attention.

This should be reminder to anyone designing safety related features, not just in software. In order to do something the wrong way, it should require more effort, more deliberation than to do the right thing. By default, it should be easy and simple to perform the correct action.

Another example of vulnerable SQL query:

$ids = join(',', $ids);
$dbConn->GetAll("SELECT * FROM campaigns WHERE id IN ({$ids})");

Again, note how the author is taking the easy way and just puts the parameters in the SQL string.

Now compare to the proper way to write this query:

$dbConn->GetAll('SELECT * FROM campaigns WHERE FIND_IN_SET (id, ' . $dbConn->Param('') . ')', array(join(',', $ids)));

See how complex it looks like? No wonder the developers are taking shortcuts.

I hope you realize, that it’s not the developers to blame. They are under all sorts of pressures to release features faster. The solution is to have an automatic check done for us, on commit, that will warn us, should a potential SQL injection code is committed to the repository.

Inner Workings

The way PHP-Reaper works is to find all the ADOdb methods that deal with executing SQL queries: getone(), getrow(), getall(), getcol(), getassoc(), execute(), replace(). Then it check the string in the first argument, investigates if it’s just a constant (meaning it’s safe), or it’s a string that is constructed on the fly by string concatenation or interpolation (potentially unsafe). Then we try to check where all the parts that construct the SQL query string are coming from, if all of them are constants, then it is safe. If however they are set on the fly, or due to the dynamic nature of PHP, we cannot detect where they coming from, we’re flagging the code as potentially vulnerable to SQL injection.

When you run PHP-Reaper for the first time on a code base, it may report a lot of unsafe sql queries, and you need to manually review them all and fix them. All may not be exploitable, but it’s just good engineering practice to bind the SQL query parameters anyway. You never know, in the future, some of those parameters may become controllable by a user of the system (an attacker).

Once you fix all the potential SQL injections, run PHP-Reaper on commit, as part of your CI job and it will fail the build every time it detects a problem. If you keep your commits small, then you’ll need to fix no more than one or two SQL queries at a time.

Exclusions

There are times when you investigate potential SQL vulnerability in the code and it turns out that it cannot be exploited. It may require lots of rewrite to get it to pass the PHP-Reaper. In such cases, you can add а comment above the SQL query in question:

//safesql

This will ignore the problem and will not fail the build. However this should be temporary solution. The SQL query maybe safe now, but in the future, someone may do a code change and actually make the same SQL query vulnerable. If the comments stays, this new vulnerability will slip under the radar. My recommendation is to use //safesql only when absolutely needed and for a short period of time. Ideally you should rewrite the SQL query and bind all its parameters.

Comparison with web vulnerability scanners

Using PHP-Reaper will get rid of all SQL injection vulnerabilities in your code. A whole class of problems will not exist in your web application anymore. Compare this to the regular web application vulnerability scanners like Burp or Acunetix. They test your application through the UI, and may not be able to reach all the vulnerable code. This maybe because you login with different user, cannot reach a piece of functionality due to a specific settings, actions may need to be performed in a certain order.

It’s no only that web application vulnerabilities scanners will miss vulnerable code, but they are also slow. Compare that to PHP-Reaper that will detect all vulnerabilities and runs for seconds even on larger code base. Or course web application vulnerabilities scanners are useful for detecting other types of attack vectors - like XSS or CSRF, but at least you don’t have to worry about SQL injections. You can safely remove scanning for SQL injections and your vulnerabilities detection tool will run faster.

Potential pitfalls

• Only analyzes PHP code that’s using ADOdb to connect to the DB
• As PHP is dynamic there is no way to statically determine which methods are called from which class. This means that PHP-Reaper is detecting the ADOdb method that deal with SQL queries only by their name - getone(), getrow(), getall(), getcol(), getassoc(), execute(), replace(). If you have other methods with the same names, you will get false positives. As a workaround, rename your methods.

Conclusion

PHP-Reaper solves a particular, nasty and time consuming problem that we had. The existing tools we’re either nonexistent or we’re inefficient (web application vulnerability scanners). So we developed our own solution. You’ll likely not have the exact same problem. However, I want to inspire you to think and use different angles to solve your problems and when improving the quality of your code. This is the sign of a true Lean Organization. Custom tools and static code analysis can go a long way and save you time and money.