w3c / webappsec-suborigins

Suborigins
https://w3c.github.io/webappsec-suborigins/
Other
25 stars 9 forks source link

Default to withCredentials=true for CORS requests #33

Closed arturjanc closed 8 years ago

arturjanc commented 8 years ago

Many (most?) applications which currently use same-origin XHR do it to fetch authenticated resources and since "old-style" XHR is sent with cookies this works fine. However, putting an application into a suborigin upgrades such requests to CORS, meaning that they will default to withCredentials=false and cookies will not be sent.

This poses a problem for adopters because it means that any part of the code where XHR is sent will now have to explicitly set withCredentials=true for the application to function properly. Especially in places where JS libraries are used for XHR changing this can get tricky.

Can we upgrade CORS sent from suborigins to set withCredentials by default? This could potentially only be done for same-origin requests (though I assume this might be hairy), or could be behind an unsafe-* flag.

I believe doing this would have no security impact -- an attacker who has an XSS in a suborigin can already issue their own requests and manually set withCredentials=true. The only case I can think of is applications deliberately sending non-credentialed requests to endpoints which would behave differently if they received cookies, but I haven't seen cases like this so far.

mikewest commented 8 years ago

I'm a little concerned at the tradeoff here: we'd add some sort of automatically-tack-on-credentials-for-same-physical-origin-requests flag, whose benefit is basically that you don't have to type xhr.withCredentials = true;? That doesn't seem like a high bar, honestly. Where does it get complicated?

Moreover, what would this mean for preflights? Would you expect to be able to drop them for non-"safe" HTTP methods like POST, which wouldn't have sent a preflight in a pre-suborigin world?

annevk commented 8 years ago

Also, note that with fetch() you always have to define whether you want to include credentials, since by default they are omitted.

mikewest commented 8 years ago

Right. I assume Artur would appreciate the same behavior for fetch(), though it's less relevant since the applications that Suborigins are targeting are almost certainly ~old.

A related question, though, is what we'd do for things like the crossorigin attribute and any preflights it might generate.

joelweinberger commented 8 years ago

Whatever we decide to do (if anything), we could possible just attach the behavior to the unsafe-cookies flag, assuming it continues to exist. Since the premise with unsafe-cookies is more-or-less that cookies are shared with the physical origin, it seems reasonable to assume you'll probably want them shipped to other resources on the same physical origin.

devd commented 8 years ago

Playing with suborigins, we saw that jquery will just add a X-Requested-With header to same origin (as defined by URI) requests so setting crossDomain to true was mandatory anyhow, otherwise jQuery will slow down with a preflight for every XHR and the server typically doesn't know how to deal with the preflight. Since there were some changes that were needed anyhow, I vote holding off on this for now.

arturjanc commented 8 years ago

Thanks for the discussion, I'll try to answer the feedback in random (hopefully logical) order, starting with Mike's comments :)

@mikewest, I would still respect the crossorigin attribute if specified -- no change there. For preflights, similarly, there'd be no change -- the server which defines a Suborigin will have to properly handle preflights and set ACAO/ACAS headers to allow CORS requests made by the suborigin to be accepted.

The only problem this is trying to solve is the case of old-style same-origin XHR being "unexpectedly" upgraded to CORS when the application is being put on a subdomain, and losing cookies in the request. The benefit of defaulting to withCredentials=true in this case is that the app could put itself in a suborigin with purely server-side changes: 1) Add handling of preflights and set proper ACA* headers for requests from the suborigin, 2) Start setting the Suborigin header.

Requiring the application to refactor all its uses of XHR and set withCredentials to maintain existing behavior of sending cookies would introduce an additional non-trivial step of having to audit/change the application in all places which send XHRs. In many cases this would mean changing JS sourced by your app but not directly under your control, you'd have to deal with XHR wrappers which don't know CORS, etc. Alternatively, you could attempt to polyfill this, but then you have to change all renderable documents in your application which might send an XHR, which is also a big undertaking.

Compared to the purely server-side model as a "base case" for suborigins, I think that this added burden could be significant, especially since defaulting to withCredentials=false in this case has no security value.

arturjanc commented 8 years ago

@metromoxie I wouldn't be opposed to bundling this with unsafe-cookies, but mostly because I don't see any drawbacks of setting withCredentials=true in the first place. Note that this shouldn't be "unsafe" in any meaningful way because an attacker with an XSS in the suborigin can by definition issue such cookied requests.

@devd I believe the condition of adopting suborigins is that you have to make the server handle preflights and set ACA* headers. OTOH in our testing, we found that there were few client-side changes required (in some cases none) other than forcing withCredentials, which was necessary in every app. Did you notice other common situations where client-side code had to be changed?

Re: the slowdown introduced by the preflight, it can certainly be an issue and IIRC initially we threw around some ideas such as defining suborigins with a path to which XHRs could still be made without a preflight. But this is probably something that can be decoupled from this issue?

annevk commented 8 years ago

If these are actually meant to be treated as distinct origins, treating them same-origin for some APIs and even including credentials by default does seem rather dangerous. That encourages setups which make confused deputy attacks easier.

arturjanc commented 8 years ago

Suborigins are meant to be like distinct origins, but with two important caveats:

I would suggest that changing the withCredentials behavior (or bundling it with unsafe-cookies, or putting it behind a new unsafe-* flag) is in line with the points above: it will significantly facilitate adoption (no client-side code changes), but will not provide an XSS attacker with any additional capacity they would not otherwise have.

Something I would definitely be worried about is if turning on suborigins for a given application had the potential to introduce new security issues in that app. I believe in this case upgrading same-origin CORS requests to carry cookies would not have that effect.

annevk commented 8 years ago

What if the same-origin-cross-suborigin CORS request redirects to cross-origin? What about the server becoming more vulnerable to confused deputy attacks?

arturjanc commented 8 years ago

For an application that will adopt Suborigins the resources for which the credentialed CORS would be sent from the suborigin are resources that are already being requested with cookies via same-origin XHR. If the server doesn't purposefully alter its behavior to redirect those requests to cross-origin, there will be no change for the client (the request will return data as before). If there is a logic bug and such requests are now redirected to cross-origin and set withCredentials, the redirect URL is still under the control of the server so at most it would send a cookied request to an unexpected destination (rather than an attacker-controlled URL).

I understand the class of "confused deputy" bugs you're worried about is if the server redirected that now-credentialed request to a URL chosen by the attacker and that could lead to e.g. CSRF bugs elsewhere in the origin? This doesn't sound very likely to me in practice because we're not talking about arbitrary open redirects, but rather the exact endpoints from which the application is currently fetching data with XHR; and the data in the request is also not controlled by the attacker (it's the same request as the application used to send, but now it's CORS).

It also seems that any potentially worrying behavior like this -- if it were to occur -- would be caught when testing the deployment of suborigins for a given application.

annevk commented 8 years ago

Let's state it another way, if you open yourself up to same-origin-cross-suborigin CORS, how likely is it you open yourself up to cross-origin CORS?

And if it's a redirect already, you'd be changing how it works cross-origin, which seems suboptimal and more likely to break things in a way that is hard to fix (the other origin might not want to opt into credentials sharing).

arturjanc commented 8 years ago

I assume your first question is for server-side configuration, i.e. setting ACA* headers to allow requests from the suborigin? It definitely requires care, but note that this is mostly orthogonal to the withCredentials issue -- to keep the suborigined application from breaking, someone will have to set withCredentials so that same-(sub)origin CORS requests carry cookies (as they currently do with XHR), and the server will have to set response headers that will allow those requests. The main question is whether the addition of withCredentials can happen by default (e.g. behind an unsafe-* flag) or whether developers will be required to change their applications and add it manually.

Unless I'm misunderstanding, your second point is about applications which currently use XHR to fetch a resource that issues a 30x redirect. In this case, there are two possibilities:

  1. The redirect points to a same-origin resource, in which case the redirect is followed and cookies are sent. If we set withCredentials in this case the behavior will be identical (modulo required response headers).
  2. The redirect points to a cross-origin resource and the client doesn't set withCredentials. In this case if we "inject" withCredentials the request will fail unless the response sets ACAC (which it might not do). I believe the main negative consequence here would be that the request would be rejected so the application owner would have to address this before putting her app in a suborigin. (An alternative solution could be to respect withCredentials=false in suborigins if it's set explicitly, but if it was not defined default to true; would that be an improvement?)

FWIW, I'd be comfortable with case #2 being a blocker for suborigins. In case a bit of anecdata helps, in a review of several applications which would benefit from living in suborigins, I didn't encounter any cases of XHR requests leading to cross-origin redirects.

devd commented 8 years ago

hmm .. so make sure I am understanding right, a unsafe-something directive that says "default to withCredentials" to true will only mean that same physical origin XHRs will be withCredentials=True. Would that be enough?

I am surprised you are not concerned about the perf impact of preflights. I had to set crossDomain to true and withCredentials to False manually in jQuery to keep it from being confused. It does seem like you could do this in jQuery right now or whatever XHR library the code is using. So, it does seem weird for this to go into the spec when JS could fake it. I am all for adding things to the spec that help the JS fake it.

arturjanc commented 8 years ago

Yes, adding a flag like this would work. I am by no means married to the particular idea of changing the default withCredentials behavior of suborigin->physical-origin CORS, I was just -- likely incorrectly -- assuming this would be cleaner than another flag in this case.

Performance is a concern, but it's a fairly distant second to the ability to actually adopt/deploy suborigins, and requiring seemingly simple changes to client-side code is likely to be a big obstacle for many developers; I think we can point to CSP and the overwhelming number of users with unsafe-inline as an example. Also, many apps that I think would move to suborigins issue a relatively small number of XHRs (e.g. older apps, marketing pages/single-page apps) or could otherwise take the hit ("internal"/admin UIs). So while we should continue thinking about performance -- I'm certain it will be important sooner rather than later -- I wouldn't want to give up on making adoption as easy as possible in the meantime.

devd commented 8 years ago

that seems fair --- out of flexibility, I would prefer we don't marry it to another flag if @metromoxie is ok with it. I would also prefer not doing it just yet: I still think some JS magic can just do it for you in the preload.

joelweinberger commented 8 years ago

We probably will have to clean it up a bit, but we basically added support for this with 'unsafe-credentials' in #52, so I'm closing this up.