1357330 - Frozen NaN-valued property can be overwritten with different NaN value

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

Test case:
---
function byteValue(value) {
  var isLittleEndian = new Uint8Array(new Uint16Array([1]).buffer)[0] !== 0;
  var ui8 = new Uint8Array(new Float64Array([value]).buffer);

  var hex = "0123456789ABCDEF";
  var s = "";
  for (var i = 0; i < 8; ++i) {
    var v = ui8[isLittleEndian ? 7 - i : i];
    s += hex[(v >> 4) & 0xf] + hex[v & 0xf];
  }
  return s;
}

var obj = {};
Object.defineProperty(obj, "prop", {value: NaN});
Object.defineProperty(obj, "prop", {value: -NaN});

assertEq(byteValue(obj.prop), byteValue(NaN));
---

Expected: Test passes
Actual: Assertion failed: got "FFF8000000000000", expected "7FF8000000000000"

Comment 1

[André Bargull [:anba]]

Attachment: bug1357330.patch

Adds the missing step 7.a.i.3. from ES2018 9.1.6.3 ValidateAndApplyPropertyDescriptor. Also updates a couple of outdated comments and replaces manual bit-testing for shape attributes with the existing IsXXX helpers.

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8859285 [details] [diff] [review]
bug1357330.patch

Review of attachment 8859285 [details] [diff] [review]:
-----------------------------------------------------------------

Nice find, could you commit a test for this as well?

Out of curiosity, was this observable some other way than distinct NaNs before, like Proxy traps?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

What's the severity of this and how far back does it go?

Comment 4

[Shu-yu Guo [:shu]]

Is this the immediately exploitable kind of security-sensitive or is it the leaking information kind of security-sensitive?

Comment 5

[André Bargull [:anba]]

(In reply to [Out of Office Until 24-April] Ryan VanderMeulen [:RyanVM] from comment #3)
> What's the severity of this and how far back does it go?

I think it goes back to bug 1125624 and bug 1148750. For the severity I'd probably go with sec-other, because this issue is limited to different NaN values and when similar issues happened in the past (search for "non-configurable" or "frozen"), most of the time we didn't even treat them as sec-bugs.

Comment 6

[André Bargull [:anba]]

Attachment: bug1357330-testcase.patch

Test case from comment #0. 

We'll get more thorough coverage through test262 as soon as I fix (and extend) [1].

[1] https://github.com/tc39/test262/blob/master/test/built-ins/Object/internals/DefineOwnProperty/nan-equivalence.js

Comment 7

[André Bargull [:anba]]

(In reply to Shu-yu Guo [:shu] from comment #2)
> Nice find, could you commit a test for this as well?

Added.

> Out of curiosity, was this observable some other way than distinct NaNs
> before, like Proxy traps?

No, I don't think so. This should only be reproducible with native objects.

Comment 8

[André Bargull [:anba]]

Comment on attachment 8859497 [details] [diff] [review]
bug1357330-testcase.patch

Forgot to qref, will add the proper patch later. :-/

Comment 9

[André Bargull [:anba]]

Attachment: bug1357330-testcase.patch

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76bff2cde94c44b7bd1c3d2ce847db095175577f
https://hg.mozilla.org/integration/mozilla-inbound/rev/da5a14ae4ce3084aa63a502e96af1a4c6ef76853

Doesn't sound like it's worth backporting to ESR52 (and Fx54 RC build is this week, so won't be going there either), but feel free to set the status back to affected and argue for it if you feel otherwise.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76bff2cde94c
https://hg.mozilla.org/mozilla-central/rev/da5a14ae4ce3