FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: make test-crypto-timing-safe-equal-benchmarks robust by Trott · Pull Request #38493 · nodejs/node · GitHub

/ node Public

test: make test-crypto-timing-safe-equal-benchmarks robust - #38493

Closed
Trott wants to merge 1 commit into
nodejs:mainfrom
Trott:fileify
Closed

test: make test-crypto-timing-safe-equal-benchmarks robust#38493
Trott wants to merge 1 commit into
nodejs:mainfrom
Trott:fileify

Conversation

Trott commented May 1, 2021

Copy link
Copy Markdown
Member

Avoid possible V8 optimizations that may invalidate benchmark. This is
done by writing randm UUIDs to file and reading the files again as
needed, rather than having hardcoded strings for the buffer contents.

Refs: #38488 (comment)

Avoid possible V8 optimizations that may invalidate benchmark. This is
done by writing randm UUIDs to file and reading the files again as
needed, rather than having hardcoded strings for the buffer contents.

Refs: nodejs#38488 (comment)
github-actions Bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels May 1, 2021

Copy link
Copy Markdown
Collaborator

Trott commented May 1, 2021

Copy link
Copy Markdown
Member Author

Stress test against this PR: https://ci.nodejs.org/job/node-stress-single-test/298/

Recent stress test against master: https://ci.nodejs.org/job/node-stress-single-test/295/

Trott commented May 1, 2021

Copy link
Copy Markdown
Member Author

Stress test against this PR: https://ci.nodejs.org/job/node-stress-single-test/298/

Recent stress test against master: https://ci.nodejs.org/job/node-stress-single-test/295/

Stress test shows it being very reliable, but also a lot slower (unsurprisingly). But at least the test is what's slower and not the timingSafeEqual() implementation.

Copy link
Copy Markdown
Collaborator

Trott commented May 1, 2021

Copy link
Copy Markdown
Member Author

CI is green, stress test had only 4 failures in 1000 runs. I had to reduce the number of tries by a factor of ten but I think 4 failures is still way more than would be expected if there was a normal distribution. I guess I still think adding HMAC as in #38488 is the way to go but I'll absolutely defer to those more knowledgable about crypto, security, and statistics.

Copy link
Copy Markdown
Member

I guess I still think adding HMAC as in #38488 is the way to go but I'll absolutely defer to those more knowledgable about crypto, security, and statistics.

@Trott Is it possible that #38488 only appears to fix the problem because the time to compute HMAC far outweighs any comparison operations and the effects of optimizations?

Trott commented May 12, 2021

Copy link
Copy Markdown
Member Author

I guess I still think adding HMAC as in #38488 is the way to go but I'll absolutely defer to those more knowledgable about crypto, security, and statistics.

@Trott Is it possible that #38488 only appears to fix the problem because the time to compute HMAC far outweighs any comparison operations and the effects of optimizations?

I believe that it actually solves the problem, not merely appears to, for exactly that reason, yes. As I understand it, that is the intended effect of adding HMAC, not a side effect.

@not-an-aardvark Is that correct?

Trott commented May 12, 2021

Copy link
Copy Markdown
Member Author

I guess I still think adding HMAC as in #38488 is the way to go but I'll absolutely defer to those more knowledgable about crypto, security, and statistics.

@Trott Is it possible that #38488 only appears to fix the problem because the time to compute HMAC far outweighs any comparison operations and the effects of optimizations?

I believe that it actually solves the problem, not merely appears to, for exactly that reason, yes. As I understand it, that is the intended effect of adding HMAC, not a side effect.

@not-an-aardvark Is that correct?

Oh wait, now I'm thinking I'm misremembering. It's been a while since I looked at this. Yeah, I guess the idea is HMAC results in comparing hashes which should be the same (using the OpenSSL timing safe equal comparison) whether there's success or failure? I can't keep things straight.

Yeah, it's entirely possible that the HMAC thing "fixes" it by slowing introducing a ton of extra work basically resulting in too much noise in the timing measurements so differentiating timing on success vs. failure becomes harder. I actually, uh, am not sure.

Copy link
Copy Markdown
Contributor

It's unclear to me whether adding an HMAC in #38488 fixed this test -- that would depend on what specifically was causing the test to fail in the first place, which IIUC we never nailed down.

However, the main advantage of #38488 is that if it was merged, we would no longer need to care about this test, because we would be able to have much higher confidence that the function is timing-safe, regardless of whether it's constant-time. So if we merged #38488 and this test started being flaky again in the future, we could just safely delete it.

Trott commented May 16, 2021

Copy link
Copy Markdown
Member Author

I'd like to find some way to gather minds to talk about this and #38488 and decide once-and-for-all which of the two (or both or neither) should land. It seems to me that we would benefit from:

  1. A summary document explaining what is known and what questions are unanswered
  2. A list of people that should be involved in the conversation
  3. Some way to determine the best way to have the conversation and ensure it actually comes to some kind of conclusion

For the first item, I'll do my best to draft a document, although if someone more informed on crypto timing issues wants to do it, I'll surely defer to them.

For the second item, offhand, I'd think the list would be @not-an-aardvark, @panva, @tniessen, @bnoordhuis (if they're available and willing/interested), and me. @jasnell has been involved in some of this stuff too, but I'm not sure how much they care to (or have time to) dive into the details on this. They're welcome, though, if it's of interest.

For the third item, I guess I'd prefer a real-time conversation over Zoom or something else, but I know that may not work for everyone of course, and may not be workable at all depending on time zones, schedules, fluency with spoken English, network reliability, and probably a million other things. Open to other ideas: IRC/Slack? Email? GitHub issue or discussion?

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL