Skip to content

Commit

Permalink
Don't rotate old tokens by default
Browse files Browse the repository at this point in the history 
Per feedback from @joepie91 -- anyone worried about large volumes of session disk space usage can turn this feature on and turn the number down
  • Loading branch information
@sarciszewski
sarciszewski committed Mar 1, 2015
1 parent 241c942 commit 6ed24cf
Showing 1 changed file with 6 additions and 1 deletion.
7 changes: 6 additions & 1 deletion src/AntiCSRF.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,10 @@ class AntiCSRF
const FORM_TOKEN = '_CSRF_TOKEN';
const SESSION_INDEX = 'CSRF';
const HASH_ALGO = 'sha256';
const RECYCLE_AFTER = 100;
const RECYCLE_AFTER = 65535;

This comment has been minimized.

Sign in to view

Copy link
@nochso

nochso Mar 1, 2015

I'm not worried about disk space for sessions, however Slim's SessionCookie saves $_SESSION data in an encrypted cookie. That would blow up in your face.

Why set such a high number and disable it? If you've generated more than a 100 new tokens, I doubt the user would miss having the 101th token around.

I know I can change it, just wondering though.

This comment has been minimized.

Sign in to view

Copy link
@sarciszewski

sarciszewski Mar 1, 2015

Author Contributor

slimphp/Slim#1034

Also, see in a later commit that I added a reconfigure() method.

This comment has been minimized.

Sign in to view

Copy link
@joepie91

joepie91 Mar 1, 2015

@nochso Regarding my feedback:

It is not uncommon for users to have a form in one tab and continue browsing the site in another tab, potentially generating hundreds of CSRF tokens. If the user only later comes back to that tab, enters something into the form and submits it, they will be faced with a CSRF error and - depending on the phase of the moon and how their browser is feeling that day - potentially lose their form input.

The idea behind disabling this by default is that, at worst, it will inconvenience the developer (due to disk space usage or weird cookie schemes not working) - ie. the person who can notice the issue and solve it. Enabling it by default makes the worst case that it will annoy the user, who will likely never report it or even know where to report it - this is a considerably worse outcome.

It therefore makes more sense to disable this by default and document how to enable it, along with the trade-offs involved - this way, a clueless developer who doesn't read the documentation will only inconvenience himself, never the users.

I should also note that this is yet another reason why Slim's session storage is an awful idea to have as a default.

This comment has been minimized.

Sign in to view

Copy link
@nochso

nochso Mar 1, 2015

Regardless of Slim, how about also being able to set a maximum age?

I'd probably set that to session.cookie_lifetime or ballpark it when set to 0. In my opinion days old tokens should not be valid and I don't care when a user submits a form he opened days ago.

This comment has been minimized.

Sign in to view

Copy link
@joepie91

joepie91 Mar 1, 2015

You may not care, but your user will. There's no fundamental reason why tokens should expire after a few days.

This comment has been minimized.

Sign in to view

Copy link
@nochso

nochso Mar 1, 2015

I was thinking along the lines of closing the window a leaked token might get used, but I suppose that's not necessary?

Anyway, I still think $expire_old should be true by default to avoid eventually serializing arrays with more than 64k elements.

This comment has been minimized.

Sign in to view

Copy link
@joepie91

joepie91 Mar 1, 2015

No, that's not a problem. The idea of CSRF tokens is that they rely on a $remoteSite being unable to make requests to a $targetSite on behalf of $user, and be able to read the response. Basically, the CSRF token is protected by that barrier - you can't make cross-domain AJAX requests by default, and you can load a URL in an image tag but you won't be able to read the response.

In that scenario, how old a token is doesn't matter - you just need to prevent people from 'guessing' them through brute force. A rate limiter is a much better solution for that - with a token expiry, you'd still have no idea how far an attacker could bruteforce before it expires, so it wouldn't provide you any security with certainty.

public static $hmac_ip = true;
public static $expire_old = false;

/**
* Insert a CSRF token to a form
Expand Down Expand Up @@ -188,6 +189,10 @@ private static function generateToken($lockto)
*/
private static function recycleTokens()
{
if (!self::$expire_old) {
// This is turned off.
return;
}
// Sort by creation time
\uasort($_SESSION[self::SESSION_INDEX], function($a, $b) {
return $a['created'] - $b['created'];
Expand Down

0 comments on commit 6ed24cf

Please sign in to comment.