summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorwebchick2015-09-12 15:56:01 (GMT)
committerwebchick2015-09-12 15:56:01 (GMT)
commit5a46f6aed438bcd1fedda1bb4e539de9c81bd130 (patch)
tree71636a7987ee86c30493b479adb6ade12c106696
parentd24a890690e9a852c3818380b0fb6e69ccc60af1 (diff)
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
-rw-r--r--core/lib/Drupal/Core/Database/Query/Condition.php19
-rw-r--r--core/modules/system/src/Tests/Database/DatabaseTestBase.php1
-rw-r--r--core/modules/system/src/Tests/Database/QueryTest.php68
-rw-r--r--core/modules/system/tests/modules/database_test/database_test.install2
-rw-r--r--core/tests/Drupal/Tests/Core/Database/ConditionTest.php168
5 files changed, 258 insertions, 0 deletions
diff --git a/core/lib/Drupal/Core/Database/Query/Condition.php b/core/lib/Drupal/Core/Database/Query/Condition.php
index 4bb1d09..2b9574e 100644
--- a/core/lib/Drupal/Core/Database/Query/Condition.php
+++ b/core/lib/Drupal/Core/Database/Query/Condition.php
@@ -188,6 +188,25 @@ class Condition implements ConditionInterface, \Countable {
'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 dd674b6..a6b7611 100644
--- a/core/modules/system/src/Tests/Database/DatabaseTestBase.php
+++ b/core/modules/system/src/Tests/Database/DatabaseTestBase.php
@@ -31,6 +31,7 @@ abstract class DatabaseTestBase extends KernelTestBase {
'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 4e9d84b..dd613c5 100644
--- a/core/modules/system/src/Tests/Database/QueryTest.php
+++ b/core/modules/system/src/Tests/Database/QueryTest.php
@@ -67,6 +67,74 @@ class QueryTest extends DatabaseTestBase {
}
/**
+ * 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.
*
* @see \Drupal\Core\Database\Driver\sqlite\Statement::getStatement()
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 3f77a75..f6b4f68 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 0000000..c5e7e7e
--- /dev/null
+++ b/core/tests/Drupal/Tests/Core/Database/ConditionTest.php
@@ -0,0 +1,168 @@
+<?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;
+ }
+
+}