Return-Path: X-Original-To: apmail-httpd-dev-archive@www.apache.org Delivered-To: apmail-httpd-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id B3B987FDA for ; Tue, 23 Aug 2011 22:28:34 +0000 (UTC) Received: (qmail 86656 invoked by uid 500); 23 Aug 2011 22:28:33 -0000 Delivered-To: apmail-httpd-dev-archive@httpd.apache.org Received: (qmail 86540 invoked by uid 500); 23 Aug 2011 22:28:32 -0000 Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm Precedence: bulk Reply-To: dev@httpd.apache.org list-help: list-unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@httpd.apache.org Received: (qmail 86532 invoked by uid 99); 23 Aug 2011 22:28:32 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 23 Aug 2011 22:28:32 +0000 X-ASF-Spam-Status: No, hits=-2.3 required=5.0 tests=RCVD_IN_DNSWL_MED,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (athena.apache.org: local policy) Received: from [188.40.99.202] (HELO eru.sfritsch.de) (188.40.99.202) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 23 Aug 2011 22:28:26 +0000 Received: from [10.1.1.6] (helo=k.localnet) by eru.sfritsch.de with esmtp (Exim 4.72) (envelope-from ) id 1QvzS0-00084O-7D for dev@httpd.apache.org; Wed, 24 Aug 2011 00:28:04 +0200 From: Stefan Fritsch To: dev@httpd.apache.org Subject: Re: DoS with mod_deflate & range requests Date: Wed, 24 Aug 2011 00:28:03 +0200 User-Agent: KMail/1.13.7 (Linux/3.0.0-1-amd64; KDE/4.6.5; x86_64; ; ) References: <4E541CE5.1080608@rowe-clan.net> In-Reply-To: <4E541CE5.1080608@rowe-clan.net> MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_zlCVOa3Flpq6HDF" Message-Id: <201108240028.03308.sf@sfritsch.de> --Boundary-00=_zlCVOa3Flpq6HDF Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit On Tuesday 23 August 2011, William A. Rowe Jr. wrote: > On 8/23/2011 4:00 PM, Greg Ames wrote: > > On Tue, Aug 23, 2011 at 3:32 PM, William A. Rowe Jr. wrote: > > I suggest we should be parsing and reassembling the list > > before we start the bucket logic. > > > > I propose we satisfy range requests in the only sensible > > manner, returning the ranges in sequence, > > > > yeah, overlapping ranges should be merged up front. That ought to > > completely fix the issue. > > So the only remaining question; are we free to reorder them into > sequence? Good point. I haven't seen anything in the RFC about that. I guess that there are at least some clients that will be broken by that. Nevertheless, I have done a first try at a patch. The necessary modification to only merge and not reorder should be small. I have done only limited testing, so there are probably some bugs. There are no tests with multiple ranges in the test-framework, yet. > Even in the most pedantic case, I believe that the total array > shouldn't ever exceed 1024, because in those cases a large number > of the acceptable expected ranges should be in the nnn-nnn, > format, or 8 characters long, out of our MAX_LINE_LENGTH of some > 8190. If we argue that asking for single bytes is simply wrong, > we should probably allocate some 16 ranges and grow the list by a > power of four, resulting in a max of some 4 allocs and maximum > memory consumption of less than 64k per request. Just counting the commas in the header line seems acceptable to me. In any case, single byte ranges are explicitly mentioned in the RFC as example, so we probably should not disallow those. --Boundary-00=_zlCVOa3Flpq6HDF Content-Type: text/x-patch; charset="ISO-8859-1"; name="merge_ranges.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="merge_ranges.diff" diff --git a/modules/http/byterange_filter.c b/modules/http/byterange_filter.c index 13bf0a1..6a40420 100644 --- a/modules/http/byterange_filter.c +++ b/modules/http/byterange_filter.c @@ -61,6 +61,11 @@ APLOG_USE_MODULE(http); +/* returns + * 0 if range invalid or whole file + * +1 for valid range spec + * -1 for start > end + */ static int parse_byterange(char *range, apr_off_t clength, apr_off_t *start, apr_off_t *end) { @@ -114,13 +119,6 @@ static int parse_byterange(char *range, apr_off_t clength, static int ap_set_byterange(request_rec *r); -typedef struct byterange_ctx { - apr_bucket_brigade *bb; - int num_ranges; - char *boundary; - char *bound_head; -} byterange_ctx; - /* * Here we try to be compatible with clients that want multipart/x-byteranges * instead of multipart/byteranges (also see above), as per HTTP/1.1. We @@ -136,26 +134,43 @@ static int use_range_x(request_rec *r) && ap_strstr_c(ua, "MSIE 3"))); } +struct range_spec { + apr_off_t start, end; +}; + +static int sort_fn(const void *a_, const void *b_) +{ + const struct range_spec *a = a_, *b = b_; + if (a->start < b->start) + return -1; + else if (a->start == b->start) + return 0; + else + return 1; +} + #define BYTERANGE_FMT "%" APR_OFF_T_FMT "-%" APR_OFF_T_FMT "/%" APR_OFF_T_FMT #define PARTITION_ERR_FMT "apr_brigade_partition() failed " \ "[%" APR_OFF_T_FMT ",%" APR_OFF_T_FMT "]" +#define MAX(o1, o2) ((o1 > o2) ? o1 : o2) AP_CORE_DECLARE_NONSTD(apr_status_t) ap_byterange_filter(ap_filter_t *f, apr_bucket_brigade *bb) { -#define MIN_LENGTH(len1, len2) ((len1 > len2) ? len2 : len1) request_rec *r = f->r; conn_rec *c = r->connection; - byterange_ctx *ctx; apr_bucket *e; apr_bucket_brigade *bsend; - apr_off_t range_start; - apr_off_t range_end; + apr_off_t max = 0; char *current; apr_off_t clength = 0; apr_status_t rv; - int found = 0; + int found = 0, i = 0; + int need_sort = 0; int num_ranges; + char *boundary = NULL; + char *bound_head = NULL; + struct range_spec *ranges; /* Iterate through the brigade until reaching EOS or a bucket with * unknown length. */ @@ -183,64 +198,78 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_byterange_filter(ap_filter_t *f, return ap_pass_brigade(f->next, bb); } - ctx = apr_pcalloc(r->pool, sizeof(*ctx)); - ctx->num_ranges = num_ranges; - /* create a brigade in case we never call ap_save_brigade() */ - ctx->bb = apr_brigade_create(r->pool, c->bucket_alloc); - - if (ctx->num_ranges > 1) { + if (num_ranges > 1) { /* Is ap_make_content_type required here? */ const char *orig_ct = ap_make_content_type(r, r->content_type); - ctx->boundary = apr_psprintf(r->pool, "%" APR_UINT64_T_HEX_FMT "%lx", - (apr_uint64_t)r->request_time, (long) getpid()); + boundary = apr_psprintf(r->pool, "%" APR_UINT64_T_HEX_FMT "%lx", + (apr_uint64_t)r->request_time, (long) getpid()); ap_set_content_type(r, apr_pstrcat(r->pool, "multipart", use_range_x(r) ? "/x-" : "/", "byteranges; boundary=", - ctx->boundary, NULL)); + boundary, NULL)); if (orig_ct) { - ctx->bound_head = apr_pstrcat(r->pool, - CRLF "--", ctx->boundary, - CRLF "Content-type: ", - orig_ct, - CRLF "Content-range: bytes ", - NULL); + bound_head = apr_pstrcat(r->pool, CRLF "--", boundary, + CRLF "Content-type: ", orig_ct, + CRLF "Content-range: bytes ", NULL); } else { /* if we have no type for the content, do our best */ - ctx->bound_head = apr_pstrcat(r->pool, - CRLF "--", ctx->boundary, - CRLF "Content-range: bytes ", - NULL); + bound_head = apr_pstrcat(r->pool, CRLF "--", boundary, + CRLF "Content-range: bytes ", NULL); } - ap_xlate_proto_to_ascii(ctx->bound_head, strlen(ctx->bound_head)); + ap_xlate_proto_to_ascii(bound_head, strlen(bound_head)); } + ranges = apr_palloc(r->pool, sizeof(*ranges) * num_ranges); /* this brigade holds what we will be sending */ bsend = apr_brigade_create(r->pool, c->bucket_alloc); - while ((current = ap_getword(r->pool, &r->range, ',')) - && (rv = parse_byterange(current, clength, &range_start, - &range_end))) { - apr_bucket *e2; - apr_bucket *ec; - - if (rv == -1) { + while ((current = ap_getword(r->pool, &r->range, ',')) && *current) { + AP_DEBUG_ASSERT(i < num_ranges); + rv = parse_byterange(current, clength, &ranges[i].start, + &ranges[i].end); + if (rv == -1) continue; + else if (rv == 0) + break; + + if (i == 0 || ranges[i].start > max) + max = ranges[i].end; + else + need_sort = 1; + + i++; + } + num_ranges = i; + if (need_sort) { + int j = 0; + qsort(ranges, num_ranges, sizeof(*ranges), sort_fn); + for (i = 1; i < num_ranges; i++) { + if (ranges[j].end + 1 >= ranges[i].start) + ranges[j].end = MAX(ranges[i].end, ranges[j].end); + else + j++; } + num_ranges = j + 1; + } + + for (i = 0; i < num_ranges; i++) { + apr_bucket *e2; + apr_bucket *ec; /* These calls to apr_brigage_partition should only fail in * pathological cases, e.g. a file being truncated whilst * being served. */ - if ((rv = apr_brigade_partition(bb, range_start, &ec)) != APR_SUCCESS) { + if ((rv = apr_brigade_partition(bb, ranges[i].start, &ec)) != APR_SUCCESS) { ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, - PARTITION_ERR_FMT, range_start, clength); + PARTITION_ERR_FMT, ranges[i].start, clength); continue; } - if ((rv = apr_brigade_partition(bb, range_end+1, &e2)) != APR_SUCCESS) { + if ((rv = apr_brigade_partition(bb, ranges[i].end+1, &e2)) != APR_SUCCESS) { ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, - PARTITION_ERR_FMT, range_end+1, clength); + PARTITION_ERR_FMT, ranges[i].end+1, clength); continue; } @@ -249,20 +278,20 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_byterange_filter(ap_filter_t *f, /* For single range requests, we must produce Content-Range header. * Otherwise, we need to produce the multipart boundaries. */ - if (ctx->num_ranges == 1) { + if (!boundary) { apr_table_setn(r->headers_out, "Content-Range", apr_psprintf(r->pool, "bytes " BYTERANGE_FMT, - range_start, range_end, clength)); + ranges[i].start, ranges[i].end, clength)); } else { char *ts; - e = apr_bucket_pool_create(ctx->bound_head, strlen(ctx->bound_head), + e = apr_bucket_pool_create(bound_head, strlen(bound_head), r->pool, c->bucket_alloc); APR_BRIGADE_INSERT_TAIL(bsend, e); ts = apr_psprintf(r->pool, BYTERANGE_FMT CRLF CRLF, - range_start, range_end, clength); + ranges[i].start, ranges[i].end, clength); ap_xlate_proto_to_ascii(ts, strlen(ts)); e = apr_bucket_pool_create(ts, strlen(ts), r->pool, c->bucket_alloc); @@ -300,11 +329,11 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_byterange_filter(ap_filter_t *f, return ap_pass_brigade(f->next, bsend); } - if (ctx->num_ranges > 1) { + if (boundary) { char *end; /* add the final boundary */ - end = apr_pstrcat(r->pool, CRLF "--", ctx->boundary, "--" CRLF, NULL); + end = apr_pstrcat(r->pool, CRLF "--", boundary, "--" CRLF, NULL); ap_xlate_proto_to_ascii(end, strlen(end)); e = apr_bucket_pool_create(end, strlen(end), r->pool, c->bucket_alloc); APR_BRIGADE_INSERT_TAIL(bsend, e); @@ -379,17 +408,15 @@ static int ap_set_byterange(request_rec *r) } } - if (!ap_strchr_c(range, ',')) { - /* a single range */ - num_ranges = 1; - } - else { - /* a multiple range */ - num_ranges = 2; - } - r->status = HTTP_PARTIAL_CONTENT; r->range = range + 6; + num_ranges = 1; + range = ap_strchr_c(r->range, ','); + + while (range) { + num_ranges++; + range = ap_strchr_c(range + 1, ','); + } return num_ranges; } --Boundary-00=_zlCVOa3Flpq6HDF--