Skip to content

fix(tls): allow no preceding ccs #1

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.

Sign up for GitHub

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

Draft
jamilbk wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

fix(tls): allow no preceding ccs #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions lib/ssl/src/tls_client_connection_1_3.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,11 +318,14 @@ hello_middlebox_assert(enter, _, State) ->
{keep_state, State};
hello_middlebox_assert(internal, #change_cipher_spec{}, State) ->
tls_gen_connection:next_event(wait_ee, no_record, State);
hello_middlebox_assert(internal = Type, #encrypted_extensions{} = Msg,
hello_middlebox_assert(internal, #encrypted_extensions{} = Msg,
#state{ssl_options = #{log_level := Level}} = State) ->
ssl_logger:log(warning, Level, #{description => "Failed to assert middlebox server message",
reason => [{missing, #change_cipher_spec{}}]}, ?LOCATION),
ssl_gen_statem:handle_common_event(Type, Msg, hello_middlebox_assert, State);
%% D.4. Middlebox Compatibility Mode: the server's dummy change_cipher_spec
%% is optional (a server in non-middlebox mode never sends it), so its
%% absence must be tolerated. Process the EncryptedExtensions in 'wait_ee'.
{next_state, wait_ee, State, [{next_event, internal, Msg}]};
hello_middlebox_assert(info, Msg, State) ->
tls_gen_connection:gen_info(Msg, ?STATE(hello_middlebox_assert), State);
hello_middlebox_assert(Type, Msg, State) ->
Expand All @@ -338,11 +341,14 @@ hello_retry_middlebox_assert(enter, _, State) ->
{keep_state, State};
hello_retry_middlebox_assert(internal, #change_cipher_spec{}, State) ->
tls_gen_connection:next_event(wait_sh, no_record, State);
hello_retry_middlebox_assert(internal = Type, #server_hello{} = Msg,
hello_retry_middlebox_assert(internal, #server_hello{} = Msg,
#state{ssl_options = #{log_level := Level}} = State) ->
ssl_logger:log(warning, Level, #{description => "Failed to assert middlebox server message",
reason => [{missing, #change_cipher_spec{}}]}, ?LOCATION),
ssl_gen_statem:handle_common_event(Type, Msg, ?STATE(hello_retry_middlebox_assert), State);
%% D.4. Middlebox Compatibility Mode: the server's dummy change_cipher_spec
%% is optional (a server in non-middlebox mode never sends it), so its
%% absence must be tolerated. Process the ServerHello in 'wait_sh'.
{next_state, wait_sh, State, [{next_event, internal, Msg}]};
hello_retry_middlebox_assert(info, Msg, State) ->
tls_gen_connection:gen_info(Msg, ?STATE(hello_retry_middlebox_assert), State);
hello_retry_middlebox_assert(Type, Msg, State) ->
Expand Down
16 changes: 16 additions & 0 deletions lib/ssl/test/openssl_server_cert_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@
unsupported_sign_algo_cert_client_auth/1,
hello_retry_request/0,
hello_retry_request/1,
hello_retry_request_no_middlebox/0,
hello_retry_request_no_middlebox/1,
no_middlebox_server/0,
no_middlebox_server/1,
custom_groups/0,
custom_groups/1,
mlkem_groups/0,
Expand Down Expand Up @@ -138,6 +142,8 @@ tls_1_3_protocol_groups() ->
tls_1_3_tests() ->
[
hello_retry_request,
hello_retry_request_no_middlebox,
no_middlebox_server,
custom_groups,
mlkem_groups,
mlkem_hybrid_groups,
Expand Down Expand Up @@ -274,6 +280,16 @@ hello_retry_request() ->
hello_retry_request(Config) ->
ssl_cert_tests:hello_retry_request(Config).
%%--------------------------------------------------------------------
hello_retry_request_no_middlebox() ->
ssl_cert_tests:hello_retry_request_no_middlebox().
hello_retry_request_no_middlebox(Config) ->
ssl_cert_tests:hello_retry_request_no_middlebox(Config).
%%--------------------------------------------------------------------
no_middlebox_server() ->
ssl_cert_tests:no_middlebox_server().
no_middlebox_server(Config) ->
ssl_cert_tests:no_middlebox_server(Config).
%%--------------------------------------------------------------------
custom_groups() ->
ssl_cert_tests:custom_groups().
custom_groups(Config) ->
Expand Down
48 changes: 48 additions & 0 deletions lib/ssl/test/ssl_cert_tests.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@
certs_keys_signature_algs_selection/1,
hello_retry_request/0,
hello_retry_request/1,
hello_retry_request_no_middlebox/0,
hello_retry_request_no_middlebox/1,
no_middlebox_server/0,
no_middlebox_server/1,
custom_groups/0,
custom_groups/1,
mlkem_groups/0,
Expand Down Expand Up @@ -751,6 +755,50 @@ hello_retry_request(Config) ->

ssl_test_lib:basic_test(ClientOpts, ServerOpts, Config).
%%--------------------------------------------------------------------
hello_retry_request_no_middlebox() ->
[{doc, "Test that a TLS-1.3 client using middlebox compatibility mode can "
"complete a HelloRetryRequest handshake against a server that does not "
"send the optional dummy change_cipher_spec record (RFC 8446 D.4). "
"Regression test for GH-11237: the client must tolerate the missing "
"change_cipher_spec in the hello_retry_middlebox_assert state instead "
"of aborting with an unexpected_message alert."}].

hello_retry_request_no_middlebox(Config) ->
ClientOpts0 = ssl_test_lib:ssl_options(client_cert_opts, Config),
ServerOpts0 = ssl_test_lib:ssl_options(server_cert_opts, Config),

{ServerOpts1, ClientOpts} =
group_config(Config,
[{versions, ['tlsv1.3']} | ServerOpts0],
[{versions, ['tlsv1.3']},
{middlebox_comp_mode, true} | ClientOpts0]),

%% Tell the OpenSSL server to omit the dummy change_cipher_spec records,
%% in particular the one normally sent after the HelloRetryRequest.
ServerOpts = [{middlebox, false} | ServerOpts1],

ssl_test_lib:basic_test(ClientOpts, ServerOpts, Config).
%%--------------------------------------------------------------------
no_middlebox_server() ->
[{doc, "Test that a TLS-1.3 client using middlebox compatibility mode can "
"complete an ordinary (non-HelloRetryRequest) handshake against a server "
"that does not send the optional dummy change_cipher_spec record "
"(RFC 8446 D.4). The client must tolerate the missing change_cipher_spec "
"in the hello_middlebox_assert state instead of aborting with an "
"unexpected_message alert."}].

no_middlebox_server(Config) ->
ClientOpts0 = ssl_test_lib:ssl_options(client_cert_opts, Config),
ServerOpts0 = ssl_test_lib:ssl_options(server_cert_opts, Config),

ClientOpts = [{versions, ['tlsv1.3']},
{middlebox_comp_mode, true} | ClientOpts0],
%% Tell the OpenSSL server to omit the dummy change_cipher_spec record it
%% would otherwise send after its ServerHello.
ServerOpts = [{versions, ['tlsv1.3']}, {middlebox, false} | ServerOpts0],

ssl_test_lib:basic_test(ClientOpts, ServerOpts, Config).
%%--------------------------------------------------------------------
custom_groups() ->
[{doc,"Test that ssl server can select a common group for key-exchange"}].

Expand Down
21 changes: 18 additions & 3 deletions lib/ssl/test/ssl_test_lib.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -941,7 +941,16 @@ init_openssl_server(openssl, _, Options) ->
Args0 ++ ["-early_data", "-no_anti_replay", "-max_early_data",
integer_to_list(MaxSize)]
end,
Args = maybe_force_ipv4(Args1),
Args2 = case proplists:get_value(middlebox, Options, true) of
false ->
%% Disable the dummy change_cipher_spec records that
%% s_server otherwise sends for middlebox compatibility
%% (RFC 8446 D.4).
Args1 ++ ["-no_middlebox"];
_ ->
Args1
end,
Args = maybe_force_ipv4(Args2),
SslPort = portable_open_port(Exe, Args),
wait_for_openssl_server(Port, proplists:get_value(protocol, Options, tls)),
Pid ! {started, SslPort},
Expand Down Expand Up @@ -2545,17 +2554,23 @@ start_server(openssl, ClientOpts, ServerOpts, Config) ->
SessionArgs = proplists:get_value(session_args, Config, []),
DOpenssl = proplists:get_value(debug_openssl, ServerOpts, false),
Debug = openssl_debug_options(DOpenssl),
%% Disable the dummy change_cipher_spec records that s_server otherwise
%% sends for middlebox compatibility (RFC 8446 D.4).
MiddleBox = case proplists:get_value(middlebox, ServerOpts, true) of
false -> ["-no_middlebox"];
_ -> []
end,

Args = case Groups0 of
undefined ->
["s_server", "-accept", integer_to_list(Port), cipher_flag(Version),
ciphers(Ciphers, Version),
version_flag(Version)] ++ sig_algs(SigAlgs) ++ CertArgs ++ SessionArgs ++ Debug;
version_flag(Version)] ++ MiddleBox ++ sig_algs(SigAlgs) ++ CertArgs ++ SessionArgs ++ Debug;
Group ->
["s_server", "-accept", integer_to_list(Port), cipher_flag(Version),
ciphers(Ciphers, Version), "-groups", Group,
version_flag(Version)] ++
sig_algs(SigAlgs) ++ CertArgs ++ SessionArgs ++ Debug
MiddleBox ++ sig_algs(SigAlgs) ++ CertArgs ++ SessionArgs ++ Debug
end,
OpenSslPort = portable_open_port(Exe, Args),
true = port_command(OpenSslPort, "Hello world"),
Expand Down