CorsFilter's Vary: Origin still a bit broken, can poison a cache to deny access

[Gunnar Brand <Gu...@interface-projects.de>]

Hello.

Due to external input and internal development I recently had to work with CORS and the Tomcat CorsFilter and encountered the issues
https://bz.apache.org/bugzilla/show_bug.cgi?id=61101 - CorsFilter should add Vary header to response
https://bz.apache.org/bugzilla/show_bug.cgi?id=62343 - CORS security: reflecting any origin header value when configured to * is dangerous
It seems these issues have been fixed and we told our customer to update to the latest release (contains at least fix for 61101) to get the “Vary: Origin” header of which absence they complained about.

But we were still getting complaints from the customer, and looking at the source code it seems “Vary: Origin” is only set if the request contains an origin header. I don’t believe this is sufficient. The spec states:

6.4 Implementation Considerations
This section is non-normative.
Resources that wish to enable themselves to be shared with multiple Origins but do not respond uniformly with "*" must in practice generate the Access-Control-Allow-Origin header dynamically in response to every request they wish to allow. As a consequence, authors of such resources should send a Vary: Origin HTTP header or provide other appropriate control directives to prevent caching of such responses, which may be inaccurate if re-used across-origins.

So if the same resource is accessed without origin (CORSRequestType.NOT_CORS) or origin is local (also CORSRequestType.NOT_CORS, i.e. the origin is discarded right now and no Vary: Origin set) and later with non-local origin, a caching layer in between can still be confused.
Even worse, there are user agents (Edge, older Firefox) that do html form posts w/o origin header at all and thus could be actively used to poison the cache (aside from doing worse to the servlet handling the call): https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10482384/

A confused/poisoned cache will return the local origin response to all callers thus effectively denying any non-local client access since the access control headers will be missing. This has affected people before:
- https://bugs.chromium.org/p/chromium/issues/detail?id=409090
- https://stackoverflow.com/questions/44800431/caching-effect-on-cors-no-access-control-allow-origin-header-is-present-on-th

I believe that for all URLs the CorsFilter matches, any unique Origin header’s value is a variant, since the returned access control headers differ. But also its absence is a variant (allowed headers are missing altogether) and thus warrants a “Vary: Origin”. I.e. always set this header unless it’s an invalid CORS request. I had to write a little filter that is configured next to the CorsFilter and checks for the missing “Vary: Origin” header to add it when being called by CorsFilter in the filter chain.


That aside, setting “Vary: Origin” is of course very bad for caching. But then the CorsFilter should probably only be mapped to APIs called remotely.
As improvement, since with Bug 62343 allowed.origins = ‘*’ will always set Access-Control-Allow-Origin to '*', if any origin is allowed the filter could help caching by:
- always setting the access control response headers, i.e. even if not a CORS request (should not have negative effects)
- not setting “Vary: Origin”
This would probably greatly improve caching for public APIs that do not rely on credentials.
Kind of discussed here: https://bugs.eclipse.org/bugs/show_bug.cgi?id=443530 

A single allowed origin is similar, but alas Access-Control-Allow-Origin headers will differ for this origin and the local origin (set to allowed host / missing) and thus cannot be improved the same way.

Sincerely,
Gunnar Brand

--
Gunnar Brand | Softwareentwickler
interface projects GmbH | www.intergator.de
Zwinglistraße 11/13 | 01277 Dresden | Deutschland
Tel: +49-351-31809-41 | Fax: +49-351-31809-33

Folgen Sie intergator auf:
Twitter : https://twitter.com/intergator
xing    : https://www.xing.com/companies/interfaceprojectsgmbh
LinkedIn: http://www.linkedin.com/company/interface-projects-gmbh
YouTube : http://www.youtube.com/user/TheEnterpriseSearch?feature=watch
Facebook: https://www.facebook.com/intergator/
RSS     : https://www.intergator.de/feed/

Geschäftsführer: Dr. Uwe Crenze, Eduard Daoud
Amtsgericht Dresden HRB 8740

Haftungsausschluss / Disclaimer
http://www.intergator.de/email-disclaimer.shtml




---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: CorsFilter's Vary: Origin still a bit broken, can poison a cache to deny access

[Mark Thomas <ma...@apache.org>]

On 06/07/18 08:24, Mark Thomas wrote:

<snip/>

> Agreed. That should be an easy change to make. I'll also review the code
> to see if we use the Vary header anywhere else that needs a similar tweak.

<snip/>

There are a couple of places where we modify the Vary header, both for
compression. The DefaultServlet and CompressionConfig.

