From a27927229f8e66a9f8e0d0b6888d71ed10d99e18 Mon Sep 17 00:00:00 2001 From: Mark Ellzey Date: Wed, 25 May 2011 12:58:59 -0400 Subject: [PATCH 1/4] Added several checks for under/overflow conditions in evhttp_handle_chunked_read --- http.c | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/http.c b/http.c index 13a0c973..fa66be45 100644 --- a/http.c +++ b/http.c @@ -840,9 +840,25 @@ evhttp_connection_done(struct evhttp_connection *evcon) static enum message_read_status evhttp_handle_chunked_read(struct evhttp_request *req, struct evbuffer *buf) { - ev_ssize_t len; + ev_ssize_t len; + + if (req == NULL || buf == NULL) { + return DATA_CORRUPTED; + } + + while (1) { + size_t buflen; + + if ((buflen = evbuffer_get_length(buf)) == 0) { + break; + } + + /* evbuffer_get_length returns size_t, but len variable is ssize_t, + * check for overflow conditions */ + if (buflen > SSIZE_MAX) { + return DATA_CORRUPTED; + } - while ((len = evbuffer_get_length(buf)) > 0) { if (req->ntoread < 0) { /* Read chunk size */ ev_int64_t ntoread; @@ -865,6 +881,12 @@ evhttp_handle_chunked_read(struct evhttp_request *req, struct evbuffer *buf) /* could not get chunk size */ return (DATA_CORRUPTED); } + + /* ntoread is signed int64, body_size is unsigned size_t, check for under/overflow conditions */ + if ((ntoread + req->body_size) < ntoread) { + return DATA_CORRUPTED; + } + if (req->body_size + (size_t)ntoread > req->evcon->max_body_size) { /* failed body length test */ event_debug(("Request body is too long")); @@ -879,12 +901,17 @@ evhttp_handle_chunked_read(struct evhttp_request *req, struct evbuffer *buf) continue; } + /* req->ntoread is signed int64, len is ssize_t, based on arch, + * ssize_t could only be 32b, check for these conditions */ + if (req->ntoread > SSIZE_MAX) { + return DATA_CORRUPTED; + } + /* don't have enough to complete a chunk; wait for more */ if (len < req->ntoread) return (MORE_DATA_EXPECTED); /* Completed chunk */ - /* XXXX fixme: what if req->ntoread is > SIZE_T_MAX? */ evbuffer_remove_buffer(buf, req->input_buffer, (size_t)req->ntoread); req->ntoread = -1; if (req->chunk_cb != NULL) { From 84560fc4dcf1f58f46a7c962ea31946424c4fea9 Mon Sep 17 00:00:00 2001 From: Mark Ellzey Date: Wed, 25 May 2011 14:31:09 -0400 Subject: [PATCH 2/4] Added overflow checks in evhttp_read_body and evhttp_get_body --- http.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/http.c b/http.c index fa66be45..152a14ca 100644 --- a/http.c +++ b/http.c @@ -979,13 +979,20 @@ evhttp_read_body(struct evhttp_connection *evcon, struct evhttp_request *req) } } else if (req->ntoread < 0) { /* Read until connection close. */ + /* check for overflow condition */ + if ((req->body_size + evbuffer_get_length(buf)) < req->body_size) { + evhttp_connection_fail(evcon, EVCON_HTTP_INVALID_HEADER); + return; + } + req->body_size += evbuffer_get_length(buf); evbuffer_add_buffer(req->input_buffer, buf); - } else if (req->chunk_cb != NULL || - evbuffer_get_length(buf) >= (size_t)req->ntoread) { + } else if (req->chunk_cb != NULL || evbuffer_get_length(buf) >= (size_t)req->ntoread) { + /* XXX: the above get_length comparison has to be fixed for overflow conditions! */ /* We've postponed moving the data until now, but we're * about to use it. */ size_t n = evbuffer_get_length(buf); + if (n > (size_t) req->ntoread) n = (size_t) req->ntoread; req->ntoread -= n; @@ -996,6 +1003,7 @@ evhttp_read_body(struct evhttp_connection *evcon, struct evhttp_request *req) if (req->body_size > req->evcon->max_body_size || (!req->chunked && req->ntoread >= 0 && (size_t)req->ntoread > req->evcon->max_body_size)) { + /* XXX: The above casted comparison must checked for overflow */ /* failed body length test */ event_debug(("Request body is too long")); evhttp_connection_fail(evcon, @@ -1936,11 +1944,12 @@ evhttp_get_body(struct evhttp_connection *evcon, struct evhttp_request *req) no, we should respond with an error. For now, just optimistically tell the client to send their message body. */ - if (req->ntoread > 0 && - (size_t)req->ntoread > req->evcon->max_body_size) { - evhttp_send_error(req, HTTP_ENTITYTOOLARGE, - NULL); - return; + if (req->ntoread > 0) { + /* ntoread is ev_int64_t, max_body_size is ev_uint64_t */ + if ((req->evcon->max_body_size <= INT64_MAX) && (ev_uint64_t)req->ntoread > req->evcon->max_body_size) { + evhttp_send_error(req, HTTP_ENTITYTOOLARGE, NULL); + return; + } } if (!evbuffer_get_length(bufferevent_get_input(evcon->bufev))) evhttp_send_continue(evcon, req); From 1814ae967705202b76ddf81bc6a43a1ae67df3be Mon Sep 17 00:00:00 2001 From: Mark Ellzey Date: Wed, 25 May 2011 18:52:07 -0400 Subject: [PATCH 3/4] updated EV_S(s)IZE definitions --- http.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/http.c b/http.c index 152a14ca..b04fa5a1 100644 --- a/http.c +++ b/http.c @@ -855,7 +855,7 @@ evhttp_handle_chunked_read(struct evhttp_request *req, struct evbuffer *buf) /* evbuffer_get_length returns size_t, but len variable is ssize_t, * check for overflow conditions */ - if (buflen > SSIZE_MAX) { + if (buflen > EV_SSIZE_MAX) { return DATA_CORRUPTED; } @@ -883,8 +883,8 @@ evhttp_handle_chunked_read(struct evhttp_request *req, struct evbuffer *buf) } /* ntoread is signed int64, body_size is unsigned size_t, check for under/overflow conditions */ - if ((ntoread + req->body_size) < ntoread) { - return DATA_CORRUPTED; + if (ntoread > EV_SIZE_MAX - req->body_size) { + return DATA_CORRUPTED; } if (req->body_size + (size_t)ntoread > req->evcon->max_body_size) { @@ -892,6 +892,7 @@ evhttp_handle_chunked_read(struct evhttp_request *req, struct evbuffer *buf) event_debug(("Request body is too long")); return (DATA_TOO_LONG); } + req->body_size += (size_t)ntoread; req->ntoread = ntoread; if (req->ntoread == 0) { @@ -903,7 +904,7 @@ evhttp_handle_chunked_read(struct evhttp_request *req, struct evbuffer *buf) /* req->ntoread is signed int64, len is ssize_t, based on arch, * ssize_t could only be 32b, check for these conditions */ - if (req->ntoread > SSIZE_MAX) { + if (req->ntoread > EV_SSIZE_MAX) { return DATA_CORRUPTED; } @@ -979,8 +980,7 @@ evhttp_read_body(struct evhttp_connection *evcon, struct evhttp_request *req) } } else if (req->ntoread < 0) { /* Read until connection close. */ - /* check for overflow condition */ - if ((req->body_size + evbuffer_get_length(buf)) < req->body_size) { + if ((size_t)(req->body_size + evbuffer_get_length(buf)) < req->body_size) { evhttp_connection_fail(evcon, EVCON_HTTP_INVALID_HEADER); return; } @@ -1946,7 +1946,7 @@ evhttp_get_body(struct evhttp_connection *evcon, struct evhttp_request *req) send their message body. */ if (req->ntoread > 0) { /* ntoread is ev_int64_t, max_body_size is ev_uint64_t */ - if ((req->evcon->max_body_size <= INT64_MAX) && (ev_uint64_t)req->ntoread > req->evcon->max_body_size) { + if ((req->evcon->max_body_size <= EV_INT64_MAX) && (ev_uint64_t)req->ntoread > req->evcon->max_body_size) { evhttp_send_error(req, HTTP_ENTITYTOOLARGE, NULL); return; } From dbb3c65288e219ce3f05efc3fb6c84ff96cf24a9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 26 May 2011 17:43:17 -0400 Subject: [PATCH 4/4] Fix compilation. --- http.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/http.c b/http.c index b04fa5a1..27ca4292 100644 --- a/http.c +++ b/http.c @@ -840,8 +840,6 @@ evhttp_connection_done(struct evhttp_connection *evcon) static enum message_read_status evhttp_handle_chunked_read(struct evhttp_request *req, struct evbuffer *buf) { - ev_ssize_t len; - if (req == NULL || buf == NULL) { return DATA_CORRUPTED; } @@ -909,7 +907,7 @@ evhttp_handle_chunked_read(struct evhttp_request *req, struct evbuffer *buf) } /* don't have enough to complete a chunk; wait for more */ - if (len < req->ntoread) + if (buflen < req->ntoread) return (MORE_DATA_EXPECTED); /* Completed chunk */