From 2cd5966eaef705ac478922e409c4b1f00a738869 Mon Sep 17 00:00:00 2001 From: ouki Date: Thu, 22 Oct 2026 16:20:34 +0900 Subject: [PATCH 1/2] Fail SSL_CTRL_CHAIN when a chain certificate cannot be added --- src/ssl.c | 45 +++++++++++++++--------- tests/api/test_ssl_cert.c | 74 +++++++++++++++++++++++++++++++++++++++ tests/api/test_ssl_cert.h | 2 ++ 3 files changed, 105 insertions(+), 16 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index 3773badb1d9..ee7a5984cfa 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -8719,11 +8719,11 @@ long wolfSSL_CTX_ctrl(WOLFSSL_CTX* ctx, int cmd, long opt, void* pt) case SSL_CTRL_CHAIN: #ifdef SESSION_CERTS { - /* - * We don't care about opt here because a copy of the certificate is - * stored anyway so increasing the reference counter is not necessary. - * Just check to make sure that it is set to one of the correct values. - */ + /* Build ctx->certChain (DER copies) from sk. A reference is taken on + * each certificate because wolfSSL_CTX_add_extra_chain_cert() consumes + * one on success. The ctx takes ownership of sk only on success; on + * failure no chain is set and the caller still owns sk. + * opt: 0 = set0, 1 = set1. */ WOLF_STACK_OF(WOLFSSL_X509)* sk = (WOLF_STACK_OF(WOLFSSL_X509)*) pt; WOLFSSL_X509* x509; int i; @@ -8736,32 +8736,45 @@ long wolfSSL_CTX_ctrl(WOLFSSL_CTX* ctx, int cmd, long opt, void* pt) if (sk) { for (i = 0; i < wolfSSL_sk_X509_num(sk); i++) { x509 = wolfSSL_sk_X509_value(sk, i); + if (x509 == NULL) { + ret = WOLFSSL_FAILURE; + break; + } /* Prevent wolfSSL_CTX_add_extra_chain_cert from freeing cert */ if (wolfSSL_X509_up_ref(x509) != 1) { WOLFSSL_MSG("Error increasing reference count"); - continue; + ret = WOLFSSL_FAILURE; + break; } if (wolfSSL_CTX_add_extra_chain_cert(ctx, x509) != WOLFSSL_SUCCESS) { WOLFSSL_MSG("Error adding certificate to context"); /* Decrease reference count on failure */ wolfSSL_X509_free(x509); - x509 = NULL; + ret = WOLFSSL_FAILURE; + break; } } + x509 = NULL; } - /* Free previous chain */ + + /* Free previous chain. On failure the ctx is left with no chain */ wolfSSL_sk_X509_pop_free(ctx->x509Chain, NULL); - ctx->x509Chain = sk; - if (sk && opt == 1) { - /* up all refs when opt == 1 */ - for (i = 0; i < wolfSSL_sk_X509_num(sk); i++) { - x509 = wolfSSL_sk_X509_value(sk, i); - if (wolfSSL_X509_up_ref(x509) != 1) { - WOLFSSL_MSG("Error increasing reference count"); - continue; + ctx->x509Chain = NULL; + if (ret == WOLFSSL_SUCCESS) { + ctx->x509Chain = sk; + if (sk && opt == 1) { + /* up all refs when opt == 1 */ + for (i = 0; i < wolfSSL_sk_X509_num(sk); i++) { + x509 = wolfSSL_sk_X509_value(sk, i); + if (wolfSSL_X509_up_ref(x509) != 1) { + WOLFSSL_MSG("Error increasing reference count"); + continue; + } } } + } else { + FreeDer(&ctx->certChain); } } #else diff --git a/tests/api/test_ssl_cert.c b/tests/api/test_ssl_cert.c index 4ff30806b8b..433d0988b8a 100644 --- a/tests/api/test_ssl_cert.c +++ b/tests/api/test_ssl_cert.c @@ -773,6 +773,80 @@ int test_wolfSSL_get_peer_certificate_ref(void) #endif return EXPECT_RESULT(); } +/* Setting a chain via SSL_CTRL_CHAIN must fail if any certificate cannot be + * added, instead of reporting success with inconsistent DER and X509 chains. + */ +int test_wolfSSL_CTX_set0_chain_bad_cert(void) +{ + EXPECT_DECLS; +#if (defined(OPENSSL_ALL) || defined(WOLFSSL_ASIO) || \ + defined(WOLFSSL_HAPROXY) || defined(WOLFSSL_NGINX)) && \ + defined(SESSION_CERTS) && !defined(NO_CERTS) && !defined(NO_TLS) && \ + !defined(NO_FILESYSTEM) && !defined(NO_RSA) && \ + (!defined(NO_WOLFSSL_CLIENT) || !defined(NO_WOLFSSL_SERVER)) + WOLFSSL_CTX* ctx = NULL; + WOLF_STACK_OF(WOLFSSL_X509)* chain = NULL; + WOLF_STACK_OF(WOLFSSL_X509)* goodChain = NULL; + WOLFSSL_X509* ca = NULL; + WOLFSSL_X509* goodCa = NULL; + WOLFSSL_X509* empty = NULL; + int ret = 0; + +#ifndef NO_WOLFSSL_CLIENT + ExpectNotNull(ctx = wolfSSL_CTX_new(wolfSSLv23_client_method())); +#else + ExpectNotNull(ctx = wolfSSL_CTX_new(wolfSSLv23_server_method())); +#endif + /* Load the leaf first so the stack entries are added as chain certs. */ + ExpectIntEQ(wolfSSL_CTX_use_certificate_file(ctx, svrCertFile, + WOLFSSL_FILETYPE_PEM), WOLFSSL_SUCCESS); + + /* Set a valid chain first so the failure path must release it. */ + ExpectNotNull(goodChain = sk_X509_new_null()); + ExpectNotNull(goodCa = wolfSSL_X509_load_certificate_file(caCertFile, + WOLFSSL_FILETYPE_PEM)); + ExpectIntGT(sk_X509_push(goodChain, goodCa), 0); + if (EXPECT_FAIL()) { + X509_free(goodCa); + } + ret = (int)SSL_CTX_set0_chain(ctx, goodChain); + ExpectIntEQ(ret, WOLFSSL_SUCCESS); + /* ctx owns goodChain on success */ + if (ret != WOLFSSL_SUCCESS) { + sk_X509_pop_free(goodChain, X509_free); + } + + ExpectNotNull(chain = sk_X509_new_null()); + ExpectNotNull(ca = wolfSSL_X509_load_certificate_file(caCertFile, + WOLFSSL_FILETYPE_PEM)); + ExpectIntGT(sk_X509_push(chain, ca), 0); + if (EXPECT_FAIL()) { + X509_free(ca); + } + /* An allocated but unpopulated X509 has no DER and cannot be added. */ + ExpectNotNull(empty = X509_new()); + ExpectIntGT(sk_X509_push(chain, empty), 0); + if (EXPECT_FAIL()) { + X509_free(empty); + } + + ret = (int)SSL_CTX_set0_chain(ctx, chain); + ExpectIntNE(ret, WOLFSSL_SUCCESS); + /* On failure, the ctx must not keep a partially built chain. */ + if (ctx != NULL) { + ExpectNull(ctx->certChain); + ExpectNull(ctx->x509Chain); + } + + /* The ctx takes ownership of the stack only on success. */ + if (ret != WOLFSSL_SUCCESS) { + sk_X509_pop_free(chain, X509_free); + } + + wolfSSL_CTX_free(ctx); +#endif + return EXPECT_RESULT(); +} /* Guarded to match its only caller, test_wolfSSL_CTX_set_client_cert_cb(), * which needs OPENSSL_EXTRA for the ctx->CBClientCert field. */ diff --git a/tests/api/test_ssl_cert.h b/tests/api/test_ssl_cert.h index 952e007321e..625516ef95f 100644 --- a/tests/api/test_ssl_cert.h +++ b/tests/api/test_ssl_cert.h @@ -44,6 +44,7 @@ int test_wolfSSL_get_chain_cert_pem(void); int test_wolfSSL_cmp_peer_cert_to_file(void); int test_wolfSSL_peer_cert_copy_oom(void); int test_wolfSSL_get_peer_certificate_ref(void); +int test_wolfSSL_CTX_set0_chain_bad_cert(void); int test_wolfSSL_CTX_set_client_cert_cb(void); int test_wolfSSL_CTX_set_cert_cb(void); int test_wolfSSL_cert_setup_cb_ret(void); @@ -93,6 +94,7 @@ int test_wolfSSL_SESSION_get0_peer_full_chain(void); TEST_DECL_GROUP("ssl_cert", test_wolfSSL_cmp_peer_cert_to_file), \ TEST_DECL_GROUP("ssl_cert", test_wolfSSL_peer_cert_copy_oom), \ TEST_DECL_GROUP("ssl_cert", test_wolfSSL_get_peer_certificate_ref), \ + TEST_DECL_GROUP("ssl_cert", test_wolfSSL_CTX_set0_chain_bad_cert), \ TEST_DECL_GROUP("ssl_cert", test_wolfSSL_CTX_set_client_cert_cb), \ TEST_DECL_GROUP("ssl_cert", test_wolfSSL_CTX_set_cert_cb), \ TEST_DECL_GROUP("ssl_cert", test_wolfSSL_cert_setup_cb_ret), \ From 317836b682aa279efd58cad405d81e759aa4d61f Mon Sep 17 00:00:00 2001 From: ouki Date: Thu, 22 Oct 2026 17:09:01 +0900 Subject: [PATCH 2/2] Reset certChainCnt on failure and require PEM for chain test --- src/ssl.c | 1 + tests/api/test_ssl_cert.c | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/ssl.c b/src/ssl.c index ee7a5984cfa..2a418b025a7 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -8775,6 +8775,7 @@ long wolfSSL_CTX_ctrl(WOLFSSL_CTX* ctx, int cmd, long opt, void* pt) } } else { FreeDer(&ctx->certChain); + ctx->certChainCnt = 0; } } #else diff --git a/tests/api/test_ssl_cert.c b/tests/api/test_ssl_cert.c index 433d0988b8a..2d19e204548 100644 --- a/tests/api/test_ssl_cert.c +++ b/tests/api/test_ssl_cert.c @@ -783,6 +783,7 @@ int test_wolfSSL_CTX_set0_chain_bad_cert(void) defined(WOLFSSL_HAPROXY) || defined(WOLFSSL_NGINX)) && \ defined(SESSION_CERTS) && !defined(NO_CERTS) && !defined(NO_TLS) && \ !defined(NO_FILESYSTEM) && !defined(NO_RSA) && \ + defined(WOLFSSL_PEM_TO_DER) && \ (!defined(NO_WOLFSSL_CLIENT) || !defined(NO_WOLFSSL_SERVER)) WOLFSSL_CTX* ctx = NULL; WOLF_STACK_OF(WOLFSSL_X509)* chain = NULL; @@ -836,6 +837,7 @@ int test_wolfSSL_CTX_set0_chain_bad_cert(void) if (ctx != NULL) { ExpectNull(ctx->certChain); ExpectNull(ctx->x509Chain); + ExpectIntEQ(ctx->certChainCnt, 0); } /* The ctx takes ownership of the stack only on success. */