Closed Bug 966609 Opened 6 years ago Closed 6 years ago

XMLHttpRequest.responseXML.childNodes fires permission denied exception

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla31

People

(Reporter: jori.seidel, Assigned: gkrizsanits)

Details

Attachments

(1 file, 1 obsolete file)

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'.
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)
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
Gabor, can you take a look at this?
Flags: needinfo?(gkrizsanits)
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

...
Flags: needinfo?(gkrizsanits) → needinfo?(bobbyholley)
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.
Flags: needinfo?(bobbyholley)
(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.
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.
Attachment #8388412 - Flags: feedback?(bobbyholley)
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.
Attachment #8388412 - Flags: feedback?(bobbyholley)
(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...
(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?
(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 :)
(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?
(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...
(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.
Alright, now it makes sense, thanks for the explanation.
Attachment #8388412 - Attachment is obsolete: true
Attachment #8393487 - Flags: review?(bobbyholley)
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.
Attachment #8393487 - Flags: review?(continuation)
Attachment #8393487 - Flags: review?(bobbyholley)
Attachment #8393487 - Flags: review+
Comment on attachment 8393487 [details] [diff] [review]
nsWrapperCache for SandboxPrivate. v2

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

CC stuff looks good to me.
Attachment #8393487 - Flags: review?(continuation) → review+
(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;
https://hg.mozilla.org/mozilla-central/rev/f4e975c7d8d3
Assignee: nobody → gkrizsanits
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.