Correctly set initiated_in_secure_context on requests from shared/service workers, (issue 2110163002 by mek@chromium.org)

0 views
Skip to first unread message

m...@chromium.org

unread,
Jun 29, 2016, 6:47:47 PM6/29/16
to ho...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, chromium...@chromium.org, dari...@chromium.org, fal...@chromium.org, horo+...@chromium.org, j...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko...@chromium.org, kinuko+ser...@chromium.org, mich...@chromium.org, mlamouri+wa...@chromium.org, nhi...@chromium.org, servicewor...@chromium.org, tz...@chromium.org
Reviewers: horo
CL: https://codereview.chromium.org/2110163002/

Description:
Correctly set initiated_in_secure_context on requests from shared/service
workers,

This field was added in
https://crrev.com/dd933bda0baa6a13ab0120f0056a2b783e459efb
to limit foreign fetch to only intercept requests made from secure contexts,
but wasn't set for shared and service worker initiated requests. So this
fixes intercepting requests made from shared and service workers by a
foreign fetch service worker.

BUG=540509

Base URL: https://chromium.googlesource.com/chromium/src.git@skip-service-worker-foreign-fetch

Affected files (+145, -9 lines):
M content/browser/service_worker/foreign_fetch_request_handler.cc
M content/renderer/service_worker/service_worker_context_client.cc
M content/renderer/shared_worker/embedded_shared_worker_stub.cc
M third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html
A third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html
M third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/foreign-fetch-helper-iframe.html
A third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/foreign-fetch-helper-script.js
A third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/foreign-fetch-helper-worker.js


ho...@chromium.org

unread,
Jun 30, 2016, 6:12:09 AM6/30/16
to m...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, chromium...@chromium.org, dari...@chromium.org, fal...@chromium.org, horo+...@chromium.org, j...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko...@chromium.org, kinuko+ser...@chromium.org, mich...@chromium.org, mlamouri+wa...@chromium.org, nhi...@chromium.org, servicewor...@chromium.org, tz...@chromium.org
This patch lgtm.

But if the fetch request from SW goes to the other SW, it is possible to make
infinite loop.
Do you have any plan to protect the browser from the infinite loop attack?



https://codereview.chromium.org/2110163002/diff/1/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html
(right):

https://codereview.chromium.org/2110163002/diff/1/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html#newcode50
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html:50:
let worker = new
SharedWorker('resources/foreign-fetch-helper-script.js');
nit: Wrap at 80 columns.
https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests

https://codereview.chromium.org/2110163002/diff/1/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html#newcode86
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html:86:
}, 'Service Worker does not intercept fetches from an insecure dedicated
worker.');
ditto

https://codereview.chromium.org/2110163002/diff/1/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html#newcode101
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html:101:
}, 'Service Worker does not intercept fetches from an insecure shared
worker.');
ditto

https://codereview.chromium.org/2110163002/

m...@chromium.org

unread,
Jun 30, 2016, 6:43:12 PM6/30/16
to ho...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, chromium...@chromium.org, dari...@chromium.org, fal...@chromium.org, horo+...@chromium.org, j...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko...@chromium.org, kinuko+ser...@chromium.org, mich...@chromium.org, mlamouri+wa...@chromium.org, nhi...@chromium.org, servicewor...@chromium.org, tz...@chromium.org
On 2016/06/30 at 10:12:09, horo wrote:
> But if the fetch request from SW goes to the other SW, it is possible to make
infinite loop.
> Do you have any plan to protect the browser from the infinite loop attack?

Good question. I think we had some discussions about this from the standards
side a while ago, although I can't find those at the moment. But the conclusion
there was that actually detecting any kind of loops like this is going to be
nearly impossible, and probably we don't need to do so. To actually end up in a
loop a SW will have to do make a different request than what it was handling in
its onforeignfetch event, and it can even do so asynchronously (or it could even
postmessage a different same-origin service worker, and have that service worker
make the new fetch request).

The worst that can happen is that multiple service workers bombard each other
with foreignfetch events. It shouldn't really effect the browser too much. All
this stuff is async, so from the browsers point of view this isn't very
different from a page repeatedly calling fetch. I guess the only extra bad thing
about this is that this is yet another vector for a service worker (or in this
case a set of service workers) to keep themselves alive, arbitrarily long after
any page that might have used them has gone away (postmessage can already do
that). Not sure what heuristics we could come up with automatically detect
misbehaving service workers. If the things we're worried about is actually
actively malicious service workers those heuristics have to be a lot better than
if we're just trying to catch accidentally misbehaving service workers.



https://codereview.chromium.org/2110163002/diff/1/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html
(right):

https://codereview.chromium.org/2110163002/diff/1/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html#newcode50
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html:50:
let worker = new
SharedWorker('resources/foreign-fetch-helper-script.js');
On 2016/06/30 at 10:12:09, horo wrote:
> nit: Wrap at 80 columns.
> https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests

Done


https://codereview.chromium.org/2110163002/diff/1/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html#newcode86
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html:86:
}, 'Service Worker does not intercept fetches from an insecure dedicated
worker.');
On 2016/06/30 at 10:12:09, horo wrote:
> ditto

Done


https://codereview.chromium.org/2110163002/diff/1/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html#newcode101
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-workers.html:101:
}, 'Service Worker does not intercept fetches from an insecure shared
worker.');
On 2016/06/30 at 10:12:09, horo wrote:
> ditto

Done

https://codereview.chromium.org/2110163002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jun 30, 2016, 7:05:02 PM6/30/16
to m...@chromium.org, ho...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, chromium...@chromium.org, dari...@chromium.org, fal...@chromium.org, horo+...@chromium.org, j...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko...@chromium.org, kinuko+ser...@chromium.org, mich...@chromium.org, mlamouri+wa...@chromium.org, nhi...@chromium.org, servicewor...@chromium.org, tz...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jun 30, 2016, 8:32:54 PM6/30/16
to m...@chromium.org, ho...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, chromium...@chromium.org, dari...@chromium.org, fal...@chromium.org, horo+...@chromium.org, j...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko...@chromium.org, kinuko+ser...@chromium.org, mich...@chromium.org, mlamouri+wa...@chromium.org, nhi...@chromium.org, servicewor...@chromium.org, tz...@chromium.org
Committed patchset #2 (id:20001)

https://codereview.chromium.org/2110163002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jun 30, 2016, 8:34:16 PM6/30/16
to m...@chromium.org, ho...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, chromium...@chromium.org, dari...@chromium.org, fal...@chromium.org, horo+...@chromium.org, j...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko...@chromium.org, kinuko+ser...@chromium.org, mich...@chromium.org, mlamouri+wa...@chromium.org, nhi...@chromium.org, servicewor...@chromium.org, tz...@chromium.org
Patchset 2 (id:??) landed as
https://crrev.com/ebb3beacf3c308eb79b4f3e034a8144df0a16a18
Cr-Commit-Position: refs/heads/master@{#403372}

https://codereview.chromium.org/2110163002/
Reply all
Reply to author
Forward
0 new messages