From 21c9201d2fe1aff7c7756b70c1327b9108327bce Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Mon, 19 Feb 2024 20:25:40 +0100 Subject: [PATCH] =?UTF-8?q?Don't=20share=20tcn=5Fssl=5Fverify=5Fconfig=5Ft?= =?UTF-8?q?=20between=20tcn=5Fssl=5Fstate=5Ft=20and=20tcn=5Fs=E2=80=A6=20(?= =?UTF-8?q?#850)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …sl_ctxt_t Motivation: Let's not share the same memory between tcn_ssl_state_t and tcn_ssl_ctxt_t as it makes lifetime handling of the memory and prone to segfaults Modifications: - Let tcn_ssl_state_t contain tcn_ssl_verify_config_t and copy over the config from the tcn_ssl_ctxt_t if needed. - Remove free calls that are not needed anymore Result: Fix possible lifetime issue --- openssl-dynamic/src/main/c/ssl.c | 27 +++++------------------- openssl-dynamic/src/main/c/ssl_private.h | 2 +- openssl-dynamic/src/main/c/sslcontext.c | 7 +++--- 3 files changed, 9 insertions(+), 27 deletions(-) diff --git a/openssl-dynamic/src/main/c/ssl.c b/openssl-dynamic/src/main/c/ssl.c index 7bb66df5c..35d6810ca 100644 --- a/openssl-dynamic/src/main/c/ssl.c +++ b/openssl-dynamic/src/main/c/ssl.c @@ -919,8 +919,8 @@ static tcn_ssl_state_t* new_ssl_state(tcn_ssl_ctxt_t* ctx) { memset(state, 0, sizeof(tcn_ssl_state_t)); state->ctx = ctx; - // Initially we will share the configuration from the SSLContext. - state->verify_config = &ctx->verify_config; + // Initially we will copy the configuration from the SSLContext. + memcpy(&state->verify_config, &ctx->verify_config, sizeof(tcn_ssl_verify_config_t)); return state; } @@ -929,12 +929,6 @@ static void free_ssl_state(JNIEnv* e, tcn_ssl_state_t* state) { return; } - // Only free the verify_config if it is not shared with the SSLContext. - if (state->verify_config != NULL && state->verify_config != &state->ctx->verify_config) { - OPENSSL_free(state->verify_config); - state->verify_config = NULL; - } - tcn_ssl_task_free(e, state->ssl_task); state->ssl_task = NULL; @@ -1526,24 +1520,13 @@ TCN_IMPLEMENT_CALL(void, SSL, setVerify)(TCN_STDARGS, jlong ssl, jint level, jin state = tcn_SSL_get_app_state(ssl_); TCN_ASSERT(state != NULL); TCN_ASSERT(state->ctx != NULL); - TCN_ASSERT(state->verify_config != NULL); - - // If we are sharing the configuration from the SSLContext we now need to create a new configuration just for this SSL. - if (state->verify_config == &state->ctx->verify_config) { - if ((state->verify_config = (tcn_ssl_verify_config_t*) OPENSSL_malloc(sizeof(tcn_ssl_verify_config_t))) == NULL) { - tcn_ThrowException(e, "failed to allocate tcn_ssl_verify_config_t"); - return; - } - // Copy the verify depth form the context in case depth is <0. - state->verify_config->verify_depth = state->ctx->verify_config.verify_depth; - } #ifdef OPENSSL_IS_BORINGSSL - SSL_set_custom_verify(ssl_, tcn_set_verify_config(state->verify_config, level, depth), tcn_SSL_cert_custom_verify); + SSL_set_custom_verify(ssl_, tcn_set_verify_config(&state->verify_config, level, depth), tcn_SSL_cert_custom_verify); #else // No need to specify a callback for SSL_set_verify because we override the default certificate verification via SSL_CTX_set_cert_verify_callback. - SSL_set_verify(ssl_, tcn_set_verify_config(state->verify_config, level, depth), NULL); - SSL_set_verify_depth(ssl_, state->verify_config->verify_depth); + SSL_set_verify(ssl_, tcn_set_verify_config(&state->verify_config, level, depth), NULL); + SSL_set_verify_depth(ssl_, state->verify_config.verify_depth); #endif // OPENSSL_IS_BORINGSSL } diff --git a/openssl-dynamic/src/main/c/ssl_private.h b/openssl-dynamic/src/main/c/ssl_private.h index f8f75a692..c3a572217 100644 --- a/openssl-dynamic/src/main/c/ssl_private.h +++ b/openssl-dynamic/src/main/c/ssl_private.h @@ -398,7 +398,7 @@ struct tcn_ssl_state_t { int handshakeCount; tcn_ssl_ctxt_t *ctx; tcn_ssl_task_t* ssl_task; - tcn_ssl_verify_config_t* verify_config; + tcn_ssl_verify_config_t verify_config; }; #define TCN_GET_SSL_CTX(ssl, C) \ diff --git a/openssl-dynamic/src/main/c/sslcontext.c b/openssl-dynamic/src/main/c/sslcontext.c index ec8dcbb24..f6ce11fc4 100644 --- a/openssl-dynamic/src/main/c/sslcontext.c +++ b/openssl-dynamic/src/main/c/sslcontext.c @@ -1426,14 +1426,13 @@ static jbyteArray get_certs(JNIEnv *e, SSL* ssl, STACK_OF(X509)* chain) { tcn_ssl_state_t* state = tcn_SSL_get_app_state(ssl); TCN_ASSERT(state != NULL); - TCN_ASSERT(state->verify_config != NULL); // SSL_CTX_set_verify_depth() and SSL_set_verify_depth() set the limit up to which depth certificates in a chain are // used during the verification procedure. If the certificate chain is longer than allowed, the certificates above // the limit are ignored. Error messages are generated as if these certificates would not be present, // most likely a X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY will be issued. // https://www.openssl.org/docs/man1.0.2/ssl/SSL_set_verify.html - int len = TCN_MIN(state->verify_config->verify_depth, totalQueuedLength); + int len = TCN_MIN(state->verify_config.verify_depth, totalQueuedLength); unsigned i; int length; @@ -1562,7 +1561,7 @@ static int SSL_cert_verify(X509_STORE_CTX *ctx, void *arg) { #endif // X509_V_ERR_UNSPECIFIED - // TODO(scott): if verify_config->verify_depth == SSL_CVERIFY_OPTIONAL we have the option to let the handshake + // TODO(scott): if verify_config.verify_depth == SSL_CVERIFY_OPTIONAL we have the option to let the handshake // succeed for some of the "informational" error messages (e.g. X509_V_ERR_EMAIL_MISMATCH ?) complete: @@ -1674,7 +1673,7 @@ enum ssl_verify_result_t tcn_SSL_cert_custom_verify(SSL* ssl, uint8_t *out_alert result = X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY; } - // TODO(scott): if verify_config->verify_depth == SSL_CVERIFY_OPTIONAL we have the option to let the handshake + // TODO(scott): if verify_config.verify_depth == SSL_CVERIFY_OPTIONAL we have the option to let the handshake // succeed for some of the "informational" error messages (e.g. X509_V_ERR_EMAIL_MISMATCH ?) } complete: