From f1d38f6427b031cdf0541c9d06d1c9ff5fe05fd8 Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Mon, 24 Jul 2023 20:49:44 -0700 Subject: [PATCH] Address maddeleine@'s PR comments --- tests/unit/s2n_resume_test.c | 148 ++++++++++++++++++++++------------- tls/s2n_resume.c | 18 ++--- 2 files changed, 104 insertions(+), 62 deletions(-) diff --git a/tests/unit/s2n_resume_test.c b/tests/unit/s2n_resume_test.c index 5c6fa56103b..49ef6bbfe81 100644 --- a/tests/unit/s2n_resume_test.c +++ b/tests/unit/s2n_resume_test.c @@ -1644,11 +1644,6 @@ int main(int argc, char **argv) uint8_t *ticket_bytes = s2n_stuffer_raw_read(&ticket_stuffer, ticket_size); EXPECT_NOT_NULL(ticket_bytes); - DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT), - s2n_connection_ptr_free); - EXPECT_NOT_NULL(conn); - EXPECT_SUCCESS(s2n_connection_set_config(conn, config)); - /* Test that deserialize modifies the connection in limited ways. * * No mechanism exists to do more than a shallow comparison of two connections. @@ -1658,32 +1653,72 @@ int main(int argc, char **argv) * test that s2n_deserialize_resumption_state can successfully operate * on an s2n_connection with a limited number of its pointers initialized. */ - struct s2n_connection empty_conn = { 0 }; - struct s2n_crypto_parameters crypto_params = { .cipher_suite = &s2n_null_cipher_suite }; - empty_conn.secure = &crypto_params; - empty_conn.mode = S2N_CLIENT; - /* We can safely assume that a connection doesn't modify its config */ - empty_conn.config = conn->config; - EXPECT_SUCCESS(s2n_connection_set_session(&empty_conn, ticket_bytes, ticket_size)); - EXPECT_SUCCESS(s2n_free(&empty_conn.client_ticket)); - - /* Trigger the deserialization failure as late as possible. - * The last byte is optional, so drop the last two bytes. + { + struct s2n_connection empty_conn = { 0 }; + struct s2n_crypto_parameters crypto_params = { .cipher_suite = &s2n_null_cipher_suite }; + empty_conn.secure = &crypto_params; + empty_conn.mode = S2N_CLIENT; + /* We can safely assume that a connection doesn't modify its config */ + empty_conn.config = config; + EXPECT_SUCCESS(s2n_connection_set_session(&empty_conn, ticket_bytes, ticket_size)); + EXPECT_SUCCESS(s2n_free(&empty_conn.client_ticket)); + }; + + /* Test that deserialize does not modify the connection on parsing failure, + * given the constraints proven above. + */ + { + DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT), + s2n_connection_ptr_free); + EXPECT_NOT_NULL(conn); + EXPECT_SUCCESS(s2n_connection_set_config(conn, config)); + + /* Trigger the deserialization failure as late as possible. + * The last byte is optional, so drop the last two bytes. + */ + size_t bad_ticket_size = ticket_size - 2; + + /* Test the connection is not modified by a failed deserialize */ + uint8_t saved_conn[sizeof(struct s2n_connection)] = { 0 }; + EXPECT_MEMCPY_SUCCESS(saved_conn, conn, sizeof(struct s2n_connection)); + uint8_t saved_secure[sizeof(struct s2n_crypto_parameters)] = { 0 }; + EXPECT_MEMCPY_SUCCESS(saved_secure, conn->secure, sizeof(struct s2n_crypto_parameters)); + EXPECT_FAILURE_WITH_ERRNO(s2n_connection_set_session(conn, ticket_bytes, bad_ticket_size), + S2N_ERR_STUFFER_OUT_OF_DATA); + EXPECT_BYTEARRAY_EQUAL(saved_conn, conn, sizeof(struct s2n_connection)); + EXPECT_BYTEARRAY_EQUAL(saved_secure, conn->secure, sizeof(struct s2n_crypto_parameters)); + + /* No valid ticket */ + EXPECT_EQUAL(s2n_connection_get_session_length(conn), 0); + }; + + /* Test that deserialize does not modify the connection on a cipher selection failure, + * given the constraints proven above. */ - ticket_size -= 2; - - /* Test the connection is not modified by a failed deserialize */ - uint8_t saved_conn[sizeof(struct s2n_connection)] = { 0 }; - EXPECT_MEMCPY_SUCCESS(saved_conn, conn, sizeof(struct s2n_connection)); - uint8_t saved_secure[sizeof(struct s2n_crypto_parameters)] = { 0 }; - EXPECT_MEMCPY_SUCCESS(saved_secure, conn->secure, sizeof(struct s2n_crypto_parameters)); - EXPECT_FAILURE_WITH_ERRNO(s2n_connection_set_session(conn, ticket_bytes, ticket_size), - S2N_ERR_STUFFER_OUT_OF_DATA); - EXPECT_BYTEARRAY_EQUAL(saved_conn, conn, sizeof(struct s2n_connection)); - EXPECT_BYTEARRAY_EQUAL(saved_secure, conn->secure, sizeof(struct s2n_crypto_parameters)); - - /* No valid ticket */ - EXPECT_EQUAL(s2n_connection_get_session_length(conn), 0); + { + DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT), + s2n_connection_ptr_free); + EXPECT_NOT_NULL(conn); + EXPECT_SUCCESS(s2n_connection_set_config(conn, config)); + + /* Trigger the deserialization failure when checking the validity + * of the chosen cipher, not when parsing the ticket. + */ + EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(conn, "null")); + + /* Test the connection is not modified by a failed deserialize */ + uint8_t saved_conn[sizeof(struct s2n_connection)] = { 0 }; + EXPECT_MEMCPY_SUCCESS(saved_conn, conn, sizeof(struct s2n_connection)); + uint8_t saved_secure[sizeof(struct s2n_crypto_parameters)] = { 0 }; + EXPECT_MEMCPY_SUCCESS(saved_secure, conn->secure, sizeof(struct s2n_crypto_parameters)); + EXPECT_FAILURE_WITH_ERRNO(s2n_connection_set_session(conn, ticket_bytes, ticket_size), + S2N_ERR_CIPHER_NOT_SUPPORTED); + EXPECT_BYTEARRAY_EQUAL(saved_conn, conn, sizeof(struct s2n_connection)); + EXPECT_BYTEARRAY_EQUAL(saved_secure, conn->secure, sizeof(struct s2n_crypto_parameters)); + + /* No valid ticket */ + EXPECT_EQUAL(s2n_connection_get_session_length(conn), 0); + }; }; /* Invalid TLS1.3 tickets should not modify the connection. @@ -1702,11 +1737,6 @@ int main(int argc, char **argv) uint8_t *ticket_bytes = s2n_stuffer_raw_read(&ticket_stuffer, ticket_size); EXPECT_NOT_NULL(ticket_bytes); - DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT), - s2n_connection_ptr_free); - EXPECT_NOT_NULL(conn); - EXPECT_SUCCESS(s2n_connection_set_config(conn, config)); - /* Test that the connection is only shallowly modified by a successful deserialize. * * No mechanism exists to do more than a shallow comparison of two connections. @@ -1716,27 +1746,39 @@ int main(int argc, char **argv) * test that s2n_deserialize_resumption_state can successfully operate * on an s2n_connection with none of its pointers initialized. */ - struct s2n_connection empty_conn = { 0 }; - empty_conn.mode = S2N_CLIENT; - /* We can safely assume that a connection doesn't modify its config */ - empty_conn.config = conn->config; - EXPECT_SUCCESS(s2n_connection_set_session(&empty_conn, ticket_bytes, ticket_size)); - EXPECT_OK(s2n_psk_parameters_wipe(&empty_conn.psk_params)); - - /* Trigger the deserialization failure as late as possible in parsing. - * Drop the last byte we expect. + { + struct s2n_connection empty_conn = { 0 }; + empty_conn.mode = S2N_CLIENT; + /* We can safely assume that a connection doesn't modify its config */ + empty_conn.config = config; + EXPECT_SUCCESS(s2n_connection_set_session(&empty_conn, ticket_bytes, ticket_size)); + EXPECT_OK(s2n_psk_parameters_wipe(&empty_conn.psk_params)); + }; + + /* Test that deserialize does not modify the connection on failure, + * given the constraints proven above. */ - size_t wrong_ticket_size = ticket_size - 1; + { + DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT), + s2n_connection_ptr_free); + EXPECT_NOT_NULL(conn); + EXPECT_SUCCESS(s2n_connection_set_config(conn, config)); + + /* Trigger the deserialization failure as late as possible in parsing. + * Drop the last byte we expect. + */ + size_t bad_ticket_size = ticket_size - 1; - /* Test the connection is not modified by a failed deserialize */ - uint8_t saved[sizeof(struct s2n_connection)] = { 0 }; - EXPECT_MEMCPY_SUCCESS(saved, conn, sizeof(struct s2n_connection)); - EXPECT_FAILURE_WITH_ERRNO(s2n_connection_set_session(conn, ticket_bytes, wrong_ticket_size), - S2N_ERR_STUFFER_OUT_OF_DATA); - EXPECT_BYTEARRAY_EQUAL(saved, conn, sizeof(struct s2n_connection)); + /* Test the connection is not modified by a failed deserialize */ + uint8_t saved[sizeof(struct s2n_connection)] = { 0 }; + EXPECT_MEMCPY_SUCCESS(saved, conn, sizeof(struct s2n_connection)); + EXPECT_FAILURE_WITH_ERRNO(s2n_connection_set_session(conn, ticket_bytes, bad_ticket_size), + S2N_ERR_STUFFER_OUT_OF_DATA); + EXPECT_BYTEARRAY_EQUAL(saved, conn, sizeof(struct s2n_connection)); - /* No valid ticket */ - EXPECT_EQUAL(s2n_connection_get_session_length(conn), 0); + /* No valid ticket */ + EXPECT_EQUAL(s2n_connection_get_session_length(conn), 0); + }; }; }; diff --git a/tls/s2n_resume.c b/tls/s2n_resume.c index 5dd2a9f2140..8f9b9a22443 100644 --- a/tls/s2n_resume.c +++ b/tls/s2n_resume.c @@ -240,26 +240,26 @@ static S2N_RESULT s2n_tls12_client_deserialize_session_state(struct s2n_connecti */ struct s2n_crypto_parameters *secure = conn->secure; RESULT_ENSURE_REF(secure); - struct s2n_connection resume_conn = *conn; - struct s2n_crypto_parameters resume_secure = *secure; - resume_conn.secure = &resume_secure; + struct s2n_connection temp_conn = *conn; + struct s2n_crypto_parameters temp_secure = *secure; + temp_conn.secure = &temp_secure; - RESULT_GUARD_POSIX(s2n_stuffer_read_uint8(from, &resume_conn.resume_protocol_version)); + RESULT_GUARD_POSIX(s2n_stuffer_read_uint8(from, &temp_conn.resume_protocol_version)); uint8_t *cipher_suite_wire = s2n_stuffer_raw_read(from, S2N_TLS_CIPHER_SUITE_LEN); RESULT_ENSURE_REF(cipher_suite_wire); - RESULT_GUARD_POSIX(s2n_set_cipher_as_client(&resume_conn, cipher_suite_wire)); + RESULT_GUARD_POSIX(s2n_set_cipher_as_client(&temp_conn, cipher_suite_wire)); uint64_t then = 0; RESULT_GUARD_POSIX(s2n_stuffer_read_uint64(from, &then)); - RESULT_GUARD_POSIX(s2n_stuffer_read_bytes(from, resume_conn.secrets.version.tls12.master_secret, + RESULT_GUARD_POSIX(s2n_stuffer_read_bytes(from, temp_conn.secrets.version.tls12.master_secret, S2N_TLS_SECRET_LEN)); if (s2n_stuffer_data_available(from)) { uint8_t ems_negotiated = 0; RESULT_GUARD_POSIX(s2n_stuffer_read_uint8(from, &ems_negotiated)); - resume_conn.ems_negotiated = ems_negotiated; + temp_conn.ems_negotiated = ems_negotiated; } DEFER_CLEANUP(struct s2n_blob client_ticket = { 0 }, s2n_free); @@ -269,8 +269,8 @@ static S2N_RESULT s2n_tls12_client_deserialize_session_state(struct s2n_connecti /* Finally, actually update the connection */ RESULT_GUARD_POSIX(s2n_free(&conn->client_ticket)); - *secure = resume_secure; - *conn = resume_conn; + *secure = temp_secure; + *conn = temp_conn; conn->secure = secure; conn->client_ticket = client_ticket; ZERO_TO_DISABLE_DEFER_CLEANUP(client_ticket);