tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jean-frederic Clere <jfcl...@gmail.com>
Subject Re: bug in ChunkedInputFilter
Date Fri, 14 Jul 2006 06:38:01 GMT
Please tell: TC ignores the chunk-extension instead "ignore this data".

Cheers

Jean-Frederic

Filip Hanik - Dev Lists wrote:

> Here is the promised patch for the ChunkedInputFilter, please review 
> and comment,
>
> Filip
> -------------------------------------------------------------------------------------

>
> Index: java/org/apache/coyote/http11/Constants.java
> ===================================================================
> --- java/org/apache/coyote/http11/Constants.java    (revision 417384)
> +++ java/org/apache/coyote/http11/Constants.java    (working copy)
> @@ -83,8 +83,14 @@
>      * COLON.
>      */
>     public static final byte COLON = (byte) ':';
> +   +    /**
> +     * SEMI_COLON.
> +     */
> +    public static final byte SEMI_COLON = (byte) ';';
>
>
> +
>     /**
>      * 'A'.
>      */
> Index: java/org/apache/coyote/http11/filters/ChunkedInputFilter.java
> ===================================================================
> --- java/org/apache/coyote/http11/filters/ChunkedInputFilter.java    
> (revision 417384)
> +++ java/org/apache/coyote/http11/filters/ChunkedInputFilter.java    
> (working copy)
> @@ -27,9 +27,11 @@
> import org.apache.coyote.http11.InputFilter;
>
> /**
> - * Chunked input filter.
> + * Chunked input filter. Parses chunked data according to
> + * <a 
> href="http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.6.1">http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.6.1</a><br>

>
>  *
>  * @author Remy Maucherat
> + * @author Filip Hanik
>  */
> public class ChunkedInputFilter implements InputFilter {
>
> @@ -127,7 +129,7 @@
>
>         if (remaining <= 0) {
>             if (!parseChunkHeader()) {
> -                throw new IOException("Invalid chunk");
> +                throw new IOException("Invalid chunk header");
>             }
>             if (endChunk) {
>                 parseEndChunk();
> @@ -234,6 +236,12 @@
>
>     /**
>      * Parse the header of a chunk.
> +     * A chunk header can look like
> +     * A10CRLF
> +     * F23;ignore this dataCRLF
> +     * The letters before CRLF or the trailer mark, must be valid hex 
> digits,
> +     * we should not parse
> +     * F23IAMGONNAMESSTHISUP34CRLF
>      */
>     protected boolean parseChunkHeader()
>         throws IOException {
> @@ -241,6 +249,7 @@
>         int result = 0;
>         boolean eol = false;
>         boolean readDigit = false;
> +        boolean trailer = false;
>
>         while (!eol) {
>
> @@ -252,11 +261,18 @@
>             if (buf[pos] == Constants.CR) {
>             } else if (buf[pos] == Constants.LF) {
>                 eol = true;
> -            } else {
> +            } else if (buf[pos] == Constants.SEMI_COLON) {
> +                trailer = true;
> +            } else if (!trailer) {
> +                //don't read data after the trailer
>                 if (HexUtils.DEC[buf[pos]] != -1) {
>                     readDigit = true;
>                     result *= 16;
>                     result += HexUtils.DEC[buf[pos]];
> +                } else {
> +                    //we shouldn't allow invalid, non hex characters
> +                    //in the chunked header
> +                    return false;
>                 }
>             }
>
>
>
> -------------------------------------------------------------------------------------

>
>
> Filip Hanik - Dev Lists wrote:
>
>> Remy Maucherat wrote:
>>
>>> Filip Hanik - Dev Lists wrote:
>>>
>>>> gents, I know, back again, and totally bastardizing the HTTP protocol,
>>>>
>>>> I have a test request that looks like this:
>>>> ----------------------------------
>>>> POST /comet HTTP/1.1CRLF
>>>> User-Agent: Jakarta Commons-HttpClient/3.0.1CRLF
>>>> Host: 127.0.0.1:8080CRLF
>>>> Transfer-Encoding: chunkedCRLF
>>>> CRLF
>>>> 10CRLF
>>>> test data test 1CRLF
>>>> ---------------------------------------
>>>>
>>>> If I keep sending up
>>>> ---------------------------------------
>>>> 10CRLF
>>>> test data test 1CRLF
>>>> ---------------------------------------
>>>>
>>>> we are all happy, however, if I bastardize the next request and 
>>>> send up the same request twice, so the 2nd request of the HTTP 
>>>> request above becomes the body of the first chunked request, then 
>>>> suddenly the chunked input filter will set its remaining bytes to 
>>>> over 50k remaining bytes, and everything gets streamed into the 
>>>> servlet.
>>>>
>>>> The ChunkedInputFilter is written to handle correct chunks very 
>>>> well, but when incorrect chunked bodies come through, it passes all 
>>>> the content through, instead of throwing an error.
>>>>
>>>> If no one objects, I would like to add in some error checking, even 
>>>> though it is the clients responsibility to provide correct data, 
>>>> the web server should not let incorrect data through as that could 
>>>> be a semi http buffer overflow :)
>>>
>>>
>>> The only thing which determines if a chunk is correct is if the 
>>> chunk length is valid and readable. The parseChunkHeader method 
>>> includes the ability to return false and throw an IOE if an invalid 
>>> header is received.
>>
>> yes, and there is where the bug lies, it doesn't throw an IOException 
>> when it gets an invalid header, I'll fix it and submit the patch for 
>> review so that we can get it taken care of.
>>
>> Filip
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>>
>
>------------------------------------------------------------------------
>
>Index: java/org/apache/coyote/http11/Constants.java
>===================================================================
>--- java/org/apache/coyote/http11/Constants.java	(revision 417384)
>+++ java/org/apache/coyote/http11/Constants.java	(working copy)
>@@ -83,8 +83,14 @@
>      * COLON.
>      */
>     public static final byte COLON = (byte) ':';
>+    
>+    /**
>+     * SEMI_COLON.
>+     */
>+    public static final byte SEMI_COLON = (byte) ';';
> 
> 
>+
>     /**
>      * 'A'.
>      */
>Index: java/org/apache/coyote/http11/filters/ChunkedInputFilter.java
>===================================================================
>--- java/org/apache/coyote/http11/filters/ChunkedInputFilter.java	(revision 417384)
>+++ java/org/apache/coyote/http11/filters/ChunkedInputFilter.java	(working copy)
>@@ -27,9 +27,11 @@
> import org.apache.coyote.http11.InputFilter;
> 
> /**
>- * Chunked input filter.
>+ * Chunked input filter. Parses chunked data according to
>+ * <a href="http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.6.1">http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.6.1</a><br>
>  * 
>  * @author Remy Maucherat
>+ * @author Filip Hanik
>  */
> public class ChunkedInputFilter implements InputFilter {
> 
>@@ -127,7 +129,7 @@
> 
>         if (remaining <= 0) {
>             if (!parseChunkHeader()) {
>-                throw new IOException("Invalid chunk");
>+                throw new IOException("Invalid chunk header");
>             }
>             if (endChunk) {
>                 parseEndChunk();
>@@ -234,6 +236,12 @@
> 
>     /**
>      * Parse the header of a chunk.
>+     * A chunk header can look like 
>+     * A10CRLF
>+     * F23;ignore this dataCRLF
>+     * The letters before CRLF or the trailer mark, must be valid hex digits, 
>+     * we should not parse 
>+     * F23IAMGONNAMESSTHISUP34CRLF
>      */
>     protected boolean parseChunkHeader()
>         throws IOException {
>@@ -241,6 +249,7 @@
>         int result = 0;
>         boolean eol = false;
>         boolean readDigit = false;
>+        boolean trailer = false;
> 
>         while (!eol) {
> 
>@@ -252,11 +261,18 @@
>             if (buf[pos] == Constants.CR) {
>             } else if (buf[pos] == Constants.LF) {
>                 eol = true;
>-            } else {
>+            } else if (buf[pos] == Constants.SEMI_COLON) {
>+                trailer = true;
>+            } else if (!trailer) { 
>+                //don't read data after the trailer
>                 if (HexUtils.DEC[buf[pos]] != -1) {
>                     readDigit = true;
>                     result *= 16;
>                     result += HexUtils.DEC[buf[pos]];
>+                } else {
>+                    //we shouldn't allow invalid, non hex characters
>+                    //in the chunked header
>+                    return false;
>                 }
>             }
> 
>
>  
>
>------------------------------------------------------------------------
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>For additional commands, e-mail: dev-help@tomcat.apache.org
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message