Bug 88139 – The value in Access-Control-Allow-Origin is not being matched correctly for CORS-enabled requests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Pablo Flouret]

Step 3 of the resource sharing check algorithm in http://dvcs.w3.org/hg/cors/raw-file/tip/Overview.html#resource-sharing-check says:
[[
3. If the value of Access-Control-Allow-Origin is not a case-sensitive match for the value of the Origin header as defined by its specification, return fail and terminate this algorithm.
]]

Right now the value in Access-Control-Allow-Origin is compared via SecurityOrigin::isSameSchemeHostPort(), so the url is parsed and (i presume) normalized.

[Pablo Flouret]

Adam,

I can make a patch, but do you think it would break a lot of things in the wild?

[Adam Barth]

I suspect ap has an opinion here.  My understanding is that Firefox follows the spec, so I think we should give following the spec a try.  This is a security check, so we don't want to be sloppy.

[Alexey Proskuryakov]

Yup.

[Odin Hørthe Omdal (odinho, Opera)]

The testcase there is pointing to a wrong domain (www.w3c-test.org), it should be no-www. The server should probably be configured in a bit more restrictive way so that it's easier to rely on the domain.

Anyway, there's an updated test, that doesn't test for the exception code (which is not that interesting):

http://w3c-test.org/webappsec/tests/cors/submitted/opera/staging/origin.htm

That URI will also change at one point if it moves to approved.

[Pablo Flouret]

Created attachment 145633 [details]
Patch

[Adam Barth]

This patch LGTM.  Let's give folks a chance to comment before landing.

[Adam Barth]

@peter: I wanted to add the WebVisible keyword to this patch, but it doesn't seem to exist yet.

Comment on attachment 145633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145633&action=review

