-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ssl: Reject unsupported previous version with protocol alert #6041
ssl: Reject unsupported previous version with protocol alert #6041
Conversation
CT Test Results 2 files 64 suites 44m 21s ⏱️ Results for commit 378e694. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
c5a8e8c
to
789ab64
Compare
1efcc1a
to
67568c9
Compare
lib/ssl/src/tls_connection_1_3.erl
Outdated
start(internal, #client_hello{}, State0) -> | ||
ssl_gen_statem:handle_own_alert(?ALERT_REC(?FATAL, ?PROTOCOL_VERSION), ?FUNCTION_NAME, State0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like a comment for each clause matching #client_hello
explaining what it means and when it can occur.
As I understand it the clauses handle the following cases:
- The user has requested to handle part of the handshake indicated by existence of the
{handshake, hello}
option. We return to the user which can continue the handshake - The client has sent which versions it wants to negotiate and we will check it that is acceptable
- ??? When can this happen (a
#client_hello{}
without the extensionclient_hello_versions
I suggest that we merge the first 2 clauses together and that the test regarding the handshake
option is done only if tls_record:is_acceptable_version({3,4}, ClientVersions)
returns true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I will add some comments. I actually think there are some more things that we might need to think about for this issue so I will work on a new version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a new version now, that will avoid double-checking the version when {handshake, hello} is used. I took the opportunity to get rid of a legacy mechanism and use gen_statem to postpone the handling of the client/server-hello until we receive the continuation with possible new options. I think this can also warrant some work on version handling in TLS-1.2 with considerations of adoption of some TLS-1.3 mechanisms mentioned in the RFC that preferably should affect also TLS-1.2 which I am not certain that we have adopted. This I think should be handled as a separate issue I will make a ticket.
3b5b0c1
to
82a0d24
Compare
82a0d24
to
378e694
Compare
lib/ssl/src/tls_connection_1_3.erl
Outdated
_Else -> | ||
{next_state, hello, State#state{start_or_recv_from = From, | ||
handshake_env = HSEnv#handshake_env{handshake_continue = continue}}, | ||
[{change_callback_module, tls_connection}, | ||
{{timeout, handshake}, Timeout, close}]} | ||
end; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When does this happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a corner case for the server if version is changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a corner case that could happen for the server if the user changes the version option and client happens to work a certain way.
Change implementation use gen_statem postpone for hello message when users want to pause handshake instead of legacy "reimplementation" of postpone Closes erlang#5950
378e694
to
ac9ee1e
Compare
The content of this PR will soon be included in a patch, but as the solution now has a merge conflict I need to handle this locally. |
Closes #5950