ssl: Improve maintainability and correct keylogging

Remove confusing update function and adjust key logging functionality.
Also add timeout to clear secrets on client side if connection
apparently succeeds and keylog_hs is specified.

The adjustment also means that skipping of key log messages in
ssl_session_SUITE test can be changed back as correction of
connection steping also requires a change in the key
logging functionality.

Author: IngelaAndin

lib/ssl/src/ssl_gen_statem.erl

@@ -1929,16 +1929,16 @@ connection_info(#state{handshake_env = #handshake_env{sni_hostname = SNIHostname
      {sni_hostname, SNIHostname},
      {srp_username, SrpUsername} | CurveInfo] ++ MFLInfo ++ ssl_options_list(Opts).
 
-security_info(#state{connection_states = ConnectionStates,
+security_info(#state{connection_states = #{current_read := Read,
+                                           current_write := Write},
                      static_env = #static_env{role = Role},
                      connection_env = #connection_env{negotiated_version = Version},
                      ssl_options = Opts,
                      protocol_specific = ProtocolSpecific}) ->
-    ReadState = ssl_record:current_connection_state(ConnectionStates, read),
     #{security_parameters :=
 	  #security_parameters{client_random = ClientRand,
                                server_random = ServerRand,
-                               master_secret = MasterSecret}} = ReadState,
+                               master_secret = MasterSecret}} = Read,
     SecInfo = [{client_random, ClientRand},
                {server_random, ServerRand}, {master_secret, MasterSecret}],
     KeepSecrets = maps:get(keep_secrets, Opts, false),
@@ -1948,30 +1948,31 @@ security_info(#state{connection_states = ConnectionStates,
             %% to be able to run in user process context
             [{keep_secrets, false} | SecInfo];
         _  -> %% true or keylog fun tuple
-            maybe_security_info_1_3(Version, ReadState, ProtocolSpecific, Role)
+            maybe_security_info_1_3(Version, Read, Write,  ProtocolSpecific, Role)
                 ++ SecInfo
     end.
 
-maybe_security_info_1_3(Version, #{security_parameters :=
-                             #security_parameters{application_traffic_secret = AppTrafSecretRead,
-                                                  client_early_data_secret = EarlyData
-                                                 }} = ReadState,
-              ProtocolSpecific, Role) when ?TLS_GTE(Version, ?TLS_1_3)->
+maybe_security_info_1_3(Version,
+                        #{security_parameters :=
+                              #security_parameters{application_traffic_secret = AppTrafSecretRead,
+                                                   client_early_data_secret = EarlyData
+                                                  }} = Read, Write,
+                        ProtocolSpecific, Role) when ?TLS_GTE(Version, ?TLS_1_3)->
     N = maps:get(num_key_updates, ProtocolSpecific, 0),
     Sender = maps:get(sender, ProtocolSpecific, undefined),
     case Role of
         server ->
             tls_handshake_1_3:early_data_secret(EarlyData) ++
-                tls_handshake_1_3:hs_traffic_secrets(ReadState) ++
+                tls_handshake_1_3:hs_traffic_secrets(Read, Write) ++
                 [{client_traffic_secret, AppTrafSecretRead, N},
                  {Role, Sender}];
         client ->
             tls_handshake_1_3:early_data_secret(EarlyData) ++
-                tls_handshake_1_3:hs_traffic_secrets(ReadState) ++
+                tls_handshake_1_3:hs_traffic_secrets(Read, Write) ++
                 [{server_traffic_secret, AppTrafSecretRead, N},
                  {Role, Sender}]
     end;
-maybe_security_info_1_3(_,_,_,_) ->
+maybe_security_info_1_3(_,_,_,_,_) ->
     [].
 
 record_cb(tls) ->
