Re: [Libevent-users] Patch: add constraints on HTTP first line/headers/body size

To: libevent-users@xxxxxxxxxxxxx

Subject: Re: [Libevent-users] Patch: add constraints on HTTP first line/headers/body size

From: Constantine Verutin <cverutin@xxxxxxxxx>

Date: Thu, 15 Oct 2009 22:27:21 +0300

Delivered-to: archiver@xxxxxxxx

Delivered-to: libevent-users-outgoing@xxxxxxxx

Delivered-to: libevent-users@xxxxxxxx

Delivery-date: Thu, 15 Oct 2009 15:27:24 -0400

Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:content-type; bh=d7KgZAMGCNSCoDfSSpGdBjDt4EioctFS357JT3IWWIo=; b=eKmJDH6Ikms1wOOiOXi1bTl4+rr1MIjf42Bl76rMQMJfNVYJqCgVVCCsbbB9vNuVMu fgkDg8td7Np9TUNtm9eHShBBzdSdCMuGeGBQF6+G835+rKzqwW44e1SaWxuxDmnMbm4I pJleqUNokSUQufYnEpSZSCJZkFaKT7FPlkT/k=

Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; b=NuPpl/nvLSSlsuZjNTn0IdN4231Q0MziB2NHwPVQaf0G4kjYt5F+o/pZQAsce0X96x lzUnCpng19z/o/LkgZREVsl4r7pwDUPYD4y1gu/kCmSDy5NXrCkBpzl5kwBDm1Wt185S xCDOeufpd7r5A14NcinJIk5a/kEiffXqR/uoM=

In-reply-to: <20091008000104.GN18887@xxxxxxxxxxxxxx>

References: <5e0d09550910071050v6deba397g60e3013d150522a5@xxxxxxxxxxxxxx> <82a458c00910071216n4019785drbb6d6ab5f926cf0c@xxxxxxxxxxxxxx> <5e0d09550910071430j30b82956hb64af11d4372a69f@xxxxxxxxxxxxxx> <20091008000104.GN18887@xxxxxxxxxxxxxx>

Reply-to: libevent-users@xxxxxxxxxxxxx

Sender: owner-libevent-users@xxxxxxxxxxxxx

On Thu, Oct 8, 2009 at 3:01 AM, Nick Mathewson <nickm@xxxxxxxxxxxxx> wrote:

So long as we are painting the bike shed, I'd suggest that we just
admit that we want a signed type and use an ssize_t (or in our case,
an ev_ssize_t) for this. No muss, no fuss.

As for the original patch, I'm wondering a bit about the complexity.
I'm assuming that the idea here is to keep from running out of memory
if the HTTP request or response is too big or complex, and that sounds
like a fine idea. But what's the rationale for having separate limits
for the first line and for the total headers? And why limit both
the number of headers and their total length?

Please see the updated patch.

