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.

Sign up for GitHub

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

Add HRR compliance comments and tests for TLS RFC section 4.2.8 #3362

Merged
merged 14 commits into from
Jul 6, 2022
Merged

Add HRR compliance comments and tests for TLS RFC section 4.2.8 #3362

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
707cad8
Add HRR compliance comments and tests for TLS RFC section 4.2.8
goatgoose Jun 14, 2022
2e43c0a
use s2n_negotiate_until_message to make test shorter
goatgoose Jun 21, 2022
df36987
remove duplicate selected_group check
goatgoose Jun 21, 2022
ea0a1f9
make the new vs old negotiated curve more clear
goatgoose Jun 22, 2022
5725d38
more s2n_negotiate_until_messages
goatgoose Jun 22, 2022
0df8df9
fix linter
goatgoose Jun 22, 2022
36cf6a7
remove exception for early data on hrr recv
goatgoose Jun 23, 2022
75009a9
remove test for hrr early data exception
goatgoose Jun 23, 2022
bb0302b
add compliance comment for error on check for (1)
goatgoose Jun 23, 2022
18f74e7
add check (1) to s2n_server_hello_retry_recv
goatgoose Jun 30, 2022
88dc849
check for duplicate key share before sending
goatgoose Jul 1, 2022
386e42e
simplify test in client_hello_retry_test
goatgoose Jul 6, 2022
9373fc8
move compliance comment to where handshake is aborted
goatgoose Jul 6, 2022
c1e9166
Merge branch 'main' into hrr-doc-4-2-8
goatgoose Jul 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 166 additions & 0 deletions tests/unit/s2n_client_hello_retry_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1388,6 +1388,172 @@ int main(int argc, char **argv)
S2N_ERR_BAD_MESSAGE);
}

/**
*= https://tools.ietf.org/rfc/rfc8446#4.2.8
*= type=test
*# Upon receipt of this extension in a HelloRetryRequest, the client
*# MUST verify that (1) the selected_group field corresponds to a group
*# which was provided in the "supported_groups" extension in the
*# original ClientHello
**/
{
/* Create a custom security policy without secp521r1 */
const struct s2n_ecc_named_curve *const test_ecc_pref_list_for_retry[] = {
&s2n_ecc_curve_secp256r1,
&s2n_ecc_curve_secp384r1,
};
const struct s2n_ecc_preferences test_ecc_preferences_for_retry = {
.count = s2n_array_len(test_ecc_pref_list_for_retry),
.ecc_curves = test_ecc_pref_list_for_retry,
};
struct s2n_security_policy security_policy_test_tls13_retry_temp = {
.minimum_protocol_version = S2N_TLS10,
.cipher_preferences = &cipher_preferences_20190801,
.kem_preferences = &kem_preferences_null,
.signature_preferences = &s2n_signature_preferences_20200207,
.certificate_signature_preferences = &s2n_certificate_signature_preferences_20201110,
.ecc_preferences = &test_ecc_preferences_for_retry,
};

DEFER_CLEANUP(struct s2n_cert_chain_and_key *chain_and_key,
s2n_cert_chain_and_key_ptr_free);
EXPECT_SUCCESS(s2n_test_cert_chain_and_key_new(&chain_and_key,
S2N_DEFAULT_ECDSA_TEST_CERT_CHAIN,
S2N_DEFAULT_ECDSA_TEST_PRIVATE_KEY));

DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(),
s2n_config_ptr_free);
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, chain_and_key));
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "default_tls13"));
EXPECT_SUCCESS(s2n_config_disable_x509_verification(config));

DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER),
s2n_connection_ptr_free);
EXPECT_NOT_NULL(server_conn);
EXPECT_SUCCESS(s2n_connection_set_config(server_conn, config));

DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT),
s2n_connection_ptr_free);
EXPECT_NOT_NULL(client_conn);
EXPECT_SUCCESS(s2n_connection_set_config(client_conn, config));

struct s2n_test_io_pair io_pair = { 0 };
EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair));
EXPECT_SUCCESS(s2n_connections_set_io_pair(client_conn, server_conn, &io_pair));

/* Force the HRR path */
client_conn->security_policy_override = &security_policy_test_tls13_retry_temp;