My initial thought was to create a utility class to reduce the
duplication (since we'd need a third instance of this code for CORS).
Then I looked at the code more closely. The two implementations we
currently have each handle a slightly different set of edge cases. As I
started to combine the two, I thought of a few more edge cases that
weren't handled by either.

What started of as a simple reduce duplication of a few lines got rather
more complex.

Where I ended up was with a new utility class but one that handles all
the edge cases I can think off as well as tidying up after any
applications that modify the Vary header but don't handle all the edge
cases.

Commits to follow.

Mark


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: CorsFilter's Vary: Origin still a bit broken, can poison a cache to deny access

[Mark Thomas <ma...@apache.org>]

On 05/07/18 18:13, Gunnar Brand wrote:

<snip/>

> I believe that for all URLs the CorsFilter matches, any unique Origin header’s value is a variant, since the returned access control headers differ. But also its absence is a variant (allowed headers are missing altogether) and thus warrants a “Vary: Origin”. I.e. always set this header unless it’s an invalid CORS request.

Agreed. That should be an easy change to make. I'll also review the code
to see if we use the Vary header anywhere else that needs a similar tweak.

<snip/>

> That aside, setting “Vary: Origin” is of course very bad for caching. But then the CorsFilter should probably only be mapped to APIs called remotely.
> As improvement, since with Bug 62343 allowed.origins = ‘*’ will always set Access-Control-Allow-Origin to '*', if any origin is allowed the filter could help caching by:
> - always setting the access control response headers, i.e. even if not a CORS request (should not have negative effects)
> - not setting “Vary: Origin”
> This would probably greatly improve caching for public APIs that do not rely on credentials.
> Kind of discussed here: https://bugs.eclipse.org/bugs/show_bug.cgi?id=443530

My initial thoughts are that this makes sense but I do want to think
this through to make sure I haven't missed anything.

Thanks for your report.

Mark

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: CorsFilter's Vary: Origin still a bit broken, can poison a cache to deny access

[Mark Thomas <ma...@apache.org>]

On 05/07/18 18:13, Gunnar Brand wrote:

<snip/>

> That aside, setting “Vary: Origin” is of course very bad for caching. But then the CorsFilter should probably only be mapped to APIs called remotely.
> As improvement, since with Bug 62343 allowed.origins = ‘*’ will always set Access-Control-Allow-Origin to '*', if any origin is allowed the filter could help caching by:
> - always setting the access control response headers, i.e. even if not a CORS request (should not have negative effects)
> - not setting “Vary: Origin”
> This would probably greatly improve caching for public APIs that do not rely on credentials.
> Kind of discussed here: https://bugs.eclipse.org/bugs/show_bug.cgi?id=443530 
> 
> A single allowed origin is similar, but alas Access-Control-Allow-Origin headers will differ for this origin and the local origin (set to allowed host / missing) and thus cannot be improved the same way.

I've been looking at how Tomcat's CORS Filter interacts with caching and
have reached the following conclusions.

Caches automatically differentiate by HTTP method and most only consider
caching GET but also cache HEAD and OPTIONS.

INVALID responses are never cached since they do not meet the
requirements of RFC 7234 section 3.

That leaves SIMPLE, PRE_FLIGHT, ACTUAL and NOT_CORS to consider.

For GET or HEAD when anyOriginAllowed is true,
SIMPLE and ACTUAL add the following headers:
- Access-Control-Allow-Origin
- Access-Control-Expose-Headers
PRE_FLIGHT is not applicable to these methods
NOT_CORS adds the following headers:
- None

Therefore, for cache friendliness, we should always add the following
headers for GET and HEAD when anyOriginAllowed is true:
- Access-Control-Allow-Origin
- Access-Control-Expose-Headers

For OPTIONS when anyOriginAllowed is true,
SIMPLE and ACTUAL add the following headers:
- Access-Control-Allow-Origin
- Access-Control-Expose-Headers
PRE_FLIGHT adds the following headers
- Access-Control-Max-Age
- Access-Control-Allow-Methods
- Access-Control-Allow-Headers
NOT_CORS adds the following headers:
- None

With PRE_FLIGHT the response also varies with:
- Access-Control-Request-Headers
- Access-Control-Request-Method

Therefore, for cache correctness we should add
Access-Control-Request-Headers and Access-Control-Request-Method to the
Vary header for all OPTIONS requests.

Therefore, for cache friendliness, we should add the following headers
for OPTIONS requests when anyOriginAllowed is true:
- Access-Control-Allow-Origin
- Access-Control-Expose-Headers
- Access-Control-Max-Age
- Access-Control-Allow-Methods
- Access-Control-Allow-Headers


A sanity check on the above would be appreciated. I hope to implement
the above changes early next week.

Mark

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org