Skip to content
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

Add a bit to Opaque Responses to distinguish redirects #79

Closed
slightlyoff opened this issue Jul 13, 2015 · 36 comments
Closed

Add a bit to Opaque Responses to distinguish redirects #79

slightlyoff opened this issue Jul 13, 2015 · 36 comments

Comments

@slightlyoff
Copy link

(apologies if this is a dupe)

@sirdarckcat has brought up an issue (documented here: http://sirdarckcat.blogspot.de/2015/05/service-workers-secure-open-redirect.html ) that seems like it'd be mostly solved through the addition of a single bit to Response objects which notes if the content is the result of a redirect.

At the webappsec f2f today in Berlin, the group there seems to agree that exposing this bit on Responses doesn't leak any new information; it's observable via CSP + iframes already.

/cc @jakearchibald @jungkees @annevk @metromoxie @hillbrad

@annevk
Copy link
Member

annevk commented Jul 13, 2015

You can observe redirects with fetch(url, {redirect:"error"}) too. I guess this helps in the case where you have fetch(crossOriginURL, {mode:"no-cors"}) and there's potentially a redirect, because either way you'd get an opaque response back. And maybe in the fetch(sameOriginURL, {mode:"no-cors"}) case if you're only looking at the response and not the url.

It seems safer however to use fetch(url, {redirect:"error"}) as in that case you'll never get a response you didn't expect.

@slightlyoff
Copy link
Author

Forcing another set of RTT's to get just the single bit of data (that it was a redirect) is pretty bad, particularly in the cases where it might be a POST or non-idempotent operation.

@annevk
Copy link
Member

annevk commented Jul 14, 2015

What I'm saying is that you would always use redirect:"error". Not that you would use that to detect a redirect. (Using that and window:null would actually be more efficient for transmitting bodies too. As we can avoid teeing the body.)

What I'm wondering is why you would suddenly care about this when you get to a response while you don't care about it when making a request. What kind of scenario that helps.

@slightlyoff
Copy link
Author

No, I understand what you were saying; I don't think it's easy or reasonable. There are many systems that legitimately have redirects for "own content" but have off-origin redirects for navigation exfiltration; i.e. a secure open redirector to filter out referrer information.

@annevk
Copy link
Member

annevk commented Jul 15, 2015

Sure, but in that scenario you simply would not cache opaque responses.

@sirdarckcat
Copy link

sirdarckcat commented Jul 16, 2015 via email

@annevk
Copy link
Member

annevk commented Jul 16, 2015

Sure, but that's also a problem without redirects...

@sirdarckcat
Copy link

why is it a problem without redirects? in what other case would a cached
response be dangerous?

in this case it seems dangerous since most websites have open redirects, so
an attacker could fool a SW to cache a CORS-enabled Response object for a
same-origin Request.

@annevk
Copy link
Member

annevk commented Jul 19, 2015

Well, in that case you can already figure out you're redirected right? Since for "cors" responses you can just check their url.

@sirdarckcat
Copy link

The argument goes as follows:

  1. Many websites have open redirects (facebook, google, yahoo, outlook,
    etc..). Essentially everyone that uses OAuth or OIC protocols has an open
    redirect by design.
  2. It's likely that both, large and small sites will do cache-all
    policies. See this SW, for example:
    https://www.google.com/_/chrome/newtab-serviceworker.js

When developers code these SW, it's not expected for them to have to check
for redirects, because it's not really intuitive, and as a result would
create an XSS in that site.

@annevk
Copy link
Member

annevk commented Jul 19, 2015

Right, I understand the attack, don't worry, but if you look at OP it's suggesting to add a bit so you can figure out if the response is a redirect. You can already do this in most cases and if you don't want redirects to be followed you can accomplish that too, as I pointed out. It seems you want something else. It would help if you stated what you want.

@sirdarckcat
Copy link

sirdarckcat commented Jul 19, 2015 via email

@annevk
Copy link
Member

annevk commented Jul 19, 2015

We could certainly add restrictions to the Cache API, but this repository is about fetch() (and Fetch in general). I think if you use fetch() and you're worried about redirects, you ought to use fetch(url, {redirect:"error"}).

@sirdarckcat
Copy link

sirdarckcat commented Jul 19, 2015 via email

@annevk
Copy link
Member

annevk commented Jul 20, 2015

I'm pretty sure OP wants response.hasBeenRedirected (and it to work for all responses). (Making responses opaque doesn't really mitigate the attack I think.)

@sirdarckcat
Copy link

sirdarckcat commented Jul 21, 2015 via email

@annevk
Copy link
Member

annevk commented Jul 21, 2015

Why would you prefer making responses opaque over simply getting a network error? Or getting a "opaqueredirect" response when you're redirected?

@jakearchibald
Copy link
Collaborator

The attack demo isn't working (not https) so here's what I think the attack is, if I'm wrong, the rest of my post is nonsense.

  • Good site's SW will serve from cache if it can
  • Good site's SW will cache content it gets from the network, for a set of urls which includes an anywhere-redirect
  • Good site allows users to create a CORS request to the anywhere-redirect (via fetch/xhr indirectly, <img crossorigin> etc)
  • Evil player can use this to get their evil-site content cached against a good-site request
  • Navigating to this url will cause the evil-site content to be served as a response on the good-site origin

If this response contains script, it's running on the good-site origin.

