Merge pull request #11526 from cakephp/post-conditions

Make postConditions() less permissive.
cakephp · Dec 15, 2017 · 3bf93b7 · 3bf93b7
2 parents bdaff46 + 340059b
commit 3bf93b7
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 1 deletion.
diff --git a/lib/Cake/Controller/Controller.php b/lib/Cake/Controller/Controller.php
@@ -1018,7 +1018,12 @@ public function flash($message, $url, $pause = 1, $layout = 'flash') {
 	}
 
 /**
- * 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 @@ public function flash($message, $url, $pause = 1, $layout = 'flash') {
  *        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 @@ public function postConditions($data = array(), $op = null, $bool = 'AND', $excl
 			$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
@@ -1177,6 +1177,48 @@ public function testPostConditions() {
 		$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
  *