address PR feedbacks

Author: Boquan Fang

crypto/s2n_ecc_evp.c

@@ -424,6 +424,7 @@ int s2n_ecc_evp_write_params_point(struct s2n_ecc_evp_params *ecc_evp_params, st
     }
 #else
     uint8_t point_len = 0;
+    struct s2n_blob point_blob = { 0 };
 
     DEFER_CLEANUP(EC_KEY *ec_key = EVP_PKEY_get1_EC_KEY(ecc_evp_params->evp_pkey), EC_KEY_free_pointer);
     S2N_ERROR_IF(ec_key == NULL, S2N_ERR_ECDHE_UNSUPPORTED_CURVE);
@@ -433,20 +434,17 @@ int s2n_ecc_evp_write_params_point(struct s2n_ecc_evp_params *ecc_evp_params, st
 
     POSIX_GUARD(s2n_ecc_evp_calculate_point_length(point, group, &point_len));
     S2N_ERROR_IF(point_len != ecc_evp_params->negotiated_curve->share_size, S2N_ERR_ECDHE_SERIALIZING);
-
-    /* Use a temporary stuffer copy to perform s2n_stuffer_raw_write, so the original stuffer won't be tainted */
-    POSIX_GUARD(s2n_stuffer_reserve_space(out, point_len));
-    struct s2n_stuffer copy = *out;
-
-    struct s2n_blob point_blob = { 0 };
-    point_blob.data = s2n_stuffer_raw_write(&copy, point_len);
+    point_blob.data = s2n_stuffer_raw_write(out, point_len);
     POSIX_ENSURE_REF(point_blob.data);
     point_blob.size = point_len;
 
     POSIX_GUARD(s2n_ecc_evp_write_point_data_snug(point, group, &point_blob));
 
-    out->write_cursor = copy.write_cursor;
-    out->high_water_mark = copy.high_water_mark;
+    /**
+     * point_blob.data is not accessable outside of this function.
+     * The stuffer is not tainted after it goes out of this function's scope.
+     */
+    out->tainted = false;
 #endif
     return 0;
 }
