Closed Bug 1054359 (CVE-2014-1562) Opened 5 years ago Closed 5 years ago

LCallDOMNative incorrectly assumes |this| is always an object

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla34
Tracking Status
firefox31 --- wontfix
firefox32 + verified
firefox33 + verified
firefox34 + verified
firefox-esr24 32+ verified
firefox-esr31 32+ verified
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Keywords: sec-critical, topcrash, Whiteboard: [adv-main32+][adv-esr31.1+][adv-esr24.8+])

Attachments

(3 files)

Attached file 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).
Keywords: topcrash
Attached patch PatchSplinter 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
Status: NEW → ASSIGNED
Attachment #8473754 - Flags: review?(efaustbmo)
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 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?
Attachment #8473754 - Flags: review?(efaustbmo) → review-
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.
Attachment #8473754 - Flags: sec-approval?
Comment on attachment 8473754 [details] [diff] [review]
Patch

No, the one with two little lines. (meant r+ initially, obviously)
Attachment #8473754 - Flags: review- → review+
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.
Attachment #8473754 - Flags: sec-approval? → sec-approval+
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.
Attachment #8473754 - Flags: approval-mozilla-beta?
Attachment #8473754 - Flags: approval-mozilla-aurora?
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.
Attachment #8473754 - Flags: approval-mozilla-beta?
Attachment #8473754 - Flags: approval-mozilla-beta+
Attachment #8473754 - Flags: approval-mozilla-aurora?
Attachment #8473754 - Flags: approval-mozilla-aurora+
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.
Attachment #8473754 - Flags: approval-mozilla-esr31?
Attached patch Patch for ESR24Splinter 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.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8474692 - Flags: review+
Attachment #8474692 - Flags: approval-mozilla-esr24?
Flags: needinfo?(jdemooij)
Attachment #8473754 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Attachment #8474692 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
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.
(In reply to Boriz Zbarsky [:bz] from comment #16)
> We definitely need a followup to fix isDOMClass here.

Filed bug 1055768.
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.
Depends on: 1059456
Alias: CVE-2014-1562
Group: javascript-core-security
Depends on: 1062857
No longer depends on: 1059456
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.