[tor-commits] [tor/master] Fix undefined behavior with pointer addition in channeltls.c

[nickm at torproject.org]

commit 092ac26ea28c17d13bd1d01cab9b3dec276c4dbc
Author: Nick Mathewson <nickm at torproject.org>
Date:   Wed Dec 11 14:45:48 2013 -0500

    Fix undefined behavior with pointer addition in channeltls.c
    
    In C, it's a bad idea to do this:
    
       char *cp = array;
       char *end = array + array_len;
    
       /* .... */
    
       if (cp + 3 >= end) { /* out of bounds */ }
    
    because cp+3 might be more than one off the end of the array, and
    you are only allowed to construct pointers to the array elements,
    and to an element one past the end.  Instead you have to say
    
       if (cp - array + 3 >= array_len) { /* ... */ }
    
    or something like that.
    
    This patch fixes two of these: one in process_versions_cell
    introduced in 0.2.0.10-alpha, and one in process_certs_cell
    introduced in 0.2.3.6-alpha.  These are both tracked under bug
    10363. "bobnomnom" found and reported both. See also 10313.
    
    In our code, this is likely to be a problem as we used it only if we
    get a nasty allocator that makes allocations end close to (void*)-1.
    But it's best not to have to worry about such things at all, so
    let's just fix all of these we can find.
---
 changes/bug10363    |    6 ++++++
 src/or/channeltls.c |   23 +++++++++++++++--------
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/changes/bug10363 b/changes/bug10363
new file mode 100644
index 0000000..cf5b7fd
--- /dev/null
+++ b/changes/bug10363
@@ -0,0 +1,6 @@
+  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".
+
diff --git a/src/or/channeltls.c b/src/or/channeltls.c
index f751c0d..d59083b 100644
--- a/src/or/channeltls.c
+++ b/src/or/channeltls.c
@@ -1174,7 +1174,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);
@@ -1206,11 +1205,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,
@@ -1567,12 +1570,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 ||