Re: svn commit: r290965 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_io.c ssl_engine_kernel.c ssl_private.h

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

jorton@apache.org wrote:
> Author: jorton
> Date: Thu Sep 22 08:38:14 2005
> New Revision: 290965
> 
> URL: http://svn.apache.org/viewcvs?rev=290965&view=rev
> Log:
> Implement a (bounded) buffer of request body data to provide a limited
> but safe fix for the mod_ssl renegotiation-vs-requests-with-bodies

You did happen to notice that the code required here matches exactly the
new http proxy spool_reqbody_cl code?

I mentioned before that apreq aught to be plugged in, but I've noticed
that the model is upside down...

We need -ONE- filter to simply slurp bodies, set them aside in a file as
necessary, and then replace the sock/memory bucket with the 'bounded' 
memory bucket (for -short- responses), and when the bound overflows,
either a file bucket or mmap bucket of the slurped body.

This filter simply needs to live in core.  apreq can use it.  ssl can
use it.  proxy_http can use it.

And if it's a single filter, then (if nobody transforms the file or mmap
bucket) we don't duplicate this buffer.

Comments?

Bill

Re: svn commit: r290965 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_io.c ssl_engine_kernel.c ssl_private.h

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Joe Orton wrote:
> On Thu, Sep 22, 2005 at 01:43:36PM -0500, William Rowe wrote:
> 
>>jorton@apache.org wrote:
>>
>>>Author: jorton
>>>Date: Thu Sep 22 08:38:14 2005
>>>New Revision: 290965
>>>
>>>URL: http://svn.apache.org/viewcvs?rev=290965&view=rev
>>>Log:
>>>Implement a (bounded) buffer of request body data to provide a limited
>>>but safe fix for the mod_ssl renegotiation-vs-requests-with-bodies
>>
>>You did happen to notice that the code required here matches exactly the
>>new http proxy spool_reqbody_cl code?
> 
> It's similar but not the same, the proxy can pass brigades on but this 
> code cannot, so it has to setaside the buckets into the special pool.  I 
> doubt there is an opportunity for worthwhile refactoring here but I'd be 
> happy to be proven wrong ;)
> 
> There's not really that much in common between the two functions beyond 
> the "get brigade from input filters, iterate over brigade doing stuff" 
> idiom which naturally is repeated across the entire code base.

Hmmm... you are right, I'm thinking this might not be an httpd function,
but an apr-util function?  The set-aside can be made identical, for
certain, but perhaps not as a filter, but instead, what about a brigade
transformation function?

The function I mentioned was the one which sets aside the entire body
to determine the C-L... not the other two flavors which stream without
any setasides.

>>I mentioned before that apreq aught to be plugged in, but I've noticed
>>that the model is upside down...
>>
>>We need -ONE- filter to simply slurp bodies, set them aside in a file as
>>necessary, and then replace the sock/memory bucket with the 'bounded' 
>>memory bucket (for -short- responses), and when the bound overflows,
>>either a file bucket or mmap bucket of the slurped body.
> 
> I certainly don't want the buffering to files behaviour for this case, 
> that's a DoS attack waiting to happen ("why did my SSL server stop 
> working properly? hey, and why is my /var/tmp full of these silly temp 
> files?").

Yes, the function would have to have limits as to what will be set aside
in the file system, obviously some flags are necessary to control the
behavior, specify the tmp location, etc.

Bill

Re: svn commit: r290965 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_io.c ssl_engine_kernel.c ssl_private.h

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Joe Orton wrote:
> On Thu, Sep 22, 2005 at 01:43:36PM -0500, William Rowe wrote:
> 
>>jorton@apache.org wrote:
>>
>>>Author: jorton
>>>Date: Thu Sep 22 08:38:14 2005
>>>New Revision: 290965
>>>
>>>URL: http://svn.apache.org/viewcvs?rev=290965&view=rev
>>>Log:
>>>Implement a (bounded) buffer of request body data to provide a limited
>>>but safe fix for the mod_ssl renegotiation-vs-requests-with-bodies
>>
>>You did happen to notice that the code required here matches exactly the
>>new http proxy spool_reqbody_cl code?
> 
> It's similar but not the same, the proxy can pass brigades on but this 
> code cannot, so it has to setaside the buckets into the special pool.  I 
> doubt there is an opportunity for worthwhile refactoring here but I'd be 
> happy to be proven wrong ;)
> 
> There's not really that much in common between the two functions beyond 
> the "get brigade from input filters, iterate over brigade doing stuff" 
> idiom which naturally is repeated across the entire code base.

