Session should not be destoryed with window close

[HighlyFavoredBA]

With Version 1.4.0, and Elastic, I noticed that Firefox, and most phone browsers log me out of Email when I close, and re open the browsers, if I restart my phone, etc., and that was also happening on my last phone that I recently had.

In a dated R C version install that I have, the (melanie2_larry_mobile) skin has been great about keeping me logged in! :--)

I prefer 7200 minutes of no activity.

// Session lifetime in minutes
$config['session_lifetime'] = 7200;

[alecpl]

What's your point exactly?

[HighlyFavoredBA]

Anybody that has a session span of more than a few hours would prefer to not be auto logged out, simply from restarting phones, etc.! Lots of Email software are excellent about keeping users logged in. :--)

[HighlyFavoredBA]

My 1.2.7 install with the (melanie2_larry_mobile) skin is great at keeping me logged in with all the mobile browsers that I've tested! As long as the (melanie2_larry_mobile) skin keeps running, I plan to keep using that on the side, until more of the kinks are removed from the 1.4 versions, and the Elastic skin.

Is the auto logging out when closing mobile browsers, a 1.4.* issue, or is it a Elastic skin issue? :--)

[alecpl]

I just noticed that closing the browser window ends a session, or it looks like that, because when you open it again you'll be "forced" to log in again. I don't think this is intentional. I agree that it used to work differently before. This need to be investigated.

[alecpl]

I did some investigation. I thought it might be related to 403d845, but if I comment out the added line the behavior is the same. So, maybe something has changed in PHP or browsers recently. I cannot find another change that would be related.

Here's what I had to do to make it working as expected:

--- a/program/lib/Roundcube/rcube.php
+++ b/program/lib/Roundcube/rcube.php
@@ -463,8 +463,7 @@ class rcube
             ini_set('session.gc_maxlifetime', $lifetime * 2);
         }
 
-        // set session cookie lifetime so it never expires (#5961)
-        ini_set('session.cookie_lifetime', 0);
+        ini_set('session.cookie_lifetime', $lifetime);
         ini_set('session.cookie_secure', $is_secure);
         ini_set('session.name', $sess_name ?: 'roundcube_sessid');
         ini_set('session.use_cookies', 1);
diff --git a/program/lib/Roundcube/rcube_session.php b/program/lib/Roundcube/rcube_session.php
index 59522a9d7..e00a36c25 100644
--- a/program/lib/Roundcube/rcube_session.php
+++ b/program/lib/Roundcube/rcube_session.php
@@ -694,7 +694,7 @@ abstract class rcube_session
     public function set_auth_cookie()
     {
         $this->cookie = $this->_mkcookie($this->now);
-        rcube_utils::setcookie($this->cookiename, $this->cookie, 0);
+        rcube_utils::setcookie($this->cookiename, $this->cookie, time() + $this->lifetime);
         $_COOKIE[$this->cookiename] = $this->cookie;
     }
 

but I feel I might be missing something. @thomascube I need a second pair of eyes to look at this.

[HighlyFavoredBA]

Great progress! :--)

[thomascube]

Roundcube's session time is by default kept short in order to prevent session hijacking. While desktop browsers keep on sending keep-alive requests to keep the session alive on the server, this might not be the case for mobile browsers when the app is in the background. rcube_utils::setcookie() omits the expiration date/time part of a cookie string which makes it a session cookie that is deleted when the browser closes. Again, this code is old and was made for desktop browsers. Mobile browsers may behave differently, I'm not quite sure. However, with regards to make Roundcube a PWA, the session lifetime and cookie handling may need a general refactoring.

[thomascube]

To make a long story short: I think the diff you pasted makes sense. If we compute a session lifetime on the server, we might as well send that as an expiration date with the cookie string.

[alecpl]

Patch applied. Fixed.

[alecpl]

I had to revert the commit, because it does not work. The session is not kept alive. It requires some more work.

