fix: Fix a bug in tls1.3 code path (#4513)

* fix a bug in tls1.3 code path with unit test to verify the change

* modify comment on if block

* fix formatting to clang-format compatible

* reformat if() block to avoid unnecessarry computation

* free memory used for test

* add a check if tls1.3 is supported before running the test

* change s2n_config_is_encrypt_decrypt_key_available() return type to S2N_RESULT

* add null checks and reformat if() block for readability

* better comments and variable naming

* style fixes and add tls 1.3 verification

* use shorter function name

* modify test to fail on HS type assersion instead of during the HS

* use shorter variable name

Co-authored-by: Lindsay Stewart <[email protected]>

* address readability

* use shorter variable names

* address PR comment

* Update tests/unit/s2n_session_ticket_test.c

Co-authored-by: Lindsay Stewart <[email protected]>

* bail with proper error code

* fix err code

---------

Co-authored-by: Lindsay Stewart <[email protected]>

Author: jouho

tests/unit/s2n_session_ticket_test.c

@@ -1348,6 +1348,74 @@ int main(int argc, char **argv)
         EXPECT_SUCCESS(s2n_config_free(config));
     }
 
+    /* Test: TLS1.3 resumption is successful when key used to encrypt ticket is in decrypt-only state */
+    if (s2n_is_tls13_fully_supported()) {
+        DEFER_CLEANUP(struct s2n_config *client_configuration = s2n_config_new(),
+                s2n_config_ptr_free);
+        EXPECT_NOT_NULL(client_configuration);
+        EXPECT_SUCCESS(s2n_config_set_session_tickets_onoff(client_configuration, 1));
+        EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_configuration, "default_tls13"));
+        EXPECT_SUCCESS(s2n_config_set_unsafe_for_testing(client_configuration));
+
+        DEFER_CLEANUP(struct s2n_config *server_configuration = s2n_config_new(),
+                s2n_config_ptr_free);
+        EXPECT_NOT_NULL(server_configuration);
+        EXPECT_SUCCESS(s2n_config_set_session_tickets_onoff(server_configuration, 1));
+        EXPECT_SUCCESS(s2n_config_set_cipher_preferences(server_configuration, "default_tls13"));
+        EXPECT_SUCCESS(s2n_config_set_unsafe_for_testing(server_configuration));
+        EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(server_configuration,
+                chain_and_key));
+
+        /* Add the key that encrypted the session ticket so that the server will be able to decrypt 
+         * the ticket successfully. 
+         */
+        EXPECT_SUCCESS(s2n_config_add_ticket_crypto_key(server_configuration, ticket_key_name1,
+                s2n_array_len(ticket_key_name1), ticket_key1, s2n_array_len(ticket_key1), 0));
+
+        /* Add a mock delay such that key 1 moves to decrypt-only state */
+        uint64_t mock_delay = server_configuration->encrypt_decrypt_key_lifetime_in_nanos;
+        EXPECT_SUCCESS(s2n_config_set_wall_clock(server_configuration, mock_nanoseconds_since_epoch,
+                &mock_delay));
+
+        /* Add one session ticket key with an intro time in the past so that the key is immediately valid */
+        POSIX_GUARD(server_configuration->wall_clock(server_configuration->sys_clock_ctx, &now));
+        uint64_t key_intro_time = (now / ONE_SEC_IN_NANOS) - ONE_SEC_DELAY;
+        EXPECT_SUCCESS(s2n_config_add_ticket_crypto_key(server_configuration, ticket_key_name2,
+                s2n_array_len(ticket_key_name2), ticket_key2, s2n_array_len(ticket_key2), key_intro_time));
+
+        DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
+                s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(client);
+        EXPECT_SUCCESS(s2n_connection_set_session(client, tls13_serialized_session_state.blob.data,
+                s2n_stuffer_data_available(&tls13_serialized_session_state)));
+        EXPECT_SUCCESS(s2n_connection_set_config(client, client_configuration));
+
+        DEFER_CLEANUP(struct s2n_connection *server = s2n_connection_new(S2N_SERVER),
+                s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(server);
+        EXPECT_SUCCESS(s2n_connection_set_config(server, server_configuration));
+
+        /* Create nonblocking pipes */
+        DEFER_CLEANUP(struct s2n_test_io_stuffer_pair test_io = { 0 },
+                s2n_io_stuffer_pair_free);
+        EXPECT_OK(s2n_io_stuffer_pair_init(&test_io));
+        EXPECT_OK(s2n_connections_set_io_stuffer_pair(client, server, &test_io));
+
+        EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server, client));
+
+        /* Verify that TLS1.3 was negotiated */
+        EXPECT_EQUAL(client->actual_protocol_version, S2N_TLS13);
+        EXPECT_EQUAL(server->actual_protocol_version, S2N_TLS13);
+
+        /* Expect a resumption handshake because the session ticket is valid.
+         * If a full handshake is performed instead, then the session ticket is incorrectly
+         * being evaluated as invalid. This was previously known to happen with a decrypt-only
+         * key because we'd incorrectly try to set a TLS1.2-only handshake type flag,
+         * triggering an error while decrypting the session ticket.
+         */
+        EXPECT_TRUE(IS_RESUMPTION_HANDSHAKE(server));
+    }
+
     EXPECT_SUCCESS(s2n_io_pair_close(&io_pair));
     EXPECT_SUCCESS(s2n_cert_chain_and_key_free(chain_and_key));
     EXPECT_SUCCESS(s2n_cert_chain_and_key_free(ecdsa_chain_and_key));

tls/extensions/s2n_client_session_ticket.c

