Don't read uninitialised data for short session IDs. #2583
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
richsalz
approved these changes
Feb 9, 2017
levitte
pushed a commit
that referenced
this pull request
Feb 10, 2017
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 <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #2583)
levitte
pushed a commit
that referenced
this pull request
Feb 10, 2017
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 <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #2583) (cherry picked from commit bd5d27c)
|
thanks! |
|
By the way, this affects 1.0.2 too. (Dunno what your backporting policy is.) |
|
1.0.2 is maintained for bugs, so it's fitting to backport this to that branch as well. If it's easily cherry-picked, we simply do that. If not, we usually make a separate PR for 1.0.2. |
|
Since this has already been applied, two reviewers must confirm that a backport is ok. |
|
(reopening for approval to backport to 1.0.2. I've just tested that it cherry-picks fine) |
mattcaswell
approved these changes
Feb 13, 2017
levitte
pushed a commit
that referenced
this pull request
Feb 13, 2017
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. (cherry picked from commit bd5d27c) Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2583)
|
Great, merged into 1.0.2 as well |
bernd-edlinger
added a commit
to bernd-edlinger/openssl
that referenced
this pull request
Feb 13, 2017
the uninitialized session_id now.
levitte
pushed a commit
that referenced
this pull request
Feb 28, 2017
the uninitialized session_id now. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #2608)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Apologies, we appear to have forgotten to upstream this one.
Checklist
I'm not familiar with your current test harnesses, so I'm not sure where best to test this. I believe you just need to offer a ClientHello with short session ID and run with MSan. valgrind may work too?
Description of change
While it's always safe to read
SSL_MAX_SSL_SESSION_ID_LENGTHbytesfrom an
SSL_SESSION'ssession_idarray, the hash function would doso with without considering if all those bytes had been written to.
This change checks
session_id_lengthbefore possibly readinguninitialised 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_sessionuses a stack-allocated placeholderSSL_SESSIONas a lookup key, so thesession_idarray may beuninitialised.
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.