Closed Bug 1482402 Opened Last year Closed Last year

message-manager-close observer notification can cause us to dispatch DOM events at unsafe times

Categories

(DevTools :: General, defect)

58 Branch
defect
Not set

Tracking

(firefox-esr60 wontfix, firefox62 wontfix, firefox63 fixed, firefox64 fixed)

RESOLVED FIXED
Firefox 64
Tracking Status
firefox-esr60 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Keywords: sec-audit, Whiteboard: [adv-main63-])

Attachments

(1 file)

This was revealed in bug 1451108 when switching us to fatal assertions when dispatching DOM events at unsafe times.

Apparently, some observer of the message-manager-close notification is causing an event to be dispatched. We need to audit them and find it.
Group: core-security → dom-core-security
How bad is this, security-wise?
Flags: needinfo?(mconley)
Keywords: sec-audit
I'm not sure. smaug might have a more informed opinion than me.
Flags: needinfo?(mconley) → needinfo?(bugs)
Possibly very bad. Running scripts when scripts aren't supposed to run. And it is unknown what the scripts might do. sec-high?
Flags: needinfo?(bugs)
Group: dom-core-security → firefox-core-security
Component: DOM → General
Product: Core → Firefox
I got this in rr, and got a JS stack:

0 setIframeVisible(iframe = [object XULFrameElement], visible = false) ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1836]
    this = [object Object]
1 selectSingleNode/<(node = [object XULElement], 9, [object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement]) ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1811]
2 forEach(callbackfn = node => {
      if (node.id === id) {
        node.setAttribute("selected", "true");
        node.setAttribute("aria-selected", "true");
      } else {
        node.removeAttribute("selected");
        node.removeAttribute("aria-selected");
      }
      // The webconsole panel is in a special location due to split console
      if (!node.id) {
        node = this.webconsolePanel;
      }

      const iframe = node.querySelector(".toolbox-panel-iframe");
      if (iframe) {
        let visible = node.id == id;
        // Prevents hiding the split-console if it is currently enabled
        if (node == this.webconsolePanel && this.splitConsole) {
          visible = true;
        }
        this.setIframeVisible(iframe, visible);
      }
    }) ["self-hosted":268]
    this = [object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement]
3 selectSingleNode(collection = [object NodeList], id = "toolbox-panel-inspector") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1791]
    this = [object Object]
4 selectTool(id = "inspector", reason = "tool_unloaded") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1886]
    this = [object Object]
5 selectTool("inspector", "tool_unloaded") ["self-hosted":987]
The message-manager-close notification is firing because of this JS stack:

0 destroyBrowserElement() ["chrome://browser/content/parent/ext-devtools-panels.js":288]
    this = [object Object]
1 buildPanel/<() ["chrome://browser/content/parent/ext-devtools-panels.js":121]
2 unloadTool(toolId = "webext-devtools-panel-_cb6cd29c-2a86-4a9a-922c-e1f522ec8baa_-32-0") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":2604]
    this = [object Object]
3 removeAdditionalTool(toolId = "webext-devtools-panel-_cb6cd29c-2a86-4a9a-922c-e1f522ec8baa_-32-0") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1625]
    this = [object Object]
4 close() ["chrome://browser/content/parent/ext-devtools-panels.js":196]
    this = [object Object]
5 unload() ["resource://gre/modules/ExtensionCommon.jsm":733]
    this = [object Object]
6 unload() ["resource://gre/modules/ExtensionParent.jsm":614]
    this = [object Object]
7 unload() ["resource://gre/modules/ExtensionParent.jsm":696]
    this = [object Object]
8 closeProxyContext(childId = "{cb6cd29c-2a86-4a9a-922c-e1f522ec8baa}.30") ["resource://gre/modules/ExtensionParent.jsm":878]
    this = [object Object]
9 observe() ["resource://gre/modules/ExtensionParent.jsm":777]
10 InterpretGeneratorResume(gen = [object Generator], val = undefined, kind = "next") ["self-hosted":1271]
11 next(val = undefined) ["self-hosted":1226]
    this = [object Generator]
