Add crypto.timingSafeEqual(). #3073

jsha · 2015-09-26T09:13:35Z

bnoordhuis · 2015-09-26T10:17:22Z

@mikeal Can I make node-forward/node public? I'd like to link to the previous discussion.

mscdex · 2015-09-26T16:35:04Z

lib/crypto.js

+// safety and benefits from never throwing an exception.
+// See discussion at
+// https://github.com/nodejs/node-v0.x-archive/issues/8560#issuecomment-60145816
+exports.timingSafeEqual = function(a, b) {


Why export this just for a test? Was it meant to be documented too?

Thanks, I did indeed mean to document it as a new method. Fixed.

brendanashworth · 2015-09-26T18:48:00Z

lib/crypto.js

+exports.timingSafeEqual = function(a, b) {
+  var key = new Buffer(32);
+  for (var i = 0; i < 32; i += 4) {
+    key.writeFloatBE(Math.random(), i);


If you're generating for randomness, Math.random is probably not the best option - I'd suggest var key = crypto.randomBytes(32) instead.

Ah, I see the (nodejs/node-v0.x-archive#8560 (comment)) discussion now. I'm not sure, but it makes sense just to use the system RNG regardless.. with Math.random you might as well fill the buffer with 4.

I went back and forth on this a few times. The natural thing would be to just call crypto.randomBytes(32), and that's what I did the first time around. But crypto.randomBytes is allowed to throw. So the options were: say that timingSafeEqual is also allowed to throw (yuck), or catch the exception and do something next-best. And if we think the next-best is safe enough, why not just go with it directly?

As Brad said over on the linked issue, even if we just filled the buffer with four, that would only reduce the strength against guessing to the birthday bound for SHA256, which is still extremely strong. The counterintuitive thing about the Double-HMAC construction is that the strength is in the properties of an HMAC function, not in the random number. Where many things in crypto fall apart if your randomness is bad, this one (surprisingly) does not.

timingSafeEqual can still throw, even without crypto.randomBytes (which I'm not even sure if that throws regularly). Next-best introduces a possible attack vector through Math.random.

That is interesting, about the strength of this. Ultimately it is up to you whether or not you want to change it. Perhaps try a benchmark and see the difference?

Well, you could make it sync/async just like randomBytes(). That way an error is thrown for sync or it's passed as the err argument for the async callback. Then use the async variation if you really don't want to wrap timingSafeEqual() in a try-catch.

timingSafeEqual can still throw

Ah, interesting! Under what conditions?

I was going off the fact that randomBytes documents the possibility of an
exception, and was assuming we'd have to similarly cover the possibility
here, even if it was quite rare. All told, I'd prefer to use
crypto.randomBytes to avoid the code smell. For me it's mainly a question
of what you'd like the Node API to look like. Is a tiny risk of exception
acceptable? I think, fwiw, that exception can never happen on Linux,
assuming OpenSSL is using /dev/urandom.
On Sep 26, 2015 13:52, "Brendan Ashworth" notifications@github.com wrote:

In lib/crypto.js
#3073 (comment):

@@ -652,6 +672,23 @@ 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.
+// Note that the strength of the double-HMAC construction does not depend
+// on the unpredictability of the key. Since randomBytes can potentially
+// throw an exception, we use Math.random, which sacrifices nothing in timing
+// safety and benefits from never throwing an exception.
+// See discussion at
+// nodejs/node-v0.x-archive#8560 (comment)
+exports.timingSafeEqual = function(a, b) {

var key = new Buffer(32);

for (var i = 0; i < 32; i += 4) {

key.writeFloatBE(Math.random(), i);

timingSafeEqual can still throw, even without crypto.randomBytes (which
I'm not even sure if that throws regularly). Next-best introduces a
possible attack vector through Math.random.

That is interesting, about the strength of this. Ultimately it is up to
you whether or not you want to change it. Perhaps try a benchmark and see
the difference?

—
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/3073/files#r40498240.

Under what conditions?

Really just under abnormal conditions, like running out of memory, but you could also cause an error from C++ by giving it a fake Buffer.

I might be wrong here, but I think the documentation for randomBytes is out of date. Under normal circumstances, the only error thrown from randomBytes is from RAND_bytes(), which throws when it doesn't have enough entropy. However, with CheckEntropy (see f68a116), it should now block until the system has enough entropy for the OpenSSL pool (see log in e5e5980). I think the documentation just needs to be updated - now that you bring it up, the docs are very confusing on that.

Edit: I've opened #3081.

Alright, switched to randomBytes, thanks for clarifying!

jorangreef · 2015-09-28T06:13:27Z

lib/crypto.js

@@ -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;


What are the secondary security consequences of adding the secret key as an instance property in the constructor?

None that I am aware of. I don't think Node makes any promises that Hmac keys are unextractable from memory after initialization, right?

Should be either hidden, documented, or at least prefixed. Not sure if prefixing is a viable solution nowdays.

brendanashworth · 2015-09-28T22:52:48Z

cc @nodejs/crypto

rvagg · 2015-09-29T14:11:30Z

@bnoordhuis I don't think that repo can be re-enabled now and even if it could we'd need to update the README and other cruft, perhaps you can just copypasta discussion if you can still find it?

jasnell · 2016-02-10T16:31:30Z

What's the status on this one?

jsha · 2016-02-11T18:03:03Z

Looks like some merge conflicts cropped up. I'll fix those, then I think this is ready to go.

jsha · 2016-02-11T18:56:13Z

Okay, updated.

jasnell · 2016-02-11T18:59:25Z

@nodejs/crypto ... PTAL

jsha · 2016-02-19T19:14:01Z

Pushed an additional change with a final equality check to address @indutny's concern about collisions, and added a type check.

indutny · 2016-02-19T19:17:53Z

LGTM, good job @jsha !

jasnell · 2016-02-19T23:28:06Z

test/parallel/test-crypto-timing-safe-equal.js

@@ -0,0 +1,13 @@
+'use strict';
+var common = require('../common');


const for the requires

jasnell · 2016-02-19T23:28:38Z

LGTM

jsha · 2016-05-12T20:18:33Z

Oops, forgot to remove .validate. Done.

indutny · 2016-05-12T20:31:17Z

lib/crypto.js

@@ -92,7 +92,6 @@ Hmac.prototype.digest = Hash.prototype.digest;
 Hmac.prototype._flush = Hash.prototype._flush;
 Hmac.prototype._transform = Hash.prototype._transform;

-


Superfluous change.

indutny · 2016-05-12T20:31:55Z

Thank you! One more nit, CI: https://ci.nodejs.org/job/node-test-pull-request/2611/ .

jsha · 2016-05-12T20:49:41Z

Done.

On Thu, May 12, 2016 at 1:33 PM, Fedor Indutny notifications@github.com
wrote:

Thank you! One more nit, CI:
https://ci.nodejs.org/job/node-test-pull-request/2611/ .

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3073 (comment)

indutny · 2016-05-12T23:59:48Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/2617/ , Jenkins rebooted

indutny · 2016-05-13T00:20:32Z

test/parallel/test-crypto-hmac.js

+const h2 = crypto.createHmac('sha1', 'Node')
+               .update('some data')
+               .update('to hmac');
+assert.ok(h2.validate(


Looks like a leftover from .validate() ;)

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

indutny · 2016-05-13T03:38:53Z

No problem, new CI: https://ci.nodejs.org/job/node-test-pull-request/2619/

jsha · 2016-05-13T04:40:59Z

Am I right in reading this CI output - just the Windows build failed? Any
idea why that would be?

On Thu, May 12, 2016 at 8:39 PM, Fedor Indutny notifications@github.com
wrote:

No problem, new CI: https://ci.nodejs.org/job/node-test-pull-request/2619/

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3073 (comment)

bnoordhuis · 2016-05-13T08:09:08Z

doc/api/crypto.md

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


Typo: possessing

MylesBorins · 2016-06-17T21:32:33Z

One more round of CI: https://ci.nodejs.org/job/node-test-pull-request/3020/

@nodejs/crypto any reason this should not land?

bnoordhuis · 2016-06-18T06:54:49Z

@thealphanerd See the discussion here: #3073 (comment)

I'm still -1. I suggest we close this.

jsha · 2016-06-18T06:58:34Z

I'm happy to just call out to CRYPTO_memcmp if it would make everyone feel better, but I think it's a non-option to offer a cryptography library without a timing safe compare.

ChALkeR · 2016-06-18T07:53:55Z

About the compiler reordering things — we could deoptimize this function deliberately (e.g. adding a never-called eval after the return), but then it would need a test to ensure that no optimizations are being done there in the future. But I really doubt that's the best thing to do here.

The chance of a random collision here is negligible, there is no known attack on sha256 that breaks it, and we could always change the hashing function. This will by all means be better than what the ecosystem currently uses (e.g. https://github.com/hapijs/cryptiles/blob/master/lib/index.js#L48-L68).

Still not sure about it, but I would prefer if we settled with some implementation and landed it, instead of saying «this is impossible in js».

Perhaps this needs to be implemented on the C++ side, then?

hillbrad · 2016-06-19T03:30:58Z

I think developers are more likely to find a safe verification method directly attached to the HMAC, but don't think this is worth bikeshedding any more after so long. This is substantially safer than the naive comparison that almost everyone uses, and provides an abstraction behind which future improvements can be iterated on. +1 to landing it.

jasnell · 2016-06-20T16:08:57Z

@ChALkeR ....

Perhaps this needs to be implemented on the C++ side, then?

I was thinking the same. If this were implemented as native code, we ought to be able to bypass the timing concerns in JS entirely. We would need to verify, however, that there are no new timing concerns introduced by the JS-to-native boundary but I would be surprised.

jasnell · 2016-08-22T18:20:41Z

Closing in favor of the ongoing work in #8040

mscdex reviewed Sep 26, 2015
View reviewed changes

mscdex added the crypto Issues and PRs related to the crypto subsystem. label Sep 26, 2015

brendanashworth added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 26, 2015

brendanashworth reviewed Sep 26, 2015
View reviewed changes

brendanashworth mentioned this pull request Sep 26, 2015

crypto.randomBytes documentation needs improvement #3081

Closed

jorangreef reviewed Sep 28, 2015
View reviewed changes

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:00

brendanashworth mentioned this pull request Feb 9, 2016

crypto: add secure key comparsion function #5139

Closed

ChALkeR mentioned this pull request Feb 9, 2016

Crypto library should have a constant-time equality function #3043

Closed

Add Hmac.validate() and timingSafeEquals().

1af1602

jsha force-pushed the timing-safe-compare branch from 6263650 to 1af1602 Compare February 11, 2016 18:53

timingSafeEqual: final === check and type checking

5840d64

jsha force-pushed the timing-safe-compare branch from 163bdde to 5840d64 Compare February 19, 2016 19:13

jasnell reviewed Feb 19, 2016
View reviewed changes

Add const to require statements.

efa01cd

jsha added 2 commits May 12, 2016 13:17

Remove Hmac.validate.

2a9ae3e

Const-ize

e35e6a5

indutny reviewed May 12, 2016
View reviewed changes

Lint

8f232da

indutny reviewed May 13, 2016
View reviewed changes

Revert test-crypto-hmac to master.

13e6533

bnoordhuis reviewed May 13, 2016
View reviewed changes

bnoordhuis removed the stalled Issues and PRs that are stalled. label May 13, 2016

Fix long lines and constify

9fd483e

not-an-aardvark mentioned this pull request Aug 9, 2016

Add crypto.timingSafeEqual() #8040

Closed

4 tasks

jasnell closed this Aug 22, 2016

sjmiller609 mentioned this pull request Apr 13, 2017

timing attack vulnerability flint-bot/sparky#7

Closed

bnoordhuis mentioned this pull request Nov 21, 2017

crypto.timingSafeEqual is not really time safe? #17178

Closed

bdewater mentioned this pull request Aug 18, 2019

Add OpenSSL.memcmp? for constant time/timing safe comparison ruby/openssl#269

Merged

		@@ -0,0 +1,13 @@
		'use strict';
		var common = require('../common');

		@@ -92,7 +92,6 @@ Hmac.prototype.digest = Hash.prototype.digest;
		Hmac.prototype._flush = Hash.prototype._flush;
		Hmac.prototype._transform = Hash.prototype._transform;

Add crypto.timingSafeEqual(). #3073

Add crypto.timingSafeEqual(). #3073

Conversation

jsha commented Sep 26, 2015

bnoordhuis commented Sep 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendanashworth commented Sep 28, 2015

rvagg commented Sep 29, 2015

jasnell commented Feb 10, 2016

jsha commented Feb 11, 2016

jsha commented Feb 11, 2016

jasnell commented Feb 11, 2016

jsha commented Feb 19, 2016

indutny commented Feb 19, 2016

Choose a reason for hiding this comment

jasnell commented Feb 19, 2016

jsha commented May 12, 2016

Choose a reason for hiding this comment

indutny commented May 12, 2016

jsha commented May 12, 2016

indutny commented May 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented May 13, 2016

jsha commented May 13, 2016

Choose a reason for hiding this comment

MylesBorins commented Jun 17, 2016

bnoordhuis commented Jun 18, 2016

jsha commented Jun 18, 2016

ChALkeR commented Jun 18, 2016 • edited Loading

hillbrad commented Jun 19, 2016

jasnell commented Jun 20, 2016

jasnell commented Aug 22, 2016

ChALkeR commented Jun 18, 2016 •

edited

Loading