refactor: prevent sending zero lifetime new session ticket by boquan-fang · Pull Request #5003 · aws/s2n-tls

boquan-fang · 2025-01-07T23:09:15Z

Release Summary:

Resolved issues:

resolves #2756.

Description of changes:

This PR resolves the concerns is issue #2756, which asks for

We should also probably handle the case where the result is 0-- should we still send the ticket?

If a new session ticket has zero lifetime, then we shouldn't send it. Hence, this PR will prevent sending new session tickets which have zero lifetime.

Add a check for `ticket_lifetime_in_sec` in `s2n_tls13_server_nst_write()` function.

We send new session ticket with this logic:

s2n-tls/tls/s2n_server_new_session_ticket.c

Lines 176 to 182 in fd41da0

    
           while (conn->tickets_to_send - conn->tickets_sent > 0) { 
        
               if (s2n_result_is_error(s2n_tls13_server_nst_write(conn, nst_stuffer))) { 
        
                   return S2N_RESULT_OK; 
        
               } 
        
               RESULT_GUARD(s2n_post_handshake_write_records(conn, blocked)); 
        
           }

By adding a checking logic for zero session ticket lifetime in s2n_tls13_server_nst_write(), the s2n_tls13_server_nst_send() function can capture the error and stop sending the zero lifetime new session ticket by returning S2N_RESULT_OK.

Add one more tests in `s2n_server_new_session_ticket_test.c` to test sending zero lifetime new session ticket.

I intentionally set the tickets_to_send variable to one, so that the s2n_tls13_server_nst_send() function will attempt to send the ticket. I also set the ticket lifetime to zero, so that s2n_generate_ticket_lifetime() function will make the ticket age to be zero. Then in the test, we check for the following:

There is zero ticket sent.
The stuffer associated with the connection has nothing written in it.

The test logic is similar to the one already in s2n_server_new_session_ticket_test.c.

s2n-tls/tests/unit/s2n_server_new_session_ticket_test.c

Lines 984 to 1017 in fd41da0

    
           { 
        
               struct s2n_config *config = NULL; 
        
               struct s2n_connection *conn = NULL; 
        
               EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_SERVER)); 
        
               EXPECT_NOT_NULL(config = s2n_config_new()); 
        
               EXPECT_OK(s2n_resumption_test_ticket_key_setup(config)); 
        
               EXPECT_SUCCESS(s2n_connection_set_config(conn, config)); 
        
               conn->actual_protocol_version = S2N_TLS13; 
        
               conn->secure->cipher_suite = &s2n_tls13_aes_128_gcm_sha256; 
        
               conn->tickets_to_send = 1; 
        
               EXPECT_NOT_EQUAL(s2n_stuffer_space_remaining(&conn->handshake.io), 0); 
        
               /* Setup io */ 
        
               struct s2n_stuffer stuffer = { 0 }; 
        
               EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&stuffer, 0)); 
        
               EXPECT_SUCCESS(s2n_connection_set_io_stuffers(&stuffer, &stuffer, conn)); 
        
               s2n_blocked_status blocked = 0; 
        
               EXPECT_OK(s2n_tls13_server_nst_send(conn, &blocked)); 
        
               EXPECT_TICKETS_SENT(conn, 1); 
        
               /* Check only one record was written */ 
        
               uint16_t record_len = 0; 
        
               EXPECT_SUCCESS(s2n_stuffer_skip_read(&stuffer, RECORD_LEN_MARKER)); 
        
               EXPECT_SUCCESS(s2n_stuffer_read_uint16(&stuffer, &record_len)); 
        
               EXPECT_TRUE(record_len > 0); 
        
               EXPECT_SUCCESS(s2n_stuffer_skip_read(&stuffer, record_len)); 
        
               EXPECT_EQUAL(s2n_stuffer_data_available(&stuffer), 0); 
        
               EXPECT_SUCCESS(s2n_stuffer_free(&stuffer)); 
        
               EXPECT_SUCCESS(s2n_connection_free(conn)); 
        
               EXPECT_SUCCESS(s2n_config_free(config)); 
        
           };

Call-outs:

Our current way of calculating session ticket age is problematic which is mentioned in issue #4583 and issue #2756. This problem will be fixed in PR #5001.

Testing:

I have mentioned how I add test in the Description of changes section. This PR is tested both locally and in the CI.

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

refactor: prevent sending zero lifetime new session ticket

6eca0b3

github-actions bot added the s2n-core team label Jan 7, 2025

boquan-fang mentioned this pull request Jan 7, 2025

fix: calculation of session ticket age #5001

Merged

boquan-fang closed this Jan 28, 2025

boquan-fang deleted the zero-session-ticket-age branch February 6, 2025 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: prevent sending zero lifetime new session ticket#5003

refactor: prevent sending zero lifetime new session ticket#5003
boquan-fang wants to merge 1 commit intoaws:mainfrom
boquan-fang:zero-session-ticket-age

boquan-fang commented Jan 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	while (conn->tickets_to_send - conn->tickets_sent > 0) {
	if (s2n_result_is_error(s2n_tls13_server_nst_write(conn, nst_stuffer))) {
	return S2N_RESULT_OK;
	}

	RESULT_GUARD(s2n_post_handshake_write_records(conn, blocked));
	}

	{
	struct s2n_config *config = NULL;
	struct s2n_connection *conn = NULL;
	EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_SERVER));
	EXPECT_NOT_NULL(config = s2n_config_new());
	EXPECT_OK(s2n_resumption_test_ticket_key_setup(config));
	EXPECT_SUCCESS(s2n_connection_set_config(conn, config));

	conn->actual_protocol_version = S2N_TLS13;
	conn->secure->cipher_suite = &s2n_tls13_aes_128_gcm_sha256;
	conn->tickets_to_send = 1;
	EXPECT_NOT_EQUAL(s2n_stuffer_space_remaining(&conn->handshake.io), 0);

	/* Setup io */
	struct s2n_stuffer stuffer = { 0 };
	EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&stuffer, 0));
	EXPECT_SUCCESS(s2n_connection_set_io_stuffers(&stuffer, &stuffer, conn));

	s2n_blocked_status blocked = 0;
	EXPECT_OK(s2n_tls13_server_nst_send(conn, &blocked));
	EXPECT_TICKETS_SENT(conn, 1);

	/* Check only one record was written */
	uint16_t record_len = 0;
	EXPECT_SUCCESS(s2n_stuffer_skip_read(&stuffer, RECORD_LEN_MARKER));
	EXPECT_SUCCESS(s2n_stuffer_read_uint16(&stuffer, &record_len));
	EXPECT_TRUE(record_len > 0);
	EXPECT_SUCCESS(s2n_stuffer_skip_read(&stuffer, record_len));
	EXPECT_EQUAL(s2n_stuffer_data_available(&stuffer), 0);

	EXPECT_SUCCESS(s2n_stuffer_free(&stuffer));
	EXPECT_SUCCESS(s2n_connection_free(conn));
	EXPECT_SUCCESS(s2n_config_free(config));
	};

Conversation

boquan-fang commented Jan 7, 2025

Release Summary:

Resolved issues:

Description of changes:

Add a check for ticket_lifetime_in_sec in s2n_tls13_server_nst_write() function.

Add one more tests in s2n_server_new_session_ticket_test.c to test sending zero lifetime new session ticket.

Call-outs:

Testing:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Add a check for `ticket_lifetime_in_sec` in `s2n_tls13_server_nst_write()` function.

Add one more tests in `s2n_server_new_session_ticket_test.c` to test sending zero lifetime new session ticket.