Fix SSL_clear() for TLSv1.3 #3954
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
bernd-edlinger
approved these changes
Jul 18, 2017
kaduk
approved these changes
Jul 18, 2017
SSL_clear() does not reset the SSL_METHOD if a session already exists in the SSL object. However, TLSv1.3 does not have an externally visible version fixed method (only an internal one). The state machine assumes that we are always starting from a version flexible method for TLSv1.3. The simplest solution is to just fix SSL_clear() to always reset the method if it is using the internal TLSv1.3 version fixed method.
davidben
reviewed
Jul 18, 2017
ssl/ssl_lib.c
Outdated
| && (s->method != s->ctx->method)) { | ||
| if (s->method != s->ctx->method | ||
| && (SSL_IS_TLS13(s) | ||
| || (!ossl_statem_get_in_handshake(s) && s->session == NULL))) { |
There was a problem hiding this comment.
I think you can remove this condition altogether and just do the s->method != s->ctx->method. The session-id mess as a remnant of SSL_set_session locking you to that version. The check exists so you do not accidentally undo the behavior remove here:
ccae4a1#diff-1d72223f932d41f617c50222791649ec
But since that behavior is removed, SSL_clear should follow suit and not special-case session behavior.
There was a problem hiding this comment.
Ah! Nice! Thanks @davidben
We now allow a different protocol version when reusing a session so we can unconditionally reset the SSL_METHOD if it has changed.
mattcaswell
force-pushed
the
tls1_3-fix-ssl_clear
branch
from
d11f8a3
to
cdeec89
Compare
|
New commit pushed addressing @davidben's comment. Also rebased. @bernd-edlinger and/or @kaduk can you reconfirm? |
kaduk
approved these changes
Jul 18, 2017
bernd-edlinger
approved these changes
Jul 18, 2017
|
Pushed. Thanks. |
levitte
pushed a commit
that referenced
this pull request
Jul 18, 2017
SSL_clear() does not reset the SSL_METHOD if a session already exists in the SSL object. However, TLSv1.3 does not have an externally visible version fixed method (only an internal one). The state machine assumes that we are always starting from a version flexible method for TLSv1.3. The simplest solution is to just fix SSL_clear() to always reset the method if it is using the internal TLSv1.3 version fixed method. Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #3954)
levitte
pushed a commit
that referenced
this pull request
Jul 18, 2017
Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #3954)
levitte
pushed a commit
that referenced
this pull request
Jul 18, 2017
We now allow a different protocol version when reusing a session so we can unconditionally reset the SSL_METHOD if it has changed. Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #3954)
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.
SSL_clear() does not reset the SSL_METHOD if a session already exists in
the SSL object. However, TLSv1.3 does not have an externally visible
version fixed method (only an internal one). The state machine assumes
that we are always starting from a version flexible method for TLSv1.3.
The simplest solution is to just fix SSL_clear() to always reset the method
if it is using the internal TLSv1.3 version fixed method.
[Aside: SSL_clear() is a horrendous abomination that should be deprecated at some point in the future]
Checklist