@@ -2243,56 +2244,111 @@ maybe_keylog_hs_callback(#alert{level = ?FATAL}, StateName,
     end;
 maybe_keylog_hs_callback(_, _, _) ->
     ok.
- 
+
 keylog_hs_alert(start, _) -> %% TLS 1.3: No secrets yet established
     {[], undefined};
 keylog_hs_alert(wait_sh, _) -> %% TLS 1.3: No secrets yet established
     {[], undefined};
-%% Server alert for certificate validation can happen when client is in connection state already.
-keylog_hs_alert(connection,  #state{static_env = #static_env{role = client},
-                                    connection_env =
-                                        #connection_env{negotiated_version = TlsVersion},
-                                    connection_states = ConnectionStates,
-                                    protocol_specific = #{sender := Sender} = PS})
+keylog_hs_alert(negotiated, #state{static_env = #static_env{role = server},
+                                   connection_env =
+                                       #connection_env{negotiated_version = TlsVersion},
+                                   connection_states = #{current_read := Read,
+                                                         current_write := Write
+                                                        }})
   when ?TLS_GTE(TlsVersion, ?TLS_1_3) ->
-    CSRead =  #{security_parameters :=
-                    #security_parameters{application_traffic_secret = SecretRead,
-                                         client_random = ClientRandom,
-                                         prf_algorithm = Prf}}
-        = ssl_record:current_connection_state(ConnectionStates, read),
-    NRead = maps:get(num_key_updates, PS, 0),
-    {ok, SecretWrite, NWrite} = call(Sender, get_application_traffic_secret),
-    {keylog_hs_1_3(CSRead) ++
-        ssl_logger:keylog_traffic_1_3(client, ClientRandom, Prf, SecretWrite, NWrite) ++
-        ssl_logger:keylog_traffic_1_3(server, ClientRandom, Prf, SecretRead, NRead), ClientRandom};
-keylog_hs_alert(_, #state{connection_env =
+     #{server_handshake_traffic_secret := ServerHSSecret,
+      security_parameters :=
+          #security_parameters{client_random = ClientRandomBin,
+                               client_early_data_secret = EarlySecret,
+                               prf_algorithm = Prf
+                              }} = Write,
+    #{client_handshake_traffic_secret := ClientHSSecret} = Read,
+    {keylog_hs_1_3(ClientRandomBin, Prf, EarlySecret, ServerHSSecret, ClientHSSecret),
+     ClientRandomBin};
+keylog_hs_alert(wait_eoed, #state{static_env = #static_env{role = server},
+                                  connection_env =
+                                      #connection_env{negotiated_version = TlsVersion},
+                                  connection_states = #{pending_read := Read,
+                                                        current_write := Write
+                                                       }})
+  when ?TLS_GTE(TlsVersion, ?TLS_1_3) ->
+    keylog_1_3_client_finished(Read, Write);
+keylog_hs_alert(StateName, #state{static_env = #static_env{role = server},
+                                  connection_states =
+                                      #{current_read := Read,
+                                        current_write := Write
+                                       }}) when StateName == wait_cert;
+                                                StateName == wait_cv;
+                                                StateName == wait_finished ->
+    keylog_1_3_client_finished(Read, Write);
+keylog_hs_alert(connection, #state{static_env = #static_env{role = client},
+                                   connection_env =
+                                       #connection_env{negotiated_version = TlsVersion},
+                                   connection_states = #{current_read := Read,
+                                                         current_write := Write
+                                               }})
+  when ?TLS_GTE(TlsVersion, ?TLS_1_3) ->
+    #{server_handshake_traffic_secret := ServerHSSecret,
+      security_parameters :=
+          #security_parameters{client_random = ClientRandomBin,
+                               client_early_data_secret = EarlySecret,
+                               prf_algorithm = Prf,
+                               master_secret = {master_secret, STrafficSecret}
+                              }}
+        = Read,
+    #{client_handshake_traffic_secret := ClientHSSecret,
+       security_parameters :=
+          #security_parameters{master_secret = {master_secret, CTrafficSecret}
+                              }} = Write,
+
+    {keylog_hs_1_3(ClientRandomBin, Prf, EarlySecret, ClientHSSecret, ServerHSSecret) ++
+         ssl_logger:keylog_traffic_1_3(client, ClientRandomBin, Prf, CTrafficSecret, 0) ++
+         ssl_logger:keylog_traffic_1_3(server, ClientRandomBin, Prf, STrafficSecret, 0),
+     ClientRandomBin};
+keylog_hs_alert(_, #state{static_env = #static_env{role = client},
+                          connection_env =
                               #connection_env{negotiated_version = TlsVersion},
-                          connection_states = ConnectionStates})
+                          connection_states = #{current_read := Read,
+                                                current_write := Write
+                                               }})
   when ?TLS_GTE(TlsVersion, ?TLS_1_3) ->
