tj/node-cookie-signature

Weak HMAC comparison

Closed this issue · 4 comments

ret2kw commented

Happened to take a peek at your code in the process of looking at an app using express-session.

https://github.com/tj/node-cookie-signature/blob/master/index.js#L42

Hashing the resulting hmac's doesn't increase the security, in fact it could probably be argued it lowers it as you are taking a sha256 hmac and making it rely on the weaker sha1 hash. I don't think this leads to an exploitable bug, but you should really just compare the resulting mac's directly to each other without messing with them.

edit: Read the rationalizations on the other closed tickets for people who pointed out sha1

Appreciate it is somewhat new, but any reason not to use crypto.timingSafeEqual() and get rid of the sha1 to avoid any timing side-channels?

https://nodejs.org/api/crypto.html#crypto_crypto_timingsafeequal_a_b

natevw commented

Glad you found the other threads. Having a built-in crypto.timingSafeEqual is exactly what this has needed to put the discussions to rest. I've re-titled this issue and hopefully we can get a patch to use that when available, and the old HMAC trick (with better comments this time :-) as a fallback for older versions.

  • 👍2
AnthonyYY commented

can anyone explain to me about what the sha1 function is used for,is it nessary?

natevw commented

I think this has been addressed so I'll close this thread out, let me know if not!