Normative: Allow NaN values to be *optionally* canonicalized

[littledan]

This patch legalizes V8's occasional canonicalization of NaNs
by changing SetValueInBuffer to either a particular value
for the implementation-distinguishable NaN value or an
implementation-defined canonical value.

This semantic change reached consensus at the November 2016
TC39 meeting.

Closes #635

This patch still needs the associated test262 update. The tests that are affected include these, but maybe more:

built-ins/Object/internals/DefineOwnProperty/nan-equivalence
built-ins/TypedArray/prototype/fill/fill-values-conversion-operations-consistent-nan
built-ins/TypedArrays/internals/DefineOwnProperty/conversion-operation-consistent-nan
built-ins/TypedArrays/internals/Set/conversion-operation-consistent-nan

[littledan]

cc @erights @allenwb

[bterlson]

@allenwb does this seem fine to you? (Just checking as I think you were involved in the discussions closely at the last meeting).

[littledan]

What do we need from here to decide whether to land this patch?

@jfbastien has clarified that, even if this patch lands, we won't really have a full description of what happens with NaN values in practice. This is because processors sometimes change the NaN bit pattern later, e.g., in MOV instructions, in a way that JS implementations which don't canonicalize are probably not guarding against.

Even as I'd personally prefer that we leave the NaN bit pattern completely unspecified, I think this patch still brings us a little bit closer to reality and would be helpful to land.

[rwaldron]

I see that this change is only being made to the Float64 branch, should it also be applied to the Float32 branch just above?

[littledan]

Fixed.

[allenwb]

A agree with Rick. It should also apply to the Float32 case

Becasue I still agree with @rwaldron

[allenwb]

I agree with Rick.

In the Float32 case a Number value which may be a NaN is converted to a Float32 so a 32-bit NaN needs to be created from the 64-bing NaN. The bit encoding of the 32-bit NaN is observable in the buffer so we want the choice of encoding to be consistent within an implementaiton, just like for Float32.

[rwaldron]

👍

[littledan]

Thanks for the reviews everyone!

[ljharb]

@littledan would you mind rebasing this PR, and summarizing the current status? I believe it has consensus, and i'm not sure if it still needs tests.

@rwaldron can you rereview after the PR is rebased?

[littledan]

Well, I would not feel comfortable making the change suggested above, to give more guarantees to Float32, given that this is unlikely to correspond to actual implementations. I would rather move this patch to removing any guarantees about how floats are canonicalized, given the information from @jfbastien, about how this is the implementation reality and is unlikely to change. @waldemarhorwat encouraged this path as well. @erights, would you be OK with this change?

[erights]

Yes, this change looks good to me.

[littledan]

@erights To be concrete, I'm talking about, not this PR, but instead a PR to say that there's no stability for NaN (since this is the implementation reality).

[erights]

@erights To be concrete, I'm talking about, not this PR, but instead a PR to say that there's no stability for NaN (since this is the implementation reality).

Thanks for clarifying. I did not know that. Link to other PR?

[aapoalas]

It was suggested in Matrix that the entire language about choosing the same NaN when serialising multiple times should be removed as spec fiction. Perhaps this PR could be repurposed for that?

[ChALkeR]

in v8, the observable behavior is seemingly random between calls there
in might canonicalize some nans but not the other identical ones in the same code on different calls
if this is what the spec change is meant to signal then perhaps that could be made a bit more clear?

[erights]

observable behavior is seemingly random between calls

Why? Such observable dynamic non-determinism opens side-channels even when when observers cannot measure duration.

[ChALkeR]

@erights that's how v8 behaves

And, reading the proposed spec change now, I assume that standarizing that was the intention of this PR
Also the word "occasional" in the PR description
Also #758 (comment)

And I'm not sure that that's worded in the spec clearly enough?

const n = new Float64Array(new Uint8Array(8).fill(0xff).buffer)[0]
;[
  NaN, NaN, NaN, NaN, NaN, NaN,
  n, n, n, n, n, n, n,
].forEach((x, i) => {
  var str = new Uint8Array(new Float64Array([x]).buffer).toString()  
  console.log(i, str)
})

prints:

0 0,0,0,0,0,0,248,127
1 0,0,0,0,0,0,248,127
2 0,0,0,0,0,0,248,127
3 0,0,0,0,0,0,248,127
4 0,0,0,0,0,0,248,127
5 0,0,0,0,0,0,248,127
6 255,255,255,255,255,255,255,255
7 255,255,255,255,255,255,255,255
8 255,255,255,255,255,255,255,255
9 0,0,0,0,0,0,248,127
10 255,255,255,255,255,255,255,255
11 255,255,255,255,255,255,255,255
12 255,255,255,255,255,255,255,255

