Bug 15100 - XMLHttpRequest::urlMatchesDocumentDomain raises error if port information does not match exactly
: XMLHttpRequest::urlMatchesDocumentDomain raises error if port information doe...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 523.x (Safari 3)
: All All
: P2 Normal
Assigned To: Nobody
: InRadar, ReviewedForRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-28 08:54 PDT by Christian Mittendorf
Modified: 2008-06-13 23:53 PDT (History)
6 users (show)

See Also:


Attachments
Added use of default http/https port for comparison if no port was specified in the url. (1.21 KB, patch)
2007-08-28 15:19 PDT, Christian Mittendorf
mjs: review-
Details | Formatted Diff | Diff
Patch for better compliance with RFC 2616 Section 3.2.3 (1003 bytes, patch)
2008-06-10 20:48 PDT, Travis Vachon
sam: review-
Details | Formatted Diff | Diff
Patch, Step 1: Clean up extra dependencies (1.17 KB, patch)
2008-06-12 22:59 PDT, Adam Barth
eric: review+
Details | Formatted Diff | Diff
Patch, Step 2: Test that we handle document.domain properly (1.67 KB, patch)
2008-06-12 23:00 PDT, Adam Barth
darin: review+
Details | Formatted Diff | Diff
Patch, Step 3: Fix default ports (5.05 KB, patch)
2008-06-12 23:02 PDT, Adam Barth
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Mittendorf 2007-08-28 08:54:29 PDT
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.
Comment 1 David Kilzer (:ddkilzer) 2007-08-28 10:00:02 PDT
<rdar://problem/5443521>
Comment 2 Alexey Proskuryakov 2007-08-28 10:44:01 PDT
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.
Comment 3 Christian Mittendorf 2007-08-28 13:39:09 PDT
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.
Comment 4 Christian Mittendorf 2007-08-28 13:46:01 PDT
(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).

Comment 5 Christian Mittendorf 2007-08-28 13:56:26 PDT
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.
Comment 6 Christian Mittendorf 2007-08-28 15:19:01 PDT
Created attachment 16149 [details]
Added use of default http/https port for comparison if no port was specified in the url.
Comment 7 David Kilzer (:ddkilzer) 2007-08-28 15:45:37 PDT
(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

Comment 8 Darin Adler 2007-08-28 16:01:20 PDT
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.
Comment 9 Alexey Proskuryakov 2007-08-28 20:04:02 PDT
(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.
Comment 10 Boris Zbarsky 2007-08-28 20:18:30 PDT
(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).
Comment 11 Maciej Stachowiak 2007-08-28 20:55:22 PDT
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.
Comment 12 Sam Weinig 2007-08-28 21:21:58 PDT
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.
Comment 13 Travis Vachon 2008-06-10 20:48:53 PDT
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.
Comment 14 Travis Vachon 2008-06-10 20:56:10 PDT
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.
Comment 15 Adam Barth 2008-06-11 00:24:41 PDT
> 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).
Comment 16 Sam Weinig 2008-06-11 00:30:41 PDT
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.
Comment 17 Adam Barth 2008-06-12 22:59:06 PDT
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.
Comment 18 Adam Barth 2008-06-12 23:00:34 PDT
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.
Comment 19 Adam Barth 2008-06-12 23:02:08 PDT
Created attachment 21674 [details]
Patch, Step 3: Fix default ports

This patch leverages our SecurityOrigin technology to get the default ports correct.
Comment 20 Eric Seidel 2008-06-12 23:14:34 PDT
Comment on attachment 21672 [details]
Patch, Step 1: Clean up extra dependencies

LGTM.
Comment 21 Eric Seidel 2008-06-12 23:15:43 PDT
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+.
Comment 22 Darin Adler 2008-06-13 14:09:19 PDT
Comment on attachment 21673 [details]
Patch, Step 2: Test that we handle document.domain properly

r=me
Comment 23 Darin Adler 2008-06-13 14:10:37 PDT
Comment on attachment 21674 [details]
Patch, Step 3: Fix default ports

r=me
Comment 24 Adam Barth 2008-06-13 23:53:52 PDT
Fixed in r34532.