Response filter escalation

[jugglinmike]

Step 12 of Main fetch sets the request's response tainting and (generally) produces a response. The subsequent step 14 reads:

14. If response is not a network error and response is not a filtered response, then run these substeps: [...]

This was implemented as a solution for #23, but @wanderview and I are wondering if it is correct.

In particular, we're thinking of cases where response is a basic filtered response, but request's response tainting is "cors". This could occur, for example, when a Service Worker uses respondWith, providing a synthetic response (or a same-origin response). In cases like these, step 14's sub-steps will not be executed. This will produce a response to a cross-origin request that has "basic" filtering.

I'm not sure if this is a problem, given that the applied filter matches the response that is ultimately created, but the mismatch between the initial request's tainting and that final response's filtering (in particular: the relaxing of the filtering rules) seems like a potential information leak. If so, would it make sense to introduce a concept of filter "strength" such that step 14 could be applied in these "filter escalation" scenarios?

[annevk]

I don't think either of these apply as https://fetch.spec.whatwg.org/#concept-http-fetch 3.3.2 unwraps filters (a synthetic response does not have a filter to begin with though).

[jugglinmike]

Step 3.3.2 sets actualResponse:

2. Set actualResponse to response, if response is not a filtered response, and to response's internal response otherwise.

...but that value is only used in the subsequent application of CSP semantics (step 3.3.4) and later to make determinations about redirects (step 5). HTTP fetch ultimately returns response in step 6:

6. Return response.

Should that final step instead read "Return actualResponse."?

[annevk]

Ah yes, you're correct. I don't think we should change the last step, that would introduce security issues.

I don't think the current setup has a security issue though. Yes, you can request a "no-cors" cross-origin URL and get a "basic" response back, but as far as I can tell that's all safe.

[jugglinmike]

Doesn't it also mean that header information (and possibly authentication information) would be exposed for those cross-origin requests?

For context: Firefox currently un-wraps the response and re-wraps according to the "outer" request's tainting. Chromium follows the letter of the specification. I wrote a demonstration test here:

web-platform-tests/wpt@master...bocoup:fetch-filtering-escalation-demo

I haven't determined whether Firefox observes any sort of filter "precedence", or if it consistently re-wraps.

[wanderview]

I don't think the current setup has a security issue though. Yes, you can request a "no-cors" cross-origin URL and get a "basic" response back, but as far as I can tell that's all safe.

It is safe, but it seems a bit surprising to me (and I would guess devs). There is no way to get a non-opaque back from fetch(crossOriginURL, { mode: 'no-cors' }) normally. It would be nice to keep that consistent instead of exposing an exceptional case; "always opaque, unless a service worker intercepts and does something".

[wanderview]

I haven't determined whether Firefox observes any sort of filter "precedence", or if it consistently re-wraps.

I believe we will "increase" tainting, but not lower it. So a basic can be promoted to cors/opaque response. A cors response can be promoted to opaque response. An opaque response cannot be changed to another type.

[annevk]

Doesn't it also mean that header information (and possibly authentication information) would be exposed for those cross-origin requests?

No. Only headers set by the creator of the request.

It is safe, but it seems a bit surprising to me (and I would guess devs).

I'm pretty sure we discussed this model at length with @jakearchibald et al.

[wanderview]

I'm pretty sure we discussed this model at length with @jakearchibald et al.

Ok, but having looked at the implementation I really dislike this decision. I think a safe invariant is "tainting only ever goes up and never goes down". In order to implement this I have to allow tainting to be downgraded. This increases the risks of future security bugs in gecko (and maybe other browsers).

I'll implement this for compat, but I just want to note I'm doing it with objection.

[wanderview]

Also, FWIW I find 3.3.2 very difficult to understand:

https://fetch.spec.whatwg.org/#concept-http-fetch

3.3.2 sets actualResponse, but then 3.3.3 uses response instead. Then 3.3.4 uses actualResponse. Its also used in step 5, but otherwise response is used throughout. The way these variables are intermixed makes me wonder if there is a typo or if its all intended.

[wanderview]

Gecko bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1369862

[wanderview]

Fixed in firefox 55 which should release in around August 8, 2017. (I would close this issue, but I don't have perms.)

[annevk]

I should probably leave it open to make the steps you found difficult to understand more clear somehow?

[annevk]

I couldn't figure out a way to improve this. All I could think of was adding "Pay careful attention to how these variables are used" but that advice really applies whenever.