Closed Bug 809674 Opened 7 years ago Closed 7 years ago

Crash with TimeEvent.view

Categories

(Core :: DOM: Events, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox17 + verified
firefox18 + verified
firefox19 + verified
firefox-esr10 17+ verified

People

(Reporter: jruderman, Assigned: bholley)

References

(Blocks 1 open bug)

Details

(Keywords: crash, sec-critical, testcase, Whiteboard: [adv-track-main17+][adv-track-esr17+])

Crash Data

Attachments

(4 files, 1 obsolete file)

Attached file testcase
This is similar to bug 805749, but with TimeEvents instead of UIEvents.
Same with PopupBlockedEvents / initPopupBlockedEvent, I think.
Fun.
This all should be handled in XPConnect layer, but I guess I'll just handle them manually
in events code.
Assignee: nobody → bugs
And I didn't know about TimeEvent. It should be implemented using event code generator.
...or maybe I did, but forgot. Apparently I reviewed the patch 2 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
Assignee: bugs → bobbyholley+bmo
Investigating with debugger watchpoints. We appear to be trashing memory here. Marking s-s.
Group: core-security
Attaching the stack where we write JSVAL_VOID into the JSContext, overwriting cx->runtime.
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.
Is this going to allow random memory corruption, or are we just going to always crash in a fairly safe fashion?
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.
Keywords: sec-high
Are we using sec-high for mitigated remote code execution?
I think this is the path of least resistance here.
Attachment #680505 - Flags: review?(mrbkap)
Attached patch Crashtest. v1 r=me (obsolete) — Splinter Review
Attachment #680506 - Flags: review+
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-highsec-critical
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+
(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
(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.
Attached patch Test. v2 r=meSplinter Review
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+
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?
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 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 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+
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?
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
Marionette was green after backout.
Attachment #680505 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Depends on: 811414
Whiteboard: [adv-track-main17+][adv-track-esr17+]
bholley - do you need help looking into the Marionette issues on 18/19?
(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
Flagging in-testsuite to remember to land the test.
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/bbe72fef7e0f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Duplicate of this bug: 805879
Kamil, can you please confirm this is fixed? Thanks.
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)
Depends on: 817567
Thank you, Kamil. Marking this bug verified.
(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+
Duplicate of this bug: 778189
Group: core-security
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?
(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.
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.