Token requirement on login page

[rosali]

When logging out and let the login page open until the session expires then error $messagesinvalidrequest? is raised because the CFSR token is not available in the session anymore.

I assume that keep-alive request should be triggered but it look like it does not send requests to keep the session alive (the timestamp in the session table does not change).

[alec]

This maight be a duplicate of #1487792

[alec]

I think we should just disable CSRF prevention code on login.

[thomasb]

Replying to alec:

I think we should just disable CSRF prevention code on login.

There was a reason why we added CSRF on login (it was a report from a security testing team). Removing it again is not a solution.

[thomasb]

I see the problem. Keep-alives are not sent on the login page but the session record where the request token is stored can expire and be removed.

[alec]

Hmmm... ok. Maybe instead of checking the token on login action, we should just check if the session is active (user is logged in)?

[alec]

Maybe we could return true from rcmail::check_request() when $_SESSION['request_tokens'] is empty or something like this patch:

--- index.php   (wersja 4607)
+++ index.php   (kopia robocza)
@@ -75,7 +75,7 @@
 
 // try to log in
 if ($RCMAIL->task == 'login' && $RCMAIL->action == 'login') {
-  $request_valid = $_SESSION['temp'] && $RCMAIL->check_request(RCUBE_INPUT_POST, 'login');
+  $request_valid = empty($RCMAIL->user->ID) || ($_SESSION['temp'] && $RCMAIL->check_request(RCUBE_INPUT_POST, 'login'));
 
   // purge the session in case of new login when a session already exists 
   $RCMAIL->kill_session();

I think token check is not needed when user is not logged in.

[alec]

BTW, in case of CSRF attack user will be logged out, which itself is an issue (line 81 of index.php). Am I right?

[alec]

I've seen [32b11d32], but I think this doesn't solve related issue when user removes the session cookie #1487792.

[thomasb]

The goal of this CSRF is to prevent that a foreign site can trigger a POST request to enforce a login with credentials that don't belong to the actual user. The combination of session cookie and request token ensures that the POST request is submitted from our very own login form. If we allow an empty $_SESSION['request_tokens'] we cannot distinguish between requests sent from our login page or from somewhere else (no session initialized) and the CSRF protection is gone.

But there's an alternative to storing these request tokens in session data and thus requiring keep-alive signals: request tokens could be derived from the session cookie and the request action and salted with the des_key which should be unique for every Roundcube installation and only known to the server. This way a request token can be validated against the submitted data and no storage in session would be necessary. And it's not possible for a foreign site to generate a valid token. Of course there are disadvantages:

• Request tokens for the same action are always the same
• They cannot be used for one-time submissions and be invalidated after check (currently not a requirement)
• They don't expire with the session data on the server

Despite these disadvantages these stateless request tokens should prevent CSRF to a sufficient extent.

[thomasb]

Replying to alec:

I've seen [32b11d32], but I think this doesn't solve related issue when user removes the session cookie #1487792.

Right, it doesn't address that issue. Actually that isn't an issue/bug to me. If somebody clears cookies AFTER loading a page, things can go wrong. Allowing this would contradict with CSRF prevention.

[thomasb]

Fixed in [ec045b0a]

[rosali]

As far as I can see, your change makes it impossible to overwrite the token by a plugin. Please provide at least a hook there.

[rosali]

see comment above - forgot to re-open.

[brandond]

Even with the fix in [ec045b0a] I am still seeing the "invalid request (no data submitted)" message the first time I attempt to log in after clearing cookies. It appears that the session is not getting created properly the first time - roundcube_sessid is set, but roundcube_sessauth is not.

[thomasb]

Replying to rosali:

As far as I can see, your change makes it impossible to overwrite the token by a plugin. Please provide at least a hook there.

Why would you like to overwrite the token? The result of the request check is passed to the authenticate hook (field valid) and there you can ignore the CSRF check by setting valid to true.

[thomasb]

Replying to brandond:

Even with the fix in [ec045b0a] I am still seeing the "invalid request (no data submitted)" message the first time I attempt to log in after clearing cookies. It appears that the session is not getting created properly the first time - roundcube_sessid is set, but roundcube_sessauth is not.

Fixed in [c9f2c470].