Use crypto.timingSafeEqual for unsign

[jodevsa]

No description provided.

[natevw]

Thanks, this will be great! There's just a few things that stand out:

• unfortunately, crypto.timingSafeEqual will throw if inputs aren't equal length — we need to avoid that!
• we should use simply Buffer.from(x) — the 'ascii' encoding is inconsistent with the UTF-8 default of sign method's .update()
• let's also get rid of the /** Private */ section entirely since there's nothing there

[jodevsa]

@natevw Done!
What do you think?

[natevw]

@jodevsa Nearly there, thank you! I was about to merge it but then realized a potential issue.

I like the trick for making the buffers equal size, but instead of the deprecated Buffer(size) call, it needs to use Buffer.alloc(macBuffer.length) to be safe.

Otherwise I think it's solid:

• mac [the re-signed value] will always be at least as long as val [the untrusted input], and so there's no risk of accepting only a prefix match. ✅
• If val is the same size, it will pass/fail the equality check based on its validity in constant time. ✅
• If the untrusted input is shorter, e.g. doesn't contain a signature, it will be compared with uninitialized memory. This could result in a false acceptance: imagine an attacker provides val as simply {"user":"root"} (no .signature). We might end up copying it overtop a buffer that already contains {"user":"root"}.d87c231af27dde3385a0a838a31e70d3cf89dd97 leftover from a previous request that was valid! ❌

Starting with a zero-filled valBuffer should resolve that last risk.

[jodevsa]

@natevw
Replaced it with Buffer.alloc() 👍
So sorry for not reading the Docs, having multiple ways of initializing a buffer made me wonder which interface to choose; so I've picked the easiest :(

Thanks a lot for your valuable explanations, I've learned so many things from you and the project !

[natevw]

Merged, and published as cookie-signature@1.1.0.

Thanks for making this happen! It's good to have this in place since the earlier method had led to a lot of confusion.

[dougwilson]

Just came across this PR trying to upgrade cookie-signature in Express :) Didn't expect a minor version bump to drastically raise the minimum Node.js version, even excluding the current LTS Node.js version. Before I remove this module from Express completely (because what choice do I have?) I wanted to ask if you're set on Node.js 6.6.0 being the minimum required version. If so, that's OK and we'll work-around the issue 👍

[jodevsa]

@dougwilson ,
@natevw approach in handling a safe compare before i landed my PR, is the same approach used in tsscmp.

[dougwilson]

Thanks @jodevsa . Is there any way to get a version of this module that is compatible with Node.js LTS (4.x)?

[dougwilson]

The reason I'm asking is because I'm being told that this PR is a security fix and so I'm trying to get Express and such upgraded to the 1.1.0 version of this module, but without even 4.x support this is going to be a real mess for everyone trying to get the security update. If it's at all possible to backport the security fix that would be super appreciated :) !

[natevw]

@dougwilson Version 1.1.0 is not a security fix.

Since version 1.0.4 this library has used a timing safe comparison strategy. We used a second hash step to do so. (See #14,#17)

The "extra" hashing led to lots of confusion/discussion over the years (#15 #19 #20 #21 #23) and recently started causing warnings — which were not really of true concern due to the nature of our use — in some audit tools (#22).

So we finally made the move to the more readable crypto.timingSafeEqual here but, to re-iterate, any timing attacks should have already been taken care of in 1.0.4.

[dougwilson]

Hi @natevw I'm sorry for the confusion. I was working off of a security vulnerability report disclosed to the Express project in good faith. The user agrees now that it is not a security issue. He also told me how he came to the conclusion that 1.1.0 was a security release and it came down to the fact the commit 5fb33f0 visible in the commit history is titled "Use timingSafeEqual instead of hashing to prevent time attack" which made it sound like the commit is necessary to prevent a timing attack.

[natevw]

@dougwilson Ah, yes. The committer must have meant that as "switch from the old hashing trick to now-builtin timingSafeEqual, in the logic that prevents timing attacks". But I can see how that would be confusing!