[mkllnk]

Interesting, I used a local patch for the last two years which is very similar to yours @alecpl and it works. I will open a pull request.

PS: It was on my todo list to contribute that patch for two years as well. ;-) Today I finally looked into it and discovered that you have been working on it.

[mkllnk]

Okay, a pull request will take a little bit longer. I think we have some historically grown confusion here. We have two different cookies.

1. roundcube_sessid is set for the login. The expiry is currently set to 0 which means that it will expire when the session ends, i.e. the browser is closed.
   

   roundcubemail/program/lib/Roundcube/rcube.php

   Line 467 in 3e9aefc

   ini_set('session.cookie_lifetime', 0);

2. roundcube_sessauth is used when logged in. The expiry is currently set to 0 which means that it will expire when the session ends, i.e. the browser is closed.
   

   roundcubemail/program/lib/Roundcube/rcube_session.php

   Line 697 in 3e9aefc

   rcube_utils::setcookie($this->cookiename, $this->cookie, 0);

In addition, we have the config variable session_lifetime which defaults to 10 minutes. And the check_auth method, which checks if a user's login is still valid, verifies that the cookie has a maximum age of three time slots. One time slot is half the session lifetime, so 5 minutes by default. So cookies set within the last 10-15 minutes are seen as valid. That means, if a mobile browser session is inactive in the background for longer than 15 minutes the user is kicked out anyway. This mechanism is important to invalidate old stolen cookies.

roundcubemail/program/lib/Roundcube/rcube_session.php

Lines 133 to 134 in 3e9aefc

	$lifetime = $this->config->get('session_lifetime', 1) * 60;
	$this->set_lifetime($lifetime);

roundcubemail/program/lib/Roundcube/rcube_session.php

Lines 593 to 600 in 3e9aefc

	public function set_lifetime($lifetime)
	{
	$this->lifetime = max(120, $lifetime);
	// valid time range is now - 1/2 lifetime to now + 1/2 lifetime
	$now = time();
	$this->now = $now - ($now % ($this->lifetime / 2));
	}

roundcubemail/program/lib/Roundcube/rcube_session.php

Lines 668 to 681 in 3e9aefc

	if ($result && $this->_mkcookie($this->now) != $this->cookie) {
	$this->log("Session auth check failed for " . $this->key . "; timeslot = " . date('Y-m-d H:i:s', $this->now));
	$result = false;
	// Check if using id from a previous time slot
	for ($i = 1; $i <= 2; $i++) {
	$prev = $this->now - ($this->lifetime / 2) * $i;
	if ($this->_mkcookie($prev) == $this->cookie) {
	$this->log("Send new auth cookie for " . $this->key . ": " . $this->cookie);
	$this->set_auth_cookie();
	$result = true;
	}
	}
	}

Another line of security the setting how long PHP keeps sessions for. We tell PHP to remove all sessions older than twice the session lifetime. But there may be cases when PHP doesn't accept a configuration here. Or, for whatever reason, the garbage collector isn't run and sessions aren't cleared up.

Proposed changes

1. Let roundcube_sessid expire after session lifetime. We could reduce this since it's only the login form and people shouldn't spend much time there. But people may leave the browser tab open for a few hours or days before logging in.
2. Let roundcube_sessauth expire after three time slots which is 1.5 times the session lifetime. I think that the additional time slot isn't necessary but maybe it's catching some edge cases?

@alecpl I don't know why your patch didn't work. Can you tell what went wrong? What did you observe?

If you agree, I will create a pull request with the above changes.

[alecpl]

If we set expiration time on the cookie we have to refresh the cookie (push the expiration time) on keep-alive request. It looked to me that it didn't happen. I didn't have time to investigate yet.

[mkllnk]

Roundcube doesn't send a new cookie on every request. If the current timeslot matches then it leaves the cookie as is because it wouldn't change. A timeslot is half the session lifetime long. So by default, a timeslot is five minutes and you need to wait five minutes to get a new cookie.

