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).

7 files changed, 131 insertions, 13 deletions

diff --git a/changes/bug20894 b/changes/bug20894
new file mode 100644
index 0000000000..6443396308
--- /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 a7bce2ea6c..9024dbfe10 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 479fc8d610..6a743a6eac 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 8981fd283b..537a14a554 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 52b21d5885..9762cf2af2 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 3408da3aa9..9e7bdb8911 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 fcda564569..dc01d1a94b 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"