commit e976e4349d693b4bbb97e1694f45be5a1b22c8c7
author Adam Langley <alangley@gmail.com> Mon Feb 15 14:01:10 2016 -0800
committer David Benjamin <davidben@google.com> Thu Feb 18 15:45:48 2016 +0000
tree cd953431a4ce85ec73195d91ea1e8eef8508c929
parent f48fcaf9010a04f2616174b6d61d321bdb21b791

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.

BUG=586800

Change-Id: I5f59f245b51477d6d4fa2cdc20d40bb6b4a3eae7
Reviewed-on: https://boringssl-review.googlesource.com/7150
Reviewed-by: David Benjamin <davidben@google.com>

ssl/ssl_lib.c

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 0a3c8f7..b726011 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -181,12 +181,21 @@
   return 1;
 }
 
-static uint32_t ssl_session_hash(const SSL_SESSION *a) {
+static uint32_t ssl_session_hash(const SSL_SESSION *sess) {
+  const uint8_t *session_id = sess->session_id;
+
+  uint8_t tmp_storage[sizeof(uint32_t)];
+  if (sess->session_id_length < sizeof(tmp_storage)) {
+    memset(tmp_storage, 0, sizeof(tmp_storage));
+    memcpy(tmp_storage, sess->session_id, sess->session_id_length);
+    session_id = tmp_storage;
+  }
+
   uint32_t hash =
-      ((uint32_t)a->session_id[0]) |
-      ((uint32_t)a->session_id[1] << 8) |
-      ((uint32_t)a->session_id[2] << 16) |
-      ((uint32_t)a->session_id[3] << 24);
+      ((uint32_t)session_id[0]) |
+      ((uint32_t)session_id[1] << 8) |
+      ((uint32_t)session_id[2] << 16) |
+      ((uint32_t)session_id[3] << 24);
 
   return hash;
 }