Re: mod_deflate DoS using HEAD

2010-06-26 Thread Stefan Fritsch
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

2010-06-22 Thread Ruediger Pluem


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

2010-06-22 Thread Eric Covener
On Tue, Jun 22, 2010 at 2:37 AM, Ruediger Pluem rpl...@apache.org 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

2010-06-22 Thread Plüm, Rüdiger, VF-Group
 

 -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 
 rpl...@apache.org 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

2010-06-21 Thread Stefan Fritsch
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

2010-06-21 Thread William A. Rowe Jr.
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

2009-07-15 Thread Joe Orton
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

2009-07-15 Thread William A. Rowe, Jr.
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

2009-07-15 Thread Plüm, Rüdiger, VF-Group
 

 -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

2009-07-15 Thread Dan Poirier
William A. Rowe, Jr. wr...@rowe-clan.net 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 poir...@pobox.com



Re: mod_deflate DoS using HEAD

2009-07-15 Thread Joe Orton
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

2009-07-15 Thread Plüm, Rüdiger, VF-Group
 

 -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

2009-07-15 Thread Greg Ames
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

2009-07-15 Thread William A. Rowe, Jr.
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

2009-07-15 Thread Nick Kew

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

2009-07-14 Thread Plüm, Rüdiger, VF-Group
 

 -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

2009-07-14 Thread William A. Rowe, Jr.
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

2009-07-14 Thread Nick Kew

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 stated.  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

2009-07-14 Thread Nick Kew

Nick Kew wrote:


The content-length could've been set anyway - the simplest case being
a static file that's been stated.  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

2009-07-14 Thread Roy T. Fielding

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

2009-07-14 Thread William A. Rowe, Jr.
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

2009-07-13 Thread Nick Kew

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 using HEAD

2009-07-13 Thread William A. Rowe, Jr.
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

2009-07-13 Thread William A. Rowe, Jr.
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

2009-07-03 Thread Joe Orton
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

2009-06-28 Thread Stefan Fritsch
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