12 shutdown() ["resource://gre/modules/ExtensionParent.jsm":1186]
    this = [object Object]
13 close() ["chrome://browser/content/parent/ext-devtools.js":201]
    this = [object Object]
14 shutdownForTarget(target = TabTarget:[object XULElement]) ["chrome://browser/content/parent/ext-devtools.js":257]
    this = [object Object]
15 onToolboxDestroy(target = TabTarget:[object XULElement]) ["chrome://browser/content/parent/ext-devtools.js":402]
    this = [object Object]
16 onToolboxDestroy(TabTarget:[object XULElement]) ["self-hosted":985]
    this = [object Object]
At the time of the crash, there are 2 script blockers:

The first comes from this stack:

#1  0x00007f30739d81a5 in nsIDocument::BeginUpdate (this=0x7f305b429000) at /home/mconley/Projects/mozilla-central/dom/base/nsDocument.cpp:4898
#2  0x00007f30737a6ca3 in mozAutoDocUpdate::mozAutoDocUpdate (this=0x7ffc7728adc0, aDocument=0x7f305b429000, aNotify=true) at /home/mconley/Projects/mozilla-central/dom/base/mozAutoDocUpdate.h:28
#3  0x00007f30738c0aeb in mozilla::dom::Element::UnsetAttr (this=0x7f304a92cca0, aNameSpaceID=0, aName=0x7f307b656a70 <mozilla::detail::gGkAtoms+53872>, aNotify=true) at /home/mconley/Projects/mozilla-central/dom/base/Element.cpp:2965
#4  0x00007f30738bb875 in mozilla::dom::Element::RemoveAttribute (this=0x7f304a92cca0, aName=..., aError=...) at /home/mconley/Projects/mozilla-central/dom/base/Element.cpp:1412
#5  0x00007f30746f91c0 in mozilla::dom::Element_Binding::removeAttribute (cx=0x7f306c722000, obj=..., self=0x7f304a92cca0, args=...) at /home/mconley/Projects/mozilla-central/debug/dom/bindings/ElementBinding.cpp:1375
#6  0x00007f30749f1e35 in mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> (cx=0x7f306c722000, argc=1, vp=0x7ffc7728b4e0) at /home/mconley/Projects/mozilla-central/dom/bindings/BindingUtils.cpp:3296
#7  0x00007f3078772386 in CallJSNative (cx=0x7f306c722000, native=0x7f30749f1b7c <mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:449
#8  0x00007f3078743f26 in js::InternalCallOrConstruct (cx=0x7f306c722000, args=..., construct=js::NO_CONSTRUCT) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:537
#9  0x00007f3078744332 in InternalCall (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:588
#10 0x00007f30787443ec in js::Call (cx=0x7f306c722000, fval=..., thisv=..., args=..., rval=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:607
#11 0x00007f3078db99cb in js::ForwardingProxyHandler::call (this=0x7f307fd19cb0 <js::CrossCompartmentWrapper::singleton>, cx=0x7f306c722000, proxy=..., args=...) at /home/mconley/Projects/mozilla-central/js/src/proxy/Wrapper.cpp:176
#12 0x00007f3078d9f4e0 in js::CrossCompartmentWrapper::call (this=0x7f307fd19cb0 <js::CrossCompartmentWrapper::singleton>, cx=0x7f306c722000, wrapper=..., args=...) at /home/mconley/Projects/mozilla-central/js/src/proxy/CrossCompartmentWrapper.cpp:339
#13 0x00007f3078dafacb in js::Proxy::call (cx=0x7f306c722000, proxy=..., args=...) at /home/mconley/Projects/mozilla-central/js/src/proxy/Proxy.cpp:513
#14 0x00007f3078743cc1 in js::InternalCallOrConstruct (cx=0x7f306c722000, args=..., construct=js::NO_CONSTRUCT) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:513
#15 0x00007f3078744332 in InternalCall (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:588
#16 0x00007f3078744370 in js::CallFromStack (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:594
#17 0x00007f30787527c3 in Interpret (cx=0x7f306c722000, state=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:3243
#18 0x00007f30787439d4 in js::RunScript (cx=0x7f306c722000, state=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:429
#19 0x00007f3078744098 in js::InternalCallOrConstruct (cx=0x7f306c722000, args=..., construct=js::NO_CONSTRUCT) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:561
#20 0x00007f3078744332 in InternalCall (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:588
#21 0x00007f3078744370 in js::CallFromStack (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:594
#22 0x00007f30788b3ed6 in js::jit::DoCallFallback (cx=0x7f306c722000, frame=0x7ffc7728c508, stub_=0x7f305905c538, argc=3, vp=0x7ffc7728c468, res=...) at /home/mconley/Projects/mozilla-central/js/src/jit/BaselineIC.cpp:2582

