Merge pull request #53 from silverstripe-security/pulls/3.5/ss-2017-006

[ss-2017-006] Fix user agent invalidation on session startup (3.5 branch)
This commit is contained in:
Damian Mooyman 2017-12-06 16:25:39 +13:00 committed by GitHub
commit 44de03da01
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 45 additions and 11 deletions

View File

@ -145,15 +145,7 @@ class Session {
if($data instanceof Session) $data = $data->inst_getAll();
$this->data = $data;
if (isset($this->data['HTTP_USER_AGENT'])) {
if ($this->data['HTTP_USER_AGENT'] != $this->userAgent()) {
// Funny business detected!
$this->inst_clearAll();
$this->inst_destroy();
$this->inst_start();
}
}
$this->expireIfInvalid();
}
/**
@ -392,6 +384,9 @@ class Session {
$this->data = isset($_SESSION) ? $_SESSION : array();
}
// Ensure session is validated on start
$this->expireIfInvalid();
// Modify the timeout behaviour so it's the *inactive* time before the session expires.
// By default it's the total session lifetime
if($timeout && !headers_sent()) {
@ -631,4 +626,31 @@ class Session {
Deprecation::notice('4.0', 'Use the "Session.timeout" config setting instead');
return Config::inst()->get('Session', 'timeout');
}
/**
* Validate the user agent against the current data, resetting the
* current session if a mismatch is detected.
*
* @deprecated 3.0..4.0 Removed in 4.0
* @return bool If user agent has been set against this session, returns
* the valid state of this session as either true or false. If the agent
* isn't set it is assumed valid and returns true.
*/
private function expireIfInvalid() {
// If not set, indeterminable; Assume true as safe default
if (!isset($this->data['HTTP_USER_AGENT'])) {
return true;
}
// Agents match, deterministically true
if ($this->data['HTTP_USER_AGENT'] === $this->userAgent()) {
return true;
}
// Funny business detected!
$this->inst_clearAll();
$this->inst_destroy();
$this->inst_start();
return false;
}
}

View File

@ -97,15 +97,27 @@ class SessionTest extends SapphireTest {
$_SERVER['HTTP_USER_AGENT'] = 'Test Agent';
// Generate our session
/** @var Session $s */
$s = Injector::inst()->create('Session', array());
$s->inst_set('val', 123);
$s->inst_finalize();
$data = $s->inst_getAll();
// Change our UA
$_SERVER['HTTP_USER_AGENT'] = 'Fake Agent';
// Verify the new session reset our values
$s2 = Injector::inst()->create('Session', $s);
// Verify the new session reset our values (passed by constructor)
/** @var Session $s2 */
$s2 = Injector::inst()->create('Session', $data);
$this->assertNotEquals($s2->inst_get('val'), 123);
// Verify a started session resets our values (initiated by $_SESSION object)
/** @var Session $s3 */
$s3 = Injector::inst()->create('Session', []);
foreach ($data as $key => $value) {
$s3->inst_set($key, $value);
}
$s3->inst_start();
$this->assertNotEquals($s3->inst_get('val'), 123);
}
}