Found bug in multipart parser

[Jean-François Meessen <jf...@n-tech.be>]

Hi,

Needing Apache::Upload for an intranet application I developped at Alcatel 
Bell. I discovered a bug preventing people using Netscape 7 to upload some 
files (e.g. PDF documents)

In the parser function split_on_bdry, if a partial match is detected just 
before a bucket boundary and a real boundary just after it, the parser fails 
to detect it.
If the boundary string is for example \r\n----------123456 and the form data 
contains:
The end of the file content...\r\n[BUCKET BOUNDARY]\r\n----------123456
The \r\n before bucket boundary will be a partial match and the following \r\n 
will be interpreted as a mismatch (because the first 2 chars have already 
been matched). In this case, the parser will not detect the boundary.

It seems that using Netscape 7, in some cases a bucket boundary is induced 
just after the file content. And PDF files are terminated by a CR (\015) that 
matches the first byte of boundary. 

Here is the correction I made and the unit test improvement to check it:
diff -u -r1.49 apreq_parsers.c
--- src/apreq_parsers.c 21 Jun 2004 17:49:18 -0000      1.49
+++ src/apreq_parsers.c 25 Jun 2004 21:29:24 -0000
@@ -668,7 +668,8 @@
             continue;
         }

-        if (strncmp(bdry + off, buf, MIN(len, blen - off)) == 0) {
+mfd_split_bdry_restart:
+               if (strncmp(bdry + off, buf, MIN(len, blen - off)) == 0) {
             if ( len >= blen - off ) {
                 /* complete match */
                 if (len > blen - off)
@@ -695,6 +696,7 @@
                 APR_BRIGADE_INSERT_TAIL(out, f);
             } while (e != APR_BRIGADE_FIRST(in));
             off = 0;
+                       goto mfd_split_bdry_restart;
         }

         if (pattern != NULL && len >= blen) {
@@ -1071,7 +1073,7 @@
                                                        param->bb, ctx->bb);

                 if (param->v.status != APR_SUCCESS)
-                    return s;
+                    return param->v.status;

                 ctx->status = MFD_NEXTLINE;
                 goto mfd_parse_brigade;

===================================================================
diff -u -r1.14 parsers.c
--- t/parsers.c 19 Jun 2004 20:03:59 -0000      1.14
+++ t/parsers.c 25 Jun 2004 21:30:11 -0000
@@ -32,7 +32,7 @@
 "--AaB03x" CRLF
 "content-disposition: form-data; name=\"pics\"; filename=\"file1.txt\"" CRLF
 "Content-Type: text/plain" CRLF CRLF
-"... contents of file1.txt ..." CRLF
+"... contents of file1.txt ..." CRLF CRLF
 "--AaB03x--" CRLF;

 extern apr_bucket_brigade *bb;
@@ -120,8 +120,8 @@
         t = apreq_value_to_param(apreq_strtoval(val))->info;
         bb = apreq_value_to_param(apreq_strtoval(val))->bb;
         apr_brigade_pflatten(bb, (char **)&val, &len, p);
-        CuAssertIntEquals(tc,strlen("... contents of file1.txt ..."), len);
-        CuAssertStrNEquals(tc,"... contents of file1.txt ...", val, len);
+        CuAssertIntEquals(tc,strlen("... contents of file1.txt ..." CRLF), 
len);
+        CuAssertStrNEquals(tc,"... contents of file1.txt ..." CRLF, val, 
len);
         val = apr_table_get(t, "content-type");
         CuAssertStrEquals(tc, "text/plain", val);
     }


I also discovered that an error status was not returned correctly and fixed it 
in the patch.

I hope it will help


Jean-François

-- 
MEESSEN Jean-François
Rue Baron de Castro, 61/1
1040 Brussels
BELGIUM

Re: Found bug in multipart parser

[Joe Schaefer <jo...@sunstarsys.com>]

Jean-François Meessen <jf...@n-tech.be> writes:

[...]

> In the parser function split_on_bdry, if a partial match is detected just 
> before a bucket boundary and a real boundary just after it, the parser
> fails to detect it.  If the boundary string is for example
> \r\n----------123456 and the form data contains:
> The end of the file content...\r\n[BUCKET BOUNDARY]\r\n----------123456
> The \r\n before bucket boundary will be a partial match and the
> following \r\n will be interpreted as a mismatch (because the first 2
> chars have already been matched). In this case, the parser will not
> detect the boundary. 

Thanks alot!  Your patch looks correct to me, although there's
another way to fix apreq_parsers.c without the gotos:

Index: src/apreq_parsers.c
===================================================================
RCS file: /home/cvs/httpd-apreq-2/src/apreq_parsers.c,v
retrieving revision 1.49
diff -u -r1.49 apreq_parsers.c
--- src/apreq_parsers.c 21 Jun 2004 17:49:18 -0000      1.49
+++ src/apreq_parsers.c 26 Jun 2004 00:49:30 -0000
@@ -711,7 +711,7 @@
         else
             idx = apreq_index(buf, len, bdry, blen, APREQ_MATCH_PARTIAL);

-        if (idx > 0)
+        if (idx >= 0)
             apr_bucket_split(e, idx);

         APR_BUCKET_REMOVE(e);


It's a simpler patch, but is less efficient than yours
because we're basically wasting an extra pattern match 
and an empty bucket creation instead of just jumping 
back to the start.

[...]

> I also discovered that an error status was not returned correctly
> and fixed it in the patch.
> 
> I hope it will help

Absolutely- thanks!

-- 
Joe Schaefer