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

crypto: re-add crypto.timingSafeEqual #8304

Closed

Conversation

not-an-aardvark
Copy link
Contributor

@not-an-aardvark not-an-aardvark commented Aug 27, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto test

Description of change

Refs: #8040, #8203, #8225

This re-adds crypto.timingSafeEqual, which was previously reverted due to test failures and concerns that the implementation might not be timing-safe.

Some investigation is still needed to determine why the tests are failing on ARM systems.

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Aug 27, 2016
@Trott
Copy link
Member

Trott commented Aug 27, 2016

@not-an-aardvark
Copy link
Contributor Author

Looks like the arm tests are failing like before, but linux is working now.

One of the arm tests had a t-value of -32 (i.e. checking unequal buffers took significantly longer than checking equal buffers). This is the opposite of what was happening on linux, so it seems unlikely to be the same problem.

@not-an-aardvark
Copy link
Contributor Author

I added a temporary console.log to output more detailed info if the tests fail.

I don't have access to any ARM hardware, so debugging/reproducing this locally might be tricky. (I suppose I could look into emulating one.)

@Trott
Copy link
Member

Trott commented Aug 28, 2016

Here's a run of the test 100 times on armv7-wheezy. (Hopefully I did it right. It's building right now...) https://ci.nodejs.org/job/node-stress-single-test/871/nodes=armv7-wheezy/console

@not-an-aardvark
Copy link
Contributor Author

Wow, that's a lot of numbers. Output for posterity

@not-an-aardvark
Copy link
Contributor Author

So I'm not sure whether this is related to the test failures, but I noticed that a vast majority of the benchmark times (over 98%) ended with the digit 1. Is this just a known quirk of process.hrtime()?

@Trott
Copy link
Member

Trott commented Aug 28, 2016

@nodejs/crypto @nodejs/v8

@not-an-aardvark
Copy link
Contributor Author

Of the 52 times that the previous test failed, all of the t-values were positive numbers between 3 and 7.

I pushed another test commit to flip the order of the benchmarks; could we run another stress-test? If the problem is caused by a security issue with the timingSafeEqual implementation, we would expect the t-values to still be positive numbers after swapping. If the problem is caused by something else (e.g. V8 optimization), the t-values will be negative numbers after swapping.

@addaleax
Copy link
Member

here you go: https://ci.nodejs.org/job/node-stress-single-test/872/nodes=armv7-wheezy/console

@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Aug 28, 2016

That time all the t-values were numbers between -3 and -7, i.e. the comparisons between unequal buffers took longer.

Based on that, I'm pretty confident that this is not a security issue; whichever pair of buffers is compared first ends up with longer benchmarks, regardless of the actual contents of the buffers.

@not-an-aardvark
Copy link
Contributor Author

Pushed another commit to alternate the order of the benchmarks on each iteration.

