Bug 15100 – XMLHttpRequest::urlMatchesDocumentDomain raises error if port information does not match exactly

• Component: New Bugs
• Status: RESOLVED FIXED

[Christian Mittendorf]

The framework that we are using creates Ajax links where it adds the port to an URL. As an example, a site runs under "https://www.foo.com" and the automatically generated JavaScript link looks like "https://www.fool.com:443/Bar/app?abc=123". The current WebCore (Rev 25269) XMLHttpRequest does refuse to make this request with an "Access denied" error because the site url does not include the explicit port information ("documentURL.port() == url.port()" is false). In my example the documentURL.port() would be 0 and url.port() would be 443. Although using https without any specific port would result in using port 443 anyway. IE and Firefox do allow these links and perform the Ajax request without an error.

I would suggest to either remove the test that the port must match in both urls or to add more logic to the test method "bool XMLHttpRequest::urlMatchesDocumentDomain(const KURL& url) const" in /WebKit/WebCore/xml/XMLHttpRequest.cpp.

[David Kilzer (:ddkilzer)]

<rdar://problem/5443521>

[Alexey Proskuryakov]

Did this work in 10.4.x Safari/WebKit? If it did, this problem would be a P1, although I'm not quite sure what the correct fix would be. IIRC, the protocol<->port relationship is just a default that can be overridden via DNS, so it's not possible to know that https corresponds to port 443 without making a network connection.

[Christian Mittendorf]

It's already a problem with 10.4.x, although Safari won't display a pop-up showing an error message (Access denied). It simply doesn't work. The error popup comes with Safari 3 Beta.

[Christian Mittendorf]

(In reply to comment #2)
> IIRC, the protocol<->port relationship is just a default that can be overridden via DNS,
> so it's not possible to know that https corresponds to port 443 without making
> a network connection.

The DNS doesn't have anything to do with the protocol - port, the port being used is just a 
convention (see RFC 2616).

[Christian Mittendorf]

There is also an interesting chapter entitled "3.2.3 URI Comparison" (http://www.faqs.org/rfcs/rfc2616.html) in RFC 2516 (HTTP/1.1):

"A port that is empty or not given is equivalent to the default port for that URI-reference;"

Even though W3C does not define how "constitutes as non same-origin" (http://www.w3.org/TR/XMLHttpRequest/) is defined, refering to RFC 2516 does in my eyes make sense.

[Christian Mittendorf]

Created attachment 16149 [details]
Added use of default http/https port for comparison if no port was specified in the url.

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> Created an attachment (id=16149) [edit]
> Added use of default http/https port for comparison if no port was specified in
> the url.

Please set the "review?" flag if you would like this patch reviewed.  Thanks!

http://webkit.org/coding/contributing.html

[Darin Adler]

Comment on attachment 16149 [details]
Added use of default http/https port for comparison if no port was specified in the url.

Is this change really appropriate? Do other browsers consider this when comparing ports for security reasons? I'm not sure the actual port matters. All that matters is the security rule.

The formatting is wrong (spaces before the url) and performance-wise we don't want to call lower multiple times on the same string.

[Alexey Proskuryakov]

(In reply to comment #4)
> The DNS doesn't have anything to do with the protocol - port, the port being
> used is just a convention (see RFC 2616).

Unfortunately, it's not that simple: per RFC 2782, this information is stored in SRV records. On the other hand, it's not clear if this RFC is ever going to be implemented.

[Boris Zbarsky]

(In reply to comment #8)
> Is this change really appropriate? Do other browsers consider this when
> comparing ports for security reasons?

The relevant code in Gecko is <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/caps/src/nsScriptSecurityManager.cpp&rev=1.321&mark=326-328#319>.  This is the code that is used for all same-origin checks (including cross-origin script access).

[Maciej Stachowiak]

Comment on attachment 16149 [details]
Added use of default http/https port for comparison if no port was specified in the url.

I think we should match Mozilla and IE on this. Some technicalities about the current patch:

1) All these steps are done twice:

+    int document_port = documentURL.port();
+    
+    if (document_port == 0) {
+        if(documentURL.protocol().lower() == "http")
+            document_port = 80;
+        if(documentURL.protocol().lower() == "https")
+            document_port = 443;
+    }

It would be nice to factor this work out into a common function, especially so we can reuse it anywhere that an XSS same origin check is needed. Also, I'd suggest adding the default port for "ftp". And finally, we probably need to eventually add a way to ask the network layer for the default ports for various protocols.

r- for these technicalities. I will gladly r+ a version that improves these things. I also asked Sam Weinig to look at this, to see if we can apply the same fix to scripting checks.

[Sam Weinig]

A few comments on this itteration,

Please capitalize the first word in the comment.
+    // make sure that the default http / https port 80 / 443 is used for comparison

Our style dictates that these variables should be named documentPort and urlPort.
+    int document_port = documentURL.port();
+    int url_port = url.port();

Please put a space after the if.  This goes for a few other places as well.  In addition, a slightly better way to do this comparison is to use the equalIgnoringCase(stringA, stringB) function.
+        if(documentURL.protocol().lower() == "http")

If we make this change, we definitely need to make it in the isSafeScript check as well.

[Travis Vachon]

Created attachment 21617 [details]
Patch for better compliance with RFC 2616 Section 3.2.3

Take protocol default port into account when comparing URL ports.

[Travis Vachon]

I've added a new patch, based on the original, rejected patch, to fix this bug. I'm not particularly fluent in C++, so please excuse any obvious mistakes.

I'd also just like to add to this conversation by pointing out a real world bug caused in part by the current (unpatched) behavior:

https://bugzilla.osafoundation.org/show_bug.cgi?id=12118

Bugs like this can be fairly subtle and difficult to track down, and can be particularly frustrating due to "correct" behavior in Firefox and IE.

This is all to say that I'm personally invested in getting this patch included in trunk, so please don't hesitate to let me know how this process can be sped along.

[Adam Barth]

> I've added a new patch, based on the original, rejected patch, to fix this bug.
> I'm not particularly fluent in C++, so please excuse any obvious mistakes.

We have some good technology for doing these kinds of comparisons.  You should take the document's securityOrigin property and call it's isSameSchemeHostPort method to compare with the target URL (which you should parse into a SecurityOrigin).

[Sam Weinig]

Comment on attachment 21617 [details]
Patch for better compliance with RFC 2616 Section 3.2.3

This should be done with SecurityOrigins.  I have a patch to change this.

[Adam Barth]

Created attachment 21672 [details]
Patch, Step 1: Clean up extra dependencies

Saw some things worth changing while fixing this bug.  Here's the first step.

[Adam Barth]

Created attachment 21673 [details]
Patch, Step 2: Test that we handle document.domain properly

Step 2.  We handle document.domain properly for XMLHttpRequest, but we don't have a test for it.  Let's make sure we don't regress.

[Adam Barth]

Created attachment 21674 [details]
Patch, Step 3: Fix default ports

This patch leverages our SecurityOrigin technology to get the default ports correct.

[Eric Seidel (no email)]

Comment on attachment 21672 [details]
Patch, Step 1: Clean up extra dependencies

LGTM.

[Eric Seidel (no email)]

Comment on attachment 21673 [details]
Patch, Step 2: Test that we handle document.domain properly

I don't really know the background here.  This looks sane to me though.  Best to let sam or mjs give the official r+.

[Darin Adler]

Comment on attachment 21673 [details]
Patch, Step 2: Test that we handle document.domain properly

r=me

[Darin Adler]

Comment on attachment 21674 [details]
Patch, Step 3: Fix default ports

r=me

[Adam Barth]

Fixed in r34532.