809674 - Crash with TimeEvent.view

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Jesse Ruderman]

Attachment: testcase

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

Comment 1

[Jesse Ruderman]

Same with PopupBlockedEvents / initPopupBlockedEvent, I think.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

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

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 5

[Scoobidiver (away)]

On Windows: bp-c991ccc7-dda8-4df5-8d24-280e72121108.

Comment 6

[Bobby Holley (:bholley)]

Investigating with debugger watchpoints. We appear to be trashing memory here. Marking s-s.

Comment 7

[Bobby Holley (:bholley)]

Attachment: culprit stack (trashing memory)

Attaching the stack where we write JSVAL_VOID into the JSContext, overwriting cx->runtime.

Comment 8

[Bobby Holley (:bholley)]

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]]

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)]

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)]

Are we using sec-high for mitigated remote code execution?

Comment 12

[Bobby Holley (:bholley)]

Attachment: 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)]

Attachment: Crashtest. v1 r=me

Comment 14

[Andrew McCreight [:mccr8]]

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) (inactive)]

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][[e-mail]]]

(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)]

(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)]

Attachment: 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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb591d32bd6

Comment 20

[Bobby Holley (:bholley)]

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)]

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 - ex-MoCo]]

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]

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)]

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)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9716e35a4c3b

Comment 26

[Ed Morley [:emorley]]

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]]

Marionette was green after backout.

Comment 28

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-esr10/rev/46a00740eb8a

Comment 29

[Alex Keybl [:akeybl]]

bholley - do you need help looking into the Marionette issues on 18/19?

Comment 30

[Bobby Holley (:bholley)]

(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)]

Flagging in-testsuite to remember to land the test.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bbe72fef7e0f

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cf4eac9ed40f

Comment 35

[u279076]

Kamil, can you please confirm this is fixed? Thanks.

Comment 36

[Kamil Jozwiak [:kjozwiak]]

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

[u279076]

Thank you, Kamil. Marking this bug verified.

Comment 38

[Bobby Holley (:bholley)]

Pushed the tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc947f95662

Comment 39

[Ryan VanderMeulen [:RyanVM]]

(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 41

[Masatoshi Kimura [:emk]]

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)]

(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]]

Thanks for the information. I attached a patch in bug 875155.