Closed Bug 1378207 (CVE-2017-7820) Opened 7 years ago Closed 7 years ago

instanceof operator can bypass Xray and run unprivileged proxy trap

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 + fixed
firefox57 + fixed

People

(Reporter: Oriol, Assigned: gkrizsanits)

References

(Depends on 1 open bug)

Details

(Keywords: sec-low, Whiteboard: [adv-main56+])

Attachments

(2 files, 1 obsolete file)

Open the web console in a new tab, and run

  var obj, iframe = document.createElement('iframe');
  iframe.onload = function() {
    obj = iframe.contentWindow.eval(`new Proxy({}, {
      getPrototypeOf: function() { alert('proxy trap'); return null; }
    })`);
  };
  document.body.appendChild(iframe);

You can check that obj is an Xray wrapper:

  Cu.isXrayWrapper(obj); // true

Then, run

  obj instanceof Ci.nsIDOMWindow;

The trap runs and the alert is shown!! I think Xray is supposed to avoid this kind of things, e.g. this does not run the trap

  obj instanceof Window;

Since the console uses instanceof to display objects nicely, any page can exploit this to freeze Firefox by running

  var proxy = new Proxy({}, {getPrototypeOf: () => proxy });
  console.log(Object.create(proxy));

Not sure it's necessary, but since Xray is a security thing and is being bypassed, I am marking this bug as core-security.
The about:newtab page is privileged (considered part of Firefox UI). Things you try on that page can behave quite differently from things done on an actual web page.
Group: core-security → dom-core-security
Gabor, could you figure out if something bad is going on here? Thanks.
Component: Security → XPConnect
Flags: needinfo?(gkrizsanits)
Also keep in mind that things done in the web console are executed in a page-privileged sandbox which also can behave differently than code running in the page itself. not sure if that's relevant to this case -- I executed the code using a javascript: url in the addressbar (which should be closer to executing as if it were the page) and it still hung that child process.
Oh, I thought Firefox froze because the console code is privileged, but any webpage can bypass dom.max_script_run_time just with

  var proxy = new Proxy({}, {getPrototypeOf: () => proxy });
  proxy instanceof Function;

Should I file another bug for this? Is this security sensitive?
Bypassing the slow script dialog isn't such a big issue.

The bigger question here is whether chrome can actually trigger a JS proxy trap on a content object over Xrays. Proxy objects are supposed to be opaque over Xrays, so if we're executing a content-defined getPrototypeOf function, we should fix that.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> The bigger question here is whether chrome can actually trigger a JS proxy
> trap on a content object over Xrays. Proxy objects are supposed to be opaque
> over Xrays, so if we're executing a content-defined getPrototypeOf function,
> we should fix that.

Good news: that's not what's happening, xrays are fine I think.

Bad news: we don't have xrays here at all, which is quite scary. Seems like we don't execute scratchpad code any more in the good old sandbox way but using DebuggerObject::executeInGlobal which seems to be stripping off xrays. I think Eddy worked on worker debugging and there it was important not to use any xpconnect wrappers maybe that is related.

Note: we're stripping off xrays even if we execute system code on an object from content scope (so instead of about:newtab the content is some regular web content) which is a serious concern, and I'm not sure what other wrappers we might stripping off...

Jim, do you know why are we using the Debugger eval for scratchpad and if that can be the reason why we don't have xray in this case? Can that mean that we don't have any security wrappers at all in this setup? (I'll try to test that hopefully today...) Also, where else do we use this eval? This might be a very important problem...
Flags: needinfo?(gkrizsanits) → needinfo?(jimb)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6)
> Bad news: we don't have xrays here at all, which is quite scary. 

