New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment

Fetch event shouldn't fire for preflight requests

Project Member Reported by jakearchibald@chromium.org, Aug 31 2014

Issue description

(I may be being overly cautious giving this ticket a restricted view, feel free to open it up)

From a page:

var req = new XMLHttpRequest();
req.open('GET', 'http://jakearchibald.com');
req.setRequestHeader('foo', 'bar');
req.send();

The serviceworker gets a fetch event for the OPTIONS request, meaning I can respond:

self.addEventListener('fetch', function(event) {
  if (event.request.method == 'OPTIONS') {
    event.respondWith(new Response('', {
      headers: {
        'Access-Control-Allow-Origin': location.origin,
        'Access-Control-Allow-Methods': 'GET',
        'Access-Control-Allow-Headers': 'foo'
      }
    }));
  }
});

…which allows the browser to make the GET request to http://jakearchibald.com (including the foo header).

According to http://fetch.spec.whatwg.org/#concept-http-fetch, the main request gets a fetch event within the serviceworker (step 2), if the serviceworker doesn't respondWith the "skip service worker flag is set" (3.1), a CORS preflight it performed which doesn't go through the serviceworker because of the flag (3.2.1)
 

Comment 1 by wfh@chromium.org, Aug 31 2014

Labels: Security_Severity-Low ReleaseBlock-Stable Cr-Blink-SecurityFeature
Owner: jww@chromium.org
jww@ - I have a "welcome back from vacation" bug present for you!  Can you triage or reassign as necessary.
Cc: palmer@chromium.org slightlyoff@chromium.org
Status: Assigned
But the service worker is already in the same origin as it is intercepting for, right?

