diff --git a/lib/Cake/Controller/Controller.php b/lib/Cake/Controller/Controller.php index 41c848a7d..e9d5c6657 100644 --- a/lib/Cake/Controller/Controller.php +++ b/lib/Cake/Controller/Controller.php @@ -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 string|array $op A string containing an SQL comparison operator, or an array matching operators @@ -1028,6 +1033,7 @@ class Controller extends CakeObject implements CakeEventListener { * included in the returned conditions * @return array|null An array of model conditions * @deprecated 3.0.0 Will be removed in 3.0. + * @throws RuntimeException when unsafe operators are found. */ public function postConditions($data = array(), $op = null, $bool = 'AND', $exclusive = false) { if (!is_array($data) || empty($data)) { @@ -1043,9 +1049,16 @@ class Controller extends CakeObject implements CakeEventListener { $op = ''; } + $allowedChars = '#[^a-zA-Z0-9_ ]#'; $arrayOp = is_array($op); foreach ($data as $model => $fields) { + if (preg_match($allowedChars, $model)) { + throw new RuntimeException("Unsafe operator found in {$model}"); + } foreach ($fields as $field => $value) { + if (preg_match($allowedChars, $field)) { + throw new RuntimeException("Unsafe operator found in {$model}.{$field}"); + } $key = $model . '.' . $field; $fieldOp = $op; if ($arrayOp) { diff --git a/lib/Cake/Test/Case/Controller/ControllerTest.php b/lib/Cake/Test/Case/Controller/ControllerTest.php index ef157ddd1..686520df6 100644 --- a/lib/Cake/Test/Case/Controller/ControllerTest.php +++ b/lib/Cake/Test/Case/Controller/ControllerTest.php @@ -1177,6 +1177,48 @@ class ControllerTest extends CakeTestCase { $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)) + ), + array( + array('Posts' => array('id IS NULL union all select posts.* from posts where id; --' => 1)) + ), + array( + array('Post.id IS NULL; --' => array('id' => 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 *