Re: mod_deflate DoS using HEAD
On Tuesday 22 June 2010, Plüm, Rüdiger, VF-Group wrote: > I am currently +0 on wether to use the patch above or my original > proposal. Both have its pros and cons (Saving more CPU vs. be more > picky about caching and implement an RFC SHOULD). I have now commited your original patch because it is less likely to break something. If the CPU usage for compressing the first buffer turns out to be a problem, we can still change it later. Cheers, Stefan
RE: mod_deflate DoS using HEAD
> -Original Message- > From: Eric Covener > Sent: Dienstag, 22. Juni 2010 15:33 > To: dev@httpd.apache.org > Subject: Re: mod_deflate DoS using HEAD > > On Tue, Jun 22, 2010 at 2:37 AM, Ruediger Pluem > wrote: > > Does someone still have Eric's patch? Seems that I can't > find it right now. > > This is what I have, but I never got off the fence back then > much less now: > > Index: modules/filters/mod_deflate.c > === > --- modules/filters/mod_deflate.c (revision 793619) > +++ modules/filters/mod_deflate.c (working copy) > @@ -578,7 +578,7 @@ > deflate_check_etag(r, "gzip"); > > /* For a 304 response, only change the headers */ > -if (r->status == HTTP_NOT_MODIFIED) { > +if (r->status == HTTP_NOT_MODIFIED || r->header_only) { > ap_remove_output_filter(f); > return ap_pass_brigade(f->next, bb); > } > > Thanks for that. I guess the patch is not complete for current trunk. IMHO it should look like: Index: modules/filters/mod_deflate.c === --- modules/filters/mod_deflate.c (revision 955960) +++ modules/filters/mod_deflate.c (working copy) @@ -562,7 +562,7 @@ * send out the headers). */ -if (r->status != HTTP_NOT_MODIFIED) { +if ((r->status != HTTP_NOT_MODIFIED) && !r->header_only) { ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx)); ctx->bb = apr_brigade_create(r->pool, f->c->bucket_alloc); ctx->buffer = apr_palloc(r->pool, c->bufferSize); @@ -616,7 +616,7 @@ deflate_check_etag(r, "gzip"); /* For a 304 response, only change the headers */ -if (r->status == HTTP_NOT_MODIFIED) { +if ((r->status == HTTP_NOT_MODIFIED) || r->header_only) { ap_remove_output_filter(f); return ap_pass_brigade(f->next, bb); } I am currently +0 on wether to use the patch above or my original proposal. Both have its pros and cons (Saving more CPU vs. be more picky about caching and implement an RFC SHOULD). Regards Rüdiger
Re: mod_deflate DoS using HEAD
On Tue, Jun 22, 2010 at 2:37 AM, Ruediger Pluem wrote: > Does someone still have Eric's patch? Seems that I can't find it right now. This is what I have, but I never got off the fence back then much less now: Index: modules/filters/mod_deflate.c === --- modules/filters/mod_deflate.c (revision 793619) +++ modules/filters/mod_deflate.c (working copy) @@ -578,7 +578,7 @@ deflate_check_etag(r, "gzip"); /* For a 304 response, only change the headers */ -if (r->status == HTTP_NOT_MODIFIED) { +if (r->status == HTTP_NOT_MODIFIED || r->header_only) { ap_remove_output_filter(f); return ap_pass_brigade(f->next, bb); } -- Eric Covener cove...@gmail.com
Re: mod_deflate DoS using HEAD
On 06/21/2010 11:37 PM, William A. Rowe Jr. wrote: > On 6/21/2010 4:00 PM, Stefan Fritsch wrote: >> As I understand it, Rüdiger's patch may be better for caching but uses >> more CPU cycles. But it uses way less CPU than no patch at all. >> Therefore I propose to include that patch unless there is clear >> consensus that Eric's patch is to be preferred. > > Not a significant number, and Rüdiger's patch gathered +1's from myself, > gregames, nick is on the wall with a +.5 - I think your question is to > Rüdiger, with the emphasis on 'what is your decision?' based on this > last rather indecisive posting. Does someone still have Eric's patch? Seems that I can't find it right now. Regards Rüdiger
Re: mod_deflate DoS using HEAD
On 6/21/2010 4:00 PM, Stefan Fritsch wrote: > > As I understand it, Rüdiger's patch may be better for caching but uses > more CPU cycles. But it uses way less CPU than no patch at all. > Therefore I propose to include that patch unless there is clear > consensus that Eric's patch is to be preferred. Not a significant number, and Rüdiger's patch gathered +1's from myself, gregames, nick is on the wall with a +.5 - I think your question is to Rüdiger, with the emphasis on 'what is your decision?' based on this last rather indecisive posting. On 7/16/2009 9:24 AM, "Plüm, Rüdiger, VF-Group" wrote: >> >> For a large static file, Ruedigers patch suppresses the C-L entirely >> (it gets added back in down the chain for my patch, for static files >> at least) which I thought would make that prefered, if we're confident >> that we'll never do more than a zlib buffer worth of work the first >> go-round. > > Good point. So your patch would invalidate a cached entity if the > response to a GET delivered a C-L header, since HEAD and GET would > deliver different C-L headers. > OTOH I think only very small or extremely compressable responses (whether > static or not) would have a C-L in the response to a GET, because everything > that exceeeds a zlib buffer would be delivered chunked anyway.
Re: mod_deflate DoS using HEAD
On Thursday 16 July 2009, William A. Rowe, Jr. wrote: > Plüm, Rüdiger, VF-Group wrote: > > Good point. So your patch would invalidate a cached entity if the > > response to a GET delivered a C-L header, since HEAD and GET > > would deliver different C-L headers. > > OTOH I think only very small or extremely compressable responses > > (whether static or not) would have a C-L in the response to a > > GET, because everything that exceeeds a zlib buffer would be > > delivered chunked anyway. > > We don't really want to gzip that single buffer though, either. > The prime concern here is CPU cycles. In this case, there is no > advantage to performing that compression, and inconsistent > behavior leads cache and proxy authors down unfortunate > assumptions. Going back to that old thread, was there consensus on which patch is preferable? As I understand it, Rüdiger's patch may be better for caching but uses more CPU cycles. But it uses way less CPU than no patch at all. Therefore I propose to include that patch unless there is clear consensus that Eric's patch is to be preferred. As an added data point, Rüdiger's patch is in Debian 5.0 and various Ubuntus and AFAIK hasn't caused any issues.
Re: mod_deflate DoS using HEAD
Plüm, Rüdiger, VF-Group wrote: > > Good point. So your patch would invalidate a cached entity if the > response to a GET delivered a C-L header, since HEAD and GET would > deliver different C-L headers. > OTOH I think only very small or extremely compressable responses (whether > static or not) would have a C-L in the response to a GET, because everything > that exceeeds a zlib buffer would be delivered chunked anyway. We don't really want to gzip that single buffer though, either. The prime concern here is CPU cycles. In this case, there is no advantage to performing that compression, and inconsistent behavior leads cache and proxy authors down unfortunate assumptions.
RE: mod_deflate DoS using HEAD
> -Original Message- > From: Eric Covener > Sent: Donnerstag, 16. Juli 2009 16:13 > To: dev@httpd.apache.org > Subject: Re: mod_deflate DoS using HEAD > > On Wed, Jul 15, 2009 at 5:19 PM, Nick Kew wrote: > > William A. Rowe, Jr. wrote: > > > >> So +1 to the proposed patch; in fact, +1 on unsetting C-L > and treating > >> HEAD to the same processing as 304. > > > > +1. Since it's a SHOULD not a MUST, we can be pragmatic > > with the headers. > > > > That's back to Eric's original patch, isn't it? > > For a large static file, Ruedigers patch suppresses the C-L entirely > (it gets added back in down the chain for my patch, for static files > at least) which I thought would make that prefered, if we're confident > that we'll never do more than a zlib buffer worth of work the first > go-round. Good point. So your patch would invalidate a cached entity if the response to a GET delivered a C-L header, since HEAD and GET would deliver different C-L headers. OTOH I think only very small or extremely compressable responses (whether static or not) would have a C-L in the response to a GET, because everything that exceeeds a zlib buffer would be delivered chunked anyway. Regards Rüdiger
Re: mod_deflate DoS using HEAD
On Wed, Jul 15, 2009 at 5:19 PM, Nick Kew wrote: > William A. Rowe, Jr. wrote: > >> So +1 to the proposed patch; in fact, +1 on unsetting C-L and treating >> HEAD to the same processing as 304. > > +1. Since it's a SHOULD not a MUST, we can be pragmatic > with the headers. > > That's back to Eric's original patch, isn't it? For a large static file, Ruedigers patch suppresses the C-L entirely (it gets added back in down the chain for my patch, for static files at least) which I thought would make that prefered, if we're confident that we'll never do more than a zlib buffer worth of work the first go-round. -- Eric Covener cove...@gmail.com
Re: mod_deflate DoS using HEAD
William A. Rowe, Jr. wrote: So +1 to the proposed patch; in fact, +1 on unsetting C-L and treating HEAD to the same processing as 304. +1. Since it's a SHOULD not a MUST, we can be pragmatic with the headers. That's back to Eric's original patch, isn't it? -- Nick Kew
Re: mod_deflate DoS using HEAD
Joe Orton wrote: > On Wed, Jul 15, 2009 at 11:03:24AM +0200, "Plüm, Rüdiger, VF-Group" wrote: >>> I'm confused. Why do this check so late, and why does r->bytes_sent >>> matter? Why does it "screw up the protocol" if the DEFLATE >> All depends on the first brigade that passes mod_deflate. If this brigade >> contains the whole response *and* mod_deflate can compress this response >> in one go meaning calling ap_passbrigade only once to sent the fully >> compressed >> response then the content-length filter can determine the length of the >> content >> and add it to the HEAD response as the same GET request would be delivered >> with a C-L. > > I understand that, but, the whole point of doing the check early, as > Eric's proposed patch did, is to *avoid* doing all that work because it > may be exhorbitantly expensive, even in the case of a single brigade > containing the whole response - that's the issue in hand. > > The GET/304 case already omits the C-L from the response, so, I'm still > struggling to see how doing the same for HEAD somehow breaks HTTP > caching in any way. Ok, rereading 9.4, it discusses "new field values indicate that the cached entity differs from the current entity" - new; not omitted. In the absence of the header, the entity doesn't "change". It is unknown. So a cache author can/should/would[?] treat it as fresh, as you point out with respect to 304. So +1 to the proposed patch; in fact, +1 on unsetting C-L and treating HEAD to the same processing as 304.
Re: mod_deflate DoS using HEAD
On Tue, Jul 14, 2009 at 11:47 AM, "Plüm, Rüdiger, VF-Group" < ruediger.pl...@vodafone.com> wrote: > > All very true. But how about the following patch. It should do no > harm and should solve the issue in at least some cases (I think > in most cases): > > Index: modules/filters/mod_deflate.c > > +if (r->header_only && r->bytes_sent) { > +ap_remove_output_filter(f); > +return ap_pass_brigade(f->next, bb); > +} > +1 It doesn't break HTTP or caching, is simple, and any "extra" overhead (if you want to call it that) is limited to one zlib buffer as you pointed out later. Greg
RE: mod_deflate DoS using HEAD
> -Original Message- > From: Joe Orton [mailto:jor...@redhat.com] > Sent: Mittwoch, 15. Juli 2009 15:29 > To: dev@httpd.apache.org > Subject: Re: mod_deflate DoS using HEAD > > On Wed, Jul 15, 2009 at 11:03:24AM +0200, "Plüm, Rüdiger, > VF-Group" wrote: > > > I'm confused. Why do this check so late, and why does > r->bytes_sent > > > matter? Why does it "screw up the protocol" if the DEFLATE > > > > All depends on the first brigade that passes mod_deflate. > If this brigade > > contains the whole response *and* mod_deflate can compress > this response > > in one go meaning calling ap_passbrigade only once to sent > the fully compressed > > response then the content-length filter can determine the > length of the content > > and add it to the HEAD response as the same GET request > would be delivered > > with a C-L. > > I understand that, but, the whole point of doing the check early, as > Eric's proposed patch did, is to *avoid* doing all that work > because it > may be exhorbitantly expensive, even in the case of a single brigade > containing the whole response - that's the issue in hand. It isn't really this expensive, because of the second condition. Each time the internal zlib outgoing buffer with the compressed data within is full we pass this data down the chain. We don't buffer this data in the filter. It is a good filter :-). This means if this passing is not the *only* passing mod_deflate does down the chain we cannot go the C-L road anyway. So most larger responses even in one brigade if not extremely compressable will cause mod_deflate aborting compression in this case. IMHO the rule is that if the C-L of the compressed response is larger than the size of zlibs outgoing buffer we will sent the response in T-E chunked anyway. Thus we can stop compressing for a HEAD request once we got over this limit which should happen fairly early. > > The GET/304 case already omits the C-L from the response, so, > I'm still > struggling to see how doing the same for HEAD somehow breaks HTTP > caching in any way. IMHO it can cause an unnecessary invalidation of a cache entry. This is not nice. Whether this could be called a "break" is another story. Regards Rüdiger
Re: mod_deflate DoS using HEAD
On Wed, Jul 15, 2009 at 11:03:24AM +0200, "Plüm, Rüdiger, VF-Group" wrote: > > I'm confused. Why do this check so late, and why does r->bytes_sent > > matter? Why does it "screw up the protocol" if the DEFLATE > > All depends on the first brigade that passes mod_deflate. If this brigade > contains the whole response *and* mod_deflate can compress this response > in one go meaning calling ap_passbrigade only once to sent the fully > compressed > response then the content-length filter can determine the length of the > content > and add it to the HEAD response as the same GET request would be delivered > with a C-L. I understand that, but, the whole point of doing the check early, as Eric's proposed patch did, is to *avoid* doing all that work because it may be exhorbitantly expensive, even in the case of a single brigade containing the whole response - that's the issue in hand. The GET/304 case already omits the C-L from the response, so, I'm still struggling to see how doing the same for HEAD somehow breaks HTTP caching in any way. Regards, Joe
Re: mod_deflate DoS using HEAD
"William A. Rowe, Jr." writes: > Joe Orton wrote: >> >> Does 2616 mandate that a resource must always >> exactly the same set of content-codings across methods and time? >> (AFAICT there is no MUST on that front; it's a SHOULD if anything) > > Read through to the end, it breaks all proxied content; > > 9.4 HEAD > >The HEAD method is identical to GET except that the server MUST NOT >return a message-body in the response. The metainformation contained >in the HTTP headers in response to a HEAD request SHOULD be identical >to the information sent in response to a GET request. This method can >be used for obtaining metainformation about the entity implied by the >request without transferring the entity-body itself. This method is >often used for testing hypertext links for validity, accessibility, >and recent modification. > >The response to a HEAD request MAY be cacheable in the sense that the >information contained in the response MAY be used to update a >previously cached entity from that resource. If the new field values >indicate that the cached entity differs from the current entity (as >would be indicated by a change in Content-Length, Content-MD5, ETag >or Last-Modified), then the cache MUST treat the cache entry as >stale. Doesn't that last sentence just indicate that the cache entry will be invalidated? That would add some possibly unnecessary work fetching the content again the next time it's requested, but I wouldn't think it would break anything. -- Dan Poirier
RE: mod_deflate DoS using HEAD
> -Original Message- > From: Joe Orton [mailto:jor...@redhat.com] > Sent: Mittwoch, 15. Juli 2009 09:51 > To: dev@httpd.apache.org > Subject: Re: mod_deflate DoS using HEAD > > On Tue, Jul 14, 2009 at 05:47:16PM +0200, "Plüm, Rüdiger, > VF-Group" wrote: > > > > > > > -Original Message- > > > From: William A. Rowe, Jr. > > > Sent: Montag, 13. Juli 2009 23:58 > > > To: dev@httpd.apache.org > > > Subject: Re: mod_deflate DoS using HEAD > > > > > > Nick Kew wrote: > > > > Eric Covener wrote: > > > > > > > >> /* For a 304 response, only change the headers */ > > > >> -if (r->status == HTTP_NOT_MODIFIED) { > > > >> +if (r->status == HTTP_NOT_MODIFIED || > r->header_only) { > > > > > > > > Technically speaking, screws up the protocol. > > > > > > > > IMHO it would be acceptable provided: > > > > (a) it's an option for the admin, rather than enforced > > > > (b) it's documented > > > > (c) the headers are correct: either Content-Encoding is > > > > unset (uncompressed response) or Content-Length is > > > > unset. Probably the former. > > > > > > Agreed. It's not a DoS. If the admin wants to conserve CPU > > > resources, they must either; > > > > > > * cache the deflated pages (avoid user-agent header if there > > >are multiples, which reminds me we need a module to unset the > > >accept deflate trigger on non-compliant browsers running > > >very-first in the quick_handler.) > > > > > > * create gzip'ed content, navigate the choice of content through > > >multiviews. > > > > > > * do not do server-side deflation (it is expensive). > > > > > > > All very true. But how about the following patch. It should do no > > harm and should solve the issue in at least some cases (I think > > in most cases): > > I'm confused. Why do this check so late, and why does r->bytes_sent > matter? Why does it "screw up the protocol" if the DEFLATE All depends on the first brigade that passes mod_deflate. If this brigade contains the whole response *and* mod_deflate can compress this response in one go meaning calling ap_passbrigade only once to sent the fully compressed response then the content-length filter can determine the length of the content and add it to the HEAD response as the same GET request would be delivered with a C-L. If the above is not true the according GET response would be delivered with T-E chunked anyway (in the HTTP/1.1 case or without anything but closing the connection after sending the response in the HTTP/1.0 case). So there is no point in doing further compression. And r->bytes_sent != 0 indicates that we have been already in the content length filter and that it cannot measure the length of the response for creating a C-L header as I tried to explain in my comment. Well one might argue that the number of cases where the first brigade contains the whole response *and* mod_deflate can compress this response in one go meaning calling ap_passbrigade only once to sent the fully compressed response is so low that this doesn't justify this approach and that we should just get out of the way in the case of HEAD requests. Especially as providing the correct C-L in a HEAD response is only a SHOULD (see below) > filter does > nothing for a HEAD request? Because of the concern that a HEAD will > return a different C-L & C-E to a GET on the same resource > with the same > Accept-Encoding header? Does 2616 mandate that a resource > must always > exactly the same set of content-codings across methods and time? > (AFAICT there is no MUST on that front; it's a SHOULD if anything) Exactly this is my impression and it is backed by 9.4 in RFC2616. But you are correct it is only a SHOULD. Regards Rüdiger
Re: mod_deflate DoS using HEAD
Joe Orton wrote: > > I'm confused. Why do this check so late, and why does r->bytes_sent > matter? Why does it "screw up the protocol" if the DEFLATE filter does > nothing for a HEAD request? Because of the concern that a HEAD will > return a different C-L & C-E to a GET on the same resource with the same > Accept-Encoding header? Does 2616 mandate that a resource must always > exactly the same set of content-codings across methods and time? > (AFAICT there is no MUST on that front; it's a SHOULD if anything) Read through to the end, it breaks all proxied content; 9.4 HEAD The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response. The metainformation contained in the HTTP headers in response to a HEAD request SHOULD be identical to the information sent in response to a GET request. This method can be used for obtaining metainformation about the entity implied by the request without transferring the entity-body itself. This method is often used for testing hypertext links for validity, accessibility, and recent modification. The response to a HEAD request MAY be cacheable in the sense that the information contained in the response MAY be used to update a previously cached entity from that resource. If the new field values indicate that the cached entity differs from the current entity (as would be indicated by a change in Content-Length, Content-MD5, ETag or Last-Modified), then the cache MUST treat the cache entry as stale.
Re: mod_deflate DoS using HEAD
On Tue, Jul 14, 2009 at 05:47:16PM +0200, "Plüm, Rüdiger, VF-Group" wrote: > > > > -Original Message- > > From: William A. Rowe, Jr. > > Sent: Montag, 13. Juli 2009 23:58 > > To: dev@httpd.apache.org > > Subject: Re: mod_deflate DoS using HEAD > > > > Nick Kew wrote: > > > Eric Covener wrote: > > > > > >> /* For a 304 response, only change the headers */ > > >> -if (r->status == HTTP_NOT_MODIFIED) { > > >> +if (r->status == HTTP_NOT_MODIFIED || r->header_only) { > > > > > > Technically speaking, screws up the protocol. > > > > > > IMHO it would be acceptable provided: > > > (a) it's an option for the admin, rather than enforced > > > (b) it's documented > > > (c) the headers are correct: either Content-Encoding is > > > unset (uncompressed response) or Content-Length is > > > unset. Probably the former. > > > > Agreed. It's not a DoS. If the admin wants to conserve CPU > > resources, they must either; > > > > * cache the deflated pages (avoid user-agent header if there > >are multiples, which reminds me we need a module to unset the > >accept deflate trigger on non-compliant browsers running > >very-first in the quick_handler.) > > > > * create gzip'ed content, navigate the choice of content through > >multiviews. > > > > * do not do server-side deflation (it is expensive). > > > > All very true. But how about the following patch. It should do no > harm and should solve the issue in at least some cases (I think > in most cases): I'm confused. Why do this check so late, and why does r->bytes_sent matter? Why does it "screw up the protocol" if the DEFLATE filter does nothing for a HEAD request? Because of the concern that a HEAD will return a different C-L & C-E to a GET on the same resource with the same Accept-Encoding header? Does 2616 mandate that a resource must always exactly the same set of content-codings across methods and time? (AFAICT there is no MUST on that front; it's a SHOULD if anything) Regards, Joe
Re: mod_deflate DoS using HEAD
Roy T. Fielding wrote: > On Jul 14, 2009, at 10:02 AM, Nick Kew wrote: >> That, on the other hand, stands. In the case of an HTTP/1.0 >> request, we'd be closing the connection to signal end-of-response. > > Not on a HEAD request. But on the GET request with the deflate filter installed, we would not send a C-L but would close the connection upon transmission of the body, so we can drop the pretense of querying the true C-L.
Re: mod_deflate DoS using HEAD
On Jul 14, 2009, at 10:02 AM, Nick Kew wrote: That, on the other hand, stands. In the case of an HTTP/1.0 request, we'd be closing the connection to signal end-of-response. Not on a HEAD request. Roy
Re: mod_deflate DoS using HEAD
Nick Kew wrote: The content-length could've been set anyway - the simplest case being a static file that's been "stat"ed. Have we definitely unset it? D'oh. Of course we have. Is this really an optimization? Sounds like correctness :) And do we want to also validate that Accept-Encoding: chunked is present? No, deflate doesn't imply chunked encoding. That, on the other hand, stands. In the case of an HTTP/1.0 request, we'd be closing the connection to signal end-of-response. -- Nick Kew
Re: mod_deflate DoS using HEAD
William A. Rowe, Jr. wrote: Plüm, Rüdiger, VF-Group wrote: +/* + * Optimization: If we are a HEAD request and bytes_sent is not zero + * it means that we have passed the content-length filter once and + * have more data to sent. This means that the content-length filter + * could not determine our content-length for the response to the + * HEAD request anyway (the associated GET request would deliver the + * body in chunked encoding) and we can stop compressing. + */ The content-length could've been set anyway - the simplest case being a static file that's been "stat"ed. Have we definitely unset it? Is this really an optimization? Sounds like correctness :) And do we want to also validate that Accept-Encoding: chunked is present? No, deflate doesn't imply chunked encoding. +if (r->header_only && r->bytes_sent) { +ap_remove_output_filter(f); +return ap_pass_brigade(f->next, bb); +} Other than comments above - +1! Other than comments above, +0.5. The other +half is still contemplating giving control to the sysop - c.f. my previous mail on the subject. -- Nick Kew
Re: mod_deflate DoS using HEAD
Plüm, Rüdiger, VF-Group wrote: > > +/* > + * Optimization: If we are a HEAD request and bytes_sent is not zero > + * it means that we have passed the content-length filter once and > + * have more data to sent. This means that the content-length filter > + * could not determine our content-length for the response to the > + * HEAD request anyway (the associated GET request would deliver the > + * body in chunked encoding) and we can stop compressing. > + */ Is this really an optimization? Sounds like correctness :) And do we want to also validate that Accept-Encoding: chunked is present? > +if (r->header_only && r->bytes_sent) { > +ap_remove_output_filter(f); > +return ap_pass_brigade(f->next, bb); > +} Other than comments above - +1!
RE: mod_deflate DoS using HEAD
> -Original Message- > From: William A. Rowe, Jr. > Sent: Montag, 13. Juli 2009 23:58 > To: dev@httpd.apache.org > Subject: Re: mod_deflate DoS using HEAD > > Nick Kew wrote: > > Eric Covener wrote: > > > >> /* For a 304 response, only change the headers */ > >> -if (r->status == HTTP_NOT_MODIFIED) { > >> +if (r->status == HTTP_NOT_MODIFIED || r->header_only) { > > > > Technically speaking, screws up the protocol. > > > > IMHO it would be acceptable provided: > > (a) it's an option for the admin, rather than enforced > > (b) it's documented > > (c) the headers are correct: either Content-Encoding is > > unset (uncompressed response) or Content-Length is > > unset. Probably the former. > > Agreed. It's not a DoS. If the admin wants to conserve CPU > resources, they must either; > > * cache the deflated pages (avoid user-agent header if there >are multiples, which reminds me we need a module to unset the >accept deflate trigger on non-compliant browsers running >very-first in the quick_handler.) > > * create gzip'ed content, navigate the choice of content through >multiviews. > > * do not do server-side deflation (it is expensive). > All very true. But how about the following patch. It should do no harm and should solve the issue in at least some cases (I think in most cases): Index: modules/filters/mod_deflate.c === --- modules/filters/mod_deflate.c (revision 793927) +++ modules/filters/mod_deflate.c (working copy) @@ -629,6 +629,19 @@ apr_bucket *b; apr_size_t len; +/* + * Optimization: If we are a HEAD request and bytes_sent is not zero + * it means that we have passed the content-length filter once and + * have more data to sent. This means that the content-length filter + * could not determine our content-length for the response to the + * HEAD request anyway (the associated GET request would deliver the + * body in chunked encoding) and we can stop compressing. + */ +if (r->header_only && r->bytes_sent) { +ap_remove_output_filter(f); +return ap_pass_brigade(f->next, bb); +} + e = APR_BRIGADE_FIRST(bb); if (APR_BUCKET_IS_EOS(e)) { Regards Rüdiger
Re: mod_deflate DoS using HEAD
William A. Rowe, Jr. wrote: > > * do not do server-side deflation (it is expensive). Whoops - forgot one more * do not do content deflation, only transfer deflation, which should not metered by the content-length, right?
Re: mod_deflate DoS using HEAD
Nick Kew wrote: > Eric Covener wrote: > >> /* For a 304 response, only change the headers */ >> -if (r->status == HTTP_NOT_MODIFIED) { >> +if (r->status == HTTP_NOT_MODIFIED || r->header_only) { > > Technically speaking, screws up the protocol. > > IMHO it would be acceptable provided: > (a) it's an option for the admin, rather than enforced > (b) it's documented > (c) the headers are correct: either Content-Encoding is > unset (uncompressed response) or Content-Length is > unset. Probably the former. Agreed. It's not a DoS. If the admin wants to conserve CPU resources, they must either; * cache the deflated pages (avoid user-agent header if there are multiples, which reminds me we need a module to unset the accept deflate trigger on non-compliant browsers running very-first in the quick_handler.) * create gzip'ed content, navigate the choice of content through multiviews. * do not do server-side deflation (it is expensive). These two flaw reports are truly no more DoS than most CGI pages.
Re: mod_deflate DoS using HEAD
Eric Covener wrote: /* For a 304 response, only change the headers */ -if (r->status == HTTP_NOT_MODIFIED) { +if (r->status == HTTP_NOT_MODIFIED || r->header_only) { Technically speaking, screws up the protocol. IMHO it would be acceptable provided: (a) it's an option for the admin, rather than enforced (b) it's documented (c) the headers are correct: either Content-Encoding is unset (uncompressed response) or Content-Length is unset. Probably the former. -- Nick Kew
Re: mod_deflate DoS
On Sun, Jun 28, 2009 at 08:20:20PM +0200, Stefan Fritsch wrote: > we have received a bug report [1] that a DoS is possible with > mod_deflate since it does not stop to compress large files even after > the network connection has been closed. This allows to use large > amounts of CPU if there is a largish (>10 MB) file available that has > mod_deflate enabled. Thanks for posting the report. This issue has been assigned CVE-2009-1891. On the security list, Ruediger suggested these fixes, which I've proposed for inclusion in 2.2.x: http://people.apache.org/~jorton/CVE-2009-1891.1.diff http://people.apache.org/~jorton/CVE-2009-1891.2.diff along with a third fix which concerned event MPM write completion - AFAICT that is not relevant on the 2.2.x branch. Regards, Joe
mod_deflate DoS
Hi, we have received a bug report [1] that a DoS is possible with mod_deflate since it does not stop to compress large files even after the network connection has been closed. This allows to use large amounts of CPU if there is a largish (>10 MB) file available that has mod_deflate enabled. An exploit is as easy as for a in $(seq 1 100) ; do curl -H "Accept-Encoding: gzip" -is http://url.to/large/file.txt | head -1 ; done François Guerraz, the bug submitter, also suggested a patch: --- mod_deflate.c 2008-01-04 15:23:50.0 +0100 +++ mod_deflate.c.new 2009-06-26 16:50:36.0 +0200 @@ -691,6 +691,10 @@ continue; } + if (r->connection->aborted) { +return APR_ECONNABORTED; +} + /* read */ apr_bucket_read(e, &data, &len, APR_BLOCK_READ); This greatly reduces the problem. But even with it, quite a bit of data is compressed (maybe determined by APR_MMAP_LIMIT == 4MB?). Cheers, Stefan [1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=534712