JS stack:

0 selectSingleNode/<(node = [object XULElement], 9, [object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement]) ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1797]
1 forEach(callbackfn = node => {
      if (node.id === id) {
        node.setAttribute("selected", "true");
        node.setAttribute("aria-selected", "true");
      } else {
        node.removeAttribute("selected");
        node.removeAttribute("aria-selected");
      }
      // The webconsole panel is in a special location due to split console
      if (!node.id) {
        node = this.webconsolePanel;
      }

      const iframe = node.querySelector(".toolbox-panel-iframe");
      if (iframe) {
        let visible = node.id == id;
        // Prevents hiding the split-console if it is currently enabled
        if (node == this.webconsolePanel && this.splitConsole) {
          visible = true;
        }
        this.setIframeVisible(iframe, visible);
      }
    }) ["self-hosted":268]
    this = [object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement]
2 selectSingleNode(collection = [object NodeList], id = "toolbox-panel-inspector") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1791]
    this = [object Object]
3 selectTool(id = "inspector", reason = "tool_unloaded") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1886]
    this = [object Object]
4 selectTool("inspector", "tool_unloaded") ["self-hosted":987]
    this = [object Object]
5 unloadTool(toolId = "webext-devtools-panel-_e33655ae-71b3-404d-bb42-3b2be1c9cce2_-32-0") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":2624]


The second from this stack:

