Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Send zero-length NST when session key is expired #4532

Merged
merged 36 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
2e48de1
send zero-length NST when session key is expired
jouho Apr 30, 2024
becb306
add another to test the impact on tls 1.3 path
jouho Apr 30, 2024
e023d46
check if tls1.3 is fully supported
jouho Apr 30, 2024
edff88a
use shorter variable names
jouho May 1, 2024
f22a5b9
Merge branch 'main' into zero-length-tls12-NST-message
jouho May 1, 2024
8352bce
Merge branch 'main' into zero-length-tls12-NST-message
jouho May 1, 2024
c4a5474
modify test setups
jouho May 1, 2024
9beba2c
address comment
jouho May 1, 2024
8ead510
add compliance comments
jouho May 1, 2024
8838bdf
Merge branch 'main' into zero-length-tls12-NST-message
jouho May 1, 2024
ab18c07
add unit test for s2n_server_send
jouho May 6, 2024
c1948e8
Merge branch 'main' into zero-length-tls12-NST-message
jouho May 6, 2024
e86b5b3
address clang format issues
jouho May 6, 2024
ae343b8
edit rfc comment format
jouho May 6, 2024
9302a3a
fix rfc number
jouho May 6, 2024
112574c
remove unnecessary expect_false()
jouho May 6, 2024
82709ee
add test case for sending non-zero nst
jouho May 6, 2024
8118b83
use suggested comment
jouho May 7, 2024
aad3a35
remove unnecessary test case
jouho May 7, 2024
13ec6d3
check all data is read
jouho May 7, 2024
1c8b71d
give better test description
jouho May 7, 2024
0a4c8ee
add compliance comment to the test
jouho May 7, 2024
9ff9261
fix comment format
jouho May 7, 2024
474f40f
Merge branch 'main' into zero-length-tls12-NST-message
jouho May 7, 2024
4fc4de4
Merge branch 'main' into zero-length-tls12-NST-message
jouho May 8, 2024
3554753
verify ticket length and and lifetime hint is 0
jouho May 8, 2024
ca92f58
fix input orders
jouho May 8, 2024
14d56a3
address PR feedbacks
jouho May 8, 2024
6b3ace8
send zero-length nst upon failing to retrieve encrypt key
jouho May 13, 2024
9b09e91
address linter issue
jouho May 13, 2024
8aeedd3
Merge branch 'main' into zero-length-tls12-NST-message
jouho May 13, 2024
907da2b
address redundant code
jouho May 13, 2024
ab7c57d
remove unnecessary test setup
jouho May 15, 2024
1bf7d88
change return value to S2N_SUCCESS
jouho May 15, 2024
8ee8b0f
Merge branch 'main' into zero-length-tls12-NST-message
jouho May 15, 2024
b2e650d
remove unnecessary #define
jouho May 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
101 changes: 101 additions & 0 deletions tests/unit/s2n_server_new_session_ticket_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#define MAX_TEST_SESSION_SIZE 300

#define EXPECT_TICKETS_SENT(conn, count) EXPECT_OK(s2n_assert_tickets_sent(conn, count))
#define S2N_CLOCK_SYS CLOCK_REALTIME

jouho marked this conversation as resolved.
Show resolved Hide resolved
static S2N_RESULT s2n_assert_tickets_sent(struct s2n_connection *conn, uint16_t expected_tickets_sent)
{
Expand Down Expand Up @@ -80,6 +81,32 @@ static int s2n_setup_test_resumption_secret(struct s2n_connection *conn)
return S2N_SUCCESS;
}

/**
* This function is used to "skip" time in unit tests. It will mock the system
* time to be current_time (ns) + data (ns). The "data" parameter is a uint64_t
* passed in as a void*.
*/
int mock_nanoseconds_since_epoch(void *data, uint64_t *nanoseconds)
{
struct timespec current_time;

clock_gettime(S2N_CLOCK_SYS, &current_time);

/**
* current_time fields are represented as time_t, and time_t has a platform
* dependent size. On 32 bit platforms, attempting to convert the current
* system time to nanoseconds will overflow, causing odd failures in unit
* tests. We upcast current_time fields to uint64_t before multiplying to
* avoid this.
*/
*nanoseconds = 0;
*nanoseconds += (uint64_t) current_time.tv_sec * ONE_SEC_IN_NANOS;
*nanoseconds += (uint64_t) current_time.tv_nsec;
*nanoseconds += *(uint64_t *) data;

return 0;
}

int main(int argc, char **argv)
{
BEGIN_TEST();
Expand Down Expand Up @@ -801,6 +828,80 @@ int main(int argc, char **argv)
};
};

