Always generate $base_uri for every page request as opposed to doing it only on some...

git-svn-id: https://svn.code.sf.net/p/squirrelmail/code/trunk/squirrelmail@13677 7612ce4b-ef26-0410-bec9-ea0150e637f0

diff --git a/doc/ChangeLog b/doc/ChangeLog
index 730473d639f012e45ad7f47c420b0b56d0e729df..a3a95886bc79ee9e349248fe07d5dbd18e963f3f 100644 (file)
--- 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 1e686b198c75d40fee7efb68223d02d3cb8c6b8c..859b9b48bf775e5b560573ee1e3dc513aa9206a2 100644 (file)
--- 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 4e81a0762f7be5cc3680ba5c1e195385731c3b1f..7adc7925ae58d66bd749d75fc7b8a16287a47cd2 100644 (file)
--- 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 ff00fbdd9da80952ae6a8de5ae943a764ff06d3d..6bdb883cb32da5f384516b30e5846a00e15d48f3 100644 (file)
--- 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 5c6238ff7e196e92ad5cb4df873e390490957b7c..50d1172236a031020a96a8e3b6ac2fd3caa07a1b 100644 (file)
--- 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);