1228702 - Chrome code can access the location property on Exception and DOMException from off the main thread

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Abdulrahman Alqabandi]

Attachment: firefox-debug_2254_2015-11-27_18-39-20-073.log

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151029151421

Steps to reproduce:

Navigate to 'about:newTab' execute the following:

new Worker(URL.createObjectURL(new Blob(['try{importScripts("favicon.ico")}catch(e1234){for(q in e1234){console.dir(e1234[q])}};'])));



Actual results:

crash. https://crash-stats.mozilla.com/report/index/cf8e8af8-ec2e-4ac5-a5ab-810c12151128


Expected results:

No crash

Im also attaching the windbg stacktrace.

Since the crash seems weird and has never been spotted before (according to crash reporter) then this could be a potential exploitable crash.

Comment 1

[:Gijs (he/him)]

Gabor, can you take a look?

Comment 2

[Gabor Krizsanits (INACTIVE)]

This is not good. A worker is trying to access a DOMException (get_location) for which we try to use XPConnect off the main thread (for wrapping an nsIStackFrame) so we crash. My knowledge here is very limited. Bobby or Boris might know more. If it helps here is a full stack:

* thread #91: tid = 0x14c5e, 0x0000000104460974 XUL`mozilla::dom::NativeInterface2JSObjectAndThrowIfFailed(aCx=0x000000012bda1c00, aScope=Handle<JSObject *> @ 0x0000000154dfac10, aRetval=MutableHandle<JS::Value> @ 0x0000000154dfac08, aHelper=0x0000000154dface8, aIID=0x0000000108c65558, aAllowNativeWrapper=true) + 580 at BindingUtils.cpp:927, name = 'DOM Worker', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000104460974 XUL`mozilla::dom::NativeInterface2JSObjectAndThrowIfFailed(aCx=0x000000012bda1c00, aScope=Handle<JSObject *> @ 0x0000000154dfac10, aRetval=MutableHandle<JS::Value> @ 0x0000000154dfac08, aHelper=0x0000000154dface8, aIID=0x0000000108c65558, aAllowNativeWrapper=true) + 580 at BindingUtils.cpp:927
    frame #1: 0x0000000104460c86 XUL`mozilla::dom::XPCOMObjectToJsval(cx=0x000000012bda1c00, scope=Handle<JSObject *> @ 0x0000000154dfac80, helper=0x0000000154dface8, iid=0x0000000108c65558, allowNativeWrapper=true, rval=MutableHandle<JS::Value> @ 0x0000000154dfac78) + 86 at BindingUtils.cpp:979
    frame #2: 0x0000000104140ff5 XUL`bool mozilla::dom::WrapObject<nsIStackFrame>(cx=0x000000012bda1c00, p=0x000000011fde7430, cache=0x0000000000000000, iid=0x0000000108c65558, rval=MutableHandle<JS::Value> @ 0x0000000154dfad40) + 213 at BindingUtils.h:1438
    frame #3: 0x0000000104140ef4 XUL`bool mozilla::dom::WrapObject<nsIStackFrame>(cx=0x000000012bda1c00, p=0x000000011fde7430, iid=0x0000000108c65558, rval=MutableHandle<JS::Value> @ 0x0000000154dfad98) + 84 at BindingUtils.h:1462
    frame #4: 0x0000000104140dc8 XUL`bool mozilla::dom::WrapObject<nsIStackFrame>(cx=0x000000012bda1c00, p=0x0000000154dfae20, iid=0x0000000108c65558, rval=MutableHandle<JS::Value> @ 0x0000000154dfadd8) + 72 at BindingUtils.h:1499
    frame #5: 0x0000000104115652 XUL`mozilla::dom::ExceptionBinding::get_location(cx=0x000000012bda1c00, obj=Handle<JSObject *> @ 0x0000000154dfae40, self=0x00000001203ebb00, args=JSJitGetterCallArgs @ 0x0000000154dfae38) + 258 at DOMExceptionBinding.cpp:1098
    frame #6: 0x00000001044672e2 XUL`mozilla::dom::GenericBindingGetter(cx=0x000000012bda1c00, argc=0, vp=0x0000000154dfb198) + 594 at BindingUtils.cpp:2599
    frame #7: 0x00000001080f76cd XUL`js::CallJSNative(cx=0x000000012bda1c00, native=(XUL`mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) at BindingUtils.cpp:2576), args=0x0000000154dfb140)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 173 at jscntxtinlines.h:235
    frame #8: 0x00000001080b5bda XUL`js::Invoke(cx=0x000000012bda1c00, args=0x0000000154dfb140, construct=NO_CONSTRUCT) + 1034 at Interpreter.cpp:444
    frame #9: 0x00000001080dcd67 XUL`js::Invoke(cx=0x000000012bda1c00, thisv=0x0000000154dfb6b0, fval=0x0000000154dfb230, argc=0, argv=0x0000000000000000, rval=JS::MutableHandleValue @ 0x0000000154dfb130) + 615 at Interpreter.cpp:496
    frame #10: 0x00000001080dd4cf XUL`js::InvokeGetter(cx=0x000000012bda1c00, thisv=0x0000000154dfb6b0, fval=Value @ 0x0000000154dfb230, rval=JS::MutableHandleValue @ 0x0000000154dfb228) + 127 at Interpreter.cpp:605
    frame #11: 0x0000000108196359 XUL`CallGetter(cx=0x000000012bda1c00, obj=JS::HandleObject @ 0x0000000154dfb2e0, receiver=JS::HandleValue @ 0x0000000154dfb2d8, shape=js::HandleShape @ 0x0000000154dfb2d0, vp=JS::MutableHandleValue @ 0x0000000154dfb2c8) + 233 at NativeObject.cpp:1655
    frame #12: 0x00000001081848f8 XUL`bool GetExistingProperty<(js::AllowGC)1>(cx=0x000000012bda1c00, receiver=js::MaybeRooted<JS::Value, js::AllowGC>::HandleType @ 0x0000000154dfb3e0, obj=js::MaybeRooted<js::NativeObject *, js::AllowGC>::HandleType @ 0x0000000154dfb3d8, shape=js::MaybeRooted<js::Shape *, js::AllowGC>::HandleType @ 0x0000000154dfb3d0, vp=js::MaybeRooted<JS::Value, js::AllowGC>::MutableHandleType @ 0x0000000154dfb3c8)1>::HandleType, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) + 856 at NativeObject.cpp:1703
    frame #13: 0x0000000108184c31 XUL`bool NativeGetPropertyInline<(js::AllowGC)1>(cx=0x000000012bda1c00, obj=js::MaybeRooted<js::NativeObject *, js::AllowGC>::HandleType @ 0x0000000154dfb550, receiver=js::MaybeRooted<JS::Value, js::AllowGC>::HandleType @ 0x0000000154dfb548, id=js::MaybeRooted<jsid, js::AllowGC>::HandleType @ 0x0000000154dfb540, nameLookup=NotNameLookup, vp=js::MaybeRooted<JS::Value, js::AllowGC>::MutableHandleType @ 0x0000000154dfb538)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, IsNameLookup, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) + 449 at NativeObject.cpp:1922
    frame #14: 0x0000000108184a5a XUL`js::NativeGetProperty(cx=0x000000012bda1c00, obj=js::HandleNativeObject @ 0x0000000154dfb5b8, receiver=JS::HandleValue @ 0x0000000154dfb5b0, id=JS::HandleId @ 0x0000000154dfb5a8, vp=JS::MutableHandleValue @ 0x0000000154dfb5a0) + 90 at NativeObject.cpp:1956
    frame #15: 0x0000000108126ca6 XUL`js::GetProperty(cx=0x000000012bda1c00, obj=JS::HandleObject @ 0x0000000154dfb640, receiver=JS::HandleValue @ 0x0000000154dfb638, id=JS::HandleId @ 0x0000000154dfb630, vp=JS::MutableHandleValue @ 0x0000000154dfb628) + 214 at NativeObject.h:1433
    frame #16: 0x0000000108126900 XUL`js::GetProperty(cx=0x000000012bda1c00, obj=JS::HandleObject @ 0x0000000154dfb6d8, receiver=JS::HandleObject @ 0x0000000154dfb6d0, id=JS::HandleId @ 0x0000000154dfb6c8, vp=JS::MutableHandleValue @ 0x0000000154dfb6c0) + 144 at jsobj.h:831
    frame #17: 0x00000001084d3cef XUL`js::GetObjectElementOperation(cx=0x000000012bda1c00, op=JSOP_GETELEM, obj=JS::HandleObject @ 0x0000000154dfb820, receiver=JS::HandleObject @ 0x0000000154dfb818, key=JS::HandleValue @ 0x0000000154dfb810, res=JS::MutableHandleValue @ 0x0000000154dfb808) + 1183 at Interpreter-inl.h:449
    frame #18: 0x00000001084d0efb XUL`js::GetElementOperation(cx=0x000000012bda1c00, op=JSOP_GETELEM, lref=JS::MutableHandleValue @ 0x0000000154dfb910, rref=JS::HandleValue @ 0x0000000154dfb908, res=JS::MutableHandleValue @ 0x0000000154dfb900) + 667 at Interpreter-inl.h:556
    frame #19: 0x00000001084b13c9 XUL`js::jit::DoGetElemFallback(cx=0x000000012bda1c00, frame=0x0000000154dfbca8, stub_=0x000000012c397090, lhs=JS::HandleValue @ 0x0000000154dfbbf0, rhs=JS::HandleValue @ 0x0000000154dfbbe8, res=JS::MutableHandleValue @ 0x0000000154dfbbe0) + 1721 at BaselineIC.cpp:1803
    frame #20: 0x000000011f3d870b
    frame #21: 0x000000011f3d6dc4
    frame #22: 0x0000000107a55842 XUL`EnterBaseline(cx=0x000000012bda1c00, data=0x0000000154dfc1b8) + 1250 at BaselineJIT.cpp:133
    frame #23: 0x0000000107a56058 XUL`js::jit::EnterBaselineAtBranch(cx=0x000000012bda1c00, fp=0x000000013afbe030, pc="?QLM\a????QN?") + 1384 at BaselineJIT.cpp:244
    frame #24: 0x00000001080c2320 XUL`Interpret(cx=0x000000012bda1c00, state=0x0000000154dff328) + 4816 at Interpreter.cpp:1789
    frame #25: 0x00000001080c0f55 XUL`js::RunScript(cx=0x000000012bda1c00, state=0x0000000154dff328) + 725 at Interpreter.cpp:391
    frame #26: 0x00000001080dd94d XUL`js::ExecuteKernel(cx=0x000000012bda1c00, script=JS::HandleScript @ 0x0000000154dff3f8, scopeChainArg=0x0000000154a04070, newTargetValue=0x0000000154dff458, type=EXECUTE_GLOBAL, evalInFrame=(ptr_ = 0), result=0x0000000000000000) + 893 at Interpreter.cpp:650
    frame #27: 0x00000001080ddcec XUL`js::Execute(cx=0x000000012bda1c00, script=JS::HandleScript @ 0x0000000154dff4b8, scopeChainArg=0x0000000154a04070, rval=0x0000000000000000) + 828 at Interpreter.cpp:684
    frame #28: 0x0000000107e472cb XUL`Evaluate(cx=0x000000012bda1c00, scope=JS::HandleObject @ 0x0000000154dff6c8, staticScope=Handle<js::ScopeObject *> @ 0x0000000154dff6c0, optionsArg=0x0000000154dff838, srcBuf=0x0000000154dff818, rval=JS::MutableHandleValue @ 0x0000000154dff6b8) + 1019 at jsapi.cpp:4504
    frame #29: 0x0000000107e47498 XUL`JS::Evaluate(cx=0x000000012bda1c00, optionsArg=0x0000000154dff838, srcBuf=0x0000000154dff818, rval=JS::MutableHandleValue @ 0x0000000154dff788) + 200 at jsapi.cpp:4585
    frame #30: 0x000000010519c6b3 XUL`(anonymous namespace)::ScriptExecutorRunnable::WorkerRun(this=0x000000013b6af470, aCx=0x000000012bda1c00, aWorkerPrivate=0x0000000135202800) + 1667 at ScriptLoader.cpp:1776
    frame #31: 0x0000000105206012 XUL`mozilla::dom::workers::WorkerRunnable::Run(this=0x000000013b6af470) + 1874 at WorkerRunnable.cpp:360
    frame #32: 0x00000001017378bf XUL`nsThread::ProcessNextEvent(this=0x00000001200fc740, aMayWait=false, aResult=0x0000000154dffe8e) + 1935 at nsThread.cpp:964
    frame #33: 0x00000001017bf327 XUL`NS_ProcessNextEvent(aThread=0x00000001200fc740, aMayWait=false) + 151 at nsThreadUtils.cpp:297
    frame #34: 0x00000001051fdf69 XUL`mozilla::dom::workers::WorkerPrivate::RunCurrentSyncLoop(this=0x0000000135202800) + 841 at WorkerPrivate.cpp:5287
    frame #35: 0x00000001051b2ab0 XUL`mozilla::dom::workers::AutoSyncLoopHolder::Run(this=0x0000000154dfffb8) + 48 at WorkerPrivate.h:1434
    frame #36: 0x000000010518b8c7 XUL`(anonymous namespace)::LoadAllScripts(aCx=0x000000012bda1c00, aWorkerPrivate=0x0000000135202800, aLoadInfos=0x0000000154e00020, aIsMainScript=true, aWorkerScriptType=WorkerScript, aRv=0x0000000154e00078)::ScriptLoadInfo>&, bool, mozilla::dom::workers::WorkerScriptType, mozilla::ErrorResult&) + 647 at ScriptLoader.cpp:1902
    frame #37: 0x000000010518b5d6 XUL`mozilla::dom::workers::scriptloader::LoadMainScript(aCx=0x000000012bda1c00, aScriptURL=u"blob:null/42bf3d11-18fa-0d47-ad0f-6dede3e2b1d1", aWorkerScriptType=WorkerScript, aRv=0x0000000154e00078) + 166 at ScriptLoader.cpp:1997
    frame #38: 0x000000010521de54 XUL`(anonymous namespace)::CompileScriptRunnable::WorkerRun(this=0x0000000132de9e40, aCx=0x000000012bda1c00, aWorkerPrivate=0x0000000135202800) + 68 at WorkerPrivate.cpp:497
    frame #39: 0x0000000105206012 XUL`mozilla::dom::workers::WorkerRunnable::Run(this=0x0000000132de9e40) + 1874 at WorkerRunnable.cpp:360
    frame #40: 0x00000001017378bf XUL`nsThread::ProcessNextEvent(this=0x00000001200fc740, aMayWait=false, aResult=0x0000000154e0056e) + 1935 at nsThread.cpp:964
    frame #41: 0x00000001017bf327 XUL`NS_ProcessNextEvent(aThread=0x00000001200fc740, aMayWait=false) + 151 at nsThreadUtils.cpp:297
    frame #42: 0x00000001051f99e0 XUL`mozilla::dom::workers::WorkerPrivate::DoRunLoop(this=0x0000000135202800, aCx=0x000000012bda1c00) + 2160 at WorkerPrivate.cpp:4511
    frame #43: 0x0000000105195257 XUL`(anonymous namespace)::WorkerThreadPrimaryRunnable::Run(this=0x000000011fce2700) + 711 at RuntimeService.cpp:2722
    frame #44: 0x00000001017378bf XUL`nsThread::ProcessNextEvent(this=0x00000001200fc740, aMayWait=true, aResult=0x0000000154e00c3e) + 1935 at nsThread.cpp:964
    frame #45: 0x00000001017bf327 XUL`NS_ProcessNextEvent(aThread=0x00000001200fc740, aMayWait=true) + 151 at nsThreadUtils.cpp:297
    frame #46: 0x0000000101e70b1a XUL`mozilla::ipc::MessagePumpForNonMainThreads::Run(this=0x0000000132de9c80, aDelegate=0x000000014d67ea00) + 954 at MessagePump.cpp:355
    frame #47: 0x0000000101d9cae5 XUL`MessageLoop::RunInternal(this=0x000000014d67ea00) + 117 at message_loop.cc:234
    frame #48: 0x0000000101d9c9f5 XUL`MessageLoop::RunHandler(this=0x000000014d67ea00) + 21 at message_loop.cc:227
    frame #49: 0x0000000101d9c99d XUL`MessageLoop::Run(this=0x000000014d67ea00) + 45 at message_loop.cc:201
    frame #50: 0x00000001017357cd XUL`nsThread::ThreadFunc(aArg=0x00000001200fc740) + 445 at nsThread.cpp:376
    frame #51: 0x0000000101378f11 libnss3.dylib`_pt_root(arg=0x000000011a8ff420) + 449 at ptthread.c:212
    frame #52: 0x00007fff85b1a05a libsystem_pthread.dylib`_pthread_body + 131
    frame #53: 0x00007fff85b19fd7 libsystem_pthread.dylib`_pthread_start + 176
    frame #54: 0x00007fff85b173ed libsystem_pthread.dylib`thread_start + 13

