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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions doc/api/crypto.md
Original file line number Diff line number Diff line change
Expand Up @@ -5443,8 +5443,11 @@ changes:
* `b` {ArrayBuffer|Buffer|TypedArray|DataView}
* Returns: {boolean}

This function is based on a constant-time algorithm.
Returns true if `a` is equal to `b`, without leaking timing information that
This function compares the underlying bytes that represent the given
`ArrayBuffer`, `TypedArray`, or `DataView` instances using a constant-time
algorithm.

This function does not leak timing information that
would allow an attacker to guess one of the values. This is suitable for
comparing HMAC digests or secret values like authentication cookies or
[capability urls](https://www.w3.org/TR/capability-urls/).
Expand All @@ -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

`Float64Array`s, this function might return unexpected results due to IEEE 754
encoding of floating-point numbers. In particular, neither `x === y` nor
`Object.is(x, y)` implies that the byte representations of two floating-point
numbers `x` and `y` are equal.</strong>

Use of `crypto.timingSafeEqual` does not guarantee that the _surrounding_ code
is timing-safe. Care should be taken to ensure that the surrounding code does
not introduce timing vulnerabilities.
Expand Down
35 changes: 35 additions & 0 deletions test/sequential/test-crypto-timing-safe-equal.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,41 @@ assert.strictEqual(
}
}

{
// When the inputs are floating-point numbers, timingSafeEqual neither has
// equality nor SameValue semantics. It just compares the underlying bytes,
// ignoring the TypedArray type completely.

const cmp = (fn) => (a, b) => a.every((x, i) => fn(x, b[i]));
const eq = cmp((a, b) => a === b);
const is = cmp(Object.is);

function test(a, b, { equal, sameValue, timingSafeEqual }) {
assert.strictEqual(eq(a, b), equal);
assert.strictEqual(is(a, b), sameValue);
assert.strictEqual(crypto.timingSafeEqual(a, b), timingSafeEqual);
}

test(new Float32Array([NaN]), new Float32Array([NaN]), {
equal: false,
sameValue: true,
timingSafeEqual: true
});

test(new Float64Array([0]), new Float64Array([-0]), {
equal: true,
sameValue: false,
timingSafeEqual: false
});

const x = new BigInt64Array([0x7ff0000000000001n, 0xfff0000000000001n]);
test(new Float64Array(x.buffer), new Float64Array([NaN, NaN]), {
equal: false,
sameValue: true,
timingSafeEqual: false
});
}

assert.throws(
() => crypto.timingSafeEqual(Buffer.from([1, 2, 3]), Buffer.from([1, 2])),
{
Expand Down