163950 - XMLHttpRequest.open gives access denied when document.domain is explicitly set in the calling page.

Status: RESOLVED FIXED

(Core :: XML, defect, P2)

Description

[Enrique Martin del Campo]

User-Agent:       Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.1) Gecko/20020724 Netscape/7.0

If I'm hitting a webserver at http://a.foo.bar.com and I set document.domain 
= 'foo.bar.com' within my page, I then cannot call XMLHttpRequest.open
('POST','/dummy.html',true). I get an access denied error.

Given that the url I am trying to request is a relative url, this seems like 
the browser should know that I'm not going to some other domain. It works fine 
with IE, as well as with Netscape 6.2.

Reproducible: Always

Steps to Reproduce:
1. See the bug details.
2.
3.




Dig Bug.

Comment 1

[Enrique Martin del Campo]

I was wrong when I said this worked with Netscape 6.2 I was mislead because it 
doesn't actually throw an exception, but the request does not go to the server. 
I'm using a relative url, so it seems, after playing around with it, that the 
browser is using the domain as the server name, so in effect resolving the 
server location incorrectly.

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Testcase in URL sets the document.domain to a shorter format, but then
XMLHttpRequest.open() fails because of security error. This is in
nsScriptSecurityManager::CheckSameOriginDOMProp() where the code notes that
subject has changed domain but object has not. This seems weird to me in this
case since we haven't yet even loaded the object, so there is no way it could
have set the domain . With XMLHttpRequest and document.load() it should not
check this, I think. Mitch, can you help me out here?

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I will need some help from Mitch to fix the CAPS side of things, but I also
realized I need to change the way XMLHttpRequest gets the base URL for the
connection. Currently it gets this from the principal of the page, but that is
affected by document.domain (so currently even if document.domain didn't fail
because of security any relative URLs would probably fail to load). I think
XMLHttpRequest should get the base URL of the document instead.

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: (Part of?) xmlextras side of fix

Comment 5

[Mitchell Stoltz (not reading bugmail)]

->mstoltz

Comment 6

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 102085 [details] [diff] [review]
(Part of?) xmlextras side of fix

r=mstoltz on this patch. It solves part of the problem, but not all.

Comment 7

[Mitchell Stoltz (not reading bugmail)]

Clearing milestone for now.

Comment 8

[Mitchell Stoltz (not reading bugmail)]

The obvious way to fix this is to remove the requirement that both source and
target set document.domain, at least for this case. However, that will reopen
the DNS spoofing bug that was the reason we added that restriction. I don't see
any alternate solution for this bug, so we may have to think of alternate
solutions for the DNS problem.

Comment 9

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch

This patch causes the security manager to ignore the requirement that both
source and target documents have set document.domain. It only applies to
XMLHTTPRequest.open, SOAP, and XMLDocument.load, the 3 places that call
CheckConnect. This patch will allow, for example, a script on a page at a.b.com
that has set document.domain to b.com to open an XMLRequest to
http://b.com/file.xml. It will *not* allow the same page to open a request to
http://a.b.com/file.xml. Is this sufficient?

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I think it's ok for now. If more is needed later, we can open new bugs.

Comment 11

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 124004 [details] [diff] [review]
Patch

OK then, requesting reviews.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 124004 [details] [diff] [review]
Patch

r/sr=jst

Comment 13

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 124004 [details] [diff] [review]
Patch

Seeking drivers approval for 1.4. This patch slightly relaxes the security
policy for XMLHTTPRequest to fix a regression in some Web apps. The risk is
low.

Comment 14

[Mitchell Stoltz (not reading bugmail)]

Checked in on trunk, will keep the bug open until we get it into 1.4

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 124004 [details] [diff] [review]
Patch

>@@ -724,7 +725,8 @@
>                     NS_ERROR("CheckPropertyAccessImpl called without a target object or URL");
>                     return NS_ERROR_FAILURE;
>                 }
>-                rv = CheckSameOriginDOMProp(subjectPrincipal, objectPrincipal, aAction);
>+                rv = CheckSameOriginDOMProp(subjectPrincipal, objectPrincipal,
>+                                            aAction, (PRBool)aTargetURI);
>                 break;
>             }
>         default:

Isn't aTargetURI a pointer?  You can't just cast to (PRBool) (which is to say,
to (int)), and get a valid boolean.  Best to use aTargetURI != nsnull here.

>@@ -849,7 +851,8 @@
> nsresult
> nsScriptSecurityManager::CheckSameOriginDOMProp(nsIPrincipal* aSubject,
>                                                 nsIPrincipal* aObject,
>-                                                PRUint32 aAction)
>+                                                PRUint32 aAction,
>+                                                PRBool aIsCheckConnect)
> {
>     nsresult rv;
>     /*
>@@ -867,6 +870,14 @@
>         // explicitly setting document.domain then the other must also have
>         // done so in order to be considered the same origin. This prevents
>         // DNS spoofing based on document.domain (154930)
>+
>+        // But this restriction does not apply to CheckConnect calls, since
>+        // that's called for data-only load checks like XMLHTTPRequest, where
>+        // the target document has not yet loaded and can't have set its domain
>+        // (bug 163950)
>+        if (aIsCheckConnect)
>+            return NS_OK;

Are we overloading a security manager interface inappropriately, to need this
flag?

/be

Comment 16

[Mitchell Stoltz (not reading bugmail)]

Brendan,
   On your first point, will do. On your second point, I don't believe so.
CheckSameOriginDOMProp is an internal function, not part of the interface.

Comment 17

[Daniel Veditz [:dveditz]]

If I understand comment #9 correctly this does not fix the original complaint
that we don't handle relative URLs the way IE does. (It's not clear, though,
whether the IE tested was the latest version with the same document.domain
spoofing fix we applied.)

Or does Mitch's patch get combined with Heikki's in order to get the proper
relative document base for relative URLs? I'd hate to partially reopen the
document.domain spoof and not actually solve the relative URL bug that's
breaking things.

Comment 18

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The XMLExtras part was checked in long time ago as part of another bug.

However, the testcase in the URL still does not seem to work after Mitch's
patch. I thought it would...

Comment 19

[Mitchell Stoltz (not reading bugmail)]

My testcase works: http://warp.mcom.com/u/mstoltz/bugs/163950.html, which uses a
relative URL. I'm not sure what's wrong with your testcase, but I think it's
some other problem since I don't see a security error.

Comment 20

[Mitchell Stoltz (not reading bugmail)]

Correction: the testcase I mentioned is the same one. For some reason it works
from warp but not from green.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 124004 [details] [diff] [review]
Patch

Please add fixed1.4 after this is checked into the 1.4 branch

Comment 22

[Samir Gehani]

a=adt to land on the 1.4 branch.

Comment 23

[Mitchell Stoltz (not reading bugmail)]

Fixed on the branch.

Comment 24

[Philip K. Warren]

This patch was checked in without addressing the first part of comment 15. This
causes building 64-bit Mozilla to fail (Bug 228095).