tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Filip Hanik - Dev Lists <devli...@hanik.com>
Subject Re: bug in ChunkedInputFilter
Date Fri, 14 Jul 2006 01:53:42 GMT
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
>
>


Mime
View raw message