/* s2n_server_nst_send */
{
uint8_t nst_data[S2N_TLS12_TICKET_SIZE_IN_BYTES] = { 0 };

/* s2n_server_nst_send writes a non-zero ticket when a valid encryption key exists */
{
DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_NOT_NULL(config);
DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_SERVER),
s2n_connection_ptr_free);
EXPECT_NOT_NULL(conn);
EXPECT_OK(s2n_resumption_test_ticket_key_setup(config));
EXPECT_SUCCESS(s2n_connection_set_config(conn, config));

conn->session_ticket_status = S2N_NEW_TICKET;

EXPECT_SUCCESS(s2n_server_nst_send(conn));
EXPECT_TICKETS_SENT(conn, 1);

uint32_t lifetime = 0;
EXPECT_SUCCESS(s2n_stuffer_read_uint32(&conn->handshake.io, &lifetime));
EXPECT_TRUE(lifetime > 0);

uint16_t ticket_len = 0;
EXPECT_SUCCESS(s2n_stuffer_read_uint16(&conn->handshake.io, &ticket_len));
EXPECT_TRUE(ticket_len > 0);

struct s2n_blob nst_message = { 0 };
jouho marked this conversation as resolved.
Show resolved Hide resolved
EXPECT_SUCCESS(s2n_blob_init(&nst_message, nst_data, sizeof(nst_data)));
EXPECT_SUCCESS(s2n_stuffer_read(&conn->handshake.io, &nst_message));
EXPECT_EQUAL(s2n_stuffer_data_available(&conn->handshake.io), 0);
};
jouho marked this conversation as resolved.
Show resolved Hide resolved

/* s2n_server_nst_send writes a zero-length ticket when no valid encryption key exists */
{
jouho marked this conversation as resolved.
Show resolved Hide resolved
/**
*= https://www.rfc-editor.org/rfc/rfc5077#section-3.3
*= type=test
*# If the server determines that it does not want to include a
jouho marked this conversation as resolved.
Show resolved Hide resolved
*# ticket after it has included the SessionTicket extension in the
*# ServerHello, then it sends a zero-length ticket in the
*# NewSessionTicket handshake message.
**/
DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_NOT_NULL(config);
DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_SERVER),
s2n_connection_ptr_free);
EXPECT_NOT_NULL(conn);
EXPECT_OK(s2n_resumption_test_ticket_key_setup(config));
EXPECT_SUCCESS(s2n_connection_set_config(conn, config));

EXPECT_NOT_EQUAL(s2n_stuffer_space_remaining(&conn->handshake.io), 0);
jouho marked this conversation as resolved.
Show resolved Hide resolved

conn->config->use_tickets = 1;
jouho marked this conversation as resolved.
Show resolved Hide resolved
conn->session_ticket_status = S2N_NEW_TICKET;

/* Expire current session ticket key so that server no longer holds a valid key */
uint64_t mock_delay = config->encrypt_decrypt_key_lifetime_in_nanos;
EXPECT_SUCCESS(s2n_config_set_wall_clock(config, mock_nanoseconds_since_epoch,
&mock_delay));
jouho marked this conversation as resolved.
Show resolved Hide resolved

EXPECT_SUCCESS(s2n_server_nst_send(conn));
EXPECT_TICKETS_SENT(conn, 0);

uint32_t lifetime = 0;
EXPECT_SUCCESS(s2n_stuffer_read_uint32(&conn->handshake.io, &lifetime));
EXPECT_TRUE(lifetime == 0);

uint16_t ticket_len = 0;
EXPECT_SUCCESS(s2n_stuffer_read_uint16(&conn->handshake.io, &ticket_len));
EXPECT_TRUE(ticket_len == 0);
};
}

/* s2n_tls13_server_nst_send */
{
/* Mode is not server */
Expand Down
57 changes: 57 additions & 0 deletions tests/unit/s2n_session_ticket_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -1416,6 +1416,63 @@ int main(int argc, char **argv)
EXPECT_TRUE(IS_RESUMPTION_HANDSHAKE(server));
}

/* Test TLS 1.2 Server sends a zero-length ticket in the NewSessionTicket handshake
* if the ticket key was expired after SERVER_HELLO
*/
{
DEFER_CLEANUP(struct s2n_config *client_configuration = s2n_config_new(),
s2n_config_ptr_free);
EXPECT_NOT_NULL(client_configuration);
EXPECT_SUCCESS(s2n_config_set_session_tickets_onoff(client_configuration, 1));
EXPECT_SUCCESS(s2n_config_set_unsafe_for_testing(client_configuration));

DEFER_CLEANUP(struct s2n_config *server_configuration = s2n_config_new(),
s2n_config_ptr_free);
EXPECT_NOT_NULL(server_configuration);
EXPECT_SUCCESS(s2n_config_set_session_tickets_onoff(server_configuration, 1));
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(server_configuration,
chain_and_key));

