fix: tainted handshake.io and add large client hello test #5208

boquan-fang · 2025-03-19T23:17:37Z

Release Summary:

Resolved issues:

Description of changes:

Replace s2n_stuffer_raw_write() in s2n_ecc_evp_write_params(). Use a stuffer copy to perform raw write, so that the original stuffer won't get tainted.
- We need to do a s2n_stuffer_reserve_space() before creating a copy, so that the copy stuffer won't realloc memory during the raw write operation.
- We need to update the original stuffer with the copy stuffer's write_cursor and high_water_mark since they might get changed.
- Same operation in s2n_ecc_evpo_write_params(), but we don't reserve any additional space or updating any other stuffer's meta data, because the data_len is zero.
Change cipher preferences count from uint8_t to uint16_t. The RFC specifies that the total cipher suites size can be (2^16 - 2).
Add two tests in s2n_client_hello_test.c. One to generate a Client Hello that's close the maximum handshake length and another to generate a Client Hello that's larger than the maximum length.
- We set up two custom security policies with ciphersuites preferences of lots of repeating cipher suites. That can enlarge the Client Hello size.
- In the test, I have verify that handshake.io is not tainted, and the size of Client Hello is either less than 300 bytes from the maximum handshake length or exceeds the maximum handshake length.

Call-outs:

This PR is a step to verify that server will drop large (capped at 64KB) handshake message. The next step, according to the issue, is to merge this PR and fix: handshake message length integer overflow in s2n_handshake_finish_header #5206. And then do a S2N-TLS rust bindings release. Specific steps can be track in Provide an interface for TLS to communicate buffering limits s2n-quic#332 (comment).
We shouldn't use s2n_raw_write or s2n_raw_read unless we have no other ways. There are many other places in S2N-TLS where we used those two functions. We should probably do a check for all of them in the future to see if we can change all of them.

If a user appends counts of UINT16_MAX cipher suites, then the total cipher suites length would be (2^17 - 2) bytes. Our Client Hello send function will error on that, since it only reserve 16 bytes to write all cipher suites information:

s2n-tls/tls/s2n_client_hello.c

Lines 750 to 751 in 03a70fc

    
           struct s2n_stuffer_reservation available_cipher_suites_size; 
        
           POSIX_GUARD(s2n_stuffer_reserve_uint16(out, &available_cipher_suites_size));

I defined some macros in tests/unit/s2n_client_hello_test.c. I would like some feedbacks on naming and how to make the math computations better.
In tls/extensions/s2n_extension_type.c, I checked every time that we call s2n_extension_list_send in our codebase (except testing), the out stuffer is always handshake.io in s2n_connection struct. handshake.io will always be wiped at the end of each handshake message. That means the data in handshake.io is temporary. Therefore, it is safe to mark the out stuffer as not tainted after the extension is sent.
- I left a comment in tls/extensions/s2n_extension_type.c when I am reseting tainted to false. Please let me know if the comment is not clear.

Testing:

When the unit tests in the CI are testing with openssl-1.1.1 and openssl-3.0, EVP_APIS_SUPPORTED will be enabled. When the unit tests in CI are testing with openssl-1.0.2, EVP_APIS_SUPPORTED will be disabled. Hence, the CI will test for changes for both situations.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

lrstewart · 2025-03-20T17:48:31Z

crypto/s2n_ecc_evp.c


+    POSIX_GUARD(s2n_alloc(&point_blob, point_len));
    POSIX_GUARD(s2n_ecc_evp_write_point_data_snug(point, group, &point_blob));
+    POSIX_GUARD(s2n_stuffer_write(out, &point_blob));
+    POSIX_GUARD(s2n_free(&point_blob));


Allocating and then immediately freeing is pretty inefficient. Can you figure out a way to do this without an allocation?

crypto/s2n_ecc_evp.c

tests/unit/s2n_client_hello_test.c

crypto/s2n_ecc_evp.c

* Fix typos for temporary * use s2n_stuffer_data_available * add GUARD to the s2n_negotiate to write Client Hello

crypto/s2n_ecc_evp.c

tests/unit/s2n_client_hello_test.c

* Set out->tainted to false in s2n_extension_send.

lrstewart · 2025-04-01T17:10:09Z

crypto/s2n_ecc_evp.c

+    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));


This has the potential not to work. You can't read past where you've written, so this only works if read_cursor < written_data_location. I imagine it works bc read_cursor should be 0 here, but it's not technically correct.

It seems non-ideal to have to deal with the read_cursor at all. Can you figure out a less destructive way to handle this?

lrstewart · 2025-04-01T17:12:34Z

tests/unit/s2n_client_hello_test.c

