1181957 - nsTextEditorState runs script during frame destruction because DelegatedQueryInterface calls AutoEntryScript for builtinclass interfaces

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Stack:

#1  0x0000000104881a5c in mozilla::dom::AutoEntryScript::AutoEntryScript (this=0x7fff5fbf9890, aGlobalObject=0x1281658c8, aReason=0x109752134 "XPCWrappedJS QueryInterface", aIsMainThread=true, aCx=0x0) at ScriptSettings.cpp:549
#2  0x0000000103e94a2d in nsXPCWrappedJSClass::DelegatedQueryInterface (this=0x12bc26830, self=0x12ba34080, aIID=@0x109c2f5f0, aInstancePtr=0x7fff5fbf9a70) at XPCWrappedJSClass.cpp:518
#3  0x0000000103e950b2 in nsXPCWrappedJS::QueryInterface (this=0x12ba34080, aIID=@0x109c2f5f0, aInstancePtr=0x7fff5fbf9a70) at XPCWrappedJS.cpp:221
#4  0x0000000102f41332 in nsXPTCStubBase::QueryInterface (this=0x12bc30ac0, aIID=@0x109c2f5f0, aInstancePtr=0x7fff5fbf9a70) at ../../../../mozilla/xpcom/reflect/xptcall/xptcall.cpp:25
#5  0x0000000102f7ffb7 in nsQueryInterface::operator() (this=0x7fff5fbf9a88, aIID=@0x109c2f5f0, aAnswer=0x7fff5fbf9a70) at nsCOMPtr.cpp:14
#6  0x00000001047926b3 in nsCOMPtr<nsIControllerContext>::assign_from_qi (this=0x7fff5fbf9d00, aQI={mRawPtr = 0x12bc30ac0}, aIID=@0x109c2f5f0) at nsCOMPtr.h:1046
#7  0x000000010479266e in nsCOMPtr<nsIControllerContext>::nsCOMPtr (this=0x7fff5fbf9d00, aQI={mRawPtr = 0x12bc30ac0}) at nsCOMPtr.h:482
#8  0x0000000104777f6d in nsCOMPtr<nsIControllerContext>::nsCOMPtr (this=0x7fff5fbf9d00, aQI={mRawPtr = 0x12bc30ac0}) at nsCOMPtr.h:483
#9  0x0000000105da4305 in nsTextEditorState::UnbindFromFrame (this=0x12b8b3160, aFrame=0x12b850438) at nsTextEditorState.cpp:1657
#10 0x0000000105cb2ad6 in mozilla::dom::HTMLInputElement::UnbindFromFrame (this=0x12b41eca0, aFrame=0x12b850438) at HTMLInputElement.cpp:2280
#11 0x0000000105cbd5ef in non-virtual thunk to mozilla::dom::HTMLInputElement::UnbindFromFrame(nsTextControlFrame*) (this=0x12b41edc0, aFrame=0x12b850438) at HTMLInputElement.cpp:2282
#12 0x00000001070415cc in nsTextControlFrame::DestroyFrom (this=0x12b850438, aDestructRoot=0x12b845d28) at nsTextControlFrame.cpp:131

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like the basic thing is scripted controllers that we're trying to QI to nsIControllerContext so we can SetCommandContext(nullptr).

Could we do this off a script runner?

Comment 2

[(no longer active)]

Yes, I think that should be OK.

How do you reproduce this?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> How do you reproduce this?

Oh, sorry.  Apply the patch from bug 1181918, then the patches from bug 1181916 and bug 1181920 (to fix other things that would trigger the assert).  Then try to start the browser.

Comment 4

[(no longer active)]

I'll take a look.

Comment 5

[Matt Wobensmith [:mwobensmith][:matt:]]

Any update here, Ehsan?

Comment 6

[(no longer active)]

