-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
Add crypto.timingSafeEqual(). #3073
Changes from 17 commits
1af1602
5840d64
efa01cd
4b12d47
90fef1f
0514f6f
2a81a88
20710a7
5a272e2
3fdd31f
ae864f2
398618d
cf101e0
2a9ae3e
e35e6a5
8f232da
13e6533
9fd483e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -681,6 +681,13 @@ Calculates the HMAC digest of all of the data passed using [`hmac.update()`][]. | |
The `encoding` can be `'hex'`, `'binary'` or `'base64'`. If `encoding` is | ||
provided a string is returned; otherwise a [`Buffer`][] is returned; | ||
|
||
Caution: Code that uses `digest()` directly for comparison with an input value | ||
is very likely to introduce a | ||
[timing attack](http://codahale.com/a-lesson-in-timing-attacks/). | ||
Such a timing attack would allow someone to construct an | ||
HMAC value for a message of their choosing without posessing the key. | ||
Use `timingSafeEqual(a, b)` to compare digest values. | ||
|
||
The `Hmac` object can not be used again after `hmac.digest()` has been | ||
called. Multiple calls to `hmac.digest()` will result in an error being thrown. | ||
|
||
|
@@ -1211,6 +1218,15 @@ keys: | |
|
||
All paddings are defined in the `constants` module. | ||
|
||
### crypto.timingSafeEqual(a, b) | ||
|
||
Returns true if `a` is equal to `b`, without leaking timing information that would | ||
help an attacker guess one of the values. This is suitable for comparing HMAC | ||
digests or secret values like authentication cookies or | ||
[capability urls](http://www.w3.org/TR/capability-urls/). | ||
|
||
A `TypeError` will be thrown if either `a` or `b` is not a [`Buffer`][] instance. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you fix the long lines? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
### crypto.privateEncrypt(private_key, buffer) | ||
|
||
Encrypts `buffer` with `private_key`. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -661,6 +661,33 @@ function filterDuplicates(names) { | |
}).sort(); | ||
} | ||
|
||
// This implements Brad Hill's Double HMAC pattern from | ||
// https://www.nccgroup.trust/us/about-us/ | ||
// newsroom-and-events/blog/2011/february/double-hmac-verification/. | ||
// In short, it's near-impossible to write a reliable constant-time compare in a | ||
// high level language like JS, because of the many layers that can optimize | ||
// away attempts at being constant time. | ||
// | ||
// Double HMAC avoids that problem by blinding the timing channel instead. After | ||
// running the inputs through a second round of HMAC, we are free to | ||
// short-circuit comparison, because the time it takes to reach the | ||
// short-circuit has no relation to the similarity between the guessed digest | ||
// and the correct one. | ||
exports.timingSafeEqual = timingSafeEqual; | ||
function timingSafeEqual(a, b) { | ||
if (!(a instanceof Buffer)) | ||
throw new TypeError('First argument must be a Buffer'); | ||
if (!(b instanceof Buffer)) | ||
throw new TypeError('Second argument must be a Buffer'); | ||
var key = randomBytes(32); | ||
var ah = new Hmac('sha256', key).update(a).digest(); | ||
var bh = new Hmac('sha256', key).update(b).digest(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps you can make these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
// The second test, for a.equals(b), is just in case of the vanishingly small | ||
// chance of a collision. It only fires if the digest comparison passes and so | ||
// doesn't leak timing information. | ||
return ah.equals(bh) && a.equals(b); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have reservations about this. Update+digest is basically a constant factor for inputs of a given size. This function will take much longer to complete with large inputs when they're equal than when they're unequal because of the second comparison. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a particular use case you're thinking of for large inputs? The main use cases for this are small, high-entropy values like HMAC digests, capability tokens, CSRF tokens, and authentication cookies. Also, is this a reservation about security or about performance? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess a note could be added, that it is still possible to approximate the length of the target string from the operation timing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the security side: timing safe comparisons are never expected to hide length information. For instance, consider an HMAC: the length of the target string is well-known, it's only the contents that are meant to be secret. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That’s very unlikely, given that But yes, this should be constant-time or not implemented at all. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, while a disagree on some nuances (like getting past There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @addaleax Constant-time is impossible without limiting the length of the buffer. Also, it's not very clear why is constant-time needed here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ChALkeR Yeah, of course it’s implied that the time is constant given that the buffer lengths are constant, otherwise that doesn’t make much sense. I think the attack scenario @jorangreef was talking about was that, as a passive observer, one could tell successful from unsuccessful comparisons by measuring the required time, which will be higher for sucessful comparisons due to the That certainly doesn’t have the same impact as other kinds of timing attacks, but I still get why it seems like a bad idea to implement this cryptographic primitive in this way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @addaleax Ah, I see. I am almost sure this would be fine without the last comparison — the probability of a collision is negligible there, and once (if) sha256 gets broken, we could replace it with something else without an API change. Another way would be to move the implementation to the C++ side. |
||
} | ||
|
||
// Legacy API | ||
exports.__defineGetter__('createCredentials', | ||
internalUtil.deprecate(function() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
const assert = require('assert'); | ||
|
||
if (!common.hasCrypto) { | ||
console.log('1..0 # Skipped: missing crypto'); | ||
return; | ||
} | ||
const crypto = require('crypto'); | ||
|
||
assert.ok(crypto.timingSafeEqual(Buffer.from('alpha'), Buffer.from('alpha')), | ||
'equal strings not equal'); | ||
assert.ok(!crypto.timingSafeEqual(Buffer.from('alpha'), Buffer.from('beta')), | ||
'inequal strings considered equal'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: possessing