Crash with TimeEvent.view

VERIFIED FIXED in Firefox 17

Status

()

Core
DOM: Events
--
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: bholley)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {crash, sec-critical, testcase})

Trunk
mozilla19
crash, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17+ verified, firefox18+ verified, firefox19+ verified, firefox-esr1017+ verified)

Details

(Whiteboard: [adv-track-main17+][adv-track-esr17+], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 679442 [details]
testcase

This is similar to bug 805749, but with TimeEvents instead of UIEvents.
(Reporter)

Comment 1

5 years ago
Same with PopupBlockedEvents / initPopupBlockedEvent, I think.

Comment 2

5 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

5 years ago
And I didn't know about TimeEvent. It should be implemented using event code generator.

Comment 4

5 years ago
...or maybe I did, but forgot. Apparently I reviewed the patch 2 years ago :)

Comment 5

5 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

5 years ago
Assignee: bugs → bobbyholley+bmo
Investigating with debugger watchpoints. We appear to be trashing memory here. Marking s-s.
Group: core-security
Created attachment 679857 [details]
culprit stack (trashing memory)

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?
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.
Attachment #680505 - Flags: review?(mrbkap)
Created attachment 680506 [details] [diff] [review]
Crashtest. v1 r=me
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-high → sec-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.
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).
Attachment #680506 - Attachment is obsolete: true
Attachment #680858 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb591d32bd6
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?
status-firefox-esr10: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
tracking-firefox19: --- → ?
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?
https://hg.mozilla.org/releases/mozilla-beta/rev/9716e35a4c3b
status-firefox17: affected → fixed
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.

Updated

5 years ago
tracking-firefox-esr10: ? → 17+
tracking-firefox17: ? → +
tracking-firefox18: ? → +
tracking-firefox19: ? → +

Updated

5 years ago
Attachment #680505 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Depends on: 811414
https://hg.mozilla.org/releases/mozilla-esr10/rev/46a00740eb8a
status-firefox-esr10: affected → fixed
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?
Keywords: verifyme
https://hg.mozilla.org/mozilla-central/rev/bbe72fef7e0f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox19: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
https://hg.mozilla.org/releases/mozilla-aurora/rev/cf4eac9ed40f
status-firefox18: affected → fixed
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)
(Reporter)

Updated

5 years ago
Depends on: 817567
Thank you, Kamil. Marking this bug verified.
Status: RESOLVED → VERIFIED
status-firefox-esr10: fixed → verified
status-firefox17: fixed → verified
status-firefox18: fixed → verified
status-firefox19: fixed → verified
Keywords: verifyme
Pushed the tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc947f95662
(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.