From f2ab097a09390cda3307c9545ff287d41279d1e7 Mon Sep 17 00:00:00 2001 From: Dan Gudmundsson Date: Thu, 27 Apr 2023 16:07:37 +0200 Subject: ssl: Make fail_if_no_peer_cert default true if verify_peer is set Otherwise the client could not send a certificate and the server will accept the connection even if verify_peer is set and the user have forgot to set the fail_if_no_peer_cert. This is changed to make the default options safer. --- lib/ssl/src/ssl.erl | 8 ++++++-- lib/ssl/test/openssl_alpn_SUITE.erl | 2 +- lib/ssl/test/openssl_renegotiate_SUITE.erl | 2 +- lib/ssl/test/ssl_api_SUITE.erl | 8 ++++---- lib/ssl/test/ssl_test_lib.erl | 26 ++++++++++++++------------ 5 files changed, 26 insertions(+), 20 deletions(-) (limited to 'lib') diff --git a/lib/ssl/src/ssl.erl b/lib/ssl/src/ssl.erl index 6e83e16e1e..c96173e98b 100644 --- a/lib/ssl/src/ssl.erl +++ b/lib/ssl/src/ssl.erl @@ -1703,18 +1703,22 @@ validate_versions(dtls, Vsns0) -> opt_verification(UserOpts, Opts0, #{role := Role} = Env) -> {Verify, Opts1} = case get_opt_of(verify, [verify_none, verify_peer], default_verify(Role), UserOpts, Opts0) of + {old, Val} -> + {Val, Opts0}; {_, verify_none} -> {verify_none, Opts0#{verify => verify_none, verify_fun => {none_verify_fun(), []}}}; {_, verify_peer} -> %% If 'verify' is changed from verify_none to verify_peer, (via update_options/3) %% the 'verify_fun' must also be changed to undefined. %% i.e remove verify_none fun - {verify_peer, Opts0#{verify => verify_peer, verify_fun => undefined}} + Temp = Opts0#{verify => verify_peer, verify_fun => undefined}, + {verify_peer, maps:remove(fail_if_no_peer_cert, Temp)} end, Opts2 = opt_cacerts(UserOpts, Opts1, Env), {_, PartialChain} = get_opt_fun(partial_chain, 1, fun(_) -> unknown_ca end, UserOpts, Opts2), - {_, FailNoPeerCert} = get_opt_bool(fail_if_no_peer_cert, false, UserOpts, Opts2), + DefFailNoPeer = Role =:= server andalso Verify =:= verify_peer, + {_, FailNoPeerCert} = get_opt_bool(fail_if_no_peer_cert, DefFailNoPeer, UserOpts, Opts2), assert_server_only(Role, FailNoPeerCert, fail_if_no_peer_cert), option_incompatible(FailNoPeerCert andalso Verify =:= verify_none, [{verify, verify_none}, {fail_if_no_peer_cert, true}]), diff --git a/lib/ssl/test/openssl_alpn_SUITE.erl b/lib/ssl/test/openssl_alpn_SUITE.erl index bb33165bcf..1d0bc82c4e 100644 --- a/lib/ssl/test/openssl_alpn_SUITE.erl +++ b/lib/ssl/test/openssl_alpn_SUITE.erl @@ -194,7 +194,7 @@ erlang_client_alpn_openssl_server_alpn(Config) when is_list(Config) -> erlang_server_alpn_openssl_client_alpn(Config) when is_list(Config) -> ClientOpts = proplists:get_value(client_rsa_opts, Config), - ServerOpts = ssl_test_lib:ssl_options(server_rsa_verify_opts, Config), + ServerOpts = ssl_test_lib:ssl_options(server_rsa_opts, Config), Protocol = <<"spdy/2">>, Server = ssl_test_lib:start_server(erlang, [{from, self()}], [{server_opts, [{alpn_preferred_protocols, diff --git a/lib/ssl/test/openssl_renegotiate_SUITE.erl b/lib/ssl/test/openssl_renegotiate_SUITE.erl index 01b5a7a8a9..5102730396 100644 --- a/lib/ssl/test/openssl_renegotiate_SUITE.erl +++ b/lib/ssl/test/openssl_renegotiate_SUITE.erl @@ -230,7 +230,7 @@ erlang_server_openssl_client_nowrap_seqnum() -> erlang_server_openssl_client_nowrap_seqnum(Config) when is_list(Config) -> process_flag(trap_exit, true), ServerOpts = ssl_test_lib:ssl_options(server_rsa_verify_opts, Config), - ClientOpts = ssl_test_lib:ssl_options(client_rsa_opts, Config), + ClientOpts = ssl_test_lib:ssl_options(client_rsa_verify_opts, Config), {_, ServerNode, _Hostname} = ssl_test_lib:run_where(Config), diff --git a/lib/ssl/test/ssl_api_SUITE.erl b/lib/ssl/test/ssl_api_SUITE.erl index a3bb2f6bdc..215097bd4d 100644 --- a/lib/ssl/test/ssl_api_SUITE.erl +++ b/lib/ssl/test/ssl_api_SUITE.erl @@ -2699,18 +2699,18 @@ options_verify(Config) -> %% fail_if_no_peer_cert, verify, verify_fun, partial_ ?OK(#{fail_if_no_peer_cert := true, verify := verify_peer, verify_fun := undefined, partial_chain := _}, [{fail_if_no_peer_cert, true}, {verify, verify_peer}, {cacerts, [Cert]}], server), - ?OK(#{fail_if_no_peer_cert := false, verify := verify_peer, verify_fun := undefined, partial_chain := _}, - [{verify, verify_peer}, {cacerts, [Cert]}], server), + ?OK(#{fail_if_no_peer_cert := true, verify := verify_peer, verify_fun := undefined, partial_chain := _}, + [{verify, verify_peer}, {cacerts, [Cert]}], server), NewF3 = fun(_,_,_) -> ok end, NewF4 = fun(_,_,_,_) -> ok end, ?OK(#{}, [], client, [fail_if_no_peer_cert]), ?OK(#{verify := verify_none, verify_fun := {NewF3, foo}, partial_chain := _}, [{verify_fun, {NewF3, foo}}], client), - ?OK(#{fail_if_no_peer_cert := false, verify := verify_peer, verify_fun := {NewF3, foo}, partial_chain := _}, + ?OK(#{fail_if_no_peer_cert := true, verify := verify_peer, verify_fun := {NewF3, foo}, partial_chain := _}, [{verify_fun, {NewF3, foo}}, {verify, verify_peer}, {cacerts, [Cert]}], server), - ?OK(#{fail_if_no_peer_cert := false, verify := verify_peer, verify_fun := {NewF4, foo}, partial_chain := _}, + ?OK(#{fail_if_no_peer_cert := true, verify := verify_peer, verify_fun := {NewF4, foo}, partial_chain := _}, [{verify_fun, {NewF4, foo}}, {verify, verify_peer}, {cacerts, [Cert]}], server), diff --git a/lib/ssl/test/ssl_test_lib.erl b/lib/ssl/test/ssl_test_lib.erl index 01235a1d74..64ee3c9d0d 100644 --- a/lib/ssl/test/ssl_test_lib.erl +++ b/lib/ssl/test/ssl_test_lib.erl @@ -973,15 +973,19 @@ openssl_server_loop(Pid, SslPort, Args) -> start_openssl_client(Args0, Config) -> {ClientNode, _, Hostname} = run_where(Config), - ClientOpts = get_client_opts(Config), + + %% io:format("~p:~p: ~p~n",[?MODULE, ?LINE, Args0]), + %% io:format("~p:~p: ~p~n",[?MODULE, ?LINE, Config]), + + ClientOpts0 = get_client_opts(Config), + ClientOpts = proplists:get_value(options, Args0, []) ++ ClientOpts0, DefaultVersions = default_tls_version(ClientOpts), [Version | _] = proplists:get_value(versions, ClientOpts, DefaultVersions), Node = proplists:get_value(node, Args0, ClientNode), - Args = [{from, self()}, - {host, Hostname}, - {options, ClientOpts} | Args0], + Args = [{from, self()}, {host, Hostname} | ClientOpts ++ Args0], - Result = spawn_link(Node, ?MODULE, init_openssl_client, [[{version, Version} | lists:delete(return_port, Args)]]), + Result = spawn_link(Node, ?MODULE, init_openssl_client, + [[{version, Version} | lists:delete(return_port, Args)]]), receive {connected, OpenSSLPort} -> case lists:member(return_port, Args) of @@ -1636,9 +1640,7 @@ make_ec_cert_chains(UserConf, ClientChainType, ServerChainType, Config, Curve) - [{server_config, ServerConf}, {client_config, ClientConf}] = x509_test:gen_pem_config_files(GenCertData, ClientFileBase, ServerFileBase), - {[{verify, verify_peer} | ClientConf], - [{reuseaddr, true}, {verify, verify_peer} | ServerConf] - }. + {ClientConf, [{reuseaddr, true} | ServerConf]}. default_cert_chain_conf() -> %% Use only default options @@ -1654,8 +1656,8 @@ make_rsa_pss_pem(Alg, _UserConf, Config, Suffix) -> Conf = x509_test:gen_pem_config_files(GenCertData, ClientFileBase, ServerFileBase), CConf = proplists:get_value(client_config, Conf), SConf = proplists:get_value(server_config, Conf), - #{server_config => [{verify, verify_peer} | SConf], - client_config => [{verify, verify_peer} | CConf]}. + #{server_config => SConf, + client_config => CConf}. make_rsa_sni_configs() -> Sha = appropriate_sha(crypto:supports()), @@ -2032,7 +2034,7 @@ make_rsa_ecdsa_cert(Config, Curve) -> [{server_rsa_ecdsa_opts, [{reuseaddr, true} | ServerConf]}, {server_rsa_ecdsa_verify_opts, [{ssl_imp, new},{reuseaddr, true}, {verify, verify_peer} | ServerConf]}, - {client_rsa_ecdsa_opts, [{verify, cerify_none} | ClientConf]} | Config]; + {client_rsa_ecdsa_opts, [{verify, verify_none} | ClientConf]} | Config]; _ -> Config end. @@ -2449,7 +2451,7 @@ start_server(erlang, _, ServerOpts, Config) -> {mfa, {ssl_test_lib, check_key_exchange_send_active, [KeyEx]}}, - {options, [{verify, verify_peer} | ServerOpts]}]), + {options, ServerOpts}]), {Server, inet_port(Server)}. sig_algs(undefined) -> -- cgit v1.2.1 From c9c715863d195559995b311b941678f7a470c4c9 Mon Sep 17 00:00:00 2001 From: Dan Gudmundsson Date: Thu, 27 Apr 2023 16:13:48 +0200 Subject: ssl: Fix that users can send data during renegotiation A timing issue was found during testing of the previous commit. Users should be allowed to send data during the renegotiation. --- lib/ssl/src/dtls_connection.erl | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'lib') diff --git a/lib/ssl/src/dtls_connection.erl b/lib/ssl/src/dtls_connection.erl index a37a72efdc..899e7d3305 100644 --- a/lib/ssl/src/dtls_connection.erl +++ b/lib/ssl/src/dtls_connection.erl @@ -479,10 +479,7 @@ wait_cert_verify(info, Event, State) -> wait_cert_verify(state_timeout, Event, State) -> handle_state_timeout(Event, ?FUNCTION_NAME, State); wait_cert_verify(Type, Event, State) -> - try tls_dtls_connection:gen_handshake(?FUNCTION_NAME, Type, Event, State) - catch throw:#alert{} = Alert -> - ssl_gen_statem:handle_own_alert(Alert, ?FUNCTION_NAME, State) - end. + gen_handshake(?FUNCTION_NAME, Type, Event, State). %%-------------------------------------------------------------------- -spec cipher(gen_statem:event_type(), term(), #state{}) -> @@ -506,7 +503,7 @@ cipher(internal = Type, #finished{} = Event, #state{connection_states = Connecti cipher(state_timeout, Event, State) -> handle_state_timeout(Event, ?FUNCTION_NAME, State); cipher(Type, Event, State) -> - gen_handshake(?FUNCTION_NAME, Type, Event, State). + gen_handshake(?FUNCTION_NAME, Type, Event, State). %%-------------------------------------------------------------------- -spec connection(gen_statem:event_type(), @@ -761,6 +758,8 @@ alert_or_reset_connection(Alert, StateName, #state{connection_states = Cs} = Sta {next_state, connection, NewState} end. +gen_handshake(_, {call, _From}, {application_data, _Data}, _State) -> + {keep_state_and_data, [postpone]}; gen_handshake(StateName, Type, Event, State) -> try tls_dtls_connection:StateName(Type, Event, State) catch -- cgit v1.2.1