#0  nsContentUtils::AddScriptBlocker () at /home/mconley/Projects/mozilla-central/dom/base/nsContentUtils.cpp:5546
#1  0x00007f3076209607 in mozilla::PresShell::WillCauseReflow (this=0x7f305a549000) at /home/mconley/Projects/mozilla-central/debug/dist/include/mozilla/PresShell.h:415
#2  0x00007f307620a8a4 in nsAutoCauseReflowNotifier::nsAutoCauseReflowNotifier (this=0x7ffc7728acb0, aShell=0x7f305a549000) at /home/mconley/Projects/mozilla-central/layout/base/PresShell.cpp:453
#3  0x00007f30761da97c in mozilla::PresShell::AttributeChanged (this=0x7f305a549000, aElement=0x7f304a92cca0, aNameSpaceID=0, aAttribute=0x7f307b656a70 <mozilla::detail::gGkAtoms+53872>, aModType=3, aOldValue=0x7ffc7728add0) at /home/mconley/Projects/mozilla-central/layout/base/PresShell.cpp:4461
#4  0x00007f3073a88d3a in nsNodeUtils::AttributeChanged (aElement=0x7f304a92cca0, aNameSpaceID=0, aAttribute=0x7f307b656a70 <mozilla::detail::gGkAtoms+53872>, aModType=3, aOldValue=0x7ffc7728add0) at /home/mconley/Projects/mozilla-central/dom/base/nsNodeUtils.cpp:170
#5  0x00007f30738c12e7 in mozilla::dom::Element::UnsetAttr (this=0x7f304a92cca0, aNameSpaceID=0, aName=0x7f307b656a70 <mozilla::detail::gGkAtoms+53872>, aNotify=true) at /home/mconley/Projects/mozilla-central/dom/base/Element.cpp:3052
#6  0x00007f30738bb875 in mozilla::dom::Element::RemoveAttribute (this=0x7f304a92cca0, aName=..., aError=...) at /home/mconley/Projects/mozilla-central/dom/base/Element.cpp:1412
#7  0x00007f30746f91c0 in mozilla::dom::Element_Binding::removeAttribute (cx=0x7f306c722000, obj=..., self=0x7f304a92cca0, args=...) at /home/mconley/Projects/mozilla-central/debug/dom/bindings/ElementBinding.cpp:1375
#8  0x00007f30749f1e35 in mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> (cx=0x7f306c722000, argc=1, vp=0x7ffc7728b4e0) at /home/mconley/Projects/mozilla-central/dom/bindings/BindingUtils.cpp:3296
#9  0x00007f3078772386 in CallJSNative (cx=0x7f306c722000, native=0x7f30749f1b7c <mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:449
#10 0x00007f3078743f26 in js::InternalCallOrConstruct (cx=0x7f306c722000, args=..., construct=js::NO_CONSTRUCT) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:537
#11 0x00007f3078744332 in InternalCall (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:588
#12 0x00007f30787443ec in js::Call (cx=0x7f306c722000, fval=..., thisv=..., args=..., rval=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:607
#13 0x00007f3078db99cb in js::ForwardingProxyHandler::call (this=0x7f307fd19cb0 <js::CrossCompartmentWrapper::singleton>, cx=0x7f306c722000, proxy=..., args=...) at /home/mconley/Projects/mozilla-central/js/src/proxy/Wrapper.cpp:176
#14 0x00007f3078d9f4e0 in js::CrossCompartmentWrapper::call (this=0x7f307fd19cb0 <js::CrossCompartmentWrapper::singleton>, cx=0x7f306c722000, wrapper=..., args=...) at /home/mconley/Projects/mozilla-central/js/src/proxy/CrossCompartmentWrapper.cpp:339
#15 0x00007f3078dafacb in js::Proxy::call (cx=0x7f306c722000, proxy=..., args=...) at /home/mconley/Projects/mozilla-central/js/src/proxy/Proxy.cpp:513
#16 0x00007f3078743cc1 in js::InternalCallOrConstruct (cx=0x7f306c722000, args=..., construct=js::NO_CONSTRUCT) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:513
#17 0x00007f3078744332 in InternalCall (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:588
#18 0x00007f3078744370 in js::CallFromStack (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:594
#19 0x00007f30787527c3 in Interpret (cx=0x7f306c722000, state=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:3243
#20 0x00007f30787439d4 in js::RunScript (cx=0x7f306c722000, state=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:429
#21 0x00007f3078744098 in js::InternalCallOrConstruct (cx=0x7f306c722000, args=..., construct=js::NO_CONSTRUCT) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:561
#22 0x00007f3078744332 in InternalCall (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:588
#23 0x00007f3078744370 in js::CallFromStack (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:594
#24 0x00007f30788b3ed6 in js::jit::DoCallFallback (cx=0x7f306c722000, frame=0x7ffc7728c508, stub_=0x7f305905c538, argc=3, vp=0x7ffc7728c468, res=...) at /home/mconley/Projects/mozilla-central/js/src/jit/BaselineIC.cpp:2582

JS stack:

0 selectSingleNode/<(node = [object XULElement], 9, [object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement]) ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1797]
1 forEach(callbackfn = node => {
      if (node.id === id) {
        node.setAttribute("selected", "true");
        node.setAttribute("aria-selected", "true");
      } else {
        node.removeAttribute("selected");
        node.removeAttribute("aria-selected");
      }
      // The webconsole panel is in a special location due to split console
      if (!node.id) {
        node = this.webconsolePanel;
      }

      const iframe = node.querySelector(".toolbox-panel-iframe");
      if (iframe) {
        let visible = node.id == id;
        // Prevents hiding the split-console if it is currently enabled
        if (node == this.webconsolePanel && this.splitConsole) {
          visible = true;
        }
        this.setIframeVisible(iframe, visible);
      }
    }) ["self-hosted":268]
    this = [object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement]
2 selectSingleNode(collection = [object NodeList], id = "toolbox-panel-inspector") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1791]
    this = [object Object]
