Make postConditions() less permissive.

We were notified by `ooooooo_q` that postConditions() is vulnerable to
SQL injection if used without SecurityComponent tampering prevention.

This change attempts to make postConditions() safer by exploding in
unsafe scenarios.
This commit is contained in:
mark_story 2017-12-10 21:44:47 -05:00
parent 10fcd7633d
commit f66dec8a96
2 changed files with 45 additions and 1 deletions

View file

@ -1018,7 +1018,12 @@ class Controller extends CakeObject implements CakeEventListener {
} }
/** /**
* Converts POST'ed form data to a model conditions array, suitable for use in a Model::find() call. * Converts POST'ed form data to a model conditions array.
*
* If combined with SecurityComponent these conditions could be suitable
* for use in a Model::find() call. Without SecurityComponent this method
* is vulnerable creating conditions containing SQL injection. While we
* attempt to raise exceptions.
* *
* @param array $data POST'ed data organized by model and field * @param array $data POST'ed data organized by model and field
* @param string|array $op A string containing an SQL comparison operator, or an array matching operators * @param string|array $op A string containing an SQL comparison operator, or an array matching operators
@ -1046,6 +1051,9 @@ class Controller extends CakeObject implements CakeEventListener {
$arrayOp = is_array($op); $arrayOp = is_array($op);
foreach ($data as $model => $fields) { foreach ($data as $model => $fields) {
foreach ($fields as $field => $value) { foreach ($fields as $field => $value) {
if (preg_match('#[!=><~\&\|\)\(]#', $field)) {
throw new RuntimeException("Unsafe operator found in {$model}.{$field}");
}
$key = $model . '.' . $field; $key = $model . '.' . $field;
$fieldOp = $op; $fieldOp = $op;
if ($arrayOp) { if ($arrayOp) {

View file

@ -1177,6 +1177,42 @@ class ControllerTest extends CakeTestCase {
$this->assertSame($expected, $result); $this->assertSame($expected, $result);
} }
/**
* data provider for dangerous post conditions.
*
* @return array
*/
public function dangerousPostConditionsProvider() {
return array(
array(
array('Model' => array('field !=' => 1))
),
array(
array('Model' => array('field AND 1=1 OR' => 'thing'))
),
array(
array('Model' => array('field >' => 1))
),
array(
array('Model' => array('field OR RAND()' => 1))
),
);
}
/**
* test postConditions raising an exception on unsafe keys.
*
* @expectedException RuntimeException
* @dataProvider dangerousPostConditionsProvider
* @return void
*/
public function testPostConditionsDangerous($data) {
$request = new CakeRequest('controller_posts/index');
$Controller = new Controller($request);
$Controller->postConditions($data);
}
/** /**
* testControllerHttpCodes method * testControllerHttpCodes method
* *