WebGL data-view-test.html crash [@ js::types::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) ]

RESOLVED FIXED in Firefox 25, Firefox OS v1.2

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: vlad, Assigned: jandem)

Tracking

({crash, sec-critical, testcase})

unspecified
mozilla27
x86
Windows 8
crash, sec-critical, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox24 wontfix, firefox25+ verified, firefox26+ verified, firefox27+ verified, firefox-esr17 unaffected, firefox-esr2425+ verified, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed)

Details

(Whiteboard: [adv-main25+][adv-esr24-1+])

Attachments

(4 attachments, 1 obsolete attachment)

https://www.khronos.org/registry/webgl/sdk/tests/conformance/typedarrays/data-view-test.html

crashes on load in the 20130408 windows 32-bit nightly.  All crashes are at:

https://crash-stats.mozilla.com/report/index/bp-f1a443a7-070a-4211-bed2-8690c2130409
Might be a dup of bug 706438
Summary: WebGL data-view-test.html crashes → WebGL data-view-test.html crash [@ js::types::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) ]
Near-null but in the type system so still potentially scary. See prior art bug 736830 (marked DUP of a fixed bug) and bug 706438.
Looks like this involves BaselineCompiler and DataViews.
Keywords: sec-high
Keywords: crash, testcase
Unlikely to be related to bug 706438, that's a pretty common function and there must be zillions of ways to screw up the code it's checking. Similarly not all crashes in js_Interpret are the same thing.
Vlad, can you still reproduce this?  Works for me in OSX.
Flags: needinfo?(vladimir)
Yup -- just crashed on 20130612 nightly.  https://crash-stats.mozilla.com/report/index/bp-200d3d86-9f26-49b3-8efd-f8fc22130613
Flags: needinfo?(vladimir)
(Assignee)

Comment 7

4 years ago
Vlad, can you still reproduce this?

It doesn't crash on OS X, I also tried a 20130612 nightly. I wonder if this is Windows only somehow.
Flags: needinfo?(vladimir)
Very much still crashes.  Win32, 20131011 nightly: https://crash-stats.mozilla.com/report/index/7952f90d-8544-4109-aa64-d98bf2131015
Flags: needinfo?(vladimir)
Different signature now though -- matches bug 901333 now.
(Assignee)

Updated

4 years ago
Flags: needinfo?(jdemooij)
(Assignee)

Comment 10

4 years ago
I can reproduce the crash on Windows 7. Investigating.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
(Assignee)

Comment 11

4 years ago
I expected this to be a JIT bug, but it seems to be a DataView bug. Minimal testcase:

<script>
function f() {
    var arrayBuffer = (new Uint8Array([255, 255, 255, 127])).buffer;
    var view = new DataView(arrayBuffer, 0, arrayBuffer.byteLength);
    document.write(view.getFloat32(0));
}
f();
</script>

Report: https://crash-stats.mozilla.com/report/index/af128a85-ae66-4b1b-9c39-9fb2a2131015

It looks like we're not canonicalizing the NaN value correctly and interpreting it as a Value. DataViewObject::getFloat32Impl correctly calls JS_CANONICALIZE_NAN though, so maybe it's an MSVC bug? 

I also can't get this to crash in a Windows shell I downloaded. Will try a debug build now.
static inline double
JS_CANONICALIZE_NAN(double d)
{
    if (MOZ_UNLIKELY(d != d)) {
        jsval_layout l;
        l.asBits = 0x7FF8000000000000LL;
        return l.asDouble;
    }
    return d;
}

It's entirely possible |d != d| should be |mozilla::DoubleIsNaN(d)|.  We've had problems with multiple compilers in the past checking for NaN-ness in pretty much any way but using that method, no matter what *should* happen for that expression.  There's a reasonable chance that'll fix this, sadly.
Hmm. PGO bug maybe? I don't think the shell gets PGO'd..
You could try a nightly debug build, or we may even have opt non-PGO builds sitting around somewhere.
(Assignee)

Comment 15

4 years ago
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #13)
> Hmm. PGO bug maybe? I don't think the shell gets PGO'd..

Yeah I just wanted to post that: I downloaded a Win32 opt build with PGO and one without and it seems PGO only... I will do Try builds with some changes and see what works.

This means PGO is introducing random sec-critical bugs, that most of our non-Windows, shell-only fuzzers won't find :(
(Assignee)

Updated

4 years ago
Flags: needinfo?(jdemooij)
(Assignee)

Comment 16

4 years ago
PGO Try build, renames JS_CANONICALIZE_NAN to JS::CanonicalizeNaN and implements it using mozilla::IsNaN and JS::GenericNaN:

https://tbpl.mozilla.org/?tree=Try&rev=acdd62310585
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #13)
> Hmm. PGO bug maybe? I don't think the shell gets PGO'd..

