From f66dec8a96999a5ef042d9a72dec6784ac56fcc6 Mon Sep 17 00:00:00 2001 From: mark_story Date: Sun, 10 Dec 2017 21:44:47 -0500 Subject: [PATCH 1/3] 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. --- lib/Cake/Controller/Controller.php | 10 +++++- .../Test/Case/Controller/ControllerTest.php | 36 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/lib/Cake/Controller/Controller.php b/lib/Cake/Controller/Controller.php index 41c848a7d..7dd2a4ae1 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 @@ -1046,6 +1051,9 @@ class Controller extends CakeObject implements CakeEventListener { $arrayOp = is_array($op); foreach ($data as $model => $fields) { foreach ($fields as $field => $value) { + if (preg_match('#[!=><~\&\|\)\(]#', $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..26e3c6b16 100644 --- a/lib/Cake/Test/Case/Controller/ControllerTest.php +++ b/lib/Cake/Test/Case/Controller/ControllerTest.php @@ -1177,6 +1177,42 @@ 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)) + ), + ); + } + +/** + * 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 * From a9618f67f747f5ca9b3892509bd60b1d8f8eef63 Mon Sep 17 00:00:00 2001 From: mark_story Date: Mon, 11 Dec 2017 13:46:18 -0500 Subject: [PATCH 2/3] Use a permitted list instead of a ban list. This should be safer as we are more confident on what is coming in. --- lib/Cake/Controller/Controller.php | 3 ++- lib/Cake/Test/Case/Controller/ControllerTest.php | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/Cake/Controller/Controller.php b/lib/Cake/Controller/Controller.php index 7dd2a4ae1..05878f8ac 100644 --- a/lib/Cake/Controller/Controller.php +++ b/lib/Cake/Controller/Controller.php @@ -1033,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)) { @@ -1051,7 +1052,7 @@ class Controller extends CakeObject implements CakeEventListener { $arrayOp = is_array($op); foreach ($data as $model => $fields) { foreach ($fields as $field => $value) { - if (preg_match('#[!=><~\&\|\)\(]#', $field)) { + if (preg_match('#[^a-zA-Z0-9_ ]#', $field)) { throw new RuntimeException("Unsafe operator found in {$model}.{$field}"); } $key = $model . '.' . $field; diff --git a/lib/Cake/Test/Case/Controller/ControllerTest.php b/lib/Cake/Test/Case/Controller/ControllerTest.php index 26e3c6b16..716d9019f 100644 --- a/lib/Cake/Test/Case/Controller/ControllerTest.php +++ b/lib/Cake/Test/Case/Controller/ControllerTest.php @@ -1182,7 +1182,7 @@ class ControllerTest extends CakeTestCase { * * @return array */ - public function dangerousPostConditionsProvider() { + public function dangerousPostConditionsProvider() { return array( array( array('Model' => array('field !=' => 1)) @@ -1196,6 +1196,9 @@ class ControllerTest extends CakeTestCase { array( array('Model' => array('field OR RAND()' => 1)) ), + array( + array('Posts' => array('id IS NULL union all select posts.* from posts where id; --' => 1)) + ), ); } From 340059be151eae8eec32c3a70cd50a5c85c1c84e Mon Sep 17 00:00:00 2001 From: mark_story Date: Tue, 12 Dec 2017 13:45:33 -0500 Subject: [PATCH 3/3] Check model names for bad characters as well. --- lib/Cake/Controller/Controller.php | 6 +++++- lib/Cake/Test/Case/Controller/ControllerTest.php | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/Cake/Controller/Controller.php b/lib/Cake/Controller/Controller.php index 05878f8ac..e9d5c6657 100644 --- a/lib/Cake/Controller/Controller.php +++ b/lib/Cake/Controller/Controller.php @@ -1049,10 +1049,14 @@ 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('#[^a-zA-Z0-9_ ]#', $field)) { + if (preg_match($allowedChars, $field)) { throw new RuntimeException("Unsafe operator found in {$model}.{$field}"); } $key = $model . '.' . $field; diff --git a/lib/Cake/Test/Case/Controller/ControllerTest.php b/lib/Cake/Test/Case/Controller/ControllerTest.php index 716d9019f..686520df6 100644 --- a/lib/Cake/Test/Case/Controller/ControllerTest.php +++ b/lib/Cake/Test/Case/Controller/ControllerTest.php @@ -1199,6 +1199,9 @@ class ControllerTest extends CakeTestCase { 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)) + ), ); }