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 16 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
16 changes: 16 additions & 0 deletions doc/api/crypto.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: possessing

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.

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

Copy link
Member

Choose a reason for hiding this comment

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

Can you fix the long lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

### crypto.privateEncrypt(private_key, buffer)

Encrypts `buffer` with `private_key`.
Expand Down
27 changes: 27 additions & 0 deletions lib/crypto.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you can make these const?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Calling it timingSafeEqual() when it's not constant-time is guaranteed to confuse users, and that's a Really Bad Thing with anything security-related. I'm -1 on adding this to core.

Copy link
Member

Choose a reason for hiding this comment

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

This assumes the compiler won't reorder the non-directed conditionals in a few years time.

That’s very unlikely, given that .equals uses native code under the hood that the JS compiler can’t inspect.

But yes, this should be constant-time or not implemented at all.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, while a disagree on some nuances (like getting past hmac), I agree that this code won't provide the guarantee that it claims to be providing.

Copy link
Member

@ChALkeR ChALkeR Jun 18, 2016

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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 a.equals(b) call.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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() {
Expand Down
23 changes: 19 additions & 4 deletions test/parallel/test-crypto-hmac.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,26 @@ if (!common.hasCrypto) {
var crypto = require('crypto');

// Test HMAC
var h1 = crypto.createHmac('sha1', 'Node')
const h1 = crypto.createHmac('sha1', 'Node')
.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(
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

Buffer.from('19fd6e1ba73d9ed2224dd5094a71babe85d9a892', 'hex'),
'test HMAC valid'));

const h3 = crypto.createHmac('sha1', 'Node')
.update('some data')
.update('to hmac');
assert.ok(!h3.validate(
Buffer.from('6bdee6ee47fb42c53a4f44c3e4bb97591c0c3635', 'hex'),
'test HMAC not valid'));

// Test HMAC (Wikipedia Test Cases)
var wikipedia = [
Expand Down
14 changes: 14 additions & 0 deletions test/parallel/test-crypto-timing-safe-equal.js
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');