Backport f23d811 to 1.3

Use the form action URL in generated form hashes. By including the URL in generated hash for secured forms we prevent a class of abuse where a user uses one secured form to post into a controller action the form was not originally intended for. These cross action requests could potentially violate developer's mental model of how SecurityComponent works and produce unexpected/undesirable outcomes. Due to the limitations of 1.3, only the *path* is used as a hash input. This is slightly less hard than the 2.x implementation but should be enough to prevent most forms of misuse. Thanks to Kurita Takashi for pointing this issue out, and suggesting a fix.
cakephp · Apr 26, 2014 · db6cfe4 · db6cfe4
1 parent 2d4270f
commit db6cfe4
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 8 deletions.
diff --git a/cake/libs/controller/components/security.php b/cake/libs/controller/components/security.php
@@ -663,7 +663,8 @@ function _validatePost(&$controller) {
 		ksort($lockedFields, SORT_STRING);
 
 		$fieldList += $lockedFields;
-		$check = Security::hash(serialize($fieldList) . Configure::read('Security.salt'));
+		$url = $controller->here;
+		$check = Security::hash($url . serialize($fieldList) . Configure::read('Security.salt'));
 		return ($token === $check);
 	}
 

diff --git a/cake/libs/view/helpers/form.php b/cake/libs/view/helpers/form.php
@@ -81,7 +81,6 @@ class FormHelper extends AppHelper {
  */
 	var $defaultModel = null;
 
-
 /**
  * Persistent default options used by input(). Set by FormHelper::create().
  *
@@ -90,6 +89,14 @@ class FormHelper extends AppHelper {
  */
 	var $_inputDefaults = array();
 
+/**
+ * The action attribute value of the last created form.
+ * Used to make form/request specific hashes for SecurityComponent.
+ *
+ * @var string
+ */
+	var $_lastAction = '';
+
 /**
  * Introspects model information and extracts information related
  * to validation, field length and field type. Appends information into
@@ -322,6 +329,7 @@ function create($model = null, $options = array()) {
 			$append = sprintf($this->Html->tags['block'], ' style="display:none;"', $append);
 		}
 
+		$this->_lastAction = parse_url($action, PHP_URL_PATH);
 		$this->setEntity($model . '.', true);
 		$attributes = sprintf('action="%s" ', $action) . $this->_parseAttributes($htmlAttributes, null, '');
 		return sprintf($this->Html->tags['form'], $attributes) . $append;
@@ -403,7 +411,11 @@ function secure($fields = array()) {
 		ksort($locked, SORT_STRING);
 		$fields += $locked;
 
-		$fields = Security::hash(serialize($fields) . Configure::read('Security.salt'));
+		$fields = Security::hash(
+			$this->_lastAction .
+			serialize($fields) .
+			Configure::read('Security.salt')
+		);
 		$locked = implode(array_keys($locked), '|');
 
 		$out = $this->hidden('_Token.fields', array(

diff --git a/cake/tests/cases/libs/controller/components/security.test.php b/cake/tests/cases/libs/controller/components/security.test.php
@@ -582,6 +582,27 @@ function testValidatePost() {
 		$this->assertTrue($this->Controller->Security->validatePost($this->Controller));
 	}
 
+/**
+ * Test that the controller->here is part of the hash.
+ *
+ * @access public
+ * @return void
+ */
+	function testValidatePostUsesControllerHere() {
+		$this->Controller->Security->startup($this->Controller);
+		$key = $this->Controller->params['_Token']['key'];
+		$fields = 'a5475372b40f6e3ccbf9f8af191f20e1642fd877%3AModel.valid';
+
+		$this->Controller->data = array(
+			'Model' => array('username' => 'nate', 'password' => 'foo', 'valid' => '0'),
+			'_Token' => compact('key', 'fields')
+		);
+		$this->assertTrue($this->Controller->Security->validatePost($this->Controller));
+
+		$this->Controller->here = '/cake_13/tasks';
+		$this->assertFalse($this->Controller->Security->validatePost($this->Controller));
+	}
+
 /**
  * Test that validatePost fails if you are missing the session information.
  *

diff --git a/cake/tests/cases/libs/view/helpers/form.test.php b/cake/tests/cases/libs/view/helpers/form.test.php
@@ -984,7 +984,7 @@ function testFormSecurityMultipleInputFields() {
 
 		$result = $this->Form->secure($this->Form->fields);
 
-		$hash = 'c9118120e680a7201b543f562e5301006ccfcbe2%3AAddresses.0.id%7CAddresses.1.id';
+		$hash = 'f88bdc351fa388569210cf55ba6b8879763fca13%3AAddresses.0.id%7CAddresses.1.id';
 
 		$expected = array(
 			'div' => array('style' => 'display:none;'),
@@ -1041,7 +1041,7 @@ function testFormSecurityMultipleInputDisabledFields() {
 		$this->Form->input('Addresses.1.phone');
 
 		$result = $this->Form->secure($this->Form->fields);
-		$hash = '774df31936dc850b7d8a5277dc0b890123788b09%3AAddresses.0.id%7CAddresses.1.id';
+		$hash = 'cdf15a1cf5192aaa25a2ce85957e9f27c0a1a006%3AAddresses.0.id%7CAddresses.1.id';
 
 		$expected = array(
 			'div' => array('style' => 'display:none;'),
@@ -1085,7 +1085,7 @@ function testFormSecurityInputDisabledFields() {
 
 		$result = $this->Form->secure($expected);
 
-		$hash = '449b7e889128e8e52c5e81d19df68f5346571492%3AAddresses.id';
+		$hash = 'baf05f9b1087725d8adf49a847c3a9174b2df7bf%3AAddresses.id';
 		$expected = array(
 			'div' => array('style' => 'display:none;'),
 			'input' => array(
@@ -1204,8 +1204,7 @@ function testFormSecuredInput() {
 		);
 		$this->assertEqual($result, $expected);
 
-		$hash = 'bd7c4a654e5361f9a433a43f488ff9a1065d0aaf%3AUserForm.hidden%7CUserForm.stuff';
-
+		$hash = '6014b4e1c4f39eb62389712111dbe6435bec66cb%3AUserForm.hidden%7CUserForm.stuff';
 		$result = $this->Form->secure($this->Form->fields);
 		$expected = array(
 			'div' => array('style' => 'display:none;'),