966609 - XMLHttpRequest.responseXML.childNodes fires permission denied exception

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Jori Seidel]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.102 Safari/537.36

Steps to reproduce:

Make an ajax request that returns xml (<test>text</test>) and then try to read the xml content using responseXML.documentElement:

    xmlhttp=new XMLHttpRequest();
    xmlhttp.onreadystatechange=function() {
        if (xmlhttp.readyState==4 && xmlhttp.status==200) {
            console.log('finished');
            console.log(xmlhttp.responseXML.documentElement.childNodes[0].nodeValue);
            console.log('reported xml');
        }
    }
    xmlhttp.open("GET","http://messagebeam.tagpulse.nl/test/test2.php",true);
    xmlhttp.send(); 


Creating a new XML document using the responseText property does work, i.e.

var parser = new DOMParser();
var xmldoc = parser.parseFromString(xmlhttp.responseText, "text/xml");
console.log(xmlDoc.documentElement.childNodes[0].nodeValue);




Actual results:

Error: Permission denied to access property 'documentElement'


Expected results:

Display 'text'.

Comment 1

[Jori Seidel]

To clarify: this happens in a content script which has added a permission for the domain. When I originally came up with this problem I made an SO post, which led to this bug report. You can check that here: http://stackoverflow.com/questions/21451788/xmlhttprequest-responsexml-fires-permission-denied-exception (see answer and comments leading to this report)

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Looks like it throws on any access to any props/methods of responseXML -- 'toJSON' is just what we see upon serialization from the console logging. 'valueOf' or 'documentElement' also throws. Looks like a sandbox content issue

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Gabor, can you take a look at this?

Comment 4

[Gabor Krizsanits (INACTIVE)]

Uhh... I investigated this and got a bit sad. It turns out that responseXML does not work for XHRs created from a sandbox (with the wantGlobalProperty installed custom XHR ctor) at all. So I checked the test I wrote back then for the documents should know their globals patch and the test is wrong (as in not complete). Because it investigates the responseXML document only from chrome scope in the test instead of the sandbox scope. I knew this worked some time because I did a lot of testing by hand, but since the test did not enforce the right behavior, it's got broken :(

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/tests/unit/test_allowedDomainsXHR.js?force=1#30

And as it turns out from the sandbox scope wrapping the responseXML document fails in a weird way
get_responseXML tries to wrap the parent of the response document into the scope of the sandbox. So
basically it tries to wrap the sandbox into the scope of the sandbox itself and it fails in
CanCreateWrapper... I copied the stack below. Right know I'm a bit confused why this happens,
Bobby do you have any clue what might be going on here? Note: I got the same result without using expanded principal as well.

Here is a simple scratchpad test:
var sb = new Cu.Sandbox("http://messagebeam.tagpulse.nl",
                        {wantGlobalProperties:["XMLHttpRequest"]});

sb.cl = console.log;

var src = function(){
    xmlhttp=new XMLHttpRequest();
    xmlhttp.onreadystatechange=function() {
        if (xmlhttp.readyState==4 && xmlhttp.status==200) {
            cl('finished');
            xmlhttp.responseXML.documentElement;
            cl('reported xml');
        }
    }
    xmlhttp.open("GET","http://messagebeam.tagpulse.nl/test/test2.php",true);
    xmlhttp.send();
}.toSource();

Cu.evalInSandbox(src + "()",sb);

and the stack:

