Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: memory leak during STEK rotation #5146

Merged
merged 3 commits into from
Feb 28, 2025
Merged

fix: memory leak during STEK rotation #5146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43d0aed
fix: memory leak during STEK rotation
jmayclin Feb 26, 2025
35f3265
address ci failures
jmayclin Feb 26, 2025
28fdf02
address pr feedback
jmayclin Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion codebuild/bin/grep_simple_mistakes.sh
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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;
Comment on lines -136 to +135
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's several places in the code that already use s2n_set_get to iterate over the full set. Should this PR just remove ticket_key_hashes, and not yet swap the type of ticket_keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think as long as it's not too hard to review, I like the shift to array because it more accurately describes how we are using the data structure now.

But the diff would definitely be smaller if we broke it out into two PRs. 🤷 . Lmk what you think!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I lean towards breaking it up into two PRs, since you're already going to have a follow-up PR where you remove the set implementation. That seems like a more logical division to me: this PR only fixes the memory leak, that PR removes set (and part of removing set is removing uses of set).

But it's not a blocker for me.

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
Open in desktop
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
Loading