Closed
Bug 1054359
(CVE-2014-1562)
Opened 10 years ago
Closed 10 years ago
LCallDOMNative incorrectly assumes |this| is always an object
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
VERIFIED
FIXED
mozilla34
People
(Reporter: jandem, Assigned: jandem)
References
Details
(Keywords: sec-critical, topcrash, Whiteboard: [adv-main32+][adv-esr31.1+][adv-esr24.8+])
Attachments
(3 files)
294 bytes,
text/html
|
Details | |
960 bytes,
patch
|
efaust
:
review+
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
abillings
:
approval-mozilla-esr31+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
1.03 KB,
patch
|
jandem
:
review+
abillings
:
approval-mozilla-esr24+
|
Details | Diff | Splinter Review |
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).
Assignee | ||
Comment 1•10 years ago
|
||
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 | ||
Updated•10 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:
--- → ?
Comment 2•10 years ago
|
||
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•10 years ago
|
||
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-
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
Comment on attachment 8473754 [details] [diff] [review] Patch No, the one with two little lines. (meant r+ initially, obviously)
Attachment #8473754 -
Flags: review- → review+
Updated•10 years ago
|
Comment 6•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cdb000f3d8
Assignee | ||
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
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 10•10 years ago
|
||
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.
status-b2g-v2.1:
--- → fixed
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 11•10 years ago
|
||
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?
Assignee | ||
Comment 12•10 years ago
|
||
[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?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(jdemooij)
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f6cdb000f3d8 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f5bfa8f3434c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-b2g-v1.3:
--- → affected
status-b2g-v1.3T:
--- → affected
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•10 years ago
|
Attachment #8473754 -
Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Updated•10 years ago
|
Attachment #8474692 -
Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Comment 14•10 years ago
|
||
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•10 years ago
|
||
b2g30 needed the esr24 version of the patch: https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/d8ec0a9650fe
Comment 16•10 years ago
|
||
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.
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Boriz Zbarsky [:bz] from comment #16) > We definitely need a followup to fix isDOMClass here. Filed bug 1055768.
Updated•10 years ago
|
Whiteboard: [adv-main32+][adv-esr31.1+][adv-esr24.8+]
Comment 18•10 years ago
|
||
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: RESOLVED → VERIFIED
Updated•10 years ago
|
Alias: CVE-2014-1562
Updated•10 years ago
|
Group: javascript-core-security
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•