#0  nsScriptSecurityManager::CanCreateWrapper(this = 0x74e8a0, cx = 0x752140, aIID = , aObj = 0x34a5ad8, aClassInfo = 0x0) at /home/gabor/development/mozilla-central2/caps/src/nsScriptSecurityManager.cpp:1175
#1  XPCWrappedNative::InitTearOff(this = 0x34c7080, aTearOff = 0x34c70c0, aInterface = 0x90f640, needJSObject = false) at /home/gabor/development/mozilla-central2/js/xpconnect/src/XPCWrappedNative.cpp:1551
#2  XPCWrappedNative::FindTearOff(this = 0x34c7080, aInterface = 0x90f640, needJSObject = false, pError = 0x7fffffff89c0) at /home/gabor/development/mozilla-central2/js/xpconnect/src/XPCWrappedNative.cpp:1432
#3  XPCWrappedNative::GetNewOrUsed(helper = , Scope = 0x34a2f20, Interface = 0x90f640, resultWrapper = 0x7fffffff8c00) at /home/gabor/development/mozilla-central2/js/xpconnect/src/XPCWrappedNative.cpp:464
#4  XPCConvert::NativeInterface2JSObject(d = JSVAL_NULL, dest = 0x0, aHelper = , iid = 0x7ffff2883140, Interface = 0x0, allowNativeWrapper = false, pErr = 0x7fffffff8dbc) at /home/gabor/development/mozilla-central2/js/xpconnect/src/XPCConvert.cpp:881
#5  mozilla::dom::NativeInterface2JSObjectAndThrowIfFailed(aCx = 0x752140, aScope = (JSObject * const) 0x7fff988a8780 [object XMLHttpRequest], aRetval = JSVAL_NULL, aHelper = , aIID = 0x0, aAllowNativeWrapper = false) at /home/gabor/development/mozilla-central2/dom/bindings/BindingUtils.cpp:724
#6  mozilla::dom::XPCOMObjectToJsval(cx = 0x752140, scope = (JSObject * const) 0x7fff988a8780 [object XMLHttpRequest], helper = , iid = 0x0, allowNativeWrapper = false, rval = JSVAL_NULL) at /home/gabor/development/mozilla-central2/dom/bindings/BindingUtils.cpp:776
#7  mozilla::dom::WrapNativeISupportsParent<nsIGlobalObject>(cx = 0x752140, scope = (JSObject * const) 0x7fff988a8780 [object XMLHttpRequest], p = 0x34a5ad0, cache = 0x0) at ../../dist/include/mozilla/dom/BindingUtils.h:1165
#8  mozilla::dom::WrapNativeParentHelper<nsIGlobalObject, false>::Wrap(cx = 0x752140, scope = (JSObject * const) 0x7fff988a8780 [object XMLHttpRequest], parent = 0x34a5ad0, cache = 0x0) at ../../dist/include/mozilla/dom/BindingUtils.h:1235
#9  mozilla::dom::WrapNativeParent<nsIGlobalObject>(cx = 0x752140, scope = (JSObject * const) 0x7fff988a8780 [object XMLHttpRequest], p = 0x34a5ad0, cache = 0x0) at ../../dist/include/mozilla/dom/BindingUtils.h:1249
#10  mozilla::dom::WrapNativeParent<nsIGlobalObject*>(cx = 0x752140, scope = (JSObject * const) 0x7fff988a8780 [object XMLHttpRequest], p = @0x7fffffff90a0: 0x34a5ad0) at ../../dist/include/mozilla/dom/BindingUtils.h:1258
#11  mozilla::dom::XMLDocumentBinding::Wrap(aCx = 0x752140, aScope = (JSObject * const) 0x7fff988a8780 [object XMLHttpRequest], aObject = 0x34b5c30, aCache = 0x34b5c38) at /home/gabor/development/mozilla-central2/obj-x86_64-unknown-linux-gnu/dom/bindings/XMLDocumentBinding.cpp:358
#12  mozilla::dom::XMLDocumentBinding::Wrap<mozilla::dom::XMLDocument>(aCx = 0x752140, aScope = (JSObject * const) 0x7fff988a8780 [object XMLHttpRequest], aObject = 0x34b5c30) at ../../../../dist/include/mozilla/dom/XMLDocumentBinding.h:49
#13  mozilla::dom::XMLDocument::WrapNode(this = 0x34b5c30, aCx = 0x752140, aScope = (JSObject * const) 0x7fff988a8780 [object XMLHttpRequest]) at /home/gabor/development/mozilla-central2/content/xml/document/src/XMLDocument.cpp:607
#14  nsINode::WrapObject(this = 0x34b5c30, aCx = 0x752140, aScope = (JSObject * const) 0x7fff988a8780 [object XMLHttpRequest]) at /home/gabor/development/mozilla-central2/content/base/src/nsINode.cpp:2597
#15  nsIDocument::WrapObject(this = 0x34b5c30, aCx = 0x752140, aScope = (JSObject * const) 0x7fff988a8780 [object XMLHttpRequest]) at /home/gabor/development/mozilla-central2/content/base/src/nsDocument.cpp:12053
#16  mozilla::dom::WrapNewBindingObject<nsIDocument>(cx = 0x752140, scope = (JSObject * const) 0x7fff988a8780 [object XMLHttpRequest], value = 0x34b5c30, rval = $jsval((JSObject *) 0x7fff45618880 [object Function "responseXML"])) at ../../dist/include/mozilla/dom/BindingUtils.h:704
#17  mozilla::dom::XMLHttpRequestBinding::get_responseXML(cx = 0x752140, obj = (JSObject * const) 0x7fff988a8780 [object XMLHttpRequest], self = 0x34a8aa0, args = $jsval((JSObject *) 0x7fff45618880 [object Function "responseXML"])) at /home/gabor/development/mozilla-central2/obj-x86_64-unknown-linux-gnu/dom/bindings/XMLHttpRequestBinding.cpp:1032

