Hook up the other Background Fetch Mojo methods with the system

Hook up the other Background Fetch Mojo methods with the system

All methods made available through the Mojo interface will now be
appropriately validated and routed through the Background Fetch system.

BUG=692534
Review-Url: https://codereview.chromium.org/2777183002
Cr-Commit-Position: refs/heads/master@{#460104}
Committed: https://chromium.googlesource.com/chromium/src/+/16b9360d3fce4035802c4299fd2c8373d827cb53

[Peter Beverloo, 2017-03-27 16:24:04]

peter@chromium.org changed reviewers:
+ harkness@chromium.org, tsepez@chromium.org

[Peter Beverloo, 2017-03-27 16:24:05]

+harkness for review
+tsepez FYI

This needs some more work to improve deterministic timing of when we resolve the
promises.

[harkness, 2017-03-27 16:34:12]

lgtm % comments

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_job_controller.h (right):

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.h:60: //
Returns the registration id on behalf this job is fetching data.
micro nit: s/on behalf/for which

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
File
content/browser/background_fetch/background_fetch_job_controller_unittest.cc
(right):

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:171:
did_abort_by_developer_ = true;
Shouldn't this be did_abort_by_developer_ = aborted_by_developer?

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_service_impl.cc (right):

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_service_impl.cc:146:
callback.Run(blink::mojom::BackgroundFetchError::NONE,
return INVALID_TAG?

[Tom Sepez, 2017-03-27 18:43:44]

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_context.h (right):

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.h:63: int64_t
service_worker_registration_id,
My one comment (and this applies to the other CL as well) is whether these IDs
are deterministic and can be guessed by a compromised renderer from another
origin, and whether this results in a problem with the data integrity of the
service (rather than just, say, shutting down each other's fetches).

[Peter Beverloo, 2017-03-27 21:06:38]

On 2017/03/27 18:43:44, Tom Sepez wrote:
>
https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
> File content/browser/background_fetch/background_fetch_context.h (right):
> 
>
https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
> content/browser/background_fetch/background_fetch_context.h:63: int64_t
> service_worker_registration_id,
> My one comment (and this applies to the other CL as well) is whether these IDs
> are deterministic and can be guessed by a compromised renderer from another
> origin, and whether this results in a problem with the data integrity of the
> service (rather than just, say, shutting down each other's fetches).

I think we're good here. Assuming an attacker finds an {origin,
service_worker_registration_id} pair, given the Mojo endpoints, they could:

    * Fetch

Start a background fetch on behalf of another origin. They won't get access to
the data, because the JavaScript events will be independently dispatched by the
BackgroundFetchEventDispatcher.

    * UpdateUI

Update the notification title of an existing in-progress Background Fetch. Might
trick a user into clicking, but the click event will also be delivered to the
actual origin.

    * Abort

Stop an in-progress Background Fetch. Annoying, but no data and/or events are
being shared in this case.

    * GetRegistration

This arguably is the worst one. When they know the `tag` (registration
identifier) of a Background Fetch in addition to the {origin,
service_worker_registration_id} pair, they can get the developer-provided icons,
title and total download size. All immutable. Then again, if they go through
these lengths to get that data, they may as well look in the source code of the
site where I'd expect the useful bits of that to be defined.

    * GetTags

List of tags, could help a potential GetRegistration attack.

In regards to mitigation opportunities, it's not entirely straightforward. The
Service Worker does not have to run in order for these methods to be available,
which means we're pretty much dependent on Site Isolation for anything more
sophisticated.

[Tom Sepez, 2017-03-27 21:48:02]

lgtm

[Peter Beverloo, 2017-03-28 13:34:13]

The CQ bit was checked by peter@chromium.org to run a CQ dry run

[Peter Beverloo, 2017-03-28 13:34:23]

Thanks both!

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_context.h (right):

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.h:63: int64_t
service_worker_registration_id,
On 2017/03/27 18:43:44, Tom Sepez wrote:
> My one comment (and this applies to the other CL as well) is whether these IDs
> are deterministic and can be guessed by a compromised renderer from another
> origin, and whether this results in a problem with the data integrity of the
> service (rather than just, say, shutting down each other's fetches).

Acknowledged.

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_job_controller.h (right):

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.h:60: //
Returns the registration id on behalf this job is fetching data.
On 2017/03/27 16:34:12, harkness wrote:
> micro nit: s/on behalf/for which

Done.

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
File
content/browser/background_fetch/background_fetch_job_controller_unittest.cc
(right):

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:171:
did_abort_by_developer_ = true;
On 2017/03/27 16:34:12, harkness wrote:
> Shouldn't this be did_abort_by_developer_ = aborted_by_developer?

Done.

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_service_impl.cc (right):

https://codereview.chromium.org/2777183002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_service_impl.cc:146:
callback.Run(blink::mojom::BackgroundFetchError::NONE,
On 2017/03/27 16:34:12, harkness wrote:
> return INVALID_TAG?

Oops! Done.

[commit-bot: I haz the power, 2017-03-28 13:34:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 15:05:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 15:05:55]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-03-28 15:09:14]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-03-28 15:09:15]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
harkness@chromium.org
Link to the patchset: https://codereview.chromium.org/2777183002/#ps20001
(title: "Hook up the other Background Fetch Mojo methods with the system")

[commit-bot: I haz the power, 2017-03-28 15:09:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 15:14:35]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490713754843870,
"parent_rev": "92255dae474d7290f2fed43c03eeaea1258918d6", "commit_rev":
"16b9360d3fce4035802c4299fd2c8373d827cb53"}

[commit-bot: I haz the power, 2017-03-28 15:15:30]

Description was changed from

==========
Hook up the other Background Fetch Mojo methods with the system

All methods made available through the Mojo interface will now be
appropriately validated and routed through the Background Fetch system.

BUG=692534
==========

to

==========
Hook up the other Background Fetch Mojo methods with the system

All methods made available through the Mojo interface will now be
appropriately validated and routed through the Background Fetch system.

BUG=692534

Review-Url: https://codereview.chromium.org/2777183002
Cr-Commit-Position: refs/heads/master@{#460104}
Committed:
https://chromium.googlesource.com/chromium/src/+/16b9360d3fce4035802c4299fd2c...
==========

[commit-bot: I haz the power, 2017-03-28 15:15:31]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/16b9360d3fce4035802c4299fd2c...