From 5ea701569279cb247b014bc7f0f65cab8e3d9657 Mon Sep 17 00:00:00 2001 From: Carol Yeh Date: Tue, 11 Feb 2025 22:47:25 +0000 Subject: [PATCH 01/11] Add async cert validation support --- api/unstable/crl.h | 7 +- .../unit/s2n_cert_validation_callback_test.c | 122 ++++++++++++++++-- tls/s2n_client_cert.c | 12 +- tls/s2n_server_cert.c | 12 +- tls/s2n_x509_validator.c | 32 ++++- tls/s2n_x509_validator.h | 14 +- 6 files changed, 171 insertions(+), 28 deletions(-) diff --git a/api/unstable/crl.h b/api/unstable/crl.h index 0e0388c0c92..cbc1017736b 100644 --- a/api/unstable/crl.h +++ b/api/unstable/crl.h @@ -187,12 +187,13 @@ struct s2n_cert_validation_info; * * If the validation performed in the callback is successful, `s2n_cert_validation_accept()` MUST be called to allow * `s2n_negotiate()` to continue the handshake. If the validation is unsuccessful, `s2n_cert_validation_reject()` - * MUST be called, which will cause `s2n_negotiate()` to error. The behavior of `s2n_negotiate()` is undefined if + * MUST be called, which will cause `s2n_negotiate()` to error. The handshake will be paused and return `S2N_ERR_ASYNC_BLOCKED` if * neither `s2n_cert_validation_accept()` or `s2n_cert_validation_reject()` are called. * * The `info` parameter is passed to the callback in order to call APIs specific to the cert validation callback, like - * `s2n_cert_validation_accept()` and `s2n_cert_validation_reject()`. The `info` argument is only valid for the - * lifetime of the callback, and must not be used after the callback has finished. + * `s2n_cert_validation_accept()` and `s2n_cert_validation_reject()`. The `info` argument shares the same + * lifetime as `s2n_x509_validator`, and must be updated by `s2n_cert_validation_accept()` or `s2n_cert_validation_reject()` + * in order to finish the handshake. * * After calling `s2n_cert_validation_reject()`, `s2n_negotiate()` will fail with a protocol error indicating that * the cert has been rejected from the callback. If more information regarding an application's custom validation diff --git a/tests/unit/s2n_cert_validation_callback_test.c b/tests/unit/s2n_cert_validation_callback_test.c index 01615f53500..36f140aee49 100644 --- a/tests/unit/s2n_cert_validation_callback_test.c +++ b/tests/unit/s2n_cert_validation_callback_test.c @@ -187,16 +187,6 @@ int main(int argc, char *argv[]) .data = { .call_accept_or_reject = true, .accept = false, .return_success = false }, .expected_error = S2N_ERR_CANCELLED }, - { - .data = { .call_accept_or_reject = false, .return_success = false }, - .expected_error = S2N_ERR_CANCELLED - }, - - /* Error if accept or reject wasn't called from the callback */ - { - .data = { .call_accept_or_reject = false, .return_success = true }, - .expected_error = S2N_ERR_INVALID_STATE - }, }; /* clang-format on */ @@ -444,6 +434,118 @@ int main(int argc, char *argv[]) EXPECT_EQUAL(data.invoked_count, 1); } + + /* Async callback is invoked on the client after receiving the server's certificate */ + for (int i = 0; i < s2n_array_len(test_cases); i++) { + DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free); + EXPECT_NOT_NULL(config); + EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, chain_and_key)); + EXPECT_SUCCESS(s2n_config_set_verification_ca_location(config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); + EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "default")); + + struct s2n_cert_validation_data data = test_cases[i].data; + data.call_accept_or_reject = false; // async case + EXPECT_SUCCESS(s2n_config_set_cert_validation_cb(config, s2n_test_cert_validation_callback_self_talk, &data)); + + DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free); + EXPECT_NOT_NULL(server_conn); + EXPECT_SUCCESS(s2n_connection_set_config(server_conn, config)); + + DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free); + EXPECT_NOT_NULL(client_conn); + EXPECT_SUCCESS(s2n_connection_set_config(client_conn, config)); + EXPECT_SUCCESS(s2n_connection_set_blinding(client_conn, S2N_SELF_SERVICE_BLINDING)); + EXPECT_SUCCESS(s2n_set_server_name(client_conn, "localhost")); + + DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close); + EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair)); + EXPECT_SUCCESS(s2n_connection_set_io_pair(client_conn, &io_pair)); + EXPECT_SUCCESS(s2n_connection_set_io_pair(server_conn, &io_pair)); + + s2n_error expected_error = test_cases[i].expected_error; + if (expected_error == S2N_ERR_CANCELLED) { + EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), + expected_error); + continue; + } + + for (int j = 0; j < 3; j++) { + EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), + S2N_ERR_ASYNC_BLOCKED); + } + + if (test_cases[i].data.accept) { + EXPECT_SUCCESS(s2n_cert_validation_accept(&(client_conn->x509_validator.info))); + EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server_conn, client_conn)); + } else { + EXPECT_SUCCESS(s2n_cert_validation_reject(&(client_conn->x509_validator.info))); + EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), + S2N_ERR_CERT_REJECTED); + } + + EXPECT_EQUAL(data.invoked_count, 1); + } + + /* Async callback is invoked on the server after receiving the client's certificate */ + for (int i = 0; i < s2n_array_len(test_cases); i++) { + DEFER_CLEANUP(struct s2n_config *server_config = s2n_config_new(), s2n_config_ptr_free); + EXPECT_NOT_NULL(server_config); + EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(server_config, chain_and_key)); + EXPECT_SUCCESS(s2n_config_set_verification_ca_location(server_config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); + EXPECT_SUCCESS(s2n_config_set_cipher_preferences(server_config, "default")); + EXPECT_SUCCESS(s2n_config_set_client_auth_type(server_config, S2N_CERT_AUTH_REQUIRED)); + + struct s2n_cert_validation_data data = test_cases[i].data; + data.call_accept_or_reject = false; // async case + EXPECT_SUCCESS(s2n_config_set_cert_validation_cb(server_config, + s2n_test_cert_validation_callback_self_talk_server, &data)); + + DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free); + EXPECT_NOT_NULL(server_conn); + EXPECT_SUCCESS(s2n_connection_set_config(server_conn, server_config)); + EXPECT_SUCCESS(s2n_connection_set_blinding(server_conn, S2N_SELF_SERVICE_BLINDING)); + + DEFER_CLEANUP(struct s2n_config *client_config = s2n_config_new(), s2n_config_ptr_free); + EXPECT_NOT_NULL(client_config); + EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(client_config, chain_and_key)); + EXPECT_SUCCESS(s2n_config_set_verification_ca_location(client_config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); + EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_config, "default")); + EXPECT_SUCCESS(s2n_config_set_client_auth_type(client_config, S2N_CERT_AUTH_OPTIONAL)); + + DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free); + EXPECT_NOT_NULL(client_conn); + EXPECT_SUCCESS(s2n_connection_set_config(client_conn, client_config)); + EXPECT_SUCCESS(s2n_set_server_name(client_conn, "localhost")); + + DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close); + EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair)); + EXPECT_SUCCESS(s2n_connection_set_io_pair(client_conn, &io_pair)); + EXPECT_SUCCESS(s2n_connection_set_io_pair(server_conn, &io_pair)); + + s2n_error expected_error = test_cases[i].expected_error; + if (expected_error == S2N_ERR_CANCELLED) { + EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), + expected_error); + continue; + } + + for (int j = 0; j < 3; j++) { + EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), + S2N_ERR_ASYNC_BLOCKED); + } + + if (test_cases[i].data.accept) { + EXPECT_SUCCESS(s2n_cert_validation_accept(&(server_conn->x509_validator.info))); + EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server_conn, client_conn)); + } else { + EXPECT_SUCCESS(s2n_cert_validation_reject(&(server_conn->x509_validator.info))); + EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), + S2N_ERR_CERT_REJECTED); + } + + EXPECT_EQUAL(data.invoked_count, 1); + } + } END_TEST(); diff --git a/tls/s2n_client_cert.c b/tls/s2n_client_cert.c index 8d4af88645c..88b037f7729 100644 --- a/tls/s2n_client_cert.c +++ b/tls/s2n_client_cert.c @@ -101,6 +101,7 @@ static S2N_RESULT s2n_client_cert_chain_store(struct s2n_connection *conn, int s2n_client_cert_recv(struct s2n_connection *conn) { + uint32_t cert_start = conn->handshake.io.read_cursor; if (conn->actual_protocol_version == S2N_TLS13) { uint8_t certificate_request_context_len = 0; POSIX_GUARD(s2n_stuffer_read_uint8(&conn->handshake.io, &certificate_request_context_len)); @@ -130,8 +131,15 @@ int s2n_client_cert_recv(struct s2n_connection *conn) /* Determine the Cert Type, Verify the Cert, and extract the Public Key */ s2n_pkey_type pkey_type = S2N_PKEY_TYPE_UNKNOWN; - POSIX_GUARD_RESULT(s2n_x509_validator_validate_cert_chain(&conn->x509_validator, conn, cert_chain_data, - cert_chain_size, &pkey_type, &public_key)); + s2n_result result = s2n_x509_validator_validate_cert_chain(&conn->x509_validator, conn, cert_chain_data, + cert_chain_size, &pkey_type, &public_key); + if (s2n_result_is_error(result)) { + /* reset the stuffer cursor to where cert data start in order to prepare for re-entry, because + * s2n_handle_retry_state() will invoke s2n_client_cert_recv() directly without updating stuffer + */ + conn->handshake.io.read_cursor = cert_start; + POSIX_GUARD_RESULT(result); + } conn->handshake_params.client_cert_pkey_type = pkey_type; POSIX_GUARD_RESULT(s2n_pkey_setup_for_type(&public_key, pkey_type)); diff --git a/tls/s2n_server_cert.c b/tls/s2n_server_cert.c index 5c1882ceb29..63315de85d5 100644 --- a/tls/s2n_server_cert.c +++ b/tls/s2n_server_cert.c @@ -22,6 +22,7 @@ int s2n_server_cert_recv(struct s2n_connection *conn) { + uint32_t cert_start = conn->handshake.io.read_cursor; if (conn->actual_protocol_version == S2N_TLS13) { uint8_t certificate_request_context_len = 0; POSIX_GUARD(s2n_stuffer_read_uint8(&conn->handshake.io, &certificate_request_context_len)); @@ -43,8 +44,15 @@ int s2n_server_cert_recv(struct s2n_connection *conn) cert_chain.data = s2n_stuffer_raw_read(&conn->handshake.io, size_of_all_certificates); POSIX_ENSURE_REF(cert_chain.data); - POSIX_GUARD_RESULT(s2n_x509_validator_validate_cert_chain(&conn->x509_validator, conn, cert_chain.data, - cert_chain.size, &actual_cert_pkey_type, &public_key)); + s2n_result result = s2n_x509_validator_validate_cert_chain(&conn->x509_validator, conn, cert_chain.data, + cert_chain.size, &actual_cert_pkey_type, &public_key); + if (s2n_result_is_error(result)) { + /* reset the stuffer cursor to where cert data start in order to prepare for re-entry, because + * s2n_handle_retry_state() will invoke s2n_server_cert_recv() directly without updating stuffer + */ + conn->handshake.io.read_cursor = cert_start; + POSIX_GUARD_RESULT(result); + } POSIX_GUARD(s2n_is_cert_type_valid_for_auth(conn, actual_cert_pkey_type)); POSIX_GUARD_RESULT(s2n_pkey_setup_for_type(&public_key, actual_cert_pkey_type)); diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c index 4de3c03b1ff..af92fecb230 100644 --- a/tls/s2n_x509_validator.c +++ b/tls/s2n_x509_validator.c @@ -150,6 +150,7 @@ int s2n_x509_validator_init_no_x509_validation(struct s2n_x509_validator *valida validator->state = INIT; validator->cert_chain_from_wire = sk_X509_new_null(); validator->crl_lookup_list = NULL; + validator->info = (struct s2n_cert_validation_info) { 0 }; return 0; } @@ -169,6 +170,7 @@ int s2n_x509_validator_init(struct s2n_x509_validator *validator, struct s2n_x50 validator->cert_chain_from_wire = sk_X509_new_null(); validator->state = INIT; validator->crl_lookup_list = NULL; + validator->info = (struct s2n_cert_validation_info) { 0 }; return 0; } @@ -763,6 +765,9 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val case AWAITING_CRL_CALLBACK: RESULT_GUARD(s2n_crl_handle_lookup_callback_result(validator)); break; + case AWAITING_VALIDATE_CALLBACK: + RESULT_GUARD(s2n_handle_cert_validation_callback_result(validator)); + break; default: RESULT_BAIL(S2N_ERR_INVALID_CERT_STATE); } @@ -776,7 +781,8 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val RESULT_GUARD(s2n_x509_validator_check_root_cert(validator, conn)); } - if (conn->actual_protocol_version >= S2N_TLS13) { + /* skip this step on the re-entry case */ + if (validator->state != AWAITING_VALIDATE_CALLBACK && conn->actual_protocol_version >= S2N_TLS13) { /* Only process certificate extensions received in the first certificate. Extensions received in all other * certificates are ignored. * @@ -789,13 +795,14 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val RESULT_GUARD_POSIX(s2n_extension_list_process(S2N_EXTENSION_LIST_CERTIFICATE, conn, &first_certificate_extensions)); } - if (conn->config->cert_validation_cb) { - struct s2n_cert_validation_info info = { 0 }; - RESULT_ENSURE(conn->config->cert_validation_cb(conn, &info, conn->config->cert_validation_ctx) >= S2N_SUCCESS, + /* skip this step on the re-entry case */ + if (validator->state != AWAITING_VALIDATE_CALLBACK && conn->config->cert_validation_cb) { + RESULT_ENSURE(conn->config->cert_validation_cb(conn, &(validator->info), conn->config->cert_validation_ctx) >= S2N_SUCCESS, S2N_ERR_CANCELLED); - RESULT_ENSURE(info.finished, S2N_ERR_INVALID_STATE); - RESULT_ENSURE(info.accepted, S2N_ERR_CERT_REJECTED); + RESULT_GUARD(s2n_handle_cert_validation_callback_result(validator)); } + /* update state after completing the async validation */ + validator->state = VALIDATED; /* retrieve information from leaf cert */ RESULT_ENSURE_GT(sk_X509_num(validator->cert_chain_from_wire), 0); @@ -960,6 +967,19 @@ bool s2n_x509_validator_is_cert_chain_validated(const struct s2n_x509_validator return validator && (validator->state == VALIDATED || validator->state == OCSP_VALIDATED); } +S2N_RESULT s2n_handle_cert_validation_callback_result(struct s2n_x509_validator *validator) +{ + RESULT_ENSURE_REF(validator); + + if (!validator->info.finished) { + validator->state = AWAITING_VALIDATE_CALLBACK; + RESULT_BAIL(S2N_ERR_ASYNC_BLOCKED); + } + + RESULT_ENSURE(validator->info.accepted, S2N_ERR_CERT_REJECTED); + return S2N_RESULT_OK; +} + int s2n_cert_validation_accept(struct s2n_cert_validation_info *info) { POSIX_ENSURE_REF(info); diff --git a/tls/s2n_x509_validator.h b/tls/s2n_x509_validator.h index 7706fc0a031..7284aa18531 100644 --- a/tls/s2n_x509_validator.h +++ b/tls/s2n_x509_validator.h @@ -35,6 +35,7 @@ typedef enum { AWAITING_CRL_CALLBACK, VALIDATED, OCSP_VALIDATED, + AWAITING_VALIDATE_CALLBACK, } validator_state; /** Return TRUE for trusted, FALSE for untrusted **/ @@ -52,6 +53,11 @@ struct s2n_x509_trust_store { unsigned loaded_system_certs : 1; }; +struct s2n_cert_validation_info { + unsigned finished : 1; + unsigned accepted : 1; +}; + /** * You should have one instance of this per connection. */ @@ -64,11 +70,7 @@ struct s2n_x509_validator { STACK_OF(X509) *cert_chain_from_wire; int state; struct s2n_array *crl_lookup_list; -}; - -struct s2n_cert_validation_info { - unsigned finished : 1; - unsigned accepted : 1; + struct s2n_cert_validation_info info; }; /** Some libcrypto implementations do not support OCSP validation. Returns 1 if supported, 0 otherwise. */ @@ -133,3 +135,5 @@ S2N_RESULT s2n_x509_validator_validate_cert_stapled_ocsp_response(struct s2n_x50 * Should be verified before any use of the peer's certificate data. */ bool s2n_x509_validator_is_cert_chain_validated(const struct s2n_x509_validator *validator); + +S2N_RESULT s2n_handle_cert_validation_callback_result(struct s2n_x509_validator *validator); From 054ca4fb9458a52fec17211132c7d546d9a2141a Mon Sep 17 00:00:00 2001 From: Carol Yeh Date: Wed, 12 Feb 2025 01:52:38 +0000 Subject: [PATCH 02/11] Fix s2n_certificate_test and formatting --- tests/unit/s2n_cert_validation_callback_test.c | 7 ++++--- tls/s2n_x509_validator.c | 9 ++++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/unit/s2n_cert_validation_callback_test.c b/tests/unit/s2n_cert_validation_callback_test.c index 36f140aee49..116175619a1 100644 --- a/tests/unit/s2n_cert_validation_callback_test.c +++ b/tests/unit/s2n_cert_validation_callback_test.c @@ -444,7 +444,8 @@ int main(int argc, char *argv[]) EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "default")); struct s2n_cert_validation_data data = test_cases[i].data; - data.call_accept_or_reject = false; // async case + /* async case */ + data.call_accept_or_reject = false; EXPECT_SUCCESS(s2n_config_set_cert_validation_cb(config, s2n_test_cert_validation_callback_self_talk, &data)); DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free); @@ -496,7 +497,8 @@ int main(int argc, char *argv[]) EXPECT_SUCCESS(s2n_config_set_client_auth_type(server_config, S2N_CERT_AUTH_REQUIRED)); struct s2n_cert_validation_data data = test_cases[i].data; - data.call_accept_or_reject = false; // async case + /* async case */ + data.call_accept_or_reject = false; EXPECT_SUCCESS(s2n_config_set_cert_validation_cb(server_config, s2n_test_cert_validation_callback_self_talk_server, &data)); @@ -545,7 +547,6 @@ int main(int argc, char *argv[]) EXPECT_EQUAL(data.invoked_count, 1); } - } END_TEST(); diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c index af92fecb230..5880642351e 100644 --- a/tls/s2n_x509_validator.c +++ b/tls/s2n_x509_validator.c @@ -150,7 +150,7 @@ int s2n_x509_validator_init_no_x509_validation(struct s2n_x509_validator *valida validator->state = INIT; validator->cert_chain_from_wire = sk_X509_new_null(); validator->crl_lookup_list = NULL; - validator->info = (struct s2n_cert_validation_info) { 0 }; + validator->info = (struct s2n_cert_validation_info){ 0 }; return 0; } @@ -170,7 +170,7 @@ int s2n_x509_validator_init(struct s2n_x509_validator *validator, struct s2n_x50 validator->cert_chain_from_wire = sk_X509_new_null(); validator->state = INIT; validator->crl_lookup_list = NULL; - validator->info = (struct s2n_cert_validation_info) { 0 }; + validator->info = (struct s2n_cert_validation_info){ 0 }; return 0; } @@ -801,8 +801,11 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val S2N_ERR_CANCELLED); RESULT_GUARD(s2n_handle_cert_validation_callback_result(validator)); } + /* update state after completing the async validation */ - validator->state = VALIDATED; + if (validator->state == AWAITING_VALIDATE_CALLBACK) { + validator->state = VALIDATED; + } /* retrieve information from leaf cert */ RESULT_ENSURE_GT(sk_X509_num(validator->cert_chain_from_wire), 0); From dc1a52850e58b9a6a0d0cb937ecb28e47cf46c98 Mon Sep 17 00:00:00 2001 From: Carol Yeh Date: Wed, 12 Feb 2025 02:12:23 +0000 Subject: [PATCH 03/11] Fix clang formatting --- tls/s2n_x509_validator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c index 5880642351e..617f9d79ebf 100644 --- a/tls/s2n_x509_validator.c +++ b/tls/s2n_x509_validator.c @@ -801,7 +801,7 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val S2N_ERR_CANCELLED); RESULT_GUARD(s2n_handle_cert_validation_callback_result(validator)); } - + /* update state after completing the async validation */ if (validator->state == AWAITING_VALIDATE_CALLBACK) { validator->state = VALIDATED; From b6660876995bf2d187595820f5c5687fddd852a3 Mon Sep 17 00:00:00 2001 From: Carol Yeh Date: Thu, 13 Feb 2025 19:10:54 +0000 Subject: [PATCH 04/11] refactor and add comments --- api/unstable/crl.h | 14 ++-- .../unit/s2n_cert_validation_callback_test.c | 73 +++++++++++-------- tls/s2n_client_cert.c | 13 ++-- tls/s2n_server_cert.c | 13 ++-- tls/s2n_x509_validator.c | 48 ++++++------ tls/s2n_x509_validator.h | 6 +- 6 files changed, 98 insertions(+), 69 deletions(-) diff --git a/api/unstable/crl.h b/api/unstable/crl.h index cbc1017736b..44ab4dc7836 100644 --- a/api/unstable/crl.h +++ b/api/unstable/crl.h @@ -185,15 +185,19 @@ struct s2n_cert_validation_info; * - `s2n_connection_get_peer_cert_chain()` * - `s2n_connection_get_client_cert_chain()` * - * If the validation performed in the callback is successful, `s2n_cert_validation_accept()` MUST be called to allow + * When using the validation callback in a synchronous mode, `s2n_cert_validation_accept()` MUST be called to allow * `s2n_negotiate()` to continue the handshake. If the validation is unsuccessful, `s2n_cert_validation_reject()` - * MUST be called, which will cause `s2n_negotiate()` to error. The handshake will be paused and return `S2N_ERR_ASYNC_BLOCKED` if - * neither `s2n_cert_validation_accept()` or `s2n_cert_validation_reject()` are called. + * MUST be called, which will cause `s2n_negotiate()` to error. If neither `s2n_cert_validation_accept()` or + * `s2n_cert_validation_reject()` are called, the validation can be processed asynchronously. + * + * To enable the async behavior, `S2N_SUCCESS` must be returned from the validation callback. Then the handshake will + * be paused and `s2n_negotiate()` will throw a `S2N_ERR_T_BLOCKED` error. The application should check for this error and + * check `s2n_blocked_status` is set to `S2N_BLOCKED_ON_APPLICATION_INPUT`. * * The `info` parameter is passed to the callback in order to call APIs specific to the cert validation callback, like * `s2n_cert_validation_accept()` and `s2n_cert_validation_reject()`. The `info` argument shares the same - * lifetime as `s2n_x509_validator`, and must be updated by `s2n_cert_validation_accept()` or `s2n_cert_validation_reject()` - * in order to finish the handshake. + * lifetime as `s2n_connection`. In order to unpause the handshake, the application should update the `info` parameter + * by calling `s2n_cert_validation_accept()` or `s2n_cert_validation_reject()` before retrying `s2n_negotiate()`. * * After calling `s2n_cert_validation_reject()`, `s2n_negotiate()` will fail with a protocol error indicating that * the cert has been rejected from the callback. If more information regarding an application's custom validation diff --git a/tests/unit/s2n_cert_validation_callback_test.c b/tests/unit/s2n_cert_validation_callback_test.c index 116175619a1..1b4eea4ba30 100644 --- a/tests/unit/s2n_cert_validation_callback_test.c +++ b/tests/unit/s2n_cert_validation_callback_test.c @@ -23,6 +23,7 @@ struct s2n_cert_validation_data { unsigned return_success : 1; int invoked_count; + struct s2n_cert_validation_info *info; }; static int s2n_test_cert_validation_callback(struct s2n_connection *conn, struct s2n_cert_validation_info *info, void *ctx) @@ -30,6 +31,7 @@ static int s2n_test_cert_validation_callback(struct s2n_connection *conn, struct struct s2n_cert_validation_data *data = (struct s2n_cert_validation_data *) ctx; data->invoked_count += 1; + data->info = info; int ret = S2N_FAILURE; if (data->return_success) { @@ -435,17 +437,40 @@ int main(int argc, char *argv[]) EXPECT_EQUAL(data.invoked_count, 1); } + struct { + const struct s2n_cert_validation_data data; + const char *version; + } async_test_cases[] = { + /* For async cases, accept or reject API will be called outside of the validation callback. + * Iterate over both TLS 1.3 and 1.2 policies to ensure the stuffer reset logic works in all cases. + */ + { + .data = { .call_accept_or_reject = false, .accept = true, .return_success = true }, + .version = "20240501" + }, + { + .data = { .call_accept_or_reject = false, .accept = false, .return_success = true }, + .version = "20240501" + }, + { + .data = { .call_accept_or_reject = false, .accept = true, .return_success = true }, + .version = "20170210" + }, + { + .data = { .call_accept_or_reject = false, .accept = false, .return_success = true }, + .version = "20170210" + }, + }; + /* Async callback is invoked on the client after receiving the server's certificate */ - for (int i = 0; i < s2n_array_len(test_cases); i++) { + for (int i = 0; i < s2n_array_len(async_test_cases); i++) { DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free); EXPECT_NOT_NULL(config); EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, chain_and_key)); EXPECT_SUCCESS(s2n_config_set_verification_ca_location(config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); - EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "default")); + EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, async_test_cases[i].version)); - struct s2n_cert_validation_data data = test_cases[i].data; - /* async case */ - data.call_accept_or_reject = false; + struct s2n_cert_validation_data data = async_test_cases[i].data; EXPECT_SUCCESS(s2n_config_set_cert_validation_cb(config, s2n_test_cert_validation_callback_self_talk, &data)); DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free); @@ -463,23 +488,19 @@ int main(int argc, char *argv[]) EXPECT_SUCCESS(s2n_connection_set_io_pair(client_conn, &io_pair)); EXPECT_SUCCESS(s2n_connection_set_io_pair(server_conn, &io_pair)); - s2n_error expected_error = test_cases[i].expected_error; - if (expected_error == S2N_ERR_CANCELLED) { - EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), - expected_error); - continue; - } - for (int j = 0; j < 3; j++) { EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), S2N_ERR_ASYNC_BLOCKED); } + struct s2n_cert_validation_info *info = data.info; + EXPECT_NOT_NULL(info); + if (test_cases[i].data.accept) { - EXPECT_SUCCESS(s2n_cert_validation_accept(&(client_conn->x509_validator.info))); + EXPECT_SUCCESS(s2n_cert_validation_accept(info)); EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server_conn, client_conn)); } else { - EXPECT_SUCCESS(s2n_cert_validation_reject(&(client_conn->x509_validator.info))); + EXPECT_SUCCESS(s2n_cert_validation_reject(info)); EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), S2N_ERR_CERT_REJECTED); } @@ -488,17 +509,15 @@ int main(int argc, char *argv[]) } /* Async callback is invoked on the server after receiving the client's certificate */ - for (int i = 0; i < s2n_array_len(test_cases); i++) { + for (int i = 0; i < s2n_array_len(async_test_cases); i++) { DEFER_CLEANUP(struct s2n_config *server_config = s2n_config_new(), s2n_config_ptr_free); EXPECT_NOT_NULL(server_config); EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(server_config, chain_and_key)); EXPECT_SUCCESS(s2n_config_set_verification_ca_location(server_config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); - EXPECT_SUCCESS(s2n_config_set_cipher_preferences(server_config, "default")); + EXPECT_SUCCESS(s2n_config_set_cipher_preferences(server_config, async_test_cases[i].version)); EXPECT_SUCCESS(s2n_config_set_client_auth_type(server_config, S2N_CERT_AUTH_REQUIRED)); - struct s2n_cert_validation_data data = test_cases[i].data; - /* async case */ - data.call_accept_or_reject = false; + struct s2n_cert_validation_data data = async_test_cases[i].data; EXPECT_SUCCESS(s2n_config_set_cert_validation_cb(server_config, s2n_test_cert_validation_callback_self_talk_server, &data)); @@ -511,7 +530,7 @@ int main(int argc, char *argv[]) EXPECT_NOT_NULL(client_config); EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(client_config, chain_and_key)); EXPECT_SUCCESS(s2n_config_set_verification_ca_location(client_config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); - EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_config, "default")); + EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_config, async_test_cases[i].version)); EXPECT_SUCCESS(s2n_config_set_client_auth_type(client_config, S2N_CERT_AUTH_OPTIONAL)); DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free); @@ -524,23 +543,19 @@ int main(int argc, char *argv[]) EXPECT_SUCCESS(s2n_connection_set_io_pair(client_conn, &io_pair)); EXPECT_SUCCESS(s2n_connection_set_io_pair(server_conn, &io_pair)); - s2n_error expected_error = test_cases[i].expected_error; - if (expected_error == S2N_ERR_CANCELLED) { - EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), - expected_error); - continue; - } - for (int j = 0; j < 3; j++) { EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), S2N_ERR_ASYNC_BLOCKED); } + struct s2n_cert_validation_info *info = data.info; + EXPECT_NOT_NULL(info); + if (test_cases[i].data.accept) { - EXPECT_SUCCESS(s2n_cert_validation_accept(&(server_conn->x509_validator.info))); + EXPECT_SUCCESS(s2n_cert_validation_accept(info)); EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server_conn, client_conn)); } else { - EXPECT_SUCCESS(s2n_cert_validation_reject(&(server_conn->x509_validator.info))); + EXPECT_SUCCESS(s2n_cert_validation_reject(info)); EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), S2N_ERR_CERT_REJECTED); } diff --git a/tls/s2n_client_cert.c b/tls/s2n_client_cert.c index 88b037f7729..f9b10ac7d7d 100644 --- a/tls/s2n_client_cert.c +++ b/tls/s2n_client_cert.c @@ -101,7 +101,8 @@ static S2N_RESULT s2n_client_cert_chain_store(struct s2n_connection *conn, int s2n_client_cert_recv(struct s2n_connection *conn) { - uint32_t cert_start = conn->handshake.io.read_cursor; + uint32_t cert_message_start = conn->handshake.io.read_cursor; + if (conn->actual_protocol_version == S2N_TLS13) { uint8_t certificate_request_context_len = 0; POSIX_GUARD(s2n_stuffer_read_uint8(&conn->handshake.io, &certificate_request_context_len)); @@ -133,13 +134,15 @@ int s2n_client_cert_recv(struct s2n_connection *conn) s2n_pkey_type pkey_type = S2N_PKEY_TYPE_UNKNOWN; s2n_result result = s2n_x509_validator_validate_cert_chain(&conn->x509_validator, conn, cert_chain_data, cert_chain_size, &pkey_type, &public_key); - if (s2n_result_is_error(result)) { - /* reset the stuffer cursor to where cert data start in order to prepare for re-entry, because + if (s2n_result_is_error(result) && s2n_errno == S2N_ERR_ASYNC_BLOCKED) { + /* s2n_x509_validator_validate_cert_chain() handles async callbacks, which may require re-entering the handshake; + * need to reset the stuffer cursor to where cert message starts in order to prepare for a re-entry, because * s2n_handle_retry_state() will invoke s2n_client_cert_recv() directly without updating stuffer */ - conn->handshake.io.read_cursor = cert_start; - POSIX_GUARD_RESULT(result); + POSIX_GUARD(s2n_stuffer_reread(&conn->handshake.io)); + POSIX_GUARD(s2n_stuffer_skip_read(&conn->handshake.io, cert_message_start)); } + POSIX_GUARD_RESULT(result); conn->handshake_params.client_cert_pkey_type = pkey_type; POSIX_GUARD_RESULT(s2n_pkey_setup_for_type(&public_key, pkey_type)); diff --git a/tls/s2n_server_cert.c b/tls/s2n_server_cert.c index 63315de85d5..24dc72ccfd4 100644 --- a/tls/s2n_server_cert.c +++ b/tls/s2n_server_cert.c @@ -22,7 +22,8 @@ int s2n_server_cert_recv(struct s2n_connection *conn) { - uint32_t cert_start = conn->handshake.io.read_cursor; + uint32_t cert_message_start = conn->handshake.io.read_cursor; + if (conn->actual_protocol_version == S2N_TLS13) { uint8_t certificate_request_context_len = 0; POSIX_GUARD(s2n_stuffer_read_uint8(&conn->handshake.io, &certificate_request_context_len)); @@ -46,13 +47,15 @@ int s2n_server_cert_recv(struct s2n_connection *conn) s2n_result result = s2n_x509_validator_validate_cert_chain(&conn->x509_validator, conn, cert_chain.data, cert_chain.size, &actual_cert_pkey_type, &public_key); - if (s2n_result_is_error(result)) { - /* reset the stuffer cursor to where cert data start in order to prepare for re-entry, because + if (s2n_result_is_error(result) && s2n_errno == S2N_ERR_ASYNC_BLOCKED) { + /* s2n_x509_validator_validate_cert_chain() handles async callbacks, which may require re-entering the handshake; + * need to reset the stuffer cursor to where cert message starts in order to prepare for a re-entry, because * s2n_handle_retry_state() will invoke s2n_server_cert_recv() directly without updating stuffer */ - conn->handshake.io.read_cursor = cert_start; - POSIX_GUARD_RESULT(result); + POSIX_GUARD(s2n_stuffer_reread(&conn->handshake.io)); + POSIX_GUARD(s2n_stuffer_skip_read(&conn->handshake.io, cert_message_start)); } + POSIX_GUARD_RESULT(result); POSIX_GUARD(s2n_is_cert_type_valid_for_auth(conn, actual_cert_pkey_type)); POSIX_GUARD_RESULT(s2n_pkey_setup_for_type(&public_key, actual_cert_pkey_type)); diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c index 617f9d79ebf..061c2d664b9 100644 --- a/tls/s2n_x509_validator.c +++ b/tls/s2n_x509_validator.c @@ -150,7 +150,8 @@ int s2n_x509_validator_init_no_x509_validation(struct s2n_x509_validator *valida validator->state = INIT; validator->cert_chain_from_wire = sk_X509_new_null(); validator->crl_lookup_list = NULL; - validator->info = (struct s2n_cert_validation_info){ 0 }; + validator->cert_validation_info = (struct s2n_cert_validation_info){ 0 }; + validator->cert_validation_cb_invoked = false; return 0; } @@ -170,7 +171,8 @@ int s2n_x509_validator_init(struct s2n_x509_validator *validator, struct s2n_x50 validator->cert_chain_from_wire = sk_X509_new_null(); validator->state = INIT; validator->crl_lookup_list = NULL; - validator->info = (struct s2n_cert_validation_info){ 0 }; + validator->cert_validation_info = (struct s2n_cert_validation_info){ 0 }; + validator->cert_validation_cb_invoked = false; return 0; } @@ -753,8 +755,8 @@ static S2N_RESULT s2n_x509_validator_parse_leaf_certificate_extensions(struct s2 return S2N_RESULT_OK; } -S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *validator, struct s2n_connection *conn, - uint8_t *cert_chain_in, uint32_t cert_chain_len, s2n_pkey_type *pkey_type, struct s2n_pkey *public_key_out) +S2N_RESULT s2n_x509_validator_validate_cert_chain_pre(struct s2n_x509_validator *validator, struct s2n_connection *conn, + uint8_t *cert_chain_in, uint32_t cert_chain_len) { RESULT_ENSURE_REF(conn); RESULT_ENSURE_REF(conn->config); @@ -765,9 +767,6 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val case AWAITING_CRL_CALLBACK: RESULT_GUARD(s2n_crl_handle_lookup_callback_result(validator)); break; - case AWAITING_VALIDATE_CALLBACK: - RESULT_GUARD(s2n_handle_cert_validation_callback_result(validator)); - break; default: RESULT_BAIL(S2N_ERR_INVALID_CERT_STATE); } @@ -781,8 +780,7 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val RESULT_GUARD(s2n_x509_validator_check_root_cert(validator, conn)); } - /* skip this step on the re-entry case */ - if (validator->state != AWAITING_VALIDATE_CALLBACK && conn->actual_protocol_version >= S2N_TLS13) { + if (conn->actual_protocol_version >= S2N_TLS13) { /* Only process certificate extensions received in the first certificate. Extensions received in all other * certificates are ignored. * @@ -795,16 +793,23 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val RESULT_GUARD_POSIX(s2n_extension_list_process(S2N_EXTENSION_LIST_CERTIFICATE, conn, &first_certificate_extensions)); } - /* skip this step on the re-entry case */ - if (validator->state != AWAITING_VALIDATE_CALLBACK && conn->config->cert_validation_cb) { - RESULT_ENSURE(conn->config->cert_validation_cb(conn, &(validator->info), conn->config->cert_validation_ctx) >= S2N_SUCCESS, - S2N_ERR_CANCELLED); - RESULT_GUARD(s2n_handle_cert_validation_callback_result(validator)); - } + return S2N_RESULT_OK; +} - /* update state after completing the async validation */ - if (validator->state == AWAITING_VALIDATE_CALLBACK) { - validator->state = VALIDATED; +S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *validator, struct s2n_connection *conn, + uint8_t *cert_chain_in, uint32_t cert_chain_len, s2n_pkey_type *pkey_type, struct s2n_pkey *public_key_out) +{ + if (validator->cert_validation_cb_invoked) { + RESULT_GUARD(s2n_x509_validator_handle_cert_validation_callback_result(validator)); + } else { + RESULT_GUARD(s2n_x509_validator_validate_cert_chain_pre(validator, conn, cert_chain_in, cert_chain_len)); + + if (conn->config->cert_validation_cb) { + RESULT_ENSURE(conn->config->cert_validation_cb(conn, &(validator->cert_validation_info), conn->config->cert_validation_ctx) >= S2N_SUCCESS, + S2N_ERR_CANCELLED); + validator->cert_validation_cb_invoked = true; + RESULT_GUARD(s2n_x509_validator_handle_cert_validation_callback_result(validator)); + } } /* retrieve information from leaf cert */ @@ -970,16 +975,15 @@ bool s2n_x509_validator_is_cert_chain_validated(const struct s2n_x509_validator return validator && (validator->state == VALIDATED || validator->state == OCSP_VALIDATED); } -S2N_RESULT s2n_handle_cert_validation_callback_result(struct s2n_x509_validator *validator) +S2N_RESULT s2n_x509_validator_handle_cert_validation_callback_result(struct s2n_x509_validator *validator) { RESULT_ENSURE_REF(validator); - if (!validator->info.finished) { - validator->state = AWAITING_VALIDATE_CALLBACK; + if (!validator->cert_validation_info.finished) { RESULT_BAIL(S2N_ERR_ASYNC_BLOCKED); } - RESULT_ENSURE(validator->info.accepted, S2N_ERR_CERT_REJECTED); + RESULT_ENSURE(validator->cert_validation_info.accepted, S2N_ERR_CERT_REJECTED); return S2N_RESULT_OK; } diff --git a/tls/s2n_x509_validator.h b/tls/s2n_x509_validator.h index 7284aa18531..5237f3e68f3 100644 --- a/tls/s2n_x509_validator.h +++ b/tls/s2n_x509_validator.h @@ -35,7 +35,6 @@ typedef enum { AWAITING_CRL_CALLBACK, VALIDATED, OCSP_VALIDATED, - AWAITING_VALIDATE_CALLBACK, } validator_state; /** Return TRUE for trusted, FALSE for untrusted **/ @@ -70,7 +69,8 @@ struct s2n_x509_validator { STACK_OF(X509) *cert_chain_from_wire; int state; struct s2n_array *crl_lookup_list; - struct s2n_cert_validation_info info; + struct s2n_cert_validation_info cert_validation_info; + bool cert_validation_cb_invoked; }; /** Some libcrypto implementations do not support OCSP validation. Returns 1 if supported, 0 otherwise. */ @@ -136,4 +136,4 @@ S2N_RESULT s2n_x509_validator_validate_cert_stapled_ocsp_response(struct s2n_x50 */ bool s2n_x509_validator_is_cert_chain_validated(const struct s2n_x509_validator *validator); -S2N_RESULT s2n_handle_cert_validation_callback_result(struct s2n_x509_validator *validator); +S2N_RESULT s2n_x509_validator_handle_cert_validation_callback_result(struct s2n_x509_validator *validator); From 86dcbbf3a5a328faac6c4522b764188108f20778 Mon Sep 17 00:00:00 2001 From: Carol Yeh Date: Thu, 13 Feb 2025 23:14:14 +0000 Subject: [PATCH 05/11] improve re-entry handling and test cases --- .../unit/s2n_cert_validation_callback_test.c | 13 +++++++ tls/s2n_client_cert.c | 38 +++++++++---------- tls/s2n_server_cert.c | 26 +++++-------- tls/s2n_x509_validator.c | 4 +- 4 files changed, 43 insertions(+), 38 deletions(-) diff --git a/tests/unit/s2n_cert_validation_callback_test.c b/tests/unit/s2n_cert_validation_callback_test.c index 1b4eea4ba30..3f6cb8dff52 100644 --- a/tests/unit/s2n_cert_validation_callback_test.c +++ b/tests/unit/s2n_cert_validation_callback_test.c @@ -437,6 +437,7 @@ int main(int argc, char *argv[]) EXPECT_EQUAL(data.invoked_count, 1); } + /* clang-format off */ struct { const struct s2n_cert_validation_data data; const char *version; @@ -461,6 +462,7 @@ int main(int argc, char *argv[]) .version = "20170210" }, }; + /* clang-format on */ /* Async callback is invoked on the client after receiving the server's certificate */ for (int i = 0; i < s2n_array_len(async_test_cases); i++) { @@ -493,6 +495,12 @@ int main(int argc, char *argv[]) S2N_ERR_ASYNC_BLOCKED); } + /* Ensure that the server's certificate chain can be retrieved after `S2N_ERR_ASYNC_BLOCKED` */ + DEFER_CLEANUP(struct s2n_cert_chain_and_key *peer_cert_chain = s2n_cert_chain_and_key_new(), + s2n_cert_chain_and_key_ptr_free); + EXPECT_NOT_NULL(peer_cert_chain); + EXPECT_SUCCESS(s2n_connection_get_peer_cert_chain(client_conn, peer_cert_chain)); + struct s2n_cert_validation_info *info = data.info; EXPECT_NOT_NULL(info); @@ -548,6 +556,11 @@ int main(int argc, char *argv[]) S2N_ERR_ASYNC_BLOCKED); } + /* Ensure that the client's certificate chain can be retrieved after `S2N_ERR_ASYNC_BLOCKED` */ + uint8_t *der_cert_chain = 0; + uint32_t cert_chain_len = 0; + EXPECT_SUCCESS(s2n_connection_get_client_cert_chain(server_conn, &der_cert_chain, &cert_chain_len)); + struct s2n_cert_validation_info *info = data.info; EXPECT_NOT_NULL(info); diff --git a/tls/s2n_client_cert.c b/tls/s2n_client_cert.c index f9b10ac7d7d..4f08a00d5d7 100644 --- a/tls/s2n_client_cert.c +++ b/tls/s2n_client_cert.c @@ -57,8 +57,12 @@ static S2N_RESULT s2n_client_cert_chain_store(struct s2n_connection *conn, RESULT_ENSURE_REF(conn); RESULT_ENSURE_REF(raw_cert_chain); - /* There shouldn't already be a client cert chain, but free just in case */ - RESULT_GUARD_POSIX(s2n_free(&conn->handshake_params.client_cert_chain)); + /* If a client cert chain has already been stored (e.g. on the re-entry case + * of an async callback), no need to read the cert chain again. + */ + if (conn->handshake_params.client_cert_chain.size > 0) { + return S2N_RESULT_OK; + } /* Earlier versions are a basic copy */ if (conn->actual_protocol_version < S2N_TLS13) { @@ -101,25 +105,26 @@ static S2N_RESULT s2n_client_cert_chain_store(struct s2n_connection *conn, int s2n_client_cert_recv(struct s2n_connection *conn) { - uint32_t cert_message_start = conn->handshake.io.read_cursor; + /* s2n_client_cert_recv() may be re-entered due to handling an async callback. + * We operate on a copy of `handshake.io` to ensure the stuffer is initilized properly on the re-entry case. + */ + struct s2n_stuffer in = conn->handshake.io; if (conn->actual_protocol_version == S2N_TLS13) { uint8_t certificate_request_context_len = 0; - POSIX_GUARD(s2n_stuffer_read_uint8(&conn->handshake.io, &certificate_request_context_len)); + POSIX_GUARD(s2n_stuffer_read_uint8(&in, &certificate_request_context_len)); S2N_ERROR_IF(certificate_request_context_len != 0, S2N_ERR_BAD_MESSAGE); } - struct s2n_stuffer *in = &conn->handshake.io; - uint32_t cert_chain_size = 0; - POSIX_GUARD(s2n_stuffer_read_uint24(in, &cert_chain_size)); - POSIX_ENSURE(cert_chain_size <= s2n_stuffer_data_available(in), S2N_ERR_BAD_MESSAGE); + POSIX_GUARD(s2n_stuffer_read_uint24(&in, &cert_chain_size)); + POSIX_ENSURE(cert_chain_size <= s2n_stuffer_data_available(&in), S2N_ERR_BAD_MESSAGE); if (cert_chain_size == 0) { POSIX_GUARD(s2n_conn_set_handshake_no_client_cert(conn)); return S2N_SUCCESS; } - uint8_t *cert_chain_data = s2n_stuffer_raw_read(in, cert_chain_size); + uint8_t *cert_chain_data = s2n_stuffer_raw_read(&in, cert_chain_size); POSIX_ENSURE_REF(cert_chain_data); struct s2n_blob cert_chain = { 0 }; @@ -132,17 +137,8 @@ int s2n_client_cert_recv(struct s2n_connection *conn) /* Determine the Cert Type, Verify the Cert, and extract the Public Key */ s2n_pkey_type pkey_type = S2N_PKEY_TYPE_UNKNOWN; - s2n_result result = s2n_x509_validator_validate_cert_chain(&conn->x509_validator, conn, cert_chain_data, - cert_chain_size, &pkey_type, &public_key); - if (s2n_result_is_error(result) && s2n_errno == S2N_ERR_ASYNC_BLOCKED) { - /* s2n_x509_validator_validate_cert_chain() handles async callbacks, which may require re-entering the handshake; - * need to reset the stuffer cursor to where cert message starts in order to prepare for a re-entry, because - * s2n_handle_retry_state() will invoke s2n_client_cert_recv() directly without updating stuffer - */ - POSIX_GUARD(s2n_stuffer_reread(&conn->handshake.io)); - POSIX_GUARD(s2n_stuffer_skip_read(&conn->handshake.io, cert_message_start)); - } - POSIX_GUARD_RESULT(result); + POSIX_GUARD_RESULT(s2n_x509_validator_validate_cert_chain(&conn->x509_validator, conn, cert_chain_data, + cert_chain_size, &pkey_type, &public_key)); conn->handshake_params.client_cert_pkey_type = pkey_type; POSIX_GUARD_RESULT(s2n_pkey_setup_for_type(&public_key, pkey_type)); @@ -150,6 +146,8 @@ int s2n_client_cert_recv(struct s2n_connection *conn) POSIX_GUARD(s2n_pkey_check_key_exists(&public_key)); conn->handshake_params.client_public_key = public_key; + conn->handshake.io = in; + return S2N_SUCCESS; } diff --git a/tls/s2n_server_cert.c b/tls/s2n_server_cert.c index 24dc72ccfd4..11b0b6aff94 100644 --- a/tls/s2n_server_cert.c +++ b/tls/s2n_server_cert.c @@ -22,18 +22,21 @@ int s2n_server_cert_recv(struct s2n_connection *conn) { - uint32_t cert_message_start = conn->handshake.io.read_cursor; + /* s2n_client_cert_recv() may be re-entered due to handling an async callback. + * We operate on a copy of `handshake.io` to ensure the stuffer is initilized properly on the re-entry case. + */ + struct s2n_stuffer in = conn->handshake.io; if (conn->actual_protocol_version == S2N_TLS13) { uint8_t certificate_request_context_len = 0; - POSIX_GUARD(s2n_stuffer_read_uint8(&conn->handshake.io, &certificate_request_context_len)); + POSIX_GUARD(s2n_stuffer_read_uint8(&in, &certificate_request_context_len)); S2N_ERROR_IF(certificate_request_context_len != 0, S2N_ERR_BAD_MESSAGE); } uint32_t size_of_all_certificates = 0; - POSIX_GUARD(s2n_stuffer_read_uint24(&conn->handshake.io, &size_of_all_certificates)); + POSIX_GUARD(s2n_stuffer_read_uint24(&in, &size_of_all_certificates)); - S2N_ERROR_IF(size_of_all_certificates > s2n_stuffer_data_available(&conn->handshake.io) || size_of_all_certificates < 3, + S2N_ERROR_IF(size_of_all_certificates > s2n_stuffer_data_available(&in) || size_of_all_certificates < 3, S2N_ERR_BAD_MESSAGE); s2n_cert_public_key public_key; @@ -42,20 +45,11 @@ int s2n_server_cert_recv(struct s2n_connection *conn) s2n_pkey_type actual_cert_pkey_type; struct s2n_blob cert_chain = { 0 }; cert_chain.size = size_of_all_certificates; - cert_chain.data = s2n_stuffer_raw_read(&conn->handshake.io, size_of_all_certificates); + cert_chain.data = s2n_stuffer_raw_read(&in, size_of_all_certificates); POSIX_ENSURE_REF(cert_chain.data); - s2n_result result = s2n_x509_validator_validate_cert_chain(&conn->x509_validator, conn, cert_chain.data, - cert_chain.size, &actual_cert_pkey_type, &public_key); - if (s2n_result_is_error(result) && s2n_errno == S2N_ERR_ASYNC_BLOCKED) { - /* s2n_x509_validator_validate_cert_chain() handles async callbacks, which may require re-entering the handshake; - * need to reset the stuffer cursor to where cert message starts in order to prepare for a re-entry, because - * s2n_handle_retry_state() will invoke s2n_server_cert_recv() directly without updating stuffer - */ - POSIX_GUARD(s2n_stuffer_reread(&conn->handshake.io)); - POSIX_GUARD(s2n_stuffer_skip_read(&conn->handshake.io, cert_message_start)); - } - POSIX_GUARD_RESULT(result); + POSIX_GUARD_RESULT(s2n_x509_validator_validate_cert_chain(&conn->x509_validator, conn, cert_chain.data, + cert_chain.size, &actual_cert_pkey_type, &public_key)); POSIX_GUARD(s2n_is_cert_type_valid_for_auth(conn, actual_cert_pkey_type)); POSIX_GUARD_RESULT(s2n_pkey_setup_for_type(&public_key, actual_cert_pkey_type)); diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c index 061c2d664b9..80be3667b47 100644 --- a/tls/s2n_x509_validator.c +++ b/tls/s2n_x509_validator.c @@ -755,7 +755,7 @@ static S2N_RESULT s2n_x509_validator_parse_leaf_certificate_extensions(struct s2 return S2N_RESULT_OK; } -S2N_RESULT s2n_x509_validator_validate_cert_chain_pre(struct s2n_x509_validator *validator, struct s2n_connection *conn, +S2N_RESULT s2n_x509_validator_validate_cert_chain_impl(struct s2n_x509_validator *validator, struct s2n_connection *conn, uint8_t *cert_chain_in, uint32_t cert_chain_len) { RESULT_ENSURE_REF(conn); @@ -802,7 +802,7 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val if (validator->cert_validation_cb_invoked) { RESULT_GUARD(s2n_x509_validator_handle_cert_validation_callback_result(validator)); } else { - RESULT_GUARD(s2n_x509_validator_validate_cert_chain_pre(validator, conn, cert_chain_in, cert_chain_len)); + RESULT_GUARD(s2n_x509_validator_validate_cert_chain_impl(validator, conn, cert_chain_in, cert_chain_len)); if (conn->config->cert_validation_cb) { RESULT_ENSURE(conn->config->cert_validation_cb(conn, &(validator->cert_validation_info), conn->config->cert_validation_ctx) >= S2N_SUCCESS, From a5f6c0db56ab0f2cd898fec353b1a713ecc9fb95 Mon Sep 17 00:00:00 2001 From: Carol Yeh Date: Thu, 13 Feb 2025 23:30:50 +0000 Subject: [PATCH 06/11] bug fix --- tls/s2n_client_cert.c | 1 + tls/s2n_server_cert.c | 3 +++ 2 files changed, 4 insertions(+) diff --git a/tls/s2n_client_cert.c b/tls/s2n_client_cert.c index 4f08a00d5d7..5112dce9bc9 100644 --- a/tls/s2n_client_cert.c +++ b/tls/s2n_client_cert.c @@ -146,6 +146,7 @@ int s2n_client_cert_recv(struct s2n_connection *conn) POSIX_GUARD(s2n_pkey_check_key_exists(&public_key)); conn->handshake_params.client_public_key = public_key; + /* copy the stuffer back to handshake.io */ conn->handshake.io = in; return S2N_SUCCESS; diff --git a/tls/s2n_server_cert.c b/tls/s2n_server_cert.c index 11b0b6aff94..92cbcb4d646 100644 --- a/tls/s2n_server_cert.c +++ b/tls/s2n_server_cert.c @@ -55,6 +55,9 @@ int s2n_server_cert_recv(struct s2n_connection *conn) POSIX_GUARD_RESULT(s2n_pkey_setup_for_type(&public_key, actual_cert_pkey_type)); conn->handshake_params.server_public_key = public_key; + /* copy the stuffer back to handshake.io */ + conn->handshake.io = in; + return 0; } From f3043de75f7145204dc3d9f7819dfcf15553fe34 Mon Sep 17 00:00:00 2001 From: Carol Yeh Date: Mon, 17 Feb 2025 19:36:38 +0000 Subject: [PATCH 07/11] address PR comments --- api/unstable/crl.h | 16 +- .../unit/s2n_cert_validation_callback_test.c | 249 +++++++++--------- tls/s2n_client_cert.c | 4 +- tls/s2n_server_cert.c | 4 +- tls/s2n_x509_validator.c | 26 +- tls/s2n_x509_validator.h | 2 - 6 files changed, 148 insertions(+), 153 deletions(-) diff --git a/api/unstable/crl.h b/api/unstable/crl.h index 44ab4dc7836..927793a54d9 100644 --- a/api/unstable/crl.h +++ b/api/unstable/crl.h @@ -187,17 +187,17 @@ struct s2n_cert_validation_info; * * When using the validation callback in a synchronous mode, `s2n_cert_validation_accept()` MUST be called to allow * `s2n_negotiate()` to continue the handshake. If the validation is unsuccessful, `s2n_cert_validation_reject()` - * MUST be called, which will cause `s2n_negotiate()` to error. If neither `s2n_cert_validation_accept()` or - * `s2n_cert_validation_reject()` are called, the validation can be processed asynchronously. + * MUST be called, which will cause `s2n_negotiate()` to error. * - * To enable the async behavior, `S2N_SUCCESS` must be returned from the validation callback. Then the handshake will - * be paused and `s2n_negotiate()` will throw a `S2N_ERR_T_BLOCKED` error. The application should check for this error and - * check `s2n_blocked_status` is set to `S2N_BLOCKED_ON_APPLICATION_INPUT`. + * To use the validation callback asynchronously, return `S2N_SUCCESS` without calling `s2n_cert_validation_accept()` + * or `s2n_cert_validation_reject()`. This will pause the handshake, and `s2n_negotiate()` will throw a `S2N_ERR_T_BLOCKED` + * error and `s2n_blocked_status` will be set to `S2N_BLOCKED_ON_APPLICATION_INPUT`. Applications should call + * `s2n_cert_validation_accept()` or `s2n_cert_validation_reject()` to unpause the handshake before retrying `s2n_negotiate()`. * * The `info` parameter is passed to the callback in order to call APIs specific to the cert validation callback, like - * `s2n_cert_validation_accept()` and `s2n_cert_validation_reject()`. The `info` argument shares the same - * lifetime as `s2n_connection`. In order to unpause the handshake, the application should update the `info` parameter - * by calling `s2n_cert_validation_accept()` or `s2n_cert_validation_reject()` before retrying `s2n_negotiate()`. + * `s2n_cert_validation_accept()` and `s2n_cert_validation_reject()`. The `info` argument shares the same lifetime as + * `s2n_connection`. On the async case, the application should update the `info` struct by calling `s2n_cert_validation_accept()` + * or `s2n_cert_validation_reject()` outside of the callback. * * After calling `s2n_cert_validation_reject()`, `s2n_negotiate()` will fail with a protocol error indicating that * the cert has been rejected from the callback. If more information regarding an application's custom validation diff --git a/tests/unit/s2n_cert_validation_callback_test.c b/tests/unit/s2n_cert_validation_callback_test.c index 3f6cb8dff52..376b056b1a9 100644 --- a/tests/unit/s2n_cert_validation_callback_test.c +++ b/tests/unit/s2n_cert_validation_callback_test.c @@ -31,6 +31,7 @@ static int s2n_test_cert_validation_callback(struct s2n_connection *conn, struct struct s2n_cert_validation_data *data = (struct s2n_cert_validation_data *) ctx; data->invoked_count += 1; + /* Pass the `s2n_cert_validation_info` struct to application-defined `ctx` */ data->info = info; int ret = S2N_FAILURE; @@ -437,143 +438,137 @@ int main(int argc, char *argv[]) EXPECT_EQUAL(data.invoked_count, 1); } - /* clang-format off */ - struct { - const struct s2n_cert_validation_data data; - const char *version; - } async_test_cases[] = { - /* For async cases, accept or reject API will be called outside of the validation callback. - * Iterate over both TLS 1.3 and 1.2 policies to ensure the stuffer reset logic works in all cases. - */ - { - .data = { .call_accept_or_reject = false, .accept = true, .return_success = true }, - .version = "20240501" - }, - { - .data = { .call_accept_or_reject = false, .accept = false, .return_success = true }, - .version = "20240501" - }, - { - .data = { .call_accept_or_reject = false, .accept = true, .return_success = true }, - .version = "20170210" - }, - { - .data = { .call_accept_or_reject = false, .accept = false, .return_success = true }, - .version = "20170210" - }, + /* For async cases, accept or reject API will be called outside of the validation callback. + * Iterate over both TLS 1.3 and 1.2 policies to ensure the stuffer reset logic works in all cases. + */ + struct s2n_cert_validation_data async_test_cases[] = { + { .call_accept_or_reject = false, .accept = true, .return_success = true }, + { .call_accept_or_reject = false, .accept = false, .return_success = true }, }; - /* clang-format on */ + const char *versions[] = { "20240501", "20170210" }; /* Async callback is invoked on the client after receiving the server's certificate */ for (int i = 0; i < s2n_array_len(async_test_cases); i++) { - DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free); - EXPECT_NOT_NULL(config); - EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, chain_and_key)); - EXPECT_SUCCESS(s2n_config_set_verification_ca_location(config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); - EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, async_test_cases[i].version)); - - struct s2n_cert_validation_data data = async_test_cases[i].data; - EXPECT_SUCCESS(s2n_config_set_cert_validation_cb(config, s2n_test_cert_validation_callback_self_talk, &data)); - - DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free); - EXPECT_NOT_NULL(server_conn); - EXPECT_SUCCESS(s2n_connection_set_config(server_conn, config)); - - DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free); - EXPECT_NOT_NULL(client_conn); - EXPECT_SUCCESS(s2n_connection_set_config(client_conn, config)); - EXPECT_SUCCESS(s2n_connection_set_blinding(client_conn, S2N_SELF_SERVICE_BLINDING)); - EXPECT_SUCCESS(s2n_set_server_name(client_conn, "localhost")); - - DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close); - EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair)); - EXPECT_SUCCESS(s2n_connection_set_io_pair(client_conn, &io_pair)); - EXPECT_SUCCESS(s2n_connection_set_io_pair(server_conn, &io_pair)); - - for (int j = 0; j < 3; j++) { - EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), - S2N_ERR_ASYNC_BLOCKED); - } - - /* Ensure that the server's certificate chain can be retrieved after `S2N_ERR_ASYNC_BLOCKED` */ - DEFER_CLEANUP(struct s2n_cert_chain_and_key *peer_cert_chain = s2n_cert_chain_and_key_new(), - s2n_cert_chain_and_key_ptr_free); - EXPECT_NOT_NULL(peer_cert_chain); - EXPECT_SUCCESS(s2n_connection_get_peer_cert_chain(client_conn, peer_cert_chain)); - - struct s2n_cert_validation_info *info = data.info; - EXPECT_NOT_NULL(info); - - if (test_cases[i].data.accept) { - EXPECT_SUCCESS(s2n_cert_validation_accept(info)); - EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server_conn, client_conn)); - } else { - EXPECT_SUCCESS(s2n_cert_validation_reject(info)); - EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), - S2N_ERR_CERT_REJECTED); + for (int j = 0; j < s2n_array_len(versions); j++) { + DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free); + EXPECT_NOT_NULL(config); + EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, chain_and_key)); + EXPECT_SUCCESS(s2n_config_set_verification_ca_location(config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); + EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, versions[j])); + + struct s2n_cert_validation_data data = async_test_cases[i]; + EXPECT_SUCCESS(s2n_config_set_cert_validation_cb(config, s2n_test_cert_validation_callback_self_talk, &data)); + + DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free); + EXPECT_NOT_NULL(server_conn); + EXPECT_SUCCESS(s2n_connection_set_config(server_conn, config)); + + DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free); + EXPECT_NOT_NULL(client_conn); + EXPECT_SUCCESS(s2n_connection_set_config(client_conn, config)); + EXPECT_SUCCESS(s2n_connection_set_blinding(client_conn, S2N_SELF_SERVICE_BLINDING)); + EXPECT_SUCCESS(s2n_set_server_name(client_conn, "localhost")); + + DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close); + EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair)); + EXPECT_SUCCESS(s2n_connection_set_io_pair(client_conn, &io_pair)); + EXPECT_SUCCESS(s2n_connection_set_io_pair(server_conn, &io_pair)); + + for (int k = 0; k < 3; k++) { + EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), + S2N_ERR_ASYNC_BLOCKED); + EXPECT_EQUAL(data.invoked_count, 1); + } + + /* Ensure that the server's certificate chain can be retrieved after `S2N_ERR_ASYNC_BLOCKED` */ + DEFER_CLEANUP(struct s2n_cert_chain_and_key *peer_cert_chain = s2n_cert_chain_and_key_new(), + s2n_cert_chain_and_key_ptr_free); + EXPECT_NOT_NULL(peer_cert_chain); + EXPECT_SUCCESS(s2n_connection_get_peer_cert_chain(client_conn, peer_cert_chain)); + /* Ensure the certificate chain is non-empty */ + uint32_t peer_cert_chain_len = 0; + EXPECT_SUCCESS(s2n_cert_chain_get_length(peer_cert_chain, &peer_cert_chain_len)); + EXPECT_TRUE(peer_cert_chain_len > 0); + + struct s2n_cert_validation_info *info = data.info; + EXPECT_NOT_NULL(info); + + if (test_cases[i].data.accept) { + EXPECT_SUCCESS(s2n_cert_validation_accept(info)); + EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server_conn, client_conn)); + } else { + EXPECT_SUCCESS(s2n_cert_validation_reject(info)); + EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), + S2N_ERR_CERT_REJECTED); + } + + EXPECT_EQUAL(data.invoked_count, 1); } - - EXPECT_EQUAL(data.invoked_count, 1); } /* Async callback is invoked on the server after receiving the client's certificate */ for (int i = 0; i < s2n_array_len(async_test_cases); i++) { - DEFER_CLEANUP(struct s2n_config *server_config = s2n_config_new(), s2n_config_ptr_free); - EXPECT_NOT_NULL(server_config); - EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(server_config, chain_and_key)); - EXPECT_SUCCESS(s2n_config_set_verification_ca_location(server_config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); - EXPECT_SUCCESS(s2n_config_set_cipher_preferences(server_config, async_test_cases[i].version)); - EXPECT_SUCCESS(s2n_config_set_client_auth_type(server_config, S2N_CERT_AUTH_REQUIRED)); - - struct s2n_cert_validation_data data = async_test_cases[i].data; - EXPECT_SUCCESS(s2n_config_set_cert_validation_cb(server_config, - s2n_test_cert_validation_callback_self_talk_server, &data)); - - DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free); - EXPECT_NOT_NULL(server_conn); - EXPECT_SUCCESS(s2n_connection_set_config(server_conn, server_config)); - EXPECT_SUCCESS(s2n_connection_set_blinding(server_conn, S2N_SELF_SERVICE_BLINDING)); - - DEFER_CLEANUP(struct s2n_config *client_config = s2n_config_new(), s2n_config_ptr_free); - EXPECT_NOT_NULL(client_config); - EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(client_config, chain_and_key)); - EXPECT_SUCCESS(s2n_config_set_verification_ca_location(client_config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); - EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_config, async_test_cases[i].version)); - EXPECT_SUCCESS(s2n_config_set_client_auth_type(client_config, S2N_CERT_AUTH_OPTIONAL)); - - DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free); - EXPECT_NOT_NULL(client_conn); - EXPECT_SUCCESS(s2n_connection_set_config(client_conn, client_config)); - EXPECT_SUCCESS(s2n_set_server_name(client_conn, "localhost")); - - DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close); - EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair)); - EXPECT_SUCCESS(s2n_connection_set_io_pair(client_conn, &io_pair)); - EXPECT_SUCCESS(s2n_connection_set_io_pair(server_conn, &io_pair)); - - for (int j = 0; j < 3; j++) { - EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), - S2N_ERR_ASYNC_BLOCKED); - } - - /* Ensure that the client's certificate chain can be retrieved after `S2N_ERR_ASYNC_BLOCKED` */ - uint8_t *der_cert_chain = 0; - uint32_t cert_chain_len = 0; - EXPECT_SUCCESS(s2n_connection_get_client_cert_chain(server_conn, &der_cert_chain, &cert_chain_len)); - - struct s2n_cert_validation_info *info = data.info; - EXPECT_NOT_NULL(info); - - if (test_cases[i].data.accept) { - EXPECT_SUCCESS(s2n_cert_validation_accept(info)); - EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server_conn, client_conn)); - } else { - EXPECT_SUCCESS(s2n_cert_validation_reject(info)); - EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), - S2N_ERR_CERT_REJECTED); + for (int j = 0; j < s2n_array_len(versions); j++) { + DEFER_CLEANUP(struct s2n_config *server_config = s2n_config_new(), s2n_config_ptr_free); + EXPECT_NOT_NULL(server_config); + EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(server_config, chain_and_key)); + EXPECT_SUCCESS(s2n_config_set_verification_ca_location(server_config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); + EXPECT_SUCCESS(s2n_config_set_cipher_preferences(server_config, versions[j])); + EXPECT_SUCCESS(s2n_config_set_client_auth_type(server_config, S2N_CERT_AUTH_REQUIRED)); + + struct s2n_cert_validation_data data = async_test_cases[i]; + EXPECT_SUCCESS(s2n_config_set_cert_validation_cb(server_config, + s2n_test_cert_validation_callback_self_talk_server, &data)); + + DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free); + EXPECT_NOT_NULL(server_conn); + EXPECT_SUCCESS(s2n_connection_set_config(server_conn, server_config)); + EXPECT_SUCCESS(s2n_connection_set_blinding(server_conn, S2N_SELF_SERVICE_BLINDING)); + + DEFER_CLEANUP(struct s2n_config *client_config = s2n_config_new(), s2n_config_ptr_free); + EXPECT_NOT_NULL(client_config); + EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(client_config, chain_and_key)); + EXPECT_SUCCESS(s2n_config_set_verification_ca_location(client_config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); + EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_config, versions[j])); + EXPECT_SUCCESS(s2n_config_set_client_auth_type(client_config, S2N_CERT_AUTH_OPTIONAL)); + + DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free); + EXPECT_NOT_NULL(client_conn); + EXPECT_SUCCESS(s2n_connection_set_config(client_conn, client_config)); + EXPECT_SUCCESS(s2n_set_server_name(client_conn, "localhost")); + + DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close); + EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair)); + EXPECT_SUCCESS(s2n_connection_set_io_pair(client_conn, &io_pair)); + EXPECT_SUCCESS(s2n_connection_set_io_pair(server_conn, &io_pair)); + + for (int k = 0; k < 3; k++) { + EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), + S2N_ERR_ASYNC_BLOCKED); + EXPECT_EQUAL(data.invoked_count, 1); + } + + /* Ensure that the client's certificate chain can be retrieved after `S2N_ERR_ASYNC_BLOCKED` */ + uint8_t *der_cert_chain = 0; + uint32_t cert_chain_len = 0; + EXPECT_SUCCESS(s2n_connection_get_client_cert_chain(server_conn, &der_cert_chain, &cert_chain_len)); + /* Ensure the certificate chain is non-empty */ + EXPECT_TRUE(cert_chain_len > 0); + + struct s2n_cert_validation_info *info = data.info; + EXPECT_NOT_NULL(info); + + if (test_cases[i].data.accept) { + EXPECT_SUCCESS(s2n_cert_validation_accept(info)); + EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server_conn, client_conn)); + } else { + EXPECT_SUCCESS(s2n_cert_validation_reject(info)); + EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), + S2N_ERR_CERT_REJECTED); + } + + EXPECT_EQUAL(data.invoked_count, 1); } - - EXPECT_EQUAL(data.invoked_count, 1); } } diff --git a/tls/s2n_client_cert.c b/tls/s2n_client_cert.c index 5112dce9bc9..2dc540670bb 100644 --- a/tls/s2n_client_cert.c +++ b/tls/s2n_client_cert.c @@ -58,7 +58,7 @@ static S2N_RESULT s2n_client_cert_chain_store(struct s2n_connection *conn, RESULT_ENSURE_REF(raw_cert_chain); /* If a client cert chain has already been stored (e.g. on the re-entry case - * of an async callback), no need to read the cert chain again. + * of an async callback), no need to store it again. */ if (conn->handshake_params.client_cert_chain.size > 0) { return S2N_RESULT_OK; @@ -146,7 +146,7 @@ int s2n_client_cert_recv(struct s2n_connection *conn) POSIX_GUARD(s2n_pkey_check_key_exists(&public_key)); conn->handshake_params.client_public_key = public_key; - /* copy the stuffer back to handshake.io */ + /* Update handshake.io to reflect the true stuffer state after all async callbacks are handled. */ conn->handshake.io = in; return S2N_SUCCESS; diff --git a/tls/s2n_server_cert.c b/tls/s2n_server_cert.c index 92cbcb4d646..7ba1564eb03 100644 --- a/tls/s2n_server_cert.c +++ b/tls/s2n_server_cert.c @@ -22,7 +22,7 @@ int s2n_server_cert_recv(struct s2n_connection *conn) { - /* s2n_client_cert_recv() may be re-entered due to handling an async callback. + /* s2n_server_cert_recv() may be re-entered due to handling an async callback. * We operate on a copy of `handshake.io` to ensure the stuffer is initilized properly on the re-entry case. */ struct s2n_stuffer in = conn->handshake.io; @@ -55,7 +55,7 @@ int s2n_server_cert_recv(struct s2n_connection *conn) POSIX_GUARD_RESULT(s2n_pkey_setup_for_type(&public_key, actual_cert_pkey_type)); conn->handshake_params.server_public_key = public_key; - /* copy the stuffer back to handshake.io */ + /* Update handshake.io to reflect the true stuffer state after all async callbacks are handled. */ conn->handshake.io = in; return 0; diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c index 80be3667b47..a445d2bcd12 100644 --- a/tls/s2n_x509_validator.c +++ b/tls/s2n_x509_validator.c @@ -796,9 +796,23 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain_impl(struct s2n_x509_validator return S2N_RESULT_OK; } +static S2N_RESULT s2n_x509_validator_handle_cert_validation_callback_result(struct s2n_x509_validator *validator) +{ + RESULT_ENSURE_REF(validator); + + if (!validator->cert_validation_info.finished) { + RESULT_BAIL(S2N_ERR_ASYNC_BLOCKED); + } + + RESULT_ENSURE(validator->cert_validation_info.accepted, S2N_ERR_CERT_REJECTED); + return S2N_RESULT_OK; +} + S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *validator, struct s2n_connection *conn, uint8_t *cert_chain_in, uint32_t cert_chain_len, s2n_pkey_type *pkey_type, struct s2n_pkey *public_key_out) { + RESULT_ENSURE_REF(validator); + if (validator->cert_validation_cb_invoked) { RESULT_GUARD(s2n_x509_validator_handle_cert_validation_callback_result(validator)); } else { @@ -975,18 +989,6 @@ bool s2n_x509_validator_is_cert_chain_validated(const struct s2n_x509_validator return validator && (validator->state == VALIDATED || validator->state == OCSP_VALIDATED); } -S2N_RESULT s2n_x509_validator_handle_cert_validation_callback_result(struct s2n_x509_validator *validator) -{ - RESULT_ENSURE_REF(validator); - - if (!validator->cert_validation_info.finished) { - RESULT_BAIL(S2N_ERR_ASYNC_BLOCKED); - } - - RESULT_ENSURE(validator->cert_validation_info.accepted, S2N_ERR_CERT_REJECTED); - return S2N_RESULT_OK; -} - int s2n_cert_validation_accept(struct s2n_cert_validation_info *info) { POSIX_ENSURE_REF(info); diff --git a/tls/s2n_x509_validator.h b/tls/s2n_x509_validator.h index 5237f3e68f3..3bdfaf9a959 100644 --- a/tls/s2n_x509_validator.h +++ b/tls/s2n_x509_validator.h @@ -135,5 +135,3 @@ S2N_RESULT s2n_x509_validator_validate_cert_stapled_ocsp_response(struct s2n_x50 * Should be verified before any use of the peer's certificate data. */ bool s2n_x509_validator_is_cert_chain_validated(const struct s2n_x509_validator *validator); - -S2N_RESULT s2n_x509_validator_handle_cert_validation_callback_result(struct s2n_x509_validator *validator); From 64cb98618994f6d7de3dd5e0744f57a08356521e Mon Sep 17 00:00:00 2001 From: Carol Yeh Date: Mon, 17 Feb 2025 21:30:54 +0000 Subject: [PATCH 08/11] code cleanup --- api/unstable/crl.h | 3 +-- .../unit/s2n_cert_validation_callback_test.c | 26 +++++++++---------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/api/unstable/crl.h b/api/unstable/crl.h index 927793a54d9..c4f6153fdc7 100644 --- a/api/unstable/crl.h +++ b/api/unstable/crl.h @@ -196,8 +196,7 @@ struct s2n_cert_validation_info; * * The `info` parameter is passed to the callback in order to call APIs specific to the cert validation callback, like * `s2n_cert_validation_accept()` and `s2n_cert_validation_reject()`. The `info` argument shares the same lifetime as - * `s2n_connection`. On the async case, the application should update the `info` struct by calling `s2n_cert_validation_accept()` - * or `s2n_cert_validation_reject()` outside of the callback. + * `s2n_connection`. * * After calling `s2n_cert_validation_reject()`, `s2n_negotiate()` will fail with a protocol error indicating that * the cert has been rejected from the callback. If more information regarding an application's custom validation diff --git a/tests/unit/s2n_cert_validation_callback_test.c b/tests/unit/s2n_cert_validation_callback_test.c index 376b056b1a9..5dae9b72ef6 100644 --- a/tests/unit/s2n_cert_validation_callback_test.c +++ b/tests/unit/s2n_cert_validation_callback_test.c @@ -448,15 +448,15 @@ int main(int argc, char *argv[]) const char *versions[] = { "20240501", "20170210" }; /* Async callback is invoked on the client after receiving the server's certificate */ - for (int i = 0; i < s2n_array_len(async_test_cases); i++) { - for (int j = 0; j < s2n_array_len(versions); j++) { + for (int test_case_idx = 0; test_case_idx < s2n_array_len(async_test_cases); test_case_idx++) { + for (int version_idx = 0; version_idx < s2n_array_len(versions); version_idx++) { DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free); EXPECT_NOT_NULL(config); EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, chain_and_key)); EXPECT_SUCCESS(s2n_config_set_verification_ca_location(config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); - EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, versions[j])); + EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, versions[version_idx])); - struct s2n_cert_validation_data data = async_test_cases[i]; + struct s2n_cert_validation_data data = async_test_cases[test_case_idx]; EXPECT_SUCCESS(s2n_config_set_cert_validation_cb(config, s2n_test_cert_validation_callback_self_talk, &data)); DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free); @@ -474,7 +474,7 @@ int main(int argc, char *argv[]) EXPECT_SUCCESS(s2n_connection_set_io_pair(client_conn, &io_pair)); EXPECT_SUCCESS(s2n_connection_set_io_pair(server_conn, &io_pair)); - for (int k = 0; k < 3; k++) { + for (int i = 0; i < 3; i++) { EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), S2N_ERR_ASYNC_BLOCKED); EXPECT_EQUAL(data.invoked_count, 1); @@ -493,7 +493,7 @@ int main(int argc, char *argv[]) struct s2n_cert_validation_info *info = data.info; EXPECT_NOT_NULL(info); - if (test_cases[i].data.accept) { + if (test_cases[test_case_idx].data.accept) { EXPECT_SUCCESS(s2n_cert_validation_accept(info)); EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server_conn, client_conn)); } else { @@ -507,16 +507,16 @@ int main(int argc, char *argv[]) } /* Async callback is invoked on the server after receiving the client's certificate */ - for (int i = 0; i < s2n_array_len(async_test_cases); i++) { - for (int j = 0; j < s2n_array_len(versions); j++) { + for (int test_case_idx = 0; test_case_idx < s2n_array_len(async_test_cases); test_case_idx++) { + for (int version_idx = 0; version_idx < s2n_array_len(versions); version_idx++) { DEFER_CLEANUP(struct s2n_config *server_config = s2n_config_new(), s2n_config_ptr_free); EXPECT_NOT_NULL(server_config); EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(server_config, chain_and_key)); EXPECT_SUCCESS(s2n_config_set_verification_ca_location(server_config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); - EXPECT_SUCCESS(s2n_config_set_cipher_preferences(server_config, versions[j])); + EXPECT_SUCCESS(s2n_config_set_cipher_preferences(server_config, versions[version_idx])); EXPECT_SUCCESS(s2n_config_set_client_auth_type(server_config, S2N_CERT_AUTH_REQUIRED)); - struct s2n_cert_validation_data data = async_test_cases[i]; + struct s2n_cert_validation_data data = async_test_cases[test_case_idx]; EXPECT_SUCCESS(s2n_config_set_cert_validation_cb(server_config, s2n_test_cert_validation_callback_self_talk_server, &data)); @@ -529,7 +529,7 @@ int main(int argc, char *argv[]) EXPECT_NOT_NULL(client_config); EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(client_config, chain_and_key)); EXPECT_SUCCESS(s2n_config_set_verification_ca_location(client_config, S2N_DEFAULT_TEST_CERT_CHAIN, NULL)); - EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_config, versions[j])); + EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_config, versions[version_idx])); EXPECT_SUCCESS(s2n_config_set_client_auth_type(client_config, S2N_CERT_AUTH_OPTIONAL)); DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free); @@ -542,7 +542,7 @@ int main(int argc, char *argv[]) EXPECT_SUCCESS(s2n_connection_set_io_pair(client_conn, &io_pair)); EXPECT_SUCCESS(s2n_connection_set_io_pair(server_conn, &io_pair)); - for (int k = 0; k < 3; k++) { + for (int i = 0; i < 3; i++) { EXPECT_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server_conn, client_conn), S2N_ERR_ASYNC_BLOCKED); EXPECT_EQUAL(data.invoked_count, 1); @@ -558,7 +558,7 @@ int main(int argc, char *argv[]) struct s2n_cert_validation_info *info = data.info; EXPECT_NOT_NULL(info); - if (test_cases[i].data.accept) { + if (test_cases[test_case_idx].data.accept) { EXPECT_SUCCESS(s2n_cert_validation_accept(info)); EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server_conn, client_conn)); } else { From 86d905e8766ac7c61818cdb9f4a3aa360551cec5 Mon Sep 17 00:00:00 2001 From: Carol Yeh Date: Tue, 18 Feb 2025 03:03:26 +0000 Subject: [PATCH 09/11] code cleanup --- tests/unit/s2n_cert_validation_callback_test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/s2n_cert_validation_callback_test.c b/tests/unit/s2n_cert_validation_callback_test.c index 5dae9b72ef6..be4392098f0 100644 --- a/tests/unit/s2n_cert_validation_callback_test.c +++ b/tests/unit/s2n_cert_validation_callback_test.c @@ -493,7 +493,7 @@ int main(int argc, char *argv[]) struct s2n_cert_validation_info *info = data.info; EXPECT_NOT_NULL(info); - if (test_cases[test_case_idx].data.accept) { + if (async_test_cases[test_case_idx].accept) { EXPECT_SUCCESS(s2n_cert_validation_accept(info)); EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server_conn, client_conn)); } else { @@ -558,7 +558,7 @@ int main(int argc, char *argv[]) struct s2n_cert_validation_info *info = data.info; EXPECT_NOT_NULL(info); - if (test_cases[test_case_idx].data.accept) { + if (async_test_cases[test_case_idx].accept) { EXPECT_SUCCESS(s2n_cert_validation_accept(info)); EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server_conn, client_conn)); } else { From 412b0c8be69a2d67c4c7d2aa430f971f5179ce19 Mon Sep 17 00:00:00 2001 From: Carol Yeh Date: Wed, 19 Feb 2025 22:26:16 +0000 Subject: [PATCH 10/11] address PR comments --- api/unstable/crl.h | 4 ++-- tls/s2n_x509_validator.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/api/unstable/crl.h b/api/unstable/crl.h index c4f6153fdc7..22856b8af9c 100644 --- a/api/unstable/crl.h +++ b/api/unstable/crl.h @@ -185,12 +185,12 @@ struct s2n_cert_validation_info; * - `s2n_connection_get_peer_cert_chain()` * - `s2n_connection_get_client_cert_chain()` * - * When using the validation callback in a synchronous mode, `s2n_cert_validation_accept()` MUST be called to allow + * If the validation performed in the callback is successful, `s2n_cert_validation_accept()` MUST be called to allow * `s2n_negotiate()` to continue the handshake. If the validation is unsuccessful, `s2n_cert_validation_reject()` * MUST be called, which will cause `s2n_negotiate()` to error. * * To use the validation callback asynchronously, return `S2N_SUCCESS` without calling `s2n_cert_validation_accept()` - * or `s2n_cert_validation_reject()`. This will pause the handshake, and `s2n_negotiate()` will throw a `S2N_ERR_T_BLOCKED` + * or `s2n_cert_validation_reject()`. This will pause the handshake, and `s2n_negotiate()` will throw an `S2N_ERR_T_BLOCKED` * error and `s2n_blocked_status` will be set to `S2N_BLOCKED_ON_APPLICATION_INPUT`. Applications should call * `s2n_cert_validation_accept()` or `s2n_cert_validation_reject()` to unpause the handshake before retrying `s2n_negotiate()`. * diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c index a445d2bcd12..4839433e1d0 100644 --- a/tls/s2n_x509_validator.c +++ b/tls/s2n_x509_validator.c @@ -755,7 +755,7 @@ static S2N_RESULT s2n_x509_validator_parse_leaf_certificate_extensions(struct s2 return S2N_RESULT_OK; } -S2N_RESULT s2n_x509_validator_validate_cert_chain_impl(struct s2n_x509_validator *validator, struct s2n_connection *conn, +S2N_RESULT s2n_x509_validator_validate_cert_chain_pre_cb(struct s2n_x509_validator *validator, struct s2n_connection *conn, uint8_t *cert_chain_in, uint32_t cert_chain_len) { RESULT_ENSURE_REF(conn); @@ -816,10 +816,10 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val if (validator->cert_validation_cb_invoked) { RESULT_GUARD(s2n_x509_validator_handle_cert_validation_callback_result(validator)); } else { - RESULT_GUARD(s2n_x509_validator_validate_cert_chain_impl(validator, conn, cert_chain_in, cert_chain_len)); + RESULT_GUARD(s2n_x509_validator_validate_cert_chain_pre_cb(validator, conn, cert_chain_in, cert_chain_len)); if (conn->config->cert_validation_cb) { - RESULT_ENSURE(conn->config->cert_validation_cb(conn, &(validator->cert_validation_info), conn->config->cert_validation_ctx) >= S2N_SUCCESS, + RESULT_ENSURE(conn->config->cert_validation_cb(conn, &(validator->cert_validation_info), conn->config->cert_validation_ctx) == S2N_SUCCESS, S2N_ERR_CANCELLED); validator->cert_validation_cb_invoked = true; RESULT_GUARD(s2n_x509_validator_handle_cert_validation_callback_result(validator)); From 8ce90507f77c35cf1f6ea93259c84c2c7e2d2c28 Mon Sep 17 00:00:00 2001 From: Carol Yeh Date: Thu, 20 Feb 2025 00:33:04 +0000 Subject: [PATCH 11/11] revert return value change --- tls/s2n_x509_validator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c index efddae222c5..3c27e810055 100644 --- a/tls/s2n_x509_validator.c +++ b/tls/s2n_x509_validator.c @@ -818,7 +818,7 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val RESULT_GUARD(s2n_x509_validator_validate_cert_chain_pre_cb(validator, conn, cert_chain_in, cert_chain_len)); if (conn->config->cert_validation_cb) { - RESULT_ENSURE(conn->config->cert_validation_cb(conn, &(validator->cert_validation_info), conn->config->cert_validation_ctx) == S2N_SUCCESS, + RESULT_ENSURE(conn->config->cert_validation_cb(conn, &(validator->cert_validation_info), conn->config->cert_validation_ctx) >= S2N_SUCCESS, S2N_ERR_CANCELLED); validator->cert_validation_cb_invoked = true; RESULT_GUARD(s2n_x509_validator_handle_cert_validation_callback_result(validator));