> Source/WebCore/loader/CrossOriginAccessControl.cpp:152
> +    if (accessControlOriginString != securityOrigin->toString()) {

should we check for !securityOrigin->isUnique() or would that be overkill?

[Pablo Flouret]

(In reply to comment #8)
> (From update of attachment 145633 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145633&action=review
> 
> > Source/WebCore/loader/CrossOriginAccessControl.cpp:152
> > +    if (accessControlOriginString != securityOrigin->toString()) {
> 
> should we check for !securityOrigin->isUnique() or would that be overkill?

That check is already a few lines above the change.

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 145633 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=145633&action=review
> > 
> > > Source/WebCore/loader/CrossOriginAccessControl.cpp:152
> > > +    if (accessControlOriginString != securityOrigin->toString()) {
> > 
> > should we check for !securityOrigin->isUnique() or would that be overkill?
> 
> That check is already a few lines above the change.

ok, patch looks good then

[Alexey Proskuryakov]

Comment on attachment 145633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145633&action=review

> Source/WebCore/loader/CrossOriginAccessControl.cpp:152
>      // FIXME: Access-Control-Allow-Origin can contain a list of origins.
> -    RefPtr<SecurityOrigin> accessControlOrigin = SecurityOrigin::createFromString(accessControlOriginString);
> -    if (!accessControlOrigin->isSameSchemeHostPort(securityOrigin)) {
> +    if (accessControlOriginString != securityOrigin->toString()) {

I guess one way this could actually cause a compatibility regression is when Access-Control-Allow-Origin has a list of origins, which we used to mis-parse as a single one. In that case, scheme host and port could reasonably matched securityOrigin for the first item. So maybe we should address the FIXME now or very soon.

Is this relying on both accessControlOriginString and securityOrigin->toString() being lowercase? I cannot immediately see why they are.

> LayoutTests/http/tests/xmlhttprequest/resources/basic-auth/access-control-auth-basic.php:3
> -header("Access-Control-Allow-Origin: http://127.0.0.1:8000/");
> +header("Access-Control-Allow-Origin: http://127.0.0.1:8000");

Hmm. So all tests using this script used to fail in Firefox?

[Pablo Flouret]

(In reply to comment #11)
> (From update of attachment 145633 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145633&action=review
> 
> > Source/WebCore/loader/CrossOriginAccessControl.cpp:152
> >      // FIXME: Access-Control-Allow-Origin can contain a list of origins.
> > -    RefPtr<SecurityOrigin> accessControlOrigin = SecurityOrigin::createFromString(accessControlOriginString);
> > -    if (!accessControlOrigin->isSameSchemeHostPort(securityOrigin)) {
> > +    if (accessControlOriginString != securityOrigin->toString()) {
> 
> I guess one way this could actually cause a compatibility regression is when Access-Control-Allow-Origin has a list of origins, which we used to mis-parse as a single one. In that case, scheme host and port could reasonably matched securityOrigin for the first item. So maybe we should address the FIXME now or very soon.
> 

Does SecurityOrigin::createFromString() create a valid origin for the first url if accessControlOriginString is a list? I assumed the FIXME meant that having a list of origins would just not work at all. I'll look into it.

> Is this relying on both accessControlOriginString and securityOrigin->toString() being lowercase? I cannot immediately see why they are.
>

The url used for the security origin is lowercased when the SecurityOrigin is created. And then the comparison is case-sensitive with the value of Access-Control-Allow-Origin. The test covers this, but i didn't realize the test is run from 127.0.0.1, hmm. Any suggestions?

> > LayoutTests/http/tests/xmlhttprequest/resources/basic-auth/access-control-auth-basic.php:3
> > -header("Access-Control-Allow-Origin: http://127.0.0.1:8000/");
> > +header("Access-Control-Allow-Origin: http://127.0.0.1:8000");
> 
> Hmm. So all tests using this script used to fail in Firefox?

Yes, it seems like it.

[Alexey Proskuryakov]

> The url used for the security origin is lowercased when the SecurityOrigin is created. And then the comparison is case-sensitive with the value of Access-Control-Allow-Origin. The test covers this, but i didn't realize the test is run from 127.0.0.1, hmm. Any suggestions?

I misread "case sensitive" in bug description. Anyway, not sure how exactly to test, but perhaps localhost vs. LOCALHOST can be used?

>> Hmm. So all tests using this script used to fail in Firefox?
> Yes, it seems like it.

Can you test to confirm?

[Pablo Flouret]

(In reply to comment #13)
> >> Hmm. So all tests using this script used to fail in Firefox?
> > Yes, it seems like it.
> 
> Can you test to confirm?

Sorry, i misread the results when i tested it before. The tests that use the script fail outright in Firefox, see https://bugs.webkit.org/show_bug.cgi?id=37781#c9

But Firefox passes all of the tests in the w3c testcase, so theoretically the trailing slash would matter in the tests that use the script.

[Pablo Flouret]

(In reply to comment #11)
> (From update of attachment 145633 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145633&action=review
> 
> > Source/WebCore/loader/CrossOriginAccessControl.cpp:152
> >      // FIXME: Access-Control-Allow-Origin can contain a list of origins.
> > -    RefPtr<SecurityOrigin> accessControlOrigin = SecurityOrigin::createFromString(accessControlOriginString);
> > -    if (!accessControlOrigin->isSameSchemeHostPort(securityOrigin)) {
> > +    if (accessControlOriginString != securityOrigin->toString()) {
> 
> I guess one way this could actually cause a compatibility regression is when Access-Control-Allow-Origin has a list of origins, which we used to mis-parse as a single one. In that case, scheme host and port could reasonably matched securityOrigin for the first item. So maybe we should address the FIXME now or very soon.

This indeed happens, but i can't think of any examples of a list of urls that is interpreted as a single url and should still work with the spec as it stands now.

The only one example i can think of that comes close would be something like "http://localhost:8000/ http://example.org". But even if we parsed the list correctly, the trailing slash would be there and it wouldn't match.

Can you think of a problematic example?

[Adam Barth]

I don't think the list case is a problem for us yet because we never send a list in the Origin header to start with.  The FIXME is correct, but it's not something we need to worry about for this patch.

[WebKit Review Bot]

Comment on attachment 145633 [details]
Patch

Attachment 145633 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12900457

New failing tests:
http/tests/xmlhttprequest/origin-exact-matching.html

[WebKit Review Bot]

Created attachment 145746 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Pablo Flouret]

(In reply to comment #17)
> (From update of attachment 145633 [details])
> Attachment 145633 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12900457
> 
> New failing tests:
> http/tests/xmlhttprequest/origin-exact-matching.html

The cases that fail are the origins that end with a null character. It doesn't fail for me on chromium-mac, though, not sure what's going on there.

[Anne van Kesteren]

*** Bug 88628 has been marked as a duplicate of this bug. ***

[Adam Barth]

Comment on attachment 145633 [details]
Patch

One thing we can do is to land this patch without the null character tests and file a bug about the null character issue.

[Pablo Flouret]

(In reply to comment #21)
> (From update of attachment 145633 [details])
> One thing we can do is to land this patch without the null character tests and file a bug about the null character issue.

Sounds good.

[Pablo Flouret]

Created attachment 146653 [details]
Patch

[Pablo Flouret]

(In reply to comment #23)
> Created an attachment (id=146653) [details]
> Patch

Removed the trailing null character tests (and filed bug 88688). Also reworked the test a bit to test case differences properly.

[Adam Barth]

Comment on attachment 146653 [details]
Patch

Alright.  Let's give this a shot.

[WebKit Review Bot]

Comment on attachment 146653 [details]
Patch

Clearing flags on attachment: 146653

Committed r119911: <http://trac.webkit.org/changeset/119911>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 146653 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146653&action=review

> LayoutTests/http/tests/xmlhttprequest/resources/origin-exact-matching-iframe.html:9
> +var urlTemplate = "http://127.0.0.1:8000/xmlhttprequest/resources/redirect-cors.php?access-control-allow-origin=";

You're not passing any "url" argument to redirect-cors.php. What is the expected behavior in that case? On GTK and EFL ports (both using libsoup), we end up with "Too many redirects" errors because it keep redirecting the same URL (http://127.0.0.1:8000/xmlhttprequest/resources/redirect-cors.php?access-control-allow-origin=...). Shouldn't we pass some URL to avoid this?

[Pablo Flouret]

(In reply to comment #28)
> (From update of attachment 146653 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146653&action=review
> 
> > LayoutTests/http/tests/xmlhttprequest/resources/origin-exact-matching-iframe.html:9
> > +var urlTemplate = "http://127.0.0.1:8000/xmlhttprequest/resources/redirect-cors.php?access-control-allow-origin=";
> 
> You're not passing any "url" argument to redirect-cors.php. What is the expected behavior in that case? On GTK and EFL ports (both using libsoup), we end up with "Too many redirects" errors because it keep redirecting the same URL (http://127.0.0.1:8000/xmlhttprequest/resources/redirect-cors.php?access-control-allow-origin=...). Shouldn't we pass some URL to avoid this?

Hmm, i thought that without a $url or $redirect_preflight it wouldn't actually do the redirect. I'll fix the test.

[David Kilzer (:ddkilzer)]

<rdar://problem/11679305>