diff --git a/api/s2n.h b/api/s2n.h index cccc2b0c927..820837cc329 100644 --- a/api/s2n.h +++ b/api/s2n.h @@ -2214,6 +2214,9 @@ S2N_API extern int s2n_session_ticket_get_lifetime(struct s2n_session_ticket *ti /** * De-serializes the session state and updates the connection accordingly. * + * If this method fails, the connection should not be affected: calling s2n_negotiate + * with the connection should simply result in a full handshake. + * * @param conn A pointer to the s2n_connection object * @param session A pointer to a buffer of size `length` * @param length The size of the `session` buffer diff --git a/tests/unit/s2n_resume_test.c b/tests/unit/s2n_resume_test.c index 42eb7fac438..eec6c211d75 100644 --- a/tests/unit/s2n_resume_test.c +++ b/tests/unit/s2n_resume_test.c @@ -71,6 +71,66 @@ int main(int argc, char **argv) "18df06843d13a08bf2a449844c5f8a" "478001bc4d4c627984d5a41da8d0402919"); + uint8_t tls12_ticket[S2N_TLS12_STATE_SIZE_IN_BYTES_WITHOUT_EMS] = { + S2N_SERIALIZED_FORMAT_TLS12_V1, + S2N_TLS12, + TLS_RSA_WITH_AES_128_GCM_SHA256, + TICKET_ISSUE_TIME_BYTES, + }; + + uint8_t tls12_ticket_with_ems[S2N_TLS12_STATE_SIZE_IN_BYTES] = { + S2N_SERIALIZED_FORMAT_TLS12_V3, + S2N_TLS12, + TLS_RSA_WITH_AES_128_GCM_SHA256, + TICKET_ISSUE_TIME_BYTES, + }; + + uint8_t tls13_ticket[] = { + S2N_SERIALIZED_FORMAT_TLS13_V1, + S2N_TLS13, + TLS_AES_128_GCM_SHA256, + TICKET_ISSUE_TIME_BYTES, + TICKET_AGE_ADD_BYTES, + SECRET_LEN, + SECRET, + EMPTY_EARLY_DATA_SIZE, + }; + + uint8_t tls13_server_ticket[] = { + S2N_SERIALIZED_FORMAT_TLS13_V1, + S2N_TLS13, + TLS_AES_128_GCM_SHA256, + TICKET_ISSUE_TIME_BYTES, + TICKET_AGE_ADD_BYTES, + SECRET_LEN, + SECRET, + KEYING_MATERIAL_EXPIRATION_BYTES, + EMPTY_EARLY_DATA_SIZE, + }; + + uint8_t tls13_ticket_with_early_data[] = { + S2N_SERIALIZED_FORMAT_TLS13_V1, + S2N_TLS13, + TLS_AES_128_GCM_SHA256, + TICKET_ISSUE_TIME_BYTES, + TICKET_AGE_ADD_BYTES, + SECRET_LEN, + SECRET, + 0x00, + 0x00, + 0x00, + NONEMPTY_EARLY_DATA_SIZE, + APP_PROTOCOL_LEN, + APP_PROTOCOL, + 0x00, + EARLY_DATA_CONTEXT_LEN, + EARLY_DATA_CONTEXT, + }; + + uint8_t faulty_format_ticket[] = { + 0xFF, + }; + /* s2n_connection_get_session_state_size */ { /* Safety */ @@ -642,66 +702,6 @@ int main(int argc, char **argv) /* s2n_deserialize_resumption_state */ { - uint8_t tls12_ticket[S2N_TLS12_STATE_SIZE_IN_BYTES_WITHOUT_EMS] = { - S2N_SERIALIZED_FORMAT_TLS12_V1, - S2N_TLS12, - TLS_RSA_WITH_AES_128_GCM_SHA256, - TICKET_ISSUE_TIME_BYTES, - }; - - uint8_t tls12_ticket_with_ems[S2N_TLS12_STATE_SIZE_IN_BYTES] = { - S2N_SERIALIZED_FORMAT_TLS12_V3, - S2N_TLS12, - TLS_RSA_WITH_AES_128_GCM_SHA256, - TICKET_ISSUE_TIME_BYTES, - }; - - uint8_t tls13_ticket[] = { - S2N_SERIALIZED_FORMAT_TLS13_V1, - S2N_TLS13, - TLS_AES_128_GCM_SHA256, - TICKET_ISSUE_TIME_BYTES, - TICKET_AGE_ADD_BYTES, - SECRET_LEN, - SECRET, - EMPTY_EARLY_DATA_SIZE, - }; - - uint8_t tls13_server_ticket[] = { - S2N_SERIALIZED_FORMAT_TLS13_V1, - S2N_TLS13, - TLS_AES_128_GCM_SHA256, - TICKET_ISSUE_TIME_BYTES, - TICKET_AGE_ADD_BYTES, - SECRET_LEN, - SECRET, - KEYING_MATERIAL_EXPIRATION_BYTES, - EMPTY_EARLY_DATA_SIZE, - }; - - uint8_t tls13_ticket_with_early_data[] = { - S2N_SERIALIZED_FORMAT_TLS13_V1, - S2N_TLS13, - TLS_AES_128_GCM_SHA256, - TICKET_ISSUE_TIME_BYTES, - TICKET_AGE_ADD_BYTES, - SECRET_LEN, - SECRET, - 0x00, - 0x00, - 0x00, - NONEMPTY_EARLY_DATA_SIZE, - APP_PROTOCOL_LEN, - APP_PROTOCOL, - 0x00, - EARLY_DATA_CONTEXT_LEN, - EARLY_DATA_CONTEXT, - }; - - uint8_t faulty_format_ticket[] = { - 0xFF, - }; - /* Deserialize ticket with incorrect format errors */ { struct s2n_blob ticket_blob = { 0 }; @@ -1675,5 +1675,169 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_config_free(config)); }; + /* Test s2n_connection_set_session */ + { + uint8_t server_state[] = "encrypted state"; + + DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free); + EXPECT_NOT_NULL(config); + EXPECT_SUCCESS(s2n_config_set_wall_clock(config, mock_time, NULL)); + EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "test_all")); + + /* Invalid TLS1.2 tickets should not modify the connection. + * + * This basically tests that deserialization errors aren't fatal / unrecoverable. + */ + { + DEFER_CLEANUP(struct s2n_stuffer ticket_stuffer = { 0 }, s2n_stuffer_free); + EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&ticket_stuffer, 0)); + EXPECT_SUCCESS(s2n_stuffer_write_uint8(&ticket_stuffer, S2N_STATE_WITH_SESSION_TICKET)); + EXPECT_SUCCESS(s2n_stuffer_write_uint16(&ticket_stuffer, sizeof(server_state))); + EXPECT_SUCCESS(s2n_stuffer_write_bytes(&ticket_stuffer, server_state, sizeof(server_state))); + EXPECT_SUCCESS(s2n_stuffer_write_bytes(&ticket_stuffer, + tls12_ticket_with_ems, sizeof(tls12_ticket_with_ems))); + + size_t ticket_size = s2n_stuffer_data_available(&ticket_stuffer); + uint8_t *ticket_bytes = s2n_stuffer_raw_read(&ticket_stuffer, ticket_size); + EXPECT_NOT_NULL(ticket_bytes); + + /* Test that deserialize modifies the connection in limited ways. + * + * No mechanism exists to do more than a shallow comparison of two connections. + * To prove that a shallow comparison is sufficient, we need to prove + * that s2n_deserialize_resumption_state does not modify the memory + * associated with pointers on the connection. To prove that, we can + * 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 = 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. + */ + { + 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. + * + * This basically tests that deserialization errors aren't fatal / unrecoverable. + */ + { + DEFER_CLEANUP(struct s2n_stuffer ticket_stuffer = { 0 }, s2n_stuffer_free); + EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&ticket_stuffer, 0)); + EXPECT_SUCCESS(s2n_stuffer_write_uint8(&ticket_stuffer, S2N_STATE_WITH_SESSION_TICKET)); + EXPECT_SUCCESS(s2n_stuffer_write_uint16(&ticket_stuffer, sizeof(server_state))); + EXPECT_SUCCESS(s2n_stuffer_write_bytes(&ticket_stuffer, server_state, sizeof(server_state))); + EXPECT_SUCCESS(s2n_stuffer_write_bytes(&ticket_stuffer, tls13_ticket, sizeof(tls13_ticket))); + + size_t ticket_size = s2n_stuffer_data_available(&ticket_stuffer); + uint8_t *ticket_bytes = s2n_stuffer_raw_read(&ticket_stuffer, ticket_size); + EXPECT_NOT_NULL(ticket_bytes); + + /* 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. + * To prove that a shallow comparison is sufficient, we need to prove + * that s2n_deserialize_resumption_state does not modify the memory + * associated with pointers on the connection. To prove that, we can + * 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 = 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. + */ + { + 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, 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); + }; + }; + }; + END_TEST(); } diff --git a/tls/s2n_resume.c b/tls/s2n_resume.c index b797347da58..d5f10b7ad07 100644 --- a/tls/s2n_resume.c +++ b/tls/s2n_resume.c @@ -228,27 +228,53 @@ static int s2n_client_serialize_resumption_state(struct s2n_connection *conn, st return 0; } -static S2N_RESULT s2n_tls12_client_deserialize_session_state(struct s2n_connection *conn, struct s2n_stuffer *from) +static S2N_RESULT s2n_tls12_client_deserialize_session_state(struct s2n_connection *conn, + struct s2n_blob *ticket, struct s2n_stuffer *from) { RESULT_ENSURE_REF(conn); RESULT_ENSURE_REF(from); - RESULT_GUARD_POSIX(s2n_stuffer_read_uint8(from, &conn->resume_protocol_version)); + /* Operate on a copy of the connection to avoid mutating the connection on + * failure. We have tests in s2n_resume_test.c that prove this level of copy + * is sufficient. + */ + struct s2n_crypto_parameters *secure = conn->secure; + RESULT_ENSURE_REF(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, &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(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, conn->secrets.version.tls12.master_secret, S2N_TLS_SECRET_LEN)); + 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)); - conn->ems_negotiated = ems_negotiated; + temp_conn.ems_negotiated = ems_negotiated; + } + + DEFER_CLEANUP(struct s2n_blob client_ticket = { 0 }, s2n_free); + if (ticket) { + RESULT_GUARD_POSIX(s2n_dup(ticket, &client_ticket)); } + + /* Finally, actually update the connection */ + RESULT_GUARD_POSIX(s2n_free(&conn->client_ticket)); + *secure = temp_secure; + *conn = temp_conn; + conn->secure = secure; + conn->client_ticket = client_ticket; + ZERO_TO_DISABLE_DEFER_CLEANUP(client_ticket); + return S2N_RESULT_OK; } @@ -337,7 +363,8 @@ static S2N_RESULT s2n_tls13_deserialize_session_state(struct s2n_connection *con return S2N_RESULT_OK; } -S2N_RESULT s2n_deserialize_resumption_state(struct s2n_connection *conn, struct s2n_blob *psk_identity, struct s2n_stuffer *from) +S2N_RESULT s2n_deserialize_resumption_state(struct s2n_connection *conn, + struct s2n_blob *ticket, struct s2n_stuffer *from) { RESULT_ENSURE_REF(conn); RESULT_ENSURE_REF(from); @@ -349,16 +376,10 @@ S2N_RESULT s2n_deserialize_resumption_state(struct s2n_connection *conn, struct if (conn->mode == S2N_SERVER) { RESULT_GUARD_POSIX(s2n_tls12_deserialize_resumption_state(conn, from)); } else { - RESULT_GUARD(s2n_tls12_client_deserialize_session_state(conn, from)); + RESULT_GUARD(s2n_tls12_client_deserialize_session_state(conn, ticket, from)); } } else if (format == S2N_SERIALIZED_FORMAT_TLS13_V1) { - RESULT_GUARD(s2n_tls13_deserialize_session_state(conn, psk_identity, from)); - if (conn->mode == S2N_CLIENT) { - /* Free the client_ticket after setting a psk on the connection. - * This prevents s2n_connection_get_session from returning a TLS1.3 - * ticket before a ticket has been received from the server. */ - RESULT_GUARD_POSIX(s2n_free(&conn->client_ticket)); - } + RESULT_GUARD(s2n_tls13_deserialize_session_state(conn, ticket, from)); } else { RESULT_BAIL(S2N_ERR_INVALID_SERIALIZED_SESSION_STATE); } @@ -386,18 +407,19 @@ static int s2n_client_deserialize_with_session_id(struct s2n_connection *conn, s static int s2n_client_deserialize_with_session_ticket(struct s2n_connection *conn, struct s2n_stuffer *from) { - uint16_t session_ticket_len; + uint16_t session_ticket_len = 0; POSIX_GUARD(s2n_stuffer_read_uint16(from, &session_ticket_len)); if (session_ticket_len == 0 || session_ticket_len > s2n_stuffer_data_available(from)) { POSIX_BAIL(S2N_ERR_INVALID_SERIALIZED_SESSION_STATE); } - POSIX_GUARD(s2n_realloc(&conn->client_ticket, session_ticket_len)); - POSIX_GUARD(s2n_stuffer_read(from, &conn->client_ticket)); - - POSIX_GUARD_RESULT(s2n_deserialize_resumption_state(conn, &conn->client_ticket, from)); + struct s2n_blob session_ticket = { 0 }; + uint8_t *session_ticket_bytes = s2n_stuffer_raw_read(from, session_ticket_len); + POSIX_ENSURE_REF(session_ticket_bytes); + POSIX_GUARD(s2n_blob_init(&session_ticket, session_ticket_bytes, session_ticket_len)); + POSIX_GUARD_RESULT(s2n_deserialize_resumption_state(conn, &session_ticket, from)); return 0; } diff --git a/utils/s2n_mem.c b/utils/s2n_mem.c index eb332678311..c5fa06c3a08 100644 --- a/utils/s2n_mem.c +++ b/utils/s2n_mem.c @@ -232,6 +232,8 @@ int s2n_free_object(uint8_t **p_data, uint32_t size) int s2n_dup(struct s2n_blob *from, struct s2n_blob *to) { POSIX_ENSURE(initialized, S2N_ERR_NOT_INITIALIZED); + POSIX_ENSURE_REF(to); + POSIX_ENSURE_REF(from); POSIX_ENSURE_EQ(to->size, 0); POSIX_ENSURE_EQ(to->data, NULL); POSIX_ENSURE_NE(from->size, 0);