453401 - XPCCrossOriginWrapper not unwrapped when passed to XPCOM

Status: RESOLVED WONTFIX

(Core :: XPConnect, defect, P1)

Description

[Karl Tomlinson (:karlt back Feb 10)]

STR:

* Download attachment 200522 [details] and save to a file.
* Open the file, and accept privileges.

Actual Results:

  nsCanvasRenderingContext2D::DrawWindow returns NS_ERROR_FAILURE because
  win is NULL:
  nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(aWindow);

Expected Results:

  something closer to working

#0  nsCanvasRenderingContext2D::DrawWindow (this=0x20119b0, 
    aWindow=0x22e3fb0, aX=0, aY=0, aW=300, aH=300, aBGColor=@0x7fff8eb8b8c0)
    at /home/karl/moz/dev/content/canvas/src/nsCanvasRenderingContext2D.cpp:2987
#1  0x00002b371c9cb2de in NS_InvokeByIndex_P (that=0x20119b0, methodIndex=73, 
    paramCount=6, params=0x7fff8eb8b370)
    at /home/karl/moz/dev/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_linux.cpp:208
#2  0x00002b3727573a46 in XPCWrappedNative::CallMethod (ccx=@0x7fff8eb8b800, 
    mode=XPCWrappedNative::CALL_METHOD)
    at /home/karl/moz/dev/js/src/xpconnect/src/xpcwrappednative.cpp:2393
#3  0x00002b3727582ace in XPC_WN_CallMethod (cx=0x10f3c10, obj=0xd60cc0, 
    argc=6, argv=0x1c4cc18, vp=0x7fff8eb8b9b8)
    at /home/karl/moz/dev/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1473
#4  0x00002b371c4296f8 in js_Invoke (cx=0x10f3c10, argc=6, vp=0x1c4cc08, 
    flags=2) at /home/karl/moz/dev/js/src/jsinterp.cpp:1308
(gdb) p DumpJSStack ()
0 [native frame]
1 loaded() ["file:///home/karl/tmp/200522.html":11]
    w = [object Window @ 0x1776780 (native @ 0x1ca2290)]
    cx = [object CanvasRenderingContext2D @ 0x236ff00 (native @ 0x20119b0)]
    this = [object Window @ 0x116e130 (native @ 0x10fa450)]
2 <TOP LEVEL> ["file:///home/karl/tmp/200522.html":1]
    this = [object Window @ 0x116e130 (native @ 0x10fa450)]
(gdb) p DumpJSObject ( aWindow->mOuter->mJSObj)
Debugging reminders...
  class:  (JSClass*)(obj->fslots[2]-1)
  parent: (JSObject*)(obj->fslots[1])
  proto:  (JSObject*)(obj->fslots[0])

0xd65200 'native' <XPCCrossOriginWrapper>
  parent: 0xbd0280 'native' <Window>
    parent: null
    proto: 0xbd66c0 'native' <XPC_WN_ModsAllowed_NoCall_Proto_JSClass>
      parent: 0x10f2380 'native' <Window>
        parent: null
        proto: 0xbd66c0 'native' <XPC_WN_ModsAllowed_NoCall_Proto_JSClass> (SEE ABOVE)
      proto: 0xbead80 'native' <Global Scope Polluter>
        parent: 0xbd0280 'native' <Window> (SEE ABOVE)
        proto: 0xbd0340 'native' <Object>
          parent: 0xbd0280 'native' <Window> (SEE ABOVE)
          proto: null
  proto: null
$15 = void
(gdb) p *(JSClass*)(aWindow->mOuter->mJSObj->fslots[2] & ~3)
$16 = {name = 0x2b37275e159c "XPCCrossOriginWrapper", flags = 66308, 
  addProperty = 0x2b372758f3c4 <XPC_XOW_AddProperty>, 
  delProperty = 0x2b372758f26e <XPC_XOW_DelProperty>, 
  getProperty = 0x2b372758ef64 <XPC_XOW_GetProperty>, 
  setProperty = 0x2b372758ef2e <XPC_XOW_SetProperty>, 
  enumerate = 0x2b372758e7e2 <XPC_XOW_Enumerate>, 
  resolve = 0x2b372758e39e <XPC_XOW_NewResolve>, 
  convert = 0x2b372758e144 <XPC_XOW_Convert>, 
  finalize = 0x2b372758c9f8 <XPC_XOW_Finalize>, getObjectOps = 0, 
  checkAccess = 0x2b372758c97c <XPC_XOW_CheckAccess>, 
  call = 0x2b372758dc9c <XPC_XOW_Call>, 
  construct = 0x2b372758da78 <XPC_XOW_Construct>, xdrObject = 0, 
  hasInstance = 0x2b372758d8b2 <XPC_XOW_HasInstance>, mark = 0, 
  reserveSlots = 0}