...

Comment 5

[Bobby Holley (:bholley)]

Hm yeah. Trying to wrap a SandboxPrivate with NativeInterface2JSObject isn't going to work very well.

A simple fix would be to QI the argument of NativeInterface2JSObject to nsIGlobalObject and, if that succeeds, return the result of ->GetGlobalJSObject(). But that adds yet another QI on an already-slow path, so I think we should avoid it.

A better solution would be to make SandboxPrivate implement nsWrapperCache. That would simplify a lot of the weirdness. Do you know how to do that, gabor? I can explain more if need be.

Comment 6

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #5)
> A better solution would be to make SandboxPrivate implement nsWrapperCache.
> That would simplify a lot of the weirdness. Do you know how to do that,
> gabor? I can explain more if need be.

Thanks, I'll try that. I think I used to know... I'll try this approach and will get back to you on it if it turns out to be hard.

Comment 7

[Gabor Krizsanits (INACTIVE)]

Attachment: nsWrapperCache for SandboxPrivate. v1

I'm not sure about this at all... The SetDOMBindings is particularly bad, I guess we should invent a new flag for this, right? But it does fix the problem at least... Let me know if I do anything else wrong here.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8388412 [details] [diff] [review]
nsWrapperCache for SandboxPrivate. v1

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

::: js/xpconnect/public/SandboxPrivate.h
@@ +25,2 @@
>      {
> +        SetIsDOMBinding();

Yeah this is wrong. I assume you're running afoul of the nastiness in NativeInterface2JSObject? I think the logic there is probably just incorrect. After invoking GetWrapper(), I think we should do the cache->WrapObject() part only if (!flat && cache->IsDOMBinding()), and add a comment reminding the reader that SandboxPrivate stuff goes through here too.

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #8)
> Yeah this is wrong. I assume you're running afoul of the nastiness in
> NativeInterface2JSObject? I think the logic there is probably just
> incorrect. After invoking GetWrapper(), I think we should do the
> cache->WrapObject() part only if (!flat && cache->IsDOMBinding()), and add a
> comment reminding the reader that SandboxPrivate stuff goes through here too.

Yes, exactly. To make it clear we're talking about this code (http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#825)

But I'm not sure I understand what you're suggesting. You mean merging the two if's? But then JS_WrapObject would be called on flat wrappers which seems to be bad. So what we need is some way to distinguish sandbox from the flat wrapped natives, I don't see any other way than a flag or a QI... maybe JSClass... but the flag is the cheapest. Maybe I'm missing something you see here...

Comment 10

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> But then JS_WrapObject would be called on flat wrappers which
> seems to be bad.

Why is that bad?

> So what we need is some way to distinguish sandbox from the
> flat wrapped natives

I don't see how the distinction is really relevant. If the native object has the reflector in the cache, it doesn't matter whether it's a WN or not.

Here's what I was imagining.

    nsWrapperCache *cache = aHelper.GetWrapperCache();

    RootedObject flat(cx, cache ? cache->GetWrapper() : nullptr);
    if (!flat && cache && cache->IsDOMBinding()) {
        RootedObject global(cx, xpcscope->GetGlobalJSObject());
        flat = cache->WrapObject(cx, global);
        if (!flat)
            return false;
    }
    if (flat) {
        if (allowNativeWrapper && !JS_WrapObject(cx, &flat))
            return false;
        return CreateHolderIfNeeded(flat, d, dest);
    }

Or something. Thoughts?

Comment 11

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #10)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> > But then JS_WrapObject would be called on flat wrappers which
> > seems to be bad.
> 
> Why is that bad?

I don't know a lot about this code, so I was just a bit scared to make a big change on it like that. I just had this memory fragment that slim wrappers have to be morphed when reached cross compartment, and it seemed like this change might break that by returning too early. Let's see what does the try server think about it :)

