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 2 users

Issue metadata

Status: Fixed
Owner:
Last visit 25 days ago
Closed: Jul 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 2
Type: Bug-Security



Sign in to add a comment
link

Issue 805496: Security: Self-update service worker to stay alive

Reported by yannic.b...@gmail.com, Jan 24 2018 Project Member

Issue description

VULNERABILITY DETAILS
Service workers can self update to keep at least one version running.
This is reproducible at least in Chrome and Firefox (Spec bug?).

REPRODUCTION CASE
1. Create index.html to load SW
     <!DOCTYPE html>
     <script>
       navigator.serviceWorker.register('/sw.js');
     </script>

2. Create sw.js, and make sure the file will change every time the SW updates.
One simple way of doing this is by adding the current timestamp to sw.js.
(I attached a simple Go program for this).
      // Time: {now}
      function wait(ms) {
        // This is only to show
        const i = setInterval(() => console.log('is alive'), 1000);
        return new Promise(resolve => setTimeout(() => {
          clearInterval(i);
          resolve();
        }, ms));
      }

      self.addEventListener('activate', event => {
        // Don't wait too long or the worker will be killed and this does not work.
        event.waitUntil(wait(120000).then(() => {
             self.registration.update();
          }));
      });

3. Open the html file (http://localhost:5000/index.html).
This will register the SW and fire the |activate| event, which will wait some time and then self-update the SW. Because we include a unique timestamp in sw.js, SW will update and fire the |activate| event on the new worker.
This can be repeated forever to keep at least one version running. You can verify this in chrome://serviceworker-internals

On a side note: There are other bugs which can be reproduced in Chrome with similar steps:
1. If you do these steps above and wait for some time, you will see that there are many |redundant| SWs registered. They are no longer running and will be gone after closing chrome.
2. If you refresh the page or open a new tab, chrome waits for the SW to be activated, even when there a no listeners for |fetch| registered, which results in a long loading time. This behavior is not limited to |fetch| events.
 

Comment 1 by yannic.b...@gmail.com, Jan 24 2018

Comment 2 by yannic.b...@gmail.com, Jan 24 2018

If you modify the activate event handler in sw.js like this, the SW will start every time the user opens chrome.

self.addEventListener('activate', event => {
  self.registration.sync.register('foo');
  // Don't wait too long or the worker will be killed and this does not work.
  event.waitUntil(wait(120000).then(() => {
    self.registration.update();
  }));
});

Comment 3 by falken@chromium.org, Jan 25 2018

Labels: Pri-1
Status: Available (was: Unconfirmed)
Thanks for the detailed report and I agree this is important. This is a known issue but I guess we didn't have a bug yet. There might be discussion on the spec. I didn't think of the background sync aspect too.

We had a similar issue with postMessage/fetch event where we clamp down on the time allotted for the event to make sure you can't recursively extend your time-to-live.

We might consider doing this here for update() but it seems a bit tricky and there are possibly holes. An simple solution could be to disallow update() when there are no open clients.

Comment 4 by falken@chromium.org, Jan 25 2018

Actually I'm not sure about the Background Sync mechanism in c#2. It looks like sync.register() should fail if the service worker has no clients?
https://developers.google.com/web/updates/2015/12/background-sync#permissions

Comment 5 by yannic.b...@gmail.com, Jan 25 2018

It looks like I was a little too fast and the background sync part only works if Chrome is closed when there is a client or shortly after.

We could introduce a global time-to-live mechanism in ServiceWorkerRegistration and also kill all versions if the global TTL expired. We would also have to make sure that install/activate event does not extend that time, or only if there is a client attached.

We should have a spec discussion whether we want to disallow update() when there are no clients, or the calling worker is not activated. Do we have data if sites are doing this?

Comment 6 by falken@chromium.org, Jan 26 2018

Yeah, the install/activate interaction is worrisome. Typically these each grant 5 minutes of runtime, which is because we expect them to be doing somewhat heavy async things.

It seems odd if update() from a worker spawns a worker that doesn't get that time. We might consider clamping only initial script evaluation time, but that is not an effective mitigation.

Ben Kelly of Mozilla had a great idea. We could delay self update() by a increasingly large amount, eventually letting the worker die before the update starts (and aborting the update request). The delay would be reset to 0 once there is a client for the registration.

I don't see downsides to implementing this now, it should be transparent to devs and only impact sites doing a strange likely malicious thing. WDYT?

Comment 7 by falken@chromium.org, Jan 26 2018

One slight worry is a worker that does update() in push and expecting it to work even without clients.

Another possibility is the delay resets once the worker gets an event that's not an install/active event. Essentially, one event grants you the right to update, and we allow limited update otherwise that eventually dies.

Comment 8 by falken@chromium.org, Jan 26 2018

Cc: kinuko@chromium.org shimazu@chromium.org horo@chromium.org

Comment 9 by falken@chromium.org, Jan 26 2018

Yannic, would you be interested in doing implementation for this?

Comment 10 by yannic.b...@gmail.com, Jan 28 2018

Owner: yannic.b...@gmail.com
I like the version in #7 better because workers calling update() in push (or periodic sync?) will still work.

Implementing this sounds interesting. I'll give it a try!

Comment 11 by yannic.b...@gmail.com, Jan 28 2018

Status: Assigned (was: Available)

Comment 12 by falken@chromium.org, Jan 29 2018

That's great news. Thanks!

One complication is the new S13nServiceWorker implementation doesn't route some events to the browser process. But those events are only fetch events or (to be implemented) message events, so those imply a client exists.  So we might consider doing both: reset the delay when there is a client or when an event comes in.

Comment 13 by mea...@chromium.org, Jan 29 2018

Your friendly security sheriff here: I don't fully understand the implications of this bug to assign it a severity rating. Is keeping a SW always running worse than having a background tab with the same site?

Comment 14 by falken@chromium.org, Jan 30 2018

Yes, it's worse because the SW keeps running even after the tab is closed.

Our basic guarantee is the SW will go away soon after all tabs using it are closed. More technically, SW lifetime is based on events. Sites usually can't generate events to keep the SW alive when there are no tabs open. There are some exceptions like push messages, but these have been carefully designed to prevent abuse. E.g., a push message must result in a notification, so the user knows the site is being used in the background, and can revoke permissions if desired.

This is a class of bug where a site/SW can keep itself alive in a way we didn't really want it to. Previous related issues include https://bugs.chromium.org/p/chromium/issues/detail?id=647943 and https://bugs.chromium.org/p/chromium/issues/detail?id=648836

The Service Worker Security FAQ touches upon this in some sections:
https://chromium.googlesource.com/chromium/src/+/master/docs/security/service-worker-security-faq.md#do-service-workers-live-forever
https://chromium.googlesource.com/chromium/src/+/master/docs/security/service-worker-security-faq.md#do-service-workers-keep-running-after-i-close-the-tab

Comment 15 by palmer@chromium.org, Jan 30 2018

Cc: slightlyoff@chromium.org
Labels: M-65 Security_Impact-Stable Security_Severity-Low OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
This bug is a violation of what the FAQ promises:

https://chromium.googlesource.com/chromium/src/+/master/docs/security/service-worker-security-faq.md#Do-Service-Workers-keep-running-after-I-close-the-tab

If necessary, please update the FAQ as part of any fix.

I'm going to call that Low severity, per https://chromium.googlesource.com/chromium/src/+/master/docs/security/severity-guidelines.md.

Has there been a Firefox bug filed? It sounds like they know, but I want to be sure.

Comment 16 by yannic.b...@gmail.com, Jan 30 2018

Yes, a Firefox bug has been filled: https://bugzilla.mozilla.org/show_bug.cgi?id=1432846

Comment 17 by sheriffbot@chromium.org, Jan 31 2018

Project Member
Labels: -Pri-1 Pri-2

Comment 18 by yannic.b...@gmail.com, Feb 6 2018

Status: Started (was: Assigned)
A few things I noticed working on this:

- We shouldn't reset the delay on postMessage if it is coming from a service worker to prevent workers from resetting the delay.
- Should we store the delay in ServiceWorkerDatabase?

Comment 19 by sheriffbot@chromium.org, Apr 18 2018

Project Member
Labels: -M-65 M-66

Comment 20 by sheriffbot@chromium.org, May 30 2018

Project Member
Labels: -M-66 M-67

Comment 22 by sheriffbot@chromium.org, Jul 25 2018

Project Member
Labels: -M-67 Target-68 M-68

Comment 23 by bugdroid1@chromium.org, Jul 25 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b86a5cf3e919f0819589d056493278527771943e

commit b86a5cf3e919f0819589d056493278527771943e
Author: Yannic Bonenberger <contact@yannic-bonenberger.com>
Date: Wed Jul 25 14:16:13 2018

[ServiceWorker] Delay update() from workers without controllers

This CL delays the execution of update() for an increasing amount
of time if the calling worker doesn't control a client. The delay
is reset every time a controller is added to the worker, or an
event is dispatched. postMessage from service workers,
|install| and |activate| don't reset the delay.

Bug:  805496 
Change-Id: I9c25ba4315ce6a915634ecdf6405db8774c40929
Reviewed-on: https://chromium-review.googlesource.com/900763
Commit-Queue: Yannic Bonenberger <contact@yannic-bonenberger.com>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577875}
[modify] https://crrev.com/b86a5cf3e919f0819589d056493278527771943e/content/browser/service_worker/service_worker_consts.cc
[modify] https://crrev.com/b86a5cf3e919f0819589d056493278527771943e/content/browser/service_worker/service_worker_consts.h
[modify] https://crrev.com/b86a5cf3e919f0819589d056493278527771943e/content/browser/service_worker/service_worker_object_host.cc
[modify] https://crrev.com/b86a5cf3e919f0819589d056493278527771943e/content/browser/service_worker/service_worker_registration.h
[modify] https://crrev.com/b86a5cf3e919f0819589d056493278527771943e/content/browser/service_worker/service_worker_registration_object_host.cc
[modify] https://crrev.com/b86a5cf3e919f0819589d056493278527771943e/content/browser/service_worker/service_worker_registration_object_host.h
[modify] https://crrev.com/b86a5cf3e919f0819589d056493278527771943e/content/browser/service_worker/service_worker_registration_unittest.cc
[modify] https://crrev.com/b86a5cf3e919f0819589d056493278527771943e/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/b86a5cf3e919f0819589d056493278527771943e/content/browser/service_worker/service_worker_version_unittest.cc
[delete] https://crrev.com/7beedc820bff35f6f06074d8d9fb66b27f48ca02/third_party/WebKit/LayoutTests/http/tests/serviceworker/update-no-controllee.https-expected.txt

