From edb046c7a25d7d727d666128ba6796ca18348478 Mon Sep 17 00:00:00 2001 From: Ted Poole Date: Wed, 10 Jul 2024 16:37:47 +0100 Subject: [PATCH] Fixed TLS alert code mapping in SSL_CTX_set_custom_verify() Also added a test Signed-off-by: Ted Poole --- .../source/SSL_CTX_set_custom_verify.cc | 65 +++++++++---- bssl-compat/source/test/test_ssl.cc | 95 +++++++++++++++++-- 2 files changed, 138 insertions(+), 22 deletions(-) diff --git a/bssl-compat/source/SSL_CTX_set_custom_verify.cc b/bssl-compat/source/SSL_CTX_set_custom_verify.cc index dfc96f25ab..3e738aac46 100644 --- a/bssl-compat/source/SSL_CTX_set_custom_verify.cc +++ b/bssl-compat/source/SSL_CTX_set_custom_verify.cc @@ -4,6 +4,53 @@ #include "override.h" +/* + * This function maps from TLS alert codes (SSL_AD_* constants) to X509 error + * codes (X509_V_ERR_* constants). + * + * Because the set of TLS alerts and X509 errors are not equal, this is not a + * lossless mapping. However, to achieve the best mapping, this function is + * written using some knowledge of OpenSSL's internals. Specifically, the + * "x509table" array (in ssl/statem/statem_lic.c) was used to derive the switch + * statement. + * + * Each TLS alert case returns a corresponding X509 error that OpenSSL will map + * back to the same TLS alert. If a TLS alert is not explicitly handled in the + * switch, it is because OpenSSL doesn't have a mapping from any X509 value to + * that TLS alert. In these cases, the default case will return + * X509_V_ERR_APPLICATION_VERIFICATION which OpenSSL will map to + * SSL_AD_HANDSHAKE_FAILURE + * + * https://github.com/openssl/openssl/blob/9cff14fd97814baf8a9a07d8447960a64d616ada/ssl/statem/statem_lib.c#L1351-L1395 + */ +static int tls_alert_to_x590_err(int alert) +{ + switch(alert) { + case SSL_AD_BAD_CERTIFICATE: + return X509_V_ERR_CERT_REJECTED; + case SSL_AD_CERTIFICATE_EXPIRED: + return X509_V_ERR_CERT_HAS_EXPIRED; + case SSL_AD_CERTIFICATE_REVOKED: + return X509_V_ERR_CERT_REVOKED; + case SSL_AD_CERTIFICATE_UNKNOWN: + return X509_V_ERR_INVALID_NON_CA; + case SSL_AD_DECRYPT_ERROR: + return X509_V_ERR_CERT_SIGNATURE_FAILURE; + case SSL_AD_HANDSHAKE_FAILURE: + return X509_V_ERR_APPLICATION_VERIFICATION; + case SSL_AD_INTERNAL_ERROR: + return X509_V_ERR_UNSPECIFIED; + case SSL_AD_UNKNOWN_CA: + return X509_V_ERR_INVALID_CA; + case SSL_AD_UNSUPPORTED_CERTIFICATE: + return X509_V_ERR_INVALID_PURPOSE; + default: + return X509_V_ERR_APPLICATION_VERIFICATION; + } +} + + + /** * This is the OpenSSL callback which invokes the BoringSSL callback. * Return 1 to indicate verification success and 0 to indicate verification failure @@ -47,28 +94,14 @@ static int ossl_cert_verify_callback(X509_STORE_CTX *ctx, void *arg) { // Translate the TLS alert value, received from the BoringSSL callback, to an X509 error, and // set it on the X509_STORE_CTX. OpenSSL will ultimately translate the X509 error back into a // TLS alert value which it will send to the peer. - switch(alert) { - case SSL_AD_CERTIFICATE_EXPIRED: { - ossl_X509_STORE_CTX_set_error(ctx, X509_V_ERR_CERT_HAS_EXPIRED); - break; - } - case SSL_AD_UNKNOWN_CA: { - ossl_X509_STORE_CTX_set_error(ctx, X509_V_ERR_INVALID_CA); - break; - } - default: { - // Setting this X509 error sends a SSL_AD_HANDSHAKE_FAILURE alert to the peer - ossl_X509_STORE_CTX_set_error(ctx, X509_V_ERR_APPLICATION_VERIFICATION); - break; - } - } + ossl_X509_STORE_CTX_set_error(ctx, tls_alert_to_x590_err(alert)); return 0; } case ssl_verify_retry: { // TODO: Use ossl_SSL_set_retry_verify() for client side // TODO: Use ossl_ASYNC_pause/start_job() for server side (or both sides) bssl_compat_error("Async certificate validation not supported"); - ossl_X509_STORE_CTX_set_error(ctx, X509_V_ERR_APPLICATION_VERIFICATION); + ossl_X509_STORE_CTX_set_error(ctx, X509_V_ERR_INVALID_CALL); return 0; } } diff --git a/bssl-compat/source/test/test_ssl.cc b/bssl-compat/source/test/test_ssl.cc index 2a8b064cd3..17f646514f 100644 --- a/bssl-compat/source/test/test_ssl.cc +++ b/bssl-compat/source/test/test_ssl.cc @@ -1083,21 +1083,25 @@ TEST(SSLTest, test_SSL_set1_curves_list) { static bool CompleteHandshakes(SSL *client, SSL *server) { - for (;;) { + bool result {true}; + + while(result) { int client_ret = SSL_do_handshake(client); int client_err = SSL_get_error(client, client_ret); + + int server_ret = SSL_do_handshake(server); + int server_err = SSL_get_error(server, server_ret); + if (client_err != SSL_ERROR_NONE && client_err != SSL_ERROR_WANT_READ && client_err != SSL_ERROR_WANT_WRITE) { fprintf(stderr, "Client error: %s\n", SSL_error_description(client_err)); ERR_print_errors_fp(stderr); - return false; + result = false; } - int server_ret = SSL_do_handshake(server); - int server_err = SSL_get_error(server, server_ret); if (server_err != SSL_ERROR_NONE && server_err != SSL_ERROR_WANT_READ && server_err != SSL_ERROR_WANT_WRITE) { fprintf(stderr, "Server error: %s\n", SSL_error_description(server_err)); ERR_print_errors_fp(stderr); - return false; + result = false; } if (client_ret == 1 && server_ret == 1) { @@ -1105,7 +1109,7 @@ static bool CompleteHandshakes(SSL *client, SSL *server) { } } - return true; + return result; } class SocketCloser { @@ -1553,3 +1557,82 @@ TEST(SSLTest, test_SSL_set_ocsp_response_inside_select_certificate_cb) { ASSERT_EQ(sizeof(OCSP_RESPONSE), ocsp_resp_len); ASSERT_EQ(0, memcmp(OCSP_RESPONSE, ocsp_resp_data, ocsp_resp_len)); } + + +/** + * Test that setting a TLS alert and returning ssl_verify_invalid, from a + * callback installed via SSL_CTX_set_custom_verify(), results in a handshake + * failure, and that same TLS alert being received by the peer (subject to our + * restrictions on mapping from TLS alert to X509 error code and back again) + */ +TEST(SSLTest, test_SSL_CTX_set_custom_verify_alert_codes) { + TempFile server_2_key_pem { server_2_key_pem_str }; + TempFile server_2_cert_chain_pem { server_2_cert_chain_pem_str }; + + static const std::map alerts { + // Alerts that we can map + { SSL_AD_HANDSHAKE_FAILURE, "handshake failure" }, + { SSL_AD_CERTIFICATE_EXPIRED, "certificate expired" }, + { SSL_AD_BAD_CERTIFICATE, "bad certificate" }, + { SSL_AD_CERTIFICATE_REVOKED, "certificate revoked" }, + { SSL_AD_DECRYPT_ERROR, "decrypt error" }, + { SSL_AD_UNKNOWN_CA, "unknown CA" }, + { SSL_AD_CERTIFICATE_UNKNOWN, "certificate unknown" }, + { SSL_AD_UNSUPPORTED_CERTIFICATE, "unsupported certificate" }, + { SSL_AD_INTERNAL_ERROR, "internal error" }, + { SSL_AD_CERTIFICATE_REVOKED, "certificate revoked" }, + // Alerts that we cannot map + { SSL_AD_ACCESS_DENIED, "handshake failure" }, + { SSL_AD_BAD_CERTIFICATE_HASH_VALUE, "handshake failure" }, + { SSL_AD_BAD_CERTIFICATE_STATUS_RESPONSE, "handshake failure" }, + { SSL_AD_BAD_RECORD_MAC, "handshake failure" }, + { SSL_AD_CERTIFICATE_REQUIRED, "handshake failure" }, + { SSL_AD_CERTIFICATE_UNOBTAINABLE, "handshake failure" }, + { SSL_AD_CLOSE_NOTIFY, "handshake failure" }, + { SSL_AD_DECODE_ERROR, "handshake failure" }, + { SSL_AD_DECOMPRESSION_FAILURE, "handshake failure" }, + // An invalid alert code + { 255, "handshake failure" }, + }; + + for (auto const& [alert_code, alert_string] : alerts) { + int sockets[2]; + ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0, sockets)); + SocketCloser close[] { sockets[0], sockets[1] }; + + bssl::UniquePtr server_ctx(SSL_CTX_new(TLS_server_method())); + bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_client_method())); + + // Install an info callback on the server to check for the expected tls alert + // If the server reads a TLS alert, it stores the string description of the + // alert in the SSL object's app data so that the test code can check for it. + ossl_SSL_CTX_set_info_callback(server_ctx.get(), [](const SSL *ssl, int type, int val){ + if (type & ossl_SSL_CB_READ_ALERT) { + SSL_set_app_data(const_cast(ssl), ossl_SSL_alert_desc_string_long(val)); + } + }); + ASSERT_TRUE(SSL_CTX_use_certificate_chain_file(server_ctx.get(), server_2_cert_chain_pem.path())); + ASSERT_TRUE(SSL_CTX_use_PrivateKey_file(server_ctx.get(), server_2_key_pem.path(), SSL_FILETYPE_PEM)); + bssl::UniquePtr server_ssl(SSL_new(server_ctx.get())); + ASSERT_TRUE(SSL_set_fd(server_ssl.get(), sockets[0])); + SSL_set_accept_state(server_ssl.get()); + + // Set up client with a custom verify callback that will set out_alert and + // return ssl_verify_invalid. This should cause the client to send the + // specified TLS alert to the server and cause a handshake failure. + SSL_CTX_set_custom_verify(client_ctx.get(), SSL_VERIFY_PEER, [](SSL *ssl, uint8_t *out_alert) -> enum ssl_verify_result_t { + *out_alert = *static_cast(SSL_get_app_data(ssl)); + return ssl_verify_invalid; + }); + bssl::UniquePtr client_ssl(SSL_new(client_ctx.get())); + ASSERT_TRUE(SSL_set_fd(client_ssl.get(), sockets[1])); + SSL_set_connect_state(client_ssl.get()); + // Setup the TLS alert code to be sent from the custom verify callback + SSL_set_app_data(client_ssl.get(), &alert_code); + + ASSERT_FALSE(CompleteHandshakes(client_ssl.get(), server_ssl.get())); + + // Check that the server received the expected TLS alert + ASSERT_STREQ(alert_string, static_cast(SSL_get_app_data(server_ssl.get()))); + } +}