Comment 12

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=d25fc3a38247

hmm... one test is failing, this one: http://mxr.mozilla.org/mozilla-central/source/content/test/unit/test_range.js#363

Comment 13

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11)
> I don't know a lot about this code, so I was just a bit scared to make a big
> change on it like that. I just had this memory fragment that slim wrappers
> have to be morphed when reached cross compartment

slim wrappers are long gone :-)

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #12)
> hmm... one test is failing, this one:
> http://mxr.mozilla.org/mozilla-central/source/content/test/unit/test_range.
> js#363

Hm. Does changing that test from |instanceof TypeError| to |constructor.name == "TypeError"| fix it?

Comment 14

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #13)
> 
> slim wrappers are long gone :-)
> 

So we can get rid of these comments as well? http://mxr.mozilla.org/mozilla-central/search?string=slim%2Bwrapper

> 
> Hm. Does changing that test from |instanceof TypeError| to |constructor.name
> == "TypeError"| fix it?

yupp that is fixing it. But I don't really understand what is going on here tbh...

Comment 15

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> So we can get rid of these comments as well?
> http://mxr.mozilla.org/mozilla-central/search?string=slim%2Bwrapper

Yep.

> > Hm. Does changing that test from |instanceof TypeError| to |constructor.name
> > == "TypeError"| fix it?
> 
> yupp that is fixing it. But I don't really understand what is going on here
> tbh...

Per ES5, |x instanceof y| returns true if and only if |y| is on the prototype chain of |x|. But this is problematic when you involve multiple globals, because |new iwin.Object() instanceof Object| evaluates to false. WebIDL overrides this behavior for DOM objects, but at present, no such sugar exists for pure ES objects. So you have to use the constructor.name trick.

Effectively, this means that a TypeError ends up getting thrown in the scope of a different global than it was before. And that's probably fine.

Comment 16

[Gabor Krizsanits (INACTIVE)]

Attachment: nsWrapperCache for SandboxPrivate. v2

Alright, now it makes sense, thanks for the explanation.

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8393487 [details] [diff] [review]
nsWrapperCache for SandboxPrivate. v2

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

This looks right, but let's have andrew check this from a CC perspective.

::: js/xpconnect/tests/unit/test_allowedDomainsXHR.js
@@ +75,2 @@
>    sb.do_check_eq = do_check_eq;
> +  sb.httpbody = httpbody;

I don't follow what this is about. Is this testing something? If so, it needs comments.

Comment 18

[Andrew McCreight [:mccr8]]

Comment on attachment 8393487 [details] [diff] [review]
nsWrapperCache for SandboxPrivate. v2

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

CC stuff looks good to me.

Comment 19

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #17)
> > +  sb.httpbody = httpbody;
> 
> I don't follow what this is about. Is this testing something? If so, it
> needs comments.

Right, sorry about that, I've added:

  // We want to execute checkResults from the scope of the sandbox as well to
  // make sure that there are no permission errors related to nsEP. For that
  // we need to clone the function into the sandbox and make a few things
  // available for it.
  cu.evalInSandbox('var checkResults = ' + checkResults.toSource(), sb);
  sb.do_check_eq = do_check_eq;
  sb.httpbody = httpbody;

Comment 20

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=d3a7fd7a545c

Comment 21

[Gabor Krizsanits (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e975c7d8d3

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f4e975c7d8d3