Skip to content

Commit

Permalink
Address Feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@alexw91
alexw91 committed Jun 19, 2024
1 parent 0fa4c71 commit 4475e17
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 27 deletions.
24 changes: 14 additions & 10 deletions tests/unit/s2n_tls13_pq_handshake_test.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -644,16 +646,17 @@ 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));
EXPECT_TRUE(s2n_ecc_preferences_includes_curve(server_policy->ecc_preferences, s2n_ecc_curve_secp256r1.iana_id));
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;
Expand All @@ -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);
}

Expand Down
34 changes: 17 additions & 17 deletions tls/extensions/s2n_client_key_share.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
}

Expand All @@ -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 */
Expand All @@ -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;
}

Expand Down

0 comments on commit 4475e17

Please sign in to comment.