Opened 3 years ago

Closed 3 years ago

#1487829 closed Bugs (fixed)

Token requirement on login page

Reported by: rosali Owned by: thomasb
Priority: 5 Milestone: 0.6-beta
Component: Core functionality Version: 0.5.1
Severity: normal Keywords:
Cc:

Description

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).

Change History (16)

comment:1 Changed 3 years ago by alec

This might be a duplicate of #1487792

Last edited 3 years ago by alec (previous) (diff)

comment:2 follow-up: Changed 3 years ago by alec

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

comment:3 in reply to: ↑ 2 Changed 3 years ago by 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.

comment:4 Changed 3 years ago by thomasb

  • Owner set to 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.

comment:5 Changed 3 years ago by 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)?

comment:6 Changed 3 years ago by alec

Maybe we could return true from rcmail::check_request() when $_SESSION['request_tokens'] is empty?

Version 1, edited 3 years ago by alec (previous) (next) (diff)

comment:7 Changed 3 years ago by 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?

comment:8 follow-up: Changed 3 years ago by alec

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

comment:9 Changed 3 years ago by 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.

comment:10 in reply to: ↑ 8 Changed 3 years ago by 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.

comment:11 Changed 3 years ago by thomasb

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in [ec045b0a]

comment:12 follow-up: Changed 3 years ago by 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.

comment:13 Changed 3 years ago by rosali

  • Resolution fixed deleted
  • Status changed from closed to reopened

see comment above - forgot to re-open.

comment:14 follow-up: Changed 3 years ago by 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.

comment:15 in reply to: ↑ 12 Changed 3 years ago by 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.

comment:16 in reply to: ↑ 14 Changed 3 years ago by thomasb

  • Resolution set to fixed
  • Status changed from reopened to closed

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].

Note: See TracTickets for help on using tickets.