Ehh... ignore my previous comment the example checked if Cu.isXrayWrapper(obj) is true so it seems like we indeed circumvent xrays here for some reason unintentionally. In non-e10s mode this can be reproduced executing the scratchpad in the browser context (system code) on a regular web page, which is a huge problem. But there can be other issues as well depending on what the underlying problem is. The stack is (I've added a sin(1) call before the alert() in the example):

 thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x000000010a1fc4f3 XUL`js::math_sin(cx=0x000000011ec51000, argc=1, vp=0x000000011f23a708) at jsmath.cpp:905
    frame #1: 0x000000010aa77164 XUL`js::CallJSNative(cx=0x000000011ec51000, native=(XUL`js::math_sin(JSContext*, unsigned int, JS::Value*) at jsmath.cpp:904), args=0x00007fff5fbef320)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) at jscntxtinlines.h:293
    frame #2: 0x000000010a218287 XUL`js::InternalCallOrConstruct(cx=0x000000011ec51000, args=0x00007fff5fbef320, construct=NO_CONSTRUCT) at Interpreter.cpp:470
    frame #3: 0x000000010a21877d XUL`InternalCall(cx=0x000000011ec51000, args=0x00007fff5fbef320) at Interpreter.cpp:515
    frame #4: 0x000000010a21857d XUL`js::CallFromStack(cx=0x000000011ec51000, args=0x00007fff5fbef320) at Interpreter.cpp:521
    frame #5: 0x000000010a20cd29 XUL`Interpret(cx=0x000000011ec51000, state=0x00007fff5fbf0448) at Interpreter.cpp:3060
    frame #6: 0x000000010a201b19 XUL`js::RunScript(cx=0x000000011ec51000, state=0x00007fff5fbf0448) at Interpreter.cpp:410
    frame #7: 0x000000010a218385 XUL`js::InternalCallOrConstruct(cx=0x000000011ec51000, args=0x00007fff5fbf0698, construct=NO_CONSTRUCT) at Interpreter.cpp:488
    frame #8: 0x000000010a21877d XUL`InternalCall(cx=0x000000011ec51000, args=0x00007fff5fbf0698) at Interpreter.cpp:515
    frame #9: 0x000000010a2187f6 XUL`js::Call(cx=0x000000011ec51000, fval=JS::HandleValue @ 0x00007fff5fbf05c0, thisv=JS::HandleValue @ 0x00007fff5fbf05b8, args=0x00007fff5fbf0698, rval=JS::MutableHandleValue @ 0x00007fff5fbf05b0) at Interpreter.cpp:534
    frame #10: 0x000000010aa51e69 XUL`js::ScriptedProxyHandler::getPrototype(this=0x000000010ea61580, cx=0x000000011ec51000, proxy=JS::HandleObject @ 0x00007fff5fbf07a0, protop=JS::MutableHandleObject @ 0x00007fff5fbf0798) const at ScriptedProxyHandler.cpp:202
    frame #11: 0x000000010aa4dc35 XUL`js::Proxy::getPrototype(cx=0x000000011ec51000, proxy=JS::HandleObject @ 0x00007fff5fbf07f0, proto=JS::MutableHandleObject @ 0x00007fff5fbf07e8) at Proxy.cpp:197
    frame #12: 0x000000010a7eed99 XUL`js::GetPrototype(cx=0x000000011ec51000, obj=JS::HandleObject @ 0x00007fff5fbf0840, protop=JS::MutableHandleObject @ 0x00007fff5fbf0838) at jsobjinlines.h:188
    frame #13: 0x000000010a7eecd5 XUL`JS_GetPrototype(cx=0x000000011ec51000, obj=JS::HandleObject @ 0x00007fff5fbf0888, result=JS::MutableHandleObject @ 0x00007fff5fbf0880) at jsapi.cpp:2045
    frame #14: 0x000000010a89f263 XUL`js::GetObjectProto(cx=0x000000011ec51000, obj=Handle<JSObject *> @ 0x00007fff5fbf08c0, proto=MutableHandle<JSObject *> @ 0x00007fff5fbf08b8) at jsfriendapi.cpp:499
    frame #15: 0x0000000103299717 XUL`FindObjectForHasInstance(cx=0x000000011ec51000, objArg=JS::HandleObject @ 0x00007fff5fbf0990, target=JS::MutableHandleObject @ 0x00007fff5fbf0988) at XPCJSID.cpp:471
    frame #16: 0x0000000103299383 XUL`xpc::HasInstance(cx=0x000000011ec51000, objArg=JS::HandleObject @ 0x00007fff5fbf0a50, iid=0x000000011ece5100, bp=0x00007fff5fbf0bd7) at XPCJSID.cpp:488
    frame #17: 0x0000000103299895 XUL`nsJSIID::HasInstance(this=0x000000011eb7a2e0, wrapper=0x000000012171b040, cx=0x000000011ec51000, (null)=0x0000000122e27670, val=JS::HandleValue @ 0x00007fff5fbf0af0, bp=0x00007fff5fbf0bd7, _retval=0x00007fff5fbf0bbf) at XPCJSID.cpp:532
    frame #18: 0x000000010329990c XUL`non-virtual thunk to nsJSIID::HasInstance(this=0x000000011eb7a2e0, wrapper=0x000000012171b040, cx=0x000000011ec51000, (null)=0x0000000122e27670, val=JS::HandleValue @ 0x00007fff5fbf0b58, bp=0x00007fff5fbf0bd7, _retval=0x00007fff5fbf0bbf) at XPCJSID.cpp:0
    frame #19: 0x0000000103305dbc XUL`XPC_WN_Helper_HasInstance(cx=0x000000011ec51000, obj=JS::HandleObject @ 0x00007fff5fbf0bf0, valp=JS::MutableHandleValue @ 0x00007fff5fbf0be8, bp=0x00007fff5fbf270f) at XPCWrappedNativeJSOps.cpp:776
    frame #20: 0x000000010a21a04d XUL`js::HasInstance(cx=0x000000011ec51000, obj=JS::HandleObject @ 0x00007fff5fbf0c90, v=JS::HandleValue @ 0x00007fff5fbf0c88, bp=0x00007fff5fbf270f) at Interpreter.cpp:777
    frame #21: 0x000000010a214324 XUL`Interpret(cx=0x000000011ec51000, state=0x00007fff5fbf3f90) at Interpreter.cpp:3927
    frame #22: 0x000000010a201b19 XUL`js::RunScript(cx=0x000000011ec51000, state=0x00007fff5fbf3f90) at Interpreter.cpp:410
    frame #23: 0x000000010a219613 XUL`js::ExecuteKernel(cx=0x000000011ec51000, script=JS::HandleScript @ 0x00007fff5fbf4050, envChainArg=0x000000011c106fc0, newTargetValue=0x00007fff5fbf4110, evalInFrame=(ptr_ = 0), result=0x00007fff5fbf4490) at Interpreter.cpp:699
    frame #24: 0x000000010ab79408 XUL`EvaluateInEnv(cx=0x000000011ec51000, env=Handle<JSObject *> @ 0x00007fff5fbf42b8, frame=(ptr_ = 0), pc=0x0000000000000000, chars=Range<const char16_t> @ 0x00007fff5fbf42e0, filename="Scratchpad/2", lineno=1, rval=JS::MutableHandleValue @ 0x00007fff5fbf42a8) at Debugger.cpp:8175
    frame #25: 0x000000010ab25baf XUL`DebuggerGenericEval(cx=0x000000011ec51000, chars=const mozilla::Range<const char16_t> @ 0x00007fff5fbf47f0, bindings=JS::HandleObject @ 0x00007fff5fbf4608, options=0x00007fff5fbf4a90, status=0x00007fff5fbf4a84, value=JS::MutableHandleValue @ 0x00007fff5fbf4600, dbg=0x000000015ef30800, envArg=JS::HandleObject @ 0x00007fff5fbf45f8, iter=0x0000000000000000) at Debugger.cpp:8260
    frame #26: 0x000000010ab351cb XUL`js::DebuggerObject::executeInGlobal(cx=0x000000011ec51000, object=js::HandleDebuggerObject @ 0x00007fff5fbf4940, chars=Range<const char16_t> @ 0x00007fff5fbf4960, bindings=JS::HandleObject @ 0x00007fff5fbf4938, options=0x00007fff5fbf4a90, status=0x00007fff5fbf4a84, value=JS::MutableHandleValue @ 0x00007fff5fbf4930) at Debugger.cpp:10892
    frame #27: 0x000000010ab35680 XUL`js::DebuggerObject::executeInGlobalWithBindingsMethod(cx=0x000000011ec51000, argc=3, vp=0x00007fff5fbf4f10) at Debugger.cpp:10011
    frame #28: 0x000000010aa77164 XUL`js::CallJSNative(cx=0x000000011ec51000, native=(XUL`js::DebuggerObject::executeInGlobalWithBindingsMethod(JSContext*, unsigned int, JS::Value*) at Debugger.cpp:9985), args=0x00007fff5fbf4ec0)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) at jscntxtinlines.h:293
    frame #29: 0x000000010a218287 XUL`js::InternalCallOrConstruct(cx=0x000000011ec51000, args=0x00007fff5fbf4ec0, construct=NO_CONSTRUCT) at Interpreter.cpp:470
    frame #30: 0x000000010a21877d XUL`InternalCall(cx=0x000000011ec51000, args=0x00007fff5fbf4ec0) at Interpreter.cpp:515
    frame #31: 0x000000010a2187f6 XUL`js::Call(cx=0x000000011ec51000, fval=JS::HandleValue @ 0x00007fff5fbf4e10, thisv=JS::HandleValue @ 0x00007fff5fbf4e08, args=0x00007fff5fbf4ec0, rval=JS::MutableHandleValue @ 0x00007fff5fbf4e00) at Interpreter.cpp:534
    frame #32: 0x000000010aa5b150 XUL`js::Wrapper::call(this=0x000000010ea60f40, cx=0x000000011ec51000, proxy=JS::HandleObject @ 0x00007fff5fbf4eb0, args=0x00007fff5fbf5188) const at Wrapper.cpp:169
    frame #33: 0x000000010aa404cf XUL`js::CrossCompartmentWrapper::call(this=0x000000010ea60f40, cx=0x000000011ec51000, wrapper=JS::HandleObject @ 0x00007fff5fbf5040, args=0x00007fff5fbf5188) const at CrossCompartmentWrapper.cpp:359
    frame #34: 0x000000010aa4f9b8 XUL`js::Proxy::call(cx=0x000000011ec51000, proxy=JS::HandleObject @ 0x00007fff5fbf50e0, args=0x00007fff5fbf5188) at Proxy.cpp:481
    frame #35: 0x000000010aa50c18 XUL`js::proxy_Call(cx=0x000000011ec51000, argc=3, vp=0x000000011f23a5b0) at Proxy.cpp:741
    frame #36: 0x000000010aa77164 XUL`js::CallJSNative(cx=0x000000011ec51000, native=(XUL`js::proxy_Call(JSContext*, unsigned int, JS::Value*) at Proxy.cpp:737), args=0x00007fff5fbf7560)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) at jscntxtinlines.h:293
    frame #37: 0x000000010a2180b8 XUL`js::InternalCallOrConstruct(cx=0x000000011ec51000, args=0x00007fff5fbf7560, construct=NO_CONSTRUCT) at Interpreter.cpp:452
    frame #38: 0x000000010a21877d XUL`InternalCall(cx=0x000000011ec51000, args=0x00007fff5fbf7560) at Interpreter.cpp:515
    frame #39: 0x000000010a21857d XUL`js::CallFromStack(cx=0x000000011ec51000, args=0x00007fff5fbf7560) at Interpreter.cpp:521
    frame #40: 0x000000010a20cd29 XUL`Interpret(cx=0x000000011ec51000, state=0x00007fff5fbf8688) at Interpreter.cpp:3060
    frame #41: 0x000000010a201b19 XUL`js::RunScript(cx=0x000000011ec51000, state=0x00007fff5fbf8688) at Interpreter.cpp:410
    frame #42: 0x000000010a218385 XUL`js::InternalCallOrConstruct(cx=0x000000011ec51000, args=0x00007fff5fbfa990, construct=NO_CONSTRUCT) at Interpreter.cpp:488
    frame #43: 0x000000010a21877d XUL`InternalCall(cx=0x000000011ec51000, args=0x00007fff5fbfa990) at Interpreter.cpp:515
    frame #44: 0x000000010a21857d XUL`js::CallFromStack(cx=0x000000011ec51000, args=0x00007fff5fbfa990) at Interpreter.cpp:521
    frame #45: 0x000000010a20cd29 XUL`Interpret(cx=0x000000011ec51000, state=0x00007fff5fbfbab8) at Interpreter.cpp:3060
    frame #46: 0x000000010a201b19 XUL`js::RunScript(cx=0x000000011ec51000, state=0x00007fff5fbfbab8) at Interpreter.cpp:410
    frame #47: 0x000000010a218385 XUL`js::InternalCallOrConstruct(cx=0x000000011ec51000, args=0x00007fff5fbfbce0, construct=NO_CONSTRUCT) at Interpreter.cpp:488
    frame #48: 0x000000010a21877d XUL`InternalCall(cx=0x000000011ec51000, args=0x00007fff5fbfbce0) at Interpreter.cpp:515
    frame #49: 0x000000010a2187f6 XUL`js::Call(cx=0x000000011ec51000, fval=JS::HandleValue @ 0x00007fff5fbfbc30, thisv=JS::HandleValue @ 0x00007fff5fbfbc28, args=0x00007fff5fbfbce0, rval=JS::MutableHandleValue @ 0x00007fff5fbfbc20) at Interpreter.cpp:534
    frame #50: 0x000000010aa5b150 XUL`js::Wrapper::call(this=0x000000010ea60f40, cx=0x000000011ec51000, proxy=JS::HandleObject @ 0x00007fff5fbfbcd0, args=0x00007fff5fbfbfa8) const at Wrapper.cpp:169
    frame #51: 0x000000010aa404cf XUL`js::CrossCompartmentWrapper::call(this=0x000000010ea60f40, cx=0x000000011ec51000, wrapper=JS::HandleObject @ 0x00007fff5fbfbe60, args=0x00007fff5fbfbfa8) const at CrossCompartmentWrapper.cpp:359
    frame #52: 0x000000010aa4f9b8 XUL`js::Proxy::call(cx=0x000000011ec51000, proxy=JS::HandleObject @ 0x00007fff5fbfbf00, args=0x00007fff5fbfbfa8) at Proxy.cpp:481
    frame #53: 0x000000010aa50c18 XUL`js::proxy_Call(cx=0x000000011ec51000, argc=1, vp=0x00007fff5fbfc2f0) at Proxy.cpp:741
    frame #54: 0x000000010aa77164 XUL`js::CallJSNative(cx=0x000000011ec51000, native=(XUL`js::proxy_Call(JSContext*, unsigned int, JS::Value*) at Proxy.cpp:737), args=0x00007fff5fbfc2a0)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) at jscntxtinlines.h:293
    frame #55: 0x000000010a2180b8 XUL`js::InternalCallOrConstruct(cx=0x000000011ec51000, args=0x00007fff5fbfc2a0, construct=NO_CONSTRUCT) at Interpreter.cpp:452
    frame #56: 0x000000010a21877d XUL`InternalCall(cx=0x000000011ec51000, args=0x00007fff5fbfc2a0) at Interpreter.cpp:515
    frame #57: 0x000000010a2187f6 XUL`js::Call(cx=0x000000011ec51000, fval=JS::HandleValue @ 0x00007fff5fbfc1f0, thisv=JS::HandleValue @ 0x00007fff5fbfc1e8, args=0x00007fff5fbfc2a0, rval=JS::MutableHandleValue @ 0x00007fff5fbfc1e0) at Interpreter.cpp:534
    frame #58: 0x000000010a7f41ac XUL`JS_CallFunctionValue(cx=0x000000011ec51000, obj=JS::HandleObject @ 0x00007fff5fbfc290, fval=JS::HandleValue @ 0x00007fff5fbfc288, args=0x00007fff5fbfc648, rval=JS::MutableHandleValue @ 0x00007fff5fbfc280) at jsapi.cpp:2889
    frame #59: 0x0000000103ff8fa7 XUL`nsFrameMessageManager::ReceiveMessage(this=0x000000012ed6bd80, aTarget=0x000000012f007590, aTargetFrameLoader=0x000000013400ba80, aTargetClosed=false, aMessage=0x000000012197ec28, aIsSync=false, aCloneData=0x000000012197ec38, aCpows=0x00007fff5fbfcd00, aPrincipal=0x0000000000000000, aRetVal=0x0000000000000000) at nsFrameMessageManager.cpp:1093
    frame #60: 0x0000000103ff7713 XUL`nsFrameMessageManager::ReceiveMessage(this=0x000000012ed6bd80, aTarget=0x000000012f007590, aTargetFrameLoader=0x000000013400ba80, aMessage=0x000000012197ec28, aIsSync=false, aCloneData=0x000000012197ec38, aCpows=0x00007fff5fbfcd00, aPrincipal=0x0000000000000000, aRetVal=0x0000000000000000) at nsFrameMessageManager.cpp:902
    frame #61: 0x0000000103ffe205 XUL`nsSameProcessAsyncMessageBase::ReceiveMessage(this=0x000000012197ec28, aTarget=0x000000012f007590, aTargetFrameLoader=0x000000013400ba80, aManager=0x000000012ed6bd80) at nsFrameMessageManager.cpp:2083
    frame #62: 0x0000000104316c0f XUL`nsAsyncMessageToChild::Run(this=0x000000012197ec00) at nsFrameLoader.cpp:3187
    frame #63: 0x0000000101eb24f3 XUL`nsThread::ProcessNextEvent(this=0x000000010069b400, aMayWait=false, aResult=0x00007fff5fbfd073) at nsThread.cpp:1437
    frame #64: 0x0000000101eae02c XUL`NS_ProcessPendingEvents(aThread=0x000000010069b400, aTimeout=10) at nsThreadUtils.cpp:431
    frame #65: 0x000000010699e21e XUL`nsBaseAppShell::NativeEventCallback(this=0x000000011ef29c50) at nsBaseAppShell.cpp:97
    frame #66: 0x0000000106a3b8a7 XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x000000011ef29c50) at nsAppShell.mm:398
    frame #67: 0x00007fffd4d67321 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #68: 0x00007fffd4d4821d CoreFoundation`__CFRunLoopDoSources0 + 557
    frame #69: 0x00007fffd4d47716 CoreFoundation`__CFRunLoopRun + 934
    frame #70: 0x00007fffd4d47114 CoreFoundation`CFRunLoopRunSpecific + 420
    frame #71: 0x00007fffd42a8ebc HIToolbox`RunCurrentEventLoopInMode + 240
    frame #72: 0x00007fffd42a8cf1 HIToolbox`ReceiveNextEventCommon + 432
    frame #73: 0x00007fffd42a8b26 HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 71
    frame #74: 0x00007fffd2841a54 AppKit`_DPSNextEvent + 1120
    frame #75: 0x00007fffd2fbd7ee AppKit`-[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 2796
    frame #76: 0x0000000106a3a3a4 XUL`::-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](self=0x00000001006ec3e0, _cmd="nextEventMatchingMask:untilDate:inMode:dequeue:", mask=18446744073709551615, expiration=4001-01-01 00:00:00 UTC, mode="kCFRunLoopDefaultMode", flag=YES) at nsAppShell.mm:130
    frame #77: 0x00007fffd28363db AppKit`-[NSApplication run] + 926
    frame #78: 0x0000000106a3c32c XUL`nsAppShell::Run(this=0x000000011ef29c50) at nsAppShell.mm:672
    frame #79: 0x0000000109bba9bb XUL`nsAppStartup::Run(this=0x000000012020bbf0) at nsAppStartup.cpp:287
    frame #80: 0x0000000109d12513 XUL`XREMain::XRE_mainRun(this=0x00007fff5fbff2d8) at nsAppRunner.cpp:4589
    frame #81: 0x0000000109d13ac9 XUL`XREMain::XRE_main(this=0x00007fff5fbff2d8, argc=5, argv=0x00007fff5fbff960, aConfig=0x00007fff5fbff4a8) at nsAppRunner.cpp:4772
    frame #82: 0x0000000109d1424c XUL`XRE_main(argc=5, argv=0x00007fff5fbff960, aConfig=0x00007fff5fbff4a8) at nsAppRunner.cpp:4867
    frame #83: 0x0000000109d27627 XUL`mozilla::BootstrapImpl::XRE_main(this=0x0000000100613138, argc=5, argv=0x00007fff5fbff960, aConfig=0x00007fff5fbff4a8) at Bootstrap.cpp:45
    frame #84: 0x0000000100001086 firefox`do_main(argc=5, argv=0x00007fff5fbff960, envp=0x00007fff5fbff990) at nsBrowserApp.cpp:237
    frame #85: 0x0000000100000b61 firefox`main(argc=5, argv=0x00007fff5fbff960, envp=0x00007fff5fbff990) at nsBrowserApp.cpp:310
    frame #86: 0x0000000100000a74 firefox`start + 52
Flags: needinfo?(jimb)
It's not obvious to me that it's a problem if scratchpad stuff doesn't get XrayWrappers. Users aren't expecting XrayWrappers, they're expecting that their JS will run in the context of the page.
Ok, so here: http://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCJSID.cpp#465

we're stripping off the xray and then call the js::GetObjectProto on the object directly, which seems like a bug, but with an extra IsProxy check we should be able to fix the problem. Does that sound like an acceptable fix?

What drives me crazy is that if I try the same example from a non-e10s window, and executing the script from browser context on some regular web content, I can still reproduce the issue (in the example then document has to be replaced with content.document ofc). With some debugging it seems like in that case we use permissive OpaqueXrayTraits which makes no sense to me since the script is running under system principal while the proxy object is from a web content scope. Unless  |obj instanceof Ci.nsIDOMWindow;| enters into the compartment of obj. I can't recall how instanceof work for DOM interfaces across scopes, but if that's how they work that would explain why the issue can be reproduced in this case as well.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #8)
> It's not obvious to me that it's a problem if scratchpad stuff doesn't get
> XrayWrappers. Users aren't expecting XrayWrappers, they're expecting that
> their JS will run in the context of the page.

For scratchpad that might not be a huge problem but we have the same behavior everywhere else I think. So probably it's worth fixing it.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> Ok, so here:
> http://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCJSID.cpp#465
> 
> we're stripping off the xray and then call the js::GetObjectProto on the
> object directly, which seems like a bug, but with an extra IsProxy check we
> should be able to fix the problem. Does that sound like an acceptable fix?

No, I think the real problem here is that we're bypassing the Xray layer when walking the prototype chain, which is an issue for tricky setPrototypeOf shenanigans as well. I think the correct solution here is to do the prototype walking without unwrapping, and only unwrap in the 'is this a DOM object' part.

> 
> What drives me crazy is that if I try the same example from a non-e10s
> window, and executing the script from browser context on some regular web
> content, I can still reproduce the issue (in the example then document has
> to be replaced with content.document ofc). With some debugging it seems like
> in that case we use permissive OpaqueXrayTraits which makes no sense to me
> since the script is running under system principal while the proxy object is
> from a web content scope.

That is exactly the wrapper I would expect in this situation. Permissive because it's not a SecurityWrapper, but OpaqueXrayTraits because we don't have any useful JSXrays to Proxy objects.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> No, I think the real problem here is that we're bypassing the Xray layer
> when walking the prototype chain, which is an issue for tricky
> setPrototypeOf shenanigans as well. I think the correct solution here is to
> do the prototype walking without unwrapping, and only unwrap in the 'is this
> a DOM object' part.

Something like this? The patch is fixing the issue and passes the xpconnect tests. How concerned should we be about landing this patch? Should I land the patch under a separate bug with some vague explanation?

> 
> > 
> > What drives me crazy is that if I try the same example from a non-e10s
> > window, and executing the script from browser context on some regular web
> > content, I can still reproduce the issue (in the example then document has
> > to be replaced with content.document ofc). With some debugging it seems like
> > in that case we use permissive OpaqueXrayTraits which makes no sense to me
> > since the script is running under system principal while the proxy object is
> > from a web content scope.
> 
> That is exactly the wrapper I would expect in this situation. Permissive
> because it's not a SecurityWrapper, but OpaqueXrayTraits because we don't
> have any useful JSXrays to Proxy objects.

I've got confused about the 'not SecurityWrapper' part. The code |obj instanceof Ci.nsIDOMWindow;| runs in a scope with system principal because of the browser context (so subject principal is system) and the obj is from a content scope (so object principal is content). How can we end up with a non-security wrapper then? What am I missing?
Assignee: nobody → gkrizsanits
Attachment #8885235 - Flags: review?(bobbyholley)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #12)
> I've got confused about the 'not SecurityWrapper' part. The code |obj
> instanceof Ci.nsIDOMWindow;| runs in a scope with system principal because
> of the browser context (so subject principal is system) and the obj is from
> a content scope (so object principal is content). How can we end up with a
> non-security wrapper then? What am I missing?

"Security Wrapper" in this context means "Wrapper does not subsume wrappee", which means that CheckedUnwrap is forbidden, and that we need to prevent data exfiltration. In the case of chrome->content Xrays, we want to prevent the caller from getting confused, but don't need to worry about protecting sensitive information, since the caller can see it if it wants (via wrappedJSObject).
It occurred to me that this could be a bit simpler, and it seemed easier to
write a patch rather than trying to describe in review comments. If you could
still take care of testing/landing after review I'd appreciate it.

MozReview-Commit-ID: AR2Sta2gWRk
Attachment #8885478 - Flags: review?(gkrizsanits)
Attachment #8885235 - Attachment is obsolete: true
Attachment #8885235 - Flags: review?(bobbyholley)
Comment on attachment 8885478 [details] [diff] [review]
Stop bypassing the Xray layer when walking the prototype chain. v2

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

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> "Security Wrapper" in this context means "Wrapper does not subsume wrappee",

Right, sorry, this was a long time ago :)

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #14)
> It occurred to me that this could be a bit simpler, and it seemed easier to
> write a patch rather than trying to describe in review comments. If you could
> still take care of testing/landing after review I'd appreciate it.

I appreciate it.

::: js/xpconnect/src/XPCJSID.cpp
@@ +460,4 @@
>      RootedObject obj(cx, objArg), proto(cx);
> +    while (true) {
> +        // Try the object, or the wrappee if allowed.
> +        JSObject* o = js::IsWrapper(obj) ? js::CheckedUnwrap(obj, false) : obj;

I think obj is not guaranteed to be non-null here so IsWrapper(obj) can crash. I can replace the |while (true)| with |while (obj)| and test and land it for you, but I'm still not sure how careful we should be about landing it.
Attachment #8885478 - Flags: review?(gkrizsanits) → review+
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15)

> ::: js/xpconnect/src/XPCJSID.cpp
> @@ +460,4 @@
> >      RootedObject obj(cx, objArg), proto(cx);
> > +    while (true) {
> > +        // Try the object, or the wrappee if allowed.
> > +        JSObject* o = js::IsWrapper(obj) ? js::CheckedUnwrap(obj, false) : obj;
> 
> I think obj is not guaranteed to be non-null here so IsWrapper(obj) can
> crash. I can replace the |while (true)| with |while (obj)| and test and land
> it for you, but I'm still not sure how careful we should be about landing it.

Actually the callers guarantee that we don't call this function with null obj, so it's OK as it is.
Hi Daniel, I cannot see any way this bug can be exploited badly. The worst that can happen is that when system code is trying to call instanceof on an object from another scope, it can 1) trigger a function from that other scope 2) that function can lie about the result of instanceof. This all sounds like sec moderate to me but until the bug has an official sec category I don't want to push the patch to try or land it.
Flags: needinfo?(dveditz)
Flags: needinfo?(dveditz)
Keywords: sec-moderate
I think we missed the fact that the original code's unwrap does not stop at the window proxy, so for the cross compartment window object case the old and new method differs unintentionally (the old version returns null for the proto of the window the new version identifies the DOM object and returns true for instanceof Ci.nsIDOMWindow, so at first glance seems like a bug in the old version but I'm sure there is a reason why it had to work like this...)

