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

@@ -1388,6 +1388,151 @@ 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));
+
+         /* Set the curve to secp521r1, which was not provided in supported_groups */
+         server_conn->kex_params.server_ecc_evp_params.negotiated_curve = &s2n_ecc_curve_secp521r1;
+
+         /* 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;
+
+         /* Client receives HelloRetryRequest */
+         EXPECT_FAILURE_WITH_ERRNO(s2n_server_hello_recv(client_conn),
+                                   S2N_ERR_ECDHE_UNSUPPORTED_CURVE);
+     }
+
+     /**
+     *= 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;
+
+         /* Skip to before server sends 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));
+
+         /* 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)));
+         int server_hello = 3;
+         client_conn->handshake.message_number = 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();

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;

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

@@ -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);
@@ -604,31 +612,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];

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. */
@@ -290,11 +307,24 @@ static int s2n_server_key_share_recv(struct s2n_connection *conn, struct s2n_stu
     POSIX_GUARD(s2n_connection_get_ecc_preferences(conn, &ecc_pref));
     POSIX_ENSURE_REF(ecc_pref);
 
+    /**
+     *= 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
+     **/
     if (s2n_ecc_preferences_includes_curve(ecc_pref, negotiated_named_group_iana)) {
         POSIX_GUARD(s2n_server_key_share_recv_ecc(conn, negotiated_named_group_iana, extension));
     } else if (s2n_kem_preferences_includes_tls13_kem_group(kem_pref, negotiated_named_group_iana)) {
         POSIX_GUARD(s2n_server_key_share_recv_pq_hybrid(conn, negotiated_named_group_iana, extension));
     } else {
+        /**
+         *= https://tools.ietf.org/rfc/rfc8446#4.2.8
+         *# If either of these checks fails, then
+         *# the client MUST abort the handshake with an "illegal_parameter"
+         *# alert.
+         **/
         POSIX_BAIL(S2N_ERR_ECDHE_UNSUPPORTED_CURVE);
     }
 

tls/s2n_server_hello_retry.c

@@ -68,20 +68,18 @@ 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
+     *# 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);
@@ -93,14 +91,18 @@ 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.
-     */
-    POSIX_ENSURE((conn->early_data_state == S2N_EARLY_DATA_REJECTED) || new_key_share_requested,
-            S2N_ERR_INVALID_HELLO_RETRY);
+     *
+     *= https://tools.ietf.org/rfc/rfc8446#4.2.8
+     *# If either of these checks fails, then
+     *# the client MUST abort the handshake with an "illegal_parameter"
+     *# alert.
+     **/
+    POSIX_ENSURE(new_key_share_requested, S2N_ERR_INVALID_HELLO_RETRY);
 
     /* Update transcript hash */
     POSIX_GUARD(s2n_server_hello_retry_recreate_transcript(conn));