I haven't looked yet since Boris told me that it's not urgent.  Is it urgent now?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'm not aware of special urgency so far, since the script we run is not going to mess with frames.  The main problem is that this keeps the browser from starting up and hence I can't tell whether other things might be running less-safe scripts.  :(

Comment 8

[Andrew McCreight [:mccr8]]

I'll lower this to sec-moderate then. Thanks for the update.

Comment 9

[(no longer active)]

Attachment: Patch (will break tests)

I gave this a quick shot.  This patch lets us start up a bit more but we hit another case before we can show the first window.

But more importantly, this breaks tests.  I haven't looked much into why yet, but we may not be able to run this off of a script blocker in case something observable to script would happen too late if we did that.  What would be our options in that case?

Comment 10

[Boris Zbarsky [:bzbarsky]]

We could always add something where we whitelist a AutoEntryScript for purposes of this assertion, as long as we're very sure that the AutoEntryScript in this case doesn't cause use to hit anything that could in fact cause problems.

Comment 11

[Boris Zbarsky [:bzbarsky]]

The other thing we could do is to just do the QI to nsIControllerContext and the SetCommandContext(nullptr) bit off a scriptrunner, right?  That's what's really landing us in script: someone has a JS-implemented controller here.

Comment 12

[(no longer active)]

Note that there are two calls to SetCommandContext() and I actually think it's the other call that changes the semantics when I run it off of a script runner.

That being said, AFAICT there should be no JS controller that also implements nsIControllerContext.  Is there a way to know whether a controller is a JS one before we QI to it?  If yes, I suspect we could simply skip those ones.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Is there a way to know whether a controller is a JS one before we QI to it? 

Sorta kinda.  You can QI to nsIXPConnectJSObjectHolder.  If the QI succeeds, you're presumably looking at an XPCWrappedJS (which will return itself for that IID, instead of calling through to the JS object it wraps).

Comment 14

[Boris Zbarsky [:bzbarsky]]

Or better yet, as mccr8 points out, QI to nsIXPConnectWrappedJS.

Comment 15

[Hsin-Yi Tsai (she/her)[:hsinyi]]

Hi Masayuki, as you did several clean-ups and refactoring in the past months, can you help evaluate if this is valid? Thank you.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Looks like that the issue is gone because:

• nsIControllerContext is now a builtinclass so that it cannot be implemented by JS.
• nsIControllerContext is QIed from nsIController which is not builtinclass. However, that comes from controllers of <input> element or <textarea> element and both <input> element's controllers and <textarea> element's controllers are nsBaseCommandController which is implemented by C++.
• Finally, TextControlState::UnbindFromFrame() has its own kungFuDeathGrip so that nobody is gone even if it runs script.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I am still hitting asserts from bug 1181918 in a debug build with this stack:

#17 0x00007f8c9c0dadcb in mozilla::dom::AutoEntryScript::AutoEntryScript(nsIGlobalObject*, char const*, bool)
    (this=0x7ffec902ca50, aGlobalObject=0x7f8c81d06e80, aReason=0x7f8c934075a2 "XPCWrappedJS QueryInterface", aIsMainThread=true)
    at /home/bzbarsky/mozilla/dom-work/mozilla/dom/script/ScriptSettings.cpp:585
#18 0x00007f8c996ffc8c in nsXPCWrappedJS::DelegatedQueryInterface(nsID const&, void**)
    (this=0x7f8c76ed8f20, aIID=..., aInstancePtr=0x7ffec902cb40)
    at /home/bzbarsky/mozilla/dom-work/mozilla/js/xpconnect/src/XPCWrappedJSClass.cpp:332
#19 0x00007f8c9a11a05e in nsCOMPtr<nsIControllerContext>::assign_from_qi<nsIController>(nsQueryInterface<nsIController>, nsID const&)
    (this=0x7ffec902cb88, aQI=..., aIID=...) at ../../dist/include/nsCOMPtr.h:1157
#20 0x00007f8c9b5b3ae6 in mozilla::TextControlState::UnbindFromFrame(nsTextControlFrame*) (this=0x7f8c772fec80, aFrame=<optimized out>)
    at /home/bzbarsky/mozilla/dom-work/mozilla/dom/html/TextControlState.cpp:2490
#21 0x00007f8c9c7559a8 in nsTextControlFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&)
    (this=0x7f8c7bc43ab8, aDestructRoot=0x7f8c7bc67690, aPostDestroyData=...)
    at /home/bzbarsky/mozilla/dom-work/mozilla/layout/forms/nsTextControlFrame.cpp:145

In this case, in frame 20, controller is JS-implemented. The JS-implemented thing was passed to nsXULControllers::InsertControllerAt from script, with this JS stack:

0 _initCopyCutController() ["resource:///modules/UrlbarInput.jsm":1603:32]
    this = [object Object]
1 UrlbarInput(options = "[object Object]") ["resource:///modules/UrlbarInput.jsm":223:9]
    this = [object Object]
2 anonymous() ["chrome://browser/content/browser.js":331:9]
3 get() ["resource://gre/modules/XPCOMUtils.jsm":129:50]
    this = [object ChromeWindow]
4 _setURLBarPlaceholder(name = ""Google"") ["chrome://browser/content/browser.js":4341:4]
    this = [object Object]
5 initPlaceHolder() ["chrome://browser/content/browser.js":4238:11]
    this = [object Object]
6 onDOMContentLoaded("[object Event]") ["chrome://browser/content/browser.js":1770:18]
    this = [object Object]
7 onDOMContentLoaded("[object Event]") ["self-hosted":876:16]
    this = [object ChromeWindow]

Finally, TextControlState::UnbindFromFrame() has its own kungFuDeathGrip so that nobody is gone even if it runs script.

The point is, this code is running while we are iterating over the frame tree. If that script that gets run (in this case the QI method for the JS-implemented thing) tried to ask for layout information, we would likely end up with very bad things happening.

Now we happen to control that script, so this is sort of OK in that it's not an imminent security bug. But it does prevent us landing the assertion in bug 1181918 (amongst other things that also prevent it).

Assuming we do need the scripted controllers here, maybe one option would be to QI the controller to nsIXPConnectWrappedJS (which is guaranteed to not run script) and if that succeeds avoid the QI to nsIControllerContext (which we know will fail, since that interface is builtinclass).

Alternately, maybe we could fix nsXPCWrappedJS::DelegatedQueryInterface to just fail out for IDs of builtinclass interfaces before trying to do anything with script? So just call nsXPCWrappedJS::GetInterfaceInfo at the beginning of that function, where we do the check for nsWrapperCache right now (which we could probably remove, then). Andrew, thoughts on that?

In theory an XPCWrappedJS could implement QI to a builtinclass interface via aggregation or something (as in, return an actual built-in object that knows to go back to the JS object for some other IID). In practice, I really doubt we do that or want to support it...

Comment 18

[Andrew McCreight [:mccr8]]

That sounds reasonable. It looks like nsXPCWrappedJS::CallQueryInterfaceOnJSObject, which DelegatedQueryInterface eventually calls, does that. Note the weird issue that nsISupports is builtinclass, but it is okay to QI to it. I'm not sure about any of the other special cases in this method. It does seem like conceptually if we were lazier about doing the AES it would work out, due to the shortcut in CallQueryInterfaceOnJSObject, but it looks like actually being lazy about all of that would be horrible.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #17)

I am still hitting asserts from bug 1181918 in a debug build with this stack:

I cannot access the bug, could you cc me? Or I just want to know how to reproduce it.

#17 0x00007f8c9c0dadcb in mozilla::dom::AutoEntryScript::AutoEntryScript(nsIGlobalObject*, char const*, bool)
    (this=0x7ffec902ca50, aGlobalObject=0x7f8c81d06e80, aReason=0x7f8c934075a2 "XPCWrappedJS QueryInterface", aIsMainThread=true)
    at /home/bzbarsky/mozilla/dom-work/mozilla/dom/script/ScriptSettings.cpp:585
#18 0x00007f8c996ffc8c in nsXPCWrappedJS::DelegatedQueryInterface(nsID const&, void**)
    (this=0x7f8c76ed8f20, aIID=..., aInstancePtr=0x7ffec902cb40)
    at /home/bzbarsky/mozilla/dom-work/mozilla/js/xpconnect/src/XPCWrappedJSClass.cpp:332
#19 0x00007f8c9a11a05e in nsCOMPtr<nsIControllerContext>::assign_from_qi<nsIController>(nsQueryInterface<nsIController>, nsID const&)
    (this=0x7ffec902cb88, aQI=..., aIID=...) at ../../dist/include/nsCOMPtr.h:1157
#20 0x00007f8c9b5b3ae6 in mozilla::TextControlState::UnbindFromFrame(nsTextControlFrame*) (this=0x7f8c772fec80, aFrame=<optimized out>)
    at /home/bzbarsky/mozilla/dom-work/mozilla/dom/html/TextControlState.cpp:2490
#21 0x00007f8c9c7559a8 in nsTextControlFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&)
    (this=0x7f8c7bc43ab8, aDestructRoot=0x7f8c7bc67690, aPostDestroyData=...)
    at /home/bzbarsky/mozilla/dom-work/mozilla/layout/forms/nsTextControlFrame.cpp:145

In this case, in frame 20, controller is JS-implemented. The JS-implemented thing was passed to nsXULControllers::InsertControllerAt from script, with this JS stack:

Wow, I didn't know they are exposed:

• https://searchfox.org/mozilla-central/rev/f98dad153b59a985efd4505912588d4651033395/dom/webidl/HTMLInputElement.webidl#153
• https://searchfox.org/mozilla-central/rev/f98dad153b59a985efd4505912588d4651033395/dom/webidl/HTMLTextAreaElement.webidl#88

Assuming we do need the scripted controllers here, maybe one option would be to QI the controller to nsIXPConnectWrappedJS (which is guaranteed to not run script) and if that succeeds avoid the QI to nsIControllerContext (which we know will fail, since that interface is builtinclass).

Well, I'm afraid about performance regression for adding QI twice per controller...

Comment 20

[Boris Zbarsky [:bzbarsky]]

I cannot access the bug, could you cc me?

Done.

Well, I'm afraid about performance regression for adding QI twice per controller...

Yeah. I'm going to try the XPConnect-side fix here.

Comment 21

[Boris Zbarsky [:bzbarsky]]

It looks like nsXPCWrappedJS::CallQueryInterfaceOnJSObject, which DelegatedQueryInterface eventually calls, does that.

Oh! Indeed. It just does it after possibly running script via the JS_GetPropertyById call to get the QueryInterface function.... Alright, then.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1181957. Don't set up an AutoEntryScript if we're not planning to call script in nsXPCWrappedJS::DelegatedQueryInterface. r=mccr8

For builtinclass interfaces, or ones not declared in IDL, we don't call out
into scripted QueryInterface anyway, so we can return earlier, before we ever
set up the AutoEntryScript.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9122959 [details]
Bug 1181957. Don't set up an AutoEntryScript if we're not planning to call script in nsXPCWrappedJS::DelegatedQueryInterface. r=mccr8

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily at all. In fact, I'm fairly sure this is not exploitable. It's just preventing us from adding an assert that would catch other cases that might be. Assuming those cases exist
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All of them
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: For what it's worth, I don't think this needs backporting to branches.
• How likely is this patch to cause regressions; how much testing does it need?: I think this is quite safe.

Comment 24

[Andrew McCreight [:mccr8]]

You don't need sec-approval for sec-moderate bugs.

Comment 25

[Andrew McCreight [:mccr8]]

This turned into more of an XPConnect issue than an Editor issue, so I'll move it over there.

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/60c6637391e7dffa4bbe68f660eb9125ad1c8978
https://hg.mozilla.org/mozilla-central/rev/60c6637391e7