Skip to content

Commit

Permalink
fix: memory leak during STEK rotation (#5146)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jmayclin
jmayclin authored Feb 28, 2025
1 parent 3b1255c commit 161deb1
Show file tree
Hide file tree
Showing 7 changed files with 63 additions and 87 deletions.
1 change: 0 additions & 1 deletion codebuild/bin/grep_simple_mistakes.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions tests/cbmc/sources/make_common_datastructures.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/s2n_resume_test.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
49 changes: 30 additions & 19 deletions tests/unit/s2n_session_ticket_test.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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));
Expand Down
48 changes: 3 additions & 45 deletions tls/s2n_config.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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)));
Expand All @@ -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;
Expand Down
6 changes: 2 additions & 4 deletions tls/s2n_config.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand Down
41 changes: 26 additions & 15 deletions tls/s2n_resume.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -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 (<)
Expand All @@ -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;
}

Expand All @@ -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 */
Expand Down Expand Up @@ -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) {
Expand All @@ -1027,16 +1026,28 @@ 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;
}

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;
}

Expand Down

0 comments on commit 161deb1

Please sign in to comment.