Comment 3

[Bobby Holley (:bholley)]

Boris touched all this exception stuff recently.

Comment 4

[Abdulrahman Alqabandi]

May I point out this is not exclusive to about:newTab, it also works on 'about:' but not all about pages, for example 'about:robots' & 'about:mozilla' are safe.

Comment 5

[Boris Zbarsky [:bzbarsky]]

This isn't exactly a new problem; it's been there ever since we exposed DOMExceptions on workers.  The basic issue is that the "location" property on ExceptionMembers is [ChromeOnly] and is very much not safe to call from workers.  The testcase in comment 0 indirectly calls it from a worker and that worker is started from a system-privileged page via a blob: URL, so is running with the system principal.

Now in practice this is not a security issue in my opinion because if you can touch this property you're system code already, so you can do whatever the heck you want anyway (say scribble all over the hard drive with nsIFile, or us js-ctypes to crash spectacularly or whatever).  This is why you get the crash with "about:newtab" (which is system-privileged) but not "about:mozille" (which is not).

What we could do to make it a little harder to accidentally shoot yourself in the foot here is mark this property [Exposed=Window], now that bug 1048695 has landed.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, this is exciting.  This is an Exception, not a DOMException, and the exposure set for that wasn't set up right, which caused [Exposed=Window] on a member to be a no-op.  Lovely...

