Issue #1105848 by cafuego: Fixed Unsafe query comments possible via UI.

7 files changed, 101 insertions, 24 deletions

diff --git a/includes/database/database.inc b/includes/database/database.inc
index 4539b37a7..4cc1a33d7 100644
--- a/includes/database/database.inc
+++ b/includes/database/database.inc
@@ -541,6 +541,63 @@ abstract class DatabaseConnection extends PDO {
   }
 
   /**
+   * 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.
    *
    * This method provides a central handler for the actual execution of every
diff --git a/includes/database/mysql/query.inc b/includes/database/mysql/query.inc
index f7fb52f04..888b6a5a4 100644
--- a/includes/database/mysql/query.inc
+++ b/includes/database/mysql/query.inc
@@ -42,8 +42,8 @@ class InsertQuery_mysql extends InsertQuery {
   }
 
   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 @@ class TruncateQuery_mysql extends TruncateQuery {
     // 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 fe7909e17..f3783a9ca 100644
--- a/includes/database/pgsql/query.inc
+++ b/includes/database/pgsql/query.inc
@@ -103,8 +103,8 @@ class InsertQuery_pgsql extends InsertQuery {
   }
 
   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 7f3e9ff85..23b652f9b 100644
--- a/includes/database/query.inc
+++ b/includes/database/query.inc
@@ -361,6 +361,9 @@ abstract class Query implements QueryPlaceholderInterface {
    * 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 @@ class InsertQuery extends Query {
    *   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 @@ class DeleteQuery extends Query implements QueryConditionInterface {
    *   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 @@ class TruncateQuery extends Query {
    *   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 @@ class UpdateQuery extends Query implements QueryConditionInterface {
    *   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 6e4b0dc48..716c2fc3d 100644
--- a/includes/database/select.inc
+++ b/includes/database/select.inc
@@ -1439,9 +1439,8 @@ class SelectQuery extends Query implements SelectQueryInterface {
   }
 
   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 a176ed649..6b8a72f2a 100644
--- a/includes/database/sqlite/query.inc
+++ b/includes/database/sqlite/query.inc
@@ -32,8 +32,8 @@ class InsertQuery_sqlite extends InsertQuery {
   }
 
   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 @@ class DeleteQuery_sqlite extends DeleteQuery {
  */
 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 231355ceb..c22d1fc5d 100644
--- a/modules/simpletest/tests/database_test.test
+++ b/modules/simpletest/tests/database_test.test
@@ -1325,6 +1325,27 @@ class DatabaseSelectTestCase extends DatabaseTestCase {
   }
 
   /**
+   * 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.
    */
   function testSimpleSelectConditional() {