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

doc,test: clarify timingSafeEqual semantics #43228

Merged

Conversation

tniessen
Copy link
Member

Example of a false positive:

const a = new Float32Array(10).fill(NaN);
a.every((x, i) => a[i] === x)  // false
crypto.timingSafeEqual(a, a)  // true

Example of a false negative:

const p0 = new Float32Array(10).fill(0);
const m0 = new Float32Array(10).fill(-0);
p0.every((x, i) => m0[i] === x)  // true
crypto.timingSafeEqual(p0, m0)  // false

(We should probably consider doc-deprecating this or at least we should be more careful about what TypedArrays we allow elsewhere.)

@tniessen tniessen added crypto Issues and PRs related to the crypto subsystem. security Issues and PRs related to security. labels May 28, 2022
@tniessen tniessen requested a review from Trott May 28, 2022 10:30
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 28, 2022
@Trott
Copy link
Member

Trott commented May 28, 2022

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?

@mscdex
Copy link
Contributor

mscdex commented May 28, 2022

I don't understand the problem here. timingSafeEqual() compares bytes, so the results seem expected to me.

The two examples given have to do with the larger "problem" of JavaScript equality operator behavior (NaN !== NaN and 0 === -0) rather than any gotcha with timingSafeEqual(). You can run into these same problems outside of this crypto context because they're language issues.

On a related note, you could use Object.is() instead, which should give you the equality checking you're looking for.

@tniessen
Copy link
Member Author

timingSafeEqual() compares bytes

@mscdex The current documentation does not explicitly say so, and that's exactly what this PR is trying to address.

because they're language issues.

This would apply to any language that implements a subset of IEE 754.

On a related note, you could use Object.is() instead, which should give you the equality checking you're looking for.

Sure, but this function is specifically for constant-time comparisons, which Object.is() does not implement.

@mscdex
Copy link
Contributor

mscdex commented May 28, 2022

The current documentation does not explicitly say so, and that's exactly what this PR is trying to address.

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

Sure, but this function is specifically for constant-time comparisons, which Object.is() does not implement.

I was referring to the equality checks inside the .every() callbacks.

@tniessen tniessen force-pushed the doc-crypto-timingsafeequal-floats branch from 289bf9e to 6569cdc Compare May 28, 2022 15:06
@tniessen tniessen changed the title doc: warn about using timingSafeEqual with floats doc,test: clarify timingSafeEqual semantics May 28, 2022
@tniessen tniessen force-pushed the doc-crypto-timingsafeequal-floats branch from 6569cdc to 2f626d3 Compare May 28, 2022 15:07
@tniessen
Copy link
Member Author

Added a test and reworded to closer match JS's understanding of equal versus same.

@@ -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
Copy link
Contributor

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.

Copy link
Member

@bnoordhuis bnoordhuis May 28, 2022

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

Copy link
Member Author

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.

Copy link
Contributor

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`

Copy link
Member Author

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

@Trott
Copy link
Member

Trott commented May 28, 2022

LGTM but pinging some other folks on these questions:

  • Is the documentation as clear and concise on this as we can make it?
  • Should the testing note that this is a surprising feature? Is it really a bug and should be in known_issues? Or is this expected behavior, should not change, and the test is in the right place?

@nodejs/documentation @nodejs/crypto @nodejs/testing

@tniessen tniessen force-pushed the doc-crypto-timingsafeequal-floats branch from 2f626d3 to ad12313 Compare May 28, 2022 15:33
@tniessen
Copy link
Member Author

tniessen commented May 28, 2022

@mscdex Reworded again and removed most of the text as requested.

Edit: Reworded and extended again since this does not match Object.is() semantics.

@Trott I wouldn't consider it a bug, just a very unusual and permissive function signature... for example, comparing a Float64Array to a Uint32Array also seems unusual (and probably unintentional).

Edit: As @bnoordhuis pointed out, the semantics are neither equality nor same value. Accepting Float(32|64)Array seems questionable to me.

@tniessen tniessen force-pushed the doc-crypto-timingsafeequal-floats branch from ad12313 to 20e007c Compare May 28, 2022 16:42
@@ -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
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @mscdex

@tniessen tniessen added the review wanted PRs that need reviews. label Jun 1, 2022
@tniessen
Copy link
Member Author

ping @mscdex @nodejs/documentation @nodejs/crypto

@tniessen tniessen added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 20, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 20, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 21, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 21, 2022
@nodejs-github-bot nodejs-github-bot merged commit 800cff1 into nodejs:main Jun 21, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 800cff1

F3n67u pushed a commit to F3n67u/node that referenced this pull request Jun 24, 2022
PR-URL: nodejs#43228
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #43228
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jul 20, 2022
PR-URL: #43228
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43228
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43228
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. review wanted PRs that need reviews. security Issues and PRs related to security.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants