This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 7.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/7.0.x by this push: new b191a0d Correct a regression in transfer-encoding parsing b191a0d is described below commit b191a0d9cf06f4e04257c221bfe41d2b108a9cc8 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 --- .../coyote/http11/AbstractHttp11Processor.java | 12 ++- .../apache/coyote/http11/LocalStrings.properties | 1 + .../apache/tomcat/util/http/parser/TokenList.java | 43 +++++++--- .../tomcat/util/http/parser/TestTokenList.java | 95 ++++++++++++++++++---- webapps/docs/changelog.xml | 5 ++ 5 files changed, 123 insertions(+), 33 deletions(-) diff --git a/java/org/apache/coyote/http11/AbstractHttp11Processor.java b/java/org/apache/coyote/http11/AbstractHttp11Processor.java index 787d388..e5dacca 100644 --- a/java/org/apache/coyote/http11/AbstractHttp11Processor.java +++ b/java/org/apache/coyote/http11/AbstractHttp11Processor.java @@ -1534,10 +1534,14 @@ public abstract class AbstractHttp11Processor<S> extends AbstractProcessor<S> { } if (transferEncodingValueMB != null) { List<String> encodingNames = new ArrayList<String>(); - 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 292e2c1..b12dd2e 100644 --- a/java/org/apache/coyote/http11/LocalStrings.properties +++ b/java/org/apache/coyote/http11/LocalStrings.properties @@ -27,6 +27,7 @@ http11processor.regexp.error=Error parsing regular expression [{0}] 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 7ba886c..90b0233 100644 --- a/java/org/apache/tomcat/util/http/parser/TokenList.java +++ b/java/org/apache/tomcat/util/http/parser/TokenList.java @@ -36,19 +36,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; } @@ -56,17 +63,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; } @@ -79,16 +93,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 74053bb..2946727 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<String>(); expected.add("*"); - doTestVary("*", expected); + doTestVary("*", expected, true); } @@ -39,7 +39,7 @@ public class TestTokenList { public void testSingle() throws IOException { Set<String> expected = new HashSet<String>(); expected.add("host"); - doTestVary("Host", expected); + doTestVary("Host", expected, true); } @@ -49,21 +49,21 @@ 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 { Set<String> s = Collections.emptySet(); - doTestVary("", s); + doTestVary("", s, false); } @Test public void testSingleInvalid() throws IOException { Set<String> s = Collections.emptySet(); - doTestVary("{{{", s); + doTestVary("{{{", s, false); } @@ -73,7 +73,7 @@ public class TestTokenList { expected.add("bar"); expected.add("foo"); expected.add("host"); - doTestVary("{{{, Host, Foo, Bar", expected); + doTestVary("{{{, Host, Foo, Bar", expected, false); } @@ -83,7 +83,7 @@ public class TestTokenList { expected.add("bar"); expected.add("foo"); expected.add("host"); - doTestVary("Host, {{{, Foo, Bar", expected); + doTestVary("Host, {{{, Foo, Bar", expected, false); } @@ -93,7 +93,7 @@ public class TestTokenList { expected.add("bar"); expected.add("foo"); expected.add("host"); - doTestVary("Host, Foo, Bar, {{{", expected); + doTestVary("Host, Foo, Bar, {{{", expected, false); } @@ -103,7 +103,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); } @@ -113,7 +113,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); } @@ -123,21 +123,80 @@ 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); } @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<String>(); - Vary.parseVary(reader, result); - Assert.assertEquals(expected, result); + Set<String> tokens = new HashSet<String>(); + 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<String>(); + expectedTokens.add("bar"); + expectedTokens.add("foo"); + expectedTokens.add("foo2"); + + Set<String> inputs = new HashSet<String>(); + inputs.add("foo"); + if (withNull) { + inputs.add(null); + } + inputs.add("bar, foo2"); + + Set<String> tokens = new HashSet<String>(); + + + 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<String>(); + expectedTokens.add("bar"); + expectedTokens.add("bar2"); + expectedTokens.add("foo"); + expectedTokens.add("foo2"); + expectedTokens.add("foo3"); + + Set<String> inputs = new HashSet<String>(); + inputs.add("foo"); + inputs.add("bar2, }}}, foo3"); + inputs.add("bar, foo2"); + + Set<String> tokens = new HashSet<String>(); + + + 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 index 1962d6b..99d7e83 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -129,6 +129,11 @@ When reporting / logging invalid HTTP headers encode any non-printing characters using the 0xNN form. (markt) </add> + <fix> + Correct a regression introduced in 7.0.98 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"> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org