diff --git a/docs/guide/db-query-builder.md b/docs/guide/db-query-builder.md index cd4ee71d59..4162749fe4 100644 --- a/docs/guide/db-query-builder.md +++ b/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 @@ -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 diff --git a/framework/CHANGELOG.md b/framework/CHANGELOG.md index f2b96e8060..edeb60601b 100644 --- a/framework/CHANGELOG.md +++ b/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 diff --git a/framework/UPGRADE.md b/framework/UPGRADE.md index 507754fa92..052bf46afe 100644 --- a/framework/UPGRADE.md +++ b/framework/UPGRADE.md @@ -98,6 +98,9 @@ Upgrade from Yii 2.0.13 - Replace calls to `Yii::trace()` with `Yii::debug()`. - 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 ----------------------- diff --git a/framework/db/ActiveRecord.php b/framework/db/ActiveRecord.php index babff644d1..d0ef453c39 100644 --- a/framework/db/ActiveRecord.php +++ b/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} */ diff --git a/framework/db/ActiveRecordInterface.php b/framework/db/ActiveRecordInterface.php index dbe1828bd1..c0b35bec19 100644 --- a/framework/db/ActiveRecordInterface.php +++ b/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. diff --git a/framework/db/BaseActiveRecord.php b/framework/db/BaseActiveRecord.php index 3a5e9e6a05..854396ddd3 100644 --- a/framework/db/BaseActiveRecord.php +++ b/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. * diff --git a/tests/framework/db/ActiveRecordTest.php b/tests/framework/db/ActiveRecordTest.php index d58740d8c4..976a09acfb 100644 --- a/tests/framework/db/ActiveRecordTest.php +++ b/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. *