Note the value on 9

It does return "either a particular value for the implementation-distinguishable NaN value or an implementation-defined canonical value".

Which follows the proposed spec change.

The nuance is that it does that in a non-deterministic way

An implementation must always choose the same encoding for each implementation distinguishable NaN value, or an implementation-defined canonical value.

This can be misinterpreted in a way that one would think that it always chooses the same encoding or always returns an implementation-defined canonical value.

While what this actually means is that behavior can be either of that at any point.

[ljharb]

The intention of our 2015 Munich plenary discussion, from what I recall, is essentially that "outside of Typed Arrays, there is one distinguishable NaN in the language. Typed Arrays can expose bit patterns in an implementation-defined way - meaning, there is no consistency expected or required"

[ChALkeR]

@ljharb that makes sense, but non-determinism could be indeed a side-channel
was that considered?

e.g. Spectre was announced in 2018, after that discussion 🙃

[erights]

@erights that's how v8 behaves

Why does v8 behave that way?

[ChALkeR]

Why does v8 behave that way?

I'm unsure i'm just a passerby here sorry.

What I wanted to bring the attention to is that this spec change explicitly allows that, and that allowing that non-determinism seems like the intention¹ of this change (which was reviewed, even though, pre-Spectre).

And that the wording around that is not very obvious

Footnotes

1. from (1) "legalizes V8's occasional canonicalization" + me checking how V8 behaves, (2) word "optionally" in title, and (3) https://github.com/tc39/ecma262/pull/758#issuecomment-370379042, (4) https://github.com/tc39/ecma262/issues/635#issuecomment-2813725627 ↩

[ChALkeR]

Why does v8 behave that way?

Perhaps the answer would be "because that was the consensus of the discussion back in 2015"?

Also "because tc39/test262#1476 landed"?

[erights]

What I think I remember from those old times:

Mozilla/FF/SpiderMonkey used NaN-boxing, and so always canonicalized genuine NaNs. I don't know if it was the same canonical NaN on little endian vs big endian architectures.

Google was not (and I assume is not) boxing their NaNs, and was not willing to pay in performance to do so, when the perf cost would be significant (which I believe) and the benefit is so small "No one would care."

I remember resisting a bit but giving up in the face of this genuinely compelling argument from Google.

However, I still tried to find some limitation we could agree on that would limit observable dynamic side channels. What I should have anticipated is that if the difference is too hard to test, no one would test it and implementers would proceed to violate it with no feedback. I do not remember even thinking about how to write test262 tests.

In the era before Spectre and Meltdown, I am sure most of my fears about side channels fell on deaf ears.

Again, my memories are terribly stale. I am often inaccurate of such time gaps.

[erights]

Glad to see all that test262 attention to these issues! I did not remember that.

[ChALkeR]

@ljharb

Typed Arrays can expose bit patterns in an implementation-defined way

There is (usually) a difference between implementation-defined and undefined, and what's happening is more like "undefined". v8 does not define a consistent behavior for this.

Upd: I see the definition though:

An implementation-defined facility is one that defers its definition to an external source without further qualification. This specification does not make any recommendations for particular behaviours, and conforming implementations are free to choose any behaviour within the constraints put forth by this specification.

It's unfortunate that the spec doesn't put a difference between consistent and unknown-potentially-non-deterministic non-defined-in-spec behavior (like e.g. C++ spec does for defns.impl.defined / defns.unspecified / defns.undefined), and that "implementation defined" is misleading in that aspect. It's not actually defined anywhere.

[erights]

"implementation-defined" means not spec defined.

[michaelficarra]

@ChALkeR

It's unfortunate that the spec doesn't put a difference between consistent and unknown-potentially-non-deterministic non-defined-in-spec behavior (like e.g. C++ spec does for defns.impl.defined / defns.unspecified / defns.undefined), and that "implementation defined" is misleading in that aspect.

That's because we don't need to; there's no undefined behaviour in JS. There's plenty of places where implementations are given the choice to do something or not, or are given a choice of values to produce, or are given the freedom to approximate arithmetic results, but they are never given the opportunity to throw up their hands and do anything they want. If you have a specific suggestion for a place where you think that is unclear, I'd love to hear it.