If that's the attack, I don't see how a "this came from a redirect" bit can help, as this information is already in response.url, except in the case of opaque responses. That wouldn't work for navigations, although it would work for <script>. This would only be a problem if the good-site had a <script src> pointing to the global redirect that was pointing to the evil-site, so security would be broken without SW.

Disallowing cross-origin CORS responses in response to "same-origin" navigation requests would help mitigate the navigation case. @annevk - we were trying to think of why we might need this restriction.

Another possibility (although I'm not sure we need it) is a redirect mode of "follow-same-origin".

@wanderview
Copy link
Member

Disallowing cross-origin CORS responses in response to "same-origin" navigation requests would help mitigate the navigation case. @annevk - we were trying to think of why we might need this restriction.

This would require blocking synthetic responses for "same-origin" requests as well, right? Its trivial to convert a CORS response into a synthetic response.

@annevk
Copy link
Member

annevk commented Jul 27, 2015

Disallowing cross-origin CORS responses in response to "same-origin" navigation requests would help mitigate the navigation case.

Wait, how did you end up here from where you started? Does in this scenario the service worker override the default of not following redirects? Doesn't make much sense to me.

@annevk
Copy link
Member

annevk commented Jul 27, 2015

Another possibility (although I'm not sure we need it) is a redirect mode of "follow-same-origin".

Can you explain how this is different from setting mode to "same-origin" and redirect mode to "follow"?

@jakearchibald
Copy link
Collaborator

Wait, how did you end up here from where you started?

This was resolved in IRC, but for the record: The bad stuff goes into the cache via a cors request, and comes back out via a client request. The cache would end up with an entry where the key was "good site" and the value was "bad site". Then when the navigation happens, the "bad site" entry is pulled out.

Can you explain how this is different from setting mode to "same-origin" and redirect mode to "follow"?

I thought @slightlyoff was wanting to retain "legitimate" redirects. #79 (comment)

@jakearchibald
Copy link
Collaborator

There's a further question on whether we should allow entries in the cache where request.url has a different origin to response.url, unless the response is synthetic

@annevk
Copy link
Member

annevk commented Jul 27, 2015

I thought @slightlyoff was wanting to retain "legitimate" redirects.

Sure, but how is your proposal different in terms of processing from what I wrote?

There's a further question on whether we should allow entries in the cache where request.url has a different origin to response.url

As I said elsewhere, this is getting into nutty territory and doesn't defend against same-origin -> cross-origin -> same-origin.

Also, even if we solve this for navigation, that still leaves all other cases open, so I don't think that makes much sense.

@jakearchibald
Copy link
Collaborator

if we solve this for navigation, that still leaves all other cases open

What other code-executing cases are we worried about?

@annevk
Copy link
Member

annevk commented Jul 28, 2015

Hmm, maybe that is the only vector. Would be good if @sirdarckcat could confirm.

Perhaps the Cache API should return a network error if the input request has redirect mode not set to "follow" and the response is the result of a redirect.

@sirdarckcat
Copy link

The PoC in http://sirdarckcat.blogspot.de/2015/05/service-workers-secure-open-redirect.html should work (it's non-HTTPS but the iframe is SSL).

I can imagine a couple other possible attacks, but they depend on how the Cache.match algorithm behaves, which IMHO is where we should fix this.

@annevk
Copy link
Member

annevk commented Jul 28, 2015

@sirdarckcat you keep mentioning the match algorithm, but you stop at the point of explaining how you'd fix it there. Per the model I suggested in my previous comment?

@sirdarckcat
Copy link

Sorry, I probably should stop responding on my phone :)

Yes, your solution sounds reasonable, but may not be future-proof. I think this will also become a problem:
onfetch=function(e){event.respondWith(fetch(e.request))}

And has nothing to do with Cache API, however, exploiting that would require a mechanism for me to create a cors-enabled navigational request, which AFAIK is impossible today (but might become possible in the future, one imaginary use-case could be to enable cross-origin seamless iframes, for example, although it's a strawman argument since seamless iframes are long gone crbug.com/229421)

@annevk
Copy link
Member

annevk commented Jul 28, 2015

You would expect that due to the presence of seamless the request would go through the parent's service worker? It's still not clear to me how you'd exploit that though. The "cors" bit would only be used to allow seamless, not share a script environment.

@annevk
Copy link
Member

annevk commented Jan 26, 2016

So we're going to add response.redirected that does this.

We're also going to add a check to 3.3 of HTTP fetch. See w3c/ServiceWorker#737 (comment) for details.

@annevk annevk closed this as completed in e54f6bd Jan 27, 2016
@annevk annevk removed the needsinfo label Jan 27, 2016
@wanderview
Copy link
Member

@annevk
Copy link
Member

annevk commented Aug 5, 2016

The way the specification is written doesn't actually solve this issue as redirected would always return false for opaque responses. Surprised nobody noticed thus far, maybe we don't need this feature?

@annevk annevk reopened this Aug 5, 2016
@wanderview
Copy link
Member

We don't need the script exposed getter to return true to solve the original poster's issue. With the currently spec'd and implemented solution the linked exploit no longer works. You get a network error page because respondWith() fails due to the state of the internal redirected flag.

@annevk
Copy link
Member

annevk commented Aug 10, 2016

Mkay. I guess if someone feels strongly about this they should open a new issue.

@annevk annevk closed this as completed Aug 10, 2016
@horo-t
Copy link

horo-t commented Nov 29, 2016

chromium bug: https://crbug.com/669363

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

No branches or pull requests

6 participants