From: pdontthink <pdontthink@7612ce4b-ef26-0410-bec9-ea0150e637f0>
Date: Tue, 28 Aug 2007 21:31:04 +0000 (+0000)
Subject: Finally fix up session restore functionality.  Move session handling from login.php... 
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=c7ebdfcf0b4f318f9ae50f4da877f9471e20b435;p=squirrelmail.git

Finally fix up session restore functionality.  Move session handling from login.php into init.php and fix the mess in redirect.php.  There are some important notes that need to be reviewed in redirect.php, which I am including here to get your attention: 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 THISsvn diff include/init.php src/login.php src/redirect.php src/compose.php   WHAT HIJACKING ISSUES ARE WE SUPPOSED TO BE PREVENTING HERE?

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

diff --git a/include/init.php b/include/init.php
index cc2b8e43..22696472 100644
--- a/include/init.php
+++ b/include/init.php
@@ -18,6 +18,13 @@
 error_reporting(E_ALL);
 
 
+/**
+ * Make sure we have a page name
+ *
+ */
+if ( !defined('PAGE_NAME') ) define('PAGE_NAME', NULL);
+
+
 /**
  * If register_globals are on, unregister globals.
  * Second test covers boolean set as string (php_value register_globals off).
@@ -199,18 +206,32 @@ if (!isset($session_name) || !$session_name) {
 }
 
 /**
- * if session.auto_start is On then close the session
+ * When on login page or if session.auto_start is On 
+ * we want to destroy/close the session (save off 
+ * possible session restoration values first)
  */
+if (!sqGetGlobalVar('session_expired_post', $sep, SQ_SESSION))
+    $sep = '';
+if (!sqGetGlobalVar('session_expired_location', $sel, SQ_SESSION))
+    $sel = '';
 $sSessionAutostartName = session_name();
 $sCookiePath = null;
-if ((isset($sSessionAutostartName) || $sSessionAutostartName == '') &&
-     $sSessionAutostartName !== $session_name) {
+if (PAGE_NAME == 'login' 
+ || (isset($sSessionAutostartName) && $sSessionAutostartName !== $session_name)) {
     $sCookiePath = ini_get('session.cookie_path');
     $sCookieDomain = ini_get('session.cookie_domain');
     // reset the cookie
     setcookie($sSessionAutostartName,'',time() - 604800,$sCookiePath,$sCookieDomain);
     @session_destroy();
     session_write_close();
+
+    /**
+     * in some rare instances, the session seems to stick
+     * around even after destroying it (!!), so if it does,
+     * we'll manually flatten the $_SESSION data
+     */
+    if (!empty($_SESSION))
+        $_SESSION = array();
 }
 
 /**
@@ -311,7 +332,6 @@ if (! sqgetGlobalVar('squirrelmail_language',$squirrelmail_language,SQ_COOKIE))
  * Do something special for some pages. This is based on the PAGE_NAME constand
  * set at the top of every page.
  */
-if ( !defined('PAGE_NAME') ) define('PAGE_NAME', NULL);
 switch (PAGE_NAME) {
     case 'style':
 
@@ -354,6 +374,13 @@ switch (PAGE_NAME) {
         require(SM_PATH . 'functions/page_header.php');
         require(SM_PATH . 'functions/html.php');
 
+        // put session restore data back into session if necessary
+        if (!empty($sel)) {
+            sqsession_register($sel, 'session_expired_location');
+            if (!empty($sep))
+                sqsession_register($sep, 'session_expired_post');
+        }
+
         // reset template file cache
         //
         $sTemplateID = Template::get_default_template_set();
diff --git a/src/compose.php b/src/compose.php
index bdbc3c40..52f9a489 100644
--- a/src/compose.php
+++ b/src/compose.php
@@ -329,11 +329,8 @@ if (sqsession_is_registered('session_expired_post')) {
      * extra check for username so we don't display previous post data from
      * another user during this session.
      */
-    if ($session_expired_post['username'] != $username) {
-        unset($session_expired_post);
-        sqsession_unregister('session_expired_post');
-        session_write_close();
-    } else {
+    if (!empty($session_expired_post['username']) 
+     && $session_expired_post['username'] == $username) {
         // these are the vars that we can set from the expired composed session
         $compo_var_list = array ('send_to', 'send_to_cc', 'body',
             'startMessage', 'passed_body', 'use_signature', 'signature',
diff --git a/src/login.php b/src/login.php
index 2eaea9e2..8888865b 100644
--- a/src/login.php
+++ b/src/login.php
@@ -30,34 +30,6 @@ require_once(SM_PATH . 'functions/forms.php');
  */
 set_up_language($squirrelmail_language, TRUE, TRUE);
 
-/**
- * In case the last session was not terminated properly, make sure
- * we get a new one, but make sure we preserve session_expired_*
- */
-$sep = '';
-$sel = '';
-sqGetGlobalVar('session_expired_post', $sep, SQ_SESSION);
-sqGetGlobalVar('session_expired_location', $sel, SQ_SESSION);
-
-/* blow away session */
-sqsession_destroy();
-
-/**
- * in some rare instances, the session seems to stick
- * around even after destroying it (!!), so if it does,
- * we'll manually flatten the $_SESSION data
- */
-if (!empty($_SESSION)) {
-    $_SESSION = array();
-}
-
-/* start session and put session_expired_* variables back in session */
-@sqsession_is_active();
-if (!empty($sep) && !empty($sel)) {
-    sqsession_register($sep, 'session_expired_post');
-    sqsession_register($sel, 'session_expired_location');
-}
-
 /**
  * This detects if the IMAP server has logins disabled, and if so,
  * squelches the display of the login form and puts up a message
diff --git a/src/redirect.php b/src/redirect.php
index cc2dde99..6443ce5a 100644
--- a/src/redirect.php
+++ b/src/redirect.php
@@ -71,9 +71,10 @@ $imapConnection = sqimap_login($login_username, $key, $imapServerAddress, $imapP
 /* From now on we are logged it. If the login failed then sqimap_login handles it */
 
 /* regenerate the session id to avoid session hyijacking */
-sqsession_destroy();
-@sqsession_is_active();
-session_regenerate_id();
+//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();
 /**
 * The cookie part. session_start and session_regenerate_session normally set
 * their own cookie. SquirrelMail sets another cookie which overwites the
@@ -148,10 +149,12 @@ if ( sqgetGlobalVar('session_expired_location', $session_expired_location, SQ_SE
         if ($compose_new_win) {
             // do not prefix $location here because $session_expired_location is set to the PAGE_NAME
             // of the last page
-            $redirect_url = $session_expired_location.'.php';
+            $redirect_url = $location . $session_expired_location . '.php';
         } else {
-            $redirect_url = $location.'/webmail.php?right_frame='.urlencode($session_expired_location).'php';
+            $redirect_url = $location . '/webmail.php?right_frame=compose.php';
         }
+    } else {
+        $redirect_url = $location . '/webmail.php?right_frame=' . urlencode($session_expired_location) . '.php';
     }
     unset($session_expired_location);
 }