diff --git a/includes/database/database.inc b/includes/database/database.inc
index 4539b37a74db56cb0f30c487d51e23288b6d6801..4cc1a33d75565d11302f02117687b8113690c8b0 100644
--- a/includes/database/database.inc
+++ b/includes/database/database.inc
@@ -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.
*
diff --git a/includes/database/mysql/query.inc b/includes/database/mysql/query.inc
index f7fb52f04595ee2c43dcf1afda96fe2a8e39ba2a..888b6a5a450e613b172814e53b0f53f27e76cf69 100644
--- a/includes/database/mysql/query.inc
+++ b/includes/database/mysql/query.inc
@@ -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 {
diff --git a/includes/database/pgsql/query.inc b/includes/database/pgsql/query.inc
index fe7909e17c5db9b8e4c310761a4bc46596cb9442..f3783a9ca8f9ceb4f745c650fa826ee3271e1df4 100644
--- a/includes/database/pgsql/query.inc
+++ b/includes/database/pgsql/query.inc
@@ -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);
diff --git a/includes/database/query.inc b/includes/database/query.inc
index 7f3e9ff85703eb77978013bd3d2d9c656e0939b7..23b652f9b4b439114d677db0ddd5f0a75414814a 100644
--- a/includes/database/query.inc
+++ b/includes/database/query.inc
@@ -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.
diff --git a/includes/database/select.inc b/includes/database/select.inc
index 6e4b0dc4869ae3915d4e9a2ca4f6c74665c5641f..716c2fc3dbf7f67f82ac361e2c61e2694ddf93bb 100644
--- a/includes/database/select.inc
+++ b/includes/database/select.inc
@@ -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 ';
diff --git a/includes/database/sqlite/query.inc b/includes/database/sqlite/query.inc
index a176ed64937c90e9e13ce7a680a2d0ddefefd9d9..6b8a72f2ab46aed8da9f908a7f66f4f71d27b11a 100644
--- a/includes/database/sqlite/query.inc
+++ b/includes/database/sqlite/query.inc
@@ -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) . '} ';
}
diff --git a/modules/simpletest/tests/database_test.test b/modules/simpletest/tests/database_test.test
index 231355ceb1d45fff45311381fe1191505f9cead0..c22d1fc5d0b28600d1e5f044a87795c5e03b0758 100644
--- a/modules/simpletest/tests/database_test.test
+++ b/modules/simpletest/tests/database_test.test
@@ -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.
*/