+    /* Test: Client Hello is slightly less than 64KB */
+    {


If I see this test 6mo from now, I'm going to question why this needed to be tested :) I'd bundle these two tests together under the name "Test: very large client hellos" so that it's clear why you're doing <64KB / >64KB.

lrstewart · 2025-04-01T17:16:09Z

tests/unit/s2n_client_hello_test.c

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


You check s2n_client_hello_get_raw_message_length above, but server_in here. If these two tests are supposed to be analogous, we should make the same size assertion.

Also note that server_in isn't just the CH-- it's also the record, so an extra 5 bytes for the record header. If that matters, I'd probably note that in a comment rather than doing math to strip off the header.

If these two tests are supposed to be analogous, we should make the same size assertion.

I will check server_in for both for them to be analogous. The Client Hello will not be parsed into the server connection if the message itself is a bad message, so we can't get use s2n_client_hello_get_raw_mesage_length for the second case.

I will put a comment mentioning the extra 5 bytes for record header. It shouldn't matter, since the handshake message from the first case is about 100 bytes less than S2N_MAXIMUM_HANDSHAKE_MESSAGE_LENGTH , and the second case is about 300 bytes more than S2N_MAXIMUM_HANDSHAKE_MESSAGE_LENGTH.

lrstewart · 2025-04-01T17:17:41Z

tls/extensions/s2n_extension_type.c

+     * Reset the tainted flag on the out stuffer (typically handshake.io).
+     * The handshake.io stuffer is wiped after processing each handshake message,
+     * which resets its state and marks it as not tainted.
+     * No raw data from handshake.io is permanently stored in the connection during extension send.
+     * Because of that, we can mark the out stuffer as not tainted which allows the stuffer to
+     * continue growing after each extension send.


This comment assumes handshake.io, but you note only "typically handshake.io". That makes me question whether this is safe / correct.

I will say reset the tainted flag on the out stuffer (handshake.io stuffer). It's always the handshake.io stuffer since I have checked the entire codebase for this function call.

lrstewart · 2025-04-01T17:19:12Z

tests/unit/s2n_client_hello_test.c

+    /* Test: Client Hello is slightly less than 64KB */
+    {


These tests require a lot of setup that should be the same. Can they share any setup, to better highlight the differences?

boquan-fang mentioned this pull request Mar 19, 2025

fix: handshake message length integer overflow in s2n_handshake_finish_header #5206

Merged

github-actions bot added the s2n-core team label Mar 19, 2025

boquan-fang marked this pull request as ready for review March 19, 2025 23:50

boquan-fang requested review from camshaft, lrstewart and maddeleine March 19, 2025 23:50

lrstewart reviewed Mar 20, 2025

View reviewed changes

crypto/s2n_ecc_evp.c Show resolved Hide resolved

boquan-fang force-pushed the fix-raw-write branch 2 times, most recently from 42dfff6 to 4aee908 Compare March 25, 2025 19:46

refactor: replace s2n_raw_write and add large client hello test

1f9a9c6

boquan-fang force-pushed the fix-raw-write branch from 4aee908 to 1f9a9c6 Compare March 25, 2025 19:52

boquan-fang changed the title ~~refactor: remove s2n_raw_write usage in s2n_ecc_evp_write_params_point~~ refactor: replace s2n_raw_write and add large client hello test Mar 25, 2025

copy out's write_cursor and high_water_mark

6ca8a99

boquan-fang commented Mar 25, 2025

View reviewed changes

tests/unit/s2n_client_hello_test.c Outdated Show resolved Hide resolved

Remove scoping and fix some comments

371ae42

boquan-fang force-pushed the fix-raw-write branch from efda486 to 371ae42 Compare March 25, 2025 22:34

maddeleine reviewed Mar 25, 2025

View reviewed changes

boquan-fang requested review from lrstewart and maddeleine March 25, 2025 23:23

address PR comments:

e38bd76

* Fix typos for temporary * use s2n_stuffer_data_available * add GUARD to the s2n_negotiate to write Client Hello

boquan-fang force-pushed the fix-raw-write branch from 5c29ebe to e38bd76 Compare March 25, 2025 23:26

maddeleine approved these changes Mar 26, 2025

View reviewed changes

boquan-fang changed the title ~~refactor: replace s2n_raw_write and add large client hello test~~ refactor: replace s2n_stuffer_raw_write and add large client hello test Mar 26, 2025

lrstewart reviewed Mar 26, 2025

View reviewed changes

boquan-fang force-pushed the fix-raw-write branch from e138bfe to b9792f8 Compare March 26, 2025 23:14

address PR feedbacks

a0666a3

boquan-fang force-pushed the fix-raw-write branch from b9792f8 to a0666a3 Compare March 26, 2025 23:42

address PR comments:

7db5746

* Set out->tainted to false in s2n_extension_send.

boquan-fang requested a review from maddeleine March 31, 2025 22:35

boquan-fang requested a review from lrstewart March 31, 2025 22:35

boquan-fang changed the title ~~refactor: replace s2n_stuffer_raw_write and add large client hello test~~ fix: tainted handshake.io and add large client hello test Mar 31, 2025

lrstewart reviewed Apr 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: tainted handshake.io and add large client hello test #5208

fix: tainted handshake.io and add large client hello test #5208

boquan-fang commented Mar 19, 2025 •

edited

Loading

lrstewart Mar 20, 2025

lrstewart Apr 1, 2025

lrstewart Apr 1, 2025

lrstewart Apr 1, 2025

boquan-fang Apr 1, 2025

lrstewart Apr 1, 2025

boquan-fang Apr 1, 2025

lrstewart Apr 1, 2025

	struct s2n_stuffer_reservation available_cipher_suites_size;
	POSIX_GUARD(s2n_stuffer_reserve_uint16(out, &available_cipher_suites_size));

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

fix: tainted handshake.io and add large client hello test #5208

Are you sure you want to change the base?

fix: tainted handshake.io and add large client hello test #5208

Conversation

boquan-fang commented Mar 19, 2025 • edited Loading

Release Summary:

Resolved issues:

Description of changes:

Call-outs:

Testing:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boquan-fang commented Mar 19, 2025 •

edited

Loading