-    CSRead = #{security_parameters :=
-                   #security_parameters{client_random = ClientRandom}}
-        = ssl_record:pending_connection_state(ConnectionStates, read),
-    {keylog_hs_1_3(CSRead), ClientRandom};
-keylog_hs_alert(_,_) -> % NOT Relevant pre TLS-1.3
+   #{server_handshake_traffic_secret := ServerHSSecret,
+      security_parameters :=
+                   #security_parameters{client_random = ClientRandomBin,
+                                        client_early_data_secret = EarlySecret,
+                                        prf_algorithm = Prf
+                                       }}
+        = Write,
+    #{client_handshake_traffic_secret := ClientHSSecret} = Read,
+    {keylog_hs_1_3(ClientRandomBin, Prf, EarlySecret, ClientHSSecret, ServerHSSecret),
+     ClientRandomBin};
+keylog_hs_alert(_, _) -> % NOT Relevant pre TLS-1.3
     {[], undefined}.
 
-keylog_hs_1_3(#{client_handshake_traffic_secret := ClientHSTrafficSecret,
-                server_handshake_traffic_secret := ServerHSTrafficSecret,
-                security_parameters := #security_parameters{client_random = ClientRandomBin,
-                                                            prf_algorithm = Prf,
-                                                            client_early_data_secret = EarlySecret}}) ->
+keylog_hs_1_3(ClientRandomBin, Prf, EarlySecret, ClientSecret, ServerSecret) ->
     HSItems =
-        ssl_logger:keylog_hs(ClientRandomBin, Prf, ClientHSTrafficSecret, ServerHSTrafficSecret),
+        ssl_logger:keylog_hs(ClientRandomBin, Prf, ClientSecret, ServerSecret),
     case EarlySecret of
         undefined ->
             HSItems;
         _  ->
             [ssl_logger:keylog_early_data(ClientRandomBin, Prf, EarlySecret) | HSItems]
-    end;
-keylog_hs_1_3(_) ->
-    [].
+    end.
+
+keylog_1_3_client_finished(Read, Write) ->
+    #{server_handshake_traffic_secret := ServerHSSecret,
+      security_parameters :=
+                   #security_parameters{client_random = ClientRandomBin,
+                                        client_early_data_secret = EarlySecret,
+                                        prf_algorithm = Prf,
+                                        master_secret = {master_secret, TrafficSecret}
+                                       }}
+        = Write,
+    #{client_handshake_traffic_secret := ClientHSSecret} = Read,
+    {keylog_hs_1_3(ClientRandomBin, Prf, EarlySecret, ServerHSSecret, ClientHSSecret) ++
+         ssl_logger:keylog_traffic_1_3(server, ClientRandomBin, Prf, TrafficSecret, 0),
+     ClientRandomBin}.
 
- 
 %%%################################################################
 %%%#
 %%%# Tracing

lib/ssl/src/tls_client_connection_1_3.erl

