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 2013/05/07 17:54:37 UTC
svn commit: r1479953 - in /tomcat/tc7.0.x/trunk: ./
java/org/apache/tomcat/util/http/parser/HttpParser.java
test/org/apache/tomcat/util/http/parser/TestMediaType.java
webapps/docs/changelog.xml
Author: markt
Date: Tue May 7 15:54:36 2013
New Revision: 1479953
URL: http://svn.apache.org/r1479953
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54703
Be tolerant of applications that pass CR or LF in setHeader() values.
Fix some whitespace parsing issues idnetifed by the extended test cases in readTokenOrQuotedString()
Modified:
tomcat/tc7.0.x/trunk/ (props changed)
tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java
tomcat/tc7.0.x/trunk/test/org/apache/tomcat/util/http/parser/TestMediaType.java
tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml
Propchange: tomcat/tc7.0.x/trunk/
------------------------------------------------------------------------------
Merged /tomcat/trunk:r1479248,1479951
Modified: tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java?rev=1479953&r1=1479952&r2=1479953&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java (original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java Tue May 7 15:54:36 2013
@@ -262,17 +262,34 @@ public class HttpParser {
}
}
- private static SkipConstantResult skipConstant(StringReader input,
- String constant) throws IOException {
- int len = constant.length();
+ // Skip any LWS and return the next char
+ private static int skipLws(StringReader input, boolean withReset)
+ throws IOException {
+ if (withReset) {
+ input.mark(1);
+ }
int c = input.read();
- // Skip lws
- while (c == 32 || c == 9) {
+ while (c == 32 || c == 9 || c == 10 || c == 13) {
+ if (withReset) {
+ input.mark(1);
+ }
c = input.read();
}
+ if (withReset) {
+ input.reset();
+ }
+ return c;
+ }
+
+ private static SkipConstantResult skipConstant(StringReader input,
+ String constant) throws IOException {
+ int len = constant.length();
+
+ int c = skipLws(input, false);
+
for (int i = 0; i < len; i++) {
if (i == 0 && c == -1) {
return SkipConstantResult.EOF;
@@ -296,12 +313,7 @@ public class HttpParser {
private static String readToken(StringReader input) throws IOException {
StringBuilder result = new StringBuilder();
- int c = input.read();
-
- // Skip lws
- while (c == 32 || c == 9) {
- c = input.read();
- }
+ int c = skipLws(input, false);
while (c != -1 && isToken(c)) {
result.append((char) c);
@@ -325,12 +337,7 @@ public class HttpParser {
private static String readQuotedString(StringReader input,
boolean returnQuoted) throws IOException {
- int c = input.read();
-
- // Skip lws
- while (c == 32 || c == 9) {
- c = input.read();
- }
+ int c = skipLws(input, false);
if (c != '"') {
return null;
@@ -366,12 +373,8 @@ public class HttpParser {
private static String readTokenOrQuotedString(StringReader input,
boolean returnQuoted) throws IOException {
- // Use mark/reset as skip(-1) fails when reading the last character of
- // the input
- input.mark(1);
- int c = input.read();
- // Go back so first character is available to be read again
- input.reset();
+ // Go back so first non-LWS character is available to be read again
+ int c = skipLws(input, true);
if (c == '"') {
return readQuotedString(input, returnQuoted);
@@ -398,12 +401,7 @@ public class HttpParser {
StringBuilder result = new StringBuilder();
boolean quoted = false;
- int c = input.read();
-
- // Skip lws
- while (c == 32 || c == 9) {
- c = input.read();
- }
+ int c = skipLws(input, false);
if (c == '"') {
quoted = true;
@@ -455,12 +453,7 @@ public class HttpParser {
StringBuilder result = new StringBuilder();
boolean quoted = false;
- int c = input.read();
-
- // Skip lws
- while (c == 32 || c == 9) {
- c = input.read();
- }
+ int c = skipLws(input, false);
if (c == '"') {
quoted = true;
Modified: tomcat/tc7.0.x/trunk/test/org/apache/tomcat/util/http/parser/TestMediaType.java
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/test/org/apache/tomcat/util/http/parser/TestMediaType.java?rev=1479953&r1=1479952&r2=1479953&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/test/org/apache/tomcat/util/http/parser/TestMediaType.java (original)
+++ tomcat/tc7.0.x/trunk/test/org/apache/tomcat/util/http/parser/TestMediaType.java Tue May 7 15:54:36 2013
@@ -46,6 +46,7 @@ public class TestMediaType {
new Parameter("z", "\"\"");
private static final Parameter PARAM_COMPLEX_QUOTED =
new Parameter("w", "\"foo'bar,a=b;x=y\"");
+
private static final String CHARSET = "UTF-8";
private static final String WS_CHARSET = " \tUTF-8";
private static final String CHARSET_WS = "UTF-8 \t";
@@ -61,6 +62,11 @@ public class TestMediaType {
new Parameter("charset", CHARSET_QUOTED);
+ private static final String[] LWS_VALUES = new String[] {
+ "", " ", "\t", "\r", "\n", "\r\n", " \r", " \n", " \r\n",
+ "\r ", "\n ", "\r\n ", " \r ", " \n ", " \r\n " };
+
+
@Test
public void testSimple() throws IOException {
doTest();
@@ -207,10 +213,17 @@ public class TestMediaType {
private void doTest(Parameter... parameters) throws IOException {
+ for (String lws : LWS_VALUES) {
+ doTest(lws, parameters);
+ }
+ }
+
+ private void doTest(String lws, Parameter... parameters)
+ throws IOException {
StringBuilder sb = new StringBuilder();
sb.append(TYPES);
for (Parameter p : parameters) {
- sb.append(p.toString());
+ sb.append(p.toString(lws));
}
StringReader sr = new StringReader(sb.toString());
@@ -250,14 +263,23 @@ public class TestMediaType {
@Override
public String toString() {
+ return toString("");
+ }
+
+ public String toString(String lws) {
StringBuilder sb = new StringBuilder();
+ sb.append(lws);
sb.append(";");
+ sb.append(lws);
sb.append(name);
+ sb.append(lws);
sb.append("=");
+ sb.append(lws);
sb.append(value);
+ sb.append(lws);
return sb.toString();
}
- }
+}
@Test
public void testCase() throws Exception {
Modified: tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml?rev=1479953&r1=1479952&r2=1479953&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml Tue May 7 15:54:36 2013
@@ -56,6 +56,16 @@
issues to not "pop up" wrt. others).
-->
<section name="Tomcat 7.0.41 (markt)">
+ <subsection name="Catalina">
+ <changelog>
+ <fix>
+ <bug>54703</bug>: Make parsing of HTTP Content-Type headers tolerant of
+ any CR or LF characters that appear in the value passed by the
+ application. Also fix some whitespace parsing issues identified by the
+ additional test cases. (markt)
+ </fix>
+ </changelog>
+ </subsection>
<subsection name="Cluster">
<changelog>
<add>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1479953 - in /tomcat/tc7.0.x/trunk: ./
java/org/apache/tomcat/util/http/parser/HttpParser.java test/org/apache/tomcat/util/http/parser/TestMediaType.java
webapps/docs/changelog.xml
Posted by sebb <se...@gmail.com>.
On 7 May 2013 16:54, <ma...@apache.org> wrote:
> Author: markt
> Date: Tue May 7 15:54:36 2013
> New Revision: 1479953
>
> URL: http://svn.apache.org/r1479953
> Log:
> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54703
> Be tolerant of applications that pass CR or LF in setHeader() values.
> Fix some whitespace parsing issues idnetifed by the extended test cases in readTokenOrQuotedString()
>
> Modified:
> tomcat/tc7.0.x/trunk/ (props changed)
> tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java
> tomcat/tc7.0.x/trunk/test/org/apache/tomcat/util/http/parser/TestMediaType.java
> tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml
>
> Propchange: tomcat/tc7.0.x/trunk/
> ------------------------------------------------------------------------------
> Merged /tomcat/trunk:r1479248,1479951
>
> Modified: tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java
> URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java?rev=1479953&r1=1479952&r2=1479953&view=diff
> ==============================================================================
> --- tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java (original)
> +++ tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java Tue May 7 15:54:36 2013
> @@ -262,17 +262,34 @@ public class HttpParser {
> }
> }
>
> - private static SkipConstantResult skipConstant(StringReader input,
> - String constant) throws IOException {
> - int len = constant.length();
> + // Skip any LWS and return the next char
> + private static int skipLws(StringReader input, boolean withReset)
> + throws IOException {
>
> + if (withReset) {
> + input.mark(1);
> + }
> int c = input.read();
>
> - // Skip lws
> - while (c == 32 || c == 9) {
> + while (c == 32 || c == 9 || c == 10 || c == 13) {
There are some other characters that could be considered as WS, e.g. FF and VT.
Should those be allowed for?
Also perhaps easier to read the comparisons as
while (c == ' ' || c == '\t' || c == '\n' || c == '\r') {
That also agrees with how the test case is written.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1479953 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/tomcat/util/http/parser/HttpParser.java
test/org/apache/tomcat/util/http/parser/TestMediaType.java webapps/docs/changelog.xml
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 5/8/13 10:08 AM, Mark Thomas wrote:
> On 08/05/2013 14:22, Christopher Schultz wrote:
>> Mark,
>
>> On 5/7/13 11:54 AM, markt@apache.org wrote:
>>> Author: markt Date: Tue May 7 15:54:36 2013 New Revision:
>>> 1479953
>>>
>>> URL: http://svn.apache.org/r1479953 Log: Fix
>>> https://issues.apache.org/bugzilla/show_bug.cgi?id=54703 Be
>>> tolerant of applications that pass CR or LF in setHeader()
>>> values. Fix some whitespace parsing issues idnetifed by the
>>> extended test cases in readTokenOrQuotedString()
>
>> How does this impact HTTP response-splitting exploits triggered by
>> webapps that don't sanitize their response headers?
>
> It does very little because only Content-Type headers are parsed. The
> likelihood any app vulnerable before this change is still vulenrable.
Aah, I didn't realize this was restricted to Content-Type headers -- I
was only reading the diff itself. Thanks for the clarification.
>> Also:
>
>>> + private static final String[] LWS_VALUES = new String[] { +
>>> "", " ", "\t", "\r", "\n", "\r\n", " \r", " \n", " \r\n", +
>>> "\r ", "\n ", "\r\n ", " \r ", " \n ", " \r\n " };
>
>> Is LWS_VALUES an empty string? Just a sanity check that headers
>> without any leading whitespace don't cause any problems? Seems like
>> many many other tests would verify that...
>
> No, LWS_VALUES is an array of Strings one of which is the empty
> String. Each value in the array is used for a series of tests in turn.
Sorry, I attempted to say "Is LWS_VALUES[0] an empty string?". I see
that you are running tests against each one... I was just wondering if
the empty-string test was just for completeness rather than intending
for it to be a certain type of whitespace (as opposed to none).
Thanks,
-chris
Re: svn commit: r1479953 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/tomcat/util/http/parser/HttpParser.java
test/org/apache/tomcat/util/http/parser/TestMediaType.java webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/05/2013 14:22, Christopher Schultz wrote:
> Mark,
>
> On 5/7/13 11:54 AM, markt@apache.org wrote:
>> Author: markt Date: Tue May 7 15:54:36 2013 New Revision:
>> 1479953
>>
>> URL: http://svn.apache.org/r1479953 Log: Fix
>> https://issues.apache.org/bugzilla/show_bug.cgi?id=54703 Be
>> tolerant of applications that pass CR or LF in setHeader()
>> values. Fix some whitespace parsing issues idnetifed by the
>> extended test cases in readTokenOrQuotedString()
>
> How does this impact HTTP response-splitting exploits triggered by
> webapps that don't sanitize their response headers?
It does very little because only Content-Type headers are parsed. The
likelihood any app vulnerable before this change is still vulenrable.
> Also:
>
>> + private static final String[] LWS_VALUES = new String[] { +
>> "", " ", "\t", "\r", "\n", "\r\n", " \r", " \n", " \r\n", +
>> "\r ", "\n ", "\r\n ", " \r ", " \n ", " \r\n " };
>
> Is LWS_VALUES an empty string? Just a sanity check that headers
> without any leading whitespace don't cause any problems? Seems like
> many many other tests would verify that...
No, LWS_VALUES is an array of Strings one of which is the empty
String. Each value in the array is used for a series of tests in turn.
Mark
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJRilxJAAoJEBDAHFovYFnnpFQP/1J8Z49BdozHxOPNsvq25+WV
Mn9P53L/Dbhq3U/5dr+ZUlApCxsp+RVkFyoqKxdzc9ecOWjRGBrPGLoiBup57UQp
+5jfR/p42iMsgVxD70uJx16oKjsyGM/HIrDWFDf6NkY+mYilMZQXMpRjPNRsGhyQ
g7p/o22nQd+T88aa2IlOVvu9EZSW88DYGPwxKLVmQDI2uC0DygINr1mWqMhK7R7+
DDSVxK/dm30LSRJXTHAiHcbuhU3LbW5fkyOrFMYWCH8jT0vtkAkJhg/BRVoVSwt+
Aw9uK2eX+u+wQ41Z/39/Qx1s8/e/PWnfI+hpHIfCqCMCf5TiVHUxCgAyxA7Ytev1
FraaQm9O61cNQiMvoWEc9/E150LR7YZDNbkCvQ9uH5Ma2gdjkucPB+JP4TUjzhYb
Z4Ff1hC9MOoZnaTjuU8ECrxv39EplTDnPOP9Lie5J+uaSNd3kIy5MZnN1paemZUw
/FxH2L+sz5u+ckYlA/Q9NKnxMcx6srSOLo3jZe0wjT+e08DHl+pMuL8iF1pPBUlw
ub4uil72T8qV6cR5H4Cl1YGsT1b89xsZ9/4y/WiODbeUwND8RYGTVD5fYmMSGJ10
ItmBPTXm86txlV67VbBN/QpQhZGsnvR/M5H5ErNBm+gA/kxACmqJxZHNCzuOo3Hq
vRLtFouYZx3P5UcH/fw/
=JTZo
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1479953 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/tomcat/util/http/parser/HttpParser.java
test/org/apache/tomcat/util/http/parser/TestMediaType.java webapps/docs/changelog.xml
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 5/7/13 11:54 AM, markt@apache.org wrote:
> Author: markt
> Date: Tue May 7 15:54:36 2013
> New Revision: 1479953
>
> URL: http://svn.apache.org/r1479953
> Log:
> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54703
> Be tolerant of applications that pass CR or LF in setHeader() values.
> Fix some whitespace parsing issues idnetifed by the extended test cases in readTokenOrQuotedString()
How does this impact HTTP response-splitting exploits triggered by
webapps that don't sanitize their response headers?
Also:
> + private static final String[] LWS_VALUES = new String[] {
> + "", " ", "\t", "\r", "\n", "\r\n", " \r", " \n", " \r\n",
> + "\r ", "\n ", "\r\n ", " \r ", " \n ", " \r\n " };
Is LWS_VALUES an empty string? Just a sanity check that headers without
any leading whitespace don't cause any problems? Seems like many many
other tests would verify that...
-chris