/* ClientHello 1 */
EXPECT_SUCCESS(s2n_client_hello_send(client_conn));
EXPECT_SUCCESS(s2n_stuffer_copy(&client_conn->handshake.io, &server_conn->handshake.io,
s2n_stuffer_data_available(&client_conn->handshake.io)));

/* Server receives ClientHello 1 */
EXPECT_SUCCESS(s2n_client_hello_recv(server_conn));
EXPECT_SUCCESS(s2n_set_connection_hello_retry_flags(server_conn));

/* Server sends HelloRetryRequest */
EXPECT_SUCCESS(s2n_server_hello_retry_send(server_conn));

EXPECT_SUCCESS(s2n_stuffer_wipe(&client_conn->handshake.io));
EXPECT_SUCCESS(s2n_stuffer_copy(&server_conn->handshake.io, &client_conn->handshake.io,
s2n_stuffer_data_available(&server_conn->handshake.io)));
client_conn->handshake.message_number = HELLO_RETRY_MSG_NO;

/* Set the curve to secp521r1, which was not provided in supported_groups */
client_conn->kex_params.server_ecc_evp_params.negotiated_curve = &s2n_ecc_curve_secp521r1;

/* Client receives HelloRetryRequest */
EXPECT_FAILURE_WITH_ERRNO(s2n_server_hello_recv(client_conn),
S2N_ERR_INVALID_HELLO_RETRY);
}

/**
*= https://tools.ietf.org/rfc/rfc8446#4.2.8
*= type=test
*# If using (EC)DHE key establishment and a HelloRetryRequest containing a
*# "key_share" extension was received by the client, the client MUST
*# verify that the selected NamedGroup in the ServerHello is the same as
*# that in the HelloRetryRequest. If this check fails, the client MUST
*# abort the handshake with an "illegal_parameter" alert.
**/
{
DEFER_CLEANUP(struct s2n_cert_chain_and_key *chain_and_key,
s2n_cert_chain_and_key_ptr_free);
EXPECT_SUCCESS(s2n_test_cert_chain_and_key_new(&chain_and_key,
S2N_DEFAULT_ECDSA_TEST_CERT_CHAIN,
S2N_DEFAULT_ECDSA_TEST_PRIVATE_KEY));

DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(),
s2n_config_ptr_free);
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, chain_and_key));
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "default_tls13"));
EXPECT_SUCCESS(s2n_config_disable_x509_verification(config));

DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER),
s2n_connection_ptr_free);
EXPECT_NOT_NULL(server_conn);
EXPECT_SUCCESS(s2n_connection_set_config(server_conn, config));

DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT),
s2n_connection_ptr_free);
EXPECT_NOT_NULL(client_conn);
EXPECT_SUCCESS(s2n_connection_set_config(client_conn, config));

struct s2n_test_io_pair io_pair = { 0 };
EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair));
EXPECT_SUCCESS(s2n_connections_set_io_pair(client_conn, server_conn, &io_pair));

/* Force the HRR path */
client_conn->security_policy_override = &security_policy_test_tls13_retry;

/* ClientHello 1 */
EXPECT_SUCCESS(s2n_client_hello_send(client_conn));
EXPECT_SUCCESS(s2n_stuffer_copy(&client_conn->handshake.io, &server_conn->handshake.io,
s2n_stuffer_data_available(&client_conn->handshake.io)));

/* Server receives ClientHello 1 */
EXPECT_SUCCESS(s2n_client_hello_recv(server_conn));
EXPECT_SUCCESS(s2n_set_connection_hello_retry_flags(server_conn));

/* Server sends HelloRetryRequest */
EXPECT_SUCCESS(s2n_server_hello_retry_send(server_conn));

EXPECT_SUCCESS(s2n_stuffer_wipe(&client_conn->handshake.io));
EXPECT_SUCCESS(s2n_stuffer_copy(&server_conn->handshake.io, &client_conn->handshake.io,
s2n_stuffer_data_available(&server_conn->handshake.io)));
client_conn->handshake.message_number = 1; // hello retry

/* Client receives HelloRetryRequest */
EXPECT_SUCCESS(s2n_server_hello_recv(client_conn));

