Add HRR compliance comments and tests for TLS RFC section 4.2.8

tests/unit/s2n_client_early_data_indication_test.c

@@ -440,6 +440,9 @@ int main(int argc, char **argv)
             server_conn->handshake.message_number = 2;
             client_conn->handshake.message_number = 2;
 
+            /* Update the selected_group to ensure the HRR is valid */
+            client_conn->kex_params.client_ecc_evp_params.negotiated_curve = &s2n_ecc_curve_secp521r1;
+
             EXPECT_SUCCESS(s2n_server_hello_retry_send(server_conn));
             EXPECT_SUCCESS(s2n_stuffer_copy(&server_conn->handshake.io, &client_conn->handshake.io,
                     s2n_stuffer_data_available(&server_conn->handshake.io)));

tests/unit/s2n_client_hello_retry_test.c

@@ -1415,6 +1415,146 @@ 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));
+         EXPECT_SUCCESS(s2n_connection_set_blinding(server_conn, S2N_SELF_SERVICE_BLINDING));
+
+         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));
+         EXPECT_SUCCESS(s2n_connection_set_blinding(client_conn, S2N_SELF_SERVICE_BLINDING));
+
+         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;
+
+         /* Skip to before client receives ServerHello 2 */
+         s2n_blocked_status blocked = 0;
+         EXPECT_OK(s2n_negotiate_until_message(client_conn, &blocked, SERVER_HELLO));
+         EXPECT_OK(s2n_negotiate_until_message(server_conn, &blocked, HELLO_RETRY_MSG));
+         EXPECT_OK(s2n_negotiate_until_message(server_conn, &blocked, CLIENT_HELLO));
+         EXPECT_OK(s2n_negotiate_until_message(client_conn, &blocked, CLIENT_HELLO));
+         EXPECT_OK(s2n_negotiate_until_message(client_conn, &blocked, SERVER_HELLO));
+         EXPECT_OK(s2n_negotiate_until_message(server_conn, &blocked, SERVER_HELLO));
+         EXPECT_OK(s2n_negotiate_until_message(server_conn, &blocked, ENCRYPTED_EXTENSIONS));
+
+         /* 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_ERROR_WITH_ERRNO(s2n_negotiate_until_message(client_conn, &blocked, ENCRYPTED_EXTENSIONS),
+                                 S2N_ERR_BAD_MESSAGE);
+     }
+
      EXPECT_SUCCESS(s2n_disable_tls13_in_test());
 
     END_TEST();

tests/unit/s2n_client_key_share_extension_test.c

@@ -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;
@@ -306,7 +315,7 @@ int main(int argc, char **argv)
             EXPECT_SUCCESS(s2n_connection_free(conn));
         }
 
-        /* For HelloRetryRequests, verify that we can resend an existing share to reject early data. */
+        /* For HelloRetryRequests, verify that we cannot resend an existing share. */
         {
             struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT);
             EXPECT_NOT_NULL(conn);
@@ -332,13 +341,8 @@ int main(int argc, char **argv)
             conn->early_data_state = S2N_EARLY_DATA_REJECTED;
             conn->kex_params.server_ecc_evp_params.negotiated_curve = curve;
 
-            EXPECT_SUCCESS(s2n_client_key_share_extension.send(conn, &second_extension));
-            EXPECT_EQUAL(conn->kex_params.client_ecc_evp_params.negotiated_curve, curve);
-            EXPECT_NOT_NULL(conn->kex_params.client_ecc_evp_params.evp_pkey);
-
-            /* Same shares (same bytes) are written both times */
-            EXPECT_EQUAL(first_extension.write_cursor, second_extension.write_cursor);
-            EXPECT_BYTEARRAY_EQUAL(first_extension.blob.data, second_extension.blob.data, first_extension.write_cursor);
+            EXPECT_FAILURE_WITH_ERRNO(s2n_client_key_share_extension.send(conn, &second_extension),
+                    S2N_ERR_BAD_KEY_SHARE);
 
             EXPECT_SUCCESS(s2n_stuffer_free(&first_extension));
             EXPECT_SUCCESS(s2n_stuffer_free(&second_extension));

tests/unit/s2n_server_early_data_indication_test.c

@@ -281,6 +281,9 @@ int main(int argc, char **argv)
             server_conn->handshake.message_number = 2;
             client_conn->handshake.message_number = 2;
 
