Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62343

markt-asf · markt-asf · commit d83a76732e68 · 2018-05-16T14:53:21.000Z
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
diff --git a/java/org/apache/catalina/filters/CorsFilter.java b/java/org/apache/catalina/filters/CorsFilter.java
@@ -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);
@@ -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);
@@ -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.
@@ -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:
diff --git a/java/org/apache/catalina/filters/LocalStrings.properties b/java/org/apache/catalina/filters/LocalStrings.properties
@@ -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
diff --git a/test/org/apache/catalina/filters/TestCorsFilter.java b/test/org/apache/catalina/filters/TestCorsFilter.java
@@ -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(
@@ -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(
@@ -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(
@@ -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());
     }
 
     /*
@@ -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));
@@ -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(
@@ -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);
     }
 
@@ -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);
     }
 
@@ -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
diff --git a/test/org/apache/catalina/filters/TesterFilterConfigs.java b/test/org/apache/catalina/filters/TesterFilterConfigs.java
@@ -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;
@@ -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 =
@@ -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 =
@@ -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;
@@ -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;
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
@@ -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">
