Issue #2492967 by dawehner, cilefen, greggles, larowlan, hussainweb, chx,...

Issue #2492967 by dawehner, cilefen, greggles, larowlan, hussainweb, chx, bircher, mr.baileys, webchick, Berdir, Fabianx, catch, pwolanin, Crell: SQL layer: $match_operator is vulnerable to injection attack

core/lib/Drupal/Core/Database/Query/Condition.php

@@ -188,6 +188,25 @@ public function compile(Connection $connection, PlaceholderInterface $queryPlace
               'operator' => $condition['operator'],
               'use_value' => TRUE,
             );
+            // Remove potentially dangerous characters.
+            // If something passed in an invalid character stop early, so we
+            // don't rely on a broken SQL statement when we would just replace
+            // those characters.
+            if (stripos($condition['operator'], 'UNION') !== FALSE || strpbrk($condition['operator'], '[-\'"();') !== FALSE) {
+              $this->changed = TRUE;
+              $this->arguments = [];
+              // Provide a string which will result into an empty query result.
+              $this->stringVersion = '( AND 1 = 0 )';
+
+              // Conceptually throwing an exception caused by user input is bad
+              // as you result into a WSOD, which depending on your webserver
+              // configuration can result into the assumption that your site is
+              // broken.
+              // On top of that the database API relies on __toString() which
+              // does not allow to throw exceptions.
+              trigger_error('Invalid characters in query operator: ' . $condition['operator'], E_USER_ERROR);
+              return;
+            }
             $operator = $connection->mapConditionOperator($condition['operator']);
             if (!isset($operator)) {
               $operator = $this->mapConditionOperator($condition['operator']);

core/modules/system/src/Tests/Database/DatabaseTestBase.php

@@ -31,6 +31,7 @@ protected function setUp() {
       'test_null',
       'test_serialized',
       'test_special_columns',
+      'TEST_UPPERCASE',
     ));
     self::addSampleData();
   }

core/modules/system/src/Tests/Database/QueryTest.php

@@ -66,6 +66,74 @@ public function testArrayArgumentsSQLInjection() {
     $this->assertFalse($result, 'SQL injection attempt did not result in a row being inserted in the database table.');
   }
 