(This might not actually solve this issue, depending on whether it's only the first benchmark overall that matters, or the first benchmark of each iteration. But it's probably worth trying anyway since it would be nice to have non-flaky tests even if it's not a security problem.)

@addaleax
Copy link
Member

@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Aug 28, 2016

Whoa, that didn't help; that time they all failed with positive t-values.

It's interesting that before that change, the test was failing almost exactly half of the time. I'm guessing there's some optimization that was kicking in at a random point, causing the test to fail if it kicked in between invocations, and pass if it kicked in after a pair of invocations is finished.

(I remember we reached something similar to this conclusion last time and tried manually optimizing the function without success.)

@not-an-aardvark
Copy link
Contributor Author

To prevent V8 from optimizing the cases differently, I split the runBenchmark function into runEqualBenchmark and runUnequalBenchmark.

(This was also attempted in #8203 (comment), but it seems like there weren't any timing failures on ARM during that CI run, so it might solve the issue.)

@addaleax
Copy link
Member

@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Aug 28, 2016

That version passed 96/100 times. 🎉

I have no idea what happened with the 4 failures though; t-values of 6, -31, -33, and 3.

@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Aug 28, 2016

I think this is ready for review; the tests are slightly flaky on ARM, but they seem to pass somewhat consistently on other platforms, and we have fairly good reason to believe that the inconsistencies are not a security problem.

(The implementation in this PR is the same as in #8040; only the tests have changed.)

@Trott
Copy link
Member

Trott commented Aug 30, 2016

I guess let's start with a regular CI run: https://ci.nodejs.org/job/node-test-pull-request/3881/

@not-an-aardvark
Copy link
Contributor Author

Two relevant failures:

@jasnell
Copy link
Member

jasnell commented Aug 30, 2016

Woo! tentative LGTM assuming CI looks green.

@not-an-aardvark
Copy link
Contributor Author

There's no big rush, but I'd like to keep the process moving along on this PR, if possible. Is there anything I can do to advance it, or are we just waiting for people to review it at this point?

@addaleax
Copy link
Member

addaleax commented Sep 6, 2016

LGTM

And maybe /cc @AndreasMadsen and/or @fhinkel for reviews if they have the time? I wouldn’t hold the PR up about that but it might be good to get other math-y looks.

@jasnell
Copy link
Member

jasnell commented Sep 7, 2016

Let's give it another 24 hours at least.... and perhaps another dozen or so CI runs ;-) lol (kidding about the last bit).

@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Sep 7, 2016

Is it worth running the CI again now that the Raspberry Pi machines are no longer out of commission? If this test is going to break the build again for some reason, it would be better to find now rather than after the PR is merged.

@Trott
Copy link
Member

Trott commented Sep 7, 2016

@jasnell
Copy link
Member

jasnell commented Sep 7, 2016

ha! I started one at the exact moment @Trott did. Cancelled mine!

@Trott
Copy link
Member

Trott commented Sep 7, 2016

@not-an-aardvark Can you get in touch with me out-of-band? I want to talk about the Collaborator onboarding stuff, but don't want to muddy up this issue with a bunch of irrelevant back-and-forth. Any of these work for me:

Or if you'd prefer some other mechanism to communicate, let me know.

Thanks!

@Trott
Copy link
Member

Trott commented Sep 7, 2016

CI is all green except for the perma-yellow AIX, so I think this is good to land.

Trott pushed a commit to Trott/io.js that referenced this pull request Sep 7, 2016
Reinstate crypto.timingSafeEqual() which was reverted due to test
issues. The flaky test issues are resolved in this new changeset.

PR-URL: nodejs#8304
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Trott
Copy link
Member

Trott commented Sep 7, 2016

Landed in 079accc

🎉

Thanks for persevering, @not-an-aardvark!

@Trott Trott closed this Sep 7, 2016
@not-an-aardvark not-an-aardvark deleted the timing-safe-equal-v2 branch September 7, 2016 23:34
@bnoordhuis
Copy link
Member

Sorry to be a party pooper but it seems the test is still flaky: https://ci.nodejs.org/job/node-test-commit-linux/5019/nodes=debian8-x86/consoleText

@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Sep 9, 2016

@bnoordhuis Yep, there is a PR open to fix this (see #8456).

Fishrock123 pushed a commit that referenced this pull request Sep 14, 2016
Reinstate crypto.timingSafeEqual() which was reverted due to test
issues. The flaky test issues are resolved in this new changeset.

PR-URL: #8304
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Fishrock123 added a commit that referenced this pull request Sep 14, 2016
Notable changes:

* crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
#8304
* events: Made the "max event listeners" memory leak warning more
accessible. (Anna Henningsen) #8298
* promises: Unhandled rejections now emit a process warning after the
first tick. (Benjamin Gruenbaum)
#8223
* repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
#8241
* util: Some functionality has been added to `util.inspect()`:
- Returning `this` from a custom inspect function now works. (Anna
Henningsen) #8174
- Added support for Symbol-based custom inspection methods. (Anna
Henningsen) #8174

Refs: #8428
Refs: #8457
PR-URL: #8466
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 15, 2016
Notable changes:

* crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
nodejs#8304
* events: Made the "max event listeners" memory leak warning more
accessible. (Anna Henningsen) nodejs#8298
* promises: Unhandled rejections now emit a process warning after the
first tick. (Benjamin Gruenbaum)
nodejs#8223
* repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
nodejs#8241
* util: Some functionality has been added to `util.inspect()`:
- Returning `this` from a custom inspect function now works. (Anna
Henningsen) nodejs#8174
- Added support for Symbol-based custom inspection methods. (Anna
Henningsen) nodejs#8174

Refs: nodejs#8428
Refs: nodejs#8457
PR-URL: nodejs#8466
imyller added a commit to imyller/meta-nodejs that referenced this pull request Sep 15, 2016
    Notable changes:

    * crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
    nodejs/node#8304
    * events: Made the "max event listeners" memory leak warning more
    accessible. (Anna Henningsen) nodejs/node#8298
    * promises: Unhandled rejections now emit a process warning after the
    first tick. (Benjamin Gruenbaum)
    nodejs/node#8223
    * repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
    nodejs/node#8241
    * util: Some functionality has been added to `util.inspect()`:
    - Returning `this` from a custom inspect function now works. (Anna
    Henningsen) nodejs/node#8174
    - Added support for Symbol-based custom inspection methods. (Anna
    Henningsen) nodejs/node#8174

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Sep 15, 2016
    Notable changes:

    * crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
    nodejs/node#8304
    * events: Made the "max event listeners" memory leak warning more
    accessible. (Anna Henningsen) nodejs/node#8298
    * promises: Unhandled rejections now emit a process warning after the
    first tick. (Benjamin Gruenbaum)
    nodejs/node#8223
    * repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
    nodejs/node#8241
    * util: Some functionality has been added to `util.inspect()`:
    - Returning `this` from a custom inspect function now works. (Anna
    Henningsen) nodejs/node#8174
    - Added support for Symbol-based custom inspection methods. (Anna
    Henningsen) nodejs/node#8174

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
jasnell pushed a commit that referenced this pull request Sep 27, 2016
crypto.timingSafeEqual() has been added in v6.6.0 cf. #8304

This commit adds the metadata that will display
"Added in: v6.6.0" and that can later be checked on
https://nodejs.org/api/crypto.html#crypto_crypto_timingsafeequal_a_b

PR-URL: #8796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell pushed a commit that referenced this pull request Sep 29, 2016
crypto.timingSafeEqual() has been added in v6.6.0 cf. #8304

This commit adds the metadata that will display
"Added in: v6.6.0" and that can later be checked on
https://nodejs.org/api/crypto.html#crypto_crypto_timingsafeequal_a_b

PR-URL: #8796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
crypto.timingSafeEqual() has been added in v6.6.0 cf. #8304

This commit adds the metadata that will display
"Added in: v6.6.0" and that can later be checked on
https://nodejs.org/api/crypto.html#crypto_crypto_timingsafeequal_a_b

PR-URL: #8796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants