fix synthesized response tainting to make fetch spec

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments, 5 obsolete attachments)

2.27 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
1.70 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
6.92 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
1.36 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
We are currently failing a bunch of tests in fetch-response-tainting.https.html.  This is because we set tainting on the nsIChannel immediately and then only maybe increase it when the service worker intercepts it.

Instead the fetch spec is written to return the response produced by the service worker directly.  So its exact tainting should be used even if its lower than the original nsIChannel.

So a cross-origin no-cors request that is intercepted with a basic response would result in a basic response.  Today we return an opaque in this circumstance.

See this spec issue for more discussion:

https://github.com/whatwg/fetch/issues/535
(Assignee)

Comment 1

a year ago
Created attachment 8873969 [details] [diff] [review]
P1 Expose a SynthesizeServiceWorkerTainting() on nsILoadInfo. r=ckerschb
(Assignee)

Comment 2

a year ago
Created attachment 8873970 [details] [diff] [review]
P2 Make the FetchEvent.respondWith() synthesize the exact tainting on the outer nsIChannel. r=asuth
(Assignee)

Comment 3

a year ago
Created attachment 8873971 [details] [diff] [review]
P3 Update expectations on fetch-response-taint.https.html. r=asuth
(Assignee)

Comment 5

a year ago
Created attachment 8874593 [details] [diff] [review]
P4 Update test_fetch_event.html expectations for synthesized CORS response returned for outer no-cors request. r=asuth

There is still an issue with non-e10s to figure out.  We are getting DOM_BAD_URI in one of the WPT tests.
(Assignee)

Comment 6

a year ago
The non-e10s issue is bug 1370617.
Depends on: 1370617
(Assignee)

Comment 9

a year ago
Comment on attachment 8873969 [details] [diff] [review]
P1 Expose a SynthesizeServiceWorkerTainting() on nsILoadInfo. r=ckerschb

Christoph, this adds an nsILoadInfo API to set the tainting level.  Normally we only allow the tainting to get more restrictive, but here we actually need to set to an explicit level.  It can go from more-tainted to less-tainted.

The situation where this is allowed is:

1. Page controlled by a service worker does a cross-origin no-cors request
2. Service worker intercepts and passes a basic Response to FetchEvent.respondWith()
3. Resulting page should see the basic Response instead of an opaque Response

It works this way because in the spec the Response passed to respondWith() should be passed directly back to the outer window.  Our implementation, however, has to synthesize the Response back on to the original nsIChannel.  So we end up overwriting the original tainting value.
Attachment #8873969 - Flags: review?(ckerschb)
(Assignee)

Comment 10

a year ago
Comment on attachment 8873970 [details] [diff] [review]
P2 Make the FetchEvent.respondWith() synthesize the exact tainting on the outer nsIChannel. r=asuth

This patch uses the new nsILoadInfo API to override the tainting value when we synthesize the response on the nsIChannel.
Attachment #8873970 - Flags: review?(bugmail)
(Assignee)

Comment 11

a year ago
Created attachment 8875011 [details] [diff] [review]
P3 Update expectations on fetch-response-taint.https.html. r=asuth

Update the WPT test expectations.
Attachment #8873971 - Attachment is obsolete: true
Attachment #8875011 - Flags: review?(bugmail)
(Assignee)

Comment 12

a year ago
Comment on attachment 8874593 [details] [diff] [review]
P4 Update test_fetch_event.html expectations for synthesized CORS response returned for outer no-cors request. r=asuth

Update our mochitests to expect the new behavior.
Attachment #8874593 - Flags: review?(bugmail)
Attachment #8873970 - Flags: review?(bugmail) → review+
Comment on attachment 8874593 [details] [diff] [review]
P4 Update test_fetch_event.html expectations for synthesized CORS response returned for outer no-cors request. r=asuth

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

restating: the "cors" response is now properly exposed as "cors" because we are able to lower the tainting from "no-cors"/"opaque".
Attachment #8874593 - Flags: review?(bugmail) → review+
Comment on attachment 8875011 [details] [diff] [review]
P3 Update expectations on fetch-response-taint.https.html. r=asuth

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

Confirming that the removed failures were all cases of inability to lower tainting, specifically for (cross-origin) fetch requests against OTHER_ORIGIN (by a BASE_ORIGIN page):
* SW responds with "same-origin" mode, "omit" creds to page fetch with
  * "no-cors", all 3 credential types: could not be lowered from "opaque" to "basic"
  * "cors", all 3 credential types: could not be lowered from "cors" to "basic"
* SW responds with "same-origin" mode, "same-origin" creds to page fetch with
  * "no-cors", all 3 credential types: could not be lowered from "opaque" to "basic"
  * "cors", all 3 credential types: could not be lowered from "cors" to "basic"
* SW responds with (cors-approved) "cors" mode, "omit" creds to page fetch with
  * "no-cors", all 3 credential types: could not be lowered from "opaque" to "cors"
* SW responds with (cros-approved) "cors" mode, "include" creds to page fetch with
  * "no-cors", all 3 credential types: could not be lowered from "opaque" to "cors"
Attachment #8875011 - Flags: review?(bugmail) → review+
Comment on attachment 8873969 [details] [diff] [review]
P1 Expose a SynthesizeServiceWorkerTainting() on nsILoadInfo. r=ckerschb

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

Hey Ben, I am a little worried that someone will abuse that API. I would feel more confident to just add a method to the LoadInfo and then do the following on the callsite:
static_cast<mozilla::LoadInfo*>(loadInfo.get())->SynthesizeServiceWorkerTainting(tainting);

For example, we don't allow changing the security flags after creation of the loadInfo, but for one case we had to allow that. So we added a method CloneWithNewSecFlags [1]. I would prefer if we use similar semantics for your tainting mechanism. Please add a 'hands off!'-comment in LoadInfo.h that no one should use it unless they know exactly what they are doing!

(Please note that I will be on PTO starting tonight. I am adding f+ to this patch, you can consider this as an r+ if you incorporate my suggestion!)

[1] https://dxr.mozilla.org/mozilla-central/search?q=CloneWithNewSecFlags&redirect=false
Attachment #8873969 - Flags: review?(ckerschb) → feedback+
(Assignee)

Comment 16

a year ago
Created attachment 8875310 [details] [diff] [review]
P1 Expose LoadInfo::SynthesizeServiceWorkerTainting(). r=ckerschb

Updated per the suggestion in comment 15, so claiming r+.
Attachment #8873969 - Attachment is obsolete: true
Attachment #8875310 - Flags: review+
(Assignee)

Comment 17

a year ago
Created attachment 8875311 [details] [diff] [review]
P2 Make the FetchEvent.respondWith() synthesize the exact tainting on the outer nsIChannel. r=asuth
Attachment #8873970 - Attachment is obsolete: true
Attachment #8875311 - Flags: review+
(Assignee)

Comment 18

a year ago
I'm going to land this before bug 1370617.
No longer depends on: 1370617
(Assignee)

Comment 19

a year ago
Created attachment 8875313 [details] [diff] [review]
P3 Update expectations on fetch-response-taint.https.html. r=asuth

Rebase for new bug landing order.
Attachment #8875011 - Attachment is obsolete: true
Attachment #8875313 - Flags: review+
(Assignee)

Comment 20

a year ago
Created attachment 8875314 [details] [diff] [review]
P4 Update test_fetch_event.html expectations for synthesized CORS response returned for outer no-cors request. r=asuth
Attachment #8874593 - Attachment is obsolete: true
Attachment #8875314 - Flags: review+

Comment 21

a year ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/674e032ea90c
P2 Make the FetchEvent.respondWith() synthesize the exact tainting on the outer nsIChannel. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ecdf657881e
P3 Update expectations on fetch-response-taint.https.html. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fe11fc40572
P4 Update test_fetch_event.html expectations for synthesized CORS response returned for outer no-cors request. r=asuth

Comment 23

a year ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d61441b6594e
P1 Expose LoadInfo::SynthesizeServiceWorkerTainting(). r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d7d7b7c2d29
P2 Make the FetchEvent.respondWith() synthesize the exact tainting on the outer nsIChannel. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4d1d742563
P3 Update expectations on fetch-response-taint.https.html. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a7b65a51a0b
P4 Update test_fetch_event.html expectations for synthesized CORS response returned for outer no-cors request. r=asuth
(Assignee)

Updated

4 months ago
Blocks: 1467454
You need to log in before you can comment on or make changes to this bug.