Correct a regression in transfer-encoding parsing

Invalid tokens are an error
apache · Feb 4, 2020 · 060ecc5 · 060ecc5
1 parent fdcb365
commit 060ecc5
Show file tree

Hide file tree

Showing 5 changed files with 123 additions and 33 deletions.
diff --git a/java/org/apache/coyote/http11/Http11Processor.java b/java/org/apache/coyote/http11/Http11Processor.java
@@ -723,10 +723,14 @@ private void prepareRequest() throws IOException {
             MessageBytes transferEncodingValueMB = headers.getValue("transfer-encoding");
             if (transferEncodingValueMB != null) {
                 List<String> encodingNames = new ArrayList<>();
-                TokenList.parseTokenList(headers.values("transfer-encoding"), encodingNames);
-                for (String encodingName : encodingNames) {
-                    // "identity" codings are ignored
-                    addInputFilter(inputFilters, encodingName);
+                if (TokenList.parseTokenList(headers.values("transfer-encoding"), encodingNames)) {
+                    for (String encodingName : encodingNames) {
+                        // "identity" codings are ignored
+                        addInputFilter(inputFilters, encodingName);
+                    }
+                } else {
+                    // Invalid transfer encoding
+                    badRequest("http11processor.request.invalidTransferEncoding");
                 }
             }
         }

diff --git a/java/org/apache/coyote/http11/LocalStrings.properties b/java/org/apache/coyote/http11/LocalStrings.properties
@@ -23,6 +23,7 @@ http11processor.header.parse=Error parsing HTTP request header
 http11processor.request.finish=Error finishing request
 http11processor.request.inconsistentHosts=The host specified in the request line is not consistent with the host header
 http11processor.request.invalidScheme=The HTTP request contained an absolute URI with an invalid scheme
+http11processor.request.invalidTransferEncoding=The HTTP request contained an invalid Transfer-Encoding header
 http11processor.request.invalidUri=The HTTP request contained an invalid URI
 http11processor.request.invalidUserInfo=The HTTP request contained an absolute URI with an invalid userinfo
 http11processor.request.multipleContentLength=The request contained multiple content-length headers

diff --git a/java/org/apache/tomcat/util/http/parser/TokenList.java b/java/org/apache/tomcat/util/http/parser/TokenList.java
@@ -34,37 +34,51 @@ private TokenList() {
      * Parses an enumeration of header values of the form 1#token, forcing all
      * parsed values to lower case.
      *
-     * @param inputs The headers to parse
-     * @param result The Collection (usually a list of a set) to which the
-     *                   parsed tokens should be added
+     * @param inputs     The headers to parse
+     * @param collection The Collection (usually a list of a set) to which the
+     *                       parsed tokens should be added
+     *
+     * @return {@code} true if the header values were parsed cleanly, otherwise
+     *         {@code false} (e.g. if a non-token value was encountered)
      *
      * @throws IOException If an I/O error occurs reading the header
      */
-    public static void parseTokenList(Enumeration<String> inputs, Collection<String> result) throws IOException {
+    public static boolean parseTokenList(Enumeration<String> inputs, Collection<String> collection) throws IOException {
+        boolean result = true;
         while (inputs.hasMoreElements()) {
             String nextHeaderValue = inputs.nextElement();
             if (nextHeaderValue != null) {
-                TokenList.parseTokenList(new StringReader(nextHeaderValue), result);
+                if (!TokenList.parseTokenList(new StringReader(nextHeaderValue), collection)) {
+                    result = false;
+                }
             }
         }
+        return result;
     }
 
 
     /**
      * Parses a header of the form 1#token, forcing all parsed values to lower
      * case. This is typically used when header values are case-insensitive.
      *
-     * @param input  The header to parse
-     * @param result The Collection (usually a list of a set) to which the
-     *                   parsed tokens should be added
+     * @param input      The header to parse
+     * @param collection The Collection (usually a list of a set) to which the
+     *                       parsed tokens should be added
+     *
+     * @return {@code} true if the header was parsed cleanly, otherwise
+     *         {@code false} (e.g. if a non-token value was encountered)
      *
      * @throws IOException If an I/O error occurs reading the header
      */
-    public static void parseTokenList(Reader input, Collection<String> result) throws IOException {
+    public static boolean parseTokenList(Reader input, Collection<String> collection) throws IOException {
+        boolean invalid = false;
+        boolean valid = false;
+
         do {
             String fieldName = HttpParser.readToken(input);
             if (fieldName == null) {
                 // Invalid field-name, skip to the next one
+                invalid = true;
                 HttpParser.skipUntil(input, 0, ',');
                 continue;
             }
@@ -77,16 +91,23 @@ public static void parseTokenList(Reader input, Collection<String> result) throw
             SkipResult skipResult = HttpParser.skipConstant(input, ",");
             if (skipResult == SkipResult.EOF) {
                 // EOF
-                result.add(fieldName.toLowerCase(Locale.ENGLISH));
+                valid = true;
+                collection.add(fieldName.toLowerCase(Locale.ENGLISH));
                 break;
             } else if (skipResult == SkipResult.FOUND) {
-                result.add(fieldName.toLowerCase(Locale.ENGLISH));
+                valid = true;
+                collection.add(fieldName.toLowerCase(Locale.ENGLISH));
                 continue;
             } else {
                 // Not a token - ignore it
+                invalid = true;
                 HttpParser.skipUntil(input, 0, ',');
                 continue;
             }
         } while (true);
+
+        // Only return true if at least one valid token was read and no invalid
+        // entries were found
+        return valid && !invalid;
     }
 }
diff --git a/test/org/apache/tomcat/util/http/parser/TestTokenList.java b/test/org/apache/tomcat/util/http/parser/TestTokenList.java
@@ -31,15 +31,15 @@ public class TestTokenList {
     public void testAll() throws IOException {
         Set<String> expected = new HashSet<>();
         expected.add("*");
-        doTestVary("*", expected);
+        doTestVary("*", expected, true);
     }
 
 
     @Test
     public void testSingle() throws IOException {
         Set<String> expected = new HashSet<>();
         expected.add("host");
-        doTestVary("Host", expected);
+        doTestVary("Host", expected, true);
     }
 
 
@@ -49,19 +49,19 @@ public void testMultiple() throws IOException {
         expected.add("bar");
         expected.add("foo");
         expected.add("host");
-        doTestVary("Host, Foo, Bar", expected);
+        doTestVary("Host, Foo, Bar", expected, true);
     }
 
 
     @Test
     public void testEmptyString() throws IOException {
-        doTestVary("", Collections.emptySet());
+        doTestVary("", Collections.emptySet(), false);
     }
 
 
     @Test
     public void testSingleInvalid() throws IOException {
-        doTestVary("{{{", Collections.emptySet());
+        doTestVary("{{{", Collections.emptySet(), false);
     }
 
 
@@ -71,7 +71,7 @@ public void testMultipleWithInvalidStart() throws IOException {
         expected.add("bar");
         expected.add("foo");
         expected.add("host");
-        doTestVary("{{{, Host, Foo, Bar", expected);
+        doTestVary("{{{, Host, Foo, Bar", expected, false);
     }
 
 
@@ -81,7 +81,7 @@ public void testMultipleWithInvalidMiddle() throws IOException {
         expected.add("bar");
         expected.add("foo");
         expected.add("host");
-        doTestVary("Host, {{{, Foo, Bar", expected);
+        doTestVary("Host, {{{, Foo, Bar", expected, false);
     }
 
 
@@ -91,7 +91,7 @@ public void testMultipleWithInvalidEnd() throws IOException {
         expected.add("bar");
         expected.add("foo");
         expected.add("host");
-        doTestVary("Host, Foo, Bar, {{{", expected);
+        doTestVary("Host, Foo, Bar, {{{", expected, false);
     }
 
 
@@ -101,7 +101,7 @@ public void testMultipleWithInvalidStart2() throws IOException {
         expected.add("bar");
         expected.add("foo");
         expected.add("host");
-        doTestVary("OK {{{, Host, Foo, Bar", expected);
+        doTestVary("OK {{{, Host, Foo, Bar", expected, false);
     }
 
 
@@ -111,7 +111,7 @@ public void testMultipleWithInvalidMiddle2() throws IOException {
         expected.add("bar");
         expected.add("foo");
         expected.add("host");
-        doTestVary("Host, OK {{{, Foo, Bar", expected);
+        doTestVary("Host, OK {{{, Foo, Bar", expected, false);
     }
 
 
@@ -121,21 +121,80 @@ public void testMultipleWithInvalidEnd2() throws IOException {
         expected.add("bar");
         expected.add("foo");
         expected.add("host");
-        doTestVary("Host, Foo, Bar, OK {{{", expected);
+        doTestVary("Host, Foo, Bar, OK {{{", expected, false);
     }
 
 
     @SuppressWarnings("deprecation")
-    private void doTestVary(String input, Set<String> expected) throws IOException {
+    private void doTestVary(String input, Set<String> expectedTokens, boolean expectedResult) throws IOException {
         StringReader reader = new StringReader(input);
-        Set<String> result = new HashSet<>();
-        Vary.parseVary(reader, result);
-        Assert.assertEquals(expected, result);
+        Set<String> tokens = new HashSet<>();
+        Vary.parseVary(reader, tokens);
+        Assert.assertEquals(expectedTokens, tokens);
 
         // Can't use reset(). Parser uses marks.
         reader = new StringReader(input);
-        result.clear();
-        TokenList.parseTokenList(reader, result);
-        Assert.assertEquals(expected, result);
+        tokens.clear();
+        boolean result = TokenList.parseTokenList(reader, tokens);
+        Assert.assertEquals(expectedTokens, tokens);
+        Assert.assertEquals(Boolean.valueOf(expectedResult), Boolean.valueOf(result));
     }
+
+
+    @Test
+    public void testMultipleHeadersValidWithoutNull() throws IOException {
+        doTestMultipleHeadersValid(false);
+    }
+
+
+    @Test
+    public void testMultipleHeadersValidWithNull() throws IOException {
+        doTestMultipleHeadersValid(true);
+    }
+
+
+    private void doTestMultipleHeadersValid(boolean withNull) throws IOException {
+        Set<String> expectedTokens = new HashSet<>();
+        expectedTokens.add("bar");
+        expectedTokens.add("foo");
+        expectedTokens.add("foo2");
+
+        Set<String> inputs = new HashSet<>();
+        inputs.add("foo");
+        if (withNull) {
+            inputs.add(null);
+        }
+        inputs.add("bar, foo2");
+
+        Set<String> tokens = new HashSet<>();
+
+
+        boolean result = TokenList.parseTokenList(Collections.enumeration(inputs), tokens);
+        Assert.assertEquals(expectedTokens, tokens);
+        Assert.assertTrue(result);
+    }
+
+
+    @Test
+    public void doTestMultipleHeadersInvalid() throws IOException {
+        Set<String> expectedTokens = new HashSet<>();
+        expectedTokens.add("bar");
+        expectedTokens.add("bar2");
+        expectedTokens.add("foo");
+        expectedTokens.add("foo2");
+        expectedTokens.add("foo3");
+
+        Set<String> inputs = new HashSet<>();
+        inputs.add("foo");
+        inputs.add("bar2, }}}, foo3");
+        inputs.add("bar, foo2");
+
+        Set<String> tokens = new HashSet<>();
+
+
+        boolean result = TokenList.parseTokenList(Collections.enumeration(inputs), tokens);
+        Assert.assertEquals(expectedTokens, tokens);
+        Assert.assertFalse(result);
+    }
+
 }
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
@@ -164,6 +164,11 @@
         Add support for RFC 5915 formatted, unencrypted EC key files when using
         a JSSE based TLS connector. (markt)
       </add>
+      <fix>
+        Correct a regression introduced in 9.0.28 that meant invalid tokens in
+        the <code>Transfer-Encoding</code> header were ignored rather than
+        treated as an error. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">