@@ -462,13 +460,6 @@ int s2n_ecc_evp_write_params(struct s2n_ecc_evp_params *ecc_evp_params, struct s
 
     uint8_t key_share_size = ecc_evp_params->negotiated_curve->share_size;
 
-    /* Use a temporary stuffer copy to perform s2n_stuffer_raw_write, so the original stuffer won't be tainted */
-    struct s2n_stuffer copy = *out;
-
-    /* Remember where the written data starts */
-    written->data = s2n_stuffer_raw_write(&copy, 0);
-    POSIX_ENSURE_REF(written->data);
-
     POSIX_GUARD(s2n_stuffer_write_uint8(out, TLS_EC_CURVE_TYPE_NAMED));
     POSIX_GUARD(s2n_stuffer_write_uint16(out, ecc_evp_params->negotiated_curve->iana_id));
     POSIX_GUARD(s2n_stuffer_write_uint8(out, key_share_size));
@@ -478,6 +469,18 @@ int s2n_ecc_evp_write_params(struct s2n_ecc_evp_params *ecc_evp_params, struct s
     /* key share + key share size (1) + iana (2) + curve type (1) */
     written->size = key_share_size + 4;
 
+    /* Write data after writing ecc evp params to avoid stuffer resize failure */
+    uint32_t read_cursor = out->read_cursor;
+    uint32_t written_data_location = s2n_stuffer_data_available(out) - written->size;
+
+    POSIX_GUARD(s2n_stuffer_rewrite(out));
+    POSIX_GUARD(s2n_stuffer_skip_write(out, written_data_location));
+    POSIX_GUARD(s2n_stuffer_skip_read(out, read_cursor));
+
+    /* Remember where the written data starts */
+    written->data = s2n_stuffer_raw_write(out, written->size);
+    POSIX_ENSURE_REF(written->data);
+
     return written->size;
 }
 

error/s2n_errno.c

@@ -139,6 +139,7 @@ static const char *no_such_error = "Internal s2n error";
     ERR_ENTRY(S2N_ERR_RESIZE_TAINTED_STUFFER, "cannot resize a tainted stuffer") \
     ERR_ENTRY(S2N_ERR_STUFFER_OUT_OF_DATA, "stuffer is out of data") \
     ERR_ENTRY(S2N_ERR_STUFFER_IS_FULL, "stuffer is full") \
+    ERR_ENTRY(S2N_ERR_STUFFER_IS_TAINTED, "stuffer is tainted") \
     ERR_ENTRY(S2N_ERR_STUFFER_NOT_FOUND, "stuffer expected bytes were not found") \
     ERR_ENTRY(S2N_ERR_STUFFER_HAS_UNPROCESSED_DATA, "stuffer has unprocessed data") \
     ERR_ENTRY(S2N_ERR_HASH_INVALID_ALGORITHM, "invalid hash algorithm") \

error/s2n_errno.h

@@ -170,6 +170,7 @@ typedef enum {
     S2N_ERR_RESIZE_TAINTED_STUFFER,
     S2N_ERR_STUFFER_OUT_OF_DATA,
     S2N_ERR_STUFFER_IS_FULL,
+    S2N_ERR_STUFFER_IS_TAINTED,
     S2N_ERR_STUFFER_NOT_FOUND,
     S2N_ERR_STUFFER_HAS_UNPROCESSED_DATA,
     S2N_ERR_HASH_INVALID_ALGORITHM,

tests/unit/s2n_client_hello_test.c

@@ -49,8 +49,6 @@
 #define MAX_CIPHER_SUITE_COUNT       (CIPHER_SUITES_MAX_LENGTH / S2N_TLS_CIPHER_SUITE_LEN)
 /* Drop 150 cipher suites from max, so that the total handshake message length won't exceed 64KB */
 #define REDUCED_CIPHER_SUITE_COUNT (MAX_CIPHER_SUITE_COUNT - NUM_OF_CIPHER_SUITES_TO_DROP)
-/* Reducing cipher suites by 150 creates approximately 300 bytes margin below maximum handshake length */
-#define ESTIMATED_MAX_HANDSHAKE_LENGTH_MARGIN (NUM_OF_CIPHER_SUITES_TO_DROP * S2N_TLS_CIPHER_SUITE_LEN)
 
 int s2n_parse_client_hello(struct s2n_connection *conn);
 S2N_RESULT s2n_client_hello_get_raw_extension(uint16_t extension_iana,
@@ -1963,9 +1961,6 @@ int main(int argc, char **argv)
 
     /* Test: Client Hello is slightly less than 64KB */
     {
-        DEFER_CLEANUP(struct s2n_config *client_config = s2n_config_new(), s2n_config_ptr_free);
-        EXPECT_NOT_NULL(client_config);
-
         DEFER_CLEANUP(struct s2n_config *server_config = s2n_config_new(), s2n_config_ptr_free);
         EXPECT_NOT_NULL(server_config);
 
@@ -1982,23 +1977,16 @@ int main(int argc, char **argv)
             .suites = test_cipher_suites,
         };
 
-        struct s2n_security_policy test_security_policy = {
-            .minimum_protocol_version = S2N_TLS12,
-            .cipher_preferences = &test_cipher_suites_preferences,
-            .kem_preferences = &kem_preferences_null,
-            .signature_preferences = &s2n_signature_preferences_20240501,
-            .ecc_preferences = &s2n_ecc_preferences_20240501,
-            .rules = {
-                    [S2N_PERFECT_FORWARD_SECRECY] = true,
-            },
-        };
+        const struct s2n_security_policy *default_policy = NULL;
+        EXPECT_SUCCESS(s2n_find_security_policy_from_version("default", &default_policy));
 
-        EXPECT_SUCCESS(s2n_config_disable_x509_verification(client_config));
+        /* Use default security policy but with custom cipher suites preferences */
+        struct s2n_security_policy test_security_policy = *default_policy;
+        test_security_policy.cipher_preferences = &test_cipher_suites_preferences;
 
         DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
                 s2n_connection_ptr_free);
         EXPECT_NOT_NULL(client);
-        EXPECT_SUCCESS(s2n_connection_set_config(client, client_config));
         EXPECT_SUCCESS(s2n_connection_set_blinding(client, S2N_SELF_SERVICE_BLINDING));
         client->security_policy_override = &test_security_policy;
 
@@ -2015,22 +2003,15 @@ int main(int argc, char **argv)
 
         EXPECT_OK(s2n_negotiate_test_server_and_client_until_message(server, client, SERVER_HELLO));
 
-        /* handshake.io shouldn't be tainted after sending and receiving large client hello */
-        EXPECT_TRUE(client->handshake.io.tainted == 0);
-        EXPECT_TRUE(server->handshake.io.tainted == 0);
-
         struct s2n_client_hello *client_hello = s2n_connection_get_client_hello(server);
         EXPECT_NOT_NULL(client_hello);
-        uint32_t handshake_max_len_margin = S2N_MAXIMUM_HANDSHAKE_MESSAGE_LENGTH - s2n_client_hello_get_raw_message_length(client_hello);
-        /* Size of Client Hello is less than 300 bytes from the maximum handshake length */
-        EXPECT_TRUE(handshake_max_len_margin < ESTIMATED_MAX_HANDSHAKE_LENGTH_MARGIN);
+
+        /* Size of Client Hello should be less than S2N_MAXIMUM_HANDSHAKE_MESSAGE_LENGTH*/
+        EXPECT_TRUE(s2n_client_hello_get_raw_message_length(client_hello) < S2N_MAXIMUM_HANDSHAKE_MESSAGE_LENGTH);
     }
 
     /* Test: Client Hello is larger than 64KB */
     {
-        DEFER_CLEANUP(struct s2n_config *client_config = s2n_config_new(), s2n_config_ptr_free);
-        EXPECT_NOT_NULL(client_config);
-
         DEFER_CLEANUP(struct s2n_config *server_config = s2n_config_new(), s2n_config_ptr_free);
         EXPECT_NOT_NULL(server_config);
 
@@ -2048,23 +2029,16 @@ int main(int argc, char **argv)
             .suites = test_cipher_suites,
         };
 
-        struct s2n_security_policy test_security_policy = {
-            .minimum_protocol_version = S2N_TLS12,
-            .cipher_preferences = &test_cipher_suites_preferences,
-            .kem_preferences = &kem_preferences_null,
-            .signature_preferences = &s2n_signature_preferences_20240501,
-            .ecc_preferences = &s2n_ecc_preferences_20240501,
-            .rules = {
-                    [S2N_PERFECT_FORWARD_SECRECY] = true,
-            },
-        };
+        const struct s2n_security_policy *default_policy = NULL;
+        EXPECT_SUCCESS(s2n_find_security_policy_from_version("default", &default_policy));
 
-        EXPECT_SUCCESS(s2n_config_disable_x509_verification(client_config));
+        /* Use default security policy but with custom cipher suites preferences */
+        struct s2n_security_policy test_security_policy = *default_policy;
+        test_security_policy.cipher_preferences = &test_cipher_suites_preferences;
 
         DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
                 s2n_connection_ptr_free);
         EXPECT_NOT_NULL(client);
