Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

session validation breaks if token contains the special character "+" #2734

Closed
macsdragon opened this issue May 1, 2016 · 18 comments · Fixed by #2793
Closed

session validation breaks if token contains the special character "+" #2734

macsdragon opened this issue May 1, 2016 · 18 comments · Fixed by #2793
Labels

Comments

@macsdragon
Copy link

macsdragon commented May 1, 2016

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
Copy link
Contributor

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
Copy link

r3wt commented May 7, 2016

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
Copy link
Collaborator

mxstbr commented May 7, 2016

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
Copy link

r3wt commented May 7, 2016

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
Copy link
Contributor

you might not even need rbytes. Node has randomBytes.

@r3wt
Copy link

r3wt commented May 7, 2016

@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
Copy link
Collaborator

mxstbr commented May 8, 2016

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

@r3wt
Copy link

r3wt commented May 8, 2016

@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
Copy link
Contributor

suryagh commented May 9, 2016

@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
Copy link

r3wt commented May 9, 2016

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

@mxstbr
Copy link
Collaborator

mxstbr commented May 9, 2016

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
Copy link

r3wt commented May 9, 2016

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
Copy link
Member

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.

@JedWatson JedWatson reopened this May 13, 2016
@r3wt
Copy link

r3wt commented May 13, 2016

👍 Now that i have the greenlight i will submit a PR this evening after work.

@JedWatson
Copy link
Member

Thanks @r3wt!

@r3wt
Copy link

r3wt commented May 14, 2016

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
Copy link
Contributor

suryagh commented May 14, 2016

@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
Copy link
Contributor

Noviny commented Oct 4, 2017

Closing as fixed by #2793.

@Noviny Noviny closed this as completed Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants