Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Hotfix/4785 csrf name conflicts #5918

Closed
wants to merge 12 commits into from

2 participants

Stefano Torresi Matthew Weier O'Phinney
Stefano Torresi

this PR should fix #4785

I'm not very sure about it, it is probably not very fancy, but I should have managed to fix the validator without adding any further state to it, actually even removing some, and maintain BC.

As far as I can tell, the only downside is that tokens are not statically cached any more and are regenerated each time a new validator instance is created (which may arguably be an improvement).

I had to edit ZendTest\Form\FormElementManagerFactoryTest because it was a bit of a mess. I didn't delve too much into it, but further cleaning is possible. I honestly don't know how much of a test I can change for this purpose. Also, it is failing in php 5.3.3 only, no clue why. I suppose it's not a problem since this issue was tagged for milestone 2.3.0.

Matthew Weier O'Phinney weierophinney added this to the 2.3.0 milestone
Matthew Weier O'Phinney weierophinney self-assigned this
Matthew Weier O'Phinney

Yeah, not worried about the 5.3.3 test failures at all. Merging to develop for release with 2.3.0.

Stefano Torresi stefanotorresi deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
95 library/Zend/Validator/Csrf.php
View
@@ -39,6 +39,7 @@ class Csrf extends AbstractValidator
/**
* Static cache of the session names to generated hashes
+ * @todo unused, left here to avoid BC breaks
*
* @var array
*/
@@ -117,9 +118,10 @@ public function isValid($value, $context = null)
{
$this->setValue((string) $value);
- $hash = $this->getValidationToken();
+ $tokenId = $this->getTokenIdFromHash($value);
+ $hash = $this->getValidationToken($tokenId);
- if ($value !== $hash) {
+ if ($this->getTokenFromHash($value) !== $this->getTokenFromHash($hash)) {
$this->error(self::NOT_SAME);
return false;
}
@@ -214,14 +216,7 @@ public function getSalt()
public function getHash($regenerate = false)
{
if ((null === $this->hash) || $regenerate) {
- if ($regenerate) {
- $this->hash = null;
- } else {
- $this->hash = $this->getValidationToken();
- }
- if (null === $this->hash) {
- $this->generateHash();
- }
+ $this->generateHash();
}
return $this->hash;
}
@@ -270,12 +265,20 @@ public function getTimeout()
protected function initCsrfToken()
{
$session = $this->getSession();
- //$session->setExpirationHops(1, null);
$timeout = $this->getTimeout();
if (null !== $timeout) {
$session->setExpirationSeconds($timeout);
}
- $session->hash = $this->getHash();
+
+ $hash = $this->getHash();
+ $token = $this->getTokenFromHash($hash);
+ $tokenId = $this->getTokenIdFromHash($hash);
+
+ if (! $session->tokenList) {
+ $session->tokenList = array();
+ }
+ $session->tokenList[$tokenId] = $token;
+ $session->hash = $hash; // @todo remove this, left for BC
}
/**
@@ -288,29 +291,81 @@ protected function initCsrfToken()
*/
protected function generateHash()
{
- if (isset(static::$hashCache[$this->getSessionName()])) {
- $this->hash = static::$hashCache[$this->getSessionName()];
- } else {
- $this->hash = md5($this->getSalt() . Rand::getBytes(32) . $this->getName());
- static::$hashCache[$this->getSessionName()] = $this->hash;
- }
+ $token = md5($this->getSalt() . Rand::getBytes(32) . $this->getName());
+
+ $this->hash = $this->formatHash($token, $this->generateTokenId());
+
$this->setValue($this->hash);
$this->initCsrfToken();
}
/**
+ * @return string
+ */
+ protected function generateTokenId()
+ {
+ return md5(Rand::getBytes(32));
+ }
+
+ /**
* Get validation token
*
* Retrieve token from session, if it exists.
*
+ * @param string $tokenId
* @return null|string
*/
- protected function getValidationToken()
+ protected function getValidationToken($tokenId = null)
{
$session = $this->getSession();
- if (isset($session->hash)) {
+
+ /**
+ * if no tokenId is passed we revert to the old behaviour
+ * @todo remove, here for BC
+ */
+ if (! $tokenId && isset($session->hash)) {
return $session->hash;
}
+
+ if ($tokenId && isset($session->tokenList[$tokenId])) {
+ return $this->formatHash($session->tokenList[$tokenId], $tokenId);
+ }
+
return null;
}
+
+ /**
+ * @param $token
+ * @param $tokenId
+ * @return string
+ */
+ protected function formatHash($token, $tokenId)
+ {
+ return sprintf('%s-%s', $token, $tokenId);
+ }
+
+ /**
+ * @param $hash
+ * @return string
+ */
+ protected function getTokenFromHash($hash)
+ {
+ $data = explode('-', $hash);
+ return $data[0] ?: null;
+ }
+
+ /**
+ * @param $hash
+ * @return string
+ */
+ protected function getTokenIdFromHash($hash)
+ {
+ $data = explode('-', $hash);
+
+ if (! isset($data[1])) {
+ return null;
+ }
+
+ return $data[1];
+ }
}
13 tests/ZendTest/Form/FormElementManagerFactoryTest.php
View
@@ -106,23 +106,14 @@ public function testCsrfWorkFlow()
{
$_SESSION = array();
$formClass = 'ZendTest\Form\TestAsset\CustomForm';
- $ref = new \ReflectionClass('Zend\Validator\Csrf');
- $hashPropRef = $ref->getProperty('hash');
- $hashPropRef->setAccessible(true);
- $hashCache = $ref->getProperty('hashCache');
- $hashCache->setAccessible(true);
- $hashCache->setValue(new Csrf, array());
- //check bare born
+
$preForm = new $formClass;
$preForm->prepare();
$requestHash = $preForm->get('csrf')->getValue();
- SessionContainer::setDefaultManager(null);
- $hashCache->setValue(new Csrf, array());
$postForm = $this->manager->get($formClass);
$postCsrf = $postForm->get('csrf')->getCsrfValidator();
- $storedHash = $postCsrf->getHash();
- $this->assertEquals($requestHash, $storedHash, 'Test csrf validation');
+ $this->assertTrue($postCsrf->isValid($requestHash), 'Test csrf validation');
}
}
73 tests/ZendTest/Validator/CsrfTest.php
View
@@ -187,4 +187,77 @@ public function testSettingNewSessionContainerSetsHashInNewContainer()
$test = $container->hash; // Doing this, as expiration hops are 1; have to grab on first access
$this->assertEquals($hash, $test);
}
+
+ public function testMultipleValidatorsSharingContainerGenerateDifferentHashes()
+ {
+ $validatorOne = new Csrf();
+ $validatorTwo = new Csrf();
+
+ $containerOne = $validatorOne->getSession();
+ $containerTwo = $validatorOne->getSession();
+
+ $this->assertSame($containerOne, $containerTwo);
+
+ $hashOne = $validatorOne->getHash();
+ $hashTwo = $validatorTwo->getHash();
+ $this->assertNotEquals($hashOne , $hashTwo);
+ }
+
+ public function testCanValidateAnyHashWithinTheSameContainer()
+ {
+ $validatorOne = new Csrf();
+ $validatorTwo = new Csrf();
+
+ $hashOne = $validatorOne->getHash();
+ $hashTwo = $validatorTwo->getHash();
+
+ $this->assertTrue($validatorOne->isValid($hashOne));
+ $this->assertTrue($validatorOne->isValid($hashTwo));
+ $this->assertTrue($validatorTwo->isValid($hashOne));
+ $this->assertTrue($validatorTwo->isValid($hashTwo));
+ }
+
+ public function testCannotValidateHashesOfOtherContainers()
+ {
+ $validatorOne = new Csrf();
+ $validatorTwo = new Csrf(array('name' => 'foo'));
+
+ $containerOne = $validatorOne->getSession();
+ $containerTwo = $validatorTwo->getSession();
+
+ $this->assertNotSame($containerOne, $containerTwo);
+
+ $hashOne = $validatorOne->getHash();
+ $hashTwo = $validatorTwo->getHash();
+
+ $this->assertTrue($validatorOne->isValid($hashOne));
+ $this->assertFalse($validatorOne->isValid($hashTwo));
+ $this->assertFalse($validatorTwo->isValid($hashOne));
+ $this->assertTrue($validatorTwo->isValid($hashTwo));
+ }
+
+ public function testCannotReValidateAnExpiredHash()
+ {
+ $hash = $this->validator->getHash();
+
+ $this->assertTrue($this->validator->isValid($hash));
+
+ $this->sessionManager->getStorage()->setMetadata(
+ $this->validator->getSession()->getName(),
+ array('EXPIRE' => $_SERVER['REQUEST_TIME'] - 18600)
+ );
+
+ $this->assertFalse($this->validator->isValid($hash));
+ }
+
+ public function testCanValidateHasheWithoutId()
+ {
+ $method = new \ReflectionMethod(get_class($this->validator), 'getTokenFromHash');
+ $method->setAccessible(true);
+
+ $hash = $this->validator->getHash();
+ $bareToken = $method->invoke($this->validator, $hash);
+
+ $this->assertTrue($this->validator->isValid($bareToken));
+ }
}
Something went wrong with that request. Please try again.