Skip to content

Commit

Permalink
Expand checks and sanitization on db queries (#4227)
Browse files Browse the repository at this point in the history
  • Loading branch information
@janette
janette authored Jul 17, 2024
1 parent 228bdb4 commit 933e64c
Show file tree
Hide file tree
Showing 3 changed files with 225 additions and 28 deletions.
49 changes: 37 additions & 12 deletions modules/common/src/Storage/SelectFactory.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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;

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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.
Expand All @@ -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);

Expand Down Expand Up @@ -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);
}

}
174 changes: 168 additions & 6 deletions modules/common/tests/src/Unit/Storage/QueryDataProvider.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand All @@ -31,20 +31,25 @@ public function getAllData($return): array {
'badPropertyQuery',
'unsafePropertyQuery',
'expressionQuery',
'expressionQueryWithInjection',
'nestedExpressionQuery',
'badExpressionOperandQuery',
'conditionQuery',
'likeConditionQuery',
'containsConditionQuery',
'startsWithConditionQuery',
'matchConditionQuery',
'matchConditionQueryWithSqlInjection',
'matchConditionQueryNested',
'arrayConditionQuery',
'nestedConditionGroupQuery',
'sortQuery',
'badSortQuery',
'offsetQuery',
'limitOffsetQuery',
'joinsQuery',
'joinsQueryWithInjection',
'joinsQueryWithInvalidOperator',
'joinWithPropertiesFromBothQuery',
'countQuery',
'groupByQuery',
Expand Down Expand Up @@ -95,10 +100,10 @@ public static function propertiesQuery($return) {

case self::EXCEPTION:
return '';

case self::VALUES:
return [];

}
}

Expand All @@ -117,7 +122,7 @@ public static function badPropertyQuery($return) {

case self::EXCEPTION:
return "Bad query property";

case self::VALUES:
return [];
}
Expand Down Expand Up @@ -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 [];
}
}


/**
*
*/
Expand Down Expand Up @@ -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'];
}
}

/**
*
Expand Down Expand Up @@ -486,7 +585,7 @@ public static function nestedConditionGroupQuery($return) {
case self::VALUES:
return ['value1', 'value2', 'value3'];
}

}

/**
Expand Down Expand Up @@ -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 [];
}
}

/**
*
*/
Expand Down
Loading

0 comments on commit 933e64c

Please sign in to comment.