The test is failing here: [1] since .document is a cross origin property access.

A few lines above during the instanceof check, for some reason we don't have a security wrapper around, so we manage to unwrap everything. Based on the exception thrown at [1] I don't know why is that the case, and why there is no security wrapper around, but maybe it's a special sandbox magic that I have not followed evolving for a while.
Flags: needinfo?(bobbyholley)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19)
> I think we missed the fact that the original code's unwrap does not stop at
> the window proxy, so for the cross compartment window object case the old
> and new method differs unintentionally

I don't understand - we're also passing stopAtOuter=false to js::CheckedUnwrap in the new version.

 (the old version returns null for the
> proto of the window the new version identifies the DOM object and returns
> true for instanceof Ci.nsIDOMWindow, so at first glance seems like a bug in
> the old version but I'm sure there is a reason why it had to work like
> this...)
> 
> The test is failing here: [1] since .document is a cross origin property
> access.

The footnote is missing.

> 
> A few lines above during the instanceof check, for some reason we don't have
> a security wrapper around, so we manage to unwrap everything. Based on the
> exception thrown at [1] I don't know why is that the case, and why there is
> no security wrapper around, but maybe it's a special sandbox magic that I
> have not followed evolving for a while.
Flags: needinfo?(bobbyholley)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #20)
> I don't understand - we're also passing stopAtOuter=false to
> js::CheckedUnwrap in the new version.
> 
old:
1. unwrap (window proxy too)
2. continue
3. getProto on the unwrapped