@@ -463,12 +463,13 @@ wait_finished(internal,
         State4 = Connection:queue_handshake(Finished, State3),
         %% Send first flight
         {State5, _} = Connection:send_handshake_flight(State4),
-        State6 = tls_handshake_1_3:calculate_traffic_secrets(State5),
-        State7 = tls_handshake_1_3:maybe_calculate_resumption_master_secret(State6),
-        State8 = ssl_record:step_encryption_state(State7),
-        State9 = tls_handshake_1_3:prepare_connection(State8),
+        State6 = tls_handshake_1_3:calculate_write_traffic_secrets(State5),
+        State7 = tls_handshake_1_3:calculate_read_traffic_secrets(State6),
+        State8 = tls_handshake_1_3:maybe_calculate_resumption_master_secret(State7),
+        State9 = ssl_record:step_encryption_state(State8),
+        State10 = tls_handshake_1_3:prepare_connection(State9),
         {Record, State} =
-            ssl_gen_statem:prepare_connection(State9, tls_gen_connection),
+            ssl_gen_statem:prepare_connection(State10, tls_gen_connection),
         KeepSecrets = maps:get(keep_secrets, SSLOpts, false),
         tls_gen_connection_1_3:maybe_traffic_keylog_1_3(KeepSecrets, Role,
                                                         State#state.connection_states, 0),
@@ -487,6 +488,19 @@ wait_finished(Type, Msg, State) ->
                  term(), #state{}) ->
           gen_statem:state_function_result().
 %%--------------------------------------------------------------------
+connection(enter, _, #state{ssl_options = Opts} = State) ->
+    case maps:get(keep_secrets, Opts, undefined) of
+        {keylog_hs, _} ->
+            %% Mitigate consequences of keylog_hs being activated, as
+            %% this forces the client to remember secrets longer, that
+            %% is the client certification can fail after the client
+            %% reached connection state.
+            {next_state, ?STATE(connection), State, [{timeout, 1000, forget}]};
+        _ ->
+            {keep_state, State}
+    end;
+connection(timeout, forget, State) ->
+    {next_state, ?STATE(connection), tls_handshake_1_3:forget_master_secret(State)};
 connection(info, Msg, State) ->
     tls_gen_connection:gen_info(Msg, connection, State);
 connection(Type, Msg, State) ->

lib/ssl/src/tls_gen_connection_1_3.erl

@@ -54,7 +54,7 @@
          do_maybe/0]).
 
 %%--------------------------------------------------------------------
-%%  Internal API 
+%%  Internal API
 %%--------------------------------------------------------------------
 initial_state(Role, Sender, Tab, Host, Port, Socket,
               {SSLOptions, SocketOptions, Trackers}, User,
@@ -255,13 +255,17 @@ handle_resumption(#state{handshake_env = HSEnv0} = State, _) ->
     HSEnv = HSEnv0#handshake_env{resumption = true},
     State#state{handshake_env = HSEnv}.
 
-maybe_forget_hs_secrets({keylog_hs, _}, #state{connection_states = CS} = State) ->
+maybe_forget_hs_secrets({keylog_hs, _}, #state{connection_states =
+                                                   #{current_read := Read0,
+                                                     current_write := Write0} = CS} = State) ->
     %% Server finishes the handshake last and  is able to immediately forget
     %% hs secrets used for handshake alert logging.
-    Read0 = #{security_parameters := SecParams} = ssl_record:current_connection_state(CS, read),
-    Read1 = Read0#{security_parameters => SecParams#security_parameters{client_early_data_secret = undefined}},
-    Read = maps:without([client_handshake_traffic_secret, server_handshake_traffic_secret], Read1),
-    State#state{connection_states = CS#{current_read => Read}};
+    #{security_parameters := SecParams} = Read0,
+    Read1 = Read0#{security_parameters =>
+                       SecParams#security_parameters{client_early_data_secret = undefined}},
+    Read = maps:without([client_handshake_traffic_secret], Read1),
+    Write = maps:without([server_handshake_traffic_secret], Write0),
+    State#state{connection_states = CS#{current_read => Read, current_write => Write}};
 maybe_forget_hs_secrets(_, State) ->
     State.