Keystone-classic: session validation breaks if token contains the special character "+"

[macsdragon]

keystonejs occasionally throws a csrf failure (in console) and "There was a problem with your request, please try again". in the admin interface for instance hen one tries to delete an item. it is a very tricky error that is only appearing occasionally seemingly out of the blue and bugging me for quite some time .

I did some research on it and i think I have found the problem. i noticed is that the csfr validate function fails if the generated token contains one ore more of the special character of "+".
the key
"5hgttSHHmWWMtpe8WGgyg/QatbkLHo5+rfEQs="
will be received in the token validate function as
5hgttSHHmWWMtpe8WGgyg/QatbkLHo5 rfEQs=
containing a space instead of the +

It seems like some kind of url de or encoding problem
this happens reliably each time a + character was set in the key
I am not sure about other charachters though

[geloescht]

Makes sense, + is used in URLs to encode whitespace. It would have to be escaped as %2b on the client or a different encoding than base64 should be used. I read that there is something called base64url.

[r3wt]

i encountered this problem as a jr developer, and the recommendation of the senior lead was to just replace the '+' with '' when generating the token, which is what i've always done:

https://github.com/r3wt/phpBase/blob/master/App/App.php#L74

here:
https://github.com/keystonejs/keystone/blob/master/lib/security/csrf.js#L21 either you can:

use decodeURIComponent on client and encodeURIComponent here in the server, or you can simply remove the + and the = signs. IMO removing them has no effect on the attack surface, however i'm by no means a mathematician or cryptography expert. in my experience we just removed them.

proposed fix:

function tokenize (salt, secret) {
    return salt + crypto.createHash('sha1').update(salt + secret).digest('base64').replace(/(\+|=+)/g,'');
}

not a pretty regex, someone else can optimize?

aside: i don't like that sha1 is being used. i would use this package: https://github.com/akdubya/rbytes and just pull 16 random bytes from the OS, thats pretty much the standard these days.

[mxstbr]

I'm not an expert in cryptography either, so I'd urge on the side of caution and use d/encodeURIComponent instead of removing potentially important bytes. (no idea if they really are though…)

[r3wt]

the bytes are irrelevant. the only thing that matters is that the string is long enough, and that the comparison is safe against timing attacks. therefore the optimal solution is the cheapest hash you can create with the highest amount of entropy. which is why i recommend pulling 16 bytes from OS and base64ing it, which generates plenty of entropy with almost 0 cost. so in the spirit of an optimal solution, it would be pointless to encode/decode when a simple string replacement has no effect on the security of the implementation, and only has to be executed once per session whereas you would have to decodeURICcomponent on the server for each request.

[morenoh149]

you might not even need rbytes. Node has randomBytes.

[r3wt]

@morenoh149 nice find. now we need timing attack safe string comparison. FR/PR open in node here

in the meantime we can use:

if(!crypto.hasOwnProperty('timingSafeEqual')){

    crypto.timingSafeEqual = function(a, b) {
      var key = new Buffer(32);//buffer is deprecated according to docs.
      for (var i = 0; i < 32; i += 4) {
        key.writeFloatBE(Math.random(), i);
      }
      var ah = crypto.createHmac('sha256', key).update(a);
      return ah.validate(crypto.createHmac('sha256', key).update(b).digest());
    };

}

[mxstbr]

That sounds reasonable, would you mind submitting a PR for this? Thanks!

[r3wt]

@mxstbr hmm, it looks like @suryagh has submitted a different solution for this problem. In interest of not stepping on his toes, perhaps we should discuss further?

[suryagh]

@r3wt @mxstbr I apologize if I stepped on anyone toe here. The pr @mxstbr mentioned above was for the timing safe equality check, right? Which I think is still a problem.

The PR #2793 is for the original encoding issue for which a solution has not been arrived in this thread yet. I also wanted to capture the benchmark findings in a summarized fashion, hence thought of putting it in a PR.

[r3wt]

@sayargh a solution was proposed for that as well. just a different implementation.

[mxstbr]

Taken from the PR:

If we use the hex encoding approach, then the token will be always 50 chars, however, the performance is 40% faster

That's definitely very good findings, I appreciate the research! How does that compare to the randomBytes version?

[r3wt]

essentially what i proposed for the token implementation:

1. base64 encode 16 random bytes (obtained from crypto.randomBytes(16) and remove any + or = characters.
2. use crypto.timingSafeEquals to compare the stored token vs the supplied token.

pros:

• eliminates the need for any encoding/decoding clientside/serverside.
• 64bits of cheap random data, base64 encoded, which is a cheap operation.
• non-deterministic token length (because we remove + and = symbols from token)

cons:

• possible that crypto.randomBytes() could block. its rare but it can happen.

[JedWatson]

I'm down with your solution @r3wt. Just merged #2793 because I wanted to get a fix in but would definitely appreciate a PR with your more comprehensive fix as proposed above.

[r3wt]

:+1: Now that i have the greenlight i will submit a PR this evening after work.

[JedWatson]

Thanks @r3wt!

[r3wt]

Ok, so my fix is failing. i've tried a number of ways to get at the stored token and can't seem to find where it is stored. if someone can clarify where I can find that token I would appreciate it.

[suryagh]

@r3wt I'm not sure if you want to retrieve the stored token. Can you try following approach:

var escape = require('base64-url').escape
var tsscmp = require('tsscmp')

change csrf.js#L21 to:
return escape(salt + crypto.createHash('sha1').update(salt + secret).digest('base64'));

change csrf.js#L64 to:
return tsscmp(token, tokenize(token.slice(0, exports.SECRET_LENGTH), req.session[exports.SECRET_KEY]));

[Noviny]

Closing as fixed by #2793.