Unexpected unsign passing on a random signature postfix

[muturgan]

Hi dear team.
Thank you for the amazing project!

This is my reproduce example:

const cookie = require('cookie-signature');
const value = 'value';
const secret = 'secret';
const signed = cookie.sign(value, secret);
const result = cookie.unsign(signed + 'some', secret);
console.log(result)

I expected to see a false result. But got a 'value'.

[natevw]

Thanks for the report and helpful example!

Just to be clear for anyone else following along, I wouldn't consider this a vulnerability since the cookie must still be properly signed for it to happen.

But I agree that this is a regression introduced during another bugfix. So I think it's safe to assume (for semver purposes) that nobody should be relying on this bug and we can fix it.

It happens in the chain of events where val is silently truncated when copied into a buffer for comparison purposes:

valBuffer = Buffer.alloc(macBuffer.length);
valBuffer.write(val);
return crypto.timingSafeEqual(macBuffer, valBuffer) ? str : false;

We do need valBuffer to be the right length otherwise timingSafeEqual will throw an exception but we should probably check that .write(val) was the expected size. This might be a bit tricky w.r.t. UTF-8 encoding, i.e. the expected size is not going to always equal val.length! (That situation is hopefully not super common with actual Cookie headers but people could be using this algorithm in other contexts where non-ASCII data doesn't need to be handled as carefully.)

UPDATE: upon further reflection, I think it's okay to only avoid success when val.length > valBuffer.length:

• if val.length is shorter than macBuffer.length e.g. not signed at all, the zero fill of Buffer.alloc will expose the mismatch
• if val.length (in terms of UTF-16 code units) is equal to macBuffer.length, but actually needs more bytes to .write in a UTF-8 representation, we won't catch the overflow in my proposed length check but it will still result in a buffer mismatch [but this is subtle and I might be missing a way to still sneak a bug through]

[natevw]

I've got a proposed fix for this in #41, but am waiting for some other maintainers to sign off on it before merging it in.

If this bug is blocking your own development you could npm install tj/node-cookie-signature#nvw-unsign-fixes in the meantime but I suspect most users can wait until the fix is reviewed and published.