Skip to content

Commit

Permalink
Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62343
Browse files Browse the repository at this point in the history 
Make CORS filter defaults more secure.
This is the fix for CVE-2018-8014.

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1831726 13f79535-47bb-0310-9956-ffa450edef68
  • Loading branch information
@markt-asf
markt-asf committed May 16, 2018
1 parent 6513dbd commit d83a767
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 61 deletions.
19 changes: 10 additions & 9 deletions java/org/apache/catalina/filters/CorsFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,17 +256,14 @@ protected void handleSimpleCORS(final HttpServletRequest request,

// Section 6.1.3
// Add a single Access-Control-Allow-Origin header.
if (anyOriginAllowed && !supportsCredentials) {
// If resource doesn't support credentials and if any origin is
// allowed
// to make CORS request, return header with '*'.
if (anyOriginAllowed) {
// If any origin is allowed, return header with '*'.
response.addHeader(
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN,
"*");
} else {
// If the resource supports credentials add a single
// Access-Control-Allow-Origin header, with the value of the Origin
// header as value.
// Add a single Access-Control-Allow-Origin header, with the value
// of the Origin header as value.
response.addHeader(
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN,
origin);
Expand Down Expand Up @@ -764,6 +761,10 @@ private void parseAndStore(final String allowedOrigins,
// For any value other then 'true' this will be false.
this.supportsCredentials = Boolean.parseBoolean(supportsCredentials);

if (this.supportsCredentials && this.anyOriginAllowed) {
throw new ServletException(sm.getString("corsFilter.invalidSupportsCredentials"));
}

try {
if (!preflightMaxAge.isEmpty()) {
this.preflightMaxAge = Long.parseLong(preflightMaxAge);
Expand Down Expand Up @@ -1073,7 +1074,7 @@ protected enum CORSRequestType {
/**
* By default, all origins are allowed to make requests.
*/
public static final String DEFAULT_ALLOWED_ORIGINS = "*";
public static final String DEFAULT_ALLOWED_ORIGINS = "";

/**
* By default, following methods are supported: GET, POST, HEAD and OPTIONS.
Expand All @@ -1089,7 +1090,7 @@ protected enum CORSRequestType {
/**
* By default, support credentials is turned on.
*/
public static final String DEFAULT_SUPPORTS_CREDENTIALS = "true";
public static final String DEFAULT_SUPPORTS_CREDENTIALS = "false";

/**
* By default, following headers are supported:
Expand Down
2 changes: 2 additions & 0 deletions java/org/apache/catalina/filters/LocalStrings.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
# limitations under the License.

addDefaultCharset.unsupportedCharset=Specified character set [{0}] is not supported

corsFilter.invalidSupportsCredentials=It is not allowed to configure supportsCredentials=[true] when allowedOrigins=[*]
corsFilter.invalidPreflightMaxAge=Unable to parse preflightMaxAge
corsFilter.nullRequest=HttpServletRequest object is null
corsFilter.nullRequestType=CORSRequestType object is null
Expand Down
62 changes: 15 additions & 47 deletions test/org/apache/catalina/filters/TestCorsFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,7 @@ public void testDoFilterSimpleGET() throws IOException, ServletException {
corsFilter.doFilter(request, response, filterChain);

Assert.assertTrue(response.getHeader(
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN).equals(
"https://www.apache.org"));
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN).equals("*"));
Assert.assertTrue(((Boolean) request.getAttribute(
CorsFilter.HTTP_REQUEST_ATTRIBUTE_IS_CORS_REQUEST)).booleanValue());
Assert.assertTrue(request.getAttribute(
Expand Down Expand Up @@ -88,8 +87,7 @@ public void testDoFilterSimplePOST() throws IOException, ServletException {
corsFilter.doFilter(request, response, filterChain);

Assert.assertTrue(response.getHeader(
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN).equals(
"https://www.apache.org"));
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN).equals("*"));
Assert.assertTrue(((Boolean) request.getAttribute(
CorsFilter.HTTP_REQUEST_ATTRIBUTE_IS_CORS_REQUEST)).booleanValue());
Assert.assertTrue(request.getAttribute(
Expand Down Expand Up @@ -120,8 +118,7 @@ public void testDoFilterSimpleHEAD() throws IOException, ServletException {
corsFilter.doFilter(request, response, filterChain);

Assert.assertTrue(response.getHeader(
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN).equals(
"https://www.apache.org"));
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN).equals("*"));
Assert.assertTrue(((Boolean) request.getAttribute(
CorsFilter.HTTP_REQUEST_ATTRIBUTE_IS_CORS_REQUEST)).booleanValue());
Assert.assertTrue(request.getAttribute(
Expand Down Expand Up @@ -166,41 +163,15 @@ public void testDoFilterSimpleSpecificHeader() throws IOException,
}

/*
* Tests the presence of the origin (and not '*') in the response, when
* supports credentials is enabled alongwith any origin, '*'.
* Tests the that supports credentials may not be enabled with any origin,
* '*'.
*
* @throws IOException
* @throws ServletException
*/
@Test
public void testDoFilterSimpleAnyOriginAndSupportsCredentials()
throws IOException, ServletException {
TesterHttpServletRequest request = new TesterHttpServletRequest();
request.setHeader(CorsFilter.REQUEST_HEADER_ORIGIN,
TesterFilterConfigs.HTTPS_WWW_APACHE_ORG);
request.setMethod("GET");
TesterHttpServletResponse response = new TesterHttpServletResponse();

@Test(expected=ServletException.class)
public void testDoFilterSimpleAnyOriginAndSupportsCredentials() throws ServletException {
CorsFilter corsFilter = new CorsFilter();
corsFilter.init(TesterFilterConfigs
.getFilterConfigAnyOriginAndSupportsCredentials());
corsFilter.doFilter(request, response, filterChain);

Assert.assertTrue(response.getHeader(
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN).equals(
TesterFilterConfigs.HTTPS_WWW_APACHE_ORG));
Assert.assertTrue(response.getHeader(
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_CREDENTIALS)
.equals(
"true"));
Assert.assertTrue(((Boolean) request.getAttribute(
CorsFilter.HTTP_REQUEST_ATTRIBUTE_IS_CORS_REQUEST)).booleanValue());
Assert.assertTrue(request.getAttribute(
CorsFilter.HTTP_REQUEST_ATTRIBUTE_ORIGIN).equals(
TesterFilterConfigs.HTTPS_WWW_APACHE_ORG));
Assert.assertTrue(request.getAttribute(
CorsFilter.HTTP_REQUEST_ATTRIBUTE_REQUEST_TYPE).equals(
CorsFilter.CORSRequestType.SIMPLE.name().toLowerCase(Locale.ENGLISH)));
corsFilter.init(TesterFilterConfigs.getFilterConfigAnyOriginAndSupportsCredentials());
}

/*
Expand Down Expand Up @@ -261,8 +232,7 @@ public void testDoFilterSimpleWithExposedHeaders() throws IOException,
corsFilter.doFilter(request, response, filterChain);

Assert.assertTrue(response.getHeader(
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN).equals(
"https://www.apache.org"));
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN).equals("*"));
Assert.assertTrue(response.getHeader(
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_EXPOSE_HEADERS)
.equals(TesterFilterConfigs.EXPOSED_HEADERS));
Expand Down Expand Up @@ -727,9 +697,8 @@ public String getFilterName() {
});
corsFilter.doFilter(request, response, filterChain);

Assert.assertTrue(response.getHeader(
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN).equals(
"https://www.apache.org"));
Assert.assertNull(response.getHeader(
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN));
Assert.assertTrue(((Boolean) request.getAttribute(
CorsFilter.HTTP_REQUEST_ATTRIBUTE_IS_CORS_REQUEST)).booleanValue());
Assert.assertTrue(request.getAttribute(
Expand Down Expand Up @@ -1412,7 +1381,7 @@ public void testWithFilterConfig() throws ServletException {
Assert.assertTrue(corsFilter.getAllowedOrigins().size() == 0);
Assert.assertTrue(corsFilter.isAnyOriginAllowed());
Assert.assertTrue(corsFilter.getExposedHeaders().size() == 0);
Assert.assertTrue(corsFilter.isSupportsCredentials());
Assert.assertFalse(corsFilter.isSupportsCredentials());
Assert.assertTrue(corsFilter.getPreflightMaxAge() == 1800);
}

Expand Down Expand Up @@ -1448,9 +1417,9 @@ public void testWithStringParserNull() throws ServletException {
Assert.assertTrue(corsFilter.getAllowedHttpHeaders().size() == 6);
Assert.assertTrue(corsFilter.getAllowedHttpMethods().size() == 4);
Assert.assertTrue(corsFilter.getAllowedOrigins().size() == 0);
Assert.assertTrue(corsFilter.isAnyOriginAllowed());
Assert.assertFalse(corsFilter.isAnyOriginAllowed());
Assert.assertTrue(corsFilter.getExposedHeaders().size() == 0);
Assert.assertTrue(corsFilter.isSupportsCredentials());
Assert.assertFalse(corsFilter.isSupportsCredentials());
Assert.assertTrue(corsFilter.getPreflightMaxAge() == 1800);
}

Expand Down Expand Up @@ -1554,8 +1523,7 @@ public void testDecorateRequestDisabled() throws IOException,
corsFilter.doFilter(request, response, filterChain);

Assert.assertTrue(response.getHeader(
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN).equals(
"https://www.apache.org"));
CorsFilter.RESPONSE_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN).equals("*"));
Assert.assertNull(request
.getAttribute(CorsFilter.HTTP_REQUEST_ATTRIBUTE_IS_CORS_REQUEST));
Assert.assertNull(request
Expand Down
11 changes: 6 additions & 5 deletions test/org/apache/catalina/filters/TesterFilterConfigs.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,13 @@ public class TesterFilterConfigs {
public static final TesterServletContext mockServletContext =
new TesterServletContext();

// Default config for the test is to allow any origin
public static FilterConfig getDefaultFilterConfig() {
final String allowedHttpHeaders =
CorsFilter.DEFAULT_ALLOWED_HTTP_HEADERS;
final String allowedHttpMethods =
CorsFilter.DEFAULT_ALLOWED_HTTP_METHODS;
final String allowedOrigins = CorsFilter.DEFAULT_ALLOWED_ORIGINS;
final String allowedOrigins = ANY_ORIGIN;
final String exposedHeaders = CorsFilter.DEFAULT_EXPOSED_HEADERS;
final String supportCredentials =
CorsFilter.DEFAULT_SUPPORTS_CREDENTIALS;
Expand All @@ -59,7 +60,7 @@ public static FilterConfig getFilterConfigAnyOriginAndSupportsCredentials() {
CorsFilter.DEFAULT_ALLOWED_HTTP_HEADERS;
final String allowedHttpMethods =
CorsFilter.DEFAULT_ALLOWED_HTTP_METHODS + ",PUT";
final String allowedOrigins = CorsFilter.DEFAULT_ALLOWED_ORIGINS;
final String allowedOrigins = ANY_ORIGIN;
final String exposedHeaders = CorsFilter.DEFAULT_EXPOSED_HEADERS;
final String supportCredentials = "true";
final String preflightMaxAge =
Expand All @@ -77,7 +78,7 @@ public static FilterConfig getFilterConfigAnyOriginAndSupportsCredentials() {
CorsFilter.DEFAULT_ALLOWED_HTTP_HEADERS;
final String allowedHttpMethods =
CorsFilter.DEFAULT_ALLOWED_HTTP_METHODS + ",PUT";
final String allowedOrigins = CorsFilter.DEFAULT_ALLOWED_ORIGINS;
final String allowedOrigins = ANY_ORIGIN;
final String exposedHeaders = CorsFilter.DEFAULT_EXPOSED_HEADERS;
final String supportCredentials = "false";
final String preflightMaxAge =
Expand Down Expand Up @@ -131,7 +132,7 @@ public static FilterConfig getFilterConfigWithExposedHeaders() {
CorsFilter.DEFAULT_ALLOWED_HTTP_HEADERS;
final String allowedHttpMethods =
CorsFilter.DEFAULT_ALLOWED_HTTP_METHODS;
final String allowedOrigins = CorsFilter.DEFAULT_ALLOWED_ORIGINS;
final String allowedOrigins = ANY_ORIGIN;
final String exposedHeaders = EXPOSED_HEADERS;
final String supportCredentials =
CorsFilter.DEFAULT_SUPPORTS_CREDENTIALS;
Expand Down Expand Up @@ -240,7 +241,7 @@ public static FilterConfig getFilterConfigDecorateRequestDisabled() {
CorsFilter.DEFAULT_ALLOWED_HTTP_HEADERS;
final String allowedHttpMethods =
CorsFilter.DEFAULT_ALLOWED_HTTP_METHODS;
final String allowedOrigins = CorsFilter.DEFAULT_ALLOWED_ORIGINS;
final String allowedOrigins = ANY_ORIGIN;
final String exposedHeaders = CorsFilter.DEFAULT_EXPOSED_HEADERS;
final String supportCredentials =
CorsFilter.DEFAULT_SUPPORTS_CREDENTIALS;
Expand Down
4 changes: 4 additions & 0 deletions webapps/docs/changelog.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@
usually performed during web application stop if that stop is triggered
by a JVM shutdown. (markt)
</fix>
<fix>
<bug>62343</bug>: Make CORS filter defaults more secure. This is the fix
for CVE-2018-8014. (markt)
</fix>
</changelog>
</subsection>
<subsection name="Coyote">
Expand Down

0 comments on commit d83a767

Please # to comment.