Last Comment Bug 809674 - Crash with TimeEvent.view
: Crash with TimeEvent.view
Status: VERIFIED FIXED
[adv-track-main17+][adv-track-esr17+]
: crash, sec-critical, testcase
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla19
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
: 778189 805879 (view as bug list)
Depends on: 817567 811414
Blocks: 326633
  Show dependency treegraph
 
Reported: 2012-11-07 16:28 PST by Jesse Ruderman
Modified: 2013-05-22 17:26 PDT (History)
13 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
+
verified
17+
verified


Attachments
testcase (220 bytes, text/html)
2012-11-07 16:28 PST, Jesse Ruderman
no flags Details
culprit stack (trashing memory) (13.53 KB, text/plain)
2012-11-08 15:09 PST, Bobby Holley (:bholley) (busy with Stylo)
no flags Details
Fail at call time when invoking an XPCWrappedJS method with [implicit_jscontext] or [optional_argc]. v1 (2.43 KB, patch)
2012-11-11 19:14 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑release-
akeybl: approval‑mozilla‑esr10+
abillings: sec‑approval+
Details | Diff | Splinter Review
Crashtest. v1 r=me (1.13 KB, patch)
2012-11-11 19:15 PST, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review
Test. v2 r=me (1.84 KB, patch)
2012-11-12 16:11 PST, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-11-07 16:28:20 PST
Created attachment 679442 [details]
testcase

This is similar to bug 805749, but with TimeEvents instead of UIEvents.
Comment 1 Jesse Ruderman 2012-11-07 16:29:01 PST
Same with PopupBlockedEvents / initPopupBlockedEvent, I think.
Comment 2 Olli Pettay [:smaug] 2012-11-07 17:32:12 PST
Fun.
This all should be handled in XPConnect layer, but I guess I'll just handle them manually
in events code.
Comment 3 Olli Pettay [:smaug] 2012-11-07 17:35:47 PST
And I didn't know about TimeEvent. It should be implemented using event code generator.
Comment 4 Olli Pettay [:smaug] 2012-11-07 17:37:09 PST
...or maybe I did, but forgot. Apparently I reviewed the patch 2 years ago :)
Comment 5 Scoobidiver (away) 2012-11-08 01:32:05 PST
On Windows: bp-c991ccc7-dda8-4df5-8d24-280e72121108.
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-11-08 15:06:17 PST
Investigating with debugger watchpoints. We appear to be trashing memory here. Marking s-s.
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2012-11-08 15:09:40 PST
Created attachment 679857 [details]
culprit stack (trashing memory)

Attaching the stack where we write JSVAL_VOID into the JSContext, overwriting cx->runtime.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-11-08 15:43:17 PST
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 Andrew McCreight [:mccr8] 2012-11-08 17:08:07 PST
Is this going to allow random memory corruption, or are we just going to always crash in a fairly safe fashion?
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2012-11-11 18:08:30 PST
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.
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2012-11-11 18:52:06 PST
Are we using sec-high for mitigated remote code execution?
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-11-11 19:14:40 PST
Created attachment 680505 [details] [diff] [review]
Fail at call time when invoking an XPCWrappedJS method with [implicit_jscontext] or [optional_argc]. v1

I think this is the path of least resistance here.
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2012-11-11 19:15:01 PST
Created attachment 680506 [details] [diff] [review]
Crashtest. v1 r=me
Comment 14 Andrew McCreight [:mccr8] 2012-11-11 19:50:19 PST
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".
Comment 15 Blake Kaplan (:mrbkap) 2012-11-12 15:23:16 PST
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.
Comment 16 Olli Pettay [:smaug] 2012-11-12 15:25:50 PST
(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
Comment 17 Bobby Holley (:bholley) (busy with Stylo) 2012-11-12 16:02:21 PST
(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.
Comment 18 Bobby Holley (:bholley) (busy with Stylo) 2012-11-12 16:11:11 PST
Created attachment 680858 [details] [diff] [review]
Test. v2 r=me

Turned this into a mochitest that checks to make sure we throw a helpful error
(depends on bug 810743).
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2012-11-12 16:18:21 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb591d32bd6
Comment 20 Bobby Holley (:bholley) (busy with Stylo) 2012-11-12 16:24:51 PST
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).
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-11-12 16:27:09 PST
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.
Comment 22 Al Billings [:abillings] 2012-11-12 16:27:51 PST
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.
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2012-11-12 16:39:45 PST
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.
Comment 24 Bobby Holley (:bholley) (busy with Stylo) 2012-11-12 16:52:13 PST
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.
Comment 25 Bobby Holley (:bholley) (busy with Stylo) 2012-11-12 16:59:27 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/9716e35a4c3b
Comment 26 Ed Morley [:emorley] 2012-11-13 05:51:31 PST
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 Ed Morley [:emorley] 2012-11-13 06:40:45 PST
Marionette was green after backout.
Comment 28 Bobby Holley (:bholley) (busy with Stylo) 2012-11-13 15:15:58 PST
https://hg.mozilla.org/releases/mozilla-esr10/rev/46a00740eb8a
Comment 29 Alex Keybl [:akeybl] 2012-11-15 08:34:35 PST
bholley - do you need help looking into the Marionette issues on 18/19?
Comment 30 Bobby Holley (:bholley) (busy with Stylo) 2012-11-15 10:17:59 PST
(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
Comment 31 Bobby Holley (:bholley) (busy with Stylo) 2012-11-15 10:57:22 PST
Flagging in-testsuite to remember to land the test.
Comment 32 Ryan VanderMeulen [:RyanVM] 2012-11-15 17:46:35 PST
https://hg.mozilla.org/mozilla-central/rev/bbe72fef7e0f
Comment 33 Ryan VanderMeulen [:RyanVM] 2012-11-18 14:50:44 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/cf4eac9ed40f
Comment 34 Andrew McCreight [:mccr8] 2012-11-28 15:22:15 PST
*** Bug 805879 has been marked as a duplicate of this bug. ***
Comment 35 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-29 12:02:54 PST
Kamil, can you please confirm this is fixed? Thanks.
Comment 36 Kamil Jozwiak [:kjozwiak] 2012-11-29 18:14:04 PST
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 11:31:37 PST
Thank you, Kamil. Marking this bug verified.
Comment 38 Bobby Holley (:bholley) (busy with Stylo) 2013-03-11 14:48:56 PDT
Pushed the tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc947f95662
Comment 39 Ryan VanderMeulen [:RyanVM] 2013-03-12 13:01:24 PDT
(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
Comment 40 Bobby Holley (:bholley) (busy with Stylo) 2013-03-13 22:05:17 PDT
*** Bug 778189 has been marked as a duplicate of this bug. ***
Comment 41 Masatoshi Kimura [:emk] 2013-05-21 15:44:07 PDT
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?
Comment 42 Bobby Holley (:bholley) (busy with Stylo) 2013-05-21 17:08:01 PDT
(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 Masatoshi Kimura [:emk] 2013-05-22 17:26:29 PDT
Thanks for the information. I attached a patch in bug 875155.

Note You need to log in before you can comment on or make changes to this bug.