+            /* Update the selected_group to ensure the HRR is valid */
+            client_conn->kex_params.client_ecc_evp_params.negotiated_curve = &s2n_ecc_curve_secp521r1;
+
             EXPECT_SUCCESS(s2n_server_hello_retry_send(server_conn));
             EXPECT_SUCCESS(s2n_stuffer_copy(&server_conn->handshake.io, &client_conn->handshake.io,
                     s2n_stuffer_data_available(&server_conn->handshake.io)));

tests/unit/s2n_server_hello_retry_test.c

@@ -585,7 +585,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);
@@ -608,31 +616,6 @@ int main(int argc, char **argv)
             EXPECT_SUCCESS(s2n_connection_free(conn));
         }
 
-        /* Retry for early data is valid */
-        {
-            struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT);
-            EXPECT_NOT_NULL(conn);
-            EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(conn, "20201021"));
-            conn->actual_protocol_version = S2N_TLS13;
-            conn->secure.cipher_suite = &s2n_tls13_aes_256_gcm_sha384;
-
-            conn->kex_params.server_ecc_evp_params.negotiated_curve = test_curve;
-            conn->kex_params.client_ecc_evp_params.negotiated_curve = test_curve;
-            conn->kex_params.client_ecc_evp_params.evp_pkey = EVP_PKEY_new();
-            EXPECT_NOT_NULL(conn->kex_params.client_ecc_evp_params.evp_pkey);
-
-            /* Early data rejected: allow retry */
-            conn->early_data_state = S2N_EARLY_DATA_REJECTED;
-            EXPECT_SUCCESS(s2n_server_hello_retry_recv(conn));
-
-            /* Early data not rejected: do NOT allow retry */
-            conn->early_data_state = S2N_EARLY_DATA_NOT_REQUESTED;
-            EXPECT_FAILURE_WITH_ERRNO(s2n_server_hello_retry_recv(conn),
-                    S2N_ERR_INVALID_HELLO_RETRY);
-
-            EXPECT_SUCCESS(s2n_connection_free(conn));
-        }
-
         /* Retry for multiple reasons is valid */
         {
             struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT);

tls/extensions/s2n_client_key_share.c

@@ -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];
@@ -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];
@@ -175,6 +189,16 @@ static int s2n_generate_default_pq_hybrid_key_share(struct s2n_connection *conn,
 
 static int s2n_client_key_share_send(struct s2n_connection *conn, struct s2n_stuffer *out)
 {
+    if (s2n_is_hello_retry_handshake(conn)) {
+        const struct s2n_ecc_named_curve *server_curve = conn->kex_params.server_ecc_evp_params.negotiated_curve;
+        const struct s2n_ecc_named_curve *client_curve = conn->kex_params.client_ecc_evp_params.negotiated_curve;
+        const struct s2n_kem_group *server_group = conn->kex_params.server_kem_group_params.kem_group;
+        const struct s2n_kem_group *client_group = conn->kex_params.client_kem_group_params.kem_group;
+
+        /* Ensure a new key share will be sent after a hello retry request */
+        POSIX_ENSURE(server_curve != client_curve || server_group != client_group, S2N_ERR_BAD_KEY_SHARE);
+    }
+
     struct s2n_stuffer_reservation shares_size = {0};
     POSIX_GUARD(s2n_stuffer_reserve_uint16(out, &shares_size));
     POSIX_GUARD(s2n_generate_default_pq_hybrid_key_share(conn, out));

tls/extensions/s2n_server_key_share.c

@@ -238,7 +238,24 @@ 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;
-    server_ecc_evp_params->negotiated_curve = ecc_pref->ecc_curves[supported_curve_index];
+    const struct s2n_ecc_named_curve *negotiated_curve = ecc_pref->ecc_curves[supported_curve_index];
+
+    /**
+     *= 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);
+        const struct s2n_ecc_named_curve *previous_negotiated_curve = server_ecc_evp_params->negotiated_curve;
+        POSIX_ENSURE(negotiated_curve == previous_negotiated_curve,
+                     S2N_ERR_BAD_MESSAGE);
+    }
+
+    server_ecc_evp_params->negotiated_curve = negotiated_curve;
 
     /* If this is a HelloRetryRequest, we won't have a key share. We just have the selected group.
      * Set the server negotiated curve and exit early so a proper keyshare can be generated. */