Re: Byte range implementation for httpd(8)
This might be what your thinking of. https://httpd.apache.org/security/CVE-2011-3192.txt Description: A denial of service vulnerability has been found in the way the multiple overlapping ranges are handled by the Apache HTTPD server prior to version 2.2.20: http://seclists.org/fulldisclosure/2011/Aug/175 An attack tool is circulating in the wild. Active use of this tool has been observed. The attack can be done remotely and with a modest number of requests can cause very significant memory and CPU usage on the server. Ian McWilliam From: owner-t...@openbsd.org [owner-t...@openbsd.org] on behalf of Florian Obser [flor...@openbsd.org] Sent: Monday, 4 May 2015 4:34 AM To: tech@openbsd.org Cc: Sunil Nimmagadda Subject: Re: Byte range implementation for httpd(8) On Sun, May 03, 2015 at 08:14:25PM +0200, Sebastian Benoit wrote: one question though: whats the reasoning behind MAX_RANGES 4? nginx seems to have a default of unlimited (which i think questionable), but what is Wasn't there a cve about this last year or so? You can try to burn cpu and io on the server by requesting stupid ranges, like one byte at a time, backwards for the whole file or something... reasonably seen on the internet? my best guess is one range, from some byte position to the end, when you resume a transfer. -- I'm not entirely sure you are real.
Re: Byte range implementation for httpd(8)
On Sun, May 03, 2015 at 01:46:56PM +0200, Sunil Nimmagadda wrote:
On Sat, May 02, 2015 at 02:49:30PM +, Florian Obser wrote:
Sorry for the very late reply, I'm currently very busy :/
Thank you for taking time to review it. A new patch with style nits
fixed and a gratuitous NULL check removed.
[trimming some text]
this is missing the server_file_method() song and dance from
server_file_request()
Fixed in a slightly different way than using server_file_method().
Since range request should be ignored for methods other than GET,
fallback to server_file_request() for further processing/validation.
yep, this is good.
+ if ((range = parse_range(range_str, st-st_size, nranges)) == NULL) {
+ code = 416;
+ (void)snprintf(content_range, sizeof(content_range),
+ bytes */%lld, st-st_size);
+ errstr = content_range;
+ goto abort;
+ }
hm, apache answers with the full file and 200 if the range header is
syntactically incorrect or if end start. As far as I can tell it
only answers 416 if the range actually lies outside of the file.
I am confused about the RFC here, section 4.4 states that 416 is
returned if ...the set of ranges requested has been rejected due
to invalid ranges... and a note follows immediately, Because
servers are free to ignore Range, many implementations will simply
respond with the entire selected representation in a 200 response
As you noted apache returns 200 while nginx returns 416 for an
incorrect range. What should httpd do ideally?
Thanks for checking nginx, I don't have one around.
I noted this for 2 reasons:
1) I guess apache is the defacto standard, so if it behaves
differently there might be a reason, maybe broken clients in the wild.
2) The way the code is currently structured it will require a bit of
shuffling around if we ever want apache's behaviour.
That being said, my understanding of the RFC is that your
implementation is correct. And with nginx behaving this way, too, it
shouldn't bite us in the future. :)
+
+ /* Accept byte ranges */
+ if (code == 200
+ kv_add(resp-http_headers, Accept-Ranges, bytes) == NULL)
return (-1);
I don't think we should advertise ranges for all 200 results, for
example we don't support it for directory indexes.
Agree, since RFC says server MAY send Accept-Range header and
clients MAY generate range requests without having received this
header field, dropping this header shouldn't make a difference.
Comments?
This looks good.
OK florian@ if someone wants to commit it. Or give me an OK and I'll
commit.
Index: server_file.c
===
RCS file: /cvs/src/usr.sbin/httpd/server_file.c,v
retrieving revision 1.52
diff -u -p -r1.52 server_file.c
--- server_file.c 25 Apr 2015 14:40:35 - 1.52
+++ server_file.c 3 May 2015 11:18:07 -
@@ -36,12 +36,25 @@
#define MINIMUM(a, b)(((a) (b)) ? (a) : (b))
#define MAXIMUM(a, b)(((a) (b)) ? (a) : (b))
+#define MAX_RANGES 4
-int server_file_access(struct httpd *, struct client *, char *, size_t);
-int server_file_request(struct httpd *, struct client *, char *,
- struct stat *);
-int server_file_index(struct httpd *, struct client *, struct stat *);
-int server_file_method(struct client *);
+struct range {
+ off_t start;
+ off_t end;
+};
+
+int server_file_access(struct httpd *, struct client *,
+ char *, size_t);
+int server_file_request(struct httpd *, struct client *,
+ char *, struct stat *);
+int server_partial_file_request(struct httpd *, struct client *,
+ char *, struct stat *, char *);
+int server_file_index(struct httpd *, struct client *,
+ struct stat *);
+int server_file_method(struct client *);
+int parse_range_spec(char *, size_t, struct range *);
+struct range *parse_range(char *, size_t, int *);
+int buffer_add_range(int, struct evbuffer *, struct range *);
int
server_file_access(struct httpd *env, struct client *clt,
@@ -50,6 +63,7 @@ server_file_access(struct httpd *env, st
struct http_descriptor *desc = clt-clt_descreq;
struct server_config*srv_conf = clt-clt_srv_conf;
struct stat st;
+ struct kv *r, key;
char*newpath;
int ret;
@@ -123,7 +137,13 @@ server_file_access(struct httpd *env, st
goto fail;
}
- return (server_file_request(env, clt, path, st));
+ key.kv_key = Range;
+ r = kv_find(desc-http_headers, key);
+ if (r != NULL)
+ return (server_partial_file_request(env, clt, path, st,
+ r-kv_value));
+ else
+ return
Re: Byte range implementation for httpd(8)
On Sat, May 02, 2015 at 02:49:30PM +, Florian Obser wrote:
Sorry for the very late reply, I'm currently very busy :/
Thank you for taking time to review it. A new patch with style nits
fixed and a gratuitous NULL check removed.
[trimming some text]
this is missing the server_file_method() song and dance from
server_file_request()
Fixed in a slightly different way than using server_file_method().
Since range request should be ignored for methods other than GET,
fallback to server_file_request() for further processing/validation.
+ if ((range = parse_range(range_str, st-st_size, nranges)) == NULL) {
+ code = 416;
+ (void)snprintf(content_range, sizeof(content_range),
+ bytes */%lld, st-st_size);
+ errstr = content_range;
+ goto abort;
+ }
hm, apache answers with the full file and 200 if the range header is
syntactically incorrect or if end start. As far as I can tell it
only answers 416 if the range actually lies outside of the file.
I am confused about the RFC here, section 4.4 states that 416 is
returned if ...the set of ranges requested has been rejected due
to invalid ranges... and a note follows immediately, Because
servers are free to ignore Range, many implementations will simply
respond with the entire selected representation in a 200 response
As you noted apache returns 200 while nginx returns 416 for an
incorrect range. What should httpd do ideally?
+
+ /* Accept byte ranges */
+ if (code == 200
+ kv_add(resp-http_headers, Accept-Ranges, bytes) == NULL)
return (-1);
I don't think we should advertise ranges for all 200 results, for
example we don't support it for directory indexes.
Agree, since RFC says server MAY send Accept-Range header and
clients MAY generate range requests without having received this
header field, dropping this header shouldn't make a difference.
Comments?
Index: server_file.c
===
RCS file: /cvs/src/usr.sbin/httpd/server_file.c,v
retrieving revision 1.52
diff -u -p -r1.52 server_file.c
--- server_file.c 25 Apr 2015 14:40:35 - 1.52
+++ server_file.c 3 May 2015 11:18:07 -
@@ -36,12 +36,25 @@
#define MINIMUM(a, b) (((a) (b)) ? (a) : (b))
#define MAXIMUM(a, b) (((a) (b)) ? (a) : (b))
+#define MAX_RANGES 4
-int server_file_access(struct httpd *, struct client *, char *, size_t);
-int server_file_request(struct httpd *, struct client *, char *,
- struct stat *);
-int server_file_index(struct httpd *, struct client *, struct stat *);
-int server_file_method(struct client *);
+struct range {
+ off_t start;
+ off_t end;
+};
+
+int server_file_access(struct httpd *, struct client *,
+ char *, size_t);
+int server_file_request(struct httpd *, struct client *,
+ char *, struct stat *);
+int server_partial_file_request(struct httpd *, struct client *,
+ char *, struct stat *, char *);
+int server_file_index(struct httpd *, struct client *,
+ struct stat *);
+int server_file_method(struct client *);
+int parse_range_spec(char *, size_t, struct range *);
+struct range *parse_range(char *, size_t, int *);
+int buffer_add_range(int, struct evbuffer *, struct range *);
int
server_file_access(struct httpd *env, struct client *clt,
@@ -50,6 +63,7 @@ server_file_access(struct httpd *env, st
struct http_descriptor *desc = clt-clt_descreq;
struct server_config*srv_conf = clt-clt_srv_conf;
struct stat st;
+ struct kv *r, key;
char*newpath;
int ret;
@@ -123,7 +137,13 @@ server_file_access(struct httpd *env, st
goto fail;
}
- return (server_file_request(env, clt, path, st));
+ key.kv_key = Range;
+ r = kv_find(desc-http_headers, key);
+ if (r != NULL)
+ return (server_partial_file_request(env, clt, path, st,
+ r-kv_value));
+ else
+ return (server_file_request(env, clt, path, st));
fail:
switch (errno) {
@@ -262,6 +282,143 @@ server_file_request(struct httpd *env, s
}
int
+server_partial_file_request(struct httpd *env, struct client *clt, char *path,
+struct stat *st, char *range_str)
+{
+ struct http_descriptor *resp = clt-clt_descresp;
+ struct http_descriptor *desc = clt-clt_descreq;
+ struct media_type *media, multipart_media;
+ struct range*range;
+ struct evbuffer *evb = NULL;
+ size_t content_length;
+ int code = 500, fd = -1, i, nranges, ret;
+ uint32_t boundary;
+ char
Re: Byte range implementation for httpd(8)
Florian Obser(flor...@openbsd.org) on 2015.05.03 12:39:02 +:
On Sun, May 03, 2015 at 01:46:56PM +0200, Sunil Nimmagadda wrote:
On Sat, May 02, 2015 at 02:49:30PM +, Florian Obser wrote:
Sorry for the very late reply, I'm currently very busy :/
Thank you for taking time to review it. A new patch with style nits
fixed and a gratuitous NULL check removed.
[trimming some text]
this is missing the server_file_method() song and dance from
server_file_request()
Fixed in a slightly different way than using server_file_method().
Since range request should be ignored for methods other than GET,
fallback to server_file_request() for further processing/validation.
yep, this is good.
+ if ((range = parse_range(range_str, st-st_size, nranges)) ==
NULL) {
+ code = 416;
+ (void)snprintf(content_range, sizeof(content_range),
+ bytes */%lld, st-st_size);
+ errstr = content_range;
+ goto abort;
+ }
hm, apache answers with the full file and 200 if the range header is
syntactically incorrect or if end start. As far as I can tell it
only answers 416 if the range actually lies outside of the file.
I am confused about the RFC here, section 4.4 states that 416 is
returned if ...the set of ranges requested has been rejected due
to invalid ranges... and a note follows immediately, Because
servers are free to ignore Range, many implementations will simply
respond with the entire selected representation in a 200 response
As you noted apache returns 200 while nginx returns 416 for an
incorrect range. What should httpd do ideally?
Thanks for checking nginx, I don't have one around.
I noted this for 2 reasons:
1) I guess apache is the defacto standard, so if it behaves
differently there might be a reason, maybe broken clients in the wild.
2) The way the code is currently structured it will require a bit of
shuffling around if we ever want apache's behaviour.
That being said, my understanding of the RFC is that your
implementation is correct. And with nginx behaving this way, too, it
shouldn't bite us in the future. :)
+
+ /* Accept byte ranges */
+ if (code == 200
+ kv_add(resp-http_headers, Accept-Ranges, bytes) ==
NULL)
return (-1);
I don't think we should advertise ranges for all 200 results, for
example we don't support it for directory indexes.
Agree, since RFC says server MAY send Accept-Range header and
clients MAY generate range requests without having received this
header field, dropping this header shouldn't make a difference.
Comments?
This looks good.
OK florian@ if someone wants to commit it. Or give me an OK and I'll
commit.
fwiw i dont know much about ranges, but this reads ok. two whitespace bits
below. otherwise ok benno.
one question though: whats the reasoning behind MAX_RANGES 4? nginx seems to
have a default of unlimited (which i think questionable), but what is
reasonably seen on the internet?
Index: server_file.c
===
RCS file: /cvs/src/usr.sbin/httpd/server_file.c,v
retrieving revision 1.52
diff -u -p -r1.52 server_file.c
--- server_file.c 25 Apr 2015 14:40:35 - 1.52
+++ server_file.c 3 May 2015 11:18:07 -
@@ -36,12 +36,25 @@
#define MINIMUM(a, b) (((a) (b)) ? (a) : (b))
#define MAXIMUM(a, b) (((a) (b)) ? (a) : (b))
+#define MAX_RANGES 4
-int server_file_access(struct httpd *, struct client *, char *,
size_t);
-int server_file_request(struct httpd *, struct client *, char *,
- struct stat *);
-int server_file_index(struct httpd *, struct client *, struct stat
*);
-int server_file_method(struct client *);
+struct range {
+ off_t start;
+ off_t end;
+};
+
+int server_file_access(struct httpd *, struct client *,
+ char *, size_t);
+int server_file_request(struct httpd *, struct client *,
+ char *, struct stat *);
+int server_partial_file_request(struct httpd *, struct
client *,
+ char *, struct stat *, char *);
+int server_file_index(struct httpd *, struct client *,
+ struct stat *);
+int server_file_method(struct client *);
+int parse_range_spec(char *, size_t, struct range *);
+struct range *parse_range(char *, size_t, int *);
+int buffer_add_range(int, struct evbuffer *, struct range
*);
int
server_file_access(struct httpd *env, struct client *clt,
@@ -50,6 +63,7 @@ server_file_access(struct httpd *env, st
struct http_descriptor *desc = clt-clt_descreq;
struct server_config
Re: Byte range implementation for httpd(8)
On Sun, May 03, 2015 at 08:14:25PM +0200, Sebastian Benoit wrote: one question though: whats the reasoning behind MAX_RANGES 4? nginx seems to have a default of unlimited (which i think questionable), but what is Wasn't there a cve about this last year or so? You can try to burn cpu and io on the server by requesting stupid ranges, like one byte at a time, backwards for the whole file or something... reasonably seen on the internet? my best guess is one range, from some byte position to the end, when you resume a transfer. -- I'm not entirely sure you are real.
Re: Byte range implementation for httpd(8)
Sorry for the very late reply, I'm currently very busy :/
On Fri, Apr 17, 2015 at 05:04:01AM +0200, Sunil Nimmagadda wrote:
Range requests as defined in RFC7233 is required for resuming
interrupted http(s) downloads for example:
ftp -C http://foo.bar/install57.iso
With this diff, httpd parses Range header in the requests and
provide either 206(Partial Content) or 416(Range not Satisfiable)
responses with Content-Range header set appropriately. Further,
it understands multi range request and generate satisfiable payloads
with multipart/byteranges media type.
Suggestions/comments to improve the diff are welcome.
Note, If-Range isn't implemented yet.
Index: server_file.c
===
RCS file: /cvs/src/usr.sbin/httpd/server_file.c,v
retrieving revision 1.51
diff -u -p -r1.51 server_file.c
--- server_file.c 12 Feb 2015 10:05:29 - 1.51
+++ server_file.c 17 Apr 2015 02:22:12 -
@@ -36,12 +36,23 @@
#define MINIMUM(a, b)(((a) (b)) ? (a) : (b))
#define MAXIMUM(a, b)(((a) (b)) ? (a) : (b))
+#define MAX_RANGES 4
+
+struct range {
+ off_t start;
+ off_t end;
+};
int server_file_access(struct httpd *, struct client *, char *, size_t);
int server_file_request(struct httpd *, struct client *, char *,
struct stat *);
+int server_partial_file_request(struct httpd *, struct client *, char *,
+ struct stat *, char *);
int server_file_index(struct httpd *, struct client *, struct stat *);
int server_file_method(struct client *);
+int parse_range_spec(char *, size_t, struct range *);
+struct range *parse_range(char *, size_t, int *);
+int buffer_add_range(int, struct evbuffer *, struct range *);
This whole block now needs another tab indentation after the type
(except for parse_range of course)
int
server_file_access(struct httpd *env, struct client *clt,
@@ -50,6 +61,7 @@ server_file_access(struct httpd *env, st
struct http_descriptor *desc = clt-clt_descreq;
struct server_config*srv_conf = clt-clt_srv_conf;
struct stat st;
+ struct kv *r, key;
char*newpath;
int ret;
@@ -123,7 +135,13 @@ server_file_access(struct httpd *env, st
goto fail;
}
- return (server_file_request(env, clt, path, st));
+ key.kv_key = Range;
+ r = kv_find(desc-http_headers, key);
+ if (r != NULL)
+ return (server_partial_file_request(env, clt, path, st,
+ r-kv_value));
+ else
+ return (server_file_request(env, clt, path, st));
fail:
switch (errno) {
@@ -262,6 +280,138 @@ server_file_request(struct httpd *env, s
}
int
+server_partial_file_request(struct httpd *env, struct client *clt, char
*path,
+struct stat *st, char *range_str)
+{
+ struct http_descriptor *resp = clt-clt_descresp;
+ struct media_type *media, multipart_media;
+ struct range*range;
+ struct evbuffer *evb = NULL;
+ size_t content_length;
+ int code = 500, fd = -1, i, nranges, ret;
+ char content_range[64];
+ const char *errstr = NULL;
+ uint32_t boundary;
Nit: uint32_t should be below int
+
this is missing the server_file_method() song and dance from
server_file_request()
+ if ((range = parse_range(range_str, st-st_size, nranges)) == NULL) {
+ code = 416;
+ (void)snprintf(content_range, sizeof(content_range),
+ bytes */%lld, st-st_size);
+ errstr = content_range;
+ goto abort;
+ }
hm, apache answers with the full file and 200 if the range header is
syntactically incorrect or if end start. As far as I can tell it
only answers 416 if the range actually lies outside of the file.
+
+ /* Now open the file, should be readable or we have another problem */
+ if ((fd = open(path, O_RDONLY)) == -1)
+ goto abort;
+
+ media = media_find(env-sc_mediatypes, path);
+ if ((evb = evbuffer_new()) == NULL) {
+ errstr = failed to allocate file buffer;
+ goto abort;
+ }
+
+ if (nranges == 1) {
+ (void)snprintf(content_range, sizeof(content_range),
+ bytes %lld-%lld/%lld, range-start, range-end,
+ st-st_size);
+ if (kv_add(resp-http_headers, Content-Range,
+ content_range) == NULL)
+ goto abort;
+
+ content_length = range-end - range-start + 1;
+ if (buffer_add_range(fd, evb, range) == 0)
+ goto abort;
+
+ } else {
+ content_length = 0;
+ boundary = arc4random();
+ /*
Re: Byte range implementation for httpd(8)
Any interest/comments/suggestions for this diff...
On Fri, Apr 17, 2015 at 05:04:01AM +0200, Sunil Nimmagadda wrote:
Range requests as defined in RFC7233 is required for resuming
interrupted http(s) downloads for example:
ftp -C http://foo.bar/install57.iso
With this diff, httpd parses Range header in the requests and
provide either 206(Partial Content) or 416(Range not Satisfiable)
responses with Content-Range header set appropriately. Further,
it understands multi range request and generate satisfiable payloads
with multipart/byteranges media type.
Suggestions/comments to improve the diff are welcome.
Note, If-Range isn't implemented yet.
Index: server_file.c
===
RCS file: /cvs/src/usr.sbin/httpd/server_file.c,v
retrieving revision 1.51
diff -u -p -r1.51 server_file.c
--- server_file.c 12 Feb 2015 10:05:29 - 1.51
+++ server_file.c 17 Apr 2015 02:22:12 -
@@ -36,12 +36,23 @@
#define MINIMUM(a, b)(((a) (b)) ? (a) : (b))
#define MAXIMUM(a, b)(((a) (b)) ? (a) : (b))
+#define MAX_RANGES 4
+
+struct range {
+ off_t start;
+ off_t end;
+};
int server_file_access(struct httpd *, struct client *, char *, size_t);
int server_file_request(struct httpd *, struct client *, char *,
struct stat *);
+int server_partial_file_request(struct httpd *, struct client *, char *,
+ struct stat *, char *);
int server_file_index(struct httpd *, struct client *, struct stat *);
int server_file_method(struct client *);
+int parse_range_spec(char *, size_t, struct range *);
+struct range *parse_range(char *, size_t, int *);
+int buffer_add_range(int, struct evbuffer *, struct range *);
int
server_file_access(struct httpd *env, struct client *clt,
@@ -50,6 +61,7 @@ server_file_access(struct httpd *env, st
struct http_descriptor *desc = clt-clt_descreq;
struct server_config*srv_conf = clt-clt_srv_conf;
struct stat st;
+ struct kv *r, key;
char*newpath;
int ret;
@@ -123,7 +135,13 @@ server_file_access(struct httpd *env, st
goto fail;
}
- return (server_file_request(env, clt, path, st));
+ key.kv_key = Range;
+ r = kv_find(desc-http_headers, key);
+ if (r != NULL)
+ return (server_partial_file_request(env, clt, path, st,
+ r-kv_value));
+ else
+ return (server_file_request(env, clt, path, st));
fail:
switch (errno) {
@@ -262,6 +280,138 @@ server_file_request(struct httpd *env, s
}
int
+server_partial_file_request(struct httpd *env, struct client *clt, char
*path,
+struct stat *st, char *range_str)
+{
+ struct http_descriptor *resp = clt-clt_descresp;
+ struct media_type *media, multipart_media;
+ struct range*range;
+ struct evbuffer *evb = NULL;
+ size_t content_length;
+ int code = 500, fd = -1, i, nranges, ret;
+ char content_range[64];
+ const char *errstr = NULL;
+ uint32_t boundary;
+
+ if ((range = parse_range(range_str, st-st_size, nranges)) == NULL) {
+ code = 416;
+ (void)snprintf(content_range, sizeof(content_range),
+ bytes */%lld, st-st_size);
+ errstr = content_range;
+ goto abort;
+ }
+
+ /* Now open the file, should be readable or we have another problem */
+ if ((fd = open(path, O_RDONLY)) == -1)
+ goto abort;
+
+ media = media_find(env-sc_mediatypes, path);
+ if ((evb = evbuffer_new()) == NULL) {
+ errstr = failed to allocate file buffer;
+ goto abort;
+ }
+
+ if (nranges == 1) {
+ (void)snprintf(content_range, sizeof(content_range),
+ bytes %lld-%lld/%lld, range-start, range-end,
+ st-st_size);
+ if (kv_add(resp-http_headers, Content-Range,
+ content_range) == NULL)
+ goto abort;
+
+ content_length = range-end - range-start + 1;
+ if (buffer_add_range(fd, evb, range) == 0)
+ goto abort;
+
+ } else {
+ content_length = 0;
+ boundary = arc4random();
+ /* Generate a multipart payload of byteranges */
+ while (nranges--) {
+ if ((i = evbuffer_add_printf(evb, \r\n--%ud\r\n,
+ boundary)) == -1)
+ goto abort;
+
+ content_length += i;
+ if ((i = evbuffer_add_printf(evb,
+ Content-Type: %s/%s\r\n,
+ media ==
