-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
doc,test: clarify timingSafeEqual semantics #43228
doc,test: clarify timingSafeEqual semantics #43228
Conversation
Should we add the false positive and false negative as a test (maybe in the known_issues directory?) so that we know to update the docs if it ever gets changed/fixed? |
I don't understand the problem here. The two examples given have to do with the larger "problem" of JavaScript equality operator behavior ( On a related note, you could use |
@mscdex The current documentation does not explicitly say so, and that's exactly what this PR is trying to address.
This would apply to any language that implements a subset of IEE 754.
Sure, but this function is specifically for constant-time comparisons, which |
Perhaps we should say exactly that then, as the current language this PR adds doesn't make that obvious IMO. Something like "This method compares the underlying bytes that represent any typed array or Buffer."
I was referring to the equality checks inside the |
289bf9e
to
6569cdc
Compare
6569cdc
to
2f626d3
Compare
Added a test and reworded to closer match JS's understanding of equal versus same. |
doc/api/crypto.md
Outdated
@@ -5453,6 +5456,15 @@ comparing HMAC digests or secret values like authentication cookies or | |||
must have the same byte length. An error is thrown if `a` and `b` have | |||
different byte lengths. | |||
|
|||
This function does not compare the elements of `a` and `b` directly. Instead, it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant before was adding the note about "underlying bytes" instead of adding this text. If we state we're comparing the underlying bytes, then this text isn't very useful and makes things more confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I'd just leave it at "compares bytes" and give it one or two examples. 0 vs. -0 is probably the easiest to understand but different NaNs could work too, e.g.:
const x = new BigInt64Array(["0x7ff0000000000001", "0xfff0000000000001"])
const a = new Float64Array(x.buffer)
const b = new Float64Array([NaN, NaN])
Number.isNaN(a[0]) // true
Number.isNaN(a[1]) // true
crypto.timingSafeEqual(a, b) // false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a related note, you could use
Object.is()
instead, which should give you the equality checking you're looking for.
Then I guess this earlier comment wasn't correct? Object.is()
is true, unlike timingSafeEqual
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I guess this earlier comment wasn't correct? Object.is() is true, unlike timingSafeEqual.
As I said:
I was referring to the equality checks inside the .every() callbacks.
To clarify further:
Object.is(0, -0) === false // so of course `timingSafeEqual()` returns `false`
Object.is(NaN, NaN) === true // so of course `timingSafeEqual()` returns `true`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mscdex Your comment appears to imply some (obvious) connection between Object.is()
and timingSafeEqual()
, but @bnoordhuis' example seems to contradict that:
const a = new Float64Array(new BigInt64Array([0x7ff0000000000001n]).buffer);
const b = new Float64Array(new BigInt64Array([0xfff0000000000001n]).buffer);
Object.is(a[0], b[0]); // true
crypto.timingSafeEqual(a, b); // false
LGTM but pinging some other folks on these questions:
@nodejs/documentation @nodejs/crypto @nodejs/testing |
2f626d3
to
ad12313
Compare
Edit: Reworded and extended again since this does not match @Trott I wouldn't consider it a bug, just a very unusual and permissive function signature... for example, comparing a Edit: As @bnoordhuis pointed out, the semantics are neither equality nor same value. Accepting |
ad12313
to
20e007c
Compare
@@ -5457,6 +5460,12 @@ If at least one of `a` and `b` is a `TypedArray` with more than one byte per | |||
entry, such as `Uint16Array`, the result will be computed using the platform | |||
byte order. | |||
|
|||
<strong class="critical">When both of the inputs are `Float32Array`s or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is still incorrect and misleading. If the float/double values are the same, then timingSafeEqual()
will return true
. If the values differ, then timingSafeEqual()
will return false
.
0
and -0
are different values. The fact that the ===
operator treats them as being equal is a language "issue", much like how ==
can return true
for two different value types due to type coercion.
timingSafeEqual()
is doing the correct and expected comparison here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which part is incorrect? The test that's added in this PR demonstrates that timingSafeEqual()
can return false even when Object.is()
returns true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @mscdex
ping @mscdex @nodejs/documentation @nodejs/crypto |
This comment was marked as outdated.
This comment was marked as outdated.
Landed in 800cff1 |
PR-URL: nodejs#43228 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: #43228 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: #43228 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: #43228 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs/node#43228 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Example of a false positive:
Example of a false negative:
(We should probably consider doc-deprecating this or at least we should be more careful about what
TypedArray
s we allow elsewhere.)