doc,test: clarify timingSafeEqual semantics by tniessen · Pull Request #43228 · nodejs/node · GitHub

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

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

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.

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.

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

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.

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

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.

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`

@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:

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

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

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.

Which part is incorrect? The test that's added in this PR demonstrates that timingSafeEqual() can return false even when Object.is() returns true.

ping @mscdex

ping @mscdex @nodejs/documentation @nodejs/crypto

CI: https://ci.nodejs.org/job/node-test-pull-request/44774/

Landed in 800cff1

PR-URL: #43228 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs/node#43228 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>

tniessen added crypto Issues and PRs related to the crypto subsystem. security Issues and PRs related to security. labels May 28, 2022

tniessen requested a review from Trott May 28, 2022 10:30

nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 28, 2022

tniessen force-pushed the doc-crypto-timingsafeequal-floats branch from 289bf9e to 6569cdc Compare May 28, 2022 15:06

tniessen changed the title doc: warn about using timingSafeEqual with floats doc,test: clarify timingSafeEqual semantics May 28, 2022

tniessen force-pushed the doc-crypto-timingsafeequal-floats branch from 6569cdc to 2f626d3 Compare May 28, 2022 15:07

mscdex reviewed May 28, 2022

View reviewed changes

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

doc,test: clarify timingSafeEqual semantics

20e007c

tniessen force-pushed the doc-crypto-timingsafeequal-floats branch from ad12313 to 20e007c Compare May 28, 2022 16:42

mscdex reviewed May 28, 2022

View reviewed changes

aymen94 approved these changes May 30, 2022

View reviewed changes

tniessen added the review wanted PRs that need reviews. label Jun 1, 2022

bnoordhuis approved these changes Jun 20, 2022

View reviewed changes

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 outstanding review comments, and a CI started. labels Jun 20, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 20, 2022

This comment was marked as outdated.

Sign in to view

github-actions Bot mentioned this pull request Jun 21, 2022

CI Reliability 2022-06-21 nodejs/reliability#307

Open

33 tasks

Trott approved these changes Jun 21, 2022

View reviewed changes

tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 21, 2022

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 merged commit 800cff1 into nodejs:main Jun 21, 2022

This was referenced Jun 22, 2022

CI Reliability 2022-06-22 nodejs/reliability#308

Open

CI Reliability 2022-06-23 nodejs/reliability#309

Open

CI Reliability 2022-06-24 nodejs/reliability#310

Open

This was referenced Jun 25, 2022

CI Reliability 2022-06-25 nodejs/reliability#311

Open

CI Reliability 2022-06-26 nodejs/reliability#312

Open

targos pushed a commit that referenced this pull request Jul 12, 2022

doc,test: clarify timingSafeEqual semantics

462e526

PR-URL: #43228 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>

targos mentioned this pull request Jul 12, 2022

v18.6.0 release proposal #43789

Merged

targos pushed a commit that referenced this pull request Jul 20, 2022

doc,test: clarify timingSafeEqual semantics

da098ef

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

doc,test: clarify timingSafeEqual semantics

7be07c7

PR-URL: #43228 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022

doc,test: clarify timingSafeEqual semantics

95cf67e

PR-URL: nodejs/node#43228 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>

coderabbitai Bot mentioned this pull request Jul 31, 2026

feat(090): fecha implementacao da fase 1 de papeis globais FarenRavirar/artificio#234

Merged

               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

doc,test: clarify timingSafeEqual semantics by tniessen · Pull Request #43228 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc,test: clarify timingSafeEqual semantics - #43228

doc,test: clarify timingSafeEqual semantics#43228nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom tniessen:doc-crypto-timingsafeequal-floatstniessen/node:doc-crypto-timingsafeequal-floatsCopy head branch name to clipboard

Conversation

tniessen commented May 28, 2022

Uh oh!

Trott commented May 28, 2022

Uh oh!

mscdex commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented May 28, 2022

Uh oh!

mscdex commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented May 28, 2022

Uh oh!

mscdex May 28, 2022

Choose a reason for hiding this comment

Uh oh!

bnoordhuis May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tniessen May 28, 2022

Choose a reason for hiding this comment

Uh oh!

mscdex May 28, 2022

Choose a reason for hiding this comment

Uh oh!

tniessen May 28, 2022

Choose a reason for hiding this comment

Uh oh!

Trott commented May 28, 2022

Uh oh!

tniessen commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex May 28, 2022

Choose a reason for hiding this comment

Uh oh!

tniessen May 28, 2022

Choose a reason for hiding this comment

Uh oh!

tniessen Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

tniessen commented Jun 20, 2022

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Jun 21, 2022

Uh oh!

nodejs-github-bot commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

doc,test: clarify timingSafeEqual semantics#43228
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
tniessen:doc-crypto-timingsafeequal-floats

mscdex commented May 28, 2022 •
edited
Loading

mscdex commented May 28, 2022 •
edited
Loading

bnoordhuis May 28, 2022 •
edited
Loading

tniessen commented May 28, 2022 •
edited
Loading