EXPECT_SUCCESS(s2n_config_add_ticket_crypto_key(server_configuration, ticket_key_name1,
s2n_array_len(ticket_key_name1), ticket_key1, s2n_array_len(ticket_key1), 0));

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

DEFER_CLEANUP(struct s2n_connection *server = s2n_connection_new(S2N_SERVER),
s2n_connection_ptr_free);
EXPECT_NOT_NULL(server);
EXPECT_SUCCESS(s2n_connection_set_config(server, server_configuration));

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, server, &test_io));

/* Stop the handshake after the peers have established that a ticket will be sent in this handshake. */
EXPECT_OK(s2n_negotiate_test_server_and_client_until_message(server, client, CLIENT_FINISHED));

/* Expire current session ticket key so that server no longer holds a valid key */
uint64_t mock_delay = server_configuration->encrypt_decrypt_key_lifetime_in_nanos;
EXPECT_SUCCESS(s2n_config_set_wall_clock(server_configuration, mock_nanoseconds_since_epoch,
&mock_delay));

/* Attempt to send a NewSessionTicket. This should send a zero-length NST message */
EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server, client));

/* Verify that TLS1.2 was negotiated */
EXPECT_EQUAL(client->actual_protocol_version, S2N_TLS12);
EXPECT_EQUAL(server->actual_protocol_version, S2N_TLS12);

/* Verify that the server issued zero-length session ticket */
EXPECT_TRUE(IS_ISSUING_NEW_SESSION_TICKET(server));

EXPECT_EQUAL(client->client_ticket.size, 0);
jouho marked this conversation as resolved.
Show resolved Hide resolved
EXPECT_EQUAL(client->ticket_lifetime_hint, 0);
}

EXPECT_SUCCESS(s2n_io_pair_close(&io_pair));
EXPECT_SUCCESS(s2n_cert_chain_and_key_free(chain_and_key));
EXPECT_SUCCESS(s2n_cert_chain_and_key_free(ecdsa_chain_and_key));
Expand Down
24 changes: 21 additions & 3 deletions tls/s2n_server_new_session_ticket.c
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,30 @@ int s2n_server_nst_send(struct s2n_connection *conn)
uint32_t lifetime_hint_in_secs =
(conn->config->encrypt_decrypt_key_lifetime_in_nanos + conn->config->decrypt_key_lifetime_in_nanos) / ONE_SEC_IN_NANOS;

/* When server changes it's mind mid handshake send lifetime hint and session ticket length as zero */
if (!conn->config->use_tickets) {
/* Send a zero-length ticket in the NewSessionTicket message if the server changes
* its mind mid-handshake or if there are no valid encrypt keys currently available.
*
*= https://www.rfc-editor.org/rfc/rfc5077#section-3.3
*# This message MUST be sent if the server included
*# a SessionTicket extension in the ServerHello.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right place for this compliance comment? This code doesn't ensure that the message is sent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is writing to stuffer not considered "sending a message", or are you suggesting to put this comment on top of code where I send zero-length nst message upon failing to retrieve a key like what I did in my latest change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing to a stuffer isn't automatically sending a message. This code also doesn't somehow enforce that you always write to that stuffer, just that you do in this one case.

With the way the code is currently structured, I'm not sure there's a good place for this comment.

*
*= https://www.rfc-editor.org/rfc/rfc5077#section-3.3
*# If the server determines that it does not want to include a
*# ticket after it has included the SessionTicket extension in the
*# ServerHello, then it sends a zero-length ticket in the
*# NewSessionTicket handshake message.
*
*= https://www.rfc-editor.org/rfc/rfc5077#section-3.3
*# struct {
*# uint32 ticket_lifetime_hint;
*# opaque ticket<0..2^16-1>;
*# } NewSessionTicket;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not convinced this is the right place for this compliance comment. The struct format applies to the whole message, not just to an empty message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to put this comment on top of this s2n_server_nst_send() function? Or is there more suitable place for this comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like above: it's not clear to me that there is a suitable place to put this comment given the current structure of the code.

**/
if (!conn->config->use_tickets || s2n_result_is_error(s2n_config_is_encrypt_key_available(conn->config))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this completely solve the problem? Couldn't the key still expire between here and s2n_encrypt_session_ticket? Or couldn't s2n_config_is_encrypt_key_available still otherwise disagree with s2n_get_ticket_encrypt_decrypt_key?

To completely solve the problem, you need to write an empty message in response to actually failing to retrieve the key, not in response to a new "is there a key" check. We already made an "is there a key" check during the last message, which was probably like nanoseconds ago.

POSIX_GUARD(s2n_stuffer_write_uint32(&conn->handshake.io, 0));
POSIX_GUARD(s2n_stuffer_write_uint16(&conn->handshake.io, 0));

return 0;
return S2N_SUCCESS;
}

if (!s2n_server_sending_nst(conn)) {
Expand Down