Hmmm... you are right, I'm thinking this might not be an httpd function,
but an apr-util function?  The set-aside can be made identical, for
certain, but perhaps not as a filter, but instead, what about a brigade
transformation function?

The function I mentioned was the one which sets aside the entire body
to determine the C-L... not the other two flavors which stream without
any setasides.

>>I mentioned before that apreq aught to be plugged in, but I've noticed
>>that the model is upside down...
>>
>>We need -ONE- filter to simply slurp bodies, set them aside in a file as
>>necessary, and then replace the sock/memory bucket with the 'bounded' 
>>memory bucket (for -short- responses), and when the bound overflows,
>>either a file bucket or mmap bucket of the slurped body.
> 
> I certainly don't want the buffering to files behaviour for this case, 
> that's a DoS attack waiting to happen ("why did my SSL server stop 
> working properly? hey, and why is my /var/tmp full of these silly temp 
> files?").

Yes, the function would have to have limits as to what will be set aside
in the file system, obviously some flags are necessary to control the
behavior, specify the tmp location, etc.

Bill

Re: svn commit: r290965 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_io.c ssl_engine_kernel.c ssl_private.h

[Joe Orton <jo...@redhat.com>]

On Thu, Sep 22, 2005 at 01:43:36PM -0500, William Rowe wrote:
> jorton@apache.org wrote:
> >Author: jorton
> >Date: Thu Sep 22 08:38:14 2005
> >New Revision: 290965
> >
> >URL: http://svn.apache.org/viewcvs?rev=290965&view=rev
> >Log:
> >Implement a (bounded) buffer of request body data to provide a limited
> >but safe fix for the mod_ssl renegotiation-vs-requests-with-bodies
> 
> You did happen to notice that the code required here matches exactly the
> new http proxy spool_reqbody_cl code?

It's similar but not the same, the proxy can pass brigades on but this 
code cannot, so it has to setaside the buckets into the special pool.  I 
doubt there is an opportunity for worthwhile refactoring here but I'd be 
happy to be proven wrong ;)

There's not really that much in common between the two functions beyond 
the "get brigade from input filters, iterate over brigade doing stuff" 
idiom which naturally is repeated across the entire code base.

> I mentioned before that apreq aught to be plugged in, but I've noticed
> that the model is upside down...
> 
> We need -ONE- filter to simply slurp bodies, set them aside in a file as
> necessary, and then replace the sock/memory bucket with the 'bounded' 
> memory bucket (for -short- responses), and when the bound overflows,
> either a file bucket or mmap bucket of the slurped body.

I certainly don't want the buffering to files behaviour for this case, 
that's a DoS attack waiting to happen ("why did my SSL server stop 
working properly? hey, and why is my /var/tmp full of these silly temp 
files?").

Regards,

joe

Re: svn commit: r290965 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_io.c ssl_engine_kernel.c ssl_private.h

[Joe Orton <jo...@redhat.com>]

On Thu, Sep 22, 2005 at 01:43:36PM -0500, William Rowe wrote:
> jorton@apache.org wrote:
> >Author: jorton
> >Date: Thu Sep 22 08:38:14 2005
> >New Revision: 290965
> >
> >URL: http://svn.apache.org/viewcvs?rev=290965&view=rev
> >Log:
> >Implement a (bounded) buffer of request body data to provide a limited
> >but safe fix for the mod_ssl renegotiation-vs-requests-with-bodies
> 
> You did happen to notice that the code required here matches exactly the
> new http proxy spool_reqbody_cl code?

It's similar but not the same, the proxy can pass brigades on but this 
code cannot, so it has to setaside the buckets into the special pool.  I 
doubt there is an opportunity for worthwhile refactoring here but I'd be 
happy to be proven wrong ;)

There's not really that much in common between the two functions beyond 
the "get brigade from input filters, iterate over brigade doing stuff" 
idiom which naturally is repeated across the entire code base.

> I mentioned before that apreq aught to be plugged in, but I've noticed
> that the model is upside down...
> 
> We need -ONE- filter to simply slurp bodies, set them aside in a file as
> necessary, and then replace the sock/memory bucket with the 'bounded' 
> memory bucket (for -short- responses), and when the bound overflows,
> either a file bucket or mmap bucket of the slurped body.

I certainly don't want the buffering to files behaviour for this case, 
that's a DoS attack waiting to happen ("why did my SSL server stop 
working properly? hey, and why is my /var/tmp full of these silly temp 
files?").

Regards,

joe