Anatomy of the SQL injection in Drupal’s database comment filtering system SA-CORE-2015-003

In the Drupal security advisory that was released on August 19th, 2015 Drupal’s security team announced that it solved an SQL injection vulnerability in the Drupal database API.


The security advisory contained the following description of the SQL injection vulnerability:

A vulnerability was found in the SQL comment filtering system which could allow a user with elevated permissions to inject malicious code in SQL comments.


The following sections in this article explain where exactly the problem is located in Drupal and what the vulnerability is. Next it will explain when a Drupal site is vulnerable and how the vulnerability has been solved in the Drupal core code.


Background

When Drupal developers perform database queries, they can add a comment to queries. This comment is then logged in the database query log or in the list of active queries on an SQL server. This allows for easier debugging and allows you to more easily find where a query with a performance problem is being generated.


For example, a developer could issue the following query:

db_update('example')
->condition('id', $id)
->fields(array('field2' => 10))
->comment('Update field2')
->execute()

Drupal will take the comment string, enclose it with comment characters, and will finally append the SQL statement. As such, this would result in the following SQL statement:

/* Update field2 */ UPDATE example SET field2=...


Of course, the Drupal core developers were aware that this is a door opener for SQL injection attacks. For example, if the following query is made:

db_update('example')
->condition('id', $id)
->fields(array('field2' => 10))
->comment('Exploit */ DROP TABLE node; --')
->execute()

Then this would result in the following SQL statement:

/* Exploit */ DROP TABLE node; -- */ UPDATE example SET field2=...
So unless the comment is sanitised first, this would result in the SQL server dropping the node table and ignoring the rest of the SQL statement.


In order to address this, the core developers have foreseen the filterComment function that sanitizes comments in order to make SQL injections attacks like the above harmless.

The vulnerability

In Drupal 7 versions before 7.39, the filterComment was defined as follows:

protected function filterComment($comment = '') {

 return preg_replace('/(\/\*\s*)|(\s*\*\/)/', '', $comment);

}


For those not familiar with regular expressions, the above function will replace all occurrences of /* and */ in the $comment variable with an empty string.


If we take the vulnerable SQL query from before

->comment('Exploit */ DROP TABLE node; --')
Then the comment would be sanitized to

Exploit  DROP TABLE node; --

which would eventually result in the following no longer vulnerable SQL statement:

/* Exploit  DROP TABLE node; -- */ UPDATE example SET field2=...

So the regular expression ensures that vulnerable statements in the input are effectively removed. The problem with removing vulnerable statements is that by removing a piece of text from the input, the text before and after the removed text are concatenated together. This in turn might form a vulnerable statement again. And would effectively bypass the sanitization.


Imagine the following vulnerable SQL query:

->comment('Exploit **// DROP TABLE node; --')
During sanitization the */ would removed, which transforms the comment string to the following string:

Exploit */ DROP TABLE node; --

So after sanitization the vulnerable comment string has been "sanitized" into another vulnerable comment string!


The impact

This vulnerability is only exploitable if a contributed module uses variable text in its SQL queries. Furthermore, (parts of) this variable text must be user definable. The Drupal security team has found only one contributed module that uses the comment filtering system in a way that would trigger the vulnerability. That module requires you to have a very high level of access in order to perform the attack. So if you only use contributed modules from drupal.org, chances are small that the vulnerability is exploited on your website.


If you have custom modules that are not hosted on drupal.org, then it is recommended to check the code of the modules for occurrences of the comment method. Searching through the code for the text ->comment( should quickly return all occurrences. If the comment string contains a variable, then you might be vulnerable! You should then investigate where the variable value is being set. If this is user controllable, then the modules code is vulnerable!



The solution

Personally I see two solutions to this problem:

Recursive calls to the sanitization function

One possibility is to keep executing the preg_replace function on the input until the output is equal to the input. If an SQL injection would be introduced after the first execution of the preg_replace function, then the second execution would catch it and remove it. A possible implementation could have been the following:

protected function filterComment($comment = '') {

 $sanitized_comment = preg_replace('/(\/\*\s*)|(\s*\*\/)/', '', $comment);

 while ($comment != $sanitized_comment) {

   $comment = $sanitized_comment;

   $sanitized_comment = preg_replace('/(\/\*\s*)|(\s*\*\/)/', '', $comment);

 }

 return $sanitized_comment;

}

However, the Drupal core developers have chosen the next solution.

Redefine filterComment

Another solution would be to revise the filterComment function and do the sanitization in another way. This has also been the choice of the Drupal security team. The filterComment function has been rewritten as follows:


protected function filterComment($comment = '') {

 return strtr($comment, array('*' => ' * '));

}


The function has been redefined so that malicious characters are being filtered in another way. Instead of removing the malicious characters, the function will put spaces around the star-character. The reasoning behind this is that the SQL server expects that the star “*” and slash “/” characters are only recognized as comment characters when they are adjacent to each other.


If we take the original vulnerable SQL query from above

->comment('Exploit */ DROP TABLE node; --')
Then the comment would now be sanitized to

Exploit  * / DROP TABLE node; --

which would eventually result in the following no longer vulnerable SQL statement:

"/* Exploit  * / DROP TABLE node; -- */ UPDATE example SET field2=...

References

Drupal security advisory SA-CORE-2015-003: https://www.drupal.org/SA-CORE-2015-003

Tags: 

You might also be interested in...