From 5f778d73b41884cebdcee7b95bbe7b07ae57913f Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Mon, 20 Nov 2023 20:58:20 +0100 Subject: [PATCH 1/2] Session: remove checks for matching IP address and user agent This was considered good practice in the days before always-on https, but is considered superfluous today. It even gets in the way of IPv6 privacy extensions, which is the main argument for removing them today. --- models/Session.php | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/models/Session.php b/models/Session.php index 22e0844..ebf5bf3 100644 --- a/models/Session.php +++ b/models/Session.php @@ -12,28 +12,6 @@ class Session { session_start(); - // Resuming an existing session? Check what we know! - if (isset($_SESSION['user_id'], $_SESSION['ip_address'], $_SESSION['user_agent'])) - { - // If we're not browsing over HTTPS, protect against session hijacking. - if (!isset($_SERVER['HTTPS']) && isset($_SERVER['REMOTE_ADDR']) && $_SESSION['ip_address'] !== $_SERVER['REMOTE_ADDR']) - { - $_SESSION = []; - Dispatcher::kickGuest('Your session failed to validate', 'Your IP address has changed. Please re-login and try again.'); - } - // Either way, require re-login if the browser identifier has changed. - elseif (isset($_SERVER['HTTP_USER_AGENT']) && $_SESSION['user_agent'] !== $_SERVER['HTTP_USER_AGENT']) - { - $_SESSION = []; - Dispatcher::kickGuest('Your session failed to validate', 'Your browser identifier has changed. Please re-login and try again.'); - } - } - elseif (!isset($_SESSION['ip_address'], $_SESSION['user_agent'])) - $_SESSION = [ - 'ip_address' => isset($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : '', - 'user_agent' => isset($_SERVER['HTTP_USER_AGENT']) ? $_SERVER['HTTP_USER_AGENT'] : '', - ]; - return true; } From 65ee07d95bf94826958f6e086990f17eb42fa109 Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Mon, 20 Nov 2023 20:59:35 +0100 Subject: [PATCH 2/2] Session: centralise how session tokens are handled --- controllers/Logout.php | 2 +- models/Session.php | 56 ++++++++++++++++++++++++++---------------- 2 files changed, 36 insertions(+), 22 deletions(-) diff --git a/controllers/Logout.php b/controllers/Logout.php index 1980518..fb2489f 100644 --- a/controllers/Logout.php +++ b/controllers/Logout.php @@ -11,7 +11,7 @@ class Logout extends HTMLController public function __construct() { // Clear the entire sesssion. - $_SESSION = []; + Session::clear(); // Back to the frontpage you go. header('Location: ' . BASEURL); diff --git a/models/Session.php b/models/Session.php index ebf5bf3..d1f6e04 100644 --- a/models/Session.php +++ b/models/Session.php @@ -3,22 +3,52 @@ * Session.php * Contains the key class Session. * - * Kabuki CMS (C) 2013-2015, Aaron van Geffen + * Kabuki CMS (C) 2013-2023, Aaron van Geffen *****************************************************************************/ class Session { + public static function clear() + { + $_SESSION = []; + } + public static function start() { session_start(); + if (!isset($_SESSION['session_token_key'], $_SESSION['session_token'])) + self::generateSessionToken(); + return true; } + public static function generateSessionToken() + { + $_SESSION['session_token'] = sha1(session_id() . mt_rand()); + $_SESSION['session_token_key'] = substr(preg_replace('~^\d+~', '', sha1(mt_rand() . session_id() . mt_rand())), 0, rand(7, 12)); + return true; + } + + public static function getSessionToken() + { + if (empty($_SESSION['session_token'])) + trigger_error('Call to getSessionToken without a session token being set!', E_USER_ERROR); + + return $_SESSION['session_token']; + } + + public static function getSessionTokenKey() + { + if (empty($_SESSION['session_token_key'])) + trigger_error('Call to getSessionTokenKey without a session token key being set!', E_USER_ERROR); + + return $_SESSION['session_token_key']; + } + public static function resetSessionToken() { - $_SESSION['session_token'] = sha1(session_id() . mt_rand()); - $_SESSION['session_token_key'] = substr(preg_replace('~^\d+~', '', sha1(mt_rand() . session_id() . mt_rand())), 0, rand(7, 12)); + // Old interface; now always true. return true; } @@ -45,23 +75,7 @@ class Session throw new UserFacingException('Invalid referring URL. Please reload the page and try again.'); } - // All looks good from here! But you can only use this token once, so... - return self::resetSessionToken(); - } - - public static function getSessionToken() - { - if (empty($_SESSION['session_token'])) - trigger_error('Call to getSessionToken without a session token being set!', E_USER_ERROR); - - return $_SESSION['session_token']; - } - - public static function getSessionTokenKey() - { - if (empty($_SESSION['session_token_key'])) - trigger_error('Call to getSessionTokenKey without a session token key being set!', E_USER_ERROR); - - return $_SESSION['session_token_key']; + // All looks good from here! + return true; } }