Skip to content

Commit b191a0d

Browse files
markt-asfmarkt-asf
committed
Correct a regression in transfer-encoding parsing
Invalid tokens are an error
1 parent 5dbaead commit b191a0d

File tree

5 files changed

+123
-33
lines changed

5 files changed

+123
-33
lines changed

‎java/org/apache/coyote/http11/AbstractHttp11Processor.java‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1534,10 +1534,14 @@ protected void prepareRequest() throws IOException {
15341534
}
15351535
if (transferEncodingValueMB != null) {
15361536
List<String> encodingNames = new ArrayList<String>();
1537-
TokenList.parseTokenList(headers.values("transfer-encoding"), encodingNames);
1538-
for (String encodingName : encodingNames) {
1539-
// "identity" codings are ignored
1540-
addInputFilter(inputFilters, encodingName);
1537+
if (TokenList.parseTokenList(headers.values("transfer-encoding"), encodingNames)) {
1538+
for (String encodingName : encodingNames) {
1539+
// "identity" codings are ignored
1540+
addInputFilter(inputFilters, encodingName);
1541+
}
1542+
} else {
1543+
// Invalid transfer encoding
1544+
badRequest("http11processor.request.invalidTransferEncoding");
15411545
}
15421546
}
15431547

‎java/org/apache/coyote/http11/LocalStrings.properties‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ http11processor.regexp.error=Error parsing regular expression [{0}]
2727
http11processor.request.finish=Error finishing request
2828
http11processor.request.inconsistentHosts=The host specified in the request line is not consistent with the host header
2929
http11processor.request.invalidScheme=The HTTP request contained an absolute URI with an invalid scheme
30+
http11processor.request.invalidTransferEncoding=The HTTP request contained an invalid Transfer-Encoding header
3031
http11processor.request.invalidUri=The HTTP request contained an invalid URI
3132
http11processor.request.invalidUserInfo=The HTTP request contained an absolute URI with an invalid userinfo
3233
http11processor.request.multipleContentLength=The request contained multiple content-length headers

‎java/org/apache/tomcat/util/http/parser/TokenList.java‎

Lines changed: 32 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -36,37 +36,51 @@ private TokenList() {
3636
* Parses an enumeration of header values of the form 1#token, forcing all
3737
* parsed values to lower case.
3838
*
39-
* @param inputs The headers to parse
40-
* @param result The Collection (usually a list of a set) to which the
41-
* parsed tokens should be added
39+
* @param inputs The headers to parse
40+
* @param collection The Collection (usually a list of a set) to which the
41+
* parsed tokens should be added
42+
*
43+
* @return {@code} true if the header values were parsed cleanly, otherwise
44+
* {@code false} (e.g. if a non-token value was encountered)
4245
*
4346
* @throws IOException If an I/O error occurs reading the header
4447
*/
45-
public static void parseTokenList(Enumeration<String> inputs, Collection<String> result) throws IOException {
48+
public static boolean parseTokenList(Enumeration<String> inputs, Collection<String> collection) throws IOException {
49+
boolean result = true;
4650
while (inputs.hasMoreElements()) {
4751
String nextHeaderValue = inputs.nextElement();
4852
if (nextHeaderValue != null) {
49-
TokenList.parseTokenList(new StringReader(nextHeaderValue), result);
53+
if (!TokenList.parseTokenList(new StringReader(nextHeaderValue), collection)) {
54+
result = false;
55+
}
5056
}
5157
}
58+
return result;
5259
}
5360

5461

5562
/**
5663
* Parses a header of the form 1#token, forcing all parsed values to lower
5764
* case. This is typically used when header values are case-insensitive.
5865
*
59-
* @param input The header to parse
60-
* @param result The Collection (usually a list of a set) to which the
61-
* parsed tokens should be added
66+
* @param input The header to parse
67+
* @param collection The Collection (usually a list of a set) to which the
68+
* parsed tokens should be added
69+
*
70+
* @return {@code} true if the header was parsed cleanly, otherwise
71+
* {@code false} (e.g. if a non-token value was encountered)
6272
*
6373
* @throws IOException If an I/O error occurs reading the header
6474
*/
65-
public static void parseTokenList(Reader input, Collection<String> result) throws IOException {
75+
public static boolean parseTokenList(Reader input, Collection<String> collection) throws IOException {
76+
boolean invalid = false;
77+
boolean valid = false;
78+
6679
do {
6780
String fieldName = HttpParser.readToken(input);
6881
if (fieldName == null) {
6982
// Invalid field-name, skip to the next one
83+
invalid = true;
7084
HttpParser.skipUntil(input, 0, ',');
7185
continue;
7286
}
@@ -79,16 +93,23 @@ public static void parseTokenList(Reader input, Collection<String> result) throw
7993
SkipResult skipResult = HttpParser.skipConstant(input, ",");
8094
if (skipResult == SkipResult.EOF) {
8195
// EOF
82-
result.add(fieldName.toLowerCase(Locale.ENGLISH));
96+
valid = true;
97+
collection.add(fieldName.toLowerCase(Locale.ENGLISH));
8398
break;
8499
} else if (skipResult == SkipResult.FOUND) {
85-
result.add(fieldName.toLowerCase(Locale.ENGLISH));
100+
valid = true;
101+
collection.add(fieldName.toLowerCase(Locale.ENGLISH));
86102
continue;
87103
} else {
88104
// Not a token - ignore it
105+
invalid = true;
89106
HttpParser.skipUntil(input, 0, ',');
90107
continue;
91108
}
92109
} while (true);
110+
111+
// Only return true if at least one valid token was read and no invalid
112+
// entries were found
113+
return valid && !invalid;
93114
}
94115
}

‎test/org/apache/tomcat/util/http/parser/TestTokenList.java‎

Lines changed: 77 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,15 @@ public class TestTokenList {
3131
public void testAll() throws IOException {
3232
Set<String> expected = new HashSet<String>();
3333
expected.add("*");
34-
doTestVary("*", expected);
34+
doTestVary("*", expected, true);
3535
}
3636

3737

3838
@Test
3939
public void testSingle() throws IOException {
4040
Set<String> expected = new HashSet<String>();
4141
expected.add("host");
42-
doTestVary("Host", expected);
42+
doTestVary("Host", expected, true);
4343
}
4444

4545

@@ -49,21 +49,21 @@ public void testMultiple() throws IOException {
4949
expected.add("bar");
5050
expected.add("foo");
5151
expected.add("host");
52-
doTestVary("Host, Foo, Bar", expected);
52+
doTestVary("Host, Foo, Bar", expected, true);
5353
}
5454

5555

5656
@Test
5757
public void testEmptyString() throws IOException {
5858
Set<String> s = Collections.emptySet();
59-
doTestVary("", s);
59+
doTestVary("", s, false);
6060
}
6161

6262

6363
@Test
6464
public void testSingleInvalid() throws IOException {
6565
Set<String> s = Collections.emptySet();
66-
doTestVary("{{{", s);
66+
doTestVary("{{{", s, false);
6767
}
6868

6969

@@ -73,7 +73,7 @@ public void testMultipleWithInvalidStart() throws IOException {
7373
expected.add("bar");
7474
expected.add("foo");
7575
expected.add("host");
76-
doTestVary("{{{, Host, Foo, Bar", expected);
76+
doTestVary("{{{, Host, Foo, Bar", expected, false);
7777
}
7878

7979

@@ -83,7 +83,7 @@ public void testMultipleWithInvalidMiddle() throws IOException {
8383
expected.add("bar");
8484
expected.add("foo");
8585
expected.add("host");
86-
doTestVary("Host, {{{, Foo, Bar", expected);
86+
doTestVary("Host, {{{, Foo, Bar", expected, false);
8787
}
8888

8989

@@ -93,7 +93,7 @@ public void testMultipleWithInvalidEnd() throws IOException {
9393
expected.add("bar");
9494
expected.add("foo");
9595
expected.add("host");
96-
doTestVary("Host, Foo, Bar, {{{", expected);
96+
doTestVary("Host, Foo, Bar, {{{", expected, false);
9797
}
9898

9999

@@ -103,7 +103,7 @@ public void testMultipleWithInvalidStart2() throws IOException {
103103
expected.add("bar");
104104
expected.add("foo");
105105
expected.add("host");
106-
doTestVary("OK {{{, Host, Foo, Bar", expected);
106+
doTestVary("OK {{{, Host, Foo, Bar", expected, false);
107107
}
108108

109109

@@ -113,7 +113,7 @@ public void testMultipleWithInvalidMiddle2() throws IOException {
113113
expected.add("bar");
114114
expected.add("foo");
115115
expected.add("host");
116-
doTestVary("Host, OK {{{, Foo, Bar", expected);
116+
doTestVary("Host, OK {{{, Foo, Bar", expected, false);
117117
}
118118

119119

@@ -123,21 +123,80 @@ public void testMultipleWithInvalidEnd2() throws IOException {
123123
expected.add("bar");
124124
expected.add("foo");
125125
expected.add("host");
126-
doTestVary("Host, Foo, Bar, OK {{{", expected);
126+
doTestVary("Host, Foo, Bar, OK {{{", expected, false);
127127
}
128128

129129

130130
@SuppressWarnings("deprecation")
131-
private void doTestVary(String input, Set<String> expected) throws IOException {
131+
private void doTestVary(String input, Set<String> expectedTokens, boolean expectedResult) throws IOException {
132132
StringReader reader = new StringReader(input);
133-
Set<String> result = new HashSet<String>();
134-
Vary.parseVary(reader, result);
135-
Assert.assertEquals(expected, result);
133+
Set<String> tokens = new HashSet<String>();
134+
Vary.parseVary(reader, tokens);
135+
Assert.assertEquals(expectedTokens, tokens);
136136

137137
// Can't use reset(). Parser uses marks.
138138
reader = new StringReader(input);
139-
result.clear();
140-
TokenList.parseTokenList(reader, result);
141-
Assert.assertEquals(expected, result);
139+
tokens.clear();
140+
boolean result = TokenList.parseTokenList(reader, tokens);
141+
Assert.assertEquals(expectedTokens, tokens);
142+
Assert.assertEquals(Boolean.valueOf(expectedResult), Boolean.valueOf(result));
142143
}
144+
145+
146+
@Test
147+
public void testMultipleHeadersValidWithoutNull() throws IOException {
148+
doTestMultipleHeadersValid(false);
149+
}
150+
151+
152+
@Test
153+
public void testMultipleHeadersValidWithNull() throws IOException {
154+
doTestMultipleHeadersValid(true);
155+
}
156+
157+
158+
private void doTestMultipleHeadersValid(boolean withNull) throws IOException {
159+
Set<String> expectedTokens = new HashSet<String>();
160+
expectedTokens.add("bar");
161+
expectedTokens.add("foo");
162+
expectedTokens.add("foo2");
163+
164+
Set<String> inputs = new HashSet<String>();
165+
inputs.add("foo");
166+
if (withNull) {
167+
inputs.add(null);
168+
}
169+
inputs.add("bar, foo2");
170+
171+
Set<String> tokens = new HashSet<String>();
172+
173+
174+
boolean result = TokenList.parseTokenList(Collections.enumeration(inputs), tokens);
175+
Assert.assertEquals(expectedTokens, tokens);
176+
Assert.assertTrue(result);
177+
}
178+
179+
180+
@Test
181+
public void doTestMultipleHeadersInvalid() throws IOException {
182+
Set<String> expectedTokens = new HashSet<String>();
183+
expectedTokens.add("bar");
184+
expectedTokens.add("bar2");
185+
expectedTokens.add("foo");
186+
expectedTokens.add("foo2");
187+
expectedTokens.add("foo3");
188+
189+
Set<String> inputs = new HashSet<String>();
190+
inputs.add("foo");
191+
inputs.add("bar2, }}}, foo3");
192+
inputs.add("bar, foo2");
193+
194+
Set<String> tokens = new HashSet<String>();
195+
196+
197+
boolean result = TokenList.parseTokenList(Collections.enumeration(inputs), tokens);
198+
Assert.assertEquals(expectedTokens, tokens);
199+
Assert.assertFalse(result);
200+
}
201+
143202
}

‎webapps/docs/changelog.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,11 @@
129129
When reporting / logging invalid HTTP headers encode any non-printing
130130
characters using the 0xNN form. (markt)
131131
</add>
132+
<fix>
133+
Correct a regression introduced in 7.0.98 that meant invalid tokens in
134+
the <code>Transfer-Encoding</code> header were ignored rather than
135+
treated as an error. (markt)
136+
</fix>
132137
</changelog>
133138
</subsection>
134139
<subsection name="Jasper">

0 commit comments

Comments
 (0)