diff --git a/core/lib/Drupal/Core/Database/Query/Condition.php b/core/lib/Drupal/Core/Database/Query/Condition.php index 4bb1d09ae23f34efc79a87a4c3f5a1ebd39a0c1c..2b9574e3d422355912865208fd29bf8fc3cd138e 100644 --- a/core/lib/Drupal/Core/Database/Query/Condition.php +++ b/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']); diff --git a/core/modules/system/src/Tests/Database/DatabaseTestBase.php b/core/modules/system/src/Tests/Database/DatabaseTestBase.php index dd674b6d4d3623350877ad14837dbabdb2834f09..a6b76110d1a5c76e3dbb21ced35b14f2337e4039 100644 --- a/core/modules/system/src/Tests/Database/DatabaseTestBase.php +++ b/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(); } diff --git a/core/modules/system/src/Tests/Database/QueryTest.php b/core/modules/system/src/Tests/Database/QueryTest.php index 4e9d84b157922abe223b1861a4e7720aa4945678..dd613c593c2a1b7cc3e2819cd01609e6a8ec4998 100644 --- a/core/modules/system/src/Tests/Database/QueryTest.php +++ b/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. * diff --git a/core/modules/system/tests/modules/database_test/database_test.install b/core/modules/system/tests/modules/database_test/database_test.install index 3f77a7561ee4aab0d9434436e4dd44ed493dc0a7..f6b4f68d82cc6d31c54f8f1a7a13c369008f36bd 100644 --- a/core/modules/system/tests/modules/database_test/database_test.install +++ b/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; } diff --git a/core/tests/Drupal/Tests/Core/Database/ConditionTest.php b/core/tests/Drupal/Tests/Core/Database/ConditionTest.php new file mode 100644 index 0000000000000000000000000000000000000000..c5e7e7ec589414376c022994f2c1dabdaf8a27fc --- /dev/null +++ b/core/tests/Drupal/Tests/Core/Database/ConditionTest.php @@ -0,0 +1,168 @@ +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; + } + +}