Use double HMAC comparision for tokens - upgrade scmp to tsscmp

[suryagh]

Constant-time string-comparison approach used by scmp is still vulnerable to timing attacks on node. Here is relative recent discussion by node core team. The recommendation is to use double HMAC comparison.

As a fix upgrade to tsscmp that uses the recommended approach implemented in the latest version of node

[dougwilson]

Thanks! This is good to know. Before we up and switch dependencies, is there a reason why the scmp dependency itself cannot be fixed? Was there an issue filed to the author of this module that was closed? For example, if scmp is fixed, modules like us would simply upgrade and everytone would be happier, rather than trying to swap modules for some reason. Do these modules have different purposes?

We can certainly switch modules, but I want to make sure all the bases are covered.

[suryagh]

scmp and tscmp does different things.

scmp is documented to implement constant-time-string-comparison, which many projects use for time safe comparison. tsscmp implements double hmac comparison which performs random-time-per-guess comparison of strings. Though not a big change, I'm not certain this can be called as a fix on scmp.

[dougwilson]

Ah, @suryagh, thanks for the clarification :) Yes, I would agree that random-time-string-comparison is a different thing than constant-time-string-comparison. Change makes sense to me, then 👍 . I have to keep the pull request open for a while to allow other organization members to comment, just so you know why I'm not merging it just yet, but I don't see an issue.

I know the title of the issue says "vulnerability", but I assume it isn't, otherwise hopefully this issue would have been disclosed privately until a fix could have been produced & published.

[blakeembrey]

@suryagh What's the main reason for using base64 representation and string comparison instead of .equals? As seen in this implementation: https://github.com/nodejs/node/pull/3073/files.

Edit: Mostly just curious if there's a reason to deviate from the confirmed implementation.

[suryagh]

@blakeembrey .equals is available only on the later versions of node. And trying to implement a custom binary equality for buffer was a path I do not want to take. Idea was to use build-in features of node without any external dependencies. tsscmp has no other dependency other than node.

[blakeembrey]

Gotcha. So buffer.equals was added in 0.12 (E.g. https://github.com/sindresorhus/buffer-equals).

[dougwilson]

Hi @suryagh, I just used your PR and placed this module in a stress test, and compared to the current implementation, your implementation seems to sometimes end up throwing Error: error:24064064:random number generator:SSLEAY_RAND_BYTES:PRNG not seeded from the crypto.randomBytes call. If there any way to work-around that? Thoughts? Using Node.js 0.10. I have not tried other versions yet, but this module supports down to Node.js 0.8.

[suryagh]

@dougwilson this looks to be an issue with the older versions of node where the crypto.randomBytes could throw error if there is no enough accumulated entropy. This should happen only if run immediately after the system reboot. I have worked around the issue by using pseudoRandomBytes which should not throw an error. In older versions of node it is non-cryptographically strong. However, this should not be a problem since the original solution depends on the randomness in the processing time.

The v1.0.2 of tsscmp does buffer comparison without requiring a base64 encoding, and also additional unit and benchmark tests.

P.S. it is amazing your benchmarking test is able to catch this timing issue.

[dougwilson]

Awesome! I've been running for a bit with 1.0.2 without issues. I think the reason I may have seen it is due to running on some elastic infrastructure, where things are spun up in response to traffic load.