From 1f80d9f527d2cc2933ee7040aecba908692a20ac Mon Sep 17 00:00:00 2001 From: pdontthink Date: Mon, 11 May 2009 22:50:16 +0000 Subject: [PATCH] Always generate $base_uri for every page request as opposed to doing it only on some pages. Always regenerate session ID at login to prevent session fixation by an attacker who has set a malicious cookie on the client browser. Try to clean up extraneous cookies, such as ones some browsers might actually obey from the src/ directory. Thanks to Tomas Hoger. (CVE-2009-1580) git-svn-id: https://svn.code.sf.net/p/squirrelmail/code/trunk/squirrelmail@13677 7612ce4b-ef26-0410-bec9-ea0150e637f0 --- doc/ChangeLog | 5 +++++ functions/display_messages.php | 5 ++--- functions/global.php | 22 +++++++++++++++++++++- src/redirect.php | 22 +++++++++++++++++----- src/signout.php | 6 ------ 5 files changed, 45 insertions(+), 15 deletions(-) diff --git a/doc/ChangeLog b/doc/ChangeLog index 730473d6..a3a95886 100644 --- a/doc/ChangeLog +++ b/doc/ChangeLog @@ -303,6 +303,11 @@ Version 1.5.2 - SVN [also CVE-2009-1578] - Fixed unsanitized shell command in example IMAP username mapping function (map_yp_alias) (Thanks to Niels Teusink). [CVE-2009-1579] + - Fixed session fixation issues where someone who can modify a user's + cookies could gain control of their login session. The SquirrelMail + base URI is now uniformly generated, extraneous cookies are cleaned + up and session IDs are regenerated upon every login (Thanks to Tomas + Hoger). [CVE-2009-1580] Version 1.5.1 (branched on 2006-02-12) -------------------------------------- diff --git a/functions/display_messages.php b/functions/display_messages.php index 1e686b19..859b9b48 100644 --- a/functions/display_messages.php +++ b/functions/display_messages.php @@ -37,6 +37,7 @@ function error_message($message, $mailbox, $sort, $startMessage) { * Displays error message * * Second argument ($color array) is changed to boolean $return_output as of 1.5.2. + * * @param string $message error message * @param boolean $return_output When TRUE, output is returned to caller * instead of being sent to browser (OPTIONAL; @@ -57,9 +58,7 @@ function plain_error_message($message, $return_output=FALSE) { */ function logout_error( $errString, $errTitle = '' ) { global $frame_top, $org_logo, $org_logo_width, $org_logo_height, $org_name, - $hide_sm_attributions, $squirrelmail_language, $oTemplate; - - $base_uri = sqm_baseuri(); + $hide_sm_attributions, $squirrelmail_language, $oTemplate, $base_uri; $login_link = array ( 'URI' => $base_uri . 'src/login.php', diff --git a/functions/global.php b/functions/global.php index 4e81a076..7adc7925 100644 --- a/functions/global.php +++ b/functions/global.php @@ -442,9 +442,29 @@ function sqsession_destroy() { global $base_uri, $_COOKIE, $_SESSION; - if (isset($_COOKIE[session_name()]) && session_name()) sqsetcookie(session_name(), $_COOKIE[session_name()], 1, $base_uri); + if (isset($_COOKIE[session_name()]) && session_name()) { + sqsetcookie(session_name(), $_COOKIE[session_name()], 1, $base_uri); + + /* + * Make sure to kill /src and /src/ cookies, just in case there are + * some left-over or malicious ones set in user's browser. + * NB: Note that an attacker could try to plant a cookie for one + * of the /plugins/* directories. Such cookies can block + * access to certain plugin pages, but they do not influence + * or fixate the $base_uri cookie, so we don't worry about + * trying to delete all of them here. + */ + sqsetcookie(session_name(), $_COOKIE[session_name()], 1, $base_uri . 'src'); + sqsetcookie(session_name(), $_COOKIE[session_name()], 1, $base_uri . 'src/'); + } + if (isset($_COOKIE['key']) && $_COOKIE['key']) sqsetcookie('key','SQMTRASH',1,$base_uri); + /* Make sure new session id is generated on subsequent session_start() */ + unset($_COOKIE[session_name()]); + unset($_GET[session_name()]); + unset($_POST[session_name()]); + $sessid = session_id(); if (!empty( $sessid )) { $_SESSION = array(); diff --git a/src/redirect.php b/src/redirect.php index ff00fbdd..6bdb883c 100644 --- a/src/redirect.php +++ b/src/redirect.php @@ -70,11 +70,23 @@ if ($force_username_lowercase) { $imapConnection = sqimap_login($login_username, $key, $imapServerAddress, $imapPort, 0); /* From now on we are logged it. If the login failed then sqimap_login handles it */ -/* regenerate the session id to avoid session hyijacking */ -//FIXME! IMPORTANT! SOMEONE PLEASE EXPLAIN THE SECURITY CONCERN HERE; THIS session_destroy() BORKS ANY SESSION INFORMATION ADDED ON THE LOGIN PAGE (SPECIFICALLY THE SESSION RESTORE DATA, BUT ALSO ANYTHING ADDED BY PLUGINS, ETC)... I HAVE DISABLED THIS (AND NOTE THAT THE LOGIN PAGE ALREADY EXECUTES A session_destroy() (see includes/init.php)), SO PLEASE, WHOEVER ADDED THIS, PLEASE ANALYSE THIS SITUATION AND COMMENT ON IF IT IS OK LIKE THIS!! WHAT HIJACKING ISSUES ARE WE SUPPOSED TO BE PREVENTING HERE? -//sqsession_destroy(); -//@sqsession_is_active(); -//session_regenerate_id(); +/** + * Regenerate session id to make sure that authenticated session uses + * different ID than one used before user authenticated. This is a + * countermeasure against session fixation attacks. + * NB: session_regenerate_id() was added in PHP 4.3.2 (and new session + * cookie is only sent out in this call as of PHP 4.3.3), but PHP 4 + * is not vulnerable to session fixation problems in SquirrelMail + * because it prioritizes $base_uri subdirectory cookies differently + * than PHP 5, which is otherwise vulnerable. If we really want to, + * we could define our own session_regenerate_id() when one does not + * exist, but there seems to be no reason to do so. + */ +sqsession_is_active(); +if (function_exists('session_regenerate_id')) { + session_regenerate_id(); +} + /** * The cookie part. session_start and session_regenerate_session normally set * their own cookie. SquirrelMail sets another cookie which overwites the diff --git a/src/signout.php b/src/signout.php index 5c6238ff..50d11722 100644 --- a/src/signout.php +++ b/src/signout.php @@ -32,12 +32,6 @@ if (!isset($frame_top)) { $frame_top = '_top'; } -/* If a user hits reload on the last page, $base_uri isn't set - * because it was deleted with the session. */ -if (! sqgetGlobalVar('base_uri', $base_uri, SQ_SESSION) ) { - $base_uri = sqm_baseuri(); -} - $login_uri = 'login.php'; do_hook('logout', $login_uri); -- 2.25.1