3 selectTool(id = "inspector", reason = "tool_unloaded") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1886]
    this = [object Object]
4 selectTool("inspector", "tool_unloaded") ["self-hosted":987]
    this = [object Object]
5 unloadTool(toolId = "webext-devtools-panel-_e33655ae-71b3-404d-bb42-3b2be1c9cce2_-32-0") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":2624]
What appears to be happening is that we're removing these attributes:

https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/devtools/client/framework/toolbox.js#1796-1797

and we're adding the script blocker in this UnsetAttr method here:

https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/dom/base/Element.cpp#2965

which constructs a mozAutoDocUpdate, which adds the script blocker.

All of this is, I think, in the same turn of the event loop that's calling unloadTool, which is what sends out that observer notification.
Here's a try push where I use a ScriptRunner to fire the message-manager-close notification: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a3e7d329e78d9321729bf6ed08684bdbde8834f
Does this only happen from devtools, or could it conceivably happen from either webpage or non-devtools chrome code?
It looks like our automated tests cover a single case where this happens, but I suppose it's possible that any of the other message-manager-close observers might also sometimes fire events.

message-manager-close is only ever dispatched in the parent process though, so with e10s enabled, I think potential future cuprits here is from privileged content, like our DevTools stuff.

And while we could audit all of the parent process consumers and ensure that none fire DOM events, I wonder if the more robust solution is to fire the notification only when it's safe to run script. That's what my last try push did, and I just need to figure out the test failures in _that_ push.
Okay, I think I've started to sort this out. Gonna do some more prodding with rr today to make sure I have the story straight, but I have a patch that I think fixes things.
Assignee: nobody → mconley
1. A test WebExtension using the DevTools API shuts down, and we get here:

https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/toolkit/components/extensions/ExtensionParent.jsm#1195

so we're destroying the remote XUL browser for the background / DevTools page.

Removing the node puts a script blocker on the stack:

#0  0x00007f464cca2de4 in nsContentUtils::AddScriptBlocker() () at /home/mconley/Projects/mozilla-central/dom/base/nsContentUtils.cpp:5554
#1  0x00007f464cf2d928 in nsIDocument::BeginUpdate() (this=0x7f4634f86000) at /home/mconley/Projects/mozilla-central/dom/base/nsDocument.cpp:5024
#2  0x00007f464ccbeaca in mozAutoDocUpdate::mozAutoDocUpdate(nsIDocument*, bool) (this=0x7ffc2fe55a40, aDocument=0x7f4634f86000, aNotify=true) at /home/mconley/Projects/mozilla-central/dom/base/mozAutoDocUpdate.h:28
#3  0x00007f464cf29fea in nsINode::RemoveChildNode(nsIContent*, bool) (this=0x7f4632aa4d30, aKid=0x7f462146a710, aNotify=true) at /home/mconley/Projects/mozilla-central/dom/base/nsINode.cpp:2029
#4  0x00007f464cf7eda8 in nsINode::RemoveChild(nsINode&, mozilla::ErrorResult&) (this=0x7f4632aa4d30, aOldChild=..., aError=...) at /home/mconley/Projects/mozilla-central/dom/base/nsINode.cpp:577
#5  0x00007f464cf81e8d in nsINode::Remove() (this=0x7f462146a710) at /home/mconley/Projects/mozilla-central/dom/base/nsINode.cpp:1872
#6  0x00007f464dde57b3 in mozilla::dom::Element_Binding::remove(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) (cx=0x7f4646522000, obj=..., self=0x7f462146a710, args=...) at /home/mconley/Projects/mozilla-central/debug/dom/bindings/ElementBinding.cpp:4620
#7  0x00007f464e10e3ef in mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) (cx=0x7f4646522000, argc=0, vp=0x7ffc2fe56030) at /home/mconley/Projects/mozilla-central/dom/bindings/BindingUtils.cpp:3296
#8  0x00007f4652126843 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (cx=0x7f4646522000, native=0x7f464e10e120 <mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:449