It's not actually defined anywhere.

It's expected that conformant implementations would provide a description of all the choices their implementation makes in these positions. In practice, most implementations don't. Also in practice, most implementations (and their users) don't actually care about strict conformance.

The exact text in NumericToRawBytes is

If value is NaN, rawBytes may be set to any implementation chosen IEEE 754-2019 binary64 format NaN encoding. An implementation must always choose the same encoding for each implementation distinguishable NaN value.

Based on your observations, Chrome's behaviour does not conform with the latter part of that requirement.

[ChALkeR]

@michaelficarra

The exact text in NumericToRawBytes is

This (#758, where we are now) is a PR aimed to change that exact wording you cited and legalize Chrome behavior in spec.

And that is actually undefined non-deterministic behavior.

That's because we don't need to; there's no undefined behaviour in JS.

What exactly in the spec would forbid non-determinism there once this PR is landed?

It's expected that conformant implementations would provide a description of all the choices their implementation makes in these positions.

Compare ECMAScript implementation-defined to C++ implementation-defined and C++ unspecified.
The difference is requiring that behavior to be documented somewhere.

[o-]

V8 does not guarantee a stable NaN pattern. E.g., we use different patterns for different purposes. I checked the latest discussion notes on the topic I don't think that the idea was to request a particular NaN pattern but instead allow any NaN pattern to occur.

Post-spectre we abandoned the idea of in-process read isolation. Hence there is no need to canonicalize from that pov either.

[ChALkeR]

@erights

fears about side channels

Well, it's definitely a side channel in Chrome
Here is a demo: https://junk.rray.org/poc/nani.html

It can track gc and communicate over the side-channel cross-origin (in the same process), bypass iframe storage partitioning from different sites, detect devtools, etc.

[erights]

Thanks @ChALkeR , you've done what I never figured out how to do: demonstrate that it is a real side channel that should be concerning. Thanks for pointing out that it is

not using SharedArrayBuffer, timing, or anything outside of ECMAScript spec

After Meltdown and Spectre, it has been easy to raise the alarm about side channels that can be read by those who can somehow measure duration. It has been harder to get people to appreciate two bracketing issues:

• Hardened JS, and the SES shim of Hardened JS, are both able to deny confined code the ability to measure duration.
• Any dynamic spec non-determinism might enable side channels even to those that are denied the ability to measure duration. (Static spec non-determinism leads to static side channel concerns about fingerprinting, but lets keep that separate.)

See https://papers.agoric.com/taxonomy-of-security-issues/

GC observability is another timing-independent side channel. This is why we so carefully distinguished WeakMap/WeakSet (spec determinism) from WeakRef/FinalizationRegistry (observable gc decisions). And why we then tried to severely reduce the magnitude of that side channel (by making gc within a turn hard to observe, and by allowing the omission of the WeakRef.prototype.constructor === WeakRef link.) It is also why Hardened JS does not classify WeakRef and FinalizationRegistry as globals that are safe to provide and implicitly share.

It is also why we originally proposed that a WeakRef in realm A holding a target from distinct realm B act as a strong reference. This would severely limit the magnitude of the inter-realm gc-observation side channel. Alas, not all engines already had the bookkeeping needed to distinguish realm-of-origin. Implementors were understandably reluctant to consider pervasive extra bookkeeping for an obscure corner case.

[ChALkeR]

SES

SES directly contradicts both Node.js security model and (as it turns out) V8 security model too
It's in a state that there are existing caveats in Node.js that defeat SES but it's neither a Node.js bug nor SES responsibility - so no one is responsible for that, but the whole thing is very fragile.

I'd be happy to chat about that before posting a public rant about how it doesn't work 🙃 (which is smth I've been postponing for a while now)

I have demos
(As far as I remember - including on just the code from ses readme)

Feel free to DM me in OpenJSF Slack about that.

[ChALkeR]

For context, chromium issue which works as intended: https://issues.chromium.org/issues/485920246

[erights]

@ChALkeR , how did you come up with the exploits at https://junk.rray.org/poc/nani.html ? Reliably reproducible demos of NaN side channels! Several of us have looked, and it seems like black magic to discover these. Any advice you can offer for other to learn from?

[erights]

In light of @ChALkeR 's attack, I added back the "needs consensus" label. I understand why Chromium classifies this attack as "works as intended". But I do not think that tc39 should effectively do so itself.