Bug 130578 – crossorigin element resource loading should check HTTP redirection

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[youenn fablet]

Created attachment 227412 [details]
Loading of crossorigin <img> with cross-origin redirection

When loading resources from HTML elements with the crossorigin attribute set, any redirection response should be checked according cross-origin access control rules (as defined in http://www.w3.org/TR/cors/#redirect-steps).
Currently, a check is only done for the final response.
No check seems to be done for intermediary redirection (see test in attachment).

[youenn fablet]

Redirection checks could be implemented within SubResourceLoader.
DocumentThreadableLoader already implements redirection checks.
Part of the code could be retrofitted/shared between them.

[youenn fablet]

Created attachment 227656 [details]
Patch

This patch checks redirections when request policy is set to PotentiallyCrossOriginEnabled

[youenn fablet]

Created attachment 228584 [details]
fixing EFL & Mac compilation

[youenn fablet]

The patch may be a bit large.
To ease the review, would it be better to split it into smaller pieces?
Something like:
- Patch to migrate securityOrigin from ThreadableLoaderOptions to ResourceLoaderOptions (and fix related compilation issues)
- Patch to migrate CORS redirection check routines from DocumentThreadableLoader.cpp to CrossOriginAccessControl.cpp
- Patch to add support of CORS redirection check with SubresourceLoader in case policy is PotentiallyCrossOriginEnabled.

Further work (not in this patch) would be to extend the use of PotentiallyCrossOriginEnabled policy to other loaders (ImageLoader for instance).

[Sam Weinig]

Comment on attachment 228584 [details]
fixing EFL & Mac compilation

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

> Source/WebCore/ChangeLog:8
> +        Added SecurityOrigin as a loader option (in ResourceLoaderOption).

This doesn't make sense to me.  A SecurityOrigin is not an option.

[youenn fablet]

(In reply to comment #5)
> (From update of attachment 228584 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228584&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Added SecurityOrigin as a loader option (in ResourceLoaderOption).
> 
> This doesn't make sense to me.  A SecurityOrigin is not an option.

Actually, this patch is moving SecurityOrigin from ThreadableLoaderOptions to ResourceLoaderOptions. Making consistent the two in the long run seems good to me.

The origin can be retrieved from the ResourceRequest layer.
I will rewrite the patch accordingly.

[youenn fablet]

Created attachment 228832 [details]
Using ResourceRequest origin

[youenn fablet]

Created attachment 228836 [details]
Fix mac compilation

[Darin Adler]

Comment on attachment 228836 [details]
Fix mac compilation

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

This looks really good to me, but I’m not enough of an expert on the evolution of our CORS policy to know if it’s 100% right, so I have comments but not a review.

> Source/WebCore/loader/CrossOriginAccessControl.cpp:33
> +#include "SchemeRegistry.h"

Can we remove this include from DocumentThreadableLoader.cpp too?

> Source/WebCore/loader/CrossOriginAccessControl.cpp:134
> +bool isValidCrossOriginRedirectionURL(const URL& redirectUrl)

Should be redirectURL, not redirectUrl.

> Source/WebCore/loader/SubresourceLoader.cpp:141
> +    if (options().requestOriginPolicy == PotentiallyCrossOriginEnabled)
> +        m_origin = request.httpOrigin().isEmpty() ? m_frame->document()->securityOrigin() : SecurityOrigin::createFromString(request.httpOrigin());

What about when the request origin is present, but is invalid in some way? Say, it's just a single space. Is this still appropriate?

> Source/WebCore/loader/SubresourceLoader.cpp:153
> +    if (!m_origin->canRequest(newRequest.url())) {

Is m_origin guaranteed to be non-null?

Seems it would be better to have an early return here rather than nesting the entire function inside an if.

> Source/WebCore/loader/SubresourceLoader.cpp:156
> +            ||  passesAccessControlCheck(redirectResponse, options().allowCredentials, m_origin.get(), errorDescription);

Extra space here after "||".

> Source/WebCore/loader/SubresourceLoader.cpp:160
> +            m_frame->document()->addConsoleMessage(MessageSource::Security, MessageLevel::Error, errorMessage);

What guarantees the m_frame is non-null?

What guarantees that m_frame->document() is non-null?

> Source/WebCore/loader/SubresourceLoader.cpp:195
> +        if (options().requestOriginPolicy == PotentiallyCrossOriginEnabled) {
> +            if (!checkCrossOriginAccessControl(request(), redirectResponse, newRequest)) {

Why not use && instead of a nested if statement?

> Source/WebCore/loader/SubresourceLoader.h:67
> +    bool checkCrossOriginAccessControl(const ResourceRequest&, const ResourceResponse&, ResourceRequest&);

The meaning of the output ResourceRequest is not clear without an argument name. I think we need to include the name newRequest here.

It’s strange to group this with the virtual init function. Is there a more logical place to put it in the class definition?

> Source/WebCore/loader/SubresourceLoader.h:121
> +    RefPtr<SecurityOrigin> m_origin;
> +

Please don’t add this blank line.

[youenn fablet]

Thanks for the comments.
I will improve the patch accordingly.
Inlined comments below.

> > Source/WebCore/loader/CrossOriginAccessControl.cpp:33
> > +#include "SchemeRegistry.h"
> 
> Can we remove this include from DocumentThreadableLoader.cpp too?

SchemeRegistry is used in one other place of DocumentThreadableLoader.cpp.

> > Source/WebCore/loader/CrossOriginAccessControl.cpp:134
> > +bool isValidCrossOriginRedirectionURL(const URL& redirectUrl)
> 
> Should be redirectURL, not redirectUrl.

OK

> > Source/WebCore/loader/SubresourceLoader.cpp:141
> > +    if (options().requestOriginPolicy == PotentiallyCrossOriginEnabled)
> > +        m_origin = request.httpOrigin().isEmpty() ? m_frame->document()->securityOrigin() : SecurityOrigin::createFromString(request.httpOrigin());
> 
> What about when the request origin is present, but is invalid in some way? Say, it's just a single space. Is this still appropriate?

It should be fine, the created security origin will be a "unique" one.

I will think a bit more on that code path though.
Maybe we should mandate the request origin http header to be set when using PotentiallyCrossOriginEnabled and never fallback to the document securityOrigin.


> > Source/WebCore/loader/SubresourceLoader.cpp:153
> > +    if (!m_origin->canRequest(newRequest.url())) {
> 
> Is m_origin guaranteed to be non-null?

Currently yes, as:
- the function is only called in the case of policy being PotentiallyCrossOriginEnabled
- CSS images (sole user of PotentiallyCrossOriginEnabled) properly set the request http origin header.

> Seems it would be better to have an early return here rather than nesting the entire function inside an if.

OK.

> > Source/WebCore/loader/SubresourceLoader.cpp:156
> > +            ||  passesAccessControlCheck(redirectResponse, options().allowCredentials, m_origin.get(), errorDescription);
> 
> Extra space here after "||".

OK

> > Source/WebCore/loader/SubresourceLoader.cpp:160
> > +            m_frame->document()->addConsoleMessage(MessageSource::Security, MessageLevel::Error, errorMessage);
> 
> What guarantees the m_frame is non-null?
> 
> What guarantees that m_frame->document() is non-null?

Right, I will add the necessary checks.

> > Source/WebCore/loader/SubresourceLoader.cpp:195
> > +        if (options().requestOriginPolicy == PotentiallyCrossOriginEnabled) {
> > +            if (!checkCrossOriginAccessControl(request(), redirectResponse, newRequest)) {
> 
> Why not use && instead of a nested if statement?

OK.

> > Source/WebCore/loader/SubresourceLoader.h:67
> > +    bool checkCrossOriginAccessControl(const ResourceRequest&, const ResourceResponse&, ResourceRequest&);
> 
> The meaning of the output ResourceRequest is not clear without an argument name. I think we need to include the name newRequest here.

OK

> It’s strange to group this with the virtual init function. Is there a more logical place to put it in the class definition?

I will try to find a better place.
 
> > Source/WebCore/loader/SubresourceLoader.h:121
> > +    RefPtr<SecurityOrigin> m_origin;
> > +
> 
> Please don’t add this blank line.

OK

[youenn fablet]

Created attachment 229533 [details]
Updated according comments

[Brent Fulgham]

Comment on attachment 229533 [details]
Updated according comments

Hi Youenn, this is another great patch that was not handled properly. Could you please rebase the patch against current sources and we can get this integrated? I'm sorry for all of this trouble.

[youenn fablet]

Created attachment 274276 [details]
Rebasing

[Build Bot]

Comment on attachment 274276 [details]
Rebasing

Attachment 274276 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/993702

New failing tests:
http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr-redirect.html
http/tests/xmlhttprequest/access-control-and-redirects.html
http/tests/xmlhttprequest/access-control-and-redirects-async-same-origin.html
http/tests/xmlhttprequest/redirect-cors-origin-null.html
http/tests/security/isolatedWorld/bypass-worker-csp-for-xhr-redirect-cross-origin.html

[Build Bot]

Created attachment 274278 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 274276 [details]
Rebasing

Attachment 274276 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/993704

New failing tests:
http/tests/xmlhttprequest/redirect-cors-origin-null.html
http/tests/xmlhttprequest/access-control-and-redirects.html
http/tests/xmlhttprequest/access-control-and-redirects-async-same-origin.html
http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr-redirect.html
http/tests/security/isolatedWorld/bypass-worker-csp-for-xhr-redirect-cross-origin.html

[Build Bot]

Created attachment 274279 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 274276 [details]
Rebasing

Attachment 274276 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/993703

New failing tests:
http/tests/xmlhttprequest/redirect-cors-origin-null.html
http/tests/xmlhttprequest/access-control-and-redirects.html
http/tests/xmlhttprequest/access-control-and-redirects-async-same-origin.html
http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr-redirect.html
http/tests/security/isolatedWorld/bypass-worker-csp-for-xhr-redirect-cross-origin.html

[Build Bot]

Created attachment 274280 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 274276 [details]
Rebasing

Attachment 274276 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/993710

New failing tests:
http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr-redirect.html
http/tests/xmlhttprequest/access-control-and-redirects.html
http/tests/xmlhttprequest/access-control-and-redirects-async-same-origin.html
http/tests/xmlhttprequest/redirect-cors-origin-null.html
http/tests/security/isolatedWorld/bypass-worker-csp-for-xhr-redirect-cross-origin.html

[Build Bot]

Created attachment 274283 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 274288 [details]
Readding Accept-Encoding clearing that got lost in the rebasing

[Brent Fulgham]

Comment on attachment 274288 [details]
Readding Accept-Encoding clearing that got lost in the rebasing

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

This looks good to me. I'd like Dan Bates to also take a look before committing.

> Source/WebCore/loader/SubresourceLoader.cpp:152
> +    // This would simplify resource loader users as they would only need to set the policy to PotentiallyCrossOriginEnabled.

Could you please file a Bugzilla bug for this so we can track the future work?

> Source/WebCore/loader/SubresourceLoader.cpp:403
> +    // If the request URL origin is not same origin with the original URL origin, request origin should be set to a globally unique identifier.

// If the request URL origin is not the same as the original origin, the request origin should be set to a globally unique identifier.

[Daniel Bates]

Comment on attachment 274288 [details]
Readding Accept-Encoding clearing that got lost in the rebasing

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

> Source/WebCore/ChangeLog:10
> +        Moved part of DocumentThreadableLoader redirection cross origin control code into functions in CrossOriginAccessControl.cpp.
> +        Added cross origin control for redirections in SubResourceLoader when policy is set to PotentiallyCrossOriginEnabled (using CrossOriginAccessControl.cpp new functions).
> +        Added a new test that checks that cross-origin redirections are checked against CORS (currently PotentiallyCrossOriginEnabled is used only for CSS images).

Nit: It is an unwritten convention that we wrap ChangeLog descriptions to around 100 characters.

> Source/WebCore/loader/CrossOriginAccessControl.cpp:146
> +    // Remove any header that may have been added by the network layer that cause access control to fail.

Nit: "any header" => "headers"

> Source/WebCore/loader/DocumentThreadableLoader.cpp:234
>              // Remove any headers that may have been added by the network layer that cause access control to fail.

I suggest that we remove this comment now that we have moved the header removal logic into cleanRedirectedRequestForAccessControl().

>> Source/WebCore/loader/SubresourceLoader.cpp:152
>> +    // This would simplify resource loader users as they would only need to set the policy to PotentiallyCrossOriginEnabled.
> 
> Could you please file a Bugzilla bug for this so we can track the future work?

Nit: TODO => FIXME
Nit: Subresourceloader => SubresourceLoader

(We prefer to use prefix FIXME instead of TODO to denote tasks that need to be addressed in the future per <https://webkit.org/code-style-guidelines/#comments-fixme>.)

> Source/WebCore/loader/SubresourceLoader.cpp:397
> +                (!responsePassesCORS ? errorDescription : "redirection URL not allowed");

This error message is ambiguous when responsePassesCORS evaluates to true, does not begin with a capital letter or end in a period (compared to the error description returned by passesAccessControlCheck()). Can we come up with a more descriptive error message than "redirection URL not allowed"? Maybe "Redirected to either a non-HTTP URL or a URL that contains credentials."

> LayoutTests/http/tests/security/resources/redirect-allow-star.php:2
> +    $url = $_GET['url'];

Nit: ' (single quote) => " (double quote)

With the exception of this line (2) and line 4 (below) we use double quotes for strings literals. We should one quoting style throughout this file for consistency.

> LayoutTests/http/tests/security/resources/redirect-allow-star.php:6
> +    $code = $_GET['code'];
> +    if (!isset($code))
> +        $code = 302;

This will cause a PHP undefined index warning if query parameter code is not passed because $_GET['code'] is accessed outside of language construct isset(). We can avoid this warning by passing $_GET['code']  to isset():

$code = isset($_GET["code"]) ? $_GET["code"] : 302;

> LayoutTests/http/tests/security/shape-image-cors-redirect-expected.html:29
> +    /* KO Tests */

I assuming KO is an abbreviation for "knockout". Is "KO Tests"/"ko" standard terminology for referring to tests that exercise bad/disallowed behavior? If not then I suggest we make use of words such as "disallowed" or "not OK" when talking about these tests.

> LayoutTests/http/tests/security/shape-image-cors-redirect.html:23
> +    /* Cross-origin request is OK because the "Access-Control-Allow-Origin: *" is returned for the final resource and the redirection is same origin. */

This comment is misleading. In particular, It is misleading to write "redirection is same origin" to describe that the requested document (/resources/redirect.php => http://127.0.0.1:8000/resources/redirect.php) has the same origin as this web page (http://127.0.0.1:8000) because we are actually redirecting to the cross-origin document <http://localhost:8080/security/resources/image-access-control.php>. One way to capture this concept is by talking in terms of the origin that initiating the redirect, its the same origin as the page. Maybe something like:

Cross-origin request is OK because the HTTP header "Access-Control-Allow-Origin: *" is returned for the final response when the redirection was initiated from the same origin as the page.

> LayoutTests/http/tests/security/shape-image-cors-redirect.html:33
> +    /* KO Tests */

I assuming KO is an abbreviation for "knockout". Is "KO Tests"/"ko" standard terminology for referring to tests that exercise bad/disallowed behavior? If not then I suggest we make use of words such as "disallowed" or "not OK" when talking about these tests.

> LayoutTests/http/tests/security/shape-image-cors-redirect.html:34
> +    /* Cross-origin request is not OK because a "Access-Control-Allow-Origin:" header is not returned for the final resource (redirection is same origin). */

Similarly, this comment is misleading because of its use of the phrase "redirection is same origin". Maybe write:

Cross-origin request is not OK because the HTTP header "Access-Control-Allow-Origin:" header is not returned for the final resource when the redirection was initiated from the same origin as the page.

[Daniel Bates]

You may want to consider adding a test/tests to ensure that we log console warnings with the formatting that we expect.

[youenn fablet]

Created attachment 274402 [details]
Patch for landing

[youenn fablet]

(In reply to comment #26)
> Created attachment 274402 [details]
> Patch for landing

Thanks for the review.
I integrated all comments.

[youenn fablet]

(In reply to comment #25)
> You may want to consider adding a test/tests to ensure that we log console
> warnings with the formatting that we expect.

I added some in the patch, but they are flaky (error message can be printed once or twice depending on the test run).
I filed bug 155634 to keep track of those tests.

[WebKit Commit Bot]

Comment on attachment 274402 [details]
Patch for landing

Clearing flags on attachment: 274402

Committed r198395: <http://trac.webkit.org/changeset/198395>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/25241885>