From 2886f6ee14f48bc814a8c020835ad14d56a5f107 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Sat, 6 Jul 2013 15:15:49 +1200 Subject: [PATCH 1/2] FIX Session was started every time, even if no data set Session tracks the user agent in the session, to add some detection of stolen session IDs. However this was causing a session to always be created, even if this request didnt store any data in the session. --- control/Session.php | 25 +++++++++++++++---------- tests/control/SessionTest.php | 1 + 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/control/Session.php b/control/Session.php index 8c24d3ba5..7373a5c9f 100644 --- a/control/Session.php +++ b/control/Session.php @@ -128,6 +128,14 @@ class Session { protected $changedData = array(); + protected function userAgent() { + if (isset($_SERVER['HTTP_USER_AGENT'])) { + return $_SERVER['HTTP_USER_AGENT']; + } else { + return ''; + } + } + /** * Start PHP session, then create a new Session object with the given start data. * @@ -138,14 +146,8 @@ class Session { $this->data = $data; - if (isset($_SERVER['HTTP_USER_AGENT'])) { - $ua = $_SERVER['HTTP_USER_AGENT']; - } else { - $ua = ''; - } - if (isset($this->data['HTTP_USER_AGENT'])) { - if ($this->data['HTTP_USER_AGENT'] != $ua) { + if ($this->data['HTTP_USER_AGENT'] != $this->userAgent()) { // Funny business detected! $this->inst_clearAll(); @@ -153,8 +155,6 @@ class Session { Session::start(); } } - - $this->inst_set('HTTP_USER_AGENT', $ua); } /** @@ -460,13 +460,18 @@ class Session { public function inst_getAll() { return $this->data; } - + + public function inst_finalize() { + $this->inst_set('HTTP_USER_AGENT', $this->userAgent()); + } + /** * Save data to session * Only save the changes, so that anyone manipulating $_SESSION directly doesn't get burned. */ public function inst_save() { if($this->changedData) { + $this->inst_finalize(); if(!isset($_SESSION)) Session::start(); $this->recursivelyApply($this->changedData, $_SESSION); } diff --git a/tests/control/SessionTest.php b/tests/control/SessionTest.php index aa7e317c7..49f7a427b 100644 --- a/tests/control/SessionTest.php +++ b/tests/control/SessionTest.php @@ -99,6 +99,7 @@ class SessionTest extends SapphireTest { // Generate our session $s = new Session(array()); $s->inst_set('val', 123); + $s->inst_finalize(); // Change our UA $_SERVER['HTTP_USER_AGENT'] = 'Fake Agent'; From d629d9422fecb858fca53cbbb51ed0ef3d8cfa36 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Sat, 6 Jul 2013 15:10:43 +1200 Subject: [PATCH 2/2] FIX Session::$cookie_secure so Sessions still work via HTTP Session::$cookie_secure adds the secure property to the session Set-Cookie command, so that the browser wouldnt send it to the server over an unencrypted link. However the server would still send the cookie to the browser unencrypted. Also Sessions would stop working properly in HTTP, but SilverStripe needs them for several things, such as form validation This patch effectively causes HTTP and HTTPS requests to each have their own session when cookie_secure is true. The two sessions are independant from each other, so information set in the session via HTTPS is safe from attacks on the session via HTTP, but parts of the site that use HTTP and the session will still work --- control/Director.php | 2 +- control/Session.php | 17 +++++++++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/control/Director.php b/control/Director.php index a3686d279..8597e09db 100644 --- a/control/Director.php +++ b/control/Director.php @@ -127,7 +127,7 @@ class Director implements TemplateGlobalProvider { } // Only resume a session if its not started already, and a session identifier exists - if(!isset($_SESSION) && (isset($_COOKIE[session_name()]) || isset($_REQUEST[session_name()]))) { + if(!isset($_SESSION) && Session::request_contains_session_id()) { Session::start(); } // Initiate an empty session - doesn't initialize an actual PHP session until saved (see belwo) diff --git a/control/Session.php b/control/Session.php index 7373a5c9f..a4aaf0b89 100644 --- a/control/Session.php +++ b/control/Session.php @@ -513,6 +513,16 @@ class Session { Session::set("FormInfo.$formname.formError.type", $type); } + /** + * Is there a session ID in the request? + * @return bool + */ + public static function request_contains_session_id() { + $secure = Director::is_https() && Config::inst()->get('Session', 'cookie_secure'); + $name = $secure ? 'SECSESSID' : session_name(); + return isset($_COOKIE[$name]) || isset($_REQUEST[$name]); + } + /** * Initialize session. * @@ -522,7 +532,7 @@ class Session { $path = Config::inst()->get('Session', 'cookie_path'); if(!$path) $path = Director::baseURL(); $domain = Config::inst()->get('Session', 'cookie_domain'); - $secure = Config::inst()->get('Session', 'cookie_secure'); + $secure = Director::is_https() && Config::inst()->get('Session', 'cookie_secure'); $session_path = Config::inst()->get('Session', 'session_store_path'); $timeout = Config::inst()->get('Session', 'timeout'); @@ -538,11 +548,14 @@ class Session { // Allow storing the session in a non standard location if($session_path) session_save_path($session_path); + // If we want a secure cookie for HTTPS, use a seperate session name. This lets us have a + // seperate (less secure) session for non-HTTPS requests + if($secure) session_name('SECSESSID'); + // @ is to supress win32 warnings/notices when session wasn't cleaned up properly // There's nothing we can do about this, because it's an operating system function! if($sid) session_id($sid); @session_start(); - } // Modify the timeout behaviour so it's the *inactive* time before the session expires.