refactor: replace memcmp to s2n_constant_time_equals (#4709)

aws · Sep 5, 2024 · 08d413a · 08d413a
1 parent 9964ee7
commit 08d413a
Show file tree

Hide file tree

Showing 9 changed files with 16 additions and 24 deletions.
diff --git a/codebuild/bin/grep_simple_mistakes.sh b/codebuild/bin/grep_simple_mistakes.sh
@@ -36,19 +36,12 @@ 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/stuffer/s2n_stuffer_text.c"]=1
 KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_psk.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/tls/s2n_cipher_suites.c"]=1
+KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_config.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
@@ -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);
+    POSIX_ENSURE(s2n_constant_time_equals(plain_in.data, plain_out.data, plain_in.size), S2N_ERR_KEY_MISMATCH);
 
     return 0;
 }

diff --git 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)) {
             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
@@ -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
@@ -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(s2n_constant_time_equals(config->application_protocol.data, (uint8_t *) conn->application_protocol, app_protocol_size), S2N_ERR_SAFETY);
     }
 
     return S2N_RESULT_OK;

diff --git 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)) {
             *compatible_params = candidate;
             return S2N_SUCCESS;
         }

diff --git 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);
+    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));
@@ -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)) {
             /* 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
@@ -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 (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
@@ -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),
             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);
     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),
                     S2N_ERR_BAD_MESSAGE);
 
             /* Session is resumed */