Fixed possible SQL injection through ActiveRecord::findOne()

docs/guide/db-query-builder.md

@@ -227,9 +227,19 @@ $userQuery = (new Query())->select('id')->from('user');
 $query->where(['id' => $userQuery]);
 ```
 
-Using the Hash Format, Yii internally uses parameter binding so in contrast to the [string format](#string-format), here
-you do not have to add parameters manually.
+Using the Hash Format, Yii internally uses parameter binding for values, so in contrast to the [string format](#string-format),
+here you do not have to add parameters manually. However, Yii never escape the column name, so you should never
+embed variable as a column name, especially if the variable value came from end user inputs, because this will make
+your application subject to SQL injection attack. In case you need to get column name from user, read the [Filtering Data](output-data-widgets.md#filtering-data)
+guide article. For example the following code is vulnerable:
 
+```php
+// Vulnarable code:
+$column = $request->get('column');
+$value = $request->get('value);
+$query->where([$column => $value]);
+// $value will be encoded and is safe, but $column name is not!
+```
 
 #### Operator Format <span id="operator-format"></span>
 
@@ -306,8 +316,19 @@ the operator can be one of the following:
 - `>`, `<=`, or any other valid DB operator that takes two operands: the first operand must be a column name
   while the second operand a value. For example, `['>', 'age', 10]` will generate `age>10`.
 
-Using the Operator Format, Yii internally uses parameter binding so in contrast to the [string format](#string-format), here
-you do not have to add parameters manually.
+Using the Operator Format, Yii internally uses parameter binding for values, so in contrast to the [string format](#string-format),
+here you do not have to add parameters manually. However, Yii never escape the column name, so you should never
+embed variable as a column name, especially if the variable value came from end user inputs, because this will make
+your application subject to SQL injection attack. In case you need to get column name from user, read the [Filtering Data](output-data-widgets.md#filtering-data)
+guide article. For example the following code is vulnerable:
+
+```php
+// Vulnarable code:
+$column = $request->get('column');
+$value = $request->get('value);
+$query->where(['=', $column, $value]);
+// $value will be encoded and is safe, but $column name is not!
+```
 
 #### Object Format <span id="object-format"></span>
 

framework/CHANGELOG.md

@@ -5,6 +5,7 @@ Yii Framework 2 Change Log
 ------------------------
 
 - Bug #15878: Fixed migration with a comment containing an apostrophe (MarcoMoreno)
+- Bug #15688: (CVE-2018-7269): Fixed possible SQL injection through `yii\db\ActiveRecord::findOne()`, `::findAll()` (analitic1983, silverfire)
 
 
 2.0.14.2 March 13, 2018

framework/UPGRADE.md

@@ -99,6 +99,9 @@ Upgrade from Yii 2.0.13
   - Remove calls to `yii\BaseYii::powered()`.
   - If you are using XCache or Zend data cache, those are going away in 2.1 so you might want to start looking for an alternative.
   
+* When hash format condition is used in `yii\db\ActiveRecord::findOne()` and `findAll()`, the array keys (column names) are now limited
+  to the table column names for SQL DBMSs and to the following character set for NoSQL DBMSs: `A-Z0-9a-z$_-`.
+
 Upgrade from Yii 2.0.12
 -----------------------
 

framework/db/ActiveRecord.php

@@ -8,6 +8,7 @@
 namespace yii\db;
 
 use Yii;
+use yii\base\InvalidArgumentException;
 use yii\base\InvalidConfigException;
 use yii\helpers\ArrayHelper;
 use yii\helpers\Inflector;
@@ -171,6 +172,7 @@ class ActiveRecord extends BaseActiveRecord
     protected static function findByCondition($condition)
     {
         $query = static::find();
+        $condition = static::filterCondition($condition);
 
         if (!ArrayHelper::isAssociative($condition)) {
             // query by primary key
@@ -189,6 +191,33 @@ class ActiveRecord extends BaseActiveRecord
         return $query->andWhere($condition);
     }
 
+    /**
+     * Filters array condition before its assignation to a Query filter
+     *
+     * @param array|string|int $condition
+     * @return array|string|int
+     * @throws InvalidArgumentException in case array contains not safe values
+     * @since 2.0.14.2
+     * @internal
+     */
+    protected static function filterCondition($condition)
+    {
+        if (!is_array($condition)) {
+            return $condition;
+        }
+
+        $result = [];
+        $columnNames = static::getTableSchema()->getColumnNames();
+        foreach ($condition as $key => $item) {
+            if (is_string($key) && !in_array($key, $columnNames, true)) {
+                throw new InvalidArgumentException('Key "' . $key . '" is not a column name and can not be used as a filter');
+            }
+            $result[$key] = self::filterCondition($item);
+        }
+
+        return $result;
+    }
+
     /**
      * {@inheritdoc}
      */

framework/db/ActiveRecordInterface.php

@@ -161,8 +161,10 @@ interface ActiveRecordInterface extends StaticInstanceInterface
      *    corresponding record (or `null` if not found).
      *  - a non-associative array: query by a list of primary key values and return the
      *    first record (or `null` if not found).
-     *  - an associative array of name-value pairs: query by a set of attribute values and return a single record
-     *    matching all of them (or `null` if not found). Note that `['id' => 1, 2]` is treated as a non-associative array.
+     *  - an associative array of columnName-value pairs: query by a set of attribute values and return a single record
+     *    matching all of them (or `null` if not found). Note that `['id' => 1, 2]` is treated as a non-associative array,
+     *    column names are limited to current table columns or to a set of characters `A-Z0-9a-z$_-` when DBMS table schema
+     *    is not available (for example for NoSQL drivers).
      *
      * That this method will automatically call the `one()` method and return an [[ActiveRecordInterface|ActiveRecord]]
      * instance.
@@ -210,7 +212,8 @@ interface ActiveRecordInterface extends StaticInstanceInterface
      *    primary keys and not an empty `WHERE` condition.
      *  - an associative array of name-value pairs: query by a set of attribute values and return an array of records
      *    matching all of them (or an empty array if none was found). Note that `['id' => 1, 2]` is treated as
-     *    a non-associative array.
+     *    a non-associative array. Column names are limited to current table columns or to a set of characters
+     *    `A-Z0-9a-z$_-` when DBMS table schema is not available (for example for NoSQL drivers).
      *
      * This method will automatically call the `all()` method and return an array of [[ActiveRecordInterface|ActiveRecord]]
      * instances.

framework/db/BaseActiveRecord.php

@@ -132,6 +132,7 @@ abstract class BaseActiveRecord extends Model implements ActiveRecordInterface
     protected static function findByCondition($condition)
     {
         $query = static::find();
+        $condition = static::filterCondition($condition);
 
         if (!ArrayHelper::isAssociative($condition)) {
             // query by primary key
@@ -146,6 +147,31 @@ abstract class BaseActiveRecord extends Model implements ActiveRecordInterface
         return $query->andWhere($condition);
     }
 
+    /**
+     * Filters array condition before its assignation to a Query filter
+     *
+     * @param array|string|int $condition
+     * @return array|string|int
+     * @throws InvalidArgumentException in case array contains not safe values
+     * @since 2.0.14.2
+     * @internal
+     */
+    protected static function filterCondition($condition)
+    {
+        if (!is_array($condition)) {
+            return $condition;
+        }
+
+        $result = [];
+        foreach ($condition as $key => $item) {
+            if (is_string($key) && $key !== preg_replace('/([^\w\d_$-]|(--))/', '', $key)) {
+                throw new InvalidArgumentException('Key "' . $key . '" is not a column name and can not be used as a filter');
+            }
+            $result[$key] = self::filterCondition($item);
+        }
+        return $result;
+    }
+
     /**
      * Updates the whole table using the provided attribute values and conditions.
      *

tests/framework/db/ActiveRecordTest.php

@@ -8,6 +8,7 @@
 namespace yiiunit\framework\db;
 
 use yii\db\ActiveQuery;
+use yii\db\Query;
 use yii\helpers\ArrayHelper;
 use yiiunit\data\ar\ActiveRecord;
 use yiiunit\data\ar\Animal;
@@ -1647,6 +1648,37 @@ abstract class ActiveRecordTest extends DatabaseTestCase
         CustomerQuery::$joinWithProfile = false;
     }
 
+    public function illegalValuesForFindByCondition()
+    {
+        return [
+            [['id' => ['`id`=`id` and 1' => 1]]],
+            [['id' => [
+                'legal' => 1,
+                '`id`=`id` and 1' => 1,
+            ]]],
+            [['id' => [
+                'nested_illegal' => [
+                    'false or 1=' => 1
+                ]
+            ]]],
+            [
+                [['true--' => 1]]
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider illegalValuesForFindByCondition
+     */
+    public function testValueEscapingInFindByCondition($filterWithInjection)
+    {
+        $this->expectException('yii\base\InvalidArgumentException');
+        $this->expectExceptionMessageRegExp('/^Key "(.+)?" is not a column name and can not be used as a filter$/');
+        /** @var Query $query */
+        $query = $this->invokeMethod(new Customer(), 'findByCondition', $filterWithInjection);
+        Customer::getDb()->queryBuilder->build($query);
+    }
+
     /**
      * Ensure no ambiguous column error occurs on indexBy with JOIN.
      *