new:
1. unwrap + check
2. getProto on the wrapped (window proxy is still on)

> The footnote is missing.

[1]: http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/devtools/server/actors/webextension.js#340
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #21)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #20)
> > I don't understand - we're also passing stopAtOuter=false to
> > js::CheckedUnwrap in the new version.
> > 
> old:
> 1. unwrap (window proxy too)
> 2. continue
> 3. getProto on the unwrapped
> 
> new:
> 1. unwrap + check
> 2. getProto on the wrapped (window proxy is still on)

Yeah the new behavior seems more correct to me.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19)
> A few lines above during the instanceof check, for some reason we don't have
> a security wrapper around, so we manage to unwrap everything.

There wouldn't be a security wrapper here, because this is chrome code, right?

I'm still unclear as to what exactly is failing and why after reading comment 19. Needs more investigation, or at least explanation.
Flags: needinfo?(gkrizsanits)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #22)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #21)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #20)
> > > I don't understand - we're also passing stopAtOuter=false to
> > > js::CheckedUnwrap in the new version.
> > > 
> > old:
> > 1. unwrap (window proxy too)
> > 2. continue
> > 3. getProto on the unwrapped
> > 
> > new:
> > 1. unwrap + check
> > 2. getProto on the wrapped (window proxy is still on)
> 
> Yeah the new behavior seems more correct to me.

