[tor-commits] [tor/master] Avoid illegal read off end of an array in prune_v2_cipher_list

[nickm at torproject.org]

commit cca6198c777dba463aeb4a8fba6a953cde9576a8
Author: Nick Mathewson <nickm at torproject.org>
Date:   Tue Jun 10 11:11:47 2014 -0400

    Avoid illegal read off end of an array in prune_v2_cipher_list
    
    This function is supposed to construct a list of all the ciphers in
    the "v2 link protocol cipher list" that are supported by Tor's
    openssl.  It does this by invoking ssl23_get_cipher_by_char on each
    two-byte ciphersuite ID to see which ones give a match.  But when
    ssl23_get_cipher_by_char cannot find a match for a two-byte SSL3/TLS
    ciphersuite ID, it checks to see whether it has a match for a
    three-byte SSL2 ciphersuite ID.  This was causing a read off the end
    of the 'cipherid' array.
    
    This was probably harmless in practice, but we shouldn't be having
    any uninitialized reads.
    
    (Using ssl23_get_cipher_by_char in this way is a kludge, but then
    again the entire existence of the v2 link protocol is kind of a
    kludge.  Once Tor 0.2.2 clients are all gone, we can drop this code
    entirely.)
    
    Found by starlight. Fix on 0.2.4.8-alpha. Fixes bug 12227.
---
 changes/bug12227    |    5 +++++
 src/common/tortls.c |    5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/changes/bug12227 b/changes/bug12227
new file mode 100644
index 0000000..d8b5d08
--- /dev/null
+++ b/changes/bug12227
@@ -0,0 +1,5 @@
+  o Minor bugfixes:
+    - Avoid an illegal read from stack when initializing the TLS
+      module using a version of OpenSSL without all of the ciphers
+      used by the v2 link handshake. Fixes bug 12227; bugfix on
+      0.2.4.8-alpha.  Found by "starlight".
diff --git a/src/common/tortls.c b/src/common/tortls.c
index 8f3f6a7..c13b12f 100644
--- a/src/common/tortls.c
+++ b/src/common/tortls.c
@@ -1489,10 +1489,13 @@ prune_v2_cipher_list(void)
 
   inp = outp = v2_cipher_list;
   while (*inp) {
-    unsigned char cipherid[2];
+    unsigned char cipherid[3];
     const SSL_CIPHER *cipher;
     /* Is there no better way to do this? */
     set_uint16(cipherid, htons(*inp));
+    cipherid[2] = 0; /* If ssl23_get_cipher_by_char finds no cipher starting
+                      * with a two-byte 'cipherid', it may look for a v2
+                      * cipher with the appropriate 3 bytes. */
     cipher = m->get_cipher_by_char(cipherid);
     if (cipher) {
       tor_assert((cipher->id & 0xffff) == *inp);