This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/master by this push: new 3c295d9 Correct a regression in transfer-encoding parsing 3c295d9 is described below commit 3c295d913e1d82ce25b4ad66c800313994f4e530 Author: Mark Thomas <ma...@apache.org> AuthorDate: Tue Dec 17 09:27:49 2019 +0000 Correct a regression in transfer-encoding parsing Invalid tokens are an error --- java/org/apache/coyote/http11/Http11Processor.java | 12 ++- .../apache/coyote/http11/LocalStrings.properties | 1 + .../apache/tomcat/util/http/parser/TokenList.java | 43 ++++++++--- .../tomcat/util/http/parser/TestTokenList.java | 89 ++++++++++++++++++---- 4 files changed, 115 insertions(+), 30 deletions(-) diff --git a/java/org/apache/coyote/http11/Http11Processor.java b/java/org/apache/coyote/http11/Http11Processor.java index 5c1e1a0..a365235 100644 --- a/java/org/apache/coyote/http11/Http11Processor.java +++ b/java/org/apache/coyote/http11/Http11Processor.java @@ -723,10 +723,14 @@ public class Http11Processor extends AbstractProcessor { 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 index b7430fc..6765b87 100644 --- 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 index db40877..0ab7ce1 100644 --- a/java/org/apache/tomcat/util/http/parser/TokenList.java +++ b/java/org/apache/tomcat/util/http/parser/TokenList.java @@ -34,19 +34,26 @@ public class 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; } @@ -54,17 +61,24 @@ public class TokenList { * 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 class TokenList { 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 index 642c9a7..43ea16d 100644 --- a/test/org/apache/tomcat/util/http/parser/TestTokenList.java +++ b/test/org/apache/tomcat/util/http/parser/TestTokenList.java @@ -31,7 +31,7 @@ public class TestTokenList { public void testAll() throws IOException { Set<String> expected = new HashSet<>(); expected.add("*"); - doTestVary("*", expected); + doTestVary("*", expected, true); } @@ -39,7 +39,7 @@ public class TestTokenList { 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 class TestTokenList { 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 class TestTokenList { 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 class TestTokenList { 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 class TestTokenList { 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 class TestTokenList { 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 class TestTokenList { expected.add("bar"); expected.add("foo"); expected.add("host"); - doTestVary("Host, OK {{{, Foo, Bar", expected); + doTestVary("Host, OK {{{, Foo, Bar", expected, false); } @@ -121,14 +121,73 @@ public class TestTokenList { expected.add("bar"); expected.add("foo"); expected.add("host"); - doTestVary("Host, Foo, Bar, OK {{{", expected); + doTestVary("Host, Foo, Bar, OK {{{", expected, false); } - 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<>(); - TokenList.parseTokenList(reader, result); - Assert.assertEquals(expected, result); + Set<String> tokens = new HashSet<>(); + 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); + } + } --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org