Fix for timing attacks on MAC verification.

[natevw]

This reapplies one of the patches originally documented in https://github.com/visionmedia/node-cookie-signature/pull/2. The timing attack patch was reverted for no good reason AFAICT.

[defunctzombie]

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?

[natevw]

@defunctzombie See the original pull request; @Bren2010 felt this was the cleanest solution. I'm inclined to agree: IMO any safe_str_cmp function should come out of require('crypto') rather than us adding some one-off JS implementation to this seemingly-neglected library. My main worry with a JS implementation would be V8 optimizations or other JIT-related effects that would later affect our correctness. Since we don't have a trusted/maintained safe_str_comp in node core, I appreciate the KISS approach of this. [Assuming the OP was correct regarding its acceptability.]

[defunctzombie]

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.

[tj]

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

[natevw]

@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.

[defunctzombie]

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.

[natevw]

Here's how @Bren2010 originally explained it:

The way I've fixed this is by using the digest twice in comparison, so that an attacker can't determine on which byte the comparison has failed. There are other commonly accepted ways to do this, but I don't think that they would be more efficient or take less code.

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.

[tj]

complaining helps :+1:

added you as a contributor

[tj]

would add you to npm but it's broken ATM

[natevw]

Thanks @visionmedia — any objections @defunctzombie (or others) to just publishing a new version with @Bren2010's original patch as-is, for now at least?

[defunctzombie]

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.

[tj]

ill grab that for now since it wont let me add you

[tj]

1.0.2

[tj]

nvm, npm is really broke

[tj]

I'll try again tonight

[natevw]

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.

[tj]

npm is back up, published that guy

[natevw]

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)

[natevw]

@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 npm publish the latest?

[tj]

done, added you to the package this time as well

[rlidwka]

Sounds like a better solution than the double sign.

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.

[freewil]

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 charCodeAt which returns a number instead of a char

[rlidwka]

you need to use charCodeAt which returns a number instead of a char

No I don't. That function compares buffers, not strings (which is explicitly mentioned in comments).