Nick,
On Thu, Oct 8, 2009 at 3:01 AM, Nick Mathewson <[email protected]> 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.
--
WBR,
Constantine
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
};
