[tor-commits] [tor/maint-0.2.8] Fix an errant memset() into the middle of a struct in cell_pack().

[nickm at torproject.org]

commit 8d2978b13c7462776d8a6ba6a6df5b8d9883a633
Author: Nick Mathewson <nickm at torproject.org>
Date:   Tue Jun 27 10:45:29 2017 -0400

    Fix an errant memset() into the middle of a struct in cell_pack().
    
    This mistake causes two possible bugs. I believe they are both
    harmless IRL.
    
    BUG 1: memory stomping
    
    When we call the memset, we are overwriting two 0 bytes past the end
    of packed_cell_t.body. But I think that's harmless in practice,
    because the definition of packed_cell_t is:
    
    // ...
    typedef struct packed_cell_t {
      TOR_SIMPLEQ_ENTRY(packed_cell_t) next;
      char body[CELL_MAX_NETWORK_SIZE];
      uint32_t inserted_time;
    } packed_cell_t;
    
    So we will overwrite either two bytes of inserted_time, or two bytes
    of padding, depending on how the platform handles alignment.
    
    If we're overwriting padding, that's safe.
    
    If we are overwriting the inserted_time field, that's also safe: In
    every case where we call cell_pack() from connection_or.c, we ignore
    the inserted_time field. When we call cell_pack() from relay.c, we
    don't set or use inserted_time until right after we have called
    cell_pack(). SO I believe we're safe in that case too.
    
    BUG 2: memory exposure
    
    The original reason for this memset was to avoid the possibility of
    accidentally leaking uninitialized ram to the network. Now
    remember, if wide_circ_ids is false on a connection, we shouldn't
    actually be sending more than 512 bytes of packed_cell_t.body, so
    these two bytes can only leak to the network if there is another bug
    somewhere else in the code that sends more data than is correct.
    
    Fortunately, in relay.c, where we allocate packed_cell_t in
    packed_cell_new() , we allocate it with tor_malloc_zero(), which
    clears the RAM, right before we call cell_pack. So those
    packed_cell_t.body bytes can't leak any information.
    
    That leaves the two calls to cell_pack() in connection_or.c, which
    use stack-alocated packed_cell_t instances.
    
    In or_handshake_state_record_cell(), we pass the cell's contents to
    crypto_digest_add_bytes(). When we do so, we get the number of
    bytes to pass using the same setting of wide_circ_ids as we passed
    to cell_pack(). So I believe that's safe.
    
    In connection_or_write_cell_to_buf(), we also use the same setting
    of wide_circ_ids in both calls. So I believe that's safe too.
    
    I introduced this bug with 1c0e87f6d8c7a0abdadf1b5cd9082c10abc7f4e2
    back in 0.2.4.11-alpha; it is bug 22737 and CID 1401591
---
 changes/bug22737       | 12 ++++++++++++
 src/or/connection_or.c |  4 +++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/changes/bug22737 b/changes/bug22737
new file mode 100644
index 0000000..f0de8e6
--- /dev/null
+++ b/changes/bug22737
@@ -0,0 +1,12 @@
+  o Minor bugfixes (defensive programming, undefined behavior):
+
+    - Fix a memset() off the end of an array when packing cells.  This
+      bug should be harmless in practice, since the corrupted bytes
+      are still in the same structure, and are always padding bytes,
+      ignored, or immediately overwritten, depending on compiler
+      behavior. Nevertheless, because the memset()'s purpose is to
+      make sure that any other cell-handling bugs can't expose bytes
+      to the network, we need to fix it. Fixes bug 22737; bugfix on
+      0.2.4.11-alpha. Fixes CID 1401591.
+
+
diff --git a/src/or/connection_or.c b/src/or/connection_or.c
index 8e7cd9e..99ea1ef 100644
--- a/src/or/connection_or.c
+++ b/src/or/connection_or.c
@@ -358,9 +358,11 @@ cell_pack(packed_cell_t *dst, const cell_t *src, int wide_circ_ids)
     set_uint32(dest, htonl(src->circ_id));
     dest += 4;
   } else {
+    /* Clear the last two bytes of dest, in case we can accidentally
+     * send them to the network somehow. */
+    memset(dest+CELL_MAX_NETWORK_SIZE-2, 0, 2);
     set_uint16(dest, htons(src->circ_id));
     dest += 2;
-    memset(dest+CELL_MAX_NETWORK_SIZE-2, 0, 2); /*make sure it's clear */
   }
   set_uint8(dest, src->command);
   memcpy(dest+1, src->payload, CELL_PAYLOAD_SIZE);