fix: Initial config influences client hello parsing (#4676)

aws · Aug 15, 2024 · 9cca574 · 9cca574
1 parent 2c8ae53
commit 9cca574
Show file tree

Hide file tree

Showing 7 changed files with 107 additions and 50 deletions.
diff --git a/error/s2n_errno.c b/error/s2n_errno.c
@@ -313,6 +313,7 @@ static const char *no_such_error = "Internal s2n error";
     ERR_ENTRY(S2N_ERR_INVALID_SERIALIZED_CONNECTION, "Serialized connection is invalid"); \
     ERR_ENTRY(S2N_ERR_TOO_MANY_CAS, "Too many certificate authorities in trust store"); \
     ERR_ENTRY(S2N_ERR_BAD_HEX, "Could not parse malformed hex string"); \
+    ERR_ENTRY(S2N_ERR_CONFIG_NULL_BEFORE_CH_CALLBACK, "Config set to NULL before client hello callback. This should not be possible outside of tests."); \
     /* clang-format on */
 
 #define ERR_STR_CASE(ERR, str) \

diff --git a/error/s2n_errno.h b/error/s2n_errno.h
@@ -239,6 +239,7 @@ typedef enum {
     S2N_ERR_OSSL_PROVIDER,
     S2N_ERR_BAD_HEX,
     S2N_ERR_TEST_ASSERTION,
+    S2N_ERR_CONFIG_NULL_BEFORE_CH_CALLBACK,
     S2N_ERR_T_INTERNAL_END,
 
     /* S2N_ERR_T_USAGE */

diff --git a/tests/testlib/s2n_sslv2_client_hello.h b/tests/testlib/s2n_sslv2_client_hello.h
@@ -15,7 +15,7 @@
 
 #pragma once
 
-/* The following macros can be concatanated to generate a SSLv2 ClientHello
+/* The following macros can be concatenated to generate a SSLv2 ClientHello
  * message and record 
  */
 

diff --git a/tests/unit/s2n_config_test.c b/tests/unit/s2n_config_test.c
@@ -1148,5 +1148,52 @@ int main(int argc, char **argv)
                     S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT);
         };
     };
+
+    /* Checks that servers don't use a config before the client hello callback is executed.
+     *
+     * We want to assert that a config is never used by a server until the client hello callback
+     * is called, given that users have the ability to swap out the config during this callback.
+     */
+    {
+        DEFER_CLEANUP(struct s2n_config *tls12_client_config = s2n_config_new(), s2n_config_ptr_free);
+        EXPECT_NOT_NULL(tls12_client_config);
+        /* Security policy that only supports TLS12 */
+        EXPECT_SUCCESS(s2n_config_set_cipher_preferences(tls12_client_config, "20240501"));
+
+        DEFER_CLEANUP(struct s2n_config *tls13_client_config = s2n_config_new(), s2n_config_ptr_free);
+        EXPECT_NOT_NULL(tls13_client_config);
+        /* Security policy that supports TLS13 */
+        EXPECT_SUCCESS(s2n_config_set_cipher_preferences(tls13_client_config, "20240503"));
+
+        struct s2n_config *config_arr[] = { tls12_client_config, tls13_client_config };
+
+        /* Checks that the handshake gets as far as the client hello callback with a NULL config */
+        for (size_t i = 0; i < s2n_array_len(config_arr); i++) {
+            DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT),
+                    s2n_connection_ptr_free);
+            EXPECT_NOT_NULL(client_conn);
+            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(client_conn, config_arr[i]));
+
+            /* Server config pointer is explicitly set to NULL */
+            server_conn->config = NULL;
+
+            DEFER_CLEANUP(struct s2n_test_io_stuffer_pair test_io = { 0 },
+                    s2n_io_stuffer_pair_free);
+            EXPECT_OK(s2n_io_stuffer_pair_init(&test_io));
+            EXPECT_OK(s2n_connections_set_io_stuffer_pair(client_conn, server_conn, &test_io));
+
+            /* S2N_ERR_CONFIG_NULL_BEFORE_CH_CALLBACK is only called in one location, just before the
+             * client hello callback. Therefore, we can assert that if we hit this error, we
+             * have gotten as far as the client hello callback without dereferencing the config.
+             */
+            EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn),
+                    S2N_ERR_CONFIG_NULL_BEFORE_CH_CALLBACK);
+        }
+    }
+
     END_TEST();
 }
diff --git a/tls/s2n_client_hello.c b/tls/s2n_client_hello.c
@@ -471,34 +471,6 @@ int s2n_parse_client_hello(struct s2n_connection *conn)
     conn->session_id_len = conn->client_hello.session_id.size;
     POSIX_CHECKED_MEMCPY(conn->session_id, conn->client_hello.session_id.data, conn->session_id_len);
 
