Skip to content

Commit

Permalink
Correct a regression in transfer-encoding parsing
Browse files Browse the repository at this point in the history 
Invalid tokens are an error
  • Loading branch information
@markt-asf
markt-asf committed Feb 4, 2020
1 parent e9d7be7 commit 959f1df
Show file tree
Hide file tree
Showing 5 changed files with 123 additions and 33 deletions.
12 changes: 8 additions & 4 deletions java/org/apache/coyote/http11/Http11Processor.java
View file
Expand Up @@ -965,10 +965,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");
}
}
}
Expand Down
1 change: 1 addition & 0 deletions java/org/apache/coyote/http11/LocalStrings.properties
View file
Expand Up @@ -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
Expand Down
43 changes: 32 additions & 11 deletions java/org/apache/tomcat/util/http/parser/TokenList.java
View file
Expand Up @@ -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;
}
Expand All @@ -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;
}
}
95 changes: 77 additions & 18 deletions test/org/apache/tomcat/util/http/parser/TestTokenList.java
View file
Expand Up @@ -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);
}


Expand All @@ -49,21 +49,21 @@ 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 {
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);
}


Expand All @@ -73,7 +73,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);
}


Expand All @@ -83,7 +83,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);
}


Expand All @@ -93,7 +93,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);
}


Expand All @@ -103,7 +103,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);
}


Expand All @@ -113,7 +113,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);
}


Expand All @@ -123,21 +123,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);
}

}
5 changes: 5 additions & 0 deletions webapps/docs/changelog.xml
View file
Expand Up @@ -169,6 +169,11 @@
When reporting / logging invalid HTTP headers encode any non-printing
characters using the 0xNN form. (markt)
</add>
<fix>
Correct a regression introduced in 8.5.48 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">
Expand Down

0 comments on commit 959f1df

Please sign in to comment.