Skip to content
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

crypto: add secure key comparsion function #5139

Closed
wants to merge 1 commit into from
Closed

crypto: add secure key comparsion function #5139

wants to merge 1 commit into from

Conversation

SukharevAndrey
Copy link

This commit adds function to compare cryptographic keys (as strings or buffers) without risk of timing attacks. Default comparators return result right after first symbol mismatch which can be measured and used to determine what symbol is wrong. The difference is about 5-6 times if we compare string keys of usually used lengths (2048 bits) and even higher on longer keys.

This commit adds function to compare cryptographic keys (as strings or buffers)
without risk of timing attacks. Default comparators return result right after
first symbol mismatch which can be measured and used to determine what symbol
is wrong. The difference is about 5-6 times if we compare string keys.
@ChALkeR
Copy link
Member

ChALkeR commented Feb 8, 2016

Related issue — #3043.

@ChALkeR ChALkeR added crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Feb 8, 2016
@bnoordhuis
Copy link
Member

I've commented on this before but I'll repeat it here: I don't think there is a reliable way to do truly constant time comparisons in node.

The underlying representation of a JS string (cons, flat, one byte, two byte, multi-byte) may be such that an attacker can still glean information from timing differences in comparing them. This new API function would thus give a false impression of security.

For background, I did a similar PR against node-forward but I ended up abandoning it. Even if you restrict it to buffers and typed arrays only, I'd still not be 100% confident you can't extract timing information through (for example) page faults or differing object shapes.

@SukharevAndrey
Copy link
Author

I think real timing attacks on small keys are somewhat impossible in real life even with simple comparsion, because keys are usually small (2048 and 4096 bits) and the difference on first symbol mismatch and latest symbol mismatch is hundreds of nanoseconds. On buffers it is even smaller, that is why I am comparing execution time in test on strings, not buffers.

New function reduces this difference to just a few nanoseconds. Zero difference is not really nessesary.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 8, 2016

@SukharevAndrey Your implementation at least allows to find out the length of the key.
If we pass in a string that is shorter than the key, then the time would be measurably smaller.

This method should not be used on strings or buffers of unknown lengths, so either it should throw when arguments are of unequal length (to make this a clear indication of an error to the developer, so he or she does not expect that it could be used for something like passwords) or add a third argument to specify a fixed length (but then you should double-check that you don't return false positives).

@ChALkeR
Copy link
Member

ChALkeR commented Feb 8, 2016

Also take a look at https://github.com/hapijs/cryptiles/blob/master/lib/index.js#L48-L68 — but that works only if the known secret is the first argument.

@SukharevAndrey
Copy link
Author

@ChALkeR I thought key length is a public information that any user can find out, at least in cryptography. When we compare passwords, usually they are stored as hashes (e.g. bcrypted), hash length is fixed and is not a secret information.

I can add the third parameter, but does it make any sense?

@ChALkeR
Copy link
Member

ChALkeR commented Feb 8, 2016

@SukharevAndrey Then just throw when their length mismatch, I think.

I know that you suppose that this would be used for keys, but someone could think that it's a generic contstant-time comparison method that magically does not depend on either of the lengths.

@bnoordhuis
Copy link
Member

@SukharevAndrey I think you underestimate the impact that object shapes have on run-time performance.

Apart from that, a big issue with the current implementation is that V8 may optimize the loop into one that exits early. I don't think Crankshaft is smart enough to do that but it wouldn't surprise me if TurboFan is (eventually - it may not be quite there yet.) Using Math.min() is no protection, that's an inlinable and optimizable intrinsic.

Aside, I appreciate you took the time to write tests but I'm not sure how reliable statistical analysis will be. process.hrtime() can be pretty coarse (I guess the documentation for it should mention that) and the optimizer and garbage collector can skew results.

@SukharevAndrey
Copy link
Author

@bnoordhuis I don't know then what to use to measure time. We could disable garbage collector while measuring time as it is done in Python's timeit library. It would minify risk of unexpected delays (OS internal processes may still have influence on results, but we can't fight it).

Instead of writing result to boolean variable we could xor keys like in library @ChALkeR mentioned.

But what to do with length mismatches?

@SukharevAndrey
Copy link
Author

As I can see in Node.JS there is no way to temporary disable GC :(. All other benchmarking libraries don't handle situations like that. What is the chance that GC will start collecting garbage in test? The whole test-crypto executes in less than a second. The difference in ratios in the test is big enough, so even GC can't skew results so much to fail the test in my opinion. We could increase key length to achieve even bigger difference.

@bnoordhuis
Copy link
Member

As I can see in Node.JS there is no way to temporary disable GC

That's correct, there isn't.

What is the chance that GC will start collecting garbage in test?

Non-zero. If you're very careful, you can write the hot path in a way that doesn't do allocations. It's hard to get right, though, and fairly brittle to boot; it may not carry over to another V8 release.

Even if you could disable the garbage collector somehow, there is still the optimizer/deoptimizer that can kick in at any time (and that too is not stable across V8 releases.)

@SukharevAndrey
Copy link
Author

@bnoordhuis Ok then. We can't properly measure execution time in test. The function will not always check all the strings of given length in the exact same amount of time. But none of functions will. And still it doesn't mean that we break security, the function is still safer than simple comparsion of strings or buffers. Delays by GC will affect all calculations and malefactor will not distinguish delays by GC and delays of comparsion. But GC doesn't affect calculations any moment and malefactor can use moments when GC doesn't collect garbage and theoretically use that to find mismatching symbol position.

XORing strings is 10 times slower than current implementation. We can add some simple useless calculations (such as variable addition) and use them to produce answer. Then compiler will not optimize it, or optimize it the same way for all strings of given length.

@bnoordhuis
Copy link
Member

Then compiler will not optimize it, or optimize it the same way for all strings of given length.

I think that's a dangerous assumption, long term. Take the code that @ChALkeR linked to; it's vulnerable to at least two compiler optimizations that would nullify the constant-time guarantee:

  1. specializing for the case where a === b is true, and
  2. deducing that the loop can be quit as soon as mismatch !== 0.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 8, 2016

@bnoordhuis What about converting to unpooled Buffers (or TypedArrays) and comparing those in C++ code?

This is something that is either done totally wrong, or as potentially wrong as in hapi.
It wouldn't be good just to throw that aside, saying that it can't be done.

@bnoordhuis
Copy link
Member

That's what I did in the node-forward pull request but I wasn't convinced I could make it 100% reliable. If I don't even trust my own code, you can imagine the suspicion with which I view other people's code. :-)

@brendanashworth
Copy link
Contributor

Also see: #3073.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 9, 2016

@brendanashworth Approach in #3073 looks better to me, perhaps.

@jsha
Copy link
Contributor

jsha commented Feb 11, 2016

Just to clarify a misconception here: In general, the requirement is to compare HMAC output in a timing-safe way, not public or private asymmetric keys.

@jasnell jasnell added the feature request Issues that request new features to be added to Node.js. label Jun 6, 2016
@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01
@jasnell
Copy link
Member

jasnell commented Feb 28, 2017

Closing. We have a timing safe equal method now.

@jasnell jasnell closed this Feb 28, 2017
@freewil
Copy link

freewil commented Feb 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants