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

Add crypto.timingSafeEqual(). #3073

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions doc/api/crypto.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link

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 of input and does not take in an already computed inputHMAC.
So maybe change the description to

hmac.validate(input)

Return true if and only if the computed hmac matches the hmac of the
provided input. The input argument must be provided as a Node.js Buffer.

How does that look?


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
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -1153,6 +1168,15 @@ keys:

All paddings are defined in the `constants` module.

## crypto.timingSafeEqual(a, b)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should start with ### instead of ##.


Returns true if a is equal to b, without leaking timing information that would
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a and b here should have backticks for consistency.

Also, it may be useful to include a note about how the function performs its comparison, that it compares SHA256 HMACs of its arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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`.
Expand Down
35 changes: 35 additions & 0 deletions lib/crypto.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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);
Expand All @@ -95,6 +96,28 @@ Hmac.prototype.digest = Hash.prototype.digest;
Hmac.prototype._flush = Hash.prototype._flush;
Hmac.prototype._transform = Hash.prototype._transform;

Copy link
Member

Choose a reason for hiding this comment

The 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) {
Copy link
Member

Choose a reason for hiding this comment

The 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question. What benefits does this provide over just using timingSafeEqual?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or what about adding a buffer.timingSafeEquals() and using that instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What benefits does this provide over just using timingSafeEqual?

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think it will be the best, also key property won't be required anymore which is a second bonus! ;)

};

function getDecoder(decoder, encoding) {
if (encoding === 'utf-8') encoding = 'utf8'; // Normalize encoding.
Expand Down Expand Up @@ -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() {
Expand Down
21 changes: 18 additions & 3 deletions test/parallel/test-crypto-hmac.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,24 @@ var crypto = require('crypto');
// Test HMAC
var h1 = crypto.createHmac('sha1', 'Node')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit: these can be const

.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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a leftover from .validate() ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
wrote:

In test/parallel/test-crypto-hmac.js
#3073 (comment):

            .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');
    
    +const h2 = crypto.createHmac('sha1', 'Node')
  •           .update('some data')
    
  •           .update('to hmac');
    
    +assert.ok(h2.validate(

Looks like a leftover from .validate() ;)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/3073/files/8f232da78b2bbb0628a1401f9b94a9d362d12191#r63118037

new Buffer('19fd6e1ba73d9ed2224dd5094a71babe85d9a892', 'hex'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should now use Buffer.from('19fd6e1ba73d9ed2224dd5094a71babe85d9a892', 'hex')

'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 = [
Expand Down
13 changes: 13 additions & 0 deletions test/parallel/test-crypto-timing-safe-equal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
'use strict';
var common = require('../common');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const for the requires

var assert = require('assert');

if (!common.hasCrypto) {
console.log('1..0 # Skipped: missing crypto');
return;
}
var crypto = require('crypto');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const :-)


assert.ok(crypto.timingSafeEqual('alpha', 'alpha'), 'equal strings not equal');
assert.ok(!crypto.timingSafeEqual('alpha', 'beta'),
'inequal strings considered equal');