| Summary: | crossorigin element resource loading should check HTTP redirection | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | youenn fablet <youennf> |
| Component: | DOM | Assignee: | youenn fablet <youennf> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | aestes, ap, beidson, bfulgham, buildbot, cdumez, commit-queue, dbates, esprehn+autocc, glenn, gyuyoung.kim, japhet, macpherson, man.zhong, menard, mkwst, rniwa, webkit-bug-importer |
| Priority: | P2 | Keywords: | InRadar |
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Attachments: | |||
|
Description
youenn fablet
2014-03-21 04:19:59 PDT
Redirection checks could be implemented within SubResourceLoader. DocumentThreadableLoader already implements redirection checks. Part of the code could be retrofitted/shared between them. Created attachment 227656 [details]
Patch
This patch checks redirections when request policy is set to PotentiallyCrossOriginEnabled
Created attachment 228584 [details]
fixing EFL & Mac compilation
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). 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. (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. Created attachment 228832 [details]
Using ResourceRequest origin
Created attachment 228836 [details]
Fix mac compilation
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. 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 Created attachment 229533 [details]
Updated according comments
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.
Created attachment 274276 [details]
Rebasing
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 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
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 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
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 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
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 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
Created attachment 274288 [details]
Readding Accept-Encoding clearing that got lost in the rebasing
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. 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. You may want to consider adding a test/tests to ensure that we log console warnings with the formatting that we expect. Created attachment 274402 [details]
Patch for landing
(In reply to comment #26) > Created attachment 274402 [details] > Patch for landing Thanks for the review. I integrated all comments. (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. Comment on attachment 274402 [details] Patch for landing Clearing flags on attachment: 274402 Committed r198395: <http://trac.webkit.org/changeset/198395> All reviewed patches have been landed. Closing bug. |