From 46ae0e06466c079d6ced966aede167e2ded06046 Mon Sep 17 00:00:00 2001 From: Alex Weibel Date: Wed, 12 Jun 2024 15:08:50 -0700 Subject: [PATCH] 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; }