From cdb3cbe1fcd78f6290defabc707374ccb1f850da Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Thu, 15 Aug 2024 01:53:17 +0000 Subject: [PATCH 1/9] refactor: replacing memcmp by s2n_constant_time_equals --- crypto/s2n_rsa.c | 2 +- stuffer/s2n_stuffer_text.c | 2 +- tls/s2n_cipher_suites.c | 4 ++-- tls/s2n_connection.c | 5 ++--- tls/s2n_early_data.c | 2 +- tls/s2n_kem.c | 2 +- tls/s2n_protocol_preferences.c | 2 +- tls/s2n_psk.c | 2 +- tls/s2n_resume.c | 4 ++-- tls/s2n_security_policies.c | 2 +- tls/s2n_server_hello.c | 6 +++--- utils/s2n_map.c | 6 +++--- 12 files changed, 19 insertions(+), 20 deletions(-) diff --git a/crypto/s2n_rsa.c b/crypto/s2n_rsa.c index 0274284a164..806c8b866f2 100644 --- a/crypto/s2n_rsa.c +++ b/crypto/s2n_rsa.c @@ -168,7 +168,7 @@ static int s2n_rsa_keys_match(const struct s2n_pkey *pub, const struct s2n_pkey plain_out.size = sizeof(plain_outpad); POSIX_GUARD(s2n_rsa_decrypt(priv, &enc, &plain_out)); - S2N_ERROR_IF(memcmp(plain_in.data, plain_out.data, plain_in.size), S2N_ERR_KEY_MISMATCH); + S2N_ERROR_IF(!s2n_constant_time_equals(plain_in.data, plain_out.data, plain_in.size), S2N_ERR_KEY_MISMATCH); return 0; } diff --git a/stuffer/s2n_stuffer_text.c b/stuffer/s2n_stuffer_text.c index 88f6b96b300..71ffdfce868 100644 --- a/stuffer/s2n_stuffer_text.c +++ b/stuffer/s2n_stuffer_text.c @@ -72,7 +72,7 @@ int s2n_stuffer_read_expected_str(struct s2n_stuffer *stuffer, const char *expec POSIX_ENSURE(s2n_stuffer_data_available(stuffer) >= expected_length, S2N_ERR_STUFFER_OUT_OF_DATA); uint8_t *actual = stuffer->blob.data + stuffer->read_cursor; POSIX_ENSURE_REF(actual); - POSIX_ENSURE(!memcmp(actual, expected, expected_length), S2N_ERR_STUFFER_NOT_FOUND); + POSIX_ENSURE(s2n_constant_time_equals(actual, (uint8_t *) expected, expected_length), S2N_ERR_STUFFER_NOT_FOUND); stuffer->read_cursor += expected_length; POSIX_POSTCONDITION(s2n_stuffer_validate(stuffer)); return S2N_SUCCESS; diff --git a/tls/s2n_cipher_suites.c b/tls/s2n_cipher_suites.c index 4c32e680147..e12ed47c11b 100644 --- a/tls/s2n_cipher_suites.c +++ b/tls/s2n_cipher_suites.c @@ -1151,7 +1151,7 @@ int s2n_set_cipher_as_client(struct s2n_connection *conn, uint8_t wire[S2N_TLS_C struct s2n_cipher_suite *cipher_suite = NULL; for (size_t i = 0; i < security_policy->cipher_preferences->count; i++) { const uint8_t *ours = security_policy->cipher_preferences->suites[i]->iana_value; - if (memcmp(wire, ours, S2N_TLS_CIPHER_SUITE_LEN) == 0) { + if (s2n_constant_time_equals(wire, ours, S2N_TLS_CIPHER_SUITE_LEN) == true) { cipher_suite = security_policy->cipher_preferences->suites[i]; break; } @@ -1198,7 +1198,7 @@ static int s2n_wire_ciphers_contain(const uint8_t *match, const uint8_t *wire, u for (size_t i = 0; i < count; i++) { const uint8_t *theirs = wire + (i * cipher_suite_len) + (cipher_suite_len - S2N_TLS_CIPHER_SUITE_LEN); - if (!memcmp(match, theirs, S2N_TLS_CIPHER_SUITE_LEN)) { + if (s2n_constant_time_equals(match, theirs, S2N_TLS_CIPHER_SUITE_LEN)) { return 1; } } diff --git a/tls/s2n_connection.c b/tls/s2n_connection.c index 090ec3bc476..22251a6847c 100644 --- a/tls/s2n_connection.c +++ b/tls/s2n_connection.c @@ -922,9 +922,8 @@ int s2n_connection_get_cipher_iana_value(struct s2n_connection *conn, uint8_t *f POSIX_ENSURE_MUT(second); /* ensure we've negotiated a cipher suite */ - POSIX_ENSURE(memcmp(conn->secure->cipher_suite->iana_value, - s2n_null_cipher_suite.iana_value, sizeof(s2n_null_cipher_suite.iana_value)) - != 0, + POSIX_ENSURE(!s2n_constant_time_equals(conn->secure->cipher_suite->iana_value, + s2n_null_cipher_suite.iana_value, sizeof(s2n_null_cipher_suite.iana_value)), S2N_ERR_INVALID_STATE); const uint8_t *iana_value = conn->secure->cipher_suite->iana_value; diff --git a/tls/s2n_early_data.c b/tls/s2n_early_data.c index 6898721d9fe..6e2cd36a933 100644 --- a/tls/s2n_early_data.c +++ b/tls/s2n_early_data.c @@ -96,7 +96,7 @@ static S2N_RESULT s2n_early_data_validate(struct s2n_connection *conn) const size_t app_protocol_size = strlen(conn->application_protocol); if (app_protocol_size > 0 || config->application_protocol.size > 0) { RESULT_ENSURE_EQ(config->application_protocol.size, app_protocol_size + 1 /* null-terminating char */); - RESULT_ENSURE_EQ(memcmp(config->application_protocol.data, conn->application_protocol, app_protocol_size), 0); + RESULT_ENSURE_EQ(s2n_constant_time_equals(config->application_protocol.data, (uint8_t *) conn->application_protocol, app_protocol_size), true); } return S2N_RESULT_OK; diff --git a/tls/s2n_kem.c b/tls/s2n_kem.c index 175e2e62599..cc8952a6327 100644 --- a/tls/s2n_kem.c +++ b/tls/s2n_kem.c @@ -296,7 +296,7 @@ int s2n_cipher_suite_to_kem(const uint8_t iana_value[S2N_TLS_CIPHER_SUITE_LEN], { for (size_t i = 0; i < s2n_array_len(kem_mapping); i++) { const struct s2n_iana_to_kem *candidate = &kem_mapping[i]; - if (memcmp(iana_value, candidate->iana_value, S2N_TLS_CIPHER_SUITE_LEN) == 0) { + if (s2n_constant_time_equals(iana_value, candidate->iana_value, S2N_TLS_CIPHER_SUITE_LEN) == true) { *compatible_params = candidate; return S2N_SUCCESS; } diff --git a/tls/s2n_protocol_preferences.c b/tls/s2n_protocol_preferences.c index cf5674e6fec..82cdc123fab 100644 --- a/tls/s2n_protocol_preferences.c +++ b/tls/s2n_protocol_preferences.c @@ -48,7 +48,7 @@ S2N_RESULT s2n_protocol_preferences_contain(struct s2n_blob *protocol_preference struct s2n_blob match_against = { 0 }; RESULT_GUARD(s2n_protocol_preferences_read(&app_protocols_stuffer, &match_against)); - if (match_against.size == protocol->size && memcmp(match_against.data, protocol->data, protocol->size) == 0) { + if (match_against.size == protocol->size && s2n_constant_time_equals(match_against.data, protocol->data, protocol->size) == true) { *contains = true; return S2N_RESULT_OK; } diff --git a/tls/s2n_psk.c b/tls/s2n_psk.c index b02ddc94c44..d5e3d910ec6 100644 --- a/tls/s2n_psk.c +++ b/tls/s2n_psk.c @@ -598,7 +598,7 @@ int s2n_connection_append_psk(struct s2n_connection *conn, struct s2n_psk *input POSIX_ENSURE_REF(existing_psk); bool duplicate = existing_psk->identity.size == input_psk->identity.size - && memcmp(existing_psk->identity.data, input_psk->identity.data, existing_psk->identity.size) == 0; + && s2n_constant_time_equals(existing_psk->identity.data, input_psk->identity.data, existing_psk->identity.size) == true; POSIX_ENSURE(!duplicate, S2N_ERR_DUPLICATE_PSK_IDENTITIES); } diff --git a/tls/s2n_resume.c b/tls/s2n_resume.c index c6e0e32af4d..b9402f98763 100644 --- a/tls/s2n_resume.c +++ b/tls/s2n_resume.c @@ -165,7 +165,7 @@ static int s2n_tls12_deserialize_resumption_state(struct s2n_connection *conn, s S2N_ERROR_IF(protocol_version != conn->actual_protocol_version, S2N_ERR_INVALID_SERIALIZED_SESSION_STATE); POSIX_GUARD(s2n_stuffer_read_bytes(from, cipher_suite, S2N_TLS_CIPHER_SUITE_LEN)); - S2N_ERROR_IF(memcmp(conn->secure->cipher_suite->iana_value, cipher_suite, S2N_TLS_CIPHER_SUITE_LEN), S2N_ERR_INVALID_SERIALIZED_SESSION_STATE); + S2N_ERROR_IF(!s2n_constant_time_equals(conn->secure->cipher_suite->iana_value, cipher_suite, S2N_TLS_CIPHER_SUITE_LEN), S2N_ERR_INVALID_SERIALIZED_SESSION_STATE); uint64_t now = 0; POSIX_GUARD_RESULT(s2n_config_wall_clock(conn->config, &now)); @@ -767,7 +767,7 @@ struct s2n_ticket_key *s2n_find_ticket_key(struct s2n_config *config, const uint for (uint32_t i = 0; i < ticket_keys_len; i++) { PTR_GUARD_RESULT(s2n_set_get(config->ticket_keys, i, (void **) &ticket_key)); - if (memcmp(ticket_key->key_name, name, S2N_TICKET_KEY_NAME_LEN) == 0) { + if (s2n_constant_time_equals(ticket_key->key_name, name, S2N_TICKET_KEY_NAME_LEN) == true) { /* Check to see if the key has expired */ if (now >= ticket_key->intro_timestamp + config->encrypt_decrypt_key_lifetime_in_nanos diff --git a/tls/s2n_security_policies.c b/tls/s2n_security_policies.c index d6a833ab2f8..d8af43e7b64 100644 --- a/tls/s2n_security_policies.c +++ b/tls/s2n_security_policies.c @@ -1487,7 +1487,7 @@ int s2n_connection_is_valid_for_cipher_preferences(struct s2n_connection *conn, struct s2n_cipher_suite *cipher = conn->secure->cipher_suite; POSIX_ENSURE_REF(cipher); for (int i = 0; i < security_policy->cipher_preferences->count; ++i) { - if (0 == memcmp(security_policy->cipher_preferences->suites[i]->iana_value, cipher->iana_value, S2N_TLS_CIPHER_SUITE_LEN)) { + if (true == s2n_constant_time_equals(security_policy->cipher_preferences->suites[i]->iana_value, cipher->iana_value, S2N_TLS_CIPHER_SUITE_LEN)) { return 1; } } diff --git a/tls/s2n_server_hello.c b/tls/s2n_server_hello.c index 0524213f6e4..d18bdc9d9da 100644 --- a/tls/s2n_server_hello.c +++ b/tls/s2n_server_hello.c @@ -51,7 +51,7 @@ static int s2n_random_value_is_hello_retry(struct s2n_connection *conn) { POSIX_ENSURE_REF(conn); - POSIX_ENSURE(memcmp(hello_retry_req_random, conn->handshake_params.server_random, S2N_TLS_RANDOM_DATA_LEN) == 0, + POSIX_ENSURE(s2n_constant_time_equals(hello_retry_req_random, conn->handshake_params.server_random, S2N_TLS_RANDOM_DATA_LEN) == true, S2N_ERR_INVALID_HELLO_RETRY); return S2N_SUCCESS; @@ -157,7 +157,7 @@ static int s2n_server_hello_parse(struct s2n_connection *conn) S2N_ERROR_IF(compression_method != S2N_TLS_COMPRESSION_METHOD_NULL, S2N_ERR_BAD_MESSAGE); bool session_ids_match = session_id_len != 0 && session_id_len == conn->session_id_len - && memcmp(session_id, conn->session_id, session_id_len) == 0; + && s2n_constant_time_equals(session_id, conn->session_id, session_id_len) == true; if (!session_ids_match) { conn->ems_negotiated = false; } @@ -234,7 +234,7 @@ static int s2n_server_hello_parse(struct s2n_connection *conn) if (session_ids_match) { /* check if the resumed session state is valid */ POSIX_ENSURE(conn->resume_protocol_version == conn->actual_protocol_version, S2N_ERR_BAD_MESSAGE); - POSIX_ENSURE(memcmp(conn->secure->cipher_suite->iana_value, cipher_suite_wire, S2N_TLS_CIPHER_SUITE_LEN) == 0, + POSIX_ENSURE(s2n_constant_time_equals(conn->secure->cipher_suite->iana_value, cipher_suite_wire, S2N_TLS_CIPHER_SUITE_LEN) == true, S2N_ERR_BAD_MESSAGE); /* Session is resumed */ diff --git a/utils/s2n_map.c b/utils/s2n_map.c index 93574609fac..5214a9a8b95 100644 --- a/utils/s2n_map.c +++ b/utils/s2n_map.c @@ -121,7 +121,7 @@ S2N_RESULT s2n_map_add(struct s2n_map *map, struct s2n_blob *key, struct s2n_blo /* Linear probing until we find an empty slot */ while (map->table[slot].key.size) { - if (key->size != map->table[slot].key.size || memcmp(key->data, map->table[slot].key.data, key->size)) { + if (key->size != map->table[slot].key.size || !s2n_constant_time_equals(key->data, map->table[slot].key.data, key->size)) { slot++; slot %= map->capacity; continue; @@ -153,7 +153,7 @@ S2N_RESULT s2n_map_put(struct s2n_map *map, struct s2n_blob *key, struct s2n_blo /* Linear probing until we find an empty slot */ while (map->table[slot].key.size) { - if (key->size != map->table[slot].key.size || memcmp(key->data, map->table[slot].key.data, key->size)) { + if (key->size != map->table[slot].key.size || !s2n_constant_time_equals(key->data, map->table[slot].key.data, key->size)) { slot++; slot %= map->capacity; continue; @@ -199,7 +199,7 @@ S2N_RESULT s2n_map_lookup(const struct s2n_map *map, struct s2n_blob *key, struc const uint32_t initial_slot = slot; while (map->table[slot].key.size) { - if (key->size != map->table[slot].key.size || memcmp(key->data, map->table[slot].key.data, key->size)) { + if (key->size != map->table[slot].key.size || !s2n_constant_time_equals(key->data, map->table[slot].key.data, key->size)) { slot++; slot %= map->capacity; /* We went over all the slots but found no match */ From 571f2ba382227eacefd2b2066637225246062d24 Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Thu, 15 Aug 2024 16:52:37 +0000 Subject: [PATCH 2/9] refactor: modify conditional statments for s2n_constant_time_equals --- tls/s2n_cipher_suites.c | 2 +- tls/s2n_connection.c | 5 ++--- tls/s2n_kem.c | 2 +- tls/s2n_protocol_preferences.c | 2 +- tls/s2n_psk.c | 2 +- tls/s2n_resume.c | 2 +- tls/s2n_security_policies.c | 2 +- tls/s2n_server_hello.c | 6 +++--- 8 files changed, 11 insertions(+), 12 deletions(-) diff --git a/tls/s2n_cipher_suites.c b/tls/s2n_cipher_suites.c index e12ed47c11b..abd0c8efa51 100644 --- a/tls/s2n_cipher_suites.c +++ b/tls/s2n_cipher_suites.c @@ -1151,7 +1151,7 @@ int s2n_set_cipher_as_client(struct s2n_connection *conn, uint8_t wire[S2N_TLS_C struct s2n_cipher_suite *cipher_suite = NULL; for (size_t i = 0; i < security_policy->cipher_preferences->count; i++) { const uint8_t *ours = security_policy->cipher_preferences->suites[i]->iana_value; - if (s2n_constant_time_equals(wire, ours, S2N_TLS_CIPHER_SUITE_LEN) == true) { + if (s2n_constant_time_equals(wire, ours, S2N_TLS_CIPHER_SUITE_LEN)) { cipher_suite = security_policy->cipher_preferences->suites[i]; break; } diff --git a/tls/s2n_connection.c b/tls/s2n_connection.c index 22251a6847c..aadb584d350 100644 --- a/tls/s2n_connection.c +++ b/tls/s2n_connection.c @@ -923,9 +923,8 @@ int s2n_connection_get_cipher_iana_value(struct s2n_connection *conn, uint8_t *f /* ensure we've negotiated a cipher suite */ POSIX_ENSURE(!s2n_constant_time_equals(conn->secure->cipher_suite->iana_value, - s2n_null_cipher_suite.iana_value, sizeof(s2n_null_cipher_suite.iana_value)), - S2N_ERR_INVALID_STATE); - + s2n_null_cipher_suite.iana_value, sizeof(s2n_null_cipher_suite.iana_value)), + S2N_ERR_INVALID_STATE); const uint8_t *iana_value = conn->secure->cipher_suite->iana_value; *first = iana_value[0]; *second = iana_value[1]; diff --git a/tls/s2n_kem.c b/tls/s2n_kem.c index cc8952a6327..5d7c38363cc 100644 --- a/tls/s2n_kem.c +++ b/tls/s2n_kem.c @@ -296,7 +296,7 @@ int s2n_cipher_suite_to_kem(const uint8_t iana_value[S2N_TLS_CIPHER_SUITE_LEN], { for (size_t i = 0; i < s2n_array_len(kem_mapping); i++) { const struct s2n_iana_to_kem *candidate = &kem_mapping[i]; - if (s2n_constant_time_equals(iana_value, candidate->iana_value, S2N_TLS_CIPHER_SUITE_LEN) == true) { + if (s2n_constant_time_equals(iana_value, candidate->iana_value, S2N_TLS_CIPHER_SUITE_LEN)) { *compatible_params = candidate; return S2N_SUCCESS; } diff --git a/tls/s2n_protocol_preferences.c b/tls/s2n_protocol_preferences.c index 82cdc123fab..044cf966674 100644 --- a/tls/s2n_protocol_preferences.c +++ b/tls/s2n_protocol_preferences.c @@ -48,7 +48,7 @@ S2N_RESULT s2n_protocol_preferences_contain(struct s2n_blob *protocol_preference struct s2n_blob match_against = { 0 }; RESULT_GUARD(s2n_protocol_preferences_read(&app_protocols_stuffer, &match_against)); - if (match_against.size == protocol->size && s2n_constant_time_equals(match_against.data, protocol->data, protocol->size) == true) { + if (match_against.size == protocol->size && s2n_constant_time_equals(match_against.data, protocol->data, protocol->size)) { *contains = true; return S2N_RESULT_OK; } diff --git a/tls/s2n_psk.c b/tls/s2n_psk.c index d5e3d910ec6..e1579e80cdd 100644 --- a/tls/s2n_psk.c +++ b/tls/s2n_psk.c @@ -598,7 +598,7 @@ int s2n_connection_append_psk(struct s2n_connection *conn, struct s2n_psk *input POSIX_ENSURE_REF(existing_psk); bool duplicate = existing_psk->identity.size == input_psk->identity.size - && s2n_constant_time_equals(existing_psk->identity.data, input_psk->identity.data, existing_psk->identity.size) == true; + && s2n_constant_time_equals(existing_psk->identity.data, input_psk->identity.data, existing_psk->identity.size); POSIX_ENSURE(!duplicate, S2N_ERR_DUPLICATE_PSK_IDENTITIES); } diff --git a/tls/s2n_resume.c b/tls/s2n_resume.c index b9402f98763..e37a8056336 100644 --- a/tls/s2n_resume.c +++ b/tls/s2n_resume.c @@ -767,7 +767,7 @@ struct s2n_ticket_key *s2n_find_ticket_key(struct s2n_config *config, const uint for (uint32_t i = 0; i < ticket_keys_len; i++) { PTR_GUARD_RESULT(s2n_set_get(config->ticket_keys, i, (void **) &ticket_key)); - if (s2n_constant_time_equals(ticket_key->key_name, name, S2N_TICKET_KEY_NAME_LEN) == true) { + if (s2n_constant_time_equals(ticket_key->key_name, name, S2N_TICKET_KEY_NAME_LEN)) { /* Check to see if the key has expired */ if (now >= ticket_key->intro_timestamp + config->encrypt_decrypt_key_lifetime_in_nanos diff --git a/tls/s2n_security_policies.c b/tls/s2n_security_policies.c index d8af43e7b64..12a801e00f1 100644 --- a/tls/s2n_security_policies.c +++ b/tls/s2n_security_policies.c @@ -1487,7 +1487,7 @@ int s2n_connection_is_valid_for_cipher_preferences(struct s2n_connection *conn, struct s2n_cipher_suite *cipher = conn->secure->cipher_suite; POSIX_ENSURE_REF(cipher); for (int i = 0; i < security_policy->cipher_preferences->count; ++i) { - if (true == s2n_constant_time_equals(security_policy->cipher_preferences->suites[i]->iana_value, cipher->iana_value, S2N_TLS_CIPHER_SUITE_LEN)) { + if (s2n_constant_time_equals(security_policy->cipher_preferences->suites[i]->iana_value, cipher->iana_value, S2N_TLS_CIPHER_SUITE_LEN)) { return 1; } } diff --git a/tls/s2n_server_hello.c b/tls/s2n_server_hello.c index d18bdc9d9da..d08999e4bea 100644 --- a/tls/s2n_server_hello.c +++ b/tls/s2n_server_hello.c @@ -51,7 +51,7 @@ static int s2n_random_value_is_hello_retry(struct s2n_connection *conn) { POSIX_ENSURE_REF(conn); - POSIX_ENSURE(s2n_constant_time_equals(hello_retry_req_random, conn->handshake_params.server_random, S2N_TLS_RANDOM_DATA_LEN) == true, + POSIX_ENSURE(s2n_constant_time_equals(hello_retry_req_random, conn->handshake_params.server_random, S2N_TLS_RANDOM_DATA_LEN), S2N_ERR_INVALID_HELLO_RETRY); return S2N_SUCCESS; @@ -157,7 +157,7 @@ static int s2n_server_hello_parse(struct s2n_connection *conn) S2N_ERROR_IF(compression_method != S2N_TLS_COMPRESSION_METHOD_NULL, S2N_ERR_BAD_MESSAGE); bool session_ids_match = session_id_len != 0 && session_id_len == conn->session_id_len - && s2n_constant_time_equals(session_id, conn->session_id, session_id_len) == true; + && s2n_constant_time_equals(session_id, conn->session_id, session_id_len); if (!session_ids_match) { conn->ems_negotiated = false; } @@ -234,7 +234,7 @@ static int s2n_server_hello_parse(struct s2n_connection *conn) if (session_ids_match) { /* check if the resumed session state is valid */ POSIX_ENSURE(conn->resume_protocol_version == conn->actual_protocol_version, S2N_ERR_BAD_MESSAGE); - POSIX_ENSURE(s2n_constant_time_equals(conn->secure->cipher_suite->iana_value, cipher_suite_wire, S2N_TLS_CIPHER_SUITE_LEN) == true, + POSIX_ENSURE(s2n_constant_time_equals(conn->secure->cipher_suite->iana_value, cipher_suite_wire, S2N_TLS_CIPHER_SUITE_LEN), S2N_ERR_BAD_MESSAGE); /* Session is resumed */ From ac6f580f45fe2e930cd8a374ff18a5c7b1f90fa2 Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Thu, 15 Aug 2024 22:17:13 +0000 Subject: [PATCH 3/9] refactor: respond to PR comments and attempt to fix CI problem --- codebuild/bin/grep_simple_mistakes.sh | 13 +------------ crypto/s2n_rsa.c | 2 +- stuffer/s2n_stuffer_text.c | 2 +- tls/s2n_connection.c | 5 +++-- tls/s2n_early_data.c | 2 +- 5 files changed, 7 insertions(+), 17 deletions(-) diff --git a/codebuild/bin/grep_simple_mistakes.sh b/codebuild/bin/grep_simple_mistakes.sh index 5425bcada3b..6b7d6bb767d 100755 --- a/codebuild/bin/grep_simple_mistakes.sh +++ b/codebuild/bin/grep_simple_mistakes.sh @@ -36,19 +36,8 @@ done ############################################# S2N_FILES_ASSERT_NOT_USING_MEMCMP=$(find "$PWD" -type f -name "s2n*.[ch]" -not -path "*/tests/*" -not -path "*/bindings/*") declare -A KNOWN_MEMCMP_USAGE -KNOWN_MEMCMP_USAGE["$PWD/crypto/s2n_rsa.c"]=1 -KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_early_data.c"]=1 -KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_kem.c"]=1 -KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_cipher_suites.c"]=3 -KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_server_hello.c"]=3 -KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_security_policies.c"]=1 -KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_psk.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/tls/s2n_resume.c"]=2 -KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_connection.c"]=1 -KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_protocol_preferences.c"]=1 -KNOWN_MEMCMP_USAGE["$PWD/utils/s2n_map.c"]=3 -KNOWN_MEMCMP_USAGE["$PWD/stuffer/s2n_stuffer_text.c"]=1 for file in $S2N_FILES_ASSERT_NOT_USING_MEMCMP; do # NOTE: this matches on 'memcmp', which will also match comments. However, there diff --git a/crypto/s2n_rsa.c b/crypto/s2n_rsa.c index 806c8b866f2..a89b59612b7 100644 --- a/crypto/s2n_rsa.c +++ b/crypto/s2n_rsa.c @@ -168,7 +168,7 @@ static int s2n_rsa_keys_match(const struct s2n_pkey *pub, const struct s2n_pkey plain_out.size = sizeof(plain_outpad); POSIX_GUARD(s2n_rsa_decrypt(priv, &enc, &plain_out)); - S2N_ERROR_IF(!s2n_constant_time_equals(plain_in.data, plain_out.data, plain_in.size), S2N_ERR_KEY_MISMATCH); + POSIX_ENSURE(s2n_constant_time_equals(plain_in.data, plain_out.data, plain_in.size), S2N_ERR_KEY_MISMATCH); return 0; } diff --git a/stuffer/s2n_stuffer_text.c b/stuffer/s2n_stuffer_text.c index 71ffdfce868..64d86106898 100644 --- a/stuffer/s2n_stuffer_text.c +++ b/stuffer/s2n_stuffer_text.c @@ -72,7 +72,7 @@ int s2n_stuffer_read_expected_str(struct s2n_stuffer *stuffer, const char *expec POSIX_ENSURE(s2n_stuffer_data_available(stuffer) >= expected_length, S2N_ERR_STUFFER_OUT_OF_DATA); uint8_t *actual = stuffer->blob.data + stuffer->read_cursor; POSIX_ENSURE_REF(actual); - POSIX_ENSURE(s2n_constant_time_equals(actual, (uint8_t *) expected, expected_length), S2N_ERR_STUFFER_NOT_FOUND); + POSIX_ENSURE(s2n_constant_time_equals(actual, (const uint8_t *) expected, expected_length), S2N_ERR_STUFFER_NOT_FOUND); stuffer->read_cursor += expected_length; POSIX_POSTCONDITION(s2n_stuffer_validate(stuffer)); return S2N_SUCCESS; diff --git a/tls/s2n_connection.c b/tls/s2n_connection.c index aadb584d350..22251a6847c 100644 --- a/tls/s2n_connection.c +++ b/tls/s2n_connection.c @@ -923,8 +923,9 @@ int s2n_connection_get_cipher_iana_value(struct s2n_connection *conn, uint8_t *f /* ensure we've negotiated a cipher suite */ POSIX_ENSURE(!s2n_constant_time_equals(conn->secure->cipher_suite->iana_value, - s2n_null_cipher_suite.iana_value, sizeof(s2n_null_cipher_suite.iana_value)), - S2N_ERR_INVALID_STATE); + s2n_null_cipher_suite.iana_value, sizeof(s2n_null_cipher_suite.iana_value)), + S2N_ERR_INVALID_STATE); + const uint8_t *iana_value = conn->secure->cipher_suite->iana_value; *first = iana_value[0]; *second = iana_value[1]; diff --git a/tls/s2n_early_data.c b/tls/s2n_early_data.c index 6e2cd36a933..bb61331e712 100644 --- a/tls/s2n_early_data.c +++ b/tls/s2n_early_data.c @@ -96,7 +96,7 @@ static S2N_RESULT s2n_early_data_validate(struct s2n_connection *conn) const size_t app_protocol_size = strlen(conn->application_protocol); if (app_protocol_size > 0 || config->application_protocol.size > 0) { RESULT_ENSURE_EQ(config->application_protocol.size, app_protocol_size + 1 /* null-terminating char */); - RESULT_ENSURE_EQ(s2n_constant_time_equals(config->application_protocol.data, (uint8_t *) conn->application_protocol, app_protocol_size), true); + RESULT_ENSURE(s2n_constant_time_equals(config->application_protocol.data, (uint8_t *) conn->application_protocol, app_protocol_size), S2N_ERR_SAFETY); } return S2N_RESULT_OK; From 359e6899ce4022770c6560bf4cf6d14d260954e4 Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Thu, 15 Aug 2024 23:36:20 +0000 Subject: [PATCH 4/9] Address PR and CI concerns * Let s2n_stuffer_read_expected_str use memcmp to avoid CBMC problem. --- stuffer/s2n_stuffer_text.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stuffer/s2n_stuffer_text.c b/stuffer/s2n_stuffer_text.c index 64d86106898..88f6b96b300 100644 --- a/stuffer/s2n_stuffer_text.c +++ b/stuffer/s2n_stuffer_text.c @@ -72,7 +72,7 @@ int s2n_stuffer_read_expected_str(struct s2n_stuffer *stuffer, const char *expec POSIX_ENSURE(s2n_stuffer_data_available(stuffer) >= expected_length, S2N_ERR_STUFFER_OUT_OF_DATA); uint8_t *actual = stuffer->blob.data + stuffer->read_cursor; POSIX_ENSURE_REF(actual); - POSIX_ENSURE(s2n_constant_time_equals(actual, (const uint8_t *) expected, expected_length), S2N_ERR_STUFFER_NOT_FOUND); + POSIX_ENSURE(!memcmp(actual, expected, expected_length), S2N_ERR_STUFFER_NOT_FOUND); stuffer->read_cursor += expected_length; POSIX_POSTCONDITION(s2n_stuffer_validate(stuffer)); return S2N_SUCCESS; From c676feb247d2aa0a067348e3ae22ba6bd526abeb Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Thu, 15 Aug 2024 23:58:53 +0000 Subject: [PATCH 5/9] Address PR and CI concerns: * Adding one more known memcmp location to grep_simple_mistakes.sh. --- codebuild/bin/grep_simple_mistakes.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/codebuild/bin/grep_simple_mistakes.sh b/codebuild/bin/grep_simple_mistakes.sh index 6b7d6bb767d..af514ea373b 100755 --- a/codebuild/bin/grep_simple_mistakes.sh +++ b/codebuild/bin/grep_simple_mistakes.sh @@ -38,6 +38,7 @@ S2N_FILES_ASSERT_NOT_USING_MEMCMP=$(find "$PWD" -type f -name "s2n*.[ch]" -not - declare -A KNOWN_MEMCMP_USAGE KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_cipher_suites.c"]=1 KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_config.c"]=1 +KNOWN_MEMCMP_USAGE["$PWD/stuffer/s2n_stuffer_text.c"]=1 for file in $S2N_FILES_ASSERT_NOT_USING_MEMCMP; do # NOTE: this matches on 'memcmp', which will also match comments. However, there From e7049af77e60134002b4d4e5979b95c1df95ad1a Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Fri, 16 Aug 2024 00:32:15 +0000 Subject: [PATCH 6/9] Address CR comments: * Replace S2N_ERROR_IF to POSIX_ENSURE. --- tls/s2n_resume.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tls/s2n_resume.c b/tls/s2n_resume.c index e37a8056336..abca18fcc10 100644 --- a/tls/s2n_resume.c +++ b/tls/s2n_resume.c @@ -165,7 +165,7 @@ static int s2n_tls12_deserialize_resumption_state(struct s2n_connection *conn, s S2N_ERROR_IF(protocol_version != conn->actual_protocol_version, S2N_ERR_INVALID_SERIALIZED_SESSION_STATE); POSIX_GUARD(s2n_stuffer_read_bytes(from, cipher_suite, S2N_TLS_CIPHER_SUITE_LEN)); - S2N_ERROR_IF(!s2n_constant_time_equals(conn->secure->cipher_suite->iana_value, cipher_suite, S2N_TLS_CIPHER_SUITE_LEN), S2N_ERR_INVALID_SERIALIZED_SESSION_STATE); + POSIX_ENSURE(s2n_constant_time_equals(conn->secure->cipher_suite->iana_value, cipher_suite, S2N_TLS_CIPHER_SUITE_LEN), S2N_ERR_INVALID_SERIALIZED_SESSION_STATE); uint64_t now = 0; POSIX_GUARD_RESULT(s2n_config_wall_clock(conn->config, &now)); From 12295ca5433c9d1fbacbd364777085aa5c2c482d Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Fri, 16 Aug 2024 15:52:47 +0000 Subject: [PATCH 7/9] Address CR concern: * Reorder the .sh order to fit the directory order. --- codebuild/bin/grep_simple_mistakes.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codebuild/bin/grep_simple_mistakes.sh b/codebuild/bin/grep_simple_mistakes.sh index af514ea373b..15ce596e2f4 100755 --- a/codebuild/bin/grep_simple_mistakes.sh +++ b/codebuild/bin/grep_simple_mistakes.sh @@ -36,9 +36,9 @@ done ############################################# S2N_FILES_ASSERT_NOT_USING_MEMCMP=$(find "$PWD" -type f -name "s2n*.[ch]" -not -path "*/tests/*" -not -path "*/bindings/*") declare -A KNOWN_MEMCMP_USAGE +KNOWN_MEMCMP_USAGE["$PWD/stuffer/s2n_stuffer_text.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/stuffer/s2n_stuffer_text.c"]=1 for file in $S2N_FILES_ASSERT_NOT_USING_MEMCMP; do # NOTE: this matches on 'memcmp', which will also match comments. However, there From 0feaa68218befd5ce6cd2f0f8674b989d9a80654 Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Tue, 20 Aug 2024 16:50:14 +0000 Subject: [PATCH 8/9] Remove cases with possibility of pathological inputs Remove the usage of s2n_constant_time_equals from any functions where it is possible to compare relatively large amounts of data (~ > 1 kB) even if that scenario is unlikely. --- tls/s2n_protocol_preferences.c | 2 +- tls/s2n_psk.c | 2 +- utils/s2n_map.c | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tls/s2n_protocol_preferences.c b/tls/s2n_protocol_preferences.c index 044cf966674..cf5674e6fec 100644 --- a/tls/s2n_protocol_preferences.c +++ b/tls/s2n_protocol_preferences.c @@ -48,7 +48,7 @@ S2N_RESULT s2n_protocol_preferences_contain(struct s2n_blob *protocol_preference struct s2n_blob match_against = { 0 }; RESULT_GUARD(s2n_protocol_preferences_read(&app_protocols_stuffer, &match_against)); - if (match_against.size == protocol->size && s2n_constant_time_equals(match_against.data, protocol->data, protocol->size)) { + if (match_against.size == protocol->size && memcmp(match_against.data, protocol->data, protocol->size) == 0) { *contains = true; return S2N_RESULT_OK; } diff --git a/tls/s2n_psk.c b/tls/s2n_psk.c index e1579e80cdd..b02ddc94c44 100644 --- a/tls/s2n_psk.c +++ b/tls/s2n_psk.c @@ -598,7 +598,7 @@ int s2n_connection_append_psk(struct s2n_connection *conn, struct s2n_psk *input POSIX_ENSURE_REF(existing_psk); bool duplicate = existing_psk->identity.size == input_psk->identity.size - && s2n_constant_time_equals(existing_psk->identity.data, input_psk->identity.data, existing_psk->identity.size); + && memcmp(existing_psk->identity.data, input_psk->identity.data, existing_psk->identity.size) == 0; POSIX_ENSURE(!duplicate, S2N_ERR_DUPLICATE_PSK_IDENTITIES); } diff --git a/utils/s2n_map.c b/utils/s2n_map.c index 5214a9a8b95..93574609fac 100644 --- a/utils/s2n_map.c +++ b/utils/s2n_map.c @@ -121,7 +121,7 @@ S2N_RESULT s2n_map_add(struct s2n_map *map, struct s2n_blob *key, struct s2n_blo /* Linear probing until we find an empty slot */ while (map->table[slot].key.size) { - if (key->size != map->table[slot].key.size || !s2n_constant_time_equals(key->data, map->table[slot].key.data, key->size)) { + if (key->size != map->table[slot].key.size || memcmp(key->data, map->table[slot].key.data, key->size)) { slot++; slot %= map->capacity; continue; @@ -153,7 +153,7 @@ S2N_RESULT s2n_map_put(struct s2n_map *map, struct s2n_blob *key, struct s2n_blo /* Linear probing until we find an empty slot */ while (map->table[slot].key.size) { - if (key->size != map->table[slot].key.size || !s2n_constant_time_equals(key->data, map->table[slot].key.data, key->size)) { + if (key->size != map->table[slot].key.size || memcmp(key->data, map->table[slot].key.data, key->size)) { slot++; slot %= map->capacity; continue; @@ -199,7 +199,7 @@ S2N_RESULT s2n_map_lookup(const struct s2n_map *map, struct s2n_blob *key, struc const uint32_t initial_slot = slot; while (map->table[slot].key.size) { - if (key->size != map->table[slot].key.size || !s2n_constant_time_equals(key->data, map->table[slot].key.data, key->size)) { + if (key->size != map->table[slot].key.size || memcmp(key->data, map->table[slot].key.data, key->size)) { slot++; slot %= map->capacity; /* We went over all the slots but found no match */ From 19f1016bef21742b2996d323235fc7982586498c Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Tue, 20 Aug 2024 17:30:47 +0000 Subject: [PATCH 9/9] fix: fix grep_simple_mistakes.sh to include additional memcmp calls --- codebuild/bin/grep_simple_mistakes.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/codebuild/bin/grep_simple_mistakes.sh b/codebuild/bin/grep_simple_mistakes.sh index 15ce596e2f4..82efcbf5f53 100755 --- a/codebuild/bin/grep_simple_mistakes.sh +++ b/codebuild/bin/grep_simple_mistakes.sh @@ -37,8 +37,11 @@ done S2N_FILES_ASSERT_NOT_USING_MEMCMP=$(find "$PWD" -type f -name "s2n*.[ch]" -not -path "*/tests/*" -not -path "*/bindings/*") declare -A KNOWN_MEMCMP_USAGE 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 # NOTE: this matches on 'memcmp', which will also match comments. However, there