@@ -63,7 +63,7 @@ static int s2n_client_session_ticket_recv(struct s2n_connection *conn, struct s2
     if (s2n_stuffer_data_available(extension) == S2N_TLS12_TICKET_SIZE_IN_BYTES) {
         conn->session_ticket_status = S2N_DECRYPT_TICKET;
         POSIX_GUARD(s2n_stuffer_copy(extension, &conn->client_ticket_to_decrypt, S2N_TLS12_TICKET_SIZE_IN_BYTES));
-    } else if (s2n_config_is_encrypt_decrypt_key_available(conn->config) == 1) {
+    } else if (s2n_result_is_ok(s2n_config_is_encrypt_key_available(conn->config))) {
         conn->session_ticket_status = S2N_NEW_TICKET;
     }
 

tls/s2n_handshake_io.c

@@ -1053,7 +1053,7 @@ int s2n_conn_set_handshake_type(struct s2n_connection *conn)
             POSIX_GUARD_RESULT(s2n_validate_ems_status(conn));
 
             /* Set up the handshake to send a session ticket since a valid ticket was not provided */
-            if (s2n_config_is_encrypt_decrypt_key_available(conn->config) == 1) {
+            if (s2n_result_is_ok(s2n_config_is_encrypt_key_available(conn->config))) {
                 conn->session_ticket_status = S2N_NEW_TICKET;
                 POSIX_GUARD_RESULT(s2n_handshake_type_set_tls12_flag(conn, WITH_SESSION_TICKET));
             }

tls/s2n_resume.c

@@ -610,28 +610,30 @@ int s2n_connection_is_ocsp_stapled(struct s2n_connection *conn)
     }
 }
 
-int s2n_config_is_encrypt_decrypt_key_available(struct s2n_config *config)
+S2N_RESULT s2n_config_is_encrypt_key_available(struct s2n_config *config)
 {
+    RESULT_ENSURE_REF(config);
+
     uint64_t now = 0;
     struct s2n_ticket_key *ticket_key = NULL;
-    POSIX_GUARD_RESULT(s2n_config_wall_clock(config, &now));
-    POSIX_ENSURE_REF(config->ticket_keys);
+    RESULT_GUARD(s2n_config_wall_clock(config, &now));
+    RESULT_ENSURE_REF(config->ticket_keys);
 
     uint32_t ticket_keys_len = 0;
-    POSIX_GUARD_RESULT(s2n_set_len(config->ticket_keys, &ticket_keys_len));
+    RESULT_GUARD(s2n_set_len(config->ticket_keys, &ticket_keys_len));
 
     for (uint32_t i = ticket_keys_len; i > 0; i--) {
         uint32_t idx = i - 1;
-        POSIX_GUARD_RESULT(s2n_set_get(config->ticket_keys, idx, (void **) &ticket_key));
+        RESULT_GUARD(s2n_set_get(config->ticket_keys, idx, (void **) &ticket_key));
         uint64_t key_intro_time = ticket_key->intro_timestamp;
 
         if (key_intro_time < now
                 && now < key_intro_time + config->encrypt_decrypt_key_lifetime_in_nanos) {
-            return 1;
+            return S2N_RESULT_OK;
         }
     }
 
-    return 0;
+    RESULT_BAIL(S2N_ERR_NO_TICKET_ENCRYPT_DECRYPT_KEY);
 }
 
 /* This function is used in s2n_get_ticket_encrypt_decrypt_key to compute the weight
@@ -868,18 +870,17 @@ int s2n_decrypt_session_ticket(struct s2n_connection *conn, struct s2n_stuffer *
     POSIX_GUARD(s2n_stuffer_skip_write(&state_stuffer, state_blob_size));
     POSIX_GUARD_RESULT(s2n_deserialize_resumption_state(conn, &from->blob, &state_stuffer));
 
+    if (s2n_connection_get_protocol_version(conn) >= S2N_TLS13) {
+        return S2N_SUCCESS;
+    }
+
+    /* A new key is assigned for the ticket if the key used to encrypt current ticket is expired */
     uint64_t now = 0;
     POSIX_GUARD_RESULT(s2n_config_wall_clock(conn->config, &now));
-
-    /* If the key is in decrypt-only state, then a new key is assigned
-     * for the ticket.
-     */
     if (now >= key->intro_timestamp + conn->config->encrypt_decrypt_key_lifetime_in_nanos) {
-        /* Check if a key in encrypt-decrypt state is available */
-        if (s2n_config_is_encrypt_decrypt_key_available(conn->config) == 1) {
+        if (s2n_result_is_ok(s2n_config_is_encrypt_key_available(conn->config))) {
             conn->session_ticket_status = S2N_NEW_TICKET;
             POSIX_GUARD_RESULT(s2n_handshake_type_set_tls12_flag(conn, WITH_SESSION_TICKET));
-            return S2N_SUCCESS;
         }
     }
     return S2N_SUCCESS;

tls/s2n_resume.h

@@ -75,7 +75,7 @@ int s2n_encrypt_session_ticket(struct s2n_connection *conn, struct s2n_stuffer *
 int s2n_decrypt_session_ticket(struct s2n_connection *conn, struct s2n_stuffer *from);
 int s2n_encrypt_session_cache(struct s2n_connection *conn, struct s2n_stuffer *to);
 int s2n_decrypt_session_cache(struct s2n_connection *conn, struct s2n_stuffer *from);
-int s2n_config_is_encrypt_decrypt_key_available(struct s2n_config *config);
+S2N_RESULT s2n_config_is_encrypt_key_available(struct s2n_config *config);
 int s2n_verify_unique_ticket_key(struct s2n_config *config, uint8_t *hash, uint16_t *insert_index);
 int s2n_config_wipe_expired_ticket_crypto_keys(struct s2n_config *config, int8_t expired_key_index);
 int s2n_config_store_ticket_key(struct s2n_config *config, struct s2n_ticket_key *key);