[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

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



Nick,

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.

--
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
 };