-
Notifications
You must be signed in to change notification settings - Fork 34
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
Fix for timing attacks on MAC verification. #14
Conversation
I do not like this fix. I would instead like to see us just write a safe_str_cmp function that doesn't exit early on first mismatch but instead runs to full length of values. Sounds like a better solution than the double sign. Can someone who knows more about timing attacks comment? |
@defunctzombie See the original pull request; @Bren2010 felt this was the cleanest solution. I'm inclined to agree: IMO any |
The issue is that it doesn't do the same comparison that it replaced. It signs the val for whatever reason. I think it was reverted because it broke stuff. The comparison being done needs to remain and a proper comparison function used. We had the same thing come up in bcrypt. To avoid this all you need to ensure is that you loop to the end of both strings methinks. |
I didn't realize the other one changed the digest length, that may have explained our issues, whatever they were haha I forget now that was so long ago. Maybe the node security guys can comment on the best fix since that's their domain, at a glance it seems more like we're chasing issues that (barely) exist, though of course if it is a possible issue we might as well fix it |
@defunctzombie Think about it. This only changes the verification part, not the generation part. So if it did "break stuff" that would be blatantly obvious in use — what broke stuff was a separate change to 512-byte hash. |
Yea, I see that now. I still think we can just use a good string compare over a double sign. We shall see what other comments we get but I would favor that approach. |
Here's how @Bren2010 originally explained it:
I don't have further opinion on this beyond hoping a fix for this (and that annoying missing repository field thing at least 4 people have sent Pull Requests for…) sooner rather than never. |
complaining helps 👍 added you as a contributor |
would add you to npm but it's broken ATM |
Thanks @visionmedia — any objections @defunctzombie (or others) to just publishing a new version with @Bren2010's original patch as-is, for now at least? |
My only objection is that I think the real fix is using a safe string compare and that his patch (while technically working) is doing more since it signs for no reason. The comparison is between mac and val and should be done to prevent the timing attack. Barring that, I don't actually use express signed cookies so it won't affect me. |
ill grab that for now since it wont let me add you |
1.0.2 |
nvm, npm is really broke |
I'll try again tonight |
Okay, if we're agreed this is a fix I'm going to merge this back in so @visionmedia can push it when npmjs finds itself again. If someone want to improve it with a constant-time string compare fixing it this way for now needn't prevent that. |
Fix for timing attacks on MAC verification.
npm is back up, published that guy |
Thanks, are you sure you published the version with the fix applied? At the time you tagged 1.0.2 only a couple minor fixes were landed — should be straight now (see my recent commits) |
@visionmedia I just confirmed that what you published as 1.0.2 does not include this patch, so my bump and history change to 1.0.3 right as you were publishing was probably a good thing. I'll add a git tag for 1.0.3 and could you |
done, added you to the package this time as well |
Double sign is one of the standard ways to provide fixed time comparison. If you want, here is another one: /**
* Timing-independent buffer comparison,
* returns true if two arguments are equal.
*
* @return {Boolean}
*/
function compare(buf1, buf2) {
if (buf1.length !== buf2.length) return false;
var diff = 0, len = buf1.length;
for (var i = buf1.length - 1; i >= 0; i--) {
diff |= buf1[i] ^ buf2[i];
}
return diff === 0;
}; Basically, all the difference is in performance, fixed time checking roughly doubles the time this module works. Is it worth it? Maybe, I don't know. |
Hey guys, here is a constant-time comparison function. It is basically equivalent to what @rlidwka posted above, although that snippet does have an issue - you need to use |
No I don't. That function compares buffers, not strings (which is explicitly mentioned in comments). |
This reapplies one of the patches originally documented in #2. The timing attack patch was reverted for no good reason AFAICT.