<bz> when you get a window cross-origin like that, you don't get back the
     XPCWrappedNative JSObject for it
<bz> instead you get something called an XPCCrossOriginWrapper
<bz> which is fine
<bz> but then when passing to an XPCOM interface, we should (imo) be
     unwrapping to get the underlying C++ object
<bz> instead of taking the XPCCrossOriginWrapper JSObject and wrapping it
     inside an nsXPCWrappedJS as we do here

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like we get into XPCWrappedNative::GetWrappedNativeOfJSObject but Unwrap returns null (because we only have UniversalBrowserRead), so we can't get the underlying C++ native.

I'd think we would want to unwrap unconditionally in this one place, without a security check, no?  Or does that open up an attack vector of some sort?

Comment 2

[Johnny Stenback (:jst)]

Not blocking the release on this after all. Marking wanted, P1. Blake, any thoughts on this?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

I haven't tested the patch, but this should do the trick.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Er, the comment at the top of the patch isn't correct.

Comment 6

[Boris Zbarsky [:bzbarsky]]

That fixes the UniversalXPConnect bug, not this bug, unless I'm missing something...

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Why does that testcase use UniversalBrowserRead instead of UniversalXPConnect? I could add support for UniversalBrowserRead/Write but IMO that's complicating things too much. IIRC dveditz and jst were seriously talking about dropping support for everything but UniversalXPConnect.

Comment 8

[Boris Zbarsky [:bzbarsky]]

UniversalBrowserRead is all canvas used to require in Fx2.

Comment 9

[Daniel Veditz [:dveditz]]

(In reply to comment #7)
> IIRC dveditz and jst were seriously talking about dropping
> support for everything but UniversalXPConnect.

No, I want to drop everything full-stop. If you want privileges then convince your users to install an addon or plugin. Most users understand the concept and potential dangers of installing things; they don't understand capabilities and what dangerous things can be done with each.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #9)
> (In reply to comment #7)
> > IIRC dveditz and jst were seriously talking about dropping
> > support for everything but UniversalXPConnect.
> 
> No, I want to drop everything full-stop. If you want privileges then convince
> your users to install an addon or plugin. Most users understand the concept and
> potential dangers of installing things; they don't understand capabilities and
> what dangerous things can be done with each.

We already require users to set a hidden pref before script can request privs; I don't think we should remove the ability to do something similar, if nothing else but for testing.  Noone on the web is using UniversalBrowserRead/UniversalXPConnect with canvas for anything real; it's all being done for testing, or drawWindow is being used from addons that have privs anyway... so the canvas example isn't a good one.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Ugh

This should fix it. At the principal level, UniversalXPConnect doesn't imply the weaker privileges, so we have to check for it specially.

Comment 12

[Boris Zbarsky [:bzbarsky]]

That doesn't seem right to me.  The BrowserRead/BrowserWrite stuff is about data, not properties...

Honestly, if we're just using this for tests I'm happy to require UniversalXPConnect to unwrap.  That's assuming that we don't want to unconditionally unwrap on passing to C++ code.  And it that's the case, we should have a nice long comment explaining why.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #12)
> That doesn't seem right to me.  The BrowserRead/BrowserWrite stuff is about
> data, not properties...

I thought the patch preserved this: if you can read all data in the browser, that means you can *read* but not *write* through a cross-origin XOW, and vice versa (though that seems not at all useful). Perhaps my understanding of BrowserRead/BrowserWrite is flawed...

> That's assuming that we don't want to
> unconditionally unwrap on passing to C++ code.  And it that's the case, we
> should have a nice long comment explaining why.

I'd rather do this. The idea is that if XOWs refuse to unwrap, we can get rid of all ad-hoc security checks on parameters in C++ since you simply would not be able to pass a cross origin node into e.g. a treewalker.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Based on comment 12, I'm going to mark this WONTFIX. In order to work around this, request UniversalXPConnect privileges instead of UniversalBrowserRead.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

bug 396851 tracks the patch to make UniversalXPConnect work.

Comment 16

[Daniel Veditz [:dveditz]]

(In reply to comment #10)
> We already require users to set a hidden pref before script can request privs;

A signed page can request these privs from any unsuspecting user, no opt-in pref setting required. We know such apps are still in use because we broke some of them when we tightened up jar: principals in 2.0.0.15