You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2020/02/04 14:34:35 UTC
[tomcat] branch 9.0.x updated: Correct a regression in
transfer-encoding parsing
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new 060ecc5 Correct a regression in transfer-encoding parsing
060ecc5 is described below
commit 060ecc5eb839208687b7fcc9e35287ac8eb46998
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 | 95 ++++++++++++++++++----
webapps/docs/changelog.xml | 5 ++
5 files changed, 123 insertions(+), 33 deletions(-)
diff --git a/java/org/apache/coyote/http11/Http11Processor.java b/java/org/apache/coyote/http11/Http11Processor.java
index c627f19..5296243 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 8405bda..6e3cbba 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,21 +121,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<>();
- 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
index f3045b1..c60022b 100644
--- 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">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org