For me too but I really needed the confirmation...

> 
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19)
> > A few lines above during the instanceof check, for some reason we don't have
> > a security wrapper around, so we manage to unwrap everything.
> 
> There wouldn't be a security wrapper here, because this is chrome code,
> right?
> 
> I'm still unclear as to what exactly is failing and why after reading
> comment 19. Needs more investigation, or at least explanation.

After global instanceof Ci.nsIDOMWindow returns true with the new patch
we end up calling global.document which throws (cross origin wrapper property access)

I _think_ since this is a web extension debugging test, we have a web extension sandbox with some window in the proto chain... but it's unclear to me too why we have there a security wrapper. I'm not sure if unwrapDebuggerObjectGlobal has any side effects... or it's the sandbox proxy related magic, or it's writeToGlobalPrototype related. I hoped you might know it off the top of your head, but then I will try to debug that part as well (I'm debugging it on windows so it's pretty slow, maybe I'll try to reproduce it on OSX).
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23)
> After global instanceof Ci.nsIDOMWindow returns true with the new patch
> we end up calling global.document which throws (cross origin wrapper
> property access)

Yeah, I don't understand why we have a security wrapper here, since we're definitely chrome (we're using Ci after all).

It probably has something to do with the Debugger API. The code appears to be using unsafeDereference, which I _thought_ switched from debugger proxies to regular security wrappers. But I'm fuzzy on those details, so you may want to talk to jimb or someone similar.
On a totally unrelated note, your patch does:
JSObject* o = js::IsWrapper(obj) ? js::CheckedUnwrap(obj, false) : obj;
Bug 1371865 implies that we should root |o| as well.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #25)
> On a totally unrelated note, your patch does:
> JSObject* o = js::IsWrapper(obj) ? js::CheckedUnwrap(obj, false) : obj;
> Bug 1371865 implies that we should root |o| as well.

The code in that bug was calling arbitrary virtual methods on the XPCWN within |obj|. We're not doing that here, right?
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #26)
> The code in that bug was calling arbitrary virtual methods on the XPCWN
> within |obj|. We're not doing that here, right?

We don't. I'm just curious if there is a reason not to always root for any new code just to be future proof.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #27)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #26)
> > The code in that bug was calling arbitrary virtual methods on the XPCWN
> > within |obj|. We're not doing that here, right?
> 
> We don't. I'm just curious if there is a reason not to always root for any
> new code just to be future proof.

Well, performance, and readability. And we have the static analysis to catch us. But doesn't so much matter, I'm fine with whatever.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #24)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23)
> > After global instanceof Ci.nsIDOMWindow returns true with the new patch
> > we end up calling global.document which throws (cross origin wrapper
> > property access)
> 
> Yeah, I don't understand why we have a security wrapper here, since we're
> definitely chrome (we're using Ci after all).
> 

So it's a sandbox proxy corner case when the sandbox is content and its proto is system (or just cross origin) and from a system compartment you call sb.document on it. Like:

var sb = new Cu.Sandbox("http://blah.com", {sandboxPrototype: window});
sb.document;

When SandboxProxyHandler::getPropertyDescriptor calls wrappedObject(proxy), it will find an xray on a cross origin wrapper and forwards the JS_GetPropertyDescriptorById to it, which will ofc throw.

Long story short something, somewhere creates a sandbox that does not have permission to access its own proto (which is probably a bug), and since instanceof was buggy for window objects pre-patch this problem was undetected. Now I have to figure out where and what creates this sandbox and probably fix that code, but at least it looks like nothing scary is going on here.
Flags: needinfo?(gkrizsanits)
I don't think we should change anything here, and I do believe too that the new behavior is the correct one. So I just ended up patching this code so that test stops failing. I guess I will talk to Luca about this, but it would be great if you confirmed that the comment makes sense and you agree with this decision first.
Attachment #8893935 - Flags: review?(bobbyholley)
Comment on attachment 8893935 [details] [diff] [review]
guard for XOW in the sandboxproto. v1

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

Sure makes sense to me. Get review from somebody who knows that code though.
Attachment #8893935 - Flags: review?(bobbyholley) → review+
Comment on attachment 8893935 [details] [diff] [review]
guard for XOW in the sandboxproto. v1

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

Luca, I hope my comment is descriptive enough. I'm not sure what should be the proper solution here since I'm not familiar with this code. But I'm happy to help you with working out a better solution. For the time being there is only one test that is failing because of this code and blocking me from landing an important fix, so is it OK to land this quick fix as it is and do the rest as a follow up?
Attachment #8893935 - Flags: review?(lgreco)
Comment on attachment 8893935 [details] [diff] [review]
guard for XOW in the sandboxproto. v1

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

As briefly discussed over IRC I'm ok with landing a quickfix for it first (and then in the meantime investigate where this sandbox is coming from), also if the global is being destroyed then it seems correct to me that we filter it from the ones that we can be considered "debuggable".
Attachment #8893935 - Flags: review?(lgreco) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a76755db1f3c804e8a08c1f0365db417acc5cf77
Bug 1378207 - Handle sandboxes with XOW protos in WebExtensionChildActor. r=rpl

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e06cba872427866b195b98c51bca5883f166011
Bug 1378207 - Stop bypassing the Xray layer when walking the prototype chain. r=krizsa
Looks like this needs at least a Beta approval nomination. Is this something we should consider backporting to ESR52 as well?
Flags: needinfo?(gkrizsanits)
Comment on attachment 8885478 [details] [diff] [review]
Stop bypassing the Xray layer when walking the prototype chain. v2

Approval Request Comment
[Feature/Bug causing the regression]: Not sure. It's been like this for a while.

[User impact if declined]: Xray is waived during instanceof, and instanceof for DOMWindow does not work in some special cases.

[Is this code covered by automated tests?]: Instanceof is well tested in general, but this exact scenario does not have a test. So it's unlikely that this patch breaks anything but it should be tested manually if the bug is fixed.

[Has the fix been verified in Nightly?]: Manually.

[Needs manual test from QE? If yes, steps to reproduce]: Bug description has an STR.
 
[List of other uplifts needed for the feature/fix]: Just these 2 patches.

[Is the change risky?]: I don't think so.

[Why is the change risky/not risky?]: The post patch version of the code is safer than to original in every imaginable cases, plus it's unlikely that such a generic function passes all the tests if incorrect. However as this bug exists, there can always be an untested edge case... Still, I'd sleep better with this patch than without.

[String changes made/needed]: None.
Flags: needinfo?(gkrizsanits)
Attachment #8885478 - Flags: approval-mozilla-beta?
Comment on attachment 8893935 [details] [diff] [review]
guard for XOW in the sandboxproto. v1

Approval Request Comment
[Feature/Bug causing the regression]: This code expected the broken behavior.

[User impact if declined]: A test will fail, and during add-on debugging we can hit a JS exception.

[Is this code covered by automated tests?]: Yes.

[Has the fix been verified in Nightly?]: The related test passes.

[Needs manual test from QE? If yes, steps to reproduce]: No.
 
[List of other uplifts needed for the feature/fix]: The other patch in this bug.

[Is the change risky?]: No.

[Why is the change risky/not risky?]: It's just handling an exception.

[String changes made/needed]: None.
Attachment #8893935 - Flags: approval-mozilla-beta?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #36)
> Looks like this needs at least a Beta approval nomination. Is this something
> we should consider backporting to ESR52 as well?

I'm not sure. I'd be surprised if this could be exploited... What's the requirement for backporting something to ESR?
Comment on attachment 8885478 [details] [diff] [review]
Stop bypassing the Xray layer when walking the prototype chain. v2

Fix for sec-moderate issue, let's uplift this for beta 3.
Attachment #8885478 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8893935 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/ca6af5d81565
https://hg.mozilla.org/releases/mozilla-beta/rev/7ad9e8f27a03

Doesn't sound like this is worth considering for ESR52 backport given comment 39. Feel free to set it back to affected and nominate for approval if you feel otherwise, though.
Group: dom-core-security → core-security-release
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #38)
> [Is this code covered by automated tests?]: Yes.
> 
> [Has the fix been verified in Nightly?]: The related test passes.
> 
> [Needs manual test from QE? If yes, steps to reproduce]: No.
 
Setting qe-verify- based on Gabor's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Alias: CVE-2017-7820
Whiteboard: [adv-main56+]
After re-reading the description I'm calling this sec-low
Keywords: sec-moderatesec-low
Depends on: 1430711
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.