diff options
| author | David Benjamin <davidben@google.com> | 2017-02-09 15:13:13 -0500 |
|---|---|---|
| committer | Rich Salz <rsalz@openssl.org> | 2017-02-09 17:11:12 -0500 |
| commit | bd5d27c1c6d3f83464ddf5124f18a2cac2cbb37f (patch) | |
| tree | ab9ba4daff3f05e9025df9cf0677cf3bd8376b88 /ssl | |
| parent | 76e624a003db22db2d99ece04a15e20fe44c1fbe (diff) | |
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@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2583)
Diffstat (limited to 'ssl')
| -rw-r--r-- | ssl/ssl_lib.c | 16 |
1 files changed, 12 insertions, 4 deletions
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index e4eec4a949..3b18c3ebfd 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); } |