So, is the potential impact that a malicious service worker (installed e.g. for the brief moment in time when the server was XSS'd or compromised) could prevent the server from "cleaning house" by setting new policy headers?

That seems worse than Security_Severity-Low if I understand correctly — but I might definitely not be understanding. Thanks for clues!
Labels: M-38
Project Member

Comment 4 by ClusterFuzz, Sep 2 2014

Labels: Security_Impact-Beta
This issue doesn't require a site's ServiceWorker to be hijacked.

Fetch events are fired for navigate requests within a ServiceWorker's scope, and also any subresource on those pages, regardless of destination.

Using XHR I can make a post/get request to your page from my page. As long as the headers are "basic", this request is made no-questions-asked, because you've historically been able to do this with an img, or self-submitting form. I won't get access to the content via JS, but the request is made.

If I use a different method or non-basic header, a CORS preflight request is made to the destination (OPTIONS request) asking if it's ok to make a request using the particular method and headers.

However, those CORS preflight requests are going via the serviceworker, which is wrong.

This means:

* I make a request from my origin to your origin using non-basic methods/headers
* A "fetch" event for the CORS preflight arrives in my ServiceWorker
* I respond indicating the method headers are ok
* A "fetch" event for the actual request arrives in my ServiceWorker
* I don't handle it, so the default thing happens, which is a network request

Now I've made a non-basic request to your origin without your permission. I won't be able to read the response, unless the response has CORS headers, but I just made a request without your permission.

How it should have happened:

* I make a request from my origin to your origin using non-basic methods/headers
* A "fetch" event for the actual request arrives in my ServiceWorker
* I either provide my own response, or let it go to the network

If I let it go to the network, it'll trigger a preflight request that I cannot observe/intercept in the ServiceWorker.
Labels: -Security_Severity-Low -M-38 Security_Severity-High M-37 Pri-1
This is high-severity, as overriding the the preflight means you have an arbitrary origin bypass against other origins via service worker. Much scary.
Cc: mkwst@chromium.org
Labels: -Security_Severity-High Security_Severity-Medium
Looks like I read to fast. It's not an arbitrary origin bypass, but it would allow for a range of attacks that are currently assumed to not be possible (e.g. cross-origin overriding custom headers used for XSRF protection).

+mkwst@, since he might be interested.
Project Member

Comment 8 by ClusterFuzz, Sep 3 2014

Labels: -Security_Impact-Beta Security_Impact-Stable
Labels: -M-37 M-38
after conversation with jschuh@, this can be deferred until m38

Comment 10 by jww@chromium.org, Sep 4 2014

Cc: dominicc@chromium.org
+dominicc@

dominicc, who's been working on the "fetch" event in general and would be a good fit for this? I'm not sure I'm quite up for tackling this right now.
Cc: horo@chromium.org
+horo@ who's been working on fetch() & other serviceworker parts

Comment 13 by jww@chromium.org, Sep 5 2014

Owner: horo@chromium.org
horo@, I'm actually reassigning to you as I'm not familiar with this implementation. Let me know if I can help, though.

Comment 14 by horo@chromium.org, Sep 6 2014

Cc: yhirano@chromium.org kenjibaheux@chromium.org

Comment 15 by horo@chromium.org, Sep 6 2014

Status: Started

Comment 16 by horo@chromium.org, Sep 6 2014

Labels: -M-38 M-39
In M28, OPTIONS request is not send to the SerivceWorker.
This was changed in r290247. (M38 branch cut point is r290040)

Comment 17 by horo@chromium.org, Sep 6 2014

Typo:
In M38, OPTIONS request is not send to the SerivceWorker.
This was changed in r290247. (M38 branch cut point is r290040)

Comment 18 by horo@chromium.org, Sep 6 2014

This is a bug in the implementation of HTTP fetch in DocumentThreadableLoader.
But I think it takes time to fix this.
So I created a work around patch.

https://codereview.chromium.org/548773002/
[ServiceWorker] Don't pass OPTIONS request to the ServiceWorker

Comment 19 by horo@chromium.org, Sep 6 2014

Cc: tyoshino@chromium.org

Comment 20 by horo@chromium.org, Sep 8 2014

Cc: nhiroki@chromium.org
Project Member

Comment 21 by bugdroid1@chromium.org, Sep 8 2014

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c8acfe2e520a5a44e3d158441dbc2bd2057a899

commit 2c8acfe2e520a5a44e3d158441dbc2bd2057a899
Author: horo <horo@chromium.org>
Date: Mon Sep 08 09:45:10 2014

[ServiceWorker] Don't pass OPTIONS request to the ServiceWorker

This is work around to avoid hijacking CORS preflight.
In current implementation, CORS preflight request is also sent to the ServiceWorker.
This is a bug in the implementation of HTTP fetch in DocumentThreadableLoader.
But it takes time to fix this.
So I want to introduce this work around because this is a security bug.

BUG= 409454 

Review URL: https://codereview.chromium.org/548773002

Cr-Commit-Position: refs/heads/master@{#293697}

[modify] https://chromium.googlesource.com/chromium/src.git/+/2c8acfe2e520a5a44e3d158441dbc2bd2057a899/content/browser/service_worker/service_worker_request_handler.cc
[add] https://chromium.googlesource.com/chromium/src.git/+/2c8acfe2e520a5a44e3d158441dbc2bd2057a899/content/browser/service_worker/service_worker_request_handler_unittest.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/2c8acfe2e520a5a44e3d158441dbc2bd2057a899/content/browser/service_worker/service_worker_storage.h
[modify] https://chromium.googlesource.com/chromium/src.git/+/2c8acfe2e520a5a44e3d158441dbc2bd2057a899/content/content_tests.gypi

Project Member

Comment 22 by ClusterFuzz, Sep 15 2014

Labels: Nag
horo@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz

Comment 23 by horo@chromium.org, Sep 15 2014

Status: Fixed
Project Member

Comment 24 by ClusterFuzz, Sep 15 2014

Labels: -Restrict-View-SecurityTeam -M-39 Merge-Triage M-37 M-38 Restrict-View-SecurityNotify
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-Requested label.

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
Labels: -Nag -Merge-Triage -M-37 Merge-Requested
Matthew - Merge requested for M38 (Branch 2125)
Labels: -Merge-Requested Merge-Approved
Is it typical to merge fixes for features that are behind a flag like onfetch?

Comment 28 by horo@chromium.org, Sep 24 2014

Labels: -Merge-Approved -M-38 M-39
We don't need to merge this to M38.
This problem is introduced by r290247 and fixed by r293697.
M38 branch cut point is r290040.

Labels: Release-0-M39
Project Member

Comment 30 by ClusterFuzz, Dec 22 2014

Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Project Member

Comment 31 by sheriffbot@chromium.org, 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
Project Member

Comment 32 by sheriffbot@chromium.org, 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
Labels: allpublic

Sign in to add a comment