test: make test-crypto-timing-safe-equal-benchmarks robust #38493
Conversation
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
|
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. |
|
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. |
@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. |
|
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. |
|
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:
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? |
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)