Merge remote-tracking branch 'public/bug10363_024_squashed'

5 files changed, 48 insertions, 16 deletions

diff --git a/changes/bug10363 b/changes/bug10363
new file mode 100644
index 000000000..0cdfd9aee
--- /dev/null
+++ b/changes/bug10363
@@ -0,0 +1,12 @@
+  o Major bugfixes:
+    - Fix two instances of possible undefined behavior in channeltls.c
+      that could, under unlucky circumstances, have led to a pointer
+      overflow. Fixes bug #10363; bugfixes on 0.2.0.10-alpha and
+      0.2.3.6-alpha. Reported by "bobnomnom".
+    - Fix another possibly undefined pointer operations in tor_memmem
+      fallback implementation. Another case of bug #10363; bugfix on
+      0.1.1.1-alpha.
+    - Fix another possibly undefined pointer operations in the eventdns
+      fallback implementation. Another case of bug #10363; bugfix on
+      0.1.2.1-alpha.
+
diff --git a/src/common/compat.c b/src/common/compat.c
index 135f2c9af..04c9d5923 100644
--- a/src/common/compat.c
+++ b/src/common/compat.c
@@ -560,21 +560,29 @@ tor_memmem(const void *_haystack, size_t hlen,
 #else
   /* This isn't as fast as the GLIBC implementation, but it doesn't need to
    * be. */
-  const char *p, *end;
+  const char *p, *last_possible_start;
   const char *haystack = (const char*)_haystack;
   const char *needle = (const char*)_needle;
   char first;
   tor_assert(nlen);
 
+  if (nlen > hlen)
+    return NULL;
+
   p = haystack;
-  end = haystack + hlen;
+  /* Last position at which the needle could start. */
+  last_possible_start = haystack + hlen - nlen;
   first = *(const char*)needle;
-  while ((p = memchr(p, first, end-p))) {
-    if (p+nlen > end)
-      return NULL;
+  while ((p = memchr(p, first, last_possible_start + 1 - p))) {
     if (fast_memeq(p, needle, nlen))
       return p;
-    ++p;
+    if (++p > last_possible_start) {
+      /* This comparison shouldn't be necessary, since if p was previously
+       * equal to last_possible_start, the next memchr call would be
+       * "memchr(p, first, 0)", which will return NULL. But it clarifies the
+       * logic. */
+      return NULL;
+    }
   }
   return NULL;
 #endif
diff --git a/src/ext/eventdns.c b/src/ext/eventdns.c
index 5ac9c1230..2b2988f1e 100644
--- a/src/ext/eventdns.c
+++ b/src/ext/eventdns.c
@@ -842,10 +842,11 @@ name_parse(u8 *packet, int length, int *idx, char *name_out, size_t name_out_len
 		}
 		if (label_len > 63) return -1;
 		if (cp != name_out) {
-			if (cp + 1 >= end) return -1;
+			if (cp >= name_out + name_out_len - 1) return -1;
 			*cp++ = '.';
 		}
-		if (cp + label_len >= end) return -1;
+		if (label_len > name_out_len ||
+			cp >= name_out + name_out_len - label_len) return -1;
 		memcpy(cp, packet + j, label_len);
 		cp += label_len;
 		j += label_len;
diff --git a/src/or/channeltls.c b/src/or/channeltls.c
index 959ec4744..539ead193 100644
--- a/src/or/channeltls.c
+++ b/src/or/channeltls.c
@@ -1282,7 +1282,6 @@ static void
 channel_tls_process_versions_cell(var_cell_t *cell, channel_tls_t *chan)
 {
   int highest_supported_version = 0;
-  const uint8_t *cp, *end;
   int started_here = 0;
 
   tor_assert(cell);
@@ -1322,11 +1321,15 @@ channel_tls_process_versions_cell(var_cell_t *cell, channel_tls_t *chan)
   }
 
   tor_assert(chan->conn->handshake_state);
-  end = cell->payload + cell->payload_len;
-  for (cp = cell->payload; cp+1 < end; cp += 2) {
-    uint16_t v = ntohs(get_uint16(cp));
-    if (is_or_protocol_version_known(v) && v > highest_supported_version)
-      highest_supported_version = v;
+
+  {
+    int i;
+    const uint8_t *cp = cell->payload;
+    for (i = 0; i < cell->payload_len / 2; ++i, cp += 2) {
+      uint16_t v = ntohs(get_uint16(cp));
+      if (is_or_protocol_version_known(v) && v > highest_supported_version)
+        highest_supported_version = v;
+    }
   }
   if (!highest_supported_version) {
     log_fn(LOG_PROTOCOL_WARN, LD_OR,
@@ -1685,12 +1688,16 @@ channel_tls_process_certs_cell(var_cell_t *cell, channel_tls_t *chan)
   for (i = 0; i < n_certs; ++i) {
     uint8_t cert_type;
     uint16_t cert_len;
-    if (ptr + 3 > cell->payload + cell->payload_len) {
+    if (cell->payload_len < 3)
+      goto truncated;
+    if (ptr > cell->payload + cell->payload_len - 3) {
       goto truncated;
     }
     cert_type = *ptr;
     cert_len = ntohs(get_uint16(ptr+1));
-    if (ptr + 3 + cert_len > cell->payload + cell->payload_len) {
+    if (cell->payload_len < 3 + cert_len)
+      goto truncated;
+    if (ptr > cell->payload + cell->payload_len - cert_len - 3) {
       goto truncated;
     }
     if (cert_type == OR_CERT_TYPE_TLS_LINK ||
diff --git a/src/test/test_util.c b/src/test/test_util.c
index a471b8eb1..877fa840c 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -1212,6 +1212,10 @@ test_util_strmisc(void)
     test_assert(!tor_memmem(haystack, 4, "cde", 3));
     haystack = "ababcad";
     test_eq_ptr(tor_memmem(haystack, 7, "abc", 3), haystack + 2);
+    test_eq_ptr(tor_memmem(haystack, 7, "ad", 2), haystack + 5);
+    test_eq_ptr(tor_memmem(haystack, 7, "cad", 3), haystack + 4);
+    test_assert(!tor_memmem(haystack, 7, "dadad", 5));
+    test_assert(!tor_memmem(haystack, 7, "abcdefghij", 10));
     /* memstr */
     test_eq_ptr(tor_memstr(haystack, 7, "abc"), haystack + 2);
     test_eq_ptr(tor_memstr(haystack, 7, "cad"), haystack + 4);