Review CrossOriginFilter

[sbordet]

CrossOriginFilter was written many years ago; the comments show references to Chrome 5 (!) that may not be up to date anymore.

In particular:

• Remove the disabling of the filter for WebSocket upgrades - those should be checked too.
• Verify that the model is still valid, especially for WebSocket. The current model is: inspect request, add (or not) CORS headers, then let the request go to the application. This assumes that the client will do the response CORS headers check, and fail the request on the client if the response CORS headers are missing or signal failure. However, this may not be the case for WebSocket, so that a browser receiving a 101 (although without the CORS headers and therefore invalid from the CORS point of view) may establish the WebSocket communication anyway, instead of failing the upgrade.
• Investigate the possibility (if the model above is wrong) to have a StrictCrossOriginFilter that does not let the request that fails the CORS check go to the application, and reply with 403 or something alone those lines.
• Verify that the implementation matches the latest CORS spec.

[Narasimman]

I'm working on adding CrossOriginFilter to a high traffic application and I noticed few other issues as well. Some are very outdated and not performant for large scale applications.

For example,

1. I see that the filter tries to run through the allowedOrigins patterns and checks if there is a match.
   This is done for each request where the pattern is compiled for every item in the list. This is not very performant for a large list.

2. The regex pattern matcher is created only if the pattern has a star (*). What if i have a valid pattern without * in it. Eg: mg[\d]{3}.example.com for a list of 1000 domains.

3. It would be better if there is a way to inject the whitelist into the filter through a config file without requiring to restart the servlets.

So, Could I take this up and work on this Filter as it would help for my application and the community as well.

[sbordet]

@Narasimman you are welcome to contribute on this issue. Please make sure that you follow the Jetty Contribution Guidelines here.

Would be great if you can work on each feature singularly, issue a pull request, and I'll review it.

Regarding your bullet 2, the regexp is created anyway so mg[\d]{3}.example.com is converted to mg[\d]{3}\.example\.com.

I would prefer to not have to lookup any file for configuration - they are always a mess and source of security issues; your bullet 3 should be done with JMX.

So would be great if you can work on your bullet 1, my first bullet (remove skipping WebSocket upgrades) and your bullet 3 but with JMX - please on separate pull requests.

Thanks !

[freetom]

I think this should also have the "Security" label and a high priority, as bypassing the CORS would allow other sitest to perform arbitrary actions on the behalf of logged users (that visit the malicious site), therefore potentially leaking data and performing unwanted actions.

Also, to allow any origin by default is a pretty unsafe configuration:
jetty-servlets/src/main/java/org/eclipse/jetty/servlets/CrossOriginFilter.java:151: private static final String DEFAULT_ALLOWED_ORIGINS = "*";

It should rather be the opposite. Do NOT allow any origin to make cross requests by default.

[gregw]

@freetom I agree we should increase the priority of this, however I'm not sure I agree with the point about the default, as CORS requires explicit configuration to be meaningful

[freetom]

@gregw thanks for the clarification.

By the way, I stumbled on a site in the wild that appeared to be using Jetty and had weak CORS protection. The issue laid on the fact that it was allowing too many values in the Access-Control-Allow-Origin header.

Let's say that the site had name site.com: providing whatever.site.com was working as the server responded with Access-Control-Allow-Origin: whatever.site.com. This is probably due to a value such as *.site.com given to the CrossOriginFilter. For reference, see Matcher matcher = createMatcher(origin, allowedOrigin); in CrossOriginFilter.java.

However I don't think regular expressions of this kind (using the wildcard */.* anywhere) shall be allowed as for an attacker with MITM capabilities it's easy to redirect an HTTP connection to a non-existing subdomain, inject the DNS response and get the CORS bypassed.

Another thing I noticed was that the site was allowing siteXcom where X could be any single character. This makes me think that while building the regex the . in site.com was interpreted as any character. But reading the code I found that you already fixed that as any . is replaced with a \. .. So I don't understand why this behavior.. do you have any ideas on this?

Version of Jetty on site.com: https://github.com/eclipse/jetty.project/releases/tag/jetty-9.2.9.v20150224

[jcodagnone]

Hi,

I was reviewing the CrossOriginFilter and I got stuck with the first two bullets of Narasimman (Dec 20, 2016): the performance of recompiling the the regex for each request, and the need to use regex that do not contains * character, for example https?:[/]/[.]foo[.]com(|:\d+).

I can probably create a patch for the first bullet, but probably not for the second (as it requires to define new params)

[freetom]

Be aware of https://tools.ietf.org/html/rfc6454#section-3.4.3

Returning invalid Access-Control-Allow-Origin allows domains (existing or not) to attack the user through CSRF. Therefore only a carefully selected whitelist should be allowed.

Furthermore, those domains should enforce the connectivity with HSTS, otherwise, a single, trusted subdomain without HSTS could get exploited by a MITM attack to make Cross-Site Requests on the trustee(s) domain.

[jcodagnone]

#1952 for the reuse of patterns (avoid compiling the patterns for each request).

[gregw]

My current thinking is :

• handling of the current configuration parameter should give a warning if wildcards are used - indicating the problems of allowing unknown hosts.
• for sophisticated users, a new configuration parameter will be added that allows true regular expressions. It's documentation will discourage wildcard usage and suggest that it only be used to select fixed lists of known alternatives (eg "node[0-9].mydomain.com" ).

This would not break existing configurations, give warnings about wildcard usage and allow sophisticated users to do whatever they like.

[joakime]

Another CrossOriginFilter PR #3346 was created to address the Vary header.

[lachlan-roberts]

@sbordet nudge
is this still a high priority security issue?

[geeknotnerd]

is this still a high priority security issue?

[sbordet]

I don't think it is.

[arsenalzp]

Hello,
Is this issue still valid and improvements requered?

[sbordet]

@arsenalzp I don't think this issue is valid anymore.

CrossOriginFilter is now deprecated in favor of CrossOriginHandler.

• Regex patterns are now computed in doStart()
• Default allowed origins is now empty, a more secure default; explicit configurations of origins is required.

If there are unsolved problems that I missed, or new problems, please open a new issue.