1173912 - FetchEvent.respondWith() for navigations should treat opaque Response as network errors

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

This is step 9.3.2 of respondWith() spec:

  https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#fetch-event-respondwith-method

Reads:

  "If response.type is "opaque", and request's associated request is a client request, set the respond-with error flag."

It doesn't look like we do this currently.

This will only matter once we land the patch in bug 1167808 to properly read the body of an opaque response.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I'd like to supersede this with step 2.2 case 3 of the fetch spec instead since that is more 'canonical' in the sense that it is more concerned with fetching requests than SW. Does not change the implementation. https://fetch.spec.whatwg.org/#concept-http-fetch

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Passing this to Nikhil per our meeting discussion.

Comment 3

[Daniel Veditz [:dveditz]]

Ben: what exploitable things can happen that made you call this a security bug? Are we passing the supposedly opaque data back to a worker that shouldn't get it?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Basically this check prevents an entire class of things that would need tainting otherwise.  For example:

1) window foo.com does new Worker('foo.com/sub');
2) SW intercepts with a fetch('bar.com/sub', { mode: 'no-cors' });
3) worker is opened because the principal is same origin, but the content is really cross-origin

Making client requests fail on opaque responses prevents this sort of thing.

I defer to you, though, if you think it should be a security bug or not.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Taking this back since Nikhil is on PTO next few days.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Fail opaque responses for client requests. r=ehsan

I believe this will fix the problem in this bug.  In order to write tests for it, though, I need to fix bug 1173934 so that errors are reported back.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Fail opaque responses for client requests. r=ehsan

Updated patch based on work in bug 1173934.  I will add a test in a second patch.

Comment 8

[(no longer active)]

Comment on attachment 8631257 [details] [diff] [review]
Fail opaque responses for client requests. r=ehsan

Review of attachment 8631257 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/InternalRequest.cpp
@@ +220,5 @@
> +  //
> +  // A worker request context is one of "serviceworker", "sharedworker", and
> +  // "worker".
> +  //
> +  // TODO: include equivalent check for "serviceworker" context

Service workers cannot be intercepted, so please remove this todo item.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I intend to file separate bugs for tests.  Landing based on local testing for now.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df6593df027b

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out (next to bug 1173934) for WPT4 and S4 oranges in https://hg.mozilla.org/integration/mozilla-inbound/rev/b6cebc649c49 :

https://treeherder.mozilla.org/logviewer.html#?job_id=11578077&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=11577037&repo=mozilla-inbound

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

New try build after fixing various automation failures:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=349ca56afcfa

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a4ac9e6d911

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a4ac9e6d911

This shouldn't have landed without a sec rating and/or sec-approval (if necessary). Can you please assign a rating so we can go from there at least?

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Sorry, this is my first sec bug.

Also, its unclear if this still needs to be marked as a sec bug at all.  We've disabled opaque interceptions by default in bug 1167808.  And before bug 1167808 opaque interceptions were broken, so this issue could not be taken advantage of either.

Based on that I would mark the bug sec-low or not a sec bug anymore.  I don't see where to adjust the sec rating, though.  Maybe I don't have permissions.

Ehsan, what do you think?

Comment 16

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #15)
> Also, its unclear if this still needs to be marked as a sec bug at all. 
> We've disabled opaque interceptions by default in bug 1167808.  And before
> bug 1167808 opaque interceptions were broken, so this issue could not be
> taken advantage of either.

The approval process <https://wiki.mozilla.org/Security/Bug_Approval_Process> says that bugs that only affect m-c can land without approval, and this bug was one such bug, so Ben was correct in landing it without asking for approval.

> Based on that I would mark the bug sec-low or not a sec bug anymore.  I
> don't see where to adjust the sec rating, though.  Maybe I don't have
> permissions.

This is sec-high based on the ratings here <https://wiki.mozilla.org/Security_Severity_Ratings> since it allows obtaining confidential data from websites.  (These security ratings are specified as bugzilla keywords.)  We typically consider the security risk of what would happen if the bug remained unfixed but in this case because of bug 1167808, this didn't have any practical implications even on trunk, so I'm adding the sec-audit keyword which I think captures what has happened here.

I think we can open this bug up now, but I'll leave that to the security team.