From 7474a6799b44e4988b6a7a4adcc2901ec0b993b4 Mon Sep 17 00:00:00 2001 From: Andrey Andreev Date: Fri, 31 Oct 2014 23:35:32 +0200 Subject: #3073 (feature/session): Fix session_regenerate_id() issues --- system/libraries/Session/Session_driver.php | 10 ++++++ .../Session/drivers/Session_database_driver.php | 14 ++++++++ .../Session/drivers/Session_files_driver.php | 40 +++++++++++++--------- .../Session/drivers/Session_memcached_driver.php | 26 ++++++++++++-- .../Session/drivers/Session_redis_driver.php | 32 ++++++++++++----- 5 files changed, 95 insertions(+), 27 deletions(-) diff --git a/system/libraries/Session/Session_driver.php b/system/libraries/Session/Session_driver.php index fb695dade..ad64e238a 100644 --- a/system/libraries/Session/Session_driver.php +++ b/system/libraries/Session/Session_driver.php @@ -53,6 +53,16 @@ abstract class CI_Session_driver implements SessionHandlerInterface { */ protected $_lock = FALSE; + /** + * Read session ID + * + * Used to detect session_regenerate_id() calls because PHP only calls + * write() after regenerating the ID. + * + * @var string + */ + protected $_session_id; + // ------------------------------------------------------------------------ /** diff --git a/system/libraries/Session/drivers/Session_database_driver.php b/system/libraries/Session/drivers/Session_database_driver.php index e3a3c505e..9e74605bc 100644 --- a/system/libraries/Session/drivers/Session_database_driver.php +++ b/system/libraries/Session/drivers/Session_database_driver.php @@ -111,6 +111,9 @@ class CI_Session_database_driver extends CI_Session_driver implements SessionHan { if ($this->_get_lock($session_id) !== FALSE) { + // Needed by write() to detect session_regenerate_id() calls + $this->_session_id = $session_id; + $this->_db ->select('data') ->from($this->_config['save_path']) @@ -142,6 +145,17 @@ class CI_Session_database_driver extends CI_Session_driver implements SessionHan { return FALSE; } + // Was the ID regenerated? + elseif ($session_id !== $this->_session_id) + { + if ( ! $this->_release_lock() OR ! $this->_get_lock($session_id)) + { + return FALSE; + } + + $this->_row_exists = FALSE; + $this->_session_id = $session_id; + } if ($this->_row_exists === FALSE) { diff --git a/system/libraries/Session/drivers/Session_files_driver.php b/system/libraries/Session/drivers/Session_files_driver.php index ff1553f84..3d6fa6322 100644 --- a/system/libraries/Session/drivers/Session_files_driver.php +++ b/system/libraries/Session/drivers/Session_files_driver.php @@ -114,36 +114,37 @@ class CI_Session_files_driver extends CI_Session_driver implements SessionHandle // which re-reads session data if ($this->_file_handle === NULL) { - $this->_file_path .= $session_id; - // Just using fopen() with 'c+b' mode would be perfect, but it is only // available since PHP 5.2.6 and we have to set permissions for new files, // so we'd have to hack around this ... - if (($this->_file_new = ! file_exists($this->_file_path)) === TRUE) + if (($this->_file_new = ! file_exists($this->_file_path.$session_id)) === TRUE) { - if (($this->_file_handle = fopen($this->_file_path, 'w+b')) === FALSE) + if (($this->_file_handle = fopen($this->_file_path.$session_id, 'w+b')) === FALSE) { - log_message('error', "Session: File '".$this->_file_path."' doesn't exist and cannot be created."); + log_message('error', "Session: File '".$this->_file_path.$session_id."' doesn't exist and cannot be created."); return FALSE; } } - elseif (($this->_file_handle = fopen($this->_file_path, 'r+b')) === FALSE) + elseif (($this->_file_handle = fopen($this->_file_path.$session_id, 'r+b')) === FALSE) { - log_message('error', "Session: Unable to open file '".$this->_file_path."'."); + log_message('error', "Session: Unable to open file '".$this->_file_path.$session_id."'."); return FALSE; } if (flock($this->_file_handle, LOCK_EX) === FALSE) { - log_message('error', "Session: Unable to obtain lock for file '".$this->_file_path."'."); + log_message('error', "Session: Unable to obtain lock for file '".$this->_file_path.$session_id."'."); fclose($this->_file_handle); $this->_file_handle = NULL; return FALSE; } + // Needed by write() to detect session_regenerate_id() calls + $this->_session_id = $session_id; + if ($this->_file_new) { - chmod($this->_file_path, 0600); + chmod($this->_file_path.$session_id, 0600); $this->_fingerprint = md5(''); return ''; } @@ -154,7 +155,7 @@ class CI_Session_files_driver extends CI_Session_driver implements SessionHandle } $session_data = ''; - for ($read = 0, $length = filesize($this->_file_path); $read < $length; $read += strlen($buffer)) + for ($read = 0, $length = filesize($this->_file_path.$session_id); $read < $length; $read += strlen($buffer)) { if (($buffer = fread($this->_file_handle, $length - $read)) === FALSE) { @@ -170,6 +171,13 @@ class CI_Session_files_driver extends CI_Session_driver implements SessionHandle public function write($session_id, $session_data) { + // If the two IDs don't match, we have a session_regenerate_id() call + // and we need to close the old handle and open a new one + if ($session_id !== $this->_session_id && ( ! $this->close() OR $this->read($session_id) === FALSE)) + { + return FALSE; + } + if ( ! is_resource($this->_file_handle)) { return FALSE; @@ -178,7 +186,7 @@ class CI_Session_files_driver extends CI_Session_driver implements SessionHandle { return ($this->_file_new) ? TRUE - : touch($this->_file_path); + : touch($this->_file_path.$session_id); } if ( ! $this->_file_new) @@ -218,11 +226,11 @@ class CI_Session_files_driver extends CI_Session_driver implements SessionHandle flock($this->_file_handle, LOCK_UN); fclose($this->_file_handle); - $this->_file_handle = $this->_file_new = NULL; + $this->_file_handle = $this->_file_new = $this->_session_id = NULL; return TRUE; } - return FALSE; + return TRUE; } // ------------------------------------------------------------------------ @@ -231,13 +239,13 @@ class CI_Session_files_driver extends CI_Session_driver implements SessionHandle { if ($this->close()) { - return unlink($this->_file_path) && $this->_cookie_destroy(); + return unlink($this->_file_path.$session_id) && $this->_cookie_destroy(); } elseif ($this->_file_path !== NULL) { clearstatcache(); - return file_exists($this->_file_path) - ? (unlink($this->_file_path) && $this->_cookie_destroy()) + return file_exists($this->_file_path.$session_id) + ? (unlink($this->_file_path.$session_id) && $this->_cookie_destroy()) : TRUE; } diff --git a/system/libraries/Session/drivers/Session_memcached_driver.php b/system/libraries/Session/drivers/Session_memcached_driver.php index 318c11afa..8905e8d6f 100644 --- a/system/libraries/Session/drivers/Session_memcached_driver.php +++ b/system/libraries/Session/drivers/Session_memcached_driver.php @@ -133,6 +133,9 @@ class CI_Session_memcached_driver extends CI_Session_driver implements SessionHa { if (isset($this->_memcached) && $this->_get_lock($session_id)) { + // Needed by write() to detect session_regenerate_id() calls + $this->_session_id = $session_id; + $session_data = (string) $this->_memcached->get($this->_key_prefix.$session_id); $this->_fingerprint = md5($session_data); return $session_data; @@ -143,7 +146,23 @@ class CI_Session_memcached_driver extends CI_Session_driver implements SessionHa public function write($session_id, $session_data) { - if (isset($this->_memcached, $this->_lock_key)) + if ( ! isset($this->_memcached)) + { + return FALSE; + } + // Was the ID regenerated? + elseif ($session_id !== $this->_session_id) + { + if ( ! $this->_release_lock() OR ! $this->_get_lock($session_id)) + { + return FALSE; + } + + $this->_fingerprint = md5(''); + $this->_session_id = $session_id; + } + + if (isset($this->_lock_key)) { $this->_memcached->replace($this->_lock_key, time(), 5); if ($this->_fingerprint !== ($fingerprint = md5($session_data))) @@ -189,16 +208,17 @@ class CI_Session_memcached_driver extends CI_Session_driver implements SessionHa if (isset($this->_memcached, $this->_lock_key)) { $this->_memcached->delete($this->_key_prefix.$session_id); - return ($this->_cookie_destroy() && $this->close()); + return $this->_cookie_destroy(); } - return $this->close(); + return FALSE; } // ------------------------------------------------------------------------ public function gc($maxlifetime) { + // Not necessary, Memcached takes care of that. return TRUE; } diff --git a/system/libraries/Session/drivers/Session_redis_driver.php b/system/libraries/Session/drivers/Session_redis_driver.php index ef18defe2..bc6150d2d 100644 --- a/system/libraries/Session/drivers/Session_redis_driver.php +++ b/system/libraries/Session/drivers/Session_redis_driver.php @@ -135,6 +135,9 @@ class CI_Session_redis_driver extends CI_Session_driver implements SessionHandle { if (isset($this->_redis) && $this->_get_lock($session_id)) { + // Needed by write() to detect session_regenerate_id() calls + $this->_session_id = $session_id; + $session_data = (string) $this->_redis->get($this->_key_prefix.$session_id); $this->_fingerprint = md5($session_data); return $session_data; @@ -145,7 +148,23 @@ class CI_Session_redis_driver extends CI_Session_driver implements SessionHandle public function write($session_id, $session_data) { - if (isset($this->_redis, $this->_lock_key)) + if ( ! isset($this->_redis)) + { + return FALSE; + } + // Was the ID regenerated? + elseif ($session_id !== $this->_session_id) + { + if ( ! $this->_release_lock() OR ! $this->_get_lock($session_id)) + { + return FALSE; + } + + $this->_fingerprint = md5(''); + $this->_session_id = $session_id; + } + + if (isset($this->_lock_key)) { $this->_redis->setTimeout($this->_lock_key, 5); if ($this->_fingerprint !== ($fingerprint = md5($session_data))) @@ -190,7 +209,7 @@ class CI_Session_redis_driver extends CI_Session_driver implements SessionHandle return TRUE; } - return FALSE; + return TRUE; } // ------------------------------------------------------------------------ @@ -204,20 +223,17 @@ class CI_Session_redis_driver extends CI_Session_driver implements SessionHandle log_message('debug', 'Session: Redis::delete() expected to return 1, got '.var_export($result, TRUE).' instead.'); } - return ($this->_cookie_destroy() && $this->close()); + return $this->_cookie_destroy(); } - return $this->close(); + return FALSE; } // ------------------------------------------------------------------------ public function gc($maxlifetime) { - // TODO: keys()/getKeys() is said to be performance-intensive, - // although it supports patterns (*, [charlist] at the very least). - // scan() seems to be recommended, but requires redis 2.8 - // Not sure if we need any of these though, as we set keys with expire times + // Not necessary, Redis takes care of that. return TRUE; } -- cgit v1.2.3-24-g4f1b