Closed Bug 1173912 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 --- unaffected
firefox41 --- unaffected
firefox42 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

(Keywords: sec-audit, sec-high, Whiteboard: [b2g-adv-main2.5-])

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Blocks: 1167808
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
Passing this to Nikhil per our meeting discussion.
Assignee: bkelly → nsm.nikhil
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?
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
Taking this back since Nikhil is on PTO next few days.
Assignee: nsm.nikhil → bkelly
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.
Depends on: 1173934
No longer blocks: 1167808
Updated patch based on work in bug 1173934.  I will add a test in a second patch.
Attachment #8627876 - Attachment is obsolete: true
Attachment #8631257 - Flags: review?(ehsan)
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.
Attachment #8631257 - Flags: review?(ehsan) → review+
I intend to file separate bugs for tests.  Landing based on local testing for now.
Blocks: 1183313
New try build after fixing various automation failures:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=349ca56afcfa
Flags: needinfo?(bkelly)
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?
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(bkelly)
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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?
Flags: needinfo?(bkelly) → needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
Keywords: sec-audit, sec-high
Group: core-security
Whiteboard: [b2g-adv-main2.5-]
You need to log in before you can comment on or make changes to this bug.