[openssl-commits] [openssl] master update

[Rich Salz]

The branch master has been updated
       via  bd5d27c1c6d3f83464ddf5124f18a2cac2cbb37f (commit)
      from  76e624a003db22db2d99ece04a15e20fe44c1fbe (commit)


- Log -----------------------------------------------------------------
commit bd5d27c1c6d3f83464ddf5124f18a2cac2cbb37f
Author: David Benjamin <davidben at google.com>
Date:   Thu Feb 9 15:13:13 2017 -0500

    Don't read uninitialised data for short session IDs.
    
    While it's always safe to read |SSL_MAX_SSL_SESSION_ID_LENGTH| bytes
    from an |SSL_SESSION|'s |session_id| array, the hash function would do
    so with without considering if all those bytes had been written to.
    
    This change checks |session_id_length| before possibly reading
    uninitialised memory. Since the result of the hash function was already
    attacker controlled, and since a lookup of a short session ID will
    always fail, it doesn't appear that this is anything more than a clean
    up.
    
    In particular, |ssl_get_prev_session| uses a stack-allocated placeholder
    |SSL_SESSION| as a lookup key, so the |session_id| array may be
    uninitialised.
    
    This was originally found with libFuzzer and MSan in
    https://boringssl.googlesource.com/boringssl/+/e976e4349d693b4bbb97e1694f45be5a1b22c8c7,
    then by Robert Swiecki with honggfuzz and MSan here. Thanks to both.
    
    Reviewed-by: Geoff Thorpe <geoff at openssl.org>
    Reviewed-by: Rich Salz <rsalz at openssl.org>
    (Merged from https://github.com/openssl/openssl/pull/2583)

-----------------------------------------------------------------------

Summary of changes:
 ssl/ssl_lib.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index e4eec4a..3b18c3e 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -2391,13 +2391,21 @@ int SSL_export_keying_material(SSL *s, unsigned char *out, size_t olen,
 
 static unsigned long ssl_session_hash(const SSL_SESSION *a)
 {
+    const unsigned char *session_id = a->session_id;
     unsigned long l;
+    unsigned char tmp_storage[4];
+
+    if (a->session_id_length < sizeof(tmp_storage)) {
+        memset(tmp_storage, 0, sizeof(tmp_storage));
+        memcpy(tmp_storage, a->session_id, a->session_id_length);
+        session_id = tmp_storage;
+    }
 
     l = (unsigned long)
-        ((unsigned int)a->session_id[0]) |
-        ((unsigned int)a->session_id[1] << 8L) |
-        ((unsigned long)a->session_id[2] << 16L) |
-        ((unsigned long)a->session_id[3] << 24L);
+        ((unsigned long)session_id[0]) |
+        ((unsigned long)session_id[1] << 8L) |
+        ((unsigned long)session_id[2] << 16L) |
+        ((unsigned long)session_id[3] << 24L);
     return (l);
 }