diff -ur ./libevent-2.0.2-alpha/http.c ./libevent-2.0.2-alpha-patched/http.c --- ./libevent-2.0.2-alpha/http.c 2009-05-15 09:40:58.000000000 +0300 +++ ./libevent-2.0.2-alpha-patched/http.c 2009-10-15 22:20:09.000000000 +0300 @@ -556,6 +556,19 @@ } } +void +evhttp_connection_set_max_headers_size(struct evhttp_connection *evcon, + ssize_t new_max_headers_size) +{ + evcon->max_headers_size = new_max_headers_size; +} +void +evhttp_connection_set_max_body_size(struct evhttp_connection* evcon, + ssize_t new_max_body_size) +{ + evcon->max_body_size = new_max_body_size; +} + static int evhttp_connection_incoming_fail(struct evhttp_request *req, enum evhttp_connection_error error) @@ -759,6 +772,13 @@ /* could not get chunk size */ return (DATA_CORRUPTED); } + if (req->evcon->max_body_size != SIZE_UNLIMITED && + req->body_size + ntoread > req->evcon->max_body_size) { + /* failed body length test */ + event_debug(("%s\n", "Request body is too long")); + return (DATA_CORRUPTED); + } + req->body_size += ntoread; req->ntoread = ntoread; if (req->ntoread == 0) { /* Last chunk */ @@ -839,14 +859,25 @@ } else if (req->ntoread < 0) { /* Read until connection close. */ evbuffer_add_buffer(req->input_buffer, buf); + req->body_size += evbuffer_get_length(buf); } else if (req->chunk_cb != NULL || evbuffer_get_length(buf) >= req->ntoread) { /* We've postponed moving the data until now, but we're * about to use it. */ req->ntoread -= evbuffer_get_length(buf); + req->body_size += evbuffer_get_length(buf); evbuffer_add_buffer(req->input_buffer, buf); } + if (req->evcon->max_body_size != SIZE_UNLIMITED && + req->body_size > req->evcon->max_body_size) { + /* failed body length test */ + event_debug(("%s\n", "Request body is too long")); + evhttp_connection_fail(evcon, + EVCON_HTTP_INVALID_HEADER); + return; + } + if (evbuffer_get_length(req->input_buffer) > 0 && req->chunk_cb != NULL) { req->flags |= EVHTTP_REQ_DEFER_FREE; (*req->chunk_cb)(req, req->cb_arg); @@ -1451,10 +1482,26 @@ char *line; enum message_read_status status = ALL_DATA_READ; - line = evbuffer_readln(buffer, NULL, EVBUFFER_EOL_CRLF); - if (line == NULL) - return (MORE_DATA_EXPECTED); + size_t line_length; + line = evbuffer_readln(buffer, &line_length, EVBUFFER_EOL_CRLF); + if (line == NULL) { + if (req->evcon != NULL && + req->evcon->max_headers_size != SIZE_UNLIMITED && + evbuffer_get_length(buffer) > req->evcon->max_headers_size) + return (DATA_CORRUPTED); + else + return (MORE_DATA_EXPECTED); + } + + if (req->evcon != NULL && + req->evcon->max_headers_size != SIZE_UNLIMITED && + line_length > req->evcon->max_headers_size) { + mm_free(line); + return (DATA_CORRUPTED); + } + req->headers_size = line_length; + switch (req->kind) { case EVHTTP_REQUEST: if (evhttp_parse_request_line(req, line) == -1) @@ -1502,10 +1549,18 @@ enum message_read_status status = MORE_DATA_EXPECTED; struct evkeyvalq* headers = req->input_headers; - while ((line = evbuffer_readln(buffer, NULL, EVBUFFER_EOL_CRLF)) + size_t line_length; + while ((line = evbuffer_readln(buffer, &line_length, EVBUFFER_EOL_CRLF)) != NULL) { char *skey, *svalue; + req->headers_size += line_length; + + if (req->evcon != NULL && + req->evcon->max_headers_size != SIZE_UNLIMITED && + req->headers_size > req->evcon->max_headers_size) + goto error; + if (*line == '\0') { /* Last header - Done */ status = ALL_DATA_READ; mm_free(line); @@ -1534,6 +1589,12 @@ mm_free(line); } + if (status == MORE_DATA_EXPECTED) { + if (req->evcon->max_headers_size != SIZE_UNLIMITED && + req->headers_size + evbuffer_get_length(buffer) > req->evcon->max_headers_size) + return (DATA_CORRUPTED); + } + return (status); error: @@ -1713,6 +1774,9 @@ evcon->fd = -1; evcon->port = port; + evcon->max_headers_size = SIZE_UNLIMITED; + evcon->max_body_size = SIZE_UNLIMITED; + evcon->timeout = -1; evcon->retry_cnt = evcon->retry_max = 0; @@ -2444,7 +2508,9 @@ } http->timeout = -1; - + evhttp_set_max_headers_size(http, SIZE_UNLIMITED); + evhttp_set_max_body_size(http, SIZE_UNLIMITED); + TAILQ_INIT(&http->sockets); TAILQ_INIT(&http->callbacks); TAILQ_INIT(&http->connections); @@ -2561,6 +2627,14 @@ http->timeout = timeout_in_secs; } +void evhttp_set_max_headers_size(struct evhttp* http, ssize_t max_headers_size) { + http->default_max_headers_size = max_headers_size; +} + +void evhttp_set_max_body_size(struct evhttp* http, ssize_t max_body_size) { + http->default_max_body_size = max_body_size; +} + int evhttp_set_cb(struct evhttp *http, const char *uri, void (*cb)(struct evhttp_request *, void *), void *cbarg) @@ -2626,6 +2700,9 @@ goto error; } + req->headers_size = 0; + req->body_size = 0; + req->kind = EVHTTP_RESPONSE; req->input_headers = mm_calloc(1, sizeof(struct evkeyvalq)); if (req->input_headers == NULL) { @@ -2778,6 +2855,9 @@ if (evcon == NULL) return (NULL); + evhttp_connection_set_max_headers_size(evcon, http->default_max_headers_size); + evhttp_connection_set_max_body_size(evcon, http->default_max_body_size); + evcon->flags |= EVHTTP_CON_INCOMING; evcon->state = EVCON_READING_FIRSTLINE; diff -ur ./libevent-2.0.2-alpha/http-internal.h ./libevent-2.0.2-alpha-patched/http-internal.h --- ./libevent-2.0.2-alpha/http-internal.h 2009-07-25 06:42:30.000000000 +0300 +++ ./libevent-2.0.2-alpha-patched/http-internal.h 2009-10-15 16:11:02.000000000 +0300 @@ -20,6 +20,8 @@ #define HTTP_PREFIX "http://"; #define HTTP_DEFAULTPORT 80 +#define SIZE_UNLIMITED -1 + enum message_read_status { ALL_DATA_READ = 1, MORE_DATA_EXPECTED = 0, @@ -70,6 +72,9 @@ char *address; /* address to connect to */ u_short port; + ssize_t max_headers_size; + ssize_t max_body_size; + int flags; #define EVHTTP_CON_INCOMING 0x0001 /* only one request on it ever */ #define EVHTTP_CON_OUTGOING 0x0002 /* multiple requests possible */ @@ -127,8 +132,11 @@ /* NULL if this server is not a vhost */ char *vhost_pattern; - int timeout; + int timeout; + size_t default_max_headers_size; + size_t default_max_body_size; + void (*gencb)(struct evhttp_request *req, void *); void *gencbarg; diff -ur ./libevent-2.0.2-alpha/include/event2/http.h ./libevent-2.0.2-alpha-patched/include/event2/http.h --- ./libevent-2.0.2-alpha/include/event2/http.h 2009-07-25 06:42:30.000000000 +0300 +++ ./libevent-2.0.2-alpha-patched/include/event2/http.h 2009-10-15 16:12:07.000000000 +0300 @@ -119,6 +119,9 @@ */ void evhttp_free(struct evhttp* http); +void evhttp_set_max_headers_size(struct evhttp* http, ssize_t max_headers_size); +void evhttp_set_max_body_size(struct evhttp* http, ssize_t max_body_size); + /** Set a callback for a specified URI @@ -313,6 +316,12 @@ /** Returns 1 if the request is owned by the user */ int evhttp_request_is_owned(struct evhttp_request *req); +void evhttp_connection_set_max_headers_size(struct evhttp_connection *evcon, + ssize_t new_max_headers_size); + +void evhttp_connection_set_max_body_size(struct evhttp_connection* evcon, + ssize_t new_max_body_size); + /** Frees an http connection */ void evhttp_connection_free(struct evhttp_connection *evcon); diff -ur ./libevent-2.0.2-alpha/include/event2/http_struct.h ./libevent-2.0.2-alpha-patched/include/event2/http_struct.h --- ./libevent-2.0.2-alpha/include/event2/http_struct.h 2009-05-29 01:50:37.000000000 +0300 +++ ./libevent-2.0.2-alpha-patched/include/event2/http_struct.h 2009-10-15 16:49:06.000000000 +0300 @@ -88,6 +88,9 @@ enum evhttp_request_kind kind; enum evhttp_cmd_type type; + size_t headers_size; + size_t body_size; + char *uri; /* uri after HTTP request was parsed */ char major; /* HTTP Major number */ Only in ./libevent-2.0.2-alpha/test: regress.gen.c Only in ./libevent-2.0.2-alpha/test: regress.gen.h diff -ur ./libevent-2.0.2-alpha/test/regress_http.c ./libevent-2.0.2-alpha-patched/test/regress_http.c --- ./libevent-2.0.2-alpha/test/regress_http.c 2009-07-25 06:42:29.000000000 +0300 +++ ./libevent-2.0.2-alpha-patched/test/regress_http.c 2009-10-15 18:03:14.000000000 +0300 @@ -2190,6 +2190,81 @@ evhttp_free(http); } + +static void +http_data_length_constraints_test_done(struct evhttp_request *req, void *arg) +{ + tt_assert(req); + tt_int_op(req->response_code, ==, HTTP_BADREQUEST); +end: + event_loopexit(NULL); +} + +static void +http_data_length_constraints_test(void) +{ + short port = -1; + struct evhttp_connection *evcon = NULL; + struct evhttp_request *req = NULL; + + test_ok = 0; + + http = http_setup(&port, NULL); + + evcon = evhttp_connection_new("127.0.0.1", port); + tt_assert(evcon); + + /* also bind to local host */ + evhttp_connection_set_local_address(evcon, "127.0.0.1"); + + /* + * At this point, we want to schedule an HTTP GET request + * server using our make request method. + */ + + req = evhttp_request_new(http_data_length_constraints_test_done, NULL); + tt_assert(req); + + char long_str[8192]; + memset(long_str, 'a', 8192); + long_str[8191] = '\0'; + /* Add the information that we care about */ + evhttp_set_max_headers_size(http, 8191); + evhttp_add_header(req->output_headers, "Host", "somehost"); + evhttp_add_header(req->output_headers, "Longheader", long_str); + + if (evhttp_make_request(evcon, req, EVHTTP_REQ_GET, "/?arg=val") == -1) { + tt_abort_msg("Couldn't make request"); + } + event_dispatch(); + + req = evhttp_request_new(http_data_length_constraints_test_done, NULL); + tt_assert(req); + evhttp_add_header(req->output_headers, "Host", "somehost"); + + /* GET /?arg=verylongvalue HTTP/1.1 */ + if (evhttp_make_request(evcon, req, EVHTTP_REQ_GET, long_str) == -1) { + tt_abort_msg("Couldn't make request"); + } + event_dispatch(); + + evhttp_set_max_body_size(http, 8190); + req = evhttp_request_new(http_data_length_constraints_test_done, NULL); + evhttp_add_header(req->output_headers, "Host", "somehost"); + evbuffer_add_printf(req->output_buffer, long_str); + if (evhttp_make_request(evcon, req, EVHTTP_REQ_POST, "/") == -1) { + tt_abort_msg("Couldn't make request"); + } + event_dispatch(); + + test_ok = 1; + end: + if (evcon) + evhttp_connection_free(evcon); + if (http) + evhttp_free(http); +} + #define HTTP_LEGACY(name) \ { #name, run_legacy_test_fn, TT_ISOLATED|TT_LEGACY, &legacy_setup, \ http_##name##_test } @@ -2224,6 +2299,7 @@ HTTP_LEGACY(stream_in_cancel), HTTP_LEGACY(connection_retry), + HTTP_LEGACY(data_length_constraints), END_OF_TESTCASES };