Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
markstory committed Apr 26, 2014
1 parent 4a24d6e commit f23d811
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 25 deletions.
8 changes: 7 additions & 1 deletion lib/Cake/Controller/Component/SecurityComponent.php
Expand Up @@ -510,7 +510,13 @@ protected function _validatePost(Controller $controller) {

$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);
}

Expand Down
53 changes: 34 additions & 19 deletions lib/Cake/Test/Case/Controller/Component/SecurityComponentTest.php
Expand Up @@ -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;
Expand Down Expand Up @@ -485,7 +489,7 @@ public function testValidatePost() {
$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(
Expand Down Expand Up @@ -565,7 +569,7 @@ public function testValidatePostArray() {
$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(
Expand All @@ -584,7 +588,7 @@ public function testValidatePostNoModel() {
$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(
Expand All @@ -605,7 +609,7 @@ public function testValidatePostSimple() {
$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(
Expand All @@ -626,7 +630,7 @@ public function testValidatePostComplex() {
$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(
Expand Down Expand Up @@ -655,7 +659,7 @@ public function testValidatePostMultipleSelect() {
$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(
Expand All @@ -679,7 +683,7 @@ public function testValidatePostMultipleSelect() {
$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)),
Expand All @@ -700,7 +704,7 @@ public function testValidatePostMultipleSelect() {
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(
Expand All @@ -711,7 +715,7 @@ public function testValidatePostCheckbox() {
$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'),
Expand Down Expand Up @@ -742,7 +746,7 @@ public function testValidatePostCheckbox() {
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(
Expand All @@ -765,7 +769,7 @@ public function testValidatePostWithDisabledFields() {
$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(
Expand All @@ -789,7 +793,12 @@ public function testValidatePostDisabledFieldsInData() {
$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(
Expand Down Expand Up @@ -864,7 +873,7 @@ public function testValidatePostFailDisabledFieldTampering() {
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(
Expand All @@ -885,7 +894,7 @@ public function testValidateHiddenMultipleModel() {
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 = '';

Expand Down Expand Up @@ -915,7 +924,7 @@ public function testValidateHasManyModel() {
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 = '';

Expand Down Expand Up @@ -959,7 +968,13 @@ public function testValidateNestedNumericSets() {
$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(
Expand Down Expand Up @@ -1024,7 +1039,7 @@ public function testValidateHasManyRecordsFail() {
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(
Expand Down Expand Up @@ -1055,7 +1070,7 @@ public function testFormDisabledFields() {
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(
Expand Down
8 changes: 4 additions & 4 deletions lib/Cake/Test/Case/View/Helper/FormHelperTest.php
Expand Up @@ -990,7 +990,7 @@ public function testFormSecurityMultipleInputFields() {

$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;'),
Expand Down Expand Up @@ -1055,7 +1055,7 @@ public function testFormSecurityMultipleInputDisabledFields() {
$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;'),
Expand Down Expand Up @@ -1105,7 +1105,7 @@ public function testFormSecurityInputUnlockedFields() {

$result = $this->Form->secure($expected);

$hash = '2981c38990f3f6ba935e6561dc77277966fabd6d%3AAddresses.id';
$hash = '40289bd07811587887ff56585a8526ff9da59d7a%3AAddresses.id';
$expected = array(
'div' => array('style' => 'display:none;'),
array('input' => array(
Expand Down Expand Up @@ -1228,7 +1228,7 @@ public function testFormSecuredInput() {
);
$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(
Expand Down
17 changes: 16 additions & 1 deletion lib/Cake/View/Helper/FormHelper.php
Expand Up @@ -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
*
Expand Down Expand Up @@ -458,6 +466,7 @@ public function create($model = null, $options = array()) {
$this->setEntity($model, true);
$this->_introspectModel($model, 'fields');
}
$this->_lastAction = $action;
return $this->Html->useTag('form', $action, $htmlAttributes) . $append;
}

Expand Down Expand Up @@ -563,7 +572,13 @@ public function secure($fields = array()) {

$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),
Expand Down

2 comments on commit f23d811

@chinpei215
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at all.

@lorenzo
Copy link
Member

@lorenzo lorenzo commented on f23d811 May 2, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to fix your .htaccess or your nginx configuration so instead of: index.php?url=$1 it only contains index.php for the rewrite target.

Please sign in to comment.