2. In the same tick, with the remote <browser> going away, we fire the message-manager-close notification. The DevTools code notices that, and starts tearing down the associated remote iframe that's hosting the WebExtension DevTools panel.

I believe this iframe destruction gets kicked off here: https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/devtools/client/framework/toolbox.js#2623

2. Later on in the same function, another tool is selected, since the removed one was just unloaded:

https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/devtools/client/framework/toolbox.js#2643

3. This attempts to select the right panel in the list of toolbox panels:

https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/devtools/client/framework/toolbox.js#1913

4. We end up dispatching this fake visibilitychange event on the iframe's outer window here: https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/devtools/client/framework/toolbox.js#1863

Because the event is not the "void" event, because it's still unsafe to run script (since steps 1-4 all appear to run on the same tick of the event loop), and because the outer window can't be QI'd to an nsINode, we explode here:

https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/dom/events/EventDispatcher.cpp#907 (since the patch in bug 1451108 makes this a crash instead of a warning).
So, to sum, the problem here is that on removing the background page remote <browser>, we fire a message-manager-close notification, which DevTools JS hears and dispatching an event on the outer window of a node that can't QI to nsINode.
Comment on attachment 9007866 [details]
Bug 1482402 - Dispatch DevTools toolbox visibilitychange event on the iframe document instead of the iframe window. r?gl

Gabriel [:gl] (ΦωΦ) has approved the revision.
Attachment #9007866 - Flags: review+
Comment on attachment 9007866 [details]
Bug 1482402 - Dispatch DevTools toolbox visibilitychange event on the iframe document instead of the iframe window. r?gl

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

  Not easily, I don't think. As far as I can tell, the only thing that might run script on this event is our own DevTools code.

  So I'm not even 100% sure this should be sec-high anymore.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

  No, I don't think so.

Which older supported branches are affected by this flaw?

  The event in question is dispatched since Firefox 59, so I guess that also includes the most recent ESR.

If not all supported branches, which bug introduced the flaw?

  Bug 1399242.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

  No, but it'd be pretty trivial to write them.

How likely is this patch to cause regressions; how much testing does it need?

  Very unlikely, and the automated tests should be fine.