/* Client sends ClientHello 2 */
EXPECT_SUCCESS(s2n_client_hello_send(client_conn));

EXPECT_SUCCESS(s2n_stuffer_wipe(&server_conn->handshake.io));
EXPECT_SUCCESS(s2n_stuffer_copy(&client_conn->handshake.io, &server_conn->handshake.io,
s2n_stuffer_data_available(&client_conn->handshake.io)));
server_conn->handshake.message_number = 2; // client hello

/* Server receives ClientHello 2 */
EXPECT_SUCCESS(s2n_client_hello_recv(server_conn));

/* Server sends ServerHello 2 */
EXPECT_SUCCESS(s2n_server_hello_send(server_conn));

EXPECT_SUCCESS(s2n_stuffer_wipe(&client_conn->handshake.io));
EXPECT_SUCCESS(s2n_stuffer_copy(&server_conn->handshake.io, &client_conn->handshake.io,
s2n_stuffer_data_available(&server_conn->handshake.io)));
client_conn->handshake.message_number = 3; // server hello

/* Set the negotiated curve to something other than what was sent in the HRR */
client_conn->kex_params.server_ecc_evp_params.negotiated_curve = &s2n_ecc_curve_secp521r1;

/* Client receives ServerHello 2 */
EXPECT_FAILURE_WITH_ERRNO(s2n_server_hello_recv(client_conn), S2N_ERR_BAD_MESSAGE);
}

EXPECT_SUCCESS(s2n_disable_tls13_in_test());

END_TEST();
Expand Down
13 changes: 11 additions & 2 deletions tests/unit/s2n_client_key_share_extension_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,17 @@ int main(int argc, char **argv)

