Commit 7770f082 authored by Dries's avatar Dries
Browse files

- Patch #1105848 by cafuego, aspilicious: unsafe query comments possible via UI.

parent fa18bb3a
......@@ -540,6 +540,63 @@ public function makeSequenceName($table, $field) {
return $this->prefixTables('{' . $table . '}_' . $field . '_seq');
}
/**
* Flatten an array of query comments into a single comment string.
*
* The comment string will be sanitized to avoid SQL injection attacks.
*
* @param $comments
* An array of query comment strings.
*
* @return
* A sanitized comment string.
*/
public function makeComment($comments) {
if (empty($comments))
return '';
// Flatten the array of comments.
$comment = implode('; ', $comments);
// Sanitize the comment string so as to avoid SQL injection attacks.
return '/* ' . $this->filterComment($comment) . ' */ ';
}
/**
* Sanitize a query comment string.
*
* Ensure a query comment does not include strings such as "* /" that might
* terminate the comment early. This avoids SQL injection attacks via the
* query comment. The comment strings in this example are separated by a
* space to avoid PHP parse errors.
*
* For example, the comment:
* @code
* db_update('example')
* ->condition('id', $id)
* ->fields(array('field2' => 10))
* ->comment('Exploit * / DROP TABLE node; --')
* ->execute()
* @endcode
*
* Would result in the following SQL statement being generated:
* @code
* "/ * Exploit * / DROP TABLE node; -- * / UPDATE example SET field2=..."
* @endcode
*
* Unless the comment is sanitised first, the SQL server would drop the
* node table and ignore the rest of the SQL statement.
*
* @param $comment
* A query comment string.
*
* @return
* A sanitized version of the query comment string.
*/
protected function filterComment($comment = '') {
return preg_replace('/(\/\*\s*)|(\s*\*\/)/', '', $comment);
}
/**
* Executes a query string against the database.
*
......
......@@ -42,8 +42,8 @@ public function execute() {
}
public function __toString() {
// Create a comments string to prepend to the query.
$comments = (!empty($this->comments)) ? '/* ' . implode('; ', $this->comments) . ' */ ' : '';
// Create a sanitized comment string to prepend to the query.
$comments = $this->connection->makeComment($this->comments);
// Default fields are always placed first for consistency.
$insert_fields = array_merge($this->defaultFields, $this->insertFields);
......@@ -92,8 +92,8 @@ public function __toString() {
// not transactional, and result in an implicit COMMIT. When we are in a
// transaction, fallback to the slower, but transactional, DELETE.
if ($this->connection->inTransaction()) {
// Create a comments string to prepend to the query.
$comments = (!empty($this->comments)) ? '/* ' . implode('; ', $this->comments) . ' */ ' : '';
// Create a comment string to prepend to the query.
$comments = $this->connection->makeComment($this->comments);
return $comments . 'DELETE FROM {' . $this->connection->escapeTable($this->table) . '}';
}
else {
......
......@@ -103,8 +103,8 @@ public function execute() {
}
public function __toString() {
// Create a comments string to prepend to the query.
$comments = (!empty($this->comments)) ? '/* ' . implode('; ', $this->comments) . ' */ ' : '';
// Create a sanitized comment string to prepend to the query.
$comments = $this->connection->makeComment($this->comments);
// Default fields are always placed first for consistency.
$insert_fields = array_merge($this->defaultFields, $this->insertFields);
......
......@@ -361,6 +361,9 @@ public function nextPlaceholder() {
* for easier debugging and allows you to more easily find where a query
* with a performance problem is being generated.
*
* The comment string will be sanitized to remove * / and other characters
* that may terminate the string early so as to avoid SQL injection attacks.
*
* @param $comment
* The comment string to be inserted into the query.
*
......@@ -623,9 +626,8 @@ public function execute() {
* The prepared statement.
*/
public function __toString() {
// Create a comments string to prepend to the query.
$comments = (!empty($this->comments)) ? '/* ' . implode('; ', $this->comments) . ' */ ' : '';
// Create a sanitized comment string to prepend to the query.
$comments = $this->connection->makeComment($this->comments);
// Default fields are always placed first for consistency.
$insert_fields = array_merge($this->defaultFields, $this->insertFields);
......@@ -815,9 +817,8 @@ public function execute() {
* The prepared statement.
*/
public function __toString() {
// Create a comments string to prepend to the query.
$comments = (!empty($this->comments)) ? '/* ' . implode('; ', $this->comments) . ' */ ' : '';
// Create a sanitized comment string to prepend to the query.
$comments = $this->connection->makeComment($this->comments);
$query = $comments . 'DELETE FROM {' . $this->connection->escapeTable($this->table) . '} ';
......@@ -884,8 +885,8 @@ public function execute() {
* The prepared statement.
*/
public function __toString() {
// Create a comments string to prepend to the query.
$comments = (!empty($this->comments)) ? '/* ' . implode('; ', $this->comments) . ' */ ' : '';
// Create a sanitized comment string to prepend to the query.
$comments = $this->connection->makeComment($this->comments);
return $comments . 'TRUNCATE {' . $this->connection->escapeTable($this->table) . '} ';
}
......@@ -1111,9 +1112,8 @@ public function execute() {
* The prepared statement.
*/
public function __toString() {
// Create a comments string to prepend to the query.
$comments = (!empty($this->comments)) ? '/* ' . implode('; ', $this->comments) . ' */ ' : '';
// Create a sanitized comment string to prepend to the query.
$comments = $this->connection->makeComment($this->comments);
// Expressions take priority over literal fields, so we process those first
// and remove any literal fields that conflict.
......
......@@ -1439,9 +1439,8 @@ public function countQuery() {
}
public function __toString() {
// Create a comments string to prepend to the query.
$comments = (!empty($this->comments)) ? '/* ' . implode('; ', $this->comments) . ' */ ' : '';
// Create a sanitized comment string to prepend to the query.
$comments = $this->connection->makeComment($this->comments);
// SELECT
$query = $comments . 'SELECT ';
......
......@@ -32,8 +32,8 @@ public function execute() {
}
public function __toString() {
// Create a comments string to prepend to the query.
$comments = (!empty($this->comments)) ? '/* ' . implode('; ', $this->comments) . ' */ ' : '';
// Create a sanitized comment string to prepend to the query.
$comments = $this->connection->makeComment($this->comments);
// Produce as many generic placeholders as necessary.
$placeholders = array_fill(0, count($this->insertFields), '?');
......@@ -148,8 +148,8 @@ public function execute() {
*/
class TruncateQuery_sqlite extends TruncateQuery {
public function __toString() {
// Create a comments string to prepend to the query.
$comments = (!empty($this->comments)) ? '/* ' . implode('; ', $this->comments) . ' */ ' : '';
// Create a sanitized comment string to prepend to the query.
$comments = $this->connection->makeComment($this->comments);
return $comments . 'DELETE FROM {' . $this->connection->escapeTable($this->table) . '} ';
}
......
......@@ -1324,6 +1324,27 @@ class DatabaseSelectTestCase extends DatabaseTestCase {
$this->assertEqual($query, $expected, t('The flattened query contains the comment string.'));
}
/**
* Test query COMMENT system against vulnerabilities.
*/
function testVulnerableComment() {
$query = db_select('test')->comment('Testing query comments */ SELECT nid FROM {node}; --');
$name_field = $query->addField('test', 'name');
$age_field = $query->addField('test', 'age', 'age');
$result = $query->execute();
$num_records = 0;
foreach ($result as $record) {
$num_records++;
}
$query = (string)$query;
$expected = "/* Testing query comments SELECT nid FROM {node}; -- */ SELECT test.name AS name, test.age AS age\nFROM \n{test} test";
$this->assertEqual($num_records, 4, t('Returned the correct number of rows.'));
$this->assertEqual($query, $expected, t('The flattened query contains the sanitised comment string.'));
}
/**
* Test basic conditionals on SELECT statements.
*/
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment