Expand checks and sanitization on db queries

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);
   }
 
 }

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,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',
@@ -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 [];
+    }
+  }
+
   /**
    *
    */