The shell does not get PGOed. (We could do that, but unless we were linking js.exe against mozjs.dll it wouldn't be using the same optimized binary anyway.)
(In reply to Jan de Mooij [:jandem] from comment #15)
> (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #13)
> > Hmm. PGO bug maybe? I don't think the shell gets PGO'd..
> 
> Yeah I just wanted to post that: I downloaded a Win32 opt build with PGO and
> one without and it seems PGO only... I will do Try builds with some changes
> and see what works.
> 
> This means PGO is introducing random sec-critical bugs, that most of our
> non-Windows, shell-only fuzzers won't find :(

As a vocal opponent of PGO, I'd like to say that this would be far from the first incident. PGO has introduced both top crashes and sg:crit bugs in many past releases.
(Assignee)

Comment 19

4 years ago
(In reply to David Anderson [:dvander] from comment #18)
> As a vocal opponent of PGO, I'd like to say that this would be far from the
> first incident. PGO has introduced both top crashes and sg:crit bugs in many
> past releases.

Yeah I wonder if we should start disabling MSVC PGO for most of js/src as a start (with pragmas or something better). I've had to work around multiple MSVC PGO bugs, they're always time consuming to track down and can be introduced by random changes. I know others had the same experience :(
(Assignee)

Comment 20

4 years ago
(In reply to Jan de Mooij [:jandem] from comment #16)
> PGO Try build, renames JS_CANONICALIZE_NAN to JS::CanonicalizeNaN and
> implements it using mozilla::IsNaN and JS::GenericNaN:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=acdd62310585

This build no longer crashes. I made some small changes and will put the patch up for review if this one is also stable:

https://tbpl.mozilla.org/?tree=Try&rev=8a39927d5e7e
(Assignee)

Comment 21

4 years ago
Created attachment 817824 [details] [diff] [review]
Patch
Attachment #817824 - Flags: review?(jwalden+bmo)
(In reply to Jan de Mooij [:jandem] from comment #19)
> Yeah I wonder if we should start disabling MSVC PGO for most of js/src as a
> start (with pragmas or something better). I've had to work around multiple
> MSVC PGO bugs, they're always time consuming to track down and can be
> introduced by random changes. I know others had the same experience :(

Unfortunately, I think js is where most of the PGO performance wins have come from.
That it was possible to eyeball the code (given a reduced testcase) and identify the issue, to me, suggests that it's somewhat inapposite to blame PGO.  In cases where it's a random failure of a kind we've not seen before, and had to debug from scratch, okay.  But this was an easily identified pattern that we knew was failure-prone before, and which we *should* be able to catch in reviews and such.

I'd bet comparing any float/double to itself is something we could easily use the static-analysis builds to flag.  I filed bug 927430 to add this to the clang static-analysis plugin.
Comment on attachment 817824 [details] [diff] [review]
Patch

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

Could be worth a cover bug for this change, to hide that it's a security issue, even after sec-approval and all that.  Presumably the PGO'd assembly is easy enough to figure out how to subvert here, that extra caution is warranted.

::: js/src/vm/StructuredClone.cpp
@@ +433,5 @@
>          double d;
>      } pun;
>      if (!read(&pun.u))
>          return false;
> +    *p = CanonicalizeNaN(pun.d);

Hm, let's hope this works.

::: js/src/vm/TypedArrayObject.cpp
@@ +53,5 @@
>  
>  using mozilla::IsNaN;
>  using mozilla::PodCopy;
>  using JS::GenericNaN;
> +using JS::CanonicalizeNaN;

C goes before G.
Attachment #817824 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 25

4 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #24)
> Could be worth a cover bug for this change, to hide that it's a security
> issue, even after sec-approval and all that. 

I can make it look like a standard refactoring/renaming, but how do we handle branch patches in that case? Or should the cover bug be for a (top-)crash or something?
No good way to handle the branch patch case, other than to coordinate with drivers and land as late in the cycle as possible.  :-\  Or, actually, you *could* fake it with a totally bogus topcrash, yeah.  But it's perhaps a bunch of work to make it look convincing, it's dubiously ethical (even if in pursuit of a good end), and I wouldn't be willing to do it if it were me.
(Assignee)

Comment 27

4 years ago
Created attachment 818040 [details] [diff] [review]
Patch v2

Fixes the nit. I don't think a cover bug is much help if we have to land on branches anyway, but I will let the security team decide.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
It's not easy but it's possible to guess what's going on for somebody who knows what he's doing.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
Release, beta, aurora, esr24. Windows-only so does not affect b2g branches.

> If not all supported branches, which bug introduced the flaw?
Unknown.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be very easy.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely to cause regressions.
Attachment #817824 - Attachment is obsolete: true
Attachment #818040 - Flags: sec-approval?
Attachment #818040 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: sec-high → sec-critical
Comment on attachment 818040 [details] [diff] [review]
Patch v2

sec-approval+.

Please create and nominate a patch for Aurora, Beta, and ESR24.
status-b2g18: --- → unaffected
status-firefox24: --- → wontfix
status-firefox25: --- → affected
status-firefox26: --- → affected
status-firefox27: --- → affected
status-firefox-esr17: --- → unaffected
status-firefox-esr24: --- → affected
tracking-firefox27: --- → +
tracking-firefox-esr24: --- → 25+
You can always sneak in the fix in a merge commit -- just land a simple test fix or something on somewhere non-tip, do hg merge, and before hg commit-ing, sneak in your changes.  :-)
Certain sheriffs have been known to be amenable to doing such things too when pinged. Though this is a lot more complicated since it involves branch uplifts as well.

My vote is for just waiting until the latest point in the cycle possible before landing.
Ryan, the issue here is that if we're taking it on Beta, we want to get it in a little-b beta or two.
(Assignee)

Comment 32

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f9a19a57b9
(Assignee)

Comment 33

4 years ago
Comment on attachment 818040 [details] [diff] [review]
Patch v2

sec-approval+ from abillings in comment 28 but forgot to set the flag.
Attachment #818040 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 34

4 years ago
Created attachment 818322 [details] [diff] [review]
Patch for Aurora

[Approval Request Comment]
> Bug caused by (feature/regressing bug #):
Unknown

> User impact if declined: 
Security bugs, crashes on Windows.

> Testing completed (on m-c, etc.):
On m-i.

> Risk to taking this patch (and alternatives if risky): 
Low risk.

> String or IDL/UUID changes made by this patch:
None.
Attachment #818322 - Flags: review+
Attachment #818322 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 35

4 years ago
Created attachment 818332 [details] [diff] [review]
Patch for Beta

[Approval Request Comment]
> Bug caused by (feature/regressing bug #):
Unknown.

> User impact if declined: 
Security bugs and crashes on Windows.

> Testing completed (on m-c, etc.):
On m-i.

> Risk to taking this patch (and alternatives if risky): 
Low risk.

> String or IDL/UUID changes made by this patch:
None.
Attachment #818332 - Flags: review+
Attachment #818332 - Flags: approval-mozilla-beta?
(Assignee)

Comment 36

4 years ago
Created attachment 818354 [details] [diff] [review]
Patch for ESR24

[Approval Request Comment]
> User impact if declined: 
Security bugs and crashes on Windows.

> Fix Landed on Version:
Will be backported to beta (25).

> Risk to taking this patch (and alternatives if risky): 
Very low risk.

> String or UUID changes made by this patch: 
None.
Attachment #818354 - Flags: review+
Attachment #818354 - Flags: approval-mozilla-esr24?
(In reply to Jan de Mooij [:jandem] from comment #35)
> [Approval Request Comment]
> > Bug caused by (feature/regressing bug #):
> Unknown.

This probably dates back to https://hg.mozilla.org/integration/mozilla-inbound/rev/cb31ec3a3325 which would be bug 584168 which is approximately the dawn of time.  Whether MSVC miscompilation happens at every instant between then and now is, of course, unknown.

Updated

4 years ago
tracking-firefox25: --- → +
tracking-firefox26: --- → +
Keywords: checkin-needed
Comment on attachment 818332 [details] [diff] [review]
Patch for Beta

Well want to land this on beta at the same time as m-c, given this is our second to last today
Attachment #818332 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

4 years ago
Attachment #818322 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
Attachment #818354 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b0f3b4a99461
https://hg.mozilla.org/releases/mozilla-beta/rev/d5d12f7343d6
https://hg.mozilla.org/releases/mozilla-esr24/rev/8f61daab1964
status-b2g-v1.1hd: --- → unaffected
status-b2g-v1.2: --- → fixed
status-firefox25: affected → fixed
status-firefox26: affected → fixed
status-firefox27: affected → fixed
status-firefox-esr24: affected → fixed
Keywords: checkin-needed
Whiteboard: [adv-main25+][adv-esr24-1+]
landed on central https://hg.mozilla.org/mozilla-central/rev/f3f9a19a57b9
Flags: in-testsuite?
Target Milestone: --- → mozilla27

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Duplicate of this bug: 889937
sunfish, the issue here is one it seems worth making you aware of, given bug 910814 comment 4 and bug 910814 comment 9.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 928028
Confirmed crash in FF24esr branch, 2013-10-15. 
Verified fixed in FF25 release.
status-firefox25: fixed → verified
Verified fixed in FF24.1.1esr 2013-11-20.
Verified fixed in FF26, FF27 2013-11-21.
status-firefox26: fixed → verified
status-firefox27: fixed → verified
status-firefox-esr24: fixed → verified
Group: core-security
You need to log in before you can comment on or make changes to this bug.