Bug 15100 - XMLHttpRequest::urlMatchesDocumentDomain raises error if port information does not match exactly
: XMLHttpRequest::urlMatchesDocumentDomain raises error if port information doe...
Status: RESOLVED FIXED
: WebKit
New Bugs
: 523.x (Safari 3)
: All All
: P2 Normal
Assigned To:
:
: InRadar, ReviewedForRadar
:
:
  Show dependency treegraph
 
Reported: 2007-08-28 08:54 PST by
Modified: 2008-06-13 23:53 PST (History)


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 PST, Christian Mittendorf
mjs: review-
Review Patch | Details | Formatted Diff | Diff
Patch for better compliance with RFC 2616 Section 3.2.3 (1003 bytes, patch)
2008-06-10 20:48 PST, Travis Vachon
sam: review-
Review Patch | Details | Formatted Diff | Diff
Patch, Step 1: Clean up extra dependencies (1.17 KB, patch)
2008-06-12 22:59 PST, Adam Barth
eric: review+
Review Patch | Details | Formatted Diff | Diff
Patch, Step 2: Test that we handle document.domain properly (1.67 KB, patch)
2008-06-12 23:00 PST, Adam Barth
darin: review+
Review Patch | Details | Formatted Diff | Diff
Patch, Step 3: Fix default ports (5.05 KB, patch)
2008-06-12 23:02 PST, Adam Barth
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2007-08-28 08:54:29 PST
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 From 2007-08-28 10:00:02 PST -------
<rdar://problem/5443521>
------- Comment #2 From 2007-08-28 10:44:01 PST -------
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 From 2007-08-28 13:39:09 PST -------
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 From 2007-08-28 13:46:01 PST -------
(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 From 2007-08-28 13:56:26 PST -------
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 From 2007-08-28 15:19:01 PST -------
Created an attachment (id=16149) [details]
Added use of default http/https port for comparison if no port was specified in the url.
------- Comment #7 From 2007-08-28 15:45:37 PST -------
(In reply to comment #6)
> Created an attachment (id=16149) [edit] [details]
> 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 From 2007-08-28 16:01:20 PST -------
(From update of attachment 16149 [details])
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 From 2007-08-28 20:04:02 PST -------
(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 From 2007-08-28 20:18:30 PST -------
(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 From 2007-08-28 20:55:22 PST -------
(From update of attachment 16149 [details])
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 From 2007-08-28 21:21:58 PST -------
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 From 2008-06-10 20:48:53 PST -------
Created an attachment (id=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 From 2008-06-10 20:56:10 PST -------
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 From 2008-06-11 00:24:41 PST -------
> 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 From 2008-06-11 00:30:41 PST -------
(From update of attachment 21617 [details])
This should be done with SecurityOrigins.  I have a patch to change this.
------- Comment #17 From 2008-06-12 22:59:06 PST -------
Created an attachment (id=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 From 2008-06-12 23:00:34 PST -------
Created an attachment (id=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 From 2008-06-12 23:02:08 PST -------
Created an attachment (id=21674) [details]
Patch, Step 3: Fix default ports

This patch leverages our SecurityOrigin technology to get the default ports correct.
------- Comment #20 From 2008-06-12 23:14:34 PST -------
(From update of attachment 21672 [details])
LGTM.
------- Comment #21 From 2008-06-12 23:15:43 PST -------
(From update of attachment 21673 [details])
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 From 2008-06-13 14:09:19 PST -------
(From update of attachment 21673 [details])
r=me
------- Comment #23 From 2008-06-13 14:10:37 PST -------
(From update of attachment 21674 [details])
r=me
------- Comment #24 From 2008-06-13 23:53:52 PST -------
Fixed in r34532.