Skip to content
Permalink
Browse files

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.
  • Loading branch information...
markstory committed Apr 26, 2014
1 parent 2d4270f commit db6cfe4bcedf7cf9fd8a6df59e072917f8e2dafe
@@ -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);
}
@@ -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(
@@ -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.
*
@@ -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;'),

0 comments on commit db6cfe4

Please sign in to comment.
You can’t perform that action at this time.