New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Response filter escalation #535

Closed
jugglinmike opened this Issue May 2, 2017 · 13 comments

Comments

3 participants
@jugglinmike
Contributor

jugglinmike commented May 2, 2017

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

  1. 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

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk May 2, 2017

Member

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).

Member

annevk commented May 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jugglinmike

jugglinmike May 2, 2017

Contributor

Step 3.3.2 sets actualResponse:

  1. 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:

  1. Return response.

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

Contributor

jugglinmike commented May 2, 2017

Step 3.3.2 sets actualResponse:

  1. 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:

  1. Return response.

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

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk May 2, 2017

Member

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.

Member

annevk commented May 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jugglinmike

jugglinmike May 2, 2017

Contributor

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.

Contributor

jugglinmike commented May 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview May 2, 2017

Member

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".

Member

wanderview commented May 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview May 2, 2017

Member

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.

Member

wanderview commented May 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk May 3, 2017

Member

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.

Member

annevk commented May 3, 2017

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

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Jun 2, 2017

Member

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.

Member

wanderview commented Jun 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Jun 2, 2017

Member

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.

Member

wanderview commented Jun 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
Member

wanderview commented Jun 2, 2017

@wanderview

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Jun 8, 2017

Member

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

Member

wanderview commented Jun 8, 2017

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

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Jun 23, 2017

Member

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

Member

annevk commented Jun 23, 2017

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

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Sep 5, 2017

Member

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.

Member

annevk commented Sep 5, 2017

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.

@annevk annevk closed this Sep 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment