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.

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

Replace memcmp to s2n_constant_time_equals #4709

Merged
merged 11 commits into from
Sep 5, 2024
14 changes: 2 additions & 12 deletions codebuild/bin/grep_simple_mistakes.sh
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +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/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_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
KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_cipher_suites.c"]=1
KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_config.c"]=1

for file in $S2N_FILES_ASSERT_NOT_USING_MEMCMP; do
# NOTE: this matches on 'memcmp', which will also match comments. However, there
Expand Down
2 changes: 1 addition & 1 deletion crypto/s2n_rsa.c
boquan-fang marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
4 changes: 2 additions & 2 deletions tls/s2n_cipher_suites.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
}
Expand Down
5 changes: 2 additions & 3 deletions tls/s2n_connection.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);

boquan-fang marked this conversation as resolved.
Show resolved Hide resolved
const uint8_t *iana_value = conn->secure->cipher_suite->iana_value;
Expand Down
2 changes: 1 addition & 1 deletion tls/s2n_early_data.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion tls/s2n_kem.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion tls/s2n_protocol_preferences.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
*contains = true;
return S2N_RESULT_OK;
}
Expand Down
2 changes: 1 addition & 1 deletion tls/s2n_psk.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
POSIX_ENSURE(!duplicate, S2N_ERR_DUPLICATE_PSK_IDENTITIES);
}

Expand Down
4 changes: 2 additions & 2 deletions tls/s2n_resume.c
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tls/s2n_security_policies.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
6 changes: 3 additions & 3 deletions tls/s2n_server_hello.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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 */
Expand Down
6 changes: 3 additions & 3 deletions utils/s2n_map.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 */
Expand Down
Loading