-    /* Set default key exchange curve.
-     * This is going to be our fallback if the client has no preference.
-     *
-     * P-256 is our preferred fallback option because the TLS1.3 RFC requires
-     * all implementations to support it:
-     *
-     *     https://tools.ietf.org/rfc/rfc8446#section-9.1
-     *     A TLS-compliant application MUST support key exchange with secp256r1 (NIST P-256)
-     *     and SHOULD support key exchange with X25519 [RFC7748]
-     *
-     *= https://www.rfc-editor.org/rfc/rfc4492#section-4
-     *# A client that proposes ECC cipher suites may choose not to include these extensions.
-     *# In this case, the server is free to choose any one of the elliptic curves or point formats listed in Section 5.
-     *
-     */
-    const struct s2n_ecc_preferences *ecc_pref = NULL;
-    POSIX_GUARD(s2n_connection_get_ecc_preferences(conn, &ecc_pref));
-    POSIX_ENSURE_REF(ecc_pref);
-    POSIX_ENSURE_GT(ecc_pref->count, 0);
-    if (s2n_ecc_preferences_includes_curve(ecc_pref, TLS_EC_CURVE_SECP_256_R1)) {
-        conn->kex_params.server_ecc_evp_params.negotiated_curve = &s2n_ecc_curve_secp256r1;
-    } else {
-        /* If P-256 isn't allowed by the current security policy, instead choose
-         * the first / most preferred curve.
-         */
-        conn->kex_params.server_ecc_evp_params.negotiated_curve = ecc_pref->ecc_curves[0];
-    }
-
     POSIX_GUARD_RESULT(s2n_client_hello_verify_for_retry(conn,
             &previous_hello_retry, &conn->client_hello, previous_client_random));
     return S2N_SUCCESS;
@@ -565,6 +537,34 @@ int s2n_process_client_hello(struct s2n_connection *conn)
         conn->actual_protocol_version = MIN(conn->server_protocol_version, S2N_TLS12);
     }
 
+    /* Set default key exchange curve.
+     * This is going to be our fallback if the client has no preference.
+     *
+     * P-256 is our preferred fallback option because the TLS1.3 RFC requires
+     * all implementations to support it:
+     *
+     *     https://tools.ietf.org/rfc/rfc8446#section-9.1
+     *     A TLS-compliant application MUST support key exchange with secp256r1 (NIST P-256)
+     *     and SHOULD support key exchange with X25519 [RFC7748]
+     *
+     *= https://www.rfc-editor.org/rfc/rfc4492#section-4
+     *# A client that proposes ECC cipher suites may choose not to include these extensions.
+     *# In this case, the server is free to choose any one of the elliptic curves or point formats listed in Section 5.
+     *
+     */
+    const struct s2n_ecc_preferences *ecc_pref = NULL;
+    POSIX_GUARD(s2n_connection_get_ecc_preferences(conn, &ecc_pref));
+    POSIX_ENSURE_REF(ecc_pref);
+    POSIX_ENSURE_GT(ecc_pref->count, 0);
+    if (s2n_ecc_preferences_includes_curve(ecc_pref, TLS_EC_CURVE_SECP_256_R1)) {
+        conn->kex_params.server_ecc_evp_params.negotiated_curve = &s2n_ecc_curve_secp256r1;
+    } else {
+        /* If P-256 isn't allowed by the current security policy, instead choose
+         * the first / most preferred curve.
+         */
+        conn->kex_params.server_ecc_evp_params.negotiated_curve = ecc_pref->ecc_curves[0];
+    }
+
     POSIX_GUARD(s2n_extension_list_process(S2N_EXTENSION_LIST_CLIENT_HELLO, conn, &conn->client_hello.extensions));
 
     /* After parsing extensions, select a curve and corresponding keyshare to use */
@@ -594,7 +594,8 @@ int s2n_process_client_hello(struct s2n_connection *conn)
     POSIX_CHECKED_MEMCPY(previous_cipher_suite_iana, conn->secure->cipher_suite->iana_value, S2N_TLS_CIPHER_SUITE_LEN);
 
     /* Now choose the ciphers we have certs for. */
