-
Notifications
You must be signed in to change notification settings - Fork 29.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 2 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 |
---|---|---|
|
@@ -605,6 +605,14 @@ encoding of `'binary'` is enforced. If `data` is a [`Buffer`][] then | |
|
||
This can be called many times with new data as it is streamed. | ||
|
||
### hmac.validate(inputHmac) | ||
|
||
Return true if and only if the computed hmac matches the input hmac, | ||
provided as a [Buffer](buffer.html). This uses a timing-safe comparison. | ||
|
||
The `hmac` object can not be used after `validate()` method has been | ||
called. | ||
|
||
## Class: Hmac | ||
|
||
The `Hmac` Class is a utility for creating cryptographic HMAC digests. It can | ||
|
@@ -665,6 +673,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. | ||
Prefer `validate()`, which does a timing-safe comparison. | ||
|
||
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. | ||
|
||
|
@@ -1153,6 +1168,15 @@ keys: | |
|
||
All paddings are defined in the `constants` module. | ||
|
||
## crypto.timingSafeEqual(a, 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. This should start with |
||
|
||
Returns true if a is equal to b, without leaking timing information that would | ||
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.
Also, it may be useful to include a note about how the function performs its comparison, that it compares SHA256 HMACs of its arguments. 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. Added backticks. I didn't add the SHA256 note. I feel like that's an implementation detail that should be left unspecified by the docs. |
||
help an attacker guess one of the values. This is suitable for comparing secret | ||
values like authentication cookies or | ||
[capability urls](http://www.w3.org/TR/capability-urls/). | ||
|
||
`a` and `b` can be strings or [Buffers](buffer.html). | ||
|
||
### crypto.privateEncrypt(private_key, buffer) | ||
|
||
Encrypts `buffer` with `private_key`. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,6 +83,7 @@ exports.createHmac = exports.Hmac = Hmac; | |
function Hmac(hmac, key, options) { | ||
if (!(this instanceof Hmac)) | ||
return new Hmac(hmac, key, options); | ||
this.key = key; | ||
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. What are the secondary security consequences of adding the secret key as an instance property in the constructor? 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. None that I am aware of. I don't think Node makes any promises that Hmac keys are unextractable from memory after initialization, right? 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. Should be either hidden, documented, or at least prefixed. Not sure if prefixing is a viable solution nowdays. |
||
this._handle = new binding.Hmac(); | ||
this._handle.init(hmac, toBuf(key)); | ||
LazyTransform.call(this, options); | ||
|
@@ -95,6 +96,28 @@ Hmac.prototype.digest = Hash.prototype.digest; | |
Hmac.prototype._flush = Hash.prototype._flush; | ||
Hmac.prototype._transform = Hash.prototype._transform; | ||
|
||
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. Superfluous change. |
||
// 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. | ||
// | ||
// Note: hmac object can not be used after validate() method has been called. | ||
Hmac.prototype.validate = function(inputBuffer) { | ||
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. Again, let's remove this. |
||
if (!(inputBuffer instanceof Buffer)) { | ||
throw new TypeError('Argument should be a Buffer'); | ||
} | ||
var ah = new Hmac('sha256', this.key).update(this.digest()).digest(); | ||
var bh = new Hmac('sha256', this.key).update(inputBuffer).digest(); | ||
return ah.equals(bh); | ||
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 a question. What benefits does this provide over just using 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. Or what about adding a 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.
Not much. It follows Brad Hill's original construction a little more closely, and it avoids generating a random number. But the simplicity seems worthwhile: would you like me to change it to that 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. Yep, I think it will be the best, also |
||
}; | ||
|
||
function getDecoder(decoder, encoding) { | ||
if (encoding === 'utf-8') encoding = 'utf8'; // Normalize encoding. | ||
|
@@ -662,6 +685,18 @@ function filterDuplicates(names) { | |
}).sort(); | ||
} | ||
|
||
// timingSafeEqual reuses the timing-safe Hmac.equals function (see above) for | ||
// comparison of non-hash inputs, like capability URLs or authentication tokens. | ||
exports.timingSafeEqual = function(a, b) { | ||
var key = randomBytes(32); | ||
var ah = new Hmac('sha256', key).update(a); | ||
// The final === test 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.validate(new Hmac('sha256', key).update(b).digest()) && | ||
a.toString() === b.toString(); | ||
}; | ||
|
||
// Legacy API | ||
exports.__defineGetter__('createCredentials', | ||
internalUtil.deprecate(function() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,9 +11,24 @@ var crypto = require('crypto'); | |
// Test HMAC | ||
var h1 = crypto.createHmac('sha1', 'Node') | ||
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. minor nit: these can be |
||
.update('some data') | ||
.update('to hmac') | ||
.digest('hex'); | ||
assert.equal(h1, '19fd6e1ba73d9ed2224dd5094a71babe85d9a892', 'test HMAC'); | ||
.update('to hmac'); | ||
assert.equal(h1.digest('hex'), | ||
'19fd6e1ba73d9ed2224dd5094a71babe85d9a892', | ||
'test HMAC'); | ||
|
||
var h2 = crypto.createHmac('sha1', 'Node') | ||
.update('some data') | ||
.update('to hmac'); | ||
assert.ok(h2.validate( | ||
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. Looks like a leftover from 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. Reverted, thanks. I can't check that CI URL because I'm getting 504's. On Thu, May 12, 2016 at 5:21 PM, Fedor Indutny notifications@github.com
|
||
new Buffer('19fd6e1ba73d9ed2224dd5094a71babe85d9a892', 'hex'), | ||
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. These should now use |
||
'test HMAC valid')); | ||
|
||
var h3 = crypto.createHmac('sha1', 'Node') | ||
.update('some data') | ||
.update('to hmac'); | ||
assert.ok(!h3.validate( | ||
new Buffer('6bdee6ee47fb42c53a4f44c3e4bb97591c0c3635', 'hex'), | ||
'test HMAC not valid')); | ||
|
||
// Test HMAC (Wikipedia Test Cases) | ||
var wikipedia = [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
'use strict'; | ||
var common = require('../common'); | ||
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.
|
||
var assert = require('assert'); | ||
|
||
if (!common.hasCrypto) { | ||
console.log('1..0 # Skipped: missing crypto'); | ||
return; | ||
} | ||
var crypto = require('crypto'); | ||
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.
|
||
|
||
assert.ok(crypto.timingSafeEqual('alpha', 'alpha'), 'equal strings not equal'); | ||
assert.ok(!crypto.timingSafeEqual('alpha', '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.
This might be a little misleading since
hmac.validate(input)
computes the HMAC ofinput
and does not take in an already computedinputHMAC
.So maybe change the description to
hmac.validate(input)
How does that look?