From f23d811ff59c50ef278e98bb75f4ec1e7e54a5b3 Mon Sep 17 00:00:00 2001 From: mark_story Date: Fri, 25 Apr 2014 22:05:58 -0400 Subject: [PATCH] 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. Thanks to Kurita Takashi for pointing this issue out, and suggesting a fix. --- .../Component/SecurityComponent.php | 8 ++- .../Component/SecurityComponentTest.php | 53 ++++++++++++------- .../Test/Case/View/Helper/FormHelperTest.php | 8 +-- lib/Cake/View/Helper/FormHelper.php | 17 +++++- 4 files changed, 61 insertions(+), 25 deletions(-) diff --git a/lib/Cake/Controller/Component/SecurityComponent.php b/lib/Cake/Controller/Component/SecurityComponent.php index 2f79c6a6c..6aeb77ef2 100644 --- a/lib/Cake/Controller/Component/SecurityComponent.php +++ b/lib/Cake/Controller/Component/SecurityComponent.php @@ -510,7 +510,13 @@ class SecurityComponent extends Component { $fieldList += $lockedFields; $unlocked = implode('|', $unlocked); - $check = Security::hash(serialize($fieldList) . $unlocked . Configure::read('Security.salt'), 'sha1'); + $hashParts = array( + $this->request->here(), + serialize($fieldList), + $unlocked, + Configure::read('Security.salt') + ); + $check = Security::hash(implode('', $hashParts), 'sha1'); return ($token === $check); } diff --git a/lib/Cake/Test/Case/Controller/Component/SecurityComponentTest.php b/lib/Cake/Test/Case/Controller/Component/SecurityComponentTest.php index 455758a60..0243fa90b 100644 --- a/lib/Cake/Test/Case/Controller/Component/SecurityComponentTest.php +++ b/lib/Cake/Test/Case/Controller/Component/SecurityComponentTest.php @@ -142,8 +142,12 @@ class SecurityComponentTest extends CakeTestCase { public function setUp() { parent::setUp(); - $request = new CakeRequest('posts/index', false); + $request = $this->getMock('CakeRequest', ['here'], ['posts/index', false]); $request->addParams(array('controller' => 'posts', 'action' => 'index')); + $request->expects($this->any()) + ->method('here') + ->will($this->returnValue('/posts/index')); + $this->Controller = new SecurityTestController($request); $this->Controller->Components->init($this->Controller); $this->Controller->Security = $this->Controller->TestSecurity; @@ -485,7 +489,7 @@ class SecurityComponentTest extends CakeTestCase { $this->Controller->Security->startup($this->Controller); $key = $this->Controller->request->params['_Token']['key']; - $fields = 'a5475372b40f6e3ccbf9f8af191f20e1642fd877%3AModel.valid'; + $fields = '01c1f6dbba02ac6f21b229eab1cc666839b14303%3AModel.valid'; $unlocked = ''; $this->Controller->request->data = array( @@ -565,7 +569,7 @@ class SecurityComponentTest extends CakeTestCase { $this->Controller->Security->startup($this->Controller); $key = $this->Controller->request->params['_Token']['key']; - $fields = 'f7d573650a295b94e0938d32b323fde775e5f32b%3A'; + $fields = '38504e4a341d4e6eadb437217efd91270e558d55%3A'; $unlocked = ''; $this->Controller->request->data = array( @@ -584,7 +588,7 @@ class SecurityComponentTest extends CakeTestCase { $this->Controller->Security->startup($this->Controller); $key = $this->Controller->request->params['_Token']['key']; - $fields = '540ac9c60d323c22bafe997b72c0790f39a8bdef%3A'; + $fields = 'c5bc49a6c938c820e7e538df3d8ab7bffbc97ef9%3A'; $unlocked = ''; $this->Controller->request->data = array( @@ -605,7 +609,7 @@ class SecurityComponentTest extends CakeTestCase { $this->Controller->Security->startup($this->Controller); $key = $this->Controller->request->params['_Token']['key']; - $fields = '69f493434187b867ea14b901fdf58b55d27c935d%3A'; + $fields = '5415d31b4483c1e09ddb58d2a91ba9650b12aa83%3A'; $unlocked = ''; $this->Controller->request->data = array( @@ -626,7 +630,7 @@ class SecurityComponentTest extends CakeTestCase { $this->Controller->Security->startup($this->Controller); $key = $this->Controller->request->params['_Token']['key']; - $fields = 'c9118120e680a7201b543f562e5301006ccfcbe2%3AAddresses.0.id%7CAddresses.1.id'; + $fields = 'b72a99e923687687bb5e64025d3cc65e1cecced4%3AAddresses.0.id%7CAddresses.1.id'; $unlocked = ''; $this->Controller->request->data = array( @@ -655,7 +659,7 @@ class SecurityComponentTest extends CakeTestCase { $this->Controller->Security->startup($this->Controller); $key = $this->Controller->request->params['_Token']['key']; - $fields = '422cde416475abc171568be690a98cad20e66079%3A'; + $fields = '8a764bdb989132c1d46f9a45f64ce2da5f9eebb9%3A'; $unlocked = ''; $this->Controller->request->data = array( @@ -679,7 +683,7 @@ class SecurityComponentTest extends CakeTestCase { $result = $this->Controller->Security->validatePost($this->Controller); $this->assertTrue($result); - $fields = '19464422eafe977ee729c59222af07f983010c5f%3A'; + $fields = '722de3615e63fdff899e86e85e6498b11c50bb66%3A'; $this->Controller->request->data = array( 'User.password' => 'bar', 'User.name' => 'foo', 'User.is_valid' => '1', 'Tag' => array('Tag' => array(1)), @@ -700,7 +704,7 @@ class SecurityComponentTest extends CakeTestCase { public function testValidatePostCheckbox() { $this->Controller->Security->startup($this->Controller); $key = $this->Controller->request->params['_Token']['key']; - $fields = 'a5475372b40f6e3ccbf9f8af191f20e1642fd877%3AModel.valid'; + $fields = '01c1f6dbba02ac6f21b229eab1cc666839b14303%3AModel.valid'; $unlocked = ''; $this->Controller->request->data = array( @@ -711,7 +715,7 @@ class SecurityComponentTest extends CakeTestCase { $result = $this->Controller->Security->validatePost($this->Controller); $this->assertTrue($result); - $fields = '874439ca69f89b4c4a5f50fb9c36ff56a28f5d42%3A'; + $fields = 'efbcf463a2c31e97c85d95eedc41dff9e9c6a026%3A'; $this->Controller->request->data = array( 'Model' => array('username' => '', 'password' => '', 'valid' => '0'), @@ -742,7 +746,7 @@ class SecurityComponentTest extends CakeTestCase { public function testValidatePostHidden() { $this->Controller->Security->startup($this->Controller); $key = $this->Controller->request->params['_Token']['key']; - $fields = '51ccd8cb0997c7b3d4523ecde5a109318405ef8c%3AModel.hidden%7CModel.other_hidden'; + $fields = 'baaf832a714b39a0618238ac89c7065fc8ec853e%3AModel.hidden%7CModel.other_hidden'; $unlocked = ''; $this->Controller->request->data = array( @@ -765,7 +769,7 @@ class SecurityComponentTest extends CakeTestCase { $this->Controller->Security->disabledFields = array('Model.username', 'Model.password'); $this->Controller->Security->startup($this->Controller); $key = $this->Controller->request->params['_Token']['key']; - $fields = 'ef1082968c449397bcd849f963636864383278b1%3AModel.hidden'; + $fields = 'aa7f254ebd8bf2ef118bc5ca1e191d1ae96857f5%3AModel.hidden'; $unlocked = ''; $this->Controller->request->data = array( @@ -789,7 +793,12 @@ class SecurityComponentTest extends CakeTestCase { $key = $this->Controller->request->params['_Token']['key']; $unlocked = 'Model.username'; $fields = array('Model.hidden', 'Model.password'); - $fields = urlencode(Security::hash(serialize($fields) . $unlocked . Configure::read('Security.salt'))); + $fields = urlencode(Security::hash( + '/posts/index' . + serialize($fields) . + $unlocked . + Configure::read('Security.salt')) + ); $this->Controller->request->data = array( 'Model' => array( @@ -864,7 +873,7 @@ class SecurityComponentTest extends CakeTestCase { public function testValidateHiddenMultipleModel() { $this->Controller->Security->startup($this->Controller); $key = $this->Controller->request->params['_Token']['key']; - $fields = 'a2d01072dc4660eea9d15007025f35a7a5b58e18%3AModel.valid%7CModel2.valid%7CModel3.valid'; + $fields = '38dd8a37bbb52e67ee4eb812bf1725a6a18b989b%3AModel.valid%7CModel2.valid%7CModel3.valid'; $unlocked = ''; $this->Controller->request->data = array( @@ -885,7 +894,7 @@ class SecurityComponentTest extends CakeTestCase { public function testValidateHasManyModel() { $this->Controller->Security->startup($this->Controller); $key = $this->Controller->request->params['_Token']['key']; - $fields = '51e3b55a6edd82020b3f29c9ae200e14bbeb7ee5%3AModel.0.hidden%7CModel.0.valid'; + $fields = 'dcef68de6634c60d2e60484ad0e2faec003456e6%3AModel.0.hidden%7CModel.0.valid'; $fields .= '%7CModel.1.hidden%7CModel.1.valid'; $unlocked = ''; @@ -915,7 +924,7 @@ class SecurityComponentTest extends CakeTestCase { public function testValidateHasManyRecordsPass() { $this->Controller->Security->startup($this->Controller); $key = $this->Controller->request->params['_Token']['key']; - $fields = '7a203edb3d345bbf38fe0dccae960da8842e11d7%3AAddress.0.id%7CAddress.0.primary%7C'; + $fields = '8b6880fbbd4b69279155f899652ecffdd9b4c5a1%3AAddress.0.id%7CAddress.0.primary%7C'; $fields .= 'Address.1.id%7CAddress.1.primary'; $unlocked = ''; @@ -959,7 +968,13 @@ class SecurityComponentTest extends CakeTestCase { $key = $this->Controller->request->params['_Token']['key']; $unlocked = ''; $hashFields = array('TaxonomyData'); - $fields = urlencode(Security::hash(serialize($hashFields) . $unlocked . Configure::read('Security.salt'))); + $fields = urlencode( + Security::hash( + '/posts/index' . + serialize($hashFields) . + $unlocked . + Configure::read('Security.salt'), 'sha1') + ); $this->Controller->request->data = array( 'TaxonomyData' => array( @@ -1024,7 +1039,7 @@ class SecurityComponentTest extends CakeTestCase { public function testFormDisabledFields() { $this->Controller->Security->startup($this->Controller); $key = $this->Controller->request->params['_Token']['key']; - $fields = '11842060341b9d0fc3808b90ba29fdea7054d6ad%3An%3A0%3A%7B%7D'; + $fields = '216ee717efd1a251a6d6e9efbb96005a9d09f1eb%3An%3A0%3A%7B%7D'; $unlocked = ''; $this->Controller->request->data = array( @@ -1055,7 +1070,7 @@ class SecurityComponentTest extends CakeTestCase { public function testRadio() { $this->Controller->Security->startup($this->Controller); $key = $this->Controller->request->params['_Token']['key']; - $fields = '575ef54ca4fc8cab468d6d898e9acd3a9671c17e%3An%3A0%3A%7B%7D'; + $fields = '3be63770e7953c6d2119f5377a9303372040f66f%3An%3A0%3A%7B%7D'; $unlocked = ''; $this->Controller->request->data = array( diff --git a/lib/Cake/Test/Case/View/Helper/FormHelperTest.php b/lib/Cake/Test/Case/View/Helper/FormHelperTest.php index f548663cd..4a572c964 100644 --- a/lib/Cake/Test/Case/View/Helper/FormHelperTest.php +++ b/lib/Cake/Test/Case/View/Helper/FormHelperTest.php @@ -990,7 +990,7 @@ class FormHelperTest extends CakeTestCase { $result = $this->Form->secure($this->Form->fields); - $hash = 'c9118120e680a7201b543f562e5301006ccfcbe2%3AAddresses.0.id%7CAddresses.1.id'; + $hash = 'a3b9b2ba1cb688838f92818a5970e17dd7943a78%3AAddresses.0.id%7CAddresses.1.id'; $expected = array( 'div' => array('style' => 'display:none;'), @@ -1055,7 +1055,7 @@ class FormHelperTest extends CakeTestCase { $this->Form->input('Addresses.1.phone'); $result = $this->Form->secure($this->Form->fields); - $hash = '629b6536dcece48aa41a117045628ce602ccbbb2%3AAddresses.0.id%7CAddresses.1.id'; + $hash = '5c9cadf9da008cc444d3960b481391a425a5d979%3AAddresses.0.id%7CAddresses.1.id'; $expected = array( 'div' => array('style' => 'display:none;'), @@ -1105,7 +1105,7 @@ class FormHelperTest extends CakeTestCase { $result = $this->Form->secure($expected); - $hash = '2981c38990f3f6ba935e6561dc77277966fabd6d%3AAddresses.id'; + $hash = '40289bd07811587887ff56585a8526ff9da59d7a%3AAddresses.id'; $expected = array( 'div' => array('style' => 'display:none;'), array('input' => array( @@ -1228,7 +1228,7 @@ class FormHelperTest extends CakeTestCase { ); $this->assertEquals($expected, $result); - $hash = 'bd7c4a654e5361f9a433a43f488ff9a1065d0aaf%3AUserForm.hidden%7CUserForm.stuff'; + $hash = '6014b4e1c4f39eb62389712111dbe6435bec66cb%3AUserForm.hidden%7CUserForm.stuff'; $result = $this->Form->secure($this->Form->fields); $expected = array( diff --git a/lib/Cake/View/Helper/FormHelper.php b/lib/Cake/View/Helper/FormHelper.php index 4a532003b..e69137a38 100644 --- a/lib/Cake/View/Helper/FormHelper.php +++ b/lib/Cake/View/Helper/FormHelper.php @@ -117,6 +117,14 @@ class FormHelper extends AppHelper { */ protected $_domIdSuffixes = array(); +/** + * The action attribute value of the last created form. + * Used to make form/request specific hashes for SecurityComponent. + * + * @var string + */ + protected $_lastAction = ''; + /** * Copies the validationErrors variable from the View object into this instance * @@ -458,6 +466,7 @@ class FormHelper extends AppHelper { $this->setEntity($model, true); $this->_introspectModel($model, 'fields'); } + $this->_lastAction = $action; return $this->Html->useTag('form', $action, $htmlAttributes) . $append; } @@ -563,7 +572,13 @@ class FormHelper extends AppHelper { $locked = implode(array_keys($locked), '|'); $unlocked = implode($unlockedFields, '|'); - $fields = Security::hash(serialize($fields) . $unlocked . Configure::read('Security.salt'), 'sha1'); + $hashParts = array( + $this->_lastAction, + serialize($fields), + $unlocked, + Configure::read('Security.salt') + ); + $fields = Security::hash(implode('', $hashParts), 'sha1'); $out = $this->hidden('_Token.fields', array( 'value' => urlencode($fields . ':' . $locked),