From f227c7dcd0cc8ab3b7eb5a7a44bdbebd62b5cf59 Mon Sep 17 00:00:00 2001 From: Alex Weibel Date: Wed, 24 Apr 2024 15:34:25 -0700 Subject: [PATCH 1/5] Perform 2-RTT Handshake to upgrade to PQ when possible --- ..._extensions_server_key_share_select_test.c | 14 ++-- tests/unit/s2n_tls13_pq_handshake_test.c | 78 +++++++++++++++---- tls/extensions/s2n_client_key_share.c | 43 +++++----- tls/extensions/s2n_client_supported_groups.c | 2 - tls/extensions/s2n_server_key_share.c | 29 +++++-- tls/s2n_server_hello_retry.c | 13 ++-- 6 files changed, 124 insertions(+), 55 deletions(-) diff --git a/tests/unit/s2n_extensions_server_key_share_select_test.c b/tests/unit/s2n_extensions_server_key_share_select_test.c index efd707c1c5b..08aa1439b10 100644 --- a/tests/unit/s2n_extensions_server_key_share_select_test.c +++ b/tests/unit/s2n_extensions_server_key_share_select_test.c @@ -186,6 +186,7 @@ int main() EXPECT_EQUAL(server_params->kem_group, kem_group1); EXPECT_EQUAL(server_params->kem_params.kem, kem_group1->kem); EXPECT_EQUAL(server_params->ecc_params.negotiated_curve, kem_group1->curve); + EXPECT_NULL(server_conn->kex_params.client_kem_group_params.kem_group); EXPECT_NULL(server_conn->kex_params.server_ecc_evp_params.negotiated_curve); EXPECT_TRUE(s2n_is_hello_retry_handshake(server_conn)); @@ -383,8 +384,7 @@ int main() EXPECT_SUCCESS(s2n_connection_free(server_conn)); }; - /* When client sent valid keyshares - * only for ECC, server should choose curves[0] and not send HRR. */ + /* When client sent KeyShares only for ECC but also supports PQ, server should choose PQ and send HRR. */ { struct s2n_connection *server_conn = NULL; EXPECT_NOT_NULL(server_conn = s2n_connection_new(S2N_SERVER)); @@ -423,12 +423,12 @@ int main() EXPECT_SUCCESS(s2n_extensions_server_key_share_select(server_conn)); /* Server should update its choice to curve[0], no HRR */ - EXPECT_EQUAL(server_conn->kex_params.server_ecc_evp_params.negotiated_curve, ecc_pref->ecc_curves[0]); - EXPECT_NULL(server_params->kem_group); - EXPECT_NULL(server_params->kem_params.kem); - EXPECT_NULL(server_params->ecc_params.negotiated_curve); + EXPECT_NULL(server_conn->kex_params.server_ecc_evp_params.negotiated_curve); + EXPECT_EQUAL(server_params->kem_group, kem_group0); + EXPECT_EQUAL(server_params->kem_params.kem, kem_group0->kem); + EXPECT_EQUAL(server_params->ecc_params.negotiated_curve, kem_group0->curve); EXPECT_NULL(server_conn->kex_params.client_kem_group_params.kem_group); - EXPECT_FALSE(s2n_is_hello_retry_handshake(server_conn)); + EXPECT_TRUE(s2n_is_hello_retry_handshake(server_conn)); EXPECT_SUCCESS(s2n_connection_free(server_conn)); }; diff --git a/tests/unit/s2n_tls13_pq_handshake_test.c b/tests/unit/s2n_tls13_pq_handshake_test.c index 74f26f0c288..27959a33678 100644 --- a/tests/unit/s2n_tls13_pq_handshake_test.c +++ b/tests/unit/s2n_tls13_pq_handshake_test.c @@ -26,17 +26,37 @@ /* Include C file directly to access static functions */ #include "tls/s2n_handshake_io.c" -const struct s2n_kem_group *s2n_get_highest_priority_shared_kem_group(const struct s2n_kem_preferences *client_prefs, const struct s2n_kem_preferences *server_prefs) +const struct s2n_kem_group *s2n_get_predicted_negotiated_kem_group(const struct s2n_kem_preferences *client_prefs, const struct s2n_kem_preferences *server_prefs) { PTR_ENSURE_REF(client_prefs); PTR_ENSURE_REF(server_prefs); - for (int i = 0; i < client_prefs->tls13_kem_group_count; i++) { - for (int j = 0; j < server_prefs->tls13_kem_group_count; j++) { - const struct s2n_kem_group *client_group = client_prefs->tls13_kem_groups[i]; - const struct s2n_kem_group *server_group = server_prefs->tls13_kem_groups[j]; + + /* Client will offer their highest priority PQ KeyShare in their ClientHello. This PQ KeyShare will be most preferred + * since it can be negotiated in 1-RTT (even if there are other mutually supported PQ KeyShares that the server would + * prefer over this one but would require 2-RTT's). */ + const struct s2n_kem_group *client_default = client_prefs->tls13_kem_groups[0]; + for (int i = 0; i < server_prefs->tls13_kem_group_count; i++) { + const struct s2n_kem_group *server_group = server_prefs->tls13_kem_groups[i]; + PTR_ENSURE_REF(client_default); + PTR_ENSURE_REF(server_group); + if (s2n_kem_group_is_available(client_default) && s2n_kem_group_is_available(server_group) + && client_default->iana_id == server_group->iana_id + && s2n_kem_group_is_available(client_default) == s2n_kem_group_is_available(server_group)) { + return client_default; + } + } + + /* Otherwise, if the client's default isn't supported, and a 2-RTT PQ handshake is required, the server will choose + * whichever mutually supported PQ KeyShare that is highest on the server's preference list. */ + for (int i = 0; i < server_prefs->tls13_kem_group_count; i++) { + const struct s2n_kem_group *server_group = server_prefs->tls13_kem_groups[i]; + + for (int j = 0; j < client_prefs->tls13_kem_group_count; j++) { + const struct s2n_kem_group *client_group = client_prefs->tls13_kem_groups[j]; PTR_ENSURE_REF(client_group); PTR_ENSURE_REF(server_group); if (s2n_kem_group_is_available(client_group) && s2n_kem_group_is_available(server_group) + && client_group->iana_id == server_group->iana_id && s2n_kem_group_is_available(client_group) == s2n_kem_group_is_available(server_group)) { return client_group; } @@ -46,13 +66,18 @@ const struct s2n_kem_group *s2n_get_highest_priority_shared_kem_group(const stru } int s2n_test_tls13_pq_handshake(const struct s2n_security_policy *client_sec_policy, - const struct s2n_security_policy *server_sec_policy, + const struct s2n_security_policy *server_sec_policy, const struct s2n_kem_group *expected_kem_group, const struct s2n_ecc_named_curve *expected_curve, bool hrr_expected, bool len_prefix_expected) { /* XOR check: can expect to negotiate either a KEM group, or a classic EC curve, but not both/neither */ - const struct s2n_kem_group *expected_kem_group = s2n_get_highest_priority_shared_kem_group(client_sec_policy->kem_preferences, server_sec_policy->kem_preferences); POSIX_ENSURE((expected_kem_group == NULL) != (expected_curve == NULL), S2N_ERR_SAFETY); + if (expected_kem_group != NULL) { + const struct s2n_kem_group *predicted_kem_group = s2n_get_predicted_negotiated_kem_group(client_sec_policy->kem_preferences, server_sec_policy->kem_preferences); + POSIX_ENSURE_REF(predicted_kem_group); + POSIX_ENSURE_EQ(expected_kem_group->iana_id, predicted_kem_group->iana_id); + } + /* Set up connections */ struct s2n_connection *client_conn = NULL, *server_conn = NULL; POSIX_ENSURE_REF(client_conn = s2n_connection_new(S2N_CLIENT)); @@ -118,11 +143,7 @@ int s2n_test_tls13_pq_handshake(const struct s2n_security_policy *client_sec_pol /* Server sends ServerHello or HRR */ POSIX_GUARD(s2n_conn_set_handshake_type(server_conn)); - if (hrr_expected) { - POSIX_ENSURE_EQ(s2n_conn_get_current_message_type(server_conn), HELLO_RETRY_MSG); - } else { - POSIX_ENSURE_EQ(s2n_conn_get_current_message_type(server_conn), SERVER_HELLO); - } + POSIX_ENSURE_EQ(hrr_expected, s2n_handshake_type_check_tls13_flag(server_conn, HELLO_RETRY_REQUEST)); POSIX_GUARD(s2n_handshake_write_io(server_conn)); /* Server sends CCS */ @@ -352,6 +373,7 @@ int main() struct pq_handshake_test_vector { const struct s2n_security_policy *client_policy; const struct s2n_security_policy *server_policy; + const struct s2n_kem_group *expected_kem_group; const struct s2n_ecc_named_curve *expected_curve; bool hrr_expected; bool len_prefix_expected; @@ -361,10 +383,19 @@ int main() * If PQ is disabled, the expected negotiation outcome is overridden below * before performing the handshake test. */ const struct pq_handshake_test_vector test_vectors[] = { + { + .client_policy = &security_policy_pq_tls_1_3_2023_06_01, + .server_policy = &security_policy_pq_tls_1_0_2021_05_24, + .expected_kem_group = &s2n_x25519_kyber_512_r3, + .expected_curve = NULL, + .hrr_expected = true, + .len_prefix_expected = false, + }, /* Server and Client both support PQ and TLS 1.3 */ { .client_policy = &security_policy_pq_tls_1_1_2021_05_21, .server_policy = &security_policy_pq_tls_1_1_2021_05_21, + .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = true, @@ -372,6 +403,7 @@ int main() { .client_policy = &security_policy_pq_tls_1_0_2021_05_22, .server_policy = &security_policy_pq_tls_1_0_2021_05_22, + .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = true, @@ -379,6 +411,7 @@ int main() { .client_policy = &security_policy_pq_tls_1_0_2021_05_23, .server_policy = &security_policy_pq_tls_1_0_2021_05_23, + .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = true, @@ -386,6 +419,7 @@ int main() { .client_policy = &security_policy_pq_tls_1_0_2021_05_24, .server_policy = &security_policy_pq_tls_1_0_2021_05_24, + .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = true, @@ -393,6 +427,7 @@ int main() { .client_policy = &security_policy_pq_tls_1_0_2021_05_26, .server_policy = &security_policy_pq_tls_1_0_2021_05_26, + .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = true, @@ -400,6 +435,7 @@ int main() { .client_policy = &security_policy_pq_tls_1_0_2023_01_24, .server_policy = &security_policy_pq_tls_1_0_2023_01_24, + .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = false, @@ -411,6 +447,7 @@ int main() { .client_policy = &security_policy_pq_tls_1_3_2023_06_01, .server_policy = &security_policy_pq_tls_1_3_2023_06_01, + .expected_kem_group = &s2n_secp256r1_kyber_768_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = false, @@ -418,6 +455,7 @@ int main() { .client_policy = &kyber1024_test_policy, .server_policy = &security_policy_pq_tls_1_3_2023_06_01, + .expected_kem_group = &s2n_secp521r1_kyber_1024_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = false, @@ -425,6 +463,7 @@ int main() { .client_policy = &kyber768_test_policy, .server_policy = &security_policy_pq_tls_1_3_2023_06_01, + .expected_kem_group = &s2n_secp384r1_kyber_768_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = false, @@ -436,6 +475,7 @@ int main() { .client_policy = &security_policy_pq_tls_1_1_2021_05_21, .server_policy = &security_policy_pq_tls_1_3_2023_06_01, + .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, .hrr_expected = !s2n_pq_is_enabled(), .len_prefix_expected = true, @@ -444,6 +484,7 @@ int main() { .client_policy = &kyber_test_policy_draft0, .server_policy = &kyber_test_policy_draft5, + .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = true, @@ -451,6 +492,7 @@ int main() { .client_policy = &kyber_test_policy_draft5, .server_policy = &kyber_test_policy_draft0, + .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = false, @@ -458,6 +500,7 @@ int main() { .client_policy = &security_policy_pq_tls_1_0_2021_05_24, .server_policy = &security_policy_pq_tls_1_0_2023_01_24, + .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = true, @@ -465,6 +508,7 @@ int main() { .client_policy = &security_policy_pq_tls_1_0_2023_01_24, .server_policy = &security_policy_pq_tls_1_0_2021_05_24, + .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = false, @@ -475,6 +519,7 @@ int main() { .client_policy = &security_policy_pq_tls_1_0_2020_12, .server_policy = &security_policy_pq_tls_1_0_2020_12, + .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = true, @@ -486,6 +531,7 @@ int main() { .client_policy = &security_policy_pq_tls_1_0_2020_12, .server_policy = &kyber_test_policy_draft0, + .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = true, @@ -497,6 +543,7 @@ int main() { .client_policy = &security_policy_pq_tls_1_0_2020_12, .server_policy = &kyber_test_policy_draft5, + .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, .hrr_expected = false, .len_prefix_expected = true, @@ -507,6 +554,7 @@ int main() { .client_policy = &security_policy_pq_tls_1_0_2020_12, .server_policy = &security_policy_test_all_tls13, + .expected_kem_group = NULL, .expected_curve = expected_curve, .hrr_expected = false, .len_prefix_expected = true, @@ -517,6 +565,7 @@ int main() { .client_policy = &ecc_retry_policy, .server_policy = &security_policy_test_all_tls13, + .expected_kem_group = NULL, .expected_curve = expected_curve, .hrr_expected = true, .len_prefix_expected = true, @@ -527,6 +576,7 @@ int main() { .client_policy = &security_policy_test_all_tls13, .server_policy = &security_policy_pq_tls_1_0_2020_12, + .expected_kem_group = NULL, .expected_curve = expected_curve, .hrr_expected = false, .len_prefix_expected = true, @@ -537,6 +587,7 @@ int main() { .client_policy = &security_policy_test_tls13_retry, .server_policy = &security_policy_pq_tls_1_0_2020_12, + .expected_kem_group = NULL, .expected_curve = expected_curve, .hrr_expected = true, .len_prefix_expected = true, @@ -547,6 +598,7 @@ int main() const struct pq_handshake_test_vector *vector = &test_vectors[i]; const struct s2n_security_policy *client_policy = vector->client_policy; const struct s2n_security_policy *server_policy = vector->server_policy; + const struct s2n_kem_group *kem_group = vector->expected_kem_group; const struct s2n_ecc_named_curve *curve = vector->expected_curve; bool hrr_expected = vector->hrr_expected; bool len_prefix_expected = vector->len_prefix_expected; @@ -564,7 +616,7 @@ int main() } } - EXPECT_SUCCESS(s2n_test_tls13_pq_handshake(client_policy, server_policy, curve, hrr_expected, len_prefix_expected)); + EXPECT_SUCCESS(s2n_test_tls13_pq_handshake(client_policy, server_policy, kem_group, curve, hrr_expected, len_prefix_expected)); } END_TEST(); diff --git a/tls/extensions/s2n_client_key_share.c b/tls/extensions/s2n_client_key_share.c index ad691c4fd5a..4bcf122cde5 100644 --- a/tls/extensions/s2n_client_key_share.c +++ b/tls/extensions/s2n_client_key_share.c @@ -308,9 +308,9 @@ static int s2n_client_key_share_recv_pq_hybrid(struct s2n_connection *conn, stru return S2N_SUCCESS; } - struct s2n_kem_group_params *client_params = &conn->kex_params.client_kem_group_params; + struct s2n_kem_group_params *current_best_client_params = &conn->kex_params.client_kem_group_params; - const struct s2n_kem_group *kem_group = NULL; + const struct s2n_kem_group *new_best_match = NULL; for (size_t i = 0; i < kem_pref->tls13_kem_group_count; i++) { const struct s2n_kem_group *supported_group = kem_pref->tls13_kem_groups[i]; POSIX_ENSURE_REF(supported_group); @@ -323,7 +323,7 @@ static int s2n_client_key_share_recv_pq_hybrid(struct s2n_connection *conn, stru /* Stop if we reach the current highest priority share. * Any share of lower priority is discarded. */ - if (client_params->kem_group == supported_group) { + if (current_best_client_params->kem_group == supported_group) { break; } @@ -337,20 +337,20 @@ static int s2n_client_key_share_recv_pq_hybrid(struct s2n_connection *conn, stru /* Stop if we find a match */ if (kem_group_iana_id == supported_group->iana_id) { - kem_group = supported_group; + new_best_match = supported_group; break; } } /* Ignore unsupported KEM groups */ - if (!kem_group) { + if (!new_best_match) { return S2N_SUCCESS; } /* The length of the hybrid key share must be one of two possible lengths. Its internal values are either length * prefixed, or they are not. */ uint16_t actual_hybrid_share_size = key_share->blob.size; - uint16_t unprefixed_hybrid_share_size = kem_group->curve->share_size + kem_group->kem->public_key_length; + uint16_t unprefixed_hybrid_share_size = new_best_match->curve->share_size + new_best_match->kem->public_key_length; uint16_t prefixed_hybrid_share_size = (2 * S2N_SIZE_OF_KEY_SHARE_SIZE) + unprefixed_hybrid_share_size; /* Ignore KEM groups with unexpected overall total share sizes */ @@ -364,35 +364,35 @@ static int s2n_client_key_share_recv_pq_hybrid(struct s2n_connection *conn, stru /* Ignore KEM groups with unexpected ECC share sizes */ uint16_t ec_share_size = 0; POSIX_GUARD(s2n_stuffer_read_uint16(key_share, &ec_share_size)); - if (ec_share_size != kem_group->curve->share_size) { + if (ec_share_size != new_best_match->curve->share_size) { return S2N_SUCCESS; } } - DEFER_CLEANUP(struct s2n_kem_group_params new_client_params = { 0 }, s2n_kem_group_free); - new_client_params.kem_group = kem_group; + DEFER_CLEANUP(struct s2n_kem_group_params new_best_client_params = { 0 }, s2n_kem_group_free); + new_best_client_params.kem_group = new_best_match; /* Need to save whether the client included the length prefix so that we can match their behavior in our response. */ - new_client_params.kem_params.len_prefixed = is_hybrid_share_length_prefixed; + new_best_client_params.kem_params.len_prefixed = is_hybrid_share_length_prefixed; - POSIX_GUARD(s2n_client_key_share_parse_ecc(key_share, kem_group->curve, &new_client_params.ecc_params)); + POSIX_GUARD(s2n_client_key_share_parse_ecc(key_share, new_best_match->curve, &new_best_client_params.ecc_params)); /* If we were unable to parse the EC portion of the share, negotiated_curve * will be NULL, and we should ignore the entire key share. */ - if (!new_client_params.ecc_params.negotiated_curve) { + if (!new_best_client_params.ecc_params.negotiated_curve) { return S2N_SUCCESS; } /* Note: the PQ share size is validated in s2n_kem_recv_public_key() */ /* Ignore groups with PQ public keys we can't parse */ - new_client_params.kem_params.kem = kem_group->kem; - if (s2n_kem_recv_public_key(key_share, &new_client_params.kem_params) != S2N_SUCCESS) { + new_best_client_params.kem_params.kem = new_best_match->kem; + if (s2n_kem_recv_public_key(key_share, &new_best_client_params.kem_params) != S2N_SUCCESS) { return S2N_SUCCESS; } - POSIX_GUARD(s2n_kem_group_free(client_params)); - *client_params = new_client_params; + POSIX_GUARD(s2n_kem_group_free(current_best_client_params)); + *current_best_client_params = new_best_client_params; - ZERO_TO_DISABLE_DEFER_CLEANUP(new_client_params); + ZERO_TO_DISABLE_DEFER_CLEANUP(new_best_client_params); return S2N_SUCCESS; } @@ -424,8 +424,7 @@ static int s2n_client_key_share_recv(struct s2n_connection *conn, struct s2n_stu POSIX_GUARD(s2n_stuffer_read_uint16(extension, &share_size)); POSIX_ENSURE(s2n_stuffer_data_available(extension) >= share_size, S2N_ERR_BAD_MESSAGE); - POSIX_GUARD(s2n_blob_init(&key_share_blob, - s2n_stuffer_raw_read(extension, share_size), share_size)); + POSIX_GUARD(s2n_blob_init(&key_share_blob, s2n_stuffer_raw_read(extension, share_size), share_size)); POSIX_GUARD(s2n_stuffer_init(&key_share, &key_share_blob)); POSIX_GUARD(s2n_stuffer_skip_write(&key_share, share_size)); keyshare_count++; @@ -439,6 +438,10 @@ static int s2n_client_key_share_recv(struct s2n_connection *conn, struct s2n_stu /* During a retry, the client should only have sent one keyshare */ POSIX_ENSURE(!s2n_is_hello_retry_handshake(conn) || keyshare_count == 1, S2N_ERR_BAD_MESSAGE); + /* Get the client's preferred params for the KeyShares that were actually sent */ + struct s2n_ecc_evp_params *client_ecc_params = &conn->kex_params.client_ecc_evp_params; + struct s2n_kem_group_params *client_pq_params = &conn->kex_params.client_kem_group_params; + /** * If there were no matching key shares, then we received an empty key share extension * or we didn't match a key share with a supported group. We should send a retry. @@ -448,8 +451,6 @@ static int s2n_client_key_share_recv(struct s2n_connection *conn, struct s2n_stu *# compatible "key_share" extension in the initial ClientHello, the *# server MUST respond with a HelloRetryRequest (Section 4.1.4) message. **/ - struct s2n_ecc_evp_params *client_ecc_params = &conn->kex_params.client_ecc_evp_params; - struct s2n_kem_group_params *client_pq_params = &conn->kex_params.client_kem_group_params; if (!client_pq_params->kem_group && !client_ecc_params->negotiated_curve) { POSIX_GUARD(s2n_set_hello_retry_required(conn)); } diff --git a/tls/extensions/s2n_client_supported_groups.c b/tls/extensions/s2n_client_supported_groups.c index cc30f0c9dff..e5f2a71a597 100644 --- a/tls/extensions/s2n_client_supported_groups.c +++ b/tls/extensions/s2n_client_supported_groups.c @@ -106,7 +106,6 @@ S2N_RESULT s2n_supported_groups_parse_count(struct s2n_stuffer *extension, uint1 static int s2n_client_supported_groups_recv_iana_id(struct s2n_connection *conn, uint16_t iana_id) { POSIX_ENSURE_REF(conn); - const struct s2n_ecc_preferences *ecc_pref = NULL; POSIX_GUARD(s2n_connection_get_ecc_preferences(conn, &ecc_pref)); POSIX_ENSURE_REF(ecc_pref); @@ -142,7 +141,6 @@ static int s2n_client_supported_groups_recv_iana_id(struct s2n_connection *conn, static int s2n_choose_supported_group(struct s2n_connection *conn) { POSIX_ENSURE_REF(conn); - const struct s2n_ecc_preferences *ecc_pref = NULL; POSIX_GUARD(s2n_connection_get_ecc_preferences(conn, &ecc_pref)); POSIX_ENSURE_REF(ecc_pref); diff --git a/tls/extensions/s2n_server_key_share.c b/tls/extensions/s2n_server_key_share.c index 08128c8df34..a34730990e5 100644 --- a/tls/extensions/s2n_server_key_share.c +++ b/tls/extensions/s2n_server_key_share.c @@ -354,6 +354,14 @@ int s2n_extensions_server_key_share_select(struct s2n_connection *conn) POSIX_GUARD(s2n_connection_get_kem_preferences(conn, &kem_pref)); POSIX_ENSURE_REF(kem_pref); + /* Get the client's preferred groups for the KeyShares that were actually sent by the client */ + const struct s2n_ecc_named_curve *client_curve = conn->kex_params.client_ecc_evp_params.negotiated_curve; + const struct s2n_kem_group *client_kem_group = conn->kex_params.client_kem_group_params.kem_group; + + /* Get the server's preferred groups (which may or may not have been sent in the KeyShare by the client) */ + const struct s2n_ecc_named_curve *server_curve = conn->kex_params.server_ecc_evp_params.negotiated_curve; + const struct s2n_kem_group *server_kem_group = conn->kex_params.server_kem_group_params.kem_group; + /* Boolean XOR check. When receiving the supported_groups extension, s2n server * should (exclusively) set either server_curve or server_kem_group based on the * set of mutually supported groups. If both server_curve and server_kem_group @@ -361,37 +369,44 @@ int s2n_extensions_server_key_share_select(struct s2n_connection *conn) * groups; key negotiation is not possible and the handshake should be aborted * without sending HRR. (The case of both being non-NULL should never occur, and * is an error.) */ - const struct s2n_ecc_named_curve *server_curve = conn->kex_params.server_ecc_evp_params.negotiated_curve; - const struct s2n_kem_group *server_kem_group = conn->kex_params.server_kem_group_params.kem_group; POSIX_ENSURE((server_curve == NULL) != (server_kem_group == NULL), S2N_ERR_ECDHE_UNSUPPORTED_CURVE); /* To avoid extra round trips, we prefer to negotiate a group for which we have already * received a key share (even if it is different than the group previously chosen). In * general, we prefer to negotiate PQ over ECDHE; however, if both client and server * support PQ, but the client sent only EC key shares, then we will negotiate ECHDE. */ - if (conn->kex_params.client_kem_group_params.kem_group) { + + /* Option 1: Select the best mutually supported PQ Hybrid Group that can be negotiated in 1-RTT */ + if (client_kem_group) { POSIX_ENSURE_REF(conn->kex_params.client_kem_group_params.ecc_params.negotiated_curve); POSIX_ENSURE_REF(conn->kex_params.client_kem_group_params.kem_params.kem); conn->kex_params.server_kem_group_params.kem_group = conn->kex_params.client_kem_group_params.kem_group; conn->kex_params.server_kem_group_params.ecc_params.negotiated_curve = conn->kex_params.client_kem_group_params.ecc_params.negotiated_curve; conn->kex_params.server_kem_group_params.kem_params.kem = conn->kex_params.client_kem_group_params.kem_params.kem; + conn->kex_params.server_ecc_evp_params.negotiated_curve = NULL; + return S2N_SUCCESS; + } + /* Option 2: Otherwise, if any PQ Hybrid Groups can be negotiated in 2-RTT's select it */ + if (client_kem_group == NULL && server_kem_group != NULL) { + /* Null out any available ECC curves so that they won't be sent in the ClientHelloRetry */ conn->kex_params.server_ecc_evp_params.negotiated_curve = NULL; + POSIX_GUARD(s2n_set_hello_retry_required(conn)); return S2N_SUCCESS; } - if (conn->kex_params.client_ecc_evp_params.negotiated_curve) { + /* Option 3: Otherwise, select a mutually supported classical ECC curve that can be negotiated in 1-RTT */ + if (client_curve) { conn->kex_params.server_ecc_evp_params.negotiated_curve = conn->kex_params.client_ecc_evp_params.negotiated_curve; - conn->kex_params.server_kem_group_params.kem_group = NULL; conn->kex_params.server_kem_group_params.ecc_params.negotiated_curve = NULL; conn->kex_params.server_kem_group_params.kem_params.kem = NULL; return S2N_SUCCESS; } - /* Server and client have mutually supported groups, but the client did not send key - * shares for any of them. Send HRR indicating the server's preference. */ + /* Option 4: Server and client have mutually supported groups, but the client did not send key shares for any of + * them. Send a HelloRetryRequest indicating the server's preference. */ POSIX_GUARD(s2n_set_hello_retry_required(conn)); return S2N_SUCCESS; } diff --git a/tls/s2n_server_hello_retry.c b/tls/s2n_server_hello_retry.c index fd05ca07236..25febad1fc1 100644 --- a/tls/s2n_server_hello_retry.c +++ b/tls/s2n_server_hello_retry.c @@ -69,10 +69,11 @@ int s2n_server_hello_retry_recv(struct s2n_connection *conn) POSIX_ENSURE_REF(kem_pref); const struct s2n_ecc_named_curve *named_curve = conn->kex_params.server_ecc_evp_params.negotiated_curve; - const struct s2n_kem_group *kem_group = conn->kex_params.server_kem_group_params.kem_group; + const struct s2n_kem_group *server_preferred_kem_group = conn->kex_params.server_kem_group_params.kem_group; + const struct s2n_kem_group *client_preferred_kem_group = conn->kex_params.client_kem_group_params.kem_group; /* Boolean XOR check: exactly one of {named_curve, kem_group} should be non-null. */ - POSIX_ENSURE((named_curve != NULL) != (kem_group != NULL), S2N_ERR_INVALID_HELLO_RETRY); + POSIX_ENSURE((named_curve != NULL) != (server_preferred_kem_group != NULL), S2N_ERR_INVALID_HELLO_RETRY); /** *= https://www.rfc-editor.org/rfc/rfc8446#4.2.8 @@ -85,7 +86,7 @@ int s2n_server_hello_retry_recv(struct s2n_connection *conn) if (named_curve != NULL && s2n_ecc_preferences_includes_curve(ecc_pref, named_curve->iana_id)) { selected_group_in_supported_groups = true; } - if (kem_group != NULL && s2n_kem_group_is_available(kem_group) && s2n_kem_preferences_includes_tls13_kem_group(kem_pref, kem_group->iana_id)) { + if (server_preferred_kem_group != NULL && s2n_kem_group_is_available(server_preferred_kem_group) && s2n_kem_preferences_includes_tls13_kem_group(kem_pref, server_preferred_kem_group->iana_id)) { selected_group_in_supported_groups = true; } @@ -96,14 +97,16 @@ int s2n_server_hello_retry_recv(struct s2n_connection *conn) *# in the original ClientHello. **/ bool new_key_share_requested = false; + if (named_curve != NULL) { new_key_share_requested = (named_curve != conn->kex_params.client_ecc_evp_params.negotiated_curve); } - if (kem_group != NULL) { + + if (server_preferred_kem_group != NULL) { /* If PQ is disabled, the client should not have sent any PQ IDs * in the supported_groups list of the initial ClientHello */ POSIX_ENSURE(s2n_pq_is_enabled(), S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); - new_key_share_requested = (kem_group != conn->kex_params.client_kem_group_params.kem_group); + new_key_share_requested = (server_preferred_kem_group != client_preferred_kem_group); } /** From e9b0611f386736076f45cd967960172fb8ad6ca6 Mon Sep 17 00:00:00 2001 From: Alex Weibel Date: Mon, 29 Apr 2024 11:50:03 -0700 Subject: [PATCH 2/5] Update tests to pass on Openssl 1.1.1 --- tests/unit/s2n_tls13_pq_handshake_test.c | 98 +++++++++++++++++++----- tls/extensions/s2n_server_key_share.c | 6 +- 2 files changed, 80 insertions(+), 24 deletions(-) diff --git a/tests/unit/s2n_tls13_pq_handshake_test.c b/tests/unit/s2n_tls13_pq_handshake_test.c index 27959a33678..5e1a7a4a084 100644 --- a/tests/unit/s2n_tls13_pq_handshake_test.c +++ b/tests/unit/s2n_tls13_pq_handshake_test.c @@ -36,14 +36,14 @@ const struct s2n_kem_group *s2n_get_predicted_negotiated_kem_group(const struct * prefer over this one but would require 2-RTT's). */ const struct s2n_kem_group *client_default = client_prefs->tls13_kem_groups[0]; for (int i = 0; i < server_prefs->tls13_kem_group_count; i++) { - const struct s2n_kem_group *server_group = server_prefs->tls13_kem_groups[i]; - PTR_ENSURE_REF(client_default); - PTR_ENSURE_REF(server_group); - if (s2n_kem_group_is_available(client_default) && s2n_kem_group_is_available(server_group) - && client_default->iana_id == server_group->iana_id - && s2n_kem_group_is_available(client_default) == s2n_kem_group_is_available(server_group)) { - return client_default; - } + const struct s2n_kem_group *server_group = server_prefs->tls13_kem_groups[i]; + PTR_ENSURE_REF(client_default); + PTR_ENSURE_REF(server_group); + if (s2n_kem_group_is_available(client_default) && s2n_kem_group_is_available(server_group) + && client_default->iana_id == server_group->iana_id + && s2n_kem_group_is_available(client_default) == s2n_kem_group_is_available(server_group)) { + return client_default; + } } /* Otherwise, if the client's default isn't supported, and a 2-RTT PQ handshake is required, the server will choose @@ -65,6 +65,44 @@ const struct s2n_kem_group *s2n_get_predicted_negotiated_kem_group(const struct return NULL; } +const struct s2n_ecc_named_curve *s2n_get_predicted_negotiated_ecdhe_curve(const struct s2n_security_policy *client_sec_policy, + const struct s2n_security_policy *server_sec_policy) +{ + PTR_ENSURE_REF(client_sec_policy); + PTR_ENSURE_REF(server_sec_policy); + + /* Client will offer their highest priority ECDHE KeyShare in their ClientHello. This KeyShare will be most preferred + * since it can be negotiated in 1-RTT (even if there are other mutually supported ECDHE KeyShares that the server would + * prefer over this one but would require 2-RTT's). */ + const struct s2n_ecc_named_curve *client_default = client_sec_policy->ecc_preferences->ecc_curves[0]; + for (int i = 0; i < server_sec_policy->ecc_preferences->count; i++) { + const struct s2n_ecc_named_curve *server_curve = server_sec_policy->ecc_preferences->ecc_curves[i]; + + PTR_ENSURE_REF(client_default); + PTR_ENSURE_REF(server_curve); + + if (server_curve->iana_id == client_default->iana_id) { + return client_default; + } + } + + /* Otherwise, if the client's default isn't supported, and a 2-RTT handshake is required, the server will choose + * whichever mutually supported PQ KeyShare that is highest on the server's preference list. */ + for (int i = 0; i < server_sec_policy->ecc_preferences->count; i++) { + const struct s2n_ecc_named_curve *server_curve = server_sec_policy->ecc_preferences->ecc_curves[i]; + + for (int j = 0; j < client_sec_policy->ecc_preferences->count; j++) { + const struct s2n_ecc_named_curve *client_curve = client_sec_policy->ecc_preferences->ecc_curves[j]; + PTR_ENSURE_REF(client_curve); + PTR_ENSURE_REF(server_curve); + if (client_curve->iana_id == server_curve->iana_id) { + return client_curve; + } + } + } + return NULL; +} + int s2n_test_tls13_pq_handshake(const struct s2n_security_policy *client_sec_policy, const struct s2n_security_policy *server_sec_policy, const struct s2n_kem_group *expected_kem_group, const struct s2n_ecc_named_curve *expected_curve, bool hrr_expected, bool len_prefix_expected) @@ -73,9 +111,9 @@ int s2n_test_tls13_pq_handshake(const struct s2n_security_policy *client_sec_pol POSIX_ENSURE((expected_kem_group == NULL) != (expected_curve == NULL), S2N_ERR_SAFETY); if (expected_kem_group != NULL) { - const struct s2n_kem_group *predicted_kem_group = s2n_get_predicted_negotiated_kem_group(client_sec_policy->kem_preferences, server_sec_policy->kem_preferences); - POSIX_ENSURE_REF(predicted_kem_group); - POSIX_ENSURE_EQ(expected_kem_group->iana_id, predicted_kem_group->iana_id); + const struct s2n_kem_group *predicted_kem_group = s2n_get_predicted_negotiated_kem_group(client_sec_policy->kem_preferences, server_sec_policy->kem_preferences); + POSIX_ENSURE_REF(predicted_kem_group); + POSIX_ENSURE_EQ(expected_kem_group->iana_id, predicted_kem_group->iana_id); } /* Set up connections */ @@ -364,10 +402,10 @@ int main() .ecc_preferences = security_policy_test_tls13_retry.ecc_preferences, }; - const struct s2n_ecc_named_curve *expected_curve = &s2n_ecc_curve_x25519; + const struct s2n_ecc_named_curve *default_curve = &s2n_ecc_curve_x25519; if (!s2n_is_evp_apis_supported()) { - expected_curve = &s2n_ecc_curve_secp256r1; + default_curve = &s2n_ecc_curve_secp256r1; } struct pq_handshake_test_vector { @@ -388,7 +426,7 @@ int main() .server_policy = &security_policy_pq_tls_1_0_2021_05_24, .expected_kem_group = &s2n_x25519_kyber_512_r3, .expected_curve = NULL, - .hrr_expected = true, + .hrr_expected = s2n_pq_is_enabled(), .len_prefix_expected = false, }, /* Server and Client both support PQ and TLS 1.3 */ @@ -555,7 +593,7 @@ int main() .client_policy = &security_policy_pq_tls_1_0_2020_12, .server_policy = &security_policy_test_all_tls13, .expected_kem_group = NULL, - .expected_curve = expected_curve, + .expected_curve = default_curve, .hrr_expected = false, .len_prefix_expected = true, }, @@ -566,7 +604,7 @@ int main() .client_policy = &ecc_retry_policy, .server_policy = &security_policy_test_all_tls13, .expected_kem_group = NULL, - .expected_curve = expected_curve, + .expected_curve = default_curve, .hrr_expected = true, .len_prefix_expected = true, }, @@ -577,7 +615,7 @@ int main() .client_policy = &security_policy_test_all_tls13, .server_policy = &security_policy_pq_tls_1_0_2020_12, .expected_kem_group = NULL, - .expected_curve = expected_curve, + .expected_curve = default_curve, .hrr_expected = false, .len_prefix_expected = true, }, @@ -588,7 +626,7 @@ int main() .client_policy = &security_policy_test_tls13_retry, .server_policy = &security_policy_pq_tls_1_0_2020_12, .expected_kem_group = NULL, - .expected_curve = expected_curve, + .expected_curve = default_curve, .hrr_expected = true, .len_prefix_expected = true, }, @@ -609,11 +647,29 @@ int main() * curves, so if the server policy doesn't contain x25519, modify * that expectation to a NIST curve. */ - if (!s2n_ecc_preferences_includes_curve(server_policy->ecc_preferences, expected_curve->iana_id)) { + + const struct s2n_ecc_named_curve *client_default = client_policy->ecc_preferences->ecc_curves[0]; + const struct s2n_ecc_named_curve *predicted_curve = s2n_get_predicted_negotiated_ecdhe_curve(client_policy, server_policy); + + /* If either policy doesn't support the default curve, fall back to p256 as it should be in common with every ECC preference list. */ + if (!s2n_ecc_preferences_includes_curve(client_policy->ecc_preferences, default_curve->iana_id) + || !s2n_ecc_preferences_includes_curve(server_policy->ecc_preferences, default_curve->iana_id)) { + EXPECT_TRUE(s2n_ecc_preferences_includes_curve(client_policy->ecc_preferences, s2n_ecc_curve_secp256r1.iana_id)); + EXPECT_TRUE(s2n_ecc_preferences_includes_curve(server_policy->ecc_preferences, s2n_ecc_curve_secp256r1.iana_id)); curve = &s2n_ecc_curve_secp256r1; - } else { - curve = expected_curve; } + + /* The client's preferred curve will be a higher priority than the default if both sides support TLS 1.3 since it can be chosen in 1-RTT. */ + if (s2n_security_policy_supports_tls13(client_policy) && s2n_security_policy_supports_tls13(server_policy) + && s2n_ecc_preferences_includes_curve(server_policy->ecc_preferences, client_default->iana_id)) { + curve = client_default; + } + + EXPECT_EQUAL(curve->iana_id, predicted_curve->iana_id); + } + + if (!s2n_kem_group_is_available(kem_group)) { + kem_group = NULL; } EXPECT_SUCCESS(s2n_test_tls13_pq_handshake(client_policy, server_policy, kem_group, curve, hrr_expected, len_prefix_expected)); diff --git a/tls/extensions/s2n_server_key_share.c b/tls/extensions/s2n_server_key_share.c index a34730990e5..2c091f3362c 100644 --- a/tls/extensions/s2n_server_key_share.c +++ b/tls/extensions/s2n_server_key_share.c @@ -377,7 +377,7 @@ int s2n_extensions_server_key_share_select(struct s2n_connection *conn) * support PQ, but the client sent only EC key shares, then we will negotiate ECHDE. */ /* Option 1: Select the best mutually supported PQ Hybrid Group that can be negotiated in 1-RTT */ - if (client_kem_group) { + if (client_kem_group != NULL) { POSIX_ENSURE_REF(conn->kex_params.client_kem_group_params.ecc_params.negotiated_curve); POSIX_ENSURE_REF(conn->kex_params.client_kem_group_params.kem_params.kem); @@ -388,8 +388,8 @@ int s2n_extensions_server_key_share_select(struct s2n_connection *conn) return S2N_SUCCESS; } - /* Option 2: Otherwise, if any PQ Hybrid Groups can be negotiated in 2-RTT's select it */ - if (client_kem_group == NULL && server_kem_group != NULL) { + /* Option 2: Otherwise, if any PQ Hybrid Groups can be negotiated in 2-RTT's select it.*/ + if (server_kem_group != NULL) { /* Null out any available ECC curves so that they won't be sent in the ClientHelloRetry */ conn->kex_params.server_ecc_evp_params.negotiated_curve = NULL; POSIX_GUARD(s2n_set_hello_retry_required(conn)); From 3d9250e9523cafb7fbf7860b3495359beda781c8 Mon Sep 17 00:00:00 2001 From: Alex Weibel Date: Mon, 20 May 2024 16:14:13 -0700 Subject: [PATCH 3/5] Cleanup --- ..._extensions_server_key_share_select_test.c | 1 - tests/unit/s2n_tls13_pq_handshake_test.c | 25 +++++++++---------- tls/extensions/s2n_client_key_share.c | 6 ++--- tls/extensions/s2n_client_supported_groups.c | 2 ++ tls/extensions/s2n_server_key_share.c | 8 +++--- 5 files changed, 20 insertions(+), 22 deletions(-) diff --git a/tests/unit/s2n_extensions_server_key_share_select_test.c b/tests/unit/s2n_extensions_server_key_share_select_test.c index 08aa1439b10..04cf1f6cb7e 100644 --- a/tests/unit/s2n_extensions_server_key_share_select_test.c +++ b/tests/unit/s2n_extensions_server_key_share_select_test.c @@ -186,7 +186,6 @@ int main() EXPECT_EQUAL(server_params->kem_group, kem_group1); EXPECT_EQUAL(server_params->kem_params.kem, kem_group1->kem); EXPECT_EQUAL(server_params->ecc_params.negotiated_curve, kem_group1->curve); - EXPECT_NULL(server_conn->kex_params.client_kem_group_params.kem_group); EXPECT_NULL(server_conn->kex_params.server_ecc_evp_params.negotiated_curve); EXPECT_TRUE(s2n_is_hello_retry_handshake(server_conn)); diff --git a/tests/unit/s2n_tls13_pq_handshake_test.c b/tests/unit/s2n_tls13_pq_handshake_test.c index 5e1a7a4a084..839e3cfbc6a 100644 --- a/tests/unit/s2n_tls13_pq_handshake_test.c +++ b/tests/unit/s2n_tls13_pq_handshake_test.c @@ -110,12 +110,6 @@ int s2n_test_tls13_pq_handshake(const struct s2n_security_policy *client_sec_pol /* XOR check: can expect to negotiate either a KEM group, or a classic EC curve, but not both/neither */ POSIX_ENSURE((expected_kem_group == NULL) != (expected_curve == NULL), S2N_ERR_SAFETY); - if (expected_kem_group != NULL) { - const struct s2n_kem_group *predicted_kem_group = s2n_get_predicted_negotiated_kem_group(client_sec_policy->kem_preferences, server_sec_policy->kem_preferences); - POSIX_ENSURE_REF(predicted_kem_group); - POSIX_ENSURE_EQ(expected_kem_group->iana_id, predicted_kem_group->iana_id); - } - /* Set up connections */ struct s2n_connection *client_conn = NULL, *server_conn = NULL; POSIX_ENSURE_REF(client_conn = s2n_connection_new(S2N_CLIENT)); @@ -642,12 +636,7 @@ int main() bool len_prefix_expected = vector->len_prefix_expected; if (!s2n_pq_is_enabled()) { - /* If PQ is disabled, for older policies we expect to negotiate - * x25519 ECDH if available. Newer policies only include NIST - * curves, so if the server policy doesn't contain x25519, modify - * that expectation to a NIST curve. - */ - + EXPECT_TRUE(client_policy->ecc_preferences->count > 0); const struct s2n_ecc_named_curve *client_default = client_policy->ecc_preferences->ecc_curves[0]; const struct s2n_ecc_named_curve *predicted_curve = s2n_get_predicted_negotiated_ecdhe_curve(client_policy, server_policy); @@ -659,12 +648,14 @@ int main() curve = &s2n_ecc_curve_secp256r1; } - /* The client's preferred curve will be a higher priority than the default if both sides support TLS 1.3 since it can be chosen in 1-RTT. */ + /* The client's preferred curve will be a higher priority than the default if both sides support TLS 1.3, + * and if the client's default can be chosen by the server in 1-RTT. */ if (s2n_security_policy_supports_tls13(client_policy) && s2n_security_policy_supports_tls13(server_policy) && s2n_ecc_preferences_includes_curve(server_policy->ecc_preferences, client_default->iana_id)) { curve = client_default; } + /* Finally, confirm that the expected curve listed in the test vector matches the output of s2n_get_predicted_negotiated_ecdhe_curve() */ EXPECT_EQUAL(curve->iana_id, predicted_curve->iana_id); } @@ -672,6 +663,14 @@ int main() kem_group = NULL; } + if (kem_group != NULL) { + const struct s2n_kem_group *predicted_kem_group = s2n_get_predicted_negotiated_kem_group(client_policy->kem_preferences, server_policy->kem_preferences); + POSIX_ENSURE_REF(predicted_kem_group); + + /* Confirm that the expected KEM Group listed in the test vector matches the output of s2n_get_predicted_negotiated_kem_group() */ + POSIX_ENSURE_EQ(kem_group->iana_id, predicted_kem_group->iana_id); + } + EXPECT_SUCCESS(s2n_test_tls13_pq_handshake(client_policy, server_policy, kem_group, curve, hrr_expected, len_prefix_expected)); } diff --git a/tls/extensions/s2n_client_key_share.c b/tls/extensions/s2n_client_key_share.c index 4bcf122cde5..a48b9c67dc8 100644 --- a/tls/extensions/s2n_client_key_share.c +++ b/tls/extensions/s2n_client_key_share.c @@ -438,10 +438,6 @@ static int s2n_client_key_share_recv(struct s2n_connection *conn, struct s2n_stu /* During a retry, the client should only have sent one keyshare */ POSIX_ENSURE(!s2n_is_hello_retry_handshake(conn) || keyshare_count == 1, S2N_ERR_BAD_MESSAGE); - /* Get the client's preferred params for the KeyShares that were actually sent */ - struct s2n_ecc_evp_params *client_ecc_params = &conn->kex_params.client_ecc_evp_params; - struct s2n_kem_group_params *client_pq_params = &conn->kex_params.client_kem_group_params; - /** * If there were no matching key shares, then we received an empty key share extension * or we didn't match a key share with a supported group. We should send a retry. @@ -451,6 +447,8 @@ static int s2n_client_key_share_recv(struct s2n_connection *conn, struct s2n_stu *# compatible "key_share" extension in the initial ClientHello, the *# server MUST respond with a HelloRetryRequest (Section 4.1.4) message. **/ + struct s2n_ecc_evp_params *client_ecc_params = &conn->kex_params.client_ecc_evp_params; + struct s2n_kem_group_params *client_pq_params = &conn->kex_params.client_kem_group_params; if (!client_pq_params->kem_group && !client_ecc_params->negotiated_curve) { POSIX_GUARD(s2n_set_hello_retry_required(conn)); } diff --git a/tls/extensions/s2n_client_supported_groups.c b/tls/extensions/s2n_client_supported_groups.c index e5f2a71a597..cc30f0c9dff 100644 --- a/tls/extensions/s2n_client_supported_groups.c +++ b/tls/extensions/s2n_client_supported_groups.c @@ -106,6 +106,7 @@ S2N_RESULT s2n_supported_groups_parse_count(struct s2n_stuffer *extension, uint1 static int s2n_client_supported_groups_recv_iana_id(struct s2n_connection *conn, uint16_t iana_id) { POSIX_ENSURE_REF(conn); + const struct s2n_ecc_preferences *ecc_pref = NULL; POSIX_GUARD(s2n_connection_get_ecc_preferences(conn, &ecc_pref)); POSIX_ENSURE_REF(ecc_pref); @@ -141,6 +142,7 @@ static int s2n_client_supported_groups_recv_iana_id(struct s2n_connection *conn, static int s2n_choose_supported_group(struct s2n_connection *conn) { POSIX_ENSURE_REF(conn); + const struct s2n_ecc_preferences *ecc_pref = NULL; POSIX_GUARD(s2n_connection_get_ecc_preferences(conn, &ecc_pref)); POSIX_ENSURE_REF(ecc_pref); diff --git a/tls/extensions/s2n_server_key_share.c b/tls/extensions/s2n_server_key_share.c index 2c091f3362c..e20dc2452d5 100644 --- a/tls/extensions/s2n_server_key_share.c +++ b/tls/extensions/s2n_server_key_share.c @@ -388,7 +388,7 @@ int s2n_extensions_server_key_share_select(struct s2n_connection *conn) return S2N_SUCCESS; } - /* Option 2: Otherwise, if any PQ Hybrid Groups can be negotiated in 2-RTT's select it.*/ + /* Option 2: Otherwise, if any PQ Hybrid Groups can be negotiated in 2-RTT's select that one. */ if (server_kem_group != NULL) { /* Null out any available ECC curves so that they won't be sent in the ClientHelloRetry */ conn->kex_params.server_ecc_evp_params.negotiated_curve = NULL; @@ -396,7 +396,7 @@ int s2n_extensions_server_key_share_select(struct s2n_connection *conn) return S2N_SUCCESS; } - /* Option 3: Otherwise, select a mutually supported classical ECC curve that can be negotiated in 1-RTT */ + /* Option 3: Otherwise, if there is a mutually supported classical ECDHE-only group can be negotiated in 1-RTT, select that one */ if (client_curve) { conn->kex_params.server_ecc_evp_params.negotiated_curve = conn->kex_params.client_ecc_evp_params.negotiated_curve; conn->kex_params.server_kem_group_params.kem_group = NULL; @@ -405,8 +405,8 @@ int s2n_extensions_server_key_share_select(struct s2n_connection *conn) return S2N_SUCCESS; } - /* Option 4: Server and client have mutually supported groups, but the client did not send key shares for any of - * them. Send a HelloRetryRequest indicating the server's preference. */ + /* Option 4: Server and client have at least 1 mutually supported group, but the client did not send key shares for + * any of them. Send a HelloRetryRequest indicating the server's preference. */ POSIX_GUARD(s2n_set_hello_retry_required(conn)); return S2N_SUCCESS; } From 71239fa8537019a9af013d9a4b30504a6f8a7e19 Mon Sep 17 00:00:00 2001 From: Alex Weibel Date: Wed, 12 Jun 2024 15:08:50 -0700 Subject: [PATCH 4/5] Address Feedback --- tests/unit/s2n_tls13_pq_handshake_test.c | 20 ++++++++++++-------- tls/extensions/s2n_server_key_share.c | 14 +++++--------- tls/s2n_server_hello_retry.c | 4 +++- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/tests/unit/s2n_tls13_pq_handshake_test.c b/tests/unit/s2n_tls13_pq_handshake_test.c index 839e3cfbc6a..0c85da58fef 100644 --- a/tests/unit/s2n_tls13_pq_handshake_test.c +++ b/tests/unit/s2n_tls13_pq_handshake_test.c @@ -35,13 +35,14 @@ const struct s2n_kem_group *s2n_get_predicted_negotiated_kem_group(const struct * since it can be negotiated in 1-RTT (even if there are other mutually supported PQ KeyShares that the server would * prefer over this one but would require 2-RTT's). */ const struct s2n_kem_group *client_default = client_prefs->tls13_kem_groups[0]; + PTR_ENSURE_REF(client_default); + for (int i = 0; i < server_prefs->tls13_kem_group_count; i++) { const struct s2n_kem_group *server_group = server_prefs->tls13_kem_groups[i]; - PTR_ENSURE_REF(client_default); PTR_ENSURE_REF(server_group); if (s2n_kem_group_is_available(client_default) && s2n_kem_group_is_available(server_group) && client_default->iana_id == server_group->iana_id - && s2n_kem_group_is_available(client_default) == s2n_kem_group_is_available(server_group)) { + && s2n_kem_group_is_available(client_default)) { return client_default; } } @@ -51,17 +52,19 @@ const struct s2n_kem_group *s2n_get_predicted_negotiated_kem_group(const struct for (int i = 0; i < server_prefs->tls13_kem_group_count; i++) { const struct s2n_kem_group *server_group = server_prefs->tls13_kem_groups[i]; - for (int j = 0; j < client_prefs->tls13_kem_group_count; j++) { + /* j starts at 1 since we already checked client_prefs->tls13_kem_groups[0] above */ + for (int j = 1; j < client_prefs->tls13_kem_group_count; j++) { const struct s2n_kem_group *client_group = client_prefs->tls13_kem_groups[j]; PTR_ENSURE_REF(client_group); PTR_ENSURE_REF(server_group); if (s2n_kem_group_is_available(client_group) && s2n_kem_group_is_available(server_group) && client_group->iana_id == server_group->iana_id - && s2n_kem_group_is_available(client_group) == s2n_kem_group_is_available(server_group)) { + && s2n_kem_group_is_available(client_group)) { return client_group; } } } + return NULL; } @@ -75,12 +78,11 @@ const struct s2n_ecc_named_curve *s2n_get_predicted_negotiated_ecdhe_curve(const * since it can be negotiated in 1-RTT (even if there are other mutually supported ECDHE KeyShares that the server would * prefer over this one but would require 2-RTT's). */ const struct s2n_ecc_named_curve *client_default = client_sec_policy->ecc_preferences->ecc_curves[0]; + PTR_ENSURE_REF(client_default); + for (int i = 0; i < server_sec_policy->ecc_preferences->count; i++) { const struct s2n_ecc_named_curve *server_curve = server_sec_policy->ecc_preferences->ecc_curves[i]; - - PTR_ENSURE_REF(client_default); PTR_ENSURE_REF(server_curve); - if (server_curve->iana_id == client_default->iana_id) { return client_default; } @@ -91,7 +93,8 @@ const struct s2n_ecc_named_curve *s2n_get_predicted_negotiated_ecdhe_curve(const for (int i = 0; i < server_sec_policy->ecc_preferences->count; i++) { const struct s2n_ecc_named_curve *server_curve = server_sec_policy->ecc_preferences->ecc_curves[i]; - for (int j = 0; j < client_sec_policy->ecc_preferences->count; j++) { + /* j starts at 1 since we already checked client_sec_policy->ecc_preferences->ecc_curves[0] above */ + for (int j = 1; j < client_sec_policy->ecc_preferences->count; j++) { const struct s2n_ecc_named_curve *client_curve = client_sec_policy->ecc_preferences->ecc_curves[j]; PTR_ENSURE_REF(client_curve); PTR_ENSURE_REF(server_curve); @@ -100,6 +103,7 @@ const struct s2n_ecc_named_curve *s2n_get_predicted_negotiated_ecdhe_curve(const } } } + return NULL; } diff --git a/tls/extensions/s2n_server_key_share.c b/tls/extensions/s2n_server_key_share.c index e20dc2452d5..1e0ecf84b71 100644 --- a/tls/extensions/s2n_server_key_share.c +++ b/tls/extensions/s2n_server_key_share.c @@ -346,14 +346,6 @@ int s2n_extensions_server_key_share_select(struct s2n_connection *conn) { POSIX_ENSURE_REF(conn); - const struct s2n_ecc_preferences *ecc_pref = NULL; - POSIX_GUARD(s2n_connection_get_ecc_preferences(conn, &ecc_pref)); - POSIX_ENSURE_REF(ecc_pref); - - const struct s2n_kem_preferences *kem_pref = NULL; - POSIX_GUARD(s2n_connection_get_kem_preferences(conn, &kem_pref)); - POSIX_ENSURE_REF(kem_pref); - /* Get the client's preferred groups for the KeyShares that were actually sent by the client */ const struct s2n_ecc_named_curve *client_curve = conn->kex_params.client_ecc_evp_params.negotiated_curve; const struct s2n_kem_group *client_kem_group = conn->kex_params.client_kem_group_params.kem_group; @@ -388,7 +380,11 @@ int s2n_extensions_server_key_share_select(struct s2n_connection *conn) return S2N_SUCCESS; } - /* Option 2: Otherwise, if any PQ Hybrid Groups can be negotiated in 2-RTT's select that one. */ + /* Option 2: Otherwise, if any PQ Hybrid Groups can be negotiated in 2-RTT's select that one. This ensures that + * clients who offer PQ (and presumably therefore have concerns about quantum computing impacting the long term + * confidentiality of their data), have their choice to offer PQ respected, even if they predict the server-side + * supports a different PQ KeyShare algorithms. This ensures clients with PQ support are never downgraded to non-PQ + * algorithms. */ if (server_kem_group != NULL) { /* Null out any available ECC curves so that they won't be sent in the ClientHelloRetry */ conn->kex_params.server_ecc_evp_params.negotiated_curve = NULL; diff --git a/tls/s2n_server_hello_retry.c b/tls/s2n_server_hello_retry.c index 25febad1fc1..702c4362c8d 100644 --- a/tls/s2n_server_hello_retry.c +++ b/tls/s2n_server_hello_retry.c @@ -86,7 +86,9 @@ int s2n_server_hello_retry_recv(struct s2n_connection *conn) if (named_curve != NULL && s2n_ecc_preferences_includes_curve(ecc_pref, named_curve->iana_id)) { selected_group_in_supported_groups = true; } - if (server_preferred_kem_group != NULL && s2n_kem_group_is_available(server_preferred_kem_group) && s2n_kem_preferences_includes_tls13_kem_group(kem_pref, server_preferred_kem_group->iana_id)) { + if (server_preferred_kem_group != NULL + && s2n_kem_group_is_available(server_preferred_kem_group) + && s2n_kem_preferences_includes_tls13_kem_group(kem_pref, server_preferred_kem_group->iana_id)) { selected_group_in_supported_groups = true; } From 34a97bc633bc5bb1557b86d3978c87c5817a6f33 Mon Sep 17 00:00:00 2001 From: Alex Weibel Date: Wed, 19 Jun 2024 12:19:04 -0700 Subject: [PATCH 5/5] Address Feedback --- ..._extensions_server_key_share_select_test.c | 1 - tests/unit/s2n_tls13_pq_handshake_test.c | 24 +++++++----- tls/extensions/s2n_client_key_share.c | 37 ++++++++++--------- 3 files changed, 33 insertions(+), 29 deletions(-) diff --git a/tests/unit/s2n_extensions_server_key_share_select_test.c b/tests/unit/s2n_extensions_server_key_share_select_test.c index 04cf1f6cb7e..9591a792fc6 100644 --- a/tests/unit/s2n_extensions_server_key_share_select_test.c +++ b/tests/unit/s2n_extensions_server_key_share_select_test.c @@ -421,7 +421,6 @@ int main() EXPECT_SUCCESS(s2n_extensions_server_key_share_select(server_conn)); - /* Server should update its choice to curve[0], no HRR */ EXPECT_NULL(server_conn->kex_params.server_ecc_evp_params.negotiated_curve); EXPECT_EQUAL(server_params->kem_group, kem_group0); EXPECT_EQUAL(server_params->kem_params.kem, kem_group0->kem); diff --git a/tests/unit/s2n_tls13_pq_handshake_test.c b/tests/unit/s2n_tls13_pq_handshake_test.c index 0c85da58fef..d8301c761de 100644 --- a/tests/unit/s2n_tls13_pq_handshake_test.c +++ b/tests/unit/s2n_tls13_pq_handshake_test.c @@ -31,9 +31,10 @@ const struct s2n_kem_group *s2n_get_predicted_negotiated_kem_group(const struct PTR_ENSURE_REF(client_prefs); PTR_ENSURE_REF(server_prefs); - /* Client will offer their highest priority PQ KeyShare in their ClientHello. This PQ KeyShare will be most preferred - * since it can be negotiated in 1-RTT (even if there are other mutually supported PQ KeyShares that the server would - * prefer over this one but would require 2-RTT's). */ + /* Client will offer their highest priority PQ KeyShare in their ClientHello. This PQ KeyShare + * will be most preferred since it can be negotiated in 1-RTT (even if there are other mutually + * supported PQ KeyShares that the server would prefer over this one but would require 2-RTT's). + */ const struct s2n_kem_group *client_default = client_prefs->tls13_kem_groups[0]; PTR_ENSURE_REF(client_default); @@ -74,9 +75,10 @@ const struct s2n_ecc_named_curve *s2n_get_predicted_negotiated_ecdhe_curve(const PTR_ENSURE_REF(client_sec_policy); PTR_ENSURE_REF(server_sec_policy); - /* Client will offer their highest priority ECDHE KeyShare in their ClientHello. This KeyShare will be most preferred - * since it can be negotiated in 1-RTT (even if there are other mutually supported ECDHE KeyShares that the server would - * prefer over this one but would require 2-RTT's). */ + /* Client will offer their highest priority ECDHE KeyShare in their ClientHello. This KeyShare + * will be most preferred since it can be negotiated in 1-RTT (even if there are other mutually + * supported ECDHE KeyShares that the server would prefer over this one but would require 2-RTT's). + */ const struct s2n_ecc_named_curve *client_default = client_sec_policy->ecc_preferences->ecc_curves[0]; PTR_ENSURE_REF(client_default); @@ -644,7 +646,8 @@ int main() const struct s2n_ecc_named_curve *client_default = client_policy->ecc_preferences->ecc_curves[0]; const struct s2n_ecc_named_curve *predicted_curve = s2n_get_predicted_negotiated_ecdhe_curve(client_policy, server_policy); - /* If either policy doesn't support the default curve, fall back to p256 as it should be in common with every ECC preference list. */ + /* If either policy doesn't support the default curve, fall back to p256 as it should + * be in common with every ECC preference list. */ if (!s2n_ecc_preferences_includes_curve(client_policy->ecc_preferences, default_curve->iana_id) || !s2n_ecc_preferences_includes_curve(server_policy->ecc_preferences, default_curve->iana_id)) { EXPECT_TRUE(s2n_ecc_preferences_includes_curve(client_policy->ecc_preferences, s2n_ecc_curve_secp256r1.iana_id)); @@ -652,8 +655,8 @@ int main() curve = &s2n_ecc_curve_secp256r1; } - /* The client's preferred curve will be a higher priority than the default if both sides support TLS 1.3, - * and if the client's default can be chosen by the server in 1-RTT. */ + /* The client's preferred curve will be a higher priority than the default if both sides + * support TLS 1.3, and if the client's default can be chosen by the server in 1-RTT. */ if (s2n_security_policy_supports_tls13(client_policy) && s2n_security_policy_supports_tls13(server_policy) && s2n_ecc_preferences_includes_curve(server_policy->ecc_preferences, client_default->iana_id)) { curve = client_default; @@ -671,7 +674,8 @@ int main() const struct s2n_kem_group *predicted_kem_group = s2n_get_predicted_negotiated_kem_group(client_policy->kem_preferences, server_policy->kem_preferences); POSIX_ENSURE_REF(predicted_kem_group); - /* Confirm that the expected KEM Group listed in the test vector matches the output of s2n_get_predicted_negotiated_kem_group() */ + /* Confirm that the expected KEM Group listed in the test vector matches the output of + * s2n_get_predicted_negotiated_kem_group() */ POSIX_ENSURE_EQ(kem_group->iana_id, predicted_kem_group->iana_id); } diff --git a/tls/extensions/s2n_client_key_share.c b/tls/extensions/s2n_client_key_share.c index a48b9c67dc8..ad691c4fd5a 100644 --- a/tls/extensions/s2n_client_key_share.c +++ b/tls/extensions/s2n_client_key_share.c @@ -308,9 +308,9 @@ static int s2n_client_key_share_recv_pq_hybrid(struct s2n_connection *conn, stru return S2N_SUCCESS; } - struct s2n_kem_group_params *current_best_client_params = &conn->kex_params.client_kem_group_params; + struct s2n_kem_group_params *client_params = &conn->kex_params.client_kem_group_params; - const struct s2n_kem_group *new_best_match = NULL; + const struct s2n_kem_group *kem_group = NULL; for (size_t i = 0; i < kem_pref->tls13_kem_group_count; i++) { const struct s2n_kem_group *supported_group = kem_pref->tls13_kem_groups[i]; POSIX_ENSURE_REF(supported_group); @@ -323,7 +323,7 @@ static int s2n_client_key_share_recv_pq_hybrid(struct s2n_connection *conn, stru /* Stop if we reach the current highest priority share. * Any share of lower priority is discarded. */ - if (current_best_client_params->kem_group == supported_group) { + if (client_params->kem_group == supported_group) { break; } @@ -337,20 +337,20 @@ static int s2n_client_key_share_recv_pq_hybrid(struct s2n_connection *conn, stru /* Stop if we find a match */ if (kem_group_iana_id == supported_group->iana_id) { - new_best_match = supported_group; + kem_group = supported_group; break; } } /* Ignore unsupported KEM groups */ - if (!new_best_match) { + if (!kem_group) { return S2N_SUCCESS; } /* The length of the hybrid key share must be one of two possible lengths. Its internal values are either length * prefixed, or they are not. */ uint16_t actual_hybrid_share_size = key_share->blob.size; - uint16_t unprefixed_hybrid_share_size = new_best_match->curve->share_size + new_best_match->kem->public_key_length; + uint16_t unprefixed_hybrid_share_size = kem_group->curve->share_size + kem_group->kem->public_key_length; uint16_t prefixed_hybrid_share_size = (2 * S2N_SIZE_OF_KEY_SHARE_SIZE) + unprefixed_hybrid_share_size; /* Ignore KEM groups with unexpected overall total share sizes */ @@ -364,35 +364,35 @@ static int s2n_client_key_share_recv_pq_hybrid(struct s2n_connection *conn, stru /* Ignore KEM groups with unexpected ECC share sizes */ uint16_t ec_share_size = 0; POSIX_GUARD(s2n_stuffer_read_uint16(key_share, &ec_share_size)); - if (ec_share_size != new_best_match->curve->share_size) { + if (ec_share_size != kem_group->curve->share_size) { return S2N_SUCCESS; } } - DEFER_CLEANUP(struct s2n_kem_group_params new_best_client_params = { 0 }, s2n_kem_group_free); - new_best_client_params.kem_group = new_best_match; + DEFER_CLEANUP(struct s2n_kem_group_params new_client_params = { 0 }, s2n_kem_group_free); + new_client_params.kem_group = kem_group; /* Need to save whether the client included the length prefix so that we can match their behavior in our response. */ - new_best_client_params.kem_params.len_prefixed = is_hybrid_share_length_prefixed; + new_client_params.kem_params.len_prefixed = is_hybrid_share_length_prefixed; - POSIX_GUARD(s2n_client_key_share_parse_ecc(key_share, new_best_match->curve, &new_best_client_params.ecc_params)); + POSIX_GUARD(s2n_client_key_share_parse_ecc(key_share, kem_group->curve, &new_client_params.ecc_params)); /* If we were unable to parse the EC portion of the share, negotiated_curve * will be NULL, and we should ignore the entire key share. */ - if (!new_best_client_params.ecc_params.negotiated_curve) { + if (!new_client_params.ecc_params.negotiated_curve) { return S2N_SUCCESS; } /* Note: the PQ share size is validated in s2n_kem_recv_public_key() */ /* Ignore groups with PQ public keys we can't parse */ - new_best_client_params.kem_params.kem = new_best_match->kem; - if (s2n_kem_recv_public_key(key_share, &new_best_client_params.kem_params) != S2N_SUCCESS) { + new_client_params.kem_params.kem = kem_group->kem; + if (s2n_kem_recv_public_key(key_share, &new_client_params.kem_params) != S2N_SUCCESS) { return S2N_SUCCESS; } - POSIX_GUARD(s2n_kem_group_free(current_best_client_params)); - *current_best_client_params = new_best_client_params; + POSIX_GUARD(s2n_kem_group_free(client_params)); + *client_params = new_client_params; - ZERO_TO_DISABLE_DEFER_CLEANUP(new_best_client_params); + ZERO_TO_DISABLE_DEFER_CLEANUP(new_client_params); return S2N_SUCCESS; } @@ -424,7 +424,8 @@ static int s2n_client_key_share_recv(struct s2n_connection *conn, struct s2n_stu POSIX_GUARD(s2n_stuffer_read_uint16(extension, &share_size)); POSIX_ENSURE(s2n_stuffer_data_available(extension) >= share_size, S2N_ERR_BAD_MESSAGE); - POSIX_GUARD(s2n_blob_init(&key_share_blob, s2n_stuffer_raw_read(extension, share_size), share_size)); + POSIX_GUARD(s2n_blob_init(&key_share_blob, + s2n_stuffer_raw_read(extension, share_size), share_size)); POSIX_GUARD(s2n_stuffer_init(&key_share, &key_share_blob)); POSIX_GUARD(s2n_stuffer_skip_write(&key_share, share_size)); keyshare_count++;