-    POSIX_GUARD(s2n_set_cipher_as_tls_server(conn, client_hello->cipher_suites.data, client_hello->cipher_suites.size / 2));
+    POSIX_GUARD(s2n_set_cipher_as_tls_server(conn, client_hello->cipher_suites.data,
+            client_hello->cipher_suites.size / 2));
 
     /* Check if this is the second client hello in a hello retry handshake */
     if (s2n_is_hello_retry_handshake(conn) && conn->handshake.message_number > 0) {
@@ -671,6 +672,12 @@ int s2n_client_hello_recv(struct s2n_connection *conn)
         /* Mark the client hello callback as invoked to avoid calling it again. */
         conn->client_hello.callback_invoked = true;
 
+        /* Do NOT move this null check. A test exists to assert that a server connection can get
+         * as far as the client hello callback without using its config. To do this we need a
+         * specific error for a null config just before the client hello callback. The test's
+         * assertions are weakened if this check is moved. */
+        POSIX_ENSURE(conn->config, S2N_ERR_CONFIG_NULL_BEFORE_CH_CALLBACK);
+
         /* Call client_hello_cb if exists, letting application to modify s2n_connection or swap s2n_config */
         if (conn->config->client_hello_cb) {
             int rc = conn->config->client_hello_cb(conn, conn->config->client_hello_cb_ctx);
@@ -854,7 +861,8 @@ int s2n_sslv2_client_hello_recv(struct s2n_connection *conn)
     /* Find potential certificate matches before we choose the cipher. */
     POSIX_GUARD(s2n_conn_find_name_matching_certs(conn));
 
-    POSIX_GUARD(s2n_set_cipher_as_sslv2_server(conn, client_hello->cipher_suites.data, client_hello->cipher_suites.size / S2N_SSLv2_CIPHER_SUITE_LEN));
+    POSIX_GUARD(s2n_set_cipher_as_sslv2_server(conn, client_hello->cipher_suites.data,
+            client_hello->cipher_suites.size / S2N_SSLv2_CIPHER_SUITE_LEN));
     POSIX_GUARD_RESULT(s2n_signature_algorithm_select(conn));
     POSIX_GUARD(s2n_select_certs_for_server_auth(conn, &conn->handshake_params.our_chain_and_key));
 

diff --git a/tls/s2n_connection.c b/tls/s2n_connection.c
@@ -1295,6 +1295,7 @@ S2N_CLEANUP_RESULT s2n_connection_apply_error_blinding(struct s2n_connection **c
         case S2N_ERR_CANCELLED:
         case S2N_ERR_CIPHER_NOT_SUPPORTED:
         case S2N_ERR_PROTOCOL_VERSION_UNSUPPORTED:
+        case S2N_ERR_CONFIG_NULL_BEFORE_CH_CALLBACK:
             RESULT_GUARD(s2n_connection_set_closed(*conn));
             break;
         default:

diff --git a/tls/s2n_handshake_io.c b/tls/s2n_handshake_io.c
@@ -1496,26 +1496,25 @@ static int s2n_handshake_read_io(struct s2n_connection *conn)
             return S2N_SUCCESS;
         }
 
-        s2n_cert_auth_type client_cert_auth_type;
-        POSIX_GUARD(s2n_connection_get_client_auth_type(conn, &client_cert_auth_type));
-
-        /* If client auth is optional, we initially assume it will not be requested.
-         * If we received a request, switch to a client auth handshake.
-         */
-        if (conn->mode == S2N_CLIENT
-                && client_cert_auth_type != S2N_CERT_AUTH_REQUIRED
-                && message_type == TLS_CERT_REQ) {
-            POSIX_ENSURE(client_cert_auth_type == S2N_CERT_AUTH_OPTIONAL, S2N_ERR_UNEXPECTED_CERT_REQUEST);
-            POSIX_ENSURE(IS_FULL_HANDSHAKE(conn), S2N_ERR_HANDSHAKE_STATE);
-            POSIX_GUARD_RESULT(s2n_handshake_type_set_flag(conn, CLIENT_AUTH));
-        }
+        if (conn->mode == S2N_CLIENT) {
+            s2n_cert_auth_type client_cert_auth_type = { 0 };
+            POSIX_GUARD(s2n_connection_get_client_auth_type(conn, &client_cert_auth_type));
+            /* If client auth is optional, we initially assume it will not be requested.
+             * If we received a request, switch to a client auth handshake.
+             */
+            if (client_cert_auth_type != S2N_CERT_AUTH_REQUIRED && message_type == TLS_CERT_REQ) {
+                POSIX_ENSURE(client_cert_auth_type == S2N_CERT_AUTH_OPTIONAL, S2N_ERR_UNEXPECTED_CERT_REQUEST);
+                POSIX_ENSURE(IS_FULL_HANDSHAKE(conn), S2N_ERR_HANDSHAKE_STATE);
+                POSIX_GUARD_RESULT(s2n_handshake_type_set_flag(conn, CLIENT_AUTH));
+            }
 
-        /* According to rfc6066 section 8, server may choose not to send "CertificateStatus" message even if it has
-         * sent "status_request" extension in the ServerHello message. */
-        if (conn->mode == S2N_CLIENT
-                && EXPECTED_MESSAGE_TYPE(conn) == TLS_SERVER_CERT_STATUS
-                && message_type != TLS_SERVER_CERT_STATUS) {
-            POSIX_GUARD_RESULT(s2n_handshake_type_unset_tls12_flag(conn, OCSP_STATUS));
+            /* According to rfc6066 section 8, the server may choose not to send a "CertificateStatus"
+             * message even if it has sent a "status_request" extension in the ServerHello message.
+             */
+            if (EXPECTED_MESSAGE_TYPE(conn) == TLS_SERVER_CERT_STATUS
+                    && message_type != TLS_SERVER_CERT_STATUS) {
+                POSIX_GUARD_RESULT(s2n_handshake_type_unset_tls12_flag(conn, OCSP_STATUS));
+            }
         }
 
         /*