Attachment #9007866 - Flags: sec-approval?
Hey smaug, given the above, do you still think this should be sec-high? The event that's being kicked off is from our DevTools code, and I'm not 100% sure how that could put our users in any kind of danger.
Flags: needinfo?(bugs)
Dan indicated in IRC that he thought this is more of a sec-moderate. I'm setting it to such and clearing the sec-approval? for it (since moderates don't need sec-approval to go in).
Keywords: sec-highsec-moderate
(In reply to Mike Conley (:mconley) (:⚙️) from comment #17)
> Hey smaug, given the above, do you still think this should be sec-high? The
> event that's being kicked off is from our DevTools code, and I'm not 100%
> sure how that could put our users in any kind of danger.

Depends on what other things happen when something handles the event. If nothing too bad, then this isn't sec-high.
Flags: needinfo?(bugs)
Group: firefox-core-security → core-security-release
Please request Beta & ESR60 approval on this when you get a chance.
Flags: needinfo?(mconley)
Comment on attachment 9007866 [details]
Bug 1482402 - Dispatch DevTools toolbox visibilitychange event on the iframe document instead of the iframe window. r?gl

Approval Request Comment
[Feature/Bug causing the regression]:

Bug 1399242

[User impact if declined]:

It's possible for DevTools to fire an event at an "unsafe" time in our browser code. We're not sure if it's exploitable or not, but we're just being cautious.

[Is this code covered by automated tests?]:

The DevTools code is, yes.

[Has the fix been verified in Nightly?]:

The fix was verified after we landed bug 1451108 on Nightly, which causes our debug builds to crash when we get into this condition.

[Needs manual test from QE? If yes, steps to reproduce]: 

N/A

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

We're changing the dispatching element of a visibilitychange event from a window to a document (which is where visibilitychange events should be dispatched from anyway)

[String changes made/needed]:

None.
Flags: needinfo?(mconley)
Attachment #9007866 - Flags: approval-mozilla-beta?
Is this needed on ESR, since it's sec-moderate?
Flags: needinfo?(ryanvm)
If the odds of being exploitable are low, we could probably just let it ride w/ Fx63. Should we revisit the sec rating (maybe sec-audit)?
Flags: needinfo?(ryanvm)
Yeah, sounds good. I don't see how this could be used against the user, but I'll let people who've seen truly evil stuff evaluate.
Keywords: sec-moderatesec-audit
Shouldn't this bug remain open now that the sec-audit keyword is set?
Flags: needinfo?(mconley)
Comment on attachment 9007866 [details]
Bug 1482402 - Dispatch DevTools toolbox visibilitychange event on the iframe document instead of the iframe window. r?gl

Uplift approved for 63 beta 8, thanks.
Flags: needinfo?(mconley)
Attachment #9007866 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Pascal Chevrel:pascalc from comment #26)
> Shouldn't this bug remain open now that the sec-audit keyword is set?

Sure, let's re-open it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [adv-main63-]
This is showing up in my fx::general triage query. Mike, how should this be prioritized? Is there work left to do here? Is it urgent?
Flags: needinfo?(mconley)
(In reply to :Gijs (he/him) from comment #30)
> This is showing up in my fx::general triage query. Mike, how should this be
> prioritized? Is there work left to do here? Is it urgent?

This is a sec-audit bug, so I think we're waiting for our security folks to tell us how exploitable this is, to see if we need to backport it to ESR.

So the urgency is unknown. The work we're waiting on is from the security team.
Flags: needinfo?(mconley)
I'm not sure I understand the state of this bug. There was a bug, and it's been fixed, as I understand it. Is that wrong? What are auditing for, more instance of this sort of bug, or assessing the severity?
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #32)
> I'm not sure I understand the state of this bug. There was a bug, and it's
> been fixed, as I understand it. Is that wrong?

You're right. There was a bug, and it's been fixed, but we're unsure of the severity, so we don't know if it needs to be backported to ESR. 

So yes, we're hoping for a severity audit to inform whether or not a backport is necessary, and whether this should ride into the next ESR point release.
Is this ultimately just about devtools, then, or is there wider impact that we're still hoping to address in this bug? If the former, can you move this to devtools, and perhaps find someone to take point on the severity audit?

(Off-hand, it'd seem best to close this as there was a patch on the bug that has landed, and any follow-up could happen somewhere else - though I'm aware comment #29 explicitly reopened so I won't close this myself.)
Flags: needinfo?(mconley)
(In reply to :Gijs (he/him) from comment #34)
> Is this ultimately just about devtools, then, or is there wider impact that
> we're still hoping to address in this bug? If the former, can you move this
> to devtools, and perhaps find someone to take point on the severity audit?

Sure, let's move this to DevTools. Since it was the only violator that caused the assertion to trip, I guess it's in their court.
 
> (Off-hand, it'd seem best to close this as there was a patch on the bug that
> has landed, and any follow-up could happen somewhere else - though I'm aware
> comment #29 explicitly reopened so I won't close this myself.)

Right - I reopened it at pascalec's request. I'll needinfo him if there's something else we need to do here.
Group: firefox-core-security
Flags: needinfo?(mconley) → needinfo?(pascalc)
Product: Firefox → DevTools
(In reply to Mike Conley (:mconley) (:⚙️) from comment #35)
> 
> Right - I reopened it at pascalec's request. I'll needinfo him if there's
> something else we need to do here.

I would expect somebody in the security team to have a look and close the bug or open a follow up if needed, which is my understanding of the sec-audit keyword per its definition (https://bugzilla.mozilla.org/describekeywords.cgi#sec-audit).

Al should we close the bug now that the patch has landed?
Flags: needinfo?(pascalc) → needinfo?(abillings)
(In reply to Pascal Chevrel:pascalc from comment #36)

> Al should we close the bug now that the patch has landed?

Yes, that's fine.
Flags: needinfo?(abillings)
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Group: firefox-core-security, core-security-release
You need to log in before you can comment on or make changes to this bug.