[mkllnk]

I found two more issues:

1. Firefox dev tools don't always show the right expiration time. I had a case where I adjusted the session lifetime, Roundcube set a new cookie but Firefox still displayed the old expiry date. Closing the dev tools and re-opening them (while keeping the RC tab open) helped to refresh the date.
2. We refresh the auth cookie regularly but the session cookie is handled by PHP (I believe) and is not updated. So with your patch, the session cookie would expire after ten minutes and we are logged out even though we still have a valid auth cookie.

[mkllnk]

The more I think about it, I'm wondering if we need two configuration variables. It would be nice to keep the current behaviour for backwards compatibility. But it would also be nice to allow for browser restarts and have an actual expiry for cookies.

Current logic

• Cookies are kept as long as the browser stays open.
• Authentication is roughly valid for the session_lifespan (10 minutes) and is renewed in active browser windows.

Expiry of the session cookie

Some people leave their browser open for months. It would be nice to force a log in after a while. Setting PHP's session.cookie_lifetime would do that for us. I personally would like to configure that to one month. So one month after login, the session cookie expires and the user has to log in again. This should be configurable.

Expiry of inactive sessions

The current authentication logic kicks people out when they have been inactive for longer than session_lifespan. That seems to be very useful logic. But we can also limit the cookie expiry to that time.

[alecpl]

I tested Roundcube 1.3 and 1.2 and the behavior is the same. So, I was wrong that it is a regression. Not a bug, but feature request.

[HighlyFavoredBA]

When I used RoundCubePlus.com Skins, and when using the (melanie2_larry_mobile), I don't recall being logged out if I closed a window, Etc. I still use (melanie2_larry_mobile) with one of my sites, and I'm almost always logged in. :--)

[HighlyFavoredBA]

Is this Resolved yet? I see that 1.4.5 Just was released. :-)

[mkllnk]

No, this was not resolved.

It's relatively easy to give session cookies an expiry date. In that case they persist browser restarts but then they can disappear at an inconvenient time in the middle of a session when the expiry date is reached.

To solve this properly, we need to rewrite the way session cookies are set and renew them regularly like the auth cookie.

As a workaround, you can set a cookie expiration date in a hundred years. But that would open up an attack vector where someone gains access to an old hard drive or a backup, extracts the cookies and uses them for to take over the session. It's very unlikely but we should make email as secure as possible.

[HighlyFavoredBA]

One of the phone browsers that I use is keeping me logged in, even when I change internet connections, and turn my phone off, and back on. So, I should be okay with waiting. 😎

[exi]

Any updates on this? #7709 has been open for 3 years now and seems like a sensible PR.

@alecpl opinions?

[Neustradamus]

@HighlyFavoredBA: Like @exi, I would like news about this PR, thanks in advance.

[exi]

A year later and I'm still manually patching this into my docker containers to not be auto-logged out all the time :/

[edgecase14]

FYI, as I mentioned in #9325, we had a cyber insurance policy quote denied, in part because their vulnerability scan flagged the non-expiring cookie as a risk. They also feature "Business Email Compromise (BEC)" as a large risk for Funds Transfer Fraud (FTF) on the front page of their advertising. Their TOP recommendation to reduce cost and risk is MFA. So I guess I would want to understand, is how does cookie based (possibly infinite) session relate to MFA -or- password based email security?
BTW, I am pursuing TLS client certificate authentication, with hardware backed keys (Yubikey or Windows Virtual Smart Card, Android StrongBox, or iPhone Secure Enclave), so this will soon not be an issue for us. I am hoping that even with a limited lifetime cookie, repeated logins won't be necessary, or will at least be 1 click with no username/password entry.

[exi]

@edgecase14 Making it configurable and setting the default to a limited lifetime but keeping infinite lifetime an option would make everyone happy.