Bug 63939 - CORS filter incorrectly implements same/local origin check
Summary: CORS filter incorrectly implements same/local origin check
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 9.0.x
Hardware: All All
: P2 major (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-19 13:09 UTC by Michael Osipov
Modified: 2019-11-30 12:10 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Osipov 2019-11-19 13:09:55 UTC
I believe org.apache.catalina.filters.CorsFilter.isLocalOrigin(HttpServletRequest, String) has two bugs:

One note upfront, I believe this method should be renamed to isSameOrigin() to use the same term as with the Fetch Standard.

Bug 1: When Origin contains a standard port Tomcat does not take that into account and omits the default port from target, .e.g.,
> curl -X OPTIONS -H "Origin: https://fqdn:443"
vs. 
> curl -X OPTIONS -H "Origin: https://fqdn"

both result in different responses.

The root cause is here: https://github.com/apache/tomcat/blob/master/java/org/apache/catalina/filters/CorsFilter.java#L656-L663

As far as I understand https://url.spec.whatwg.org/#concept-url-port, the default port for the specific protocol has to be used within the comparison.

Bug 2: at the very end "origin.equalsIgnoreCase(target.toString());" is performed while isOriginAllowed() performs a case-sensitive comparision as documented here: 
https://www.w3.org/TR/access-control/#resource-preflight-requests
This seems to be inconsistent.
Comment 1 Mark Thomas 2019-11-29 14:08:13 UTC
The CORS specification references RFC 6454 for the definition of the origin header.

RFC 6454 states that the port should only be included in serialized form (which is the form used in the HTTP header) if the port differs from the default port. Tomcat's same origin test is, therefore, correct in this respect.
Comment 2 Mark Thomas 2019-11-29 14:18:28 UTC
As far as the case sensitivity of the check (or not) goes, both the scheme and host are case insensitive although lower case is the convention for both. It really should not make any difference at all whether this check is case sensitive or not. That said, this is the CORS filter and the CORS spec says the check should be performed in a case sensitive manner so I do think we should change this. We can change the method name at the same time.

I suspect this will be part of some slightly wider refactoring in support of fixing bug 63637.
Comment 3 Michael Osipov 2019-11-29 17:24:38 UTC
(In reply to Mark Thomas from comment #1)
> The CORS specification references RFC 6454 for the definition of the origin
> header.
> 
> RFC 6454 states that the port should only be included in serialized form
> (which is the form used in the HTTP header) if the port differs from the
> default port. Tomcat's same origin test is, therefore, correct in this
> respect.

You are referring to https://tools.ietf.org/html/rfc6454#section-6, granted. But the example from https://tools.ietf.org/html/rfc6454#section-3.2.1 claim that w/ and w/o default port is the same origin, so this the Spring's CORS filter: https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/cors/CorsUtils.java#L50-L52

Would you both consider to be wrong, or just lax? Because RFC 6454 refers in the ABNF to RFC 3986 and that likely will compare them equal.
Comment 4 Michael Osipov 2019-11-29 17:30:14 UTC
(In reply to Mark Thomas from comment #2)
> As far as the case sensitivity of the check (or not) goes, both the scheme
> and host are case insensitive although lower case is the convention for
> both. It really should not make any difference at all whether this check is
> case sensitive or not. That said, this is the CORS filter and the CORS spec
> says the check should be performed in a case sensitive manner so I do think
> we should change this. We can change the method name at the same time.

Sound reasonable, I agree. If the client does not provide a clean serialized form, it is the client's fault.

> I suspect this will be part of some slightly wider refactoring in support of
> fixing bug 63637.

This issue refers to JMeter? Typo?
Comment 5 Mark Thomas 2019-11-29 23:22:45 UTC
The bug requiring refactoring is bug 63937.

The RFC 6454 inconsistency is unhelpful but the intention seems clear from section 3.2.

The Spring reference was useful as it suggested we should include ws/wss when calculating default ports.

I have a fix for this locally. Just want to run a few more tests.
Comment 6 Mark Thomas 2019-11-30 12:10:22 UTC
Fixed in:
- master for 9.0.30 onwards
- 8.5.x for 8.5.50 onwards
- 7.0.x for 7.0.99 onwards