-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Comments
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. |
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: 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. |
I'm not an expert in cryptography either, so I'd urge on the side of caution and use |
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 |
you might not even need rbytes. Node has randomBytes. |
@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());
};
} |
That sounds reasonable, would you mind submitting a PR for this? Thanks! |
@r3wt @mxstbr I apologize if I stepped on anyone toe here. The 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. |
@sayargh a solution was proposed for that as well. just a different implementation. |
Taken from the PR:
That's definitely very good findings, I appreciate the research! How does that compare to the |
essentially what i proposed for the token implementation:
pros:
cons:
|
👍 Now that i have the greenlight i will submit a PR this evening after work. |
Thanks @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. |
@r3wt I'm not sure if you want to retrieve the stored token. Can you try following approach:
change csrf.js#L21 to: change csrf.js#L64 to: |
Closing as fixed by #2793. |
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
The text was updated successfully, but these errors were encountered: