From f602ba9f2cdde55575fd3ac9ca17898525b9956f Mon Sep 17 00:00:00 2001 From: rootvector2 Date: Mon, 18 May 2026 13:23:01 +0530 Subject: [PATCH 1/2] mod_ssl: fix leak of X509 reference in quick renegotiation path In ssl_hook_Access_classic(), when SSL renegotiation is performed via the quick path, SSL_get_peer_certificate() is called and its result is used to seed the certificate stack passed to X509_STORE_CTX_init(). SSL_get_peer_certificate() increments the X509 refcount and the caller owns the returned reference. Every other call site in this file releases the reference with X509_free(); this one did not, except in the narrow case where the original peer chain was empty (or NULL) and we built a fresh stack with sk_X509_push(), in which case the trailing sk_X509_pop_free() happens to release it. In the common path -- non-empty SSL_get_peer_cert_chain(ssl) -- the returned reference was never released, leaking one X509 reference per quick renegotiation for the lifetime of the SSL_CTX. The two early returns at "Cannot find certificate storage" and the X509_STORE_CTX_init failure path leaked unconditionally. Track the owned reference separately as peer_cert and release it in all exit paths. When ownership is transferred into the freshly-created stack via sk_X509_push(), peer_cert is cleared so sk_X509_pop_free() remains the sole owner and there is no double free. --- modules/ssl/ssl_engine_kernel.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/modules/ssl/ssl_engine_kernel.c b/modules/ssl/ssl_engine_kernel.c index 9ee25ec52ca..71dce7f6b89 100644 --- a/modules/ssl/ssl_engine_kernel.c +++ b/modules/ssl/ssl_engine_kernel.c @@ -721,6 +721,7 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo if (renegotiate_quick) { STACK_OF(X509) *cert_stack; X509 *cert; + X509 *peer_cert; /* perform just a manual re-verification of the peer */ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02258) @@ -729,7 +730,12 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo cert_stack = (STACK_OF(X509) *)SSL_get_peer_cert_chain(ssl); - cert = SSL_get_peer_certificate(ssl); + /* SSL_get_peer_certificate() increments the X509 refcount; the + * reference is owned here and must be released with X509_free() + * unless ownership is transferred into a stack we then pop_free. + */ + peer_cert = SSL_get_peer_certificate(ssl); + cert = peer_cert; if (!cert_stack || (sk_X509_num(cert_stack) == 0)) { if (!cert) { @@ -746,6 +752,7 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo */ cert_stack = sk_X509_new_null(); sk_X509_push(cert_stack, cert); + peer_cert = NULL; /* ownership transferred to cert_stack */ } if (!(cert_store || @@ -754,6 +761,10 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02223) "Cannot find certificate storage"); + if (cert_stack != SSL_get_peer_cert_chain(ssl)) { + sk_X509_pop_free(cert_stack, X509_free); + } + if (peer_cert) X509_free(peer_cert); return HTTP_FORBIDDEN; } @@ -764,6 +775,10 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo cert_store_ctx = X509_STORE_CTX_new(); if (!X509_STORE_CTX_init(cert_store_ctx, cert_store, cert, cert_stack)) { X509_STORE_CTX_free(cert_store_ctx); + if (cert_stack != SSL_get_peer_cert_chain(ssl)) { + sk_X509_pop_free(cert_stack, X509_free); + } + if (peer_cert) X509_free(peer_cert); return HTTP_FORBIDDEN; } depth = SSL_get_verify_depth(ssl); @@ -790,6 +805,7 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo /* we created this ourselves, so free it */ sk_X509_pop_free(cert_stack, X509_free); } + if (peer_cert) X509_free(peer_cert); } else { char peekbuf[1]; From a93535daedc71c951d5ceb8e29e9f619eac1955a Mon Sep 17 00:00:00 2001 From: Naveed Khan Date: Thu, 2 Jul 2026 21:10:27 +0530 Subject: [PATCH 2/2] mod_ssl: split quick renegotiation verify into helper with single cleanup path Signed-off-by: Naveed Khan --- modules/ssl/ssl_engine_kernel.c | 184 +++++++++++++++++--------------- 1 file changed, 99 insertions(+), 85 deletions(-) diff --git a/modules/ssl/ssl_engine_kernel.c b/modules/ssl/ssl_engine_kernel.c index 71dce7f6b89..b1f62a433df 100644 --- a/modules/ssl/ssl_engine_kernel.c +++ b/modules/ssl/ssl_engine_kernel.c @@ -352,7 +352,101 @@ static int ssl_check_post_client_verify(request_rec *r, SSLSrvConfigRec *sc, } /* - * Access Handler, classic flavour, for SSL/TLS up to v1.2 + * Manually re-verify the peer certificate for the "quick" renegotiation + * path. Split out of ssl_hook_Access_classic() so the X509 reference from + * SSL_get_peer_certificate() and the stack we may build are released on a + * single cleanup path instead of being duplicated across each error return. + * Returns OK on success, or an HTTP_* error. + */ +static int ssl_verify_peer_quick(request_rec *r, SSL *ssl, SSL_CTX *ctx) +{ + STACK_OF(X509) *cert_stack; + X509 *cert; + X509 *peer_cert; + X509_STORE *cert_store = NULL; + X509_STORE_CTX *cert_store_ctx; + int depth; + int rv = OK; + + cert_stack = (STACK_OF(X509) *)SSL_get_peer_cert_chain(ssl); + + /* SSL_get_peer_certificate() increments the X509 refcount; the reference + * is owned here and must be released with X509_free() unless ownership is + * transferred into a stack we then pop_free. + */ + peer_cert = SSL_get_peer_certificate(ssl); + cert = peer_cert; + + if (!cert_stack || (sk_X509_num(cert_stack) == 0)) { + if (!cert) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02222) + "Cannot find peer certificate chain"); + return HTTP_FORBIDDEN; + } + + /* client cert is in the session cache, but there is + * no chain, since ssl3_get_client_certificate() + * sk_X509_shift-ed the peer cert out of the chain. + * we put it back here for the purpose of quick_renegotiation. + */ + cert_stack = sk_X509_new_null(); + sk_X509_push(cert_stack, cert); + peer_cert = NULL; /* ownership transferred to cert_stack */ + } + + if (!(cert_store || + (cert_store = SSL_CTX_get_cert_store(ctx)))) + { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02223) + "Cannot find certificate storage"); + rv = HTTP_FORBIDDEN; + goto cleanup; + } + + if (!cert) { + cert = sk_X509_value(cert_stack, 0); + } + + cert_store_ctx = X509_STORE_CTX_new(); + if (!X509_STORE_CTX_init(cert_store_ctx, cert_store, cert, cert_stack)) { + X509_STORE_CTX_free(cert_store_ctx); + rv = HTTP_FORBIDDEN; + goto cleanup; + } + depth = SSL_get_verify_depth(ssl); + + if (depth >= 0) { + X509_STORE_CTX_set_depth(cert_store_ctx, depth); + } + + X509_STORE_CTX_set_ex_data(cert_store_ctx, + SSL_get_ex_data_X509_STORE_CTX_idx(), + (char *)ssl); + + if (!X509_verify_cert(cert_store_ctx)) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02224) + "Re-negotiation verification step failed"); + ssl_log_ssl_error(SSLLOG_MARK, APLOG_ERR, r->server); + } + + SSL_set_verify_result(ssl, X509_STORE_CTX_get_error(cert_store_ctx)); + X509_STORE_CTX_cleanup(cert_store_ctx); + X509_STORE_CTX_free(cert_store_ctx); + +cleanup: + if (cert_stack != SSL_get_peer_cert_chain(ssl)) { + /* we created this ourselves, so free it */ + sk_X509_pop_free(cert_stack, X509_free); + } + if (peer_cert) { + X509_free(peer_cert); + } + + return rv; +} + +/* + * Access Handler, classic flavour, for SSL/TLS up to v1.2 * where everything can be renegotiated and no one is happy. */ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirConfigRec *dc, @@ -363,11 +457,9 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo SSL_CTX *ctx = ssl ? SSL_get_SSL_CTX(ssl) : NULL; BOOL renegotiate = FALSE, renegotiate_quick = FALSE; X509 *peercert; - X509_STORE *cert_store = NULL; - X509_STORE_CTX *cert_store_ctx; STACK_OF(SSL_CIPHER) *cipher_list_old = NULL, *cipher_list = NULL; const SSL_CIPHER *cipher = NULL; - int depth, verify_old, verify, n, rc; + int verify_old, verify, n, rc; const char *ncipher_suite; #ifdef HAVE_SRP @@ -719,93 +811,15 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo "Requesting connection re-negotiation"); if (renegotiate_quick) { - STACK_OF(X509) *cert_stack; - X509 *cert; - X509 *peer_cert; - /* perform just a manual re-verification of the peer */ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02258) "Performing quick renegotiation: " "just re-verifying the peer"); - cert_stack = (STACK_OF(X509) *)SSL_get_peer_cert_chain(ssl); - - /* SSL_get_peer_certificate() increments the X509 refcount; the - * reference is owned here and must be released with X509_free() - * unless ownership is transferred into a stack we then pop_free. - */ - peer_cert = SSL_get_peer_certificate(ssl); - cert = peer_cert; - - if (!cert_stack || (sk_X509_num(cert_stack) == 0)) { - if (!cert) { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02222) - "Cannot find peer certificate chain"); - - return HTTP_FORBIDDEN; - } - - /* client cert is in the session cache, but there is - * no chain, since ssl3_get_client_certificate() - * sk_X509_shift-ed the peer cert out of the chain. - * we put it back here for the purpose of quick_renegotiation. - */ - cert_stack = sk_X509_new_null(); - sk_X509_push(cert_stack, cert); - peer_cert = NULL; /* ownership transferred to cert_stack */ - } - - if (!(cert_store || - (cert_store = SSL_CTX_get_cert_store(ctx)))) - { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02223) - "Cannot find certificate storage"); - - if (cert_stack != SSL_get_peer_cert_chain(ssl)) { - sk_X509_pop_free(cert_stack, X509_free); - } - if (peer_cert) X509_free(peer_cert); - return HTTP_FORBIDDEN; - } - - if (!cert) { - cert = sk_X509_value(cert_stack, 0); - } - - cert_store_ctx = X509_STORE_CTX_new(); - if (!X509_STORE_CTX_init(cert_store_ctx, cert_store, cert, cert_stack)) { - X509_STORE_CTX_free(cert_store_ctx); - if (cert_stack != SSL_get_peer_cert_chain(ssl)) { - sk_X509_pop_free(cert_stack, X509_free); - } - if (peer_cert) X509_free(peer_cert); - return HTTP_FORBIDDEN; - } - depth = SSL_get_verify_depth(ssl); - - if (depth >= 0) { - X509_STORE_CTX_set_depth(cert_store_ctx, depth); - } - - X509_STORE_CTX_set_ex_data(cert_store_ctx, - SSL_get_ex_data_X509_STORE_CTX_idx(), - (char *)ssl); - - if (!X509_verify_cert(cert_store_ctx)) { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02224) - "Re-negotiation verification step failed"); - ssl_log_ssl_error(SSLLOG_MARK, APLOG_ERR, r->server); - } - - SSL_set_verify_result(ssl, X509_STORE_CTX_get_error(cert_store_ctx)); - X509_STORE_CTX_cleanup(cert_store_ctx); - X509_STORE_CTX_free(cert_store_ctx); - - if (cert_stack != SSL_get_peer_cert_chain(ssl)) { - /* we created this ourselves, so free it */ - sk_X509_pop_free(cert_stack, X509_free); + rc = ssl_verify_peer_quick(r, ssl, ctx); + if (rc != OK) { + return rc; } - if (peer_cert) X509_free(peer_cert); } else { char peekbuf[1];