commit c4f2faf3019f2c41f9c3b2b8a73b4fe41e881328 Author: Nick Mathewson nickm@torproject.org Date: Mon Feb 13 09:39:46 2017 -0500
Don't atoi off the end of a buffer chunk.
Fixes bug 20894; bugfix on 0.2.0.16-alpha.
We already applied a workaround for this as 20834, so no need to freak out (unless you didn't apply 20384 yet). --- changes/bug20894 | 8 ++++++ src/common/util.c | 13 +++++++++ src/common/util.h | 1 + src/or/buffers.c | 72 ++++++++++++++++++++++++++++++++++++++++--------- src/or/buffers.h | 5 ++++ src/test/test_buffers.c | 44 ++++++++++++++++++++++++++++++ src/test/test_util.c | 1 + 7 files changed, 131 insertions(+), 13 deletions(-)
diff --git a/changes/bug20894 b/changes/bug20894 new file mode 100644 index 0000000..6443396 --- /dev/null +++ b/changes/bug20894 @@ -0,0 +1,8 @@ + o Major bugfixes (HTTP, parsing): + - When parsing a malformed content-length field from an HTTP message, + do not read off the end of the buffer. This bug was a potential + remote denial-of-service attack against Tor clients and relays. + A workaround was released in October 2016, which prevents this + bug from crashing Tor. This is a fix for the underlying issue, + which should no longer matter (if you applied the earlier patch). + Fixes bug 20894; bugfix on 0.2.0.16-alpha. diff --git a/src/common/util.c b/src/common/util.c index a7bce2e..9024dbf 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -712,6 +712,19 @@ tor_strisnonupper(const char *s) return 1; }
+/** Return true iff every character in <b>s</b> is whitespace space; else + * return false. */ +int +tor_strisspace(const char *s) +{ + while (*s) { + if (!TOR_ISSPACE(*s)) + return 0; + s++; + } + return 1; +} + /** As strcmp, except that either string may be NULL. The NULL string is * considered to be before any non-NULL string. */ int diff --git a/src/common/util.h b/src/common/util.h index 479fc8d..6a743a6 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -186,6 +186,7 @@ void tor_strlower(char *s) ATTR_NONNULL((1)); void tor_strupper(char *s) ATTR_NONNULL((1)); int tor_strisprint(const char *s) ATTR_NONNULL((1)); int tor_strisnonupper(const char *s) ATTR_NONNULL((1)); +int tor_strisspace(const char *s); int strcmp_opt(const char *s1, const char *s2); int strcmpstart(const char *s1, const char *s2) ATTR_NONNULL((1,2)); int strcmp_len(const char *s1, const char *s2, size_t len) ATTR_NONNULL((1,2)); diff --git a/src/or/buffers.c b/src/or/buffers.c index 8981fd2..537a14a 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -1090,6 +1090,52 @@ buf_find_string_offset(const buf_t *buf, const char *s, size_t n) return -1; }
+/** + * Scan the HTTP headers in the <b>headerlen</b>-byte string at + * <b>headers</b>, looking for a "Content-Length" header. Try to set + * *<b>result_out</b> to the numeric value of that header if possible. + * Return -1 if the header was malformed, 0 if it was missing, and 1 if + * it was present and well-formed. + */ +/* STATIC */ int +buf_http_find_content_length(const char *headers, size_t headerlen, + size_t *result_out) +{ + const char *p, *newline; + char *len_str, *eos=NULL; + size_t remaining, result; + int ok; + *result_out = 0; /* The caller shouldn't look at this unless the + * return value is 1, but let's prevent confusion */ + +#define CONTENT_LENGTH "\r\nContent-Length: " + p = (char*) tor_memstr(headers, headerlen, CONTENT_LENGTH); + if (p == NULL) + return 0; + + tor_assert(p >= headers && p < headers+headerlen); + remaining = (headers+headerlen)-p; + p += strlen(CONTENT_LENGTH); + remaining -= strlen(CONTENT_LENGTH); + + newline = memchr(p, '\n', remaining); + if (newline == NULL) + return -1; + + len_str = tor_memdup_nulterm(p, newline-p); + /* We limit the size to INT_MAX because other parts of the buffer.c + * code don't like buffers to be any bigger than that. */ + result = (size_t) tor_parse_uint64(len_str, 10, 0, INT_MAX, &ok, &eos); + if (eos && !tor_strisspace(eos)) { + ok = 0; + } else { + *result_out = result; + } + tor_free(len_str); + + return ok ? 1 : -1; +} + /** There is a (possibly incomplete) http statement on <b>buf</b>, of the * form "%s\r\n\r\n%s", headers, body. (body may contain NULs.) * If a) the headers include a Content-Length field and all bytes in @@ -1115,9 +1161,10 @@ fetch_from_buf_http(buf_t *buf, char **body_out, size_t *body_used, size_t max_bodylen, int force_complete) { - char *headers, *p; - size_t headerlen, bodylen, contentlen; + char *headers; + size_t headerlen, bodylen, contentlen=0; int crlf_offset; + int r;
check(); if (!buf->head) @@ -1153,17 +1200,12 @@ fetch_from_buf_http(buf_t *buf, return -1; }
-#define CONTENT_LENGTH "\r\nContent-Length: " - p = (char*) tor_memstr(headers, headerlen, CONTENT_LENGTH); - if (p) { - int i; - i = atoi(p+strlen(CONTENT_LENGTH)); - if (i < 0) { - log_warn(LD_PROTOCOL, "Content-Length is less than zero; it looks like " - "someone is trying to crash us."); - return -1; - } - contentlen = i; + r = buf_http_find_content_length(headers, headerlen, &contentlen); + if (r == -1) { + log_warn(LD_PROTOCOL, "Content-Length is bogus; maybe " + "someone is trying to crash us."); + return -1; + } else if (r == 1) { /* if content-length is malformed, then our body length is 0. fine. */ log_debug(LD_HTTP,"Got a contentlen of %d.",(int)contentlen); if (bodylen < contentlen) { @@ -1176,7 +1218,11 @@ fetch_from_buf_http(buf_t *buf, bodylen = contentlen; log_debug(LD_HTTP,"bodylen reduced to %d.",(int)bodylen); } + } else { + tor_assert(r == 0); + /* Leave bodylen alone */ } + /* all happy. copy into the appropriate places, and return 1 */ if (headers_out) { *headers_out = tor_malloc(headerlen+1); diff --git a/src/or/buffers.h b/src/or/buffers.h index 52b21d5..9762cf2 100644 --- a/src/or/buffers.h +++ b/src/or/buffers.h @@ -97,5 +97,10 @@ struct buf_t { }; #endif
+#ifdef BUFFERS_PRIVATE +int buf_http_find_content_length(const char *headers, size_t headerlen, + size_t *result_out); +#endif + #endif
diff --git a/src/test/test_buffers.c b/src/test/test_buffers.c index 3408da3..9e7bdb8 100644 --- a/src/test/test_buffers.c +++ b/src/test/test_buffers.c @@ -765,6 +765,49 @@ test_buffers_chunk_size(void *arg) ; }
+static void +test_buffers_find_contentlen(void *arg) +{ + static const struct { + const char *headers; + int r; + int contentlen; + } results[] = { + { "Blah blah\r\nContent-Length: 1\r\n\r\n", 1, 1 }, + { "Blah blah\r\n\r\n", 0, 0 }, /* no content-len */ + { "Blah blah Content-Length: 1\r\n", 0, 0 }, /* no content-len. */ + { "Blah blah\r\nContent-Length: 100000\r\n", 1, 100000}, + { "Blah blah\r\nContent-Length: 1000000000000000000000000\r\n", -1, 0}, + { "Blah blah\r\nContent-Length: 0\r\n", 1, 0}, + { "Blah blah\r\nContent-Length: -1\r\n", -1, 0}, + { "Blah blah\r\nContent-Length: 1x\r\n", -1, 0}, + { "Blah blah\r\nContent-Length: 1 x\r\n", -1, 0}, + { "Blah blah\r\nContent-Length: 1 \r\n", 1, 1}, + { "Blah blah\r\nContent-Length: \r\n", -1, 0}, + { "Blah blah\r\nContent-Length: ", -1, 0}, + { "Blah blah\r\nContent-Length: 5050", -1, 0}, + { NULL, 0, 0 } + }; + int i; + + (void)arg; + + for (i = 0; results[i].headers; ++i) { + int r; + size_t sz; + size_t headerlen = strlen(results[i].headers); + char * tmp = tor_memdup(results[i].headers, headerlen);/* ensure no eos */ + sz = 999; /* to ensure it gets set */ + r = buf_http_find_content_length(tmp, headerlen, &sz); + tor_free(tmp); + log_debug(LD_DIR, "%d: %s", i, escaped(results[i].headers)); + tt_int_op(r, ==, results[i].r); + tt_int_op(sz, ==, results[i].contentlen); + } + done: + ; +} + struct testcase_t buffer_tests[] = { { "basic", test_buffers_basic, TT_FORK, NULL, NULL }, { "copy", test_buffer_copy, TT_FORK, NULL, NULL }, @@ -780,6 +823,7 @@ struct testcase_t buffer_tests[] = { { "tls_read_mocked", test_buffers_tls_read_mocked, 0, NULL, NULL }, { "chunk_size", test_buffers_chunk_size, 0, NULL, NULL }, + { "find_contentlen", test_buffers_find_contentlen, 0, NULL, NULL }, END_OF_TESTCASES };
diff --git a/src/test/test_util.c b/src/test/test_util.c index fcda564..dc01d1a 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -9,6 +9,7 @@ #define CONTROL_PRIVATE #define UTIL_PRIVATE #include "or.h" +#include "buffers.h" #include "config.h" #include "control.h" #include "test.h"