Comment 24 by mea...@chromium.org, Jul 27 2018

Is this fixed now?

Comment 25 by yannic.b...@gmail.com, Jul 27 2018

Labels: CVE-2018-5179
Verified that this is fixed in 70.0.3504.0

(CVE from Firefox https://bugzilla.mozilla.org/show_bug.cgi?id=1432846)

Comment 26 by falken@chromium.org, Jul 30 2018

Labels: -M-68 -Target-68 Target-70 M-70
Status: Fixed (was: Started)
Thanks Yannic!

Comment 27 by falken@chromium.org, Jul 30 2018

Cc: wanderview@chromium.org
+wanderview FYI

Comment 28 by sheriffbot@chromium.org, Jul 30 2018

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 29 by awhalley@chromium.org, Jul 30 2018

Labels: reward-topanel

Comment 30 by awhalley@chromium.org, Aug 6

Labels: -reward-topanel reward-unpaid reward-500
*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************

Comment 31 by awhalley@chromium.org, Aug 6

Cc: awhalley@chromium.org
Thanks for the report, yannic.bonenberger@! The VRP panel decided to award $500 for this. A member of our finance team will be in touch to arrange payment. Also, how would you like to appear on the Chrome release notes?

Comment 32 by awhalley@chromium.org, Aug 6

Labels: -reward-unpaid reward-inprocess

Comment 33 by yannic.b...@gmail.com, Aug 7

Wow, thanks, awhalley@! I did not expect that this qualifies for a reward. You can contact me at the email I use in this bug tracker to arrange what happens with the money.

You can use "Yannic Bonenberger" in the release notes.

Comment 34 by awhalley@google.com, Oct 15

Labels: Release-0-M70

Comment 35 by awhalley@google.com, Oct 15

Labels: CVE_description-external

Comment 36 by sheriffbot@chromium.org, Nov 5

Project Member
Labels: -Restrict-View-SecurityNotify allpublic
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

Sign in to add a comment