+  /**
+   * Tests SQL injection via condition operator.
+   */
+  public function testConditionOperatorArgumentsSQLInjection() {
+    $injection = "IS NOT NULL) ;INSERT INTO {test} (name) VALUES ('test12345678'); -- ";
+
+    // Convert errors to exceptions for testing purposes below.
+    set_error_handler(function ($severity, $message, $filename, $lineno) {
+      throw new \ErrorException($message, 0, $severity, $filename, $lineno);
+    });
+    try {
+      $result = db_select('test', 't')
+        ->fields('t')
+        ->condition('name', 1, $injection)
+        ->execute();
+      $this->fail('Should not be able to attempt SQL injection via condition operator.');
+    }
+    catch (\ErrorException $e) {
+      $this->pass('SQL injection attempt via condition arguments should result in a database exception.');
+    }
+
+    // Test that the insert query that was used in the SQL injection attempt did
+    // not result in a row being inserted in the database.
+    $result = db_select('test')
+      ->condition('name', 'test12345678')
+      ->countQuery()
+      ->execute()
+      ->fetchField();
+    $this->assertFalse($result, 'SQL injection attempt did not result in a row being inserted in the database table.');
+
+    // Attempt SQLi via union query with no unsafe characters.
+    $this->enableModules(['user']);
+    $this->installEntitySchema('user');
+    db_insert('test')
+      ->fields(['name' => '123456'])
+      ->execute();
+    $injection = "= 1 UNION ALL SELECT password FROM user WHERE uid =";
+
+    try {
+      $result = db_select('test', 't')
+        ->fields('t', array('name', 'name'))
+        ->condition('name', 1, $injection)
+        ->execute();
+      $this->fail('Should not be able to attempt SQL injection via operator.');
+    }
+    catch (\ErrorException $e) {
+      $this->pass('SQL injection attempt via condition arguments should result in a database exception.');
+    }
+
+    // Attempt SQLi via union query - uppercase tablename.
+    db_insert('TEST_UPPERCASE')
+      ->fields(['name' => 'secrets'])
+      ->execute();
+    $injection = "IS NOT NULL) UNION ALL SELECT name FROM {TEST_UPPERCASE} -- ";
+
+    try {
+      $result = db_select('test', 't')
+        ->fields('t', array('name'))
+        ->condition('name', 1, $injection)
+        ->execute();
+      $this->fail('Should not be able to attempt SQL injection via operator.');
+    }
+    catch (\ErrorException $e) {
+      $this->pass('SQL injection attempt via condition arguments should result in a database exception.');
+    }
+    restore_error_handler();
+  }
+
   /**
    * Tests numeric query parameter expansion in expressions.
    *

core/modules/system/tests/modules/database_test/database_test.install

@@ -290,5 +290,7 @@ function database_test_schema() {
     'primary key' => array('id'),
   );
 
+  $schema['TEST_UPPERCASE'] = $schema['test'];
+
   return $schema;
 }

core/tests/Drupal/Tests/Core/Database/ConditionTest.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Tests\Core\Database\ConditionTest.
+ */
+
+namespace Drupal\Tests\Core\Database;
+
+use Drupal\Core\Database\Connection;
+use Drupal\Core\Database\Query\Condition;
+use Drupal\Core\Database\Query\PlaceholderInterface;
+use Drupal\Tests\UnitTestCase;
+use Prophecy\Argument;
+
+/**
+ * @coversDefaultClass \Drupal\Core\Database\Query\Condition
+ *
+ * @group Database
+ */
+class ConditionTest extends UnitTestCase {
+
+  /**
+   * @covers ::compile
+   */
+  public function testSimpleCondition() {
+    $connection = $this->prophesize(Connection::class);
+    $connection->escapeField('name')->will(function ($args) {
+      return preg_replace('/[^A-Za-z0-9_.]+/', '', $args[0]);
+    });
+    $connection->mapConditionOperator('=')->willReturn(['operator' => '=']);
+    $connection = $connection->reveal();
+
+    $query_placeholder = $this->prophesize(PlaceholderInterface::class);
+
+    $counter = 0;
+    $query_placeholder->nextPlaceholder()->will(function() use (&$counter) {
+      return $counter++;
+    });
+    $query_placeholder->uniqueIdentifier()->willReturn(4);
+    $query_placeholder = $query_placeholder->reveal();
+
+    $condition = new Condition('AND');
+    $condition->condition('name', ['value']);
+    $condition->compile($connection, $query_placeholder);
+
+    $this->assertEquals(' (name = :db_condition_placeholder_0) ', $condition->__toString());
+    $this->assertEquals([':db_condition_placeholder_0' => 'value'], $condition->arguments());
+  }
+
+  /**
+   * @covers ::compile
+   *
+   * @dataProvider dataProviderTestCompileWithKnownOperators()
+   *
+   * @param string $expected
+   *   The expected generated SQL condition.
+   * @param string $field
+   *   The field to pass into the condition() method.
+   * @param mixed $value
+   *   The value to pass into the condition() method.
+   * @param string $operator
+   *   The operator to pass into the condition() method.
+   * @param mixed $expected_arguments
+   *   (optional) The expected set arguments.
+   */
+  public function testCompileWithKnownOperators($expected, $field, $value, $operator, $expected_arguments = NULL) {
+    $connection = $this->prophesize(Connection::class);
+    $connection->escapeField(Argument::any())->will(function ($args) {
+      return preg_replace('/[^A-Za-z0-9_.]+/', '', $args[0]);
+    });
+    $connection->mapConditionOperator(Argument::any())->willReturn(NULL);
+    $connection = $connection->reveal();
+
+    $query_placeholder = $this->prophesize(PlaceholderInterface::class);
+
+    $counter = 0;
+    $query_placeholder->nextPlaceholder()->will(function() use (&$counter) {
+      return $counter++;
+    });
+    $query_placeholder->uniqueIdentifier()->willReturn(4);
+    $query_placeholder = $query_placeholder->reveal();
+
+    $condition = new Condition('AND');
+    $condition->condition($field, $value, $operator);
+    $condition->compile($connection, $query_placeholder);
+
+    $this->assertEquals($expected, $condition->__toString());
+    if (isset($expected_arguments)) {
+      $this->assertEquals($expected_arguments, $condition->arguments());
+    }
+  }
+
+  /**
+   * Provides a list of known operations and the expected output.
+   *
+   * @return array
+   */
+  public function dataProviderTestCompileWithKnownOperators() {
+    // Below are a list of commented out test cases, which should work but
+    // aren't directly supported by core, but instead need manual handling with
+    // prefix/suffix at the moment.
+    $data = [];
+    $data[] = [' (name = :db_condition_placeholder_0) ', 'name', 'value', '='];
+    $data[] = [' (name != :db_condition_placeholder_0) ', 'name', 'value', '!='];
+    $data[] = [' (name <> :db_condition_placeholder_0) ', 'name', 'value', '<>'];
+    $data[] = [' (name >= :db_condition_placeholder_0) ', 'name', 'value', '>='];
+    $data[] = [' (name > :db_condition_placeholder_0) ', 'name', 'value', '>'];
+    $data[] = [' (name <= :db_condition_placeholder_0) ', 'name', 'value', '<='];
+    $data[] = [' (name < :db_condition_placeholder_0) ', 'name', 'value', '<'];
+    // $data[] = [' ( GREATEST (1, 2, 3) ) ', '', [1, 2, 3], 'GREATEST'];
+    $data[] = [' (name IN  (:db_condition_placeholder_0, :db_condition_placeholder_1, :db_condition_placeholder_2)) ', 'name', ['1', '2', '3'], 'IN'];
+    $data[] = [' (name NOT IN  (:db_condition_placeholder_0, :db_condition_placeholder_1, :db_condition_placeholder_2)) ', 'name', ['1', '2', '3'], 'NOT IN'];
+    // $data[] = [' ( INTERVAL (1, 2, 3) ) ', '', [1, 2, 3], 'INTERVAL'];
+    $data[] = [' (name IS NULL ) ', 'name', NULL, 'IS NULL'];
+    $data[] = [' (name IS NOT NULL ) ', 'name', NULL, 'IS NOT NULL'];
+    $data[] = [' (name IS :db_condition_placeholder_0) ', 'name', 'TRUE', 'IS'];
+    // $data[] = [' ( LEAST (1, 2, 3) ) ', '', [1, 2, 3], 'LEAST'];
+    $data[] = [" (name LIKE :db_condition_placeholder_0 ESCAPE '\\\\') ", 'name', '%muh%', 'LIKE', [':db_condition_placeholder_0' => '%muh%']];
+    $data[] = [" (name NOT LIKE :db_condition_placeholder_0 ESCAPE '\\\\') ", 'name', '%muh%', 'NOT LIKE', [':db_condition_placeholder_0' => '%muh%']];
+    $data[] = [" (name BETWEEN :db_condition_placeholder_0 AND :db_condition_placeholder_1) ", 'name', [1, 2], 'BETWEEN', [':db_condition_placeholder_0' => 1, ':db_condition_placeholder_1' => 2]];
+    // $data[] = [" (name NOT BETWEEN :db_condition_placeholder_0 AND :db_condition_placeholder_1) ", 'name', [1, 2], 'NOT BETWEEN', [':db_condition_placeholder_0' => 1, ':db_condition_placeholder_1' => 2]];
+    // $data[] = [' ( STRCMP (name, :db_condition_placeholder_0) ) ', '', ['test-string'], 'STRCMP', [':db_condition_placeholder_0' => 'test-string']];
+    // $data[] = [' (EXISTS ) ', '', NULL, 'EXISTS'];
+    // $data[] = [' (name NOT EXISTS ) ', 'name', NULL, 'NOT EXISTS'];
+
+    return $data;
+  }
+
+  /**
+   * @covers ::compile
+   *
+   * @expectedException \PHPUnit_Framework_Error
+   * @dataProvider providerTestCompileWithSqlInjectionForOperator
+   */
+  public function testCompileWithSqlInjectionForOperator($operator) {
+    $connection = $this->prophesize(Connection::class);
+    $connection->escapeField(Argument::any())->will(function ($args) {
+      return preg_replace('/[^A-Za-z0-9_.]+/', '', $args[0]);
+    });
+    $connection->mapConditionOperator(Argument::any())->willReturn(NULL);
+    $connection = $connection->reveal();
+
+    $query_placeholder = $this->prophesize(PlaceholderInterface::class);
+
+    $counter = 0;
+    $query_placeholder->nextPlaceholder()->will(function() use (&$counter) {
+      return $counter++;
+    });
+    $query_placeholder->uniqueIdentifier()->willReturn(4);
+    $query_placeholder = $query_placeholder->reveal();
+
+    $condition = new Condition('AND');
+    $condition->condition('name', 'value', $operator);
+    $condition->compile($connection, $query_placeholder);
+  }
+
+  public function providerTestCompileWithSqlInjectionForOperator() {
+    $data = [];
+    $data[] = ["IS NOT NULL) ;INSERT INTO {test} (name) VALUES ('test12345678'); -- "];
+    $data[] = ["IS NOT NULL) UNION ALL SELECT name, pass FROM {users_field_data} -- "];
+    $data[] = ["IS NOT NULL) UNION ALL SELECT name FROM {TEST_UPPERCASE} -- "];
+    $data[] = ["= 1 UNION ALL SELECT password FROM user WHERE uid ="];
+
+    return $data;
+  }
+
+}