naviserver-devel

[naviserver-devel] Re: Test Failures: maxinput, maxheaders not honoured (naviserver/nsd driver.c,1.43,1.44)

[Stephen D. <sd...@gm...>]

On 3/1/06, Vlad Seryakov <ser...@us...> wrote:
> Update of /cvsroot/naviserver/naviserver/nsd
> In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv6669/nsd
>
> Modified Files:
>         driver.c
> Log Message:
> Fixed error reporting during
> request parsing, now server returns 414/400 return codes and honors
> maxinput/maxheaders parameters. Test http-4.5 does not hang actually
> but with sndbuf/rcvbug set to so small values it takes very logn time to
> read the request, by putting Ns_Log in SockRead i was seeing reading by
> 64/192 bytes but the server was operational.
>
>
> Index: driver.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> RCS file: /cvsroot/naviserver/naviserver/nsd/driver.c,v
> retrieving revision 1.43
> retrieving revision 1.44
> diff -C2 -d -r1.43 -r1.44
> *** driver.c    1 Mar 2006 04:30:59 -0000       1.43
> --- driver.c    1 Mar 2006 17:08:38 -0000       1.44
> ***************
> *** 1626,1629 ****
> --- 1647,1658 ----
>       reqPtr->avail +=3D n;
>
> +     /*
> +      * Check the hard limit for max uploaded content size
> +      */
> +
> +     if (reqPtr->avail > sockPtr->drvPtr->maxinput) {
> +         return SOCK_ENTITYTOOLARGE;
> +     }
> +
>       return SockParse(sockPtr, spooler);
>   }
> ***************


I think this is checking that the amount of data *already* received is
larger than maxinput, and if so, then rejecting.  The idea is to not
read that much data in the first place.

I didn't look carefully so it may only be overshooting by the receive
buffer size or some such, but the same principle applies to the
following check for max headers.  Here the headers have been
completely parsed into an Ns_Set structure, and *then* the size is
checked.  Too late...  :-(

> +             /*
> +              * Check for max number of headers
> +              */
> +
> +             if (Ns_SetSize(reqPtr->headers) > sockPtr->drvPtr->maxheade=
rs) {
> +                 return SOCK_BADREQUEST;
> +             }
> +

Re: [naviserver-devel] Re: Test Failures: maxinput, maxheaders not honoured (naviserver/nsd driver.c,1.43,1.44)

[Vlad S. <vl...@cr...>]

> I think this is checking that the amount of data *already* received is
> larger than maxinput, and if so, then rejecting.  The idea is to not
> read that much data in the first place.


Yes, by the last buffer size only, it may not read the whole buffer, so 
cheking befor for requested size is not accurate yet, i may ask for 4096 
bytes but read only 100 for whatever reasons.

> I didn't look carefully so it may only be overshooting by the receive
> buffer size or some such, but the same principle applies to the
> following check for max headers.  Here the headers have been
> completely parsed into an Ns_Set structure, and *then* the size is
> checked.  Too late...  :-(

It does it after each line only, whatever amount was read is in the 
buffer already, so we parse it line by line and i check after each line.

Re: [naviserver-devel] Re: Test Failures: maxinput, maxheaders not honoured (naviserver/nsd driver.c,1.43,1.44)

[Stephen D. <sd...@gm...>]

On 3/1/06, Vlad Seryakov <vl...@cr...> wrote:
> > I think this is checking that the amount of data *already* received is
> > larger than maxinput, and if so, then rejecting.  The idea is to not
> > read that much data in the first place.
>
>
> Yes, by the last buffer size only, it may not read the whole buffer, so
> cheking befor for requested size is not accurate yet, i may ask for 4096
> bytes but read only 100 for whatever reasons.


What happens if you set maxinput to 20MB, and a user tries to upload a
21MB image?  Do they get an error message after uploading 20MB + ~8k?=20
That's how I'm reading it.

All modern browsers send a content-length header.  Maybe we should
check the reqPtr->length field as soon as it's parsed.  Round about
line 1760 in driver.c:

            s =3D Ns_SetIGet(reqPtr->headers, "content-length");
            if (s !=3D NULL) {
                int length;

                /*
                 * Honour meaningfull remote
                 * content-length hints only.
                 */

                length =3D atoi(s);
                if (length >=3D 0) {
                    reqPtr->length =3D length;
                }
                /* Check for maxinput here? */
            }


> > I didn't look carefully so it may only be overshooting by the receive
> > buffer size or some such, but the same principle applies to the
> > following check for max headers.  Here the headers have been
> > completely parsed into an Ns_Set structure, and *then* the size is
> > checked.  Too late...  :-(
>
> It does it after each line only, whatever amount was read is in the
> buffer already, so we parse it line by line and i check after each line.


It's kind of odd when reading the code that it overshoots by one, but OK...

Re: [naviserver-devel] Re: Test Failures: maxinput, maxheaders not honoured (naviserver/nsd driver.c,1.43,1.44)

[Vlad S. <vl...@cr...>]

Right, good hint

Stephen Deasey wrote:
> On 3/1/06, Vlad Seryakov <vl...@cr...> wrote:
>>> I think this is checking that the amount of data *already* received is
>>> larger than maxinput, and if so, then rejecting.  The idea is to not
>>> read that much data in the first place.
>>
>> Yes, by the last buffer size only, it may not read the whole buffer, so
>> cheking befor for requested size is not accurate yet, i may ask for 4096
>> bytes but read only 100 for whatever reasons.
> 
> 
> What happens if you set maxinput to 20MB, and a user tries to upload a
> 21MB image?  Do they get an error message after uploading 20MB + ~8k? 
> That's how I'm reading it.
> 
> All modern browsers send a content-length header.  Maybe we should
> check the reqPtr->length field as soon as it's parsed.  Round about
> line 1760 in driver.c:
> 
>             s = Ns_SetIGet(reqPtr->headers, "content-length");
>             if (s != NULL) {
>                 int length;
> 
>                 /*
>                  * Honour meaningfull remote
>                  * content-length hints only.
>                  */
> 
>                 length = atoi(s);
>                 if (length >= 0) {
>                     reqPtr->length = length;
>                 }
>                 /* Check for maxinput here? */
>             }
> 
> 
>>> I didn't look carefully so it may only be overshooting by the receive
>>> buffer size or some such, but the same principle applies to the
>>> following check for max headers.  Here the headers have been
>>> completely parsed into an Ns_Set structure, and *then* the size is
>>> checked.  Too late...  :-(
>> It does it after each line only, whatever amount was read is in the
>> buffer already, so we parse it line by line and i check after each line.
> 
> 
> It's kind of odd when reading the code that it overshoots by one, but OK...
> 
> 
> -------------------------------------------------------
> This SF.Net email is sponsored by xPML, a groundbreaking scripting language
> that extends applications into web and mobile media. Attend the live webcast
> and join the prime developer group breaking into this new coding territory!
> http://sel.as-us.falkag.net/sel?cmd=k&kid0944&bid$1720&dat1642
> _______________________________________________
> naviserver-devel mailing list
> nav...@li...
> https://lists.sourceforge.net/lists/listinfo/naviserver-devel
>