Bug 1054359 (CVE-2014-1562)

LCallDOMNative incorrectly assumes |this| is always an object

VERIFIED FIXED in Firefox 32, Firefox OS v1.3



JavaScript Engine: JIT
3 years ago
2 years ago


(Reporter: jandem, Assigned: jandem)


({sec-critical, topcrash})

sec-critical, topcrash

Firefox Tracking Flags

(firefox31 wontfix, firefox32+ verified, firefox33+ verified, firefox34+ verified, firefox-esr2432+ verified, firefox-esr3132+ verified, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)


(Whiteboard: [adv-main32+][adv-esr31.1+][adv-esr24.8+])


(3 attachments)



3 years ago
Created attachment 8473747 [details]

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).


3 years ago
Keywords: topcrash

Comment 1

3 years ago
Created attachment 8473754 [details] [diff] [review]

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.
Assignee: nobody → jdemooij
Attachment #8473754 - Flags: review?(efaustbmo)


3 years ago
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox-esr31: --- → affected
tracking-firefox32: --- → ?
tracking-firefox33: --- → ?
tracking-firefox34: --- → ?
tracking-firefox-esr31: --- → ?
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.
tracking-firefox32: ? → +
tracking-firefox33: ? → +
tracking-firefox34: ? → +

Comment 3

3 years ago
Comment on attachment 8473754 [details] [diff] [review]

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?
Attachment #8473754 - Flags: review?(efaustbmo) → review-

Comment 4

3 years ago
Comment on attachment 8473754 [details] [diff] [review]

(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?

> Which older supported branches are affected by this flaw?

> 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.
Attachment #8473754 - Flags: sec-approval?

Comment 5

3 years ago
Comment on attachment 8473754 [details] [diff] [review]

No, the one with two little lines. (meant r+ initially, obviously)
Attachment #8473754 - Flags: review- → review+
status-firefox31: affected → wontfix
status-firefox-esr24: --- → ?
tracking-firefox-esr31: ? → 32+
Comment on attachment 8473754 [details] [diff] [review]

sec-approval+ for trunk. Assuming it goes in clean, please have beta and aurora patches made and nominated so we can approve them ASAP.
Attachment #8473754 - Flags: sec-approval? → sec-approval+

Comment 7

3 years ago

Comment 8

3 years ago
Comment on attachment 8473754 [details] [diff] [review]

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.
Attachment #8473754 - Flags: approval-mozilla-beta?
Attachment #8473754 - Flags: approval-mozilla-aurora?
Comment on attachment 8473754 [details] [diff] [review]

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.
Attachment #8473754 - Flags: approval-mozilla-beta?
Attachment #8473754 - Flags: approval-mozilla-beta+
Attachment #8473754 - Flags: approval-mozilla-aurora?
Attachment #8473754 - Flags: approval-mozilla-aurora+

Looks like we need esr31 and esr24 (if affected) noms on this as well.
status-b2g-v2.1: --- → fixed
status-firefox32: affected → fixed
status-firefox33: affected → fixed
status-firefox34: affected → fixed
Flags: needinfo?(jdemooij)

Comment 11

3 years ago
Comment on attachment 8473754 [details] [diff] [review]

[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.
Attachment #8473754 - Flags: approval-mozilla-esr31?

Comment 12

3 years ago
Created attachment 8474692 [details] [diff] [review]
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.
Attachment #8474692 - Flags: review+
Attachment #8474692 - Flags: approval-mozilla-esr24?


3 years ago
Flags: needinfo?(jdemooij)
Last Resolved: 3 years ago
status-b2g-v1.3: --- → affected
status-b2g-v1.3T: --- → affected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → fixed
status-firefox-esr24: ? → affected
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Attachment #8473754 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Attachment #8474692 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
status-b2g-v1.3: affected → fixed
status-b2g-v1.3T: affected → fixed
status-b2g-v1.4: affected → fixed
status-firefox-esr24: affected → fixed
status-firefox-esr31: affected → fixed
tracking-firefox-esr24: --- → ?
b2g30 needed the esr24 version of the patch:
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

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

Filed bug 1055768.
tracking-firefox-esr24: ? → 32+
Whiteboard: [adv-main32+][adv-esr31.1+][adv-esr24.8+]
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.
status-firefox32: fixed → verified
status-firefox33: fixed → verified
status-firefox34: fixed → verified
status-firefox-esr24: fixed → verified
status-firefox-esr31: fixed → verified
Depends on: 1059456
Alias: CVE-2014-1562
Group: javascript-core-security
Depends on: 1062857


3 years ago
No longer depends on: 1059456


2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.