Closed
Bug 809674
Opened 12 years ago
Closed 12 years ago
Crash with TimeEvent.view
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
People
(Reporter: jruderman, Assigned: bholley)
References
Details
(Keywords: crash, sec-critical, testcase, Whiteboard: [adv-track-main17+][adv-track-esr17+])
Crash Data
Attachments
(4 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
mrbkap
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
lsblakk
:
approval-mozilla-release-
akeybl
:
approval-mozilla-esr10+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
This is similar to bug 805749, but with TimeEvents instead of UIEvents.
Reporter | ||
Comment 1•12 years ago
|
||
Same with PopupBlockedEvents / initPopupBlockedEvent, I think.
Comment 2•12 years ago
|
||
Fun.
This all should be handled in XPConnect layer, but I guess I'll just handle them manually
in events code.
Assignee: nobody → bugs
Comment 3•12 years ago
|
||
And I didn't know about TimeEvent. It should be implemented using event code generator.
Comment 4•12 years ago
|
||
...or maybe I did, but forgot. Apparently I reviewed the patch 2 years ago :)
Comment 5•12 years ago
|
||
On Windows: bp-c991ccc7-dda8-4df5-8d24-280e72121108.
Crash Signature: [@ JS_EndRequest ]
[@ JS_ClearPendingException] → [@ JS_EndRequest ]
[@ JS_EndRequest(JSContext*)]
[@ JS_ClearPendingException]
OS: Mac OS X → All
Hardware: x86_64 → All
Updated•12 years ago
|
Assignee: bugs → bobbyholley+bmo
Assignee | ||
Comment 6•12 years ago
|
||
Investigating with debugger watchpoints. We appear to be trashing memory here. Marking s-s.
Group: core-security
Assignee | ||
Comment 7•12 years ago
|
||
Attaching the stack where we write JSVAL_VOID into the JSContext, overwriting cx->runtime.
Assignee | ||
Comment 8•12 years ago
|
||
We appear to be crashing while getting 'onunload' from the wrappedJS, passing a jsval.
I'm pretty sure this is the XPCWrappedJS variant of bug 655878. I'll see if I can write up a fix tomorrow.
Comment 9•12 years ago
|
||
Is this going to allow random memory corruption, or are we just going to always crash in a fairly safe fashion?
Assignee | ||
Comment 10•12 years ago
|
||
OK, so I think this is bug 778189. Basically, XPCWrappedJS doesn't know about the [implicit_jscontext] and [optional_arg], and thus doesn't know that it needs to rearrange the order of the params.
So for the attribute getter, a pointer to the jscontext gets passed as param 0, but the getter thinks it's an outparam (a jsval*), and stashes JSVAL_VOID in it. This clobbers the first few bytes of the jscontext, which ends up being the runtime. We crash shortly thereafter.
Clobbering cx->runtime probably isn't a security issue. But if there were some way to avoid making that part crash, or create the same effect using optional_argc, a clever script could confuse in-params and out-params, possibly allowing it to write to arbitrary locations.
sec-critical, mitigated by the fact that attacker vectors are limited to whatever JS-implementable IDL uses [implicit_jscontext] or [optional_argc]. It may be that this is not exploitable in the status quo, but proving that definitively would probably require more work than just fixing this.
Assignee | ||
Comment 11•12 years ago
|
||
Are we using sec-high for mitigated remote code execution?
Assignee | ||
Comment 12•12 years ago
|
||
I think this is the path of least resistance here.
Attachment #680505 -
Flags: review?(mrbkap)
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #680506 -
Flags: review+
Comment 14•12 years ago
|
||
My reasoning was just "slightly worse than sec-crit, maybe, and anyways we treat sec-high identically". I guess it would be more accurate to say "sec-crit, but maybe not that bad, but let's be conservative".
Keywords: sec-high → sec-critical
Comment 15•12 years ago
|
||
Comment on attachment 680505 [details] [diff] [review]
Fail at call time when invoking an XPCWrappedJS method with [implicit_jscontext] or [optional_argc]. v1
Review of attachment 680505 [details] [diff] [review]:
-----------------------------------------------------------------
Do we have a bug on making whatever JS is implementing interfaces with [implicit_jscontext] or [optional_argc] not do that? Also, should we be marking such interfaces as builtinclass?
::: js/xpconnect/src/XPCWrappedJSClass.cpp
@@ +1147,5 @@
> + // [implicit_jscontext] and [optional_argc] have a different calling
> + // convention, which we don't support for JS-implemented components.
> + if (info->WantsOptArgc() || info->WantsContext()) {
> + JS_ReportError(cx, "IDL methods marked with [implicit_jscontext] "
> + "or [optional_argc] may not be implemented in JS");
Where does this error get reported to? The caller here is going to be C++, unless we're a double-wrapped JS object (in which case this does the Right Thing). I'd also stick an NS_WARNING in here for good measure.
Attachment #680505 -
Flags: review?(mrbkap) → review+
Comment 16•12 years ago
|
||
(In reply to Blake Kaplan (:mrbkap) from comment #15)
> Do we have a bug on making whatever JS is implementing interfaces with
> [implicit_jscontext] or [optional_argc] not do that? Also, should we be
> marking such interfaces as builtinclass?
I think we should. Unfortunately I haven't figured out how to make nsIDOMWindow builtinclass
without breaking one rather important test.
I guess that test should be just rewritting from scratch
Assignee | ||
Comment 17•12 years ago
|
||
(In reply to Blake Kaplan (:mrbkap) from comment #15)
> Do we have a bug on making whatever JS is implementing interfaces with
> [implicit_jscontext] or [optional_argc] not do that?
Well, the issue is things like nsIDOMWindow aren't builtinclass, so content can just pass {}.
> Where does this error get reported to? The caller here is going to be C++,
> unless we're a double-wrapped JS object (in which case this does the Right
> Thing). I'd also stick an NS_WARNING in here for good measure.
Ok, sounds good.
Assignee | ||
Comment 18•12 years ago
|
||
Turned this into a mochitest that checks to make sure we throw a helpful error
(depends on bug 810743).
Attachment #680506 -
Attachment is obsolete: true
Attachment #680858 -
Flags: review+
Assignee | ||
Comment 19•12 years ago
|
||
Assignee | ||
Comment 20•12 years ago
|
||
Comment on attachment 680505 [details] [diff] [review]
Fail at call time when invoking an XPCWrappedJS method with [implicit_jscontext] or [optional_argc]. v1
Doh, I probably should have flagged sec-approval _before_ pushing to inbound. Water under the bridge at this point though.
[Security approval request comment]
How easily can the security issue be deduced from the patch?
The patch is very clearly forbidding XPCWrappedJS from implementing [implicit_jscontext] and [optional_argc] interfaces. Recognizing how this is a security issue involves a lot of knowledge about XPConnect calling conventions, and even from there exploiting it is questionably possible (see comment 10). A proper exploit would be sec-critical though.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not anymore than the patch itself.
Which older supported branches are affected by this flaw?
All.
If not all supported branches, which bug introduced the flaw?
N/A.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Backports should be identical.
How likely is this patch to cause regressions; how much testing does it need?
Extremely unlikely to cause regressions (this patch only takes effect in situations where we would otherwise crash hard).
Attachment #680505 -
Flags: sec-approval?
Assignee | ||
Updated•12 years ago
|
status-firefox-esr10:
--- → affected
status-firefox17:
--- → affected
status-firefox18:
--- → affected
status-firefox19:
--- → affected
tracking-firefox-esr10:
--- → ?
tracking-firefox17:
--- → ?
tracking-firefox18:
--- → ?
tracking-firefox19:
--- → ?
Assignee | ||
Comment 21•12 years ago
|
||
Comment on attachment 680505 [details] [diff] [review]
Fail at call time when invoking an XPCWrappedJS method with [implicit_jscontext] or [optional_argc]. v1
Regression caused by (bug #): Forever.
User impact if declined: Potential sec-critical.
Testing completed (on m-c, etc.): Just pushed to inbound.
Risk to taking this patch (and alternatives if risky): Very low risk. No alternatives.
Attachment #680505 -
Flags: approval-mozilla-release?
Attachment #680505 -
Flags: approval-mozilla-beta?
Attachment #680505 -
Flags: approval-mozilla-aurora?
Comment 22•12 years ago
|
||
Comment on attachment 680505 [details] [diff] [review]
Fail at call time when invoking an XPCWrappedJS method with [implicit_jscontext] or [optional_argc]. v1
Due to accidental checkin, this is exposed so I'm giving a sec-approval+ here.
Attachment #680505 -
Flags: sec-approval? → sec-approval+
Comment 23•12 years ago
|
||
Comment on attachment 680505 [details] [diff] [review]
Fail at call time when invoking an XPCWrappedJS method with [implicit_jscontext] or [optional_argc]. v1
Won't respin 16 for this, approving for branches. Please land to beta asap for final build.
Attachment #680505 -
Flags: approval-mozilla-release?
Attachment #680505 -
Flags: approval-mozilla-release-
Attachment #680505 -
Flags: approval-mozilla-beta?
Attachment #680505 -
Flags: approval-mozilla-beta+
Attachment #680505 -
Flags: approval-mozilla-aurora?
Attachment #680505 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 24•12 years ago
|
||
Comment on attachment 680505 [details] [diff] [review]
Fail at call time when invoking an XPCWrappedJS method with [implicit_jscontext] or [optional_argc]. v1
Er, I meant to flag for esr10, not release. Sorry.
Attachment #680505 -
Flags: approval-mozilla-esr10?
Assignee | ||
Comment 25•12 years ago
|
||
Comment 26•12 years ago
|
||
Backed out on suspicion of breaking marionette-webapi:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&fromchange=26c2e6c1c22e&tochange=bcb591d32bd6&jobname=webapi
The only other suspect is bug 810743, but by my (severely untrained) eyes this looked like the stronger candidate. Will reland if tip doesn't green up :-)
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d56d537a1843
Comment 27•12 years ago
|
||
Marionette was green after backout.
Updated•12 years ago
|
Updated•12 years ago
|
Attachment #680505 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Assignee | ||
Comment 28•12 years ago
|
||
Updated•12 years ago
|
Whiteboard: [adv-track-main17+][adv-track-esr17+]
Comment 29•12 years ago
|
||
bholley - do you need help looking into the Marionette issues on 18/19?
Assignee | ||
Comment 30•12 years ago
|
||
(In reply to Alex Keybl [:akeybl] from comment #29)
> bholley - do you need help looking into the Marionette issues on 18/19?
Now that bug 811414 has landed on trunk, this should be green.
Relanded to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbe72fef7e0f
Assignee | ||
Comment 31•12 years ago
|
||
Flagging in-testsuite to remember to land the test.
Flags: in-testsuite?
Comment 32•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 33•12 years ago
|
||
Comment 35•12 years ago
|
||
Kamil, can you please confirm this is fixed? Thanks.
Comment 36•12 years ago
|
||
The following issue was tested on:
- Windows 8 x86
Used the following builds to ensure that the issue is reproducible:
Firefox 19.0a1 (Reproduced): http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-11-15-03-07-05-mozilla-central/
- Ran through the attached test case and received the following crash: [@ JS_EndRequest(JSContext*) ]
- https://crash-stats.mozilla.com/report/index/bp-0329661a-58ca-49b9-8c49-bc0c12121129
- https://crash-stats.mozilla.com/report/index/bp-77dc4373-41d7-4e6f-910d-ca5ec2121129
Used the following builds to ensure that the issue has been resolved:
Firefox 17.0 (No Issue): http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/17.0/win32/en-US/
- Used "about:" to ensure the correct build was being used
- Ran through the attached test case and received NO crashes as in 19.0a1 (.html test case loaded without any issues)
Firefox 17.0esr (No Issue): http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/17.0esr/win32/en-US/
- Used "about:" to ensure the correct build was being used
- Ran through the attached test case and received NO crashes as in 19.0a1 (.html test case loaded without any issues)
Firefox 18.0a2 (No Issue): http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-11-19-04-20-13-mozilla-aurora/
- Used "about:" to ensure the correct build was being used
- Ran through the attached test case and received NO crashes as in 19.0a1 (.html test case loaded without any issues)
Firefox 19.0a1 (No Issue): http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-11-16-03-07-25-mozilla-central/
- Used "about:" to ensure the correct build was being used
- Ran through the attached test case and received NO crashes as in 19.0a1 (.html test case loaded without any issues)
Comment 37•12 years ago
|
||
Thank you, Kamil. Marking this bug verified.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Assignee | ||
Comment 38•12 years ago
|
||
Pushed the tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc947f95662
Comment 39•12 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #38)
> Pushed the tests:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc947f95662
https://hg.mozilla.org/mozilla-central/rev/ecc947f95662
Flags: in-testsuite? → in-testsuite+
Updated•12 years ago
|
Group: core-security
Comment 41•12 years ago
|
||
I'm trying to convert TimeEvent to WebIDL bindings. This test no longer throws the exception after that.
https://tbpl.mozilla.org/?tree=Try&rev=09188f50a672
Is this expected?
Assignee | ||
Comment 42•12 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #41)
> I'm trying to convert TimeEvent to WebIDL bindings. This test no longer
> throws the exception after that.
> https://tbpl.mozilla.org/?tree=Try&rev=09188f50a672
> Is this expected?
Yeah, that's expected. This test is basically making sure that when we create a JS-implemented component implementing an interface with methods/attributes marked with [implicit_jscontext] or [optional_argc], calling those methods throws (rather than crashing, because we have no handling for such things in XPCWrappedJS).
Now that TimeEvent is no longer on XPConnect bindings, this is likely to not work. A good replacement test would just be to add something to the existing test interfaces in idl/xpctest_* and add something to the corresponding xpcshell tests to make sure that we throw appropriately for the JS-implemented case.
Comment 43•12 years ago
|
||
Thanks for the information. I attached a patch in bug 875155.
You need to log in
before you can comment on or make changes to this bug.
Description
•