From 933e64cf5a89713750d2c680490cf10667741173 Mon Sep 17 00:00:00 2001 From: Janette Day Date: Wed, 17 Jul 2024 14:36:52 -0500 Subject: [PATCH] Expand checks and sanitization on db queries (#4227) --- modules/common/src/Storage/SelectFactory.php | 49 +++-- .../src/Unit/Storage/QueryDataProvider.php | 174 +++++++++++++++++- .../src/Unit/Storage/SelectFactoryTest.php | 30 ++- 3 files changed, 225 insertions(+), 28 deletions(-) diff --git a/modules/common/src/Storage/SelectFactory.php b/modules/common/src/Storage/SelectFactory.php index 3afef0b91e..d79d8000c8 100644 --- a/modules/common/src/Storage/SelectFactory.php +++ b/modules/common/src/Storage/SelectFactory.php @@ -2,8 +2,8 @@ namespace Drupal\common\Storage; -use Drupal\Core\Database\Query\Select; use Drupal\Core\Database\Connection; +use Drupal\Core\Database\Query\Select; /** * Class to convert a DKAN Query object into a Drupal DB API Select Object. @@ -13,7 +13,7 @@ class SelectFactory { /** * A database table object, which includes a database connection. * - * @var Drupal\Core\Database\Connection + * @var \Drupal\Core\Database\Connection */ private $connection; @@ -54,7 +54,7 @@ public function __construct(Connection $connection, string $alias = 't') { /** * Create Drupal select object. * - * @param Drupal\common\Storage\Query $query + * @param \Drupal\common\Storage\Query $query * DKAN Query object. */ public function create(Query $query): Select { @@ -141,17 +141,19 @@ private function normalizeProperty(mixed $property): object { if (is_string($property) && self::safeProperty($property)) { return (object) [ "collection" => $this->alias, - "property" => $property, + "property" => $this->dbQuery->escapeField($property), "alias" => NULL, ]; } if (!is_object($property) || !isset($property->property) || !isset($property->collection)) { throw new \Exception("Bad query property: " . print_r($property, 1)); } + // Throw exception if obviously unsafe property name. self::safeProperty($property->property); - if (!isset($property->alias)) { - $property->alias = NULL; - } + // Sanitize the property name. + $property->property = $this->dbQuery->escapeField($property->property); + $property->alias = isset($property->alias) ? $this->connection->escapeAlias($property->alias) : NULL; + $property->collection = isset($property->collection) ? $this->connection->escapeTable($property->collection) : NULL; return $property; } @@ -293,7 +295,10 @@ private function addCondition($statementObj, $condition) { /** * Add a custom where condition in the case of a fulltext match operator. * - * Currently, only BOOLEAN MODE Mysql fulltext searches supported. + * Currently, only BOOLEAN MODE Mysql fulltext searches supported. Note + * that an explicit resource in the condition will be ignored to prevent + * possible injection attacks, so match conditions are unlikely to work + * well in combination with joins. * * @param \Drupal\Core\Database\Query\Select|\Drupal\Core\Database\Query\Condition $statementObj * Drupal DB API select object or condition object. @@ -304,9 +309,7 @@ private function addMatchCondition($statementObj, $condition) { $properties = explode(',', $condition->property); $fields = []; foreach ($properties as $property) { - $fields[] = ($condition->collection ?? $this->alias) - . '.' - . $property; + $fields[] = $this->alias . '.' . $this->dbQuery->escapeField($property); } $fields_list = implode(',', $fields); @@ -452,8 +455,30 @@ private function conditionString($condition): string { throw new \Exception("Invalid join condition; collection must be specified."); } + self::safeJoinOperator($condition->operator); + $collection = $this->connection->escapeTable($condition->collection); + $property = $this->dbQuery->escapeField($condition->property); $value = $this->propertyToString($condition->value); - return "{$condition->collection}.{$condition->property} $condition->operator $value"; + return "$collection.$property $condition->operator $value"; + } + + /** + * Check if a join condition operator is valid. + * + * @param string $operator + * The operator to check. + * + * @return bool + * True if valid. + * + * @throws \Exception + * If the operator is invalid. + */ + public static function safeJoinOperator(string $operator): bool { + if (in_array($operator, ['=', '!=', '<>', '>', '>=', '<', '<=', 'LIKE'])) { + return TRUE; + } + throw new \Exception('Invalid join operator: ' . $operator); } } diff --git a/modules/common/tests/src/Unit/Storage/QueryDataProvider.php b/modules/common/tests/src/Unit/Storage/QueryDataProvider.php index 3ad31f6608..8c217f5e30 100644 --- a/modules/common/tests/src/Unit/Storage/QueryDataProvider.php +++ b/modules/common/tests/src/Unit/Storage/QueryDataProvider.php @@ -11,8 +11,8 @@ * messages. Methods are public static so that other tests can easily pull them * in individually. * - * @see Drupal\Tests\common\Storage\SelectFactoryTest - * @see Drupal\Tests\datastore\Service\QueryTest + * @see \Drupal\Tests\common\Storage\SelectFactoryTest + * @see \Drupal\Tests\datastore\Service\QueryTest */ class QueryDataProvider { @@ -31,6 +31,7 @@ public function getAllData($return): array { 'badPropertyQuery', 'unsafePropertyQuery', 'expressionQuery', + 'expressionQueryWithInjection', 'nestedExpressionQuery', 'badExpressionOperandQuery', 'conditionQuery', @@ -38,6 +39,8 @@ public function getAllData($return): array { 'containsConditionQuery', 'startsWithConditionQuery', 'matchConditionQuery', + 'matchConditionQueryWithSqlInjection', + 'matchConditionQueryNested', 'arrayConditionQuery', 'nestedConditionGroupQuery', 'sortQuery', @@ -45,6 +48,8 @@ public function getAllData($return): array { 'offsetQuery', 'limitOffsetQuery', 'joinsQuery', + 'joinsQueryWithInjection', + 'joinsQueryWithInvalidOperator', 'joinWithPropertiesFromBothQuery', 'countQuery', 'groupByQuery', @@ -95,10 +100,10 @@ public static function propertiesQuery($return) { case self::EXCEPTION: return ''; - + case self::VALUES: return []; - + } } @@ -117,7 +122,7 @@ public static function badPropertyQuery($return) { case self::EXCEPTION: return "Bad query property"; - + case self::VALUES: return []; } @@ -198,6 +203,36 @@ public static function expressionQuery($return) { } } + /** + * + */ + public static function expressionQueryWithInjection($return) { + switch ($return) { + case self::QUERY_OBJECT: + $query = new Query(); + $query->properties = [ + (object) [ + "alias" => "add_one", + "expression" => (object) [ + "operator" => "+", + "operands" => ["field1 + 1) AS add_one, USER() as user, (1 ", 1], + ], + ], + ]; + return $query; + + case self::SQL: + return "SELECT (t.field11ASadd_oneUSERasuser1 + 1) AS add_one"; + + case self::EXCEPTION: + return ''; + + case self::VALUES: + return []; + } + } + + /** * */ @@ -410,6 +445,70 @@ public static function matchConditionQuery($return) { } } + /** + * SQL strings with user input, ensure no injection possible. + */ + public static function matchConditionQueryWithSqlInjection($return) { + switch ($return) { + case self::QUERY_OBJECT: + $query = new Query(); + $query->conditions = [ + (object) [ + "collection" => "t", + "property" => "field1) AGAINST (:words0 IN BOOLEAN MODE))) AND OTHER SQL INJECTION", + "value" => "value", + "operator" => "match", + ], + ]; + return $query; + + case self::SQL: + return "WHERE (MATCH(t.field1AGAINSTwords0INBOOLEANMODEANDOTHERSQLINJECTION)"; + + case self::EXCEPTION: + return ''; + + case self::VALUES: + return ['value']; + } + } + + /** + * SQL strings with user input, ensure no injection possible. + */ + public static function matchConditionQueryNested($return) { + switch ($return) { + case self::QUERY_OBJECT: + $query = new Query(); + $query->conditions = [ + (object) [ + "groupOperator" => "or", + "conditions" => [ + (object) [ + "property" => "field1", + "value" => "value1", + "operator" => "match", + ], + (object) [ + "property" => "field2", + "value" => "value2", + "operator" => "match", + ], + ], + ], + ]; + return $query; + + case self::SQL: + return "WHERE ((MATCH(t.field1) AGAINST (:words0 IN BOOLEAN MODE))) OR ((MATCH(t.field2) AGAINST (:words1 IN BOOLEAN MODE)))"; + + case self::EXCEPTION: + return ''; + + case self::VALUES: + return ['value1', 'value2']; + } + } /** * @@ -486,7 +585,7 @@ public static function nestedConditionGroupQuery($return) { case self::VALUES: return ['value1', 'value2', 'value3']; } - + } /** @@ -624,6 +723,69 @@ public static function joinsQuery($return) { } } + public static function joinsQueryWithInjection($return) { + switch ($return) { + case self::QUERY_OBJECT: + $query = new Query(); + $query->joins = [ + (object) [ + "collection" => "table2(this)", + "alias" => "l(antyt)", + "condition" => (object) [ + "collection" => "t", + "property" => "field1(ON WHAT)", + "value" => (object) [ + "collection" => "l((((", + "property" => "field1(TTT)", + ], + ], + ], + ]; + return $query; + + case self::SQL: + return "SELECT t.*, lantyt.* FROM {table} t INNER JOIN {table2this} lantyt ON t.field1ONWHAT = l.field1TTT"; + + case self::EXCEPTION: + return ''; + + case self::VALUES: + return []; + } + } + + public static function joinsQueryWithInvalidOperator($return) { + switch ($return) { + case self::QUERY_OBJECT: + $query = new Query(); + $query->joins = [ + (object) [ + "collection" => "table2", + "alias" => "l", + "condition" => (object) [ + "collection" => "t", + "property" => "field1", + "value" => (object) [ + "collection" => "l", + "property" => "field1", + ], + "operator" => ") OR 1=1 --", + ], + ], + ]; + return $query; + + case self::EXCEPTION: + return 'Invalid join operator'; + + case self::SQL: + return "SELECT t.field2 AS field2, l.field3 AS field3 FROM {table} t INNER JOIN {table2} l"; + + case self::VALUES: + return []; + } + } + /** * */ diff --git a/modules/common/tests/src/Unit/Storage/SelectFactoryTest.php b/modules/common/tests/src/Unit/Storage/SelectFactoryTest.php index 45b4da1392..1cc013d61d 100644 --- a/modules/common/tests/src/Unit/Storage/SelectFactoryTest.php +++ b/modules/common/tests/src/Unit/Storage/SelectFactoryTest.php @@ -10,7 +10,7 @@ use Drupal\Tests\common\Unit\Connection; /** - * + * @coversDefaultClass \Drupal\common\Storage\SelectFactory */ class SelectFactoryTest extends TestCase { private $query; @@ -22,6 +22,12 @@ class SelectFactoryTest extends TestCase { */ private $selectFactory; + protected function setUp(): void { + parent::setUp(); + $this->query = new Query(); + $this->selectFactory = $this->getSelectFactory(); + } + /** * @test * @@ -41,7 +47,7 @@ public function testQuery(Query $query, string $sql, string $message, array $val } } } - + /** * Test two variations of Query::testConditionByIsEqualTo() */ @@ -77,6 +83,18 @@ public function testAddDateExpressions() { $this->assertStringContainsString("DATE_FORMAT(date, '%m/%d/%Y') AS date", $this->selectToString($db_query)); } + /** + * @covers ::safeJoinOperator + */ + public function testSafeJoinOperator() { + foreach (['=', '!=', '<>', '>', '>=', '<', '<=', 'LIKE'] as $operator) { + $this->assertTrue(SelectFactory::safeJoinOperator($operator)); + } + $this->expectException(\Exception::class); + $this->expectExceptionMessage('Invalid join operator: foo'); + SelectFactory::safeJoinOperator('foo'); + } + /** * */ @@ -112,12 +130,4 @@ private function queryDebug() { print "\n" . $this->selectToString($this->selectFactory->create($this->query)); } - /** - * - */ - public function setUp():void { - $this->query = new Query(); - $this->selectFactory = $this->getSelectFactory(); - } - }