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>
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; }