/* Test s2n_client_key_share_extension.send with HelloRetryRequest */
{
/* For HelloRetryRequests when a keyshare does not match, test that s2n_client_key_share_extension.send replaces the list of keyshares,
* with a list containing a single KeyShareEntry for the server selected group. */
/**
* For HelloRetryRequests when a keyshare does not match, test that s2n_client_key_share_extension.send replaces
* the list of keyshares with a list containing a single KeyShareEntry for the server selected group.
*
*= https://tools.ietf.org/rfc/rfc8446#4.2.8
*= type=test
*# Otherwise, when sending the new ClientHello, the client MUST
*# replace the original "key_share" extension with one containing only a
*# new KeyShareEntry for the group indicated in the selected_group field
*# of the triggering HelloRetryRequest.
**/
if (s2n_is_evp_apis_supported()) {
struct s2n_connection *conn;
struct s2n_config *config;
Expand Down
10 changes: 9 additions & 1 deletion tests/unit/s2n_server_hello_retry_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,15 @@ int main(int argc, char **argv)
EXPECT_NOT_NULL(security_policy);
const struct s2n_ecc_named_curve *test_curve = security_policy->ecc_preferences->ecc_curves[0];

/* Retry for key share is valid */
/**
* Retry for key share is valid
*
*= https://tools.ietf.org/rfc/rfc8446#4.2.8
*= type=test
*# and (2) the selected_group field does not
*# correspond to a group which was provided in the "key_share" extension
*# in the original ClientHello.
**/
{
struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT);
EXPECT_NOT_NULL(conn);
Expand Down
14 changes: 14 additions & 0 deletions tls/extensions/s2n_client_key_share.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@ static int s2n_generate_default_ecc_key_share(struct s2n_connection *conn, struc
POSIX_GUARD(s2n_ecc_evp_params_free(client_params));
}

/**
*= https://tools.ietf.org/rfc/rfc8446#4.2.8
*# Otherwise, when sending the new ClientHello, the client MUST
*# replace the original "key_share" extension with one containing only a
*# new KeyShareEntry for the group indicated in the selected_group field
*# of the triggering HelloRetryRequest.
**/
client_params->negotiated_curve = server_curve;
} else {
client_params->negotiated_curve = ecc_pref->ecc_curves[0];
Expand Down Expand Up @@ -164,6 +171,13 @@ static int s2n_generate_default_pq_hybrid_key_share(struct s2n_connection *conn,
POSIX_GUARD(s2n_kem_group_free(client_params));
}

/**
*= https://tools.ietf.org/rfc/rfc8446#4.2.8
*# Otherwise, when sending the new ClientHello, the client MUST
*# replace the original "key_share" extension with one containing only a
*# new KeyShareEntry for the group indicated in the selected_group field
*# of the triggering HelloRetryRequest.
**/
client_params->kem_group = server_group;
} else {
client_params->kem_group = kem_pref->tls13_kem_groups[0];
Expand Down
15 changes: 15 additions & 0 deletions tls/extensions/s2n_server_key_share.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,21 @@ static int s2n_server_key_share_recv_ecc(struct s2n_connection *conn, uint16_t n
}

struct s2n_ecc_evp_params *server_ecc_evp_params = &conn->kex_params.server_ecc_evp_params;

/**
*= https://tools.ietf.org/rfc/rfc8446#4.2.8
*# If using (EC)DHE key establishment and a HelloRetryRequest containing a
*# "key_share" extension was received by the client, the client MUST
*# verify that the selected NamedGroup in the ServerHello is the same as
*# that in the HelloRetryRequest. If this check fails, the client MUST
*# abort the handshake with an "illegal_parameter" alert.
**/
if (s2n_is_hello_retry_handshake(conn) && !s2n_is_hello_retry_message(conn)) {
POSIX_ENSURE_REF(server_ecc_evp_params->negotiated_curve);
POSIX_ENSURE(ecc_pref->ecc_curves[supported_curve_index] == server_ecc_evp_params->negotiated_curve,
S2N_ERR_BAD_MESSAGE);
}

server_ecc_evp_params->negotiated_curve = ecc_pref->ecc_curves[supported_curve_index];

/* If this is a HelloRetryRequest, we won't have a key share. We just have the selected group.
Expand Down
50 changes: 40 additions & 10 deletions tls/s2n_server_hello_retry.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,20 +68,42 @@ int s2n_server_hello_retry_recv(struct s2n_connection *conn)
POSIX_GUARD(s2n_connection_get_kem_preferences(conn, &kem_pref));
POSIX_ENSURE_REF(kem_pref);

/* Upon receipt of the HelloRetryRequest, the client MUST verify that:
* (1) the selected_group field corresponds to a group
* which was provided in the "supported_groups" extension in the
* original ClientHello and
* (2) the selected_group field does not correspond to a group which was provided
* in the "key_share" extension in the original ClientHello.
* If either of these checks fails, then the client MUST abort the handshake. */

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;

/* 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);

/**
*= https://tools.ietf.org/rfc/rfc8446#4.2.8
*# Upon receipt of this extension in a HelloRetryRequest, the client
*# MUST verify that (1) the selected_group field corresponds to a group
*# which was provided in the "supported_groups" extension in the
*# original ClientHello
**/
bool selected_group_in_supported_groups = false;
if (named_curve != NULL) {
for (size_t i = 0; i < ecc_pref->count; i++) {
if (named_curve->iana_id == ecc_pref->ecc_curves[i]->iana_id) {
selected_group_in_supported_groups = true;
}
}
}
if (kem_group != NULL) {
for (size_t i = 0; i < kem_pref->tls13_kem_group_count; i++) {
if (kem_group->iana_id == kem_pref->tls13_kem_groups[i]->iana_id) {
selected_group_in_supported_groups = true;
break;
}
}
}

/**
*= https://tools.ietf.org/rfc/rfc8446#4.2.8
*# and (2) the selected_group field does not
*# correspond to a group which was provided in the "key_share" extension
*# 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);
Expand All @@ -93,14 +115,22 @@ int s2n_server_hello_retry_recv(struct s2n_connection *conn)
new_key_share_requested = (kem_group != conn->kex_params.client_kem_group_params.kem_group);
}

/*
/**
*= https://tools.ietf.org/rfc/rfc8446#section-4.1.4
*# Clients MUST abort the handshake with an
*# "illegal_parameter" alert if the HelloRetryRequest would not result
*# in any change in the ClientHello.
*/
*
*= https://tools.ietf.org/rfc/rfc8446#4.2.8
*= type=exception
*= reason=Permit HRRs to leave the selected_group field unmodified if sent due to rejecting early data
*# If either of these checks fails, then
*# the client MUST abort the handshake with an "illegal_parameter"
*# alert.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with early data, don't you need to offer a correct selected group? Why is there an exception for early data?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a correct selected group still needs to be offered. This exception is for this line:

s2n-tls/tls/s2n_server_hello_retry.c

Lines 107 to 108 in 5725d38

POSIX_ENSURE((conn->early_data_state == S2N_EARLY_DATA_REJECTED) || new_key_share_requested,
S2N_ERR_INVALID_HELLO_RETRY);

Without the exception, I think there should not be a check for conn->early_data_state == S2N_EARLY_DATA_REJECTED. It seems like this condition is here to allow for HRRs to not modify the selected_group field if the HRR was sent only for the purpose of rejecting early data. I think this makes sense, because if you're only rejecting early data it wouldn't make sense to also need to update the selected_group. However, this condition does not seem to be present in the RFC.

Here is the relevant part:

   Upon receipt of this extension in a HelloRetryRequest, the client
   MUST verify that (1) the selected_group field corresponds to a group
   which was provided in the "supported_groups" extension in the
   original ClientHello and (2) the selected_group field does not
   correspond to a group which was provided in the "key_share" extension
   in the original ClientHello.  If either of these checks fails, then
   the client MUST abort the handshake with an "illegal_parameter"
   alert.

Checks (1) and (2) must both succeed. And, according to check (2), new_key_share_requested should always be true. So, the exception I added is for this statement:

   If either of these checks fails, then
   the client MUST abort the handshake with an "illegal_parameter"
   alert.

Since we allow check (2) to be false if early data was rejected.

Let me know if I'm misunderstanding something or if we actually should not be making this exception!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the misunderstanding comes from the fact that the HelloRetryRequest doesn't have to include a keyshare extension. A server who is using a HRR to reject early data would not include a keyshare extension in that request. So the client doesn't need to make an exception in this case. Basically, if the server includes a keyshare extension in its HRR, it has to be asking for a keyshare change, otherwise the server can just leave that extension out.

So what I'm saying is, at minimum, there is no exception to the RFC. However, the existing code might need to be rewritten for the case where the server uses HRR to reject early data. 😪 @lrstewart do you know if we have a test for this?

This paragraph shows that the keyshare extension is optional in a HRR:

  • If a "key_share" extension was supplied in the HelloRetryRequest,
    replacing the list of shares with a list containing a single
    KeyShareEntry from the indicated group.

Copy link
Contributor Author

@goatgoose goatgoose Jun 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it okay. That makes sense. I was thinking that the extension was required for some reason.

So would the change be to check to see if a key share extension was received in s2n_server_hello_retry_recv, and if it was, ensure that check (2) succeeds here, and if it wasn't received, only check to see if conn->early_data_state == S2N_EARLY_DATA_REJECTED?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what I would do is gate line 71-107 with the check that the client actually received a keyshare extension. And then you can remove the early data rejected clause from the posix_ensure statement.

It seems like we have a test for this situation: https://github.com/aws/s2n-tls/blob/main/tests/unit/s2n_client_early_data_indication_test.c#L412.
But I think this test is flawed, because it's not appropriately mocking how a server would send a HRR if it wanted to reject early data. The server in this case should write the HRR without the keyshare ext.
Basically, additionally, I think we need to change the test so that the server excludes the keyshare ext.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at bouncy castle and openssl, it seems like implementations require a key share extension and don't provide an exception for early data. I therefor removed the exception, and fixed two failing tests that relied on not needing to update the selected_group: 36cf6a7

I also removed a test that checked for this specific exception that no longer exists: 75009a9

**/
POSIX_ENSURE((conn->early_data_state == S2N_EARLY_DATA_REJECTED) || new_key_share_requested,
S2N_ERR_INVALID_HELLO_RETRY);
POSIX_ENSURE(selected_group_in_supported_groups, S2N_ERR_INVALID_HELLO_RETRY);

/* Update transcript hash */
POSIX_GUARD(s2n_server_hello_retry_recreate_transcript(conn));
Expand Down