With that fixed, things are good.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't expose the 'location' property of Exception/DOMException on workers

We could also have the C++ getter bail out if not NS_IsMainThread(), I guess, for super-extra-safety...

Comment 8

[Abdulrahman Alqabandi]

I agree that this bug alone is not as much of a security risk, as it would be unlikely for a person to copy paste code in the console. However, my concern (or hope) was that this bug could be used with a chrome-code injection bug that could result in a remote code execution exploit. But I take it that you already have remote code execution if you have chrome-injection capabilities?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> But I take it that you already have remote code execution if you have chrome-injection
> capabilities?

Yep.  You can just write some executable data to disk and then use nsIProcess to launch that executable, for example.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8694552 [details] [diff] [review]
Don't expose the 'location' property of Exception/DOMException on workers

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

::: dom/webidl/DOMException.webidl
@@ +54,3 @@
>    readonly attribute StackFrame?             location;
>    // An inner exception that triggered this, if available.
>    readonly attribute nsISupports?            inner;

Is this threadsafe?

@@ +55,5 @@
>    // An inner exception that triggered this, if available.
>    readonly attribute nsISupports?            inner;
>  
>    // Arbitary data for the implementation.
>    readonly attribute nsISupports?            data;

and this?

@@ +59,5 @@
>    readonly attribute nsISupports?            data;
>  
>    // Formatted exception stack
>    [Throws, Replaceable]
>    readonly attribute DOMString               stack;

and this?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Is this threadsafe?

Hmm.  This might be dead code altogether.  I can't find anything that ever sets inner to non-null.  I thought something in XPConnect still did last I checked, but I guess not.  Filed bug 1229664 to nix it.

> and this?

The only places that pass in non-null data are nsXPCComponents_Exception::CallOrConstruct and XPCConvert::ConstructException.  The former is clearly mainthread-only.  The latter is only called from other XPCConvert stuff, and I think it's all mainthread-only.  But yes, we could totally hide this on workers; I'll do that in this bug.  And I'd rather like to nix it in general.

> and this?

I believe so.  This does look at an already-existing nsIStackFrame internally, but calls GetFormattedStack on it, which uses the SpiderMonkey savedstack APIs.  Those should be fine on workers, I believe.

Comment 12

[Andrew McCreight [:mccr8]]

This requires chrome privileges, so I'm going to unhide it.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87d9fe68abf2

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87d9fe68abf2