-        EXPECT_SUCCESS(s2n_connection_set_config(client, client_config));
         EXPECT_SUCCESS(s2n_connection_set_blinding(client, S2N_SELF_SERVICE_BLINDING));
         client->security_policy_override = &test_security_policy;
 
@@ -2087,10 +2061,6 @@ int main(int argc, char **argv)
         /* The size of Client Hello exceeds S2N_MAXIMUM_HANDSHAKE_MESSAGE_LENGTH */
         EXPECT_TRUE(s2n_stuffer_data_available(&io_pair.server_in) > S2N_MAXIMUM_HANDSHAKE_MESSAGE_LENGTH);
         EXPECT_ERROR_WITH_ERRNO(s2n_negotiate_test_server_and_client_until_message(server, client, SERVER_HELLO), S2N_ERR_BAD_MESSAGE);
-
-        /* handshake.io shouldn't be tainted after sending and receiving large client hello */
-        EXPECT_TRUE(client->handshake.io.tainted == 0);
-        EXPECT_TRUE(server->handshake.io.tainted == 0);
     }
 
     EXPECT_SUCCESS(s2n_cert_chain_and_key_free(chain_and_key));

tls/extensions/s2n_extension_type.c

@@ -114,6 +114,12 @@ int s2n_extension_send(const s2n_extension_type *extension_type, struct s2n_conn
     /* Write extension data */
     POSIX_GUARD(extension_type->send(conn, out));
 
+    /**
+     * Stuffer might get tainted when sending extensions, while we can't find all of them at the moment.
+     * Catch tainted stuffer during testing only, so we won't break our customers in production.
+     */
+    POSIX_DEBUG_ENSURE(out->tainted, S2N_ERR_STUFFER_IS_TAINTED);
+
     /* Record extension size */
     POSIX_GUARD(s2n_stuffer_write_vector_size(&extension_size_bytes));