1054359 - (CVE-2014-1562) LCallDOMNative incorrectly assumes |this| is always an object

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Testcase

I was looking at crash URLs for JIT crashes on crash-stats, and was able to reproduce a crash in JIT code on manpower.com

The problem is that CodeGenerator::visitCallDOMNative does:

    Register obj = masm.extractObject(Address(StackPointer, 0), argObj);

But |this| is not guaranteed to be an object. So if you do something like:

    var method = document.body.mozMatchesSelector;
    method.call(v, "selector");

And v is an integer, we'll treat it as an object pointer and this allows an attacker to do all kinds of scary things.

The attached testcase crashes Nightly for me on OS X, 32-bit (you may have to turn off off-thread compilation first).

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Having isDOMClass/getKnownClass/getTypedArrayType return |true| even if the value can be primitive is a footgun. I think we should consider changing that, but here's the safe and simple fix: only optimize DOM method calls if |this| is guaranteed to be an object.

Comment 2

[Lawrence Mandel [:lmandel] (use needinfo)]

We're running out of time for beta 32. I'd like to see whether we can get this into beta8, which goes to build on Mon, Aug 18.

Comment 3

[Eric Faust [:efaust]]

Comment on attachment 8473754 [details] [diff] [review]
Patch

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

This is a phenomenal catch. r=me for now, for uplift. Can you file a followup to deal with the footguns you mention?

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8473754 [details] [diff] [review]
Patch

(This is causing topcrashes so it'd be great if we could get this on beta.)

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not trivial but also not hard for somebody who knows what he/she'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?
All.

> 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; it just adds an extra check to guard an optimization.

Comment 5

[Eric Faust [:efaust]]

Comment on attachment 8473754 [details] [diff] [review]
Patch

No, the one with two little lines. (meant r+ initially, obviously)

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8473754 [details] [diff] [review]
Patch

sec-approval+ for trunk. Assuming it goes in clean, please have beta and aurora patches made and nominated so we can approve them ASAP.

Comment 7

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cdb000f3d8

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8473754 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Older Ion bug
[User impact if declined]: Topcrash, security bugs
[Describe test coverage new/current, TBPL]: Code should be tested on TBPL, very simple patch.
[Risks and why]: Very low; small/simple patch.
[String/UUID change made/needed]: None.

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8473754 [details] [diff] [review]
Patch

I'm approving for beta before this hits m-c as I want to take this in beta8 today. I don't expect a backout. If necessary, we'll deal with a build2.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0b59289683f
https://hg.mozilla.org/releases/mozilla-beta/rev/f5bfa8f3434c

Looks like we need esr31 and esr24 (if affected) noms on this as well.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8473754 [details] [diff] [review]
Patch

[Approval Request Comment]
User impact if declined: Security bugs, topcrashes.
Fix Landed on Version: 34, backported to 32/33.
Risk to taking this patch (and alternatives if risky): None-Low.
String or UUID changes made by this patch: None.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Patch for ESR24

[Approval Request Comment]
User impact if declined: Security bugs, topcrashes.
Fix Landed on Version: 34, backported to 32/33
Risk to taking this patch (and alternatives if risky): None-Low.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f6cdb000f3d8
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f5bfa8f3434c

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/bb8c53609400
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/a2712ce8bf92
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/709347e7fb78
https://hg.mozilla.org/releases/mozilla-esr24/rev/a2e3f3644944

Comment 15

[Ryan VanderMeulen [:RyanVM]]

b2g30 needed the esr24 version of the patch:
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/d8ec0a9650fe

Comment 16

[Boris Zbarsky [:bzbarsky]]

This is akin to bug 951517, which it looks like we didn't fix as well as we should have.  :(

We definitely need a followup to fix isDOMClass here.

Comment 17

[Jan de Mooij [:jandem]]

(In reply to Boriz Zbarsky [:bz] from comment #16)
> We definitely need a followup to fix isDOMClass here.

Filed bug 1055768.

Comment 18

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on Fx34, 2014-08-11.
Verified fixed in Fx32, Fx33 and Fx34, 2014-08-22.
Verified fixed in Fx24.8.0esr, release.
Verified fixed in Fx31.1.0esr, release.