From 161deb1508765f2cfb550b280c4b502bb7f6a19b Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Fri, 28 Feb 2025 09:43:57 -0800 Subject: [PATCH] fix: memory leak during STEK rotation (#5146) --- codebuild/bin/grep_simple_mistakes.sh | 1 - .../cbmc/sources/make_common_datastructures.c | 3 +- tests/unit/s2n_resume_test.c | 2 +- tests/unit/s2n_session_ticket_test.c | 49 ++++++++++++------- tls/s2n_config.c | 48 ++---------------- tls/s2n_config.h | 6 +-- tls/s2n_resume.c | 41 ++++++++++------ 7 files changed, 63 insertions(+), 87 deletions(-) diff --git a/codebuild/bin/grep_simple_mistakes.sh b/codebuild/bin/grep_simple_mistakes.sh index 2f2b9d29da1..36af4b974b5 100755 --- a/codebuild/bin/grep_simple_mistakes.sh +++ b/codebuild/bin/grep_simple_mistakes.sh @@ -88,7 +88,6 @@ KNOWN_MEMCMP_USAGE["$PWD/stuffer/s2n_stuffer_text.c"]=1 KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_psk.c"]=1 KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_protocol_preferences.c"]=1 KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_cipher_suites.c"]=1 -KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_config.c"]=1 KNOWN_MEMCMP_USAGE["$PWD/utils/s2n_map.c"]=3 for file in $S2N_FILES_ASSERT_NOT_USING_MEMCMP; do diff --git a/tests/cbmc/sources/make_common_datastructures.c b/tests/cbmc/sources/make_common_datastructures.c index 59c7590f10d..1def13949a9 100644 --- a/tests/cbmc/sources/make_common_datastructures.c +++ b/tests/cbmc/sources/make_common_datastructures.c @@ -396,8 +396,7 @@ struct s2n_config *cbmc_allocate_s2n_config() s2n_config->monotonic_clock_ctx = malloc(sizeof(*(s2n_config->monotonic_clock_ctx))); s2n_config->client_hello_cb = malloc(sizeof(*(s2n_config->client_hello_cb))); /* Function pointer. */ s2n_config->client_hello_cb_ctx = malloc(sizeof(*(s2n_config->client_hello_cb_ctx))); - s2n_config->ticket_keys = cbmc_allocate_s2n_set(); - s2n_config->ticket_key_hashes = cbmc_allocate_s2n_set(); + s2n_config->ticket_keys = cbmc_allocate_s2n_array(); s2n_config->cache_store_data = malloc(sizeof(*(s2n_config->cache_store_data))); s2n_config->cache_retrieve_data = malloc(sizeof(*(s2n_config->cache_retrieve_data))); s2n_config->cache_delete_data = malloc(sizeof(*(s2n_config->cache_delete_data))); diff --git a/tests/unit/s2n_resume_test.c b/tests/unit/s2n_resume_test.c index 3ce0e528364..0e8ab11c982 100644 --- a/tests/unit/s2n_resume_test.c +++ b/tests/unit/s2n_resume_test.c @@ -1532,7 +1532,7 @@ int main(int argc, char **argv) /* Manually zero out key bytes */ struct s2n_ticket_key *key = NULL; - EXPECT_OK(s2n_set_get(config->ticket_keys, 0, (void **) &key)); + EXPECT_OK(s2n_array_get(config->ticket_keys, 0, (void **) &key)); EXPECT_NOT_NULL(key); POSIX_CHECKED_MEMSET((uint8_t *) key->aes_key, 0, S2N_AES256_KEY_LEN); diff --git a/tests/unit/s2n_session_ticket_test.c b/tests/unit/s2n_session_ticket_test.c index f0d36a7db59..4638c50339c 100644 --- a/tests/unit/s2n_session_ticket_test.c +++ b/tests/unit/s2n_session_ticket_test.c @@ -577,9 +577,9 @@ int main(int argc, char **argv) EXPECT_TRUE(IS_ISSUING_NEW_SESSION_TICKET(server_conn)); /* Verify that the server has only the unexpired key */ - EXPECT_OK(s2n_set_get(server_config->ticket_keys, 0, (void **) &ticket_key)); + EXPECT_OK(s2n_array_get(server_config->ticket_keys, 0, (void **) &ticket_key)); EXPECT_BYTEARRAY_EQUAL(ticket_key->key_name, ticket_key_name2, s2n_array_len(ticket_key_name2)); - EXPECT_OK(s2n_set_len(server_config->ticket_keys, &ticket_keys_len)); + EXPECT_OK(s2n_array_num_elements(server_config->ticket_keys, &ticket_keys_len)); EXPECT_EQUAL(ticket_keys_len, 1); /* Verify that the client received NST */ @@ -765,22 +765,41 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_config_add_ticket_crypto_key(server_config, ticket_key_name3, s2n_array_len(ticket_key_name3), ticket_key3, s2n_array_len(ticket_key3), 0)); /* Try adding the expired keys */ - EXPECT_EQUAL(s2n_config_add_ticket_crypto_key(server_config, ticket_key_name2, s2n_array_len(ticket_key_name2), ticket_key2, s2n_array_len(ticket_key2), 0), -1); - EXPECT_EQUAL(s2n_config_add_ticket_crypto_key(server_config, ticket_key_name1, s2n_array_len(ticket_key_name1), ticket_key1, s2n_array_len(ticket_key1), 0), -1); + EXPECT_SUCCESS(s2n_config_add_ticket_crypto_key(server_config, ticket_key_name2, s2n_array_len(ticket_key_name2), ticket_key2, s2n_array_len(ticket_key2), 0)); + EXPECT_SUCCESS(s2n_config_add_ticket_crypto_key(server_config, ticket_key_name1, s2n_array_len(ticket_key_name1), ticket_key1, s2n_array_len(ticket_key1), 0)); - /* Verify that the config has only one unexpired key */ - EXPECT_OK(s2n_set_get(server_config->ticket_keys, 0, (void **) &ticket_key)); + /* Verify that the config has three unexpired keys */ + EXPECT_OK(s2n_array_get(server_config->ticket_keys, 0, (void **) &ticket_key)); + /* ticket_key3 should have "rotated" to the first index as other keys expired */ EXPECT_BYTEARRAY_EQUAL(ticket_key->key_name, ticket_key_name3, s2n_array_len(ticket_key_name3)); - EXPECT_OK(s2n_set_len(server_config->ticket_keys, &ticket_keys_len)); - EXPECT_EQUAL(ticket_keys_len, 1); - - /* Verify that the total number of key hashes is three */ - EXPECT_OK(s2n_set_len(server_config->ticket_key_hashes, &ticket_keys_len)); + EXPECT_OK(s2n_array_num_elements(server_config->ticket_keys, &ticket_keys_len)); EXPECT_EQUAL(ticket_keys_len, 3); EXPECT_SUCCESS(s2n_config_free(server_config)); }; + /* Attempting to add more than S2N_MAX_TICKET_KEYS causes failures. */ + { + DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free); + EXPECT_SUCCESS(s2n_config_set_session_tickets_onoff(config, 1)); + EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, chain_and_key)); + + uint8_t id = 0; + uint8_t ticket_key_buf[32] = { 0 }; + + for (uint8_t i = 0; i < S2N_MAX_TICKET_KEYS; i++) { + id = i; + ticket_key_buf[0] = i; + EXPECT_SUCCESS(s2n_config_add_ticket_crypto_key(config, + &id, sizeof(id), ticket_key_buf, s2n_array_len(ticket_key_buf), 0)); + } + + id = S2N_MAX_TICKET_KEYS; + ticket_key_buf[0] = S2N_MAX_TICKET_KEYS; + EXPECT_FAILURE(s2n_config_add_ticket_crypto_key(config, &id, sizeof(id), + ticket_key_buf, s2n_array_len(ticket_key_buf), 0)); + }; + /* Scenario 1: Client sends empty ST and server has multiple encrypt-decrypt keys to choose from for encrypting NST. */ { EXPECT_NOT_NULL(client_config = s2n_config_new()); @@ -1058,14 +1077,6 @@ int main(int argc, char **argv) EXPECT_BYTEARRAY_EQUAL(serialized_session_state + S2N_TICKET_KEY_NAME_LOCATION, ticket_key_name2, s2n_array_len(ticket_key_name2)); - /* Verify that the keys are stored from oldest to newest */ - EXPECT_OK(s2n_set_get(server_config->ticket_keys, 0, (void **) &ticket_key)); - EXPECT_BYTEARRAY_EQUAL(ticket_key->key_name, ticket_key_name2, s2n_array_len(ticket_key_name2)); - EXPECT_OK(s2n_set_get(server_config->ticket_keys, 1, (void **) &ticket_key)); - EXPECT_BYTEARRAY_EQUAL(ticket_key->key_name, ticket_key_name1, s2n_array_len(ticket_key_name1)); - EXPECT_OK(s2n_set_get(server_config->ticket_keys, 2, (void **) &ticket_key)); - EXPECT_BYTEARRAY_EQUAL(ticket_key->key_name, ticket_key_name3, s2n_array_len(ticket_key_name3)); - EXPECT_SUCCESS(s2n_shutdown_test_server_and_client(server_conn, client_conn)); EXPECT_SUCCESS(s2n_connection_free(server_conn)); diff --git a/tls/s2n_config.c b/tls/s2n_config.c index f0bbb623266..778dce920e8 100644 --- a/tls/s2n_config.c +++ b/tls/s2n_config.c @@ -308,28 +308,10 @@ struct s2n_config *s2n_config_new(void) return new_config; } -static int s2n_config_store_ticket_key_comparator(const void *a, const void *b) -{ - if (((const struct s2n_ticket_key *) a)->intro_timestamp >= ((const struct s2n_ticket_key *) b)->intro_timestamp) { - return S2N_GREATER_OR_EQUAL; - } else { - return S2N_LESS_THAN; - } -} - -static int s2n_verify_unique_ticket_key_comparator(const void *a, const void *b) -{ - return memcmp(a, b, SHA_DIGEST_LENGTH); -} - int s2n_config_init_session_ticket_keys(struct s2n_config *config) { if (config->ticket_keys == NULL) { - POSIX_ENSURE_REF(config->ticket_keys = s2n_set_new(sizeof(struct s2n_ticket_key), s2n_config_store_ticket_key_comparator)); - } - - if (config->ticket_key_hashes == NULL) { - POSIX_ENSURE_REF(config->ticket_key_hashes = s2n_set_new(SHA_DIGEST_LENGTH, s2n_verify_unique_ticket_key_comparator)); + POSIX_ENSURE_REF(config->ticket_keys = s2n_array_new_with_capacity(sizeof(struct s2n_ticket_key), S2N_MAX_TICKET_KEYS)); } return 0; @@ -338,11 +320,7 @@ int s2n_config_init_session_ticket_keys(struct s2n_config *config) int s2n_config_free_session_ticket_keys(struct s2n_config *config) { if (config->ticket_keys != NULL) { - POSIX_GUARD_RESULT(s2n_set_free_p(&config->ticket_keys)); - } - - if (config->ticket_key_hashes != NULL) { - POSIX_GUARD_RESULT(s2n_set_free_p(&config->ticket_key_hashes)); + POSIX_GUARD_RESULT(s2n_array_free_p(&config->ticket_keys)); } return 0; @@ -956,7 +934,7 @@ int s2n_config_add_ticket_crypto_key(struct s2n_config *config, POSIX_ENSURE(key_len != 0, S2N_ERR_INVALID_TICKET_KEY_LENGTH); uint32_t ticket_keys_len = 0; - POSIX_GUARD_RESULT(s2n_set_len(config->ticket_keys, &ticket_keys_len)); + POSIX_GUARD_RESULT(s2n_array_num_elements(config->ticket_keys, &ticket_keys_len)); POSIX_ENSURE(ticket_keys_len < S2N_MAX_TICKET_KEYS, S2N_ERR_TICKET_KEY_LIMIT); POSIX_ENSURE(name_len != 0, S2N_ERR_INVALID_TICKET_KEY_NAME_OR_NAME_LENGTH); @@ -967,9 +945,6 @@ int s2n_config_add_ticket_crypto_key(struct s2n_config *config, uint8_t name_data[S2N_TICKET_KEY_NAME_LEN] = { 0 }; POSIX_CHECKED_MEMCPY(name_data, name, name_len); - /* ensure the ticket name is not already present */ - POSIX_ENSURE(s2n_find_ticket_key(config, name_data) == NULL, S2N_ERR_INVALID_TICKET_KEY_NAME_OR_NAME_LENGTH); - uint8_t output_pad[S2N_AES256_KEY_LEN + S2N_TICKET_AAD_IMPLICIT_LEN] = { 0 }; struct s2n_blob out_key = { 0 }; POSIX_GUARD(s2n_blob_init(&out_key, output_pad, s2n_array_len(output_pad))); @@ -990,23 +965,6 @@ int s2n_config_add_ticket_crypto_key(struct s2n_config *config, POSIX_GUARD(s2n_hmac_new(&hmac)); POSIX_GUARD(s2n_hkdf(&hmac, S2N_HMAC_SHA256, &salt, &in_key, &info, &out_key)); - DEFER_CLEANUP(struct s2n_hash_state hash = { 0 }, s2n_hash_free); - uint8_t hash_output[SHA_DIGEST_LENGTH] = { 0 }; - - POSIX_GUARD(s2n_hash_new(&hash)); - POSIX_GUARD(s2n_hash_init(&hash, S2N_HASH_SHA1)); - POSIX_GUARD(s2n_hash_update(&hash, out_key.data, out_key.size)); - POSIX_GUARD(s2n_hash_digest(&hash, hash_output, SHA_DIGEST_LENGTH)); - - POSIX_GUARD_RESULT(s2n_set_len(config->ticket_keys, &ticket_keys_len)); - if (ticket_keys_len >= S2N_MAX_TICKET_KEY_HASHES) { - POSIX_GUARD_RESULT(s2n_set_free_p(&config->ticket_key_hashes)); - POSIX_ENSURE_REF(config->ticket_key_hashes = s2n_set_new(SHA_DIGEST_LENGTH, s2n_verify_unique_ticket_key_comparator)); - } - - /* Insert hash key into a sorted array at known index */ - POSIX_GUARD_RESULT(s2n_set_add(config->ticket_key_hashes, hash_output)); - POSIX_CHECKED_MEMCPY(session_ticket_key->key_name, name_data, s2n_array_len(name_data)); POSIX_CHECKED_MEMCPY(session_ticket_key->aes_key, out_key.data, S2N_AES256_KEY_LEN); out_key.data = output_pad + S2N_AES256_KEY_LEN; diff --git a/tls/s2n_config.h b/tls/s2n_config.h index 07d6166d762..1f780b041b6 100644 --- a/tls/s2n_config.h +++ b/tls/s2n_config.h @@ -31,8 +31,7 @@ #include "utils/s2n_blob.h" #include "utils/s2n_set.h" -#define S2N_MAX_TICKET_KEYS 48 -#define S2N_MAX_TICKET_KEY_HASHES 500 /* 10KB */ +#define S2N_MAX_TICKET_KEYS 48 /* * TLS1.3 does not allow alert messages to be fragmented, and some TLS @@ -133,8 +132,7 @@ struct s2n_config { uint64_t session_state_lifetime_in_nanos; - struct s2n_set *ticket_keys; - struct s2n_set *ticket_key_hashes; + struct s2n_array *ticket_keys; uint64_t encrypt_decrypt_key_lifetime_in_nanos; uint64_t decrypt_key_lifetime_in_nanos; diff --git a/tls/s2n_resume.c b/tls/s2n_resume.c index a8b368e9d0b..8966e31d2ec 100644 --- a/tls/s2n_resume.c +++ b/tls/s2n_resume.c @@ -637,11 +637,11 @@ S2N_RESULT s2n_config_is_encrypt_key_available(struct s2n_config *config) RESULT_ENSURE_REF(config->ticket_keys); uint32_t ticket_keys_len = 0; - RESULT_GUARD(s2n_set_len(config->ticket_keys, &ticket_keys_len)); + RESULT_GUARD(s2n_array_num_elements(config->ticket_keys, &ticket_keys_len)); for (uint32_t i = ticket_keys_len; i > 0; i--) { uint32_t idx = i - 1; - RESULT_GUARD(s2n_set_get(config->ticket_keys, idx, (void **) &ticket_key)); + RESULT_GUARD(s2n_array_get(config->ticket_keys, idx, (void **) &ticket_key)); uint64_t key_intro_time = ticket_key->intro_timestamp; if (key_intro_time <= now @@ -668,7 +668,7 @@ int s2n_compute_weight_of_encrypt_decrypt_keys(struct s2n_config *config, /* Compute weight of encrypt-decrypt keys */ for (int i = 0; i < num_encrypt_decrypt_keys; i++) { - POSIX_GUARD_RESULT(s2n_set_get(config->ticket_keys, encrypt_decrypt_keys_index[i], (void **) &ticket_key)); + POSIX_GUARD_RESULT(s2n_array_get(config->ticket_keys, encrypt_decrypt_keys_index[i], (void **) &ticket_key)); uint64_t key_intro_time = ticket_key->intro_timestamp; uint64_t key_encryption_peak_time = key_intro_time + (config->encrypt_decrypt_key_lifetime_in_nanos / 2); @@ -720,11 +720,11 @@ struct s2n_ticket_key *s2n_get_ticket_encrypt_decrypt_key(struct s2n_config *con PTR_ENSURE_REF(config->ticket_keys); uint32_t ticket_keys_len = 0; - PTR_GUARD_RESULT(s2n_set_len(config->ticket_keys, &ticket_keys_len)); + PTR_GUARD_RESULT(s2n_array_num_elements(config->ticket_keys, &ticket_keys_len)); for (uint32_t i = ticket_keys_len; i > 0; i--) { uint32_t idx = i - 1; - PTR_GUARD_RESULT(s2n_set_get(config->ticket_keys, idx, (void **) &ticket_key)); + PTR_GUARD_RESULT(s2n_array_get(config->ticket_keys, idx, (void **) &ticket_key)); uint64_t key_intro_time = ticket_key->intro_timestamp; /* A key can be used at its intro time (<=) and it can be used up to (<) @@ -742,14 +742,14 @@ struct s2n_ticket_key *s2n_get_ticket_encrypt_decrypt_key(struct s2n_config *con } if (num_encrypt_decrypt_keys == 1) { - PTR_GUARD_RESULT(s2n_set_get(config->ticket_keys, encrypt_decrypt_keys_index[0], (void **) &ticket_key)); + PTR_GUARD_RESULT(s2n_array_get(config->ticket_keys, encrypt_decrypt_keys_index[0], (void **) &ticket_key)); return ticket_key; } int8_t idx = 0; PTR_GUARD_POSIX(idx = s2n_compute_weight_of_encrypt_decrypt_keys(config, encrypt_decrypt_keys_index, num_encrypt_decrypt_keys, now)); - PTR_GUARD_RESULT(s2n_set_get(config->ticket_keys, idx, (void **) &ticket_key)); + PTR_GUARD_RESULT(s2n_array_get(config->ticket_keys, idx, (void **) &ticket_key)); return ticket_key; } @@ -764,10 +764,10 @@ struct s2n_ticket_key *s2n_find_ticket_key(struct s2n_config *config, const uint PTR_ENSURE_REF(config->ticket_keys); uint32_t ticket_keys_len = 0; - PTR_GUARD_RESULT(s2n_set_len(config->ticket_keys, &ticket_keys_len)); + PTR_GUARD_RESULT(s2n_array_num_elements(config->ticket_keys, &ticket_keys_len)); for (uint32_t i = 0; i < ticket_keys_len; i++) { - PTR_GUARD_RESULT(s2n_set_get(config->ticket_keys, i, (void **) &ticket_key)); + PTR_GUARD_RESULT(s2n_array_get(config->ticket_keys, i, (void **) &ticket_key)); if (s2n_constant_time_equals(ticket_key->key_name, name, S2N_TICKET_KEY_NAME_LEN)) { /* Check to see if the key has expired */ @@ -1013,10 +1013,9 @@ int s2n_config_wipe_expired_ticket_crypto_keys(struct s2n_config *config, int8_t POSIX_ENSURE_REF(config->ticket_keys); uint32_t ticket_keys_len = 0; - POSIX_GUARD_RESULT(s2n_set_len(config->ticket_keys, &ticket_keys_len)); - + POSIX_GUARD_RESULT(s2n_array_num_elements(config->ticket_keys, &ticket_keys_len)); for (uint32_t i = 0; i < ticket_keys_len; i++) { - POSIX_GUARD_RESULT(s2n_set_get(config->ticket_keys, i, (void **) &ticket_key)); + POSIX_GUARD_RESULT(s2n_array_get(config->ticket_keys, i, (void **) &ticket_key)); if (now >= ticket_key->intro_timestamp + config->encrypt_decrypt_key_lifetime_in_nanos + config->decrypt_key_lifetime_in_nanos) { @@ -1027,7 +1026,7 @@ int s2n_config_wipe_expired_ticket_crypto_keys(struct s2n_config *config, int8_t end: for (int j = 0; j < num_of_expired_keys; j++) { - POSIX_GUARD_RESULT(s2n_set_remove(config->ticket_keys, expired_keys_index[j] - j)); + POSIX_GUARD_RESULT(s2n_array_remove(config->ticket_keys, expired_keys_index[j] - j)); } return 0; @@ -1035,8 +1034,20 @@ int s2n_config_wipe_expired_ticket_crypto_keys(struct s2n_config *config, int8_t int s2n_config_store_ticket_key(struct s2n_config *config, struct s2n_ticket_key *key) { - /* Keys are stored from oldest to newest */ - POSIX_GUARD_RESULT(s2n_set_add(config->ticket_keys, key)); + uint32_t ticket_keys_len = 0; + POSIX_GUARD_RESULT(s2n_array_num_elements(config->ticket_keys, &ticket_keys_len)); + + /* The ticket key name and secret must both be unique. */ + for (uint32_t i = 0; i < ticket_keys_len; i++) { + struct s2n_ticket_key *other_key = NULL; + POSIX_GUARD_RESULT(s2n_array_get(config->ticket_keys, i, (void **) &other_key)); + POSIX_ENSURE(!s2n_constant_time_equals(key->key_name, other_key->key_name, s2n_array_len(key->key_name)), + S2N_ERR_INVALID_TICKET_KEY_NAME_OR_NAME_LENGTH); + POSIX_ENSURE(!s2n_constant_time_equals(key->aes_key, other_key->aes_key, s2n_array_len(key->aes_key)), + S2N_ERR_TICKET_KEY_NOT_UNIQUE); + } + + POSIX_GUARD_RESULT(s2n_array_insert_and_copy(config->ticket_keys, ticket_keys_len, key)); return S2N_SUCCESS; }