From 2129615b8a6b50ead7643325989e1aa7120a0cf0 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Thu, 5 Nov 2026 18:32:47 +0000 Subject: [PATCH 1/7] Drop the CTX OCSP request when the CTX certificate is replaced The leaf OCSP request that TLS 1.2 stapling builds is cached on the WOLFSSL_CTX (certOcspRequest) and reused by every later connection that shares the CTX certificate. Only wolfSSL_CTX_free() freed it, so after the certificate was replaced on a live CTX the server kept asking the responder about, and stapling the status of, the certificate it used to have. FreeCtxOcspRequestCache() takes the cached request off the CTX under the CTX OCSP lock and frees it. ProcessBufferCertHandleDer(), which all the CTX certificate and chain loaders go through, and wolfSSL_CTX_use_certificate() call it when they replace the leaf. The next connection that staples builds and caches a request for the new certificate. GetCtxOcspLock() now takes the CTX so that both sides can use it. Only connections that share the CTX certificate use the cached request, and replacing that certificate frees the buffer they share, so none of them can still be using it. The lock orders the drop against a connection publishing a request. The chain cache (chainOcspRequest[]) is never filled, so there is nothing to drop there. test_ocsp_ctx_request_cache_cert_change() switches the CTX from server1 to server2 with the file loader and back with wolfSSL_CTX_use_certificate() (the file loader without OPENSSL_EXTRA), and checks which certificate the request sent to the responder, and the one left cached, are for. --- src/internal.c | 68 +++++++++++++++--- src/ssl_load.c | 4 ++ tests/api.c | 1 + tests/api/test_ocsp.c | 157 ++++++++++++++++++++++++++++++++++++++++++ tests/api/test_ocsp.h | 1 + wolfssl/internal.h | 7 ++ 6 files changed, 228 insertions(+), 10 deletions(-) diff --git a/src/internal.c b/src/internal.c index 1c5196874d2..9b2145c8bdb 100644 --- a/src/internal.c +++ b/src/internal.c @@ -29588,11 +29588,9 @@ int SendFinished(WOLFSSL* ssl) #endif /* WOLFSSL_NO_TLS12 */ #endif /* !NO_TLS */ -#ifndef NO_WOLFSSL_SERVER -#if (!defined(WOLFSSL_NO_TLS12) && \ - (defined(HAVE_CERTIFICATE_STATUS_REQUEST) || \ - defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2))) || \ - (defined(WOLFSSL_TLS13) && defined(HAVE_CERTIFICATE_STATUS_REQUEST)) +#if !defined(NO_WOLFSSL_SERVER) && \ + (defined(HAVE_CERTIFICATE_STATUS_REQUEST) || \ + defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2)) /* Returns the lock that guards the OCSP request cache on the WOLFSSL_CTX. * * The cache is a field of the CTX, so it has to be serialized with a lock whose @@ -29604,13 +29602,63 @@ int SendFinished(WOLFSSL* ssl) * Returns NULL when the CTX has no cert manager to take a lock from, in which * case the caller skips the cache rather than running unserialized. */ -static wolfSSL_Mutex* GetCtxOcspLock(WOLFSSL* ssl) +static wolfSSL_Mutex* GetCtxOcspLock(WOLFSSL_CTX* ctx) { - if (ssl->ctx->cm == NULL || ssl->ctx->cm->ocsp_stapling == NULL) + if (ctx->cm == NULL || ctx->cm->ocsp_stapling == NULL) return NULL; - return &ssl->ctx->cm->ocsp_stapling->ocspLock; + return &ctx->cm->ocsp_stapling->ocspLock; +} + +/* Drops the leaf OCSP request cached on the CTX. + * + * The request is built from the CTX certificate, so it has to go whenever that + * certificate is replaced. Left in place, every later connection would ask the + * responder about, and staple the status of, the certificate the CTX used to + * have. The next connection that staples builds and caches a new one. + * + * Only connections that share the CTX certificate use the cached request, and + * replacing that certificate frees the buffer they share, so none of them can + * still be using it here. The lock orders this against a connection publishing + * a request. When it cannot be taken the request is still dropped: keeping it + * would be wrong for every connection to come. + */ +void FreeCtxOcspRequestCache(WOLFSSL_CTX* ctx) +{ + wolfSSL_Mutex* ocspLock; + OcspRequest* request; + int locked = 0; + + if (ctx == NULL) + return; + + ocspLock = GetCtxOcspLock(ctx); + if (ocspLock != NULL) { + if (wc_LockMutex(ocspLock) == 0) { + locked = 1; + } + else { + WOLFSSL_MSG("Couldn't lock CTX OCSP mutex, dropping the request"); + } + } + request = ctx->certOcspRequest; + ctx->certOcspRequest = NULL; + if (locked) { + wc_UnLockMutex(ocspLock); + } + + if (request != NULL) { + FreeOcspRequest(request); + XFREE(request, ctx->heap, DYNAMIC_TYPE_OCSP_REQUEST); + } } +#endif + +#ifndef NO_WOLFSSL_SERVER +#if (!defined(WOLFSSL_NO_TLS12) && \ + (defined(HAVE_CERTIFICATE_STATUS_REQUEST) || \ + defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2))) || \ + (defined(WOLFSSL_TLS13) && defined(HAVE_CERTIFICATE_STATUS_REQUEST)) /* Parses and decodes the certificate then initializes "request". * @@ -29716,7 +29764,7 @@ int CreateOcspResponse(WOLFSSL* ssl, OcspRequest** ocspRequest, * connections on this CTX. Failing the handshake over a missed * optimization would be the worse outcome. */ if (ret == 0 && !ssl->buffers.weOwnCert) { - wolfSSL_Mutex* ocspLock = GetCtxOcspLock(ssl); + wolfSSL_Mutex* ocspLock = GetCtxOcspLock(ssl->ctx); /* SSL_CM(ssl) can resolve through ssl->x509_store_pt, so a stapling * object on the CTX manager is not implied by the one checked @@ -30593,7 +30641,7 @@ static int BuildCertificateStatus(WOLFSSL* ssl, byte type, buffer* status, */ static OcspRequest* GetCtxOcspRequest(WOLFSSL* ssl, byte* ctxOwnsRequest) { - wolfSSL_Mutex* ocspLock = GetCtxOcspLock(ssl); + wolfSSL_Mutex* ocspLock = GetCtxOcspLock(ssl->ctx); OcspRequest* request = NULL; *ctxOwnsRequest = 0; diff --git a/src/ssl_load.c b/src/ssl_load.c index 7d1e7bbb965..b6b70626f08 100644 --- a/src/ssl_load.c +++ b/src/ssl_load.c @@ -2535,6 +2535,8 @@ static int ProcessBufferCertHandleDer(WOLFSSL_CTX* ctx, WOLFSSL* ssl, /* Store certificate as ours. */ ctx->certificate = der; /* ourCert is created on demand. */ + /* The cached OCSP request was built from the old certificate. */ + FreeCtxOcspRequestCache(ctx); } } else { @@ -5413,6 +5415,8 @@ int wolfSSL_CTX_use_certificate(WOLFSSL_CTX *ctx, WOLFSSL_X509 *x) if (res == 1) { /* Replace certificate buffer with one holding the new certificate. */ FreeDer(&ctx->certificate); + /* The cached OCSP request was built from the old certificate. */ + FreeCtxOcspRequestCache(ctx); ret = AllocCopyDer(&ctx->certificate, x->derCert->buffer, x->derCert->length, CERT_TYPE, ctx->heap); if (ret != 0) { diff --git a/tests/api.c b/tests/api.c index be6253b80bd..fada6a18c44 100644 --- a/tests/api.c +++ b/tests/api.c @@ -47473,6 +47473,7 @@ TEST_CASE testCases[] = { TEST_DECL_GROUP("ocsp", test_ocsp_status_verify_cb_would_block), TEST_DECL_GROUP("ocsp", test_dtls12_nonblock_ocsp), TEST_DECL_GROUP("ocsp", test_ocsp_ctx_request_cache), + TEST_DECL_GROUP("ocsp", test_ocsp_ctx_request_cache_cert_change), TEST_DECL_GROUP("ocsp", test_ocsp_responder), TEST_DECL_GROUP("ocsp", test_wolfIO_DecodeUrl_crlf_reject), TEST_DECL_GROUP("ocsp", test_wolfIO_DecodeUrl_host_bounds), diff --git a/tests/api/test_ocsp.c b/tests/api/test_ocsp.c index 93c68ad2325..d095633bfcb 100644 --- a/tests/api/test_ocsp.c +++ b/tests/api/test_ocsp.c @@ -6287,11 +6287,168 @@ int test_ocsp_ctx_request_cache(void) return EXPECT_RESULT(); } + +/* Replaces the server certificate and key on the CTX. With "x509Api" set the + * certificate goes in through wolfSSL_CTX_use_certificate(), otherwise through + * the file loader, so both ways of replacing it are covered. */ +static int test_ocsp_ctx_request_rotate_cert(WOLFSSL_CTX* ctx, + const char* certFile, const char* keyFile, int x509Api) +{ + EXPECT_DECLS; + +#ifdef OPENSSL_EXTRA + if (x509Api) { + WOLFSSL_X509* x509 = NULL; + + ExpectNotNull(x509 = wolfSSL_X509_load_certificate_file(certFile, + WOLFSSL_FILETYPE_PEM)); + ExpectIntEQ(wolfSSL_CTX_use_certificate(ctx, x509), WOLFSSL_SUCCESS); + wolfSSL_X509_free(x509); + } + else +#endif + { + (void)x509Api; + ExpectIntEQ(wolfSSL_CTX_use_certificate_file(ctx, certFile, + WOLFSSL_FILETYPE_PEM), WOLFSSL_SUCCESS); + } + ExpectIntEQ(wolfSSL_CTX_use_PrivateKey_file(ctx, keyFile, + WOLFSSL_FILETYPE_PEM), WOLFSSL_SUCCESS); + + return EXPECT_RESULT(); +} + +/* The cached leaf request is built from the CTX certificate, so it has to go + * when that certificate is replaced. Otherwise every later connection asks the + * responder about, and staples the status of, the certificate the CTX used to + * have. + * + * Runs three handshakes over one CTX pair: + * pass 0 - server1, builds the request and caches it, + * pass 1 - after a switch to server2 through the file loader, + * pass 2 - after a switch back to server1 through + * wolfSSL_CTX_use_certificate() (the file loader without + * OPENSSL_EXTRA). + * Both certificates have the same issuer and differ only in serial number + * (05 and 06), which is what the cached request is checked against. The + * responder always answers for server1, so pass 1 is only about which request + * went out, and pass 2 has to complete. + */ +int test_ocsp_ctx_request_cache_cert_change(void) +{ + EXPECT_DECLS; + WOLFSSL_CTX* ctx_c = NULL; + WOLFSSL_CTX* ctx_s = NULL; + WOLFSSL* ssl_c = NULL; + WOLFSSL* ssl_s = NULL; + int i; + struct test_memio_ctx test_ctx; + static const byte serials[3] = { 0x05, 0x06, 0x05 }; + + test_ocsp_ctx_request_cache_cb_cnt = 0; + test_ocsp_ctx_request_cache_firstSz = 0; + test_ocsp_ctx_request_cache_differs = 0; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, NULL, NULL, + wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0); + + /* The chain file leaves intermediate1 on the CTX as the chain, which is + * also the issuer of server2, so only the leaf changes below. */ + ExpectIntEQ(wolfSSL_CTX_use_certificate_chain_file(ctx_s, + "./certs/ocsp/server1-chain-noroot.pem"), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_use_PrivateKey_file(ctx_s, + "./certs/ocsp/server1-key.pem", WOLFSSL_FILETYPE_PEM), + WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_load_verify_locations(ctx_s, + "./certs/ocsp/root-ca-cert.pem", NULL), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_load_verify_locations(ctx_s, + "./certs/ocsp/intermediate1-ca-cert.pem", NULL), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_load_verify_locations(ctx_c, + "./certs/ocsp/root-ca-cert.pem", NULL), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_load_verify_locations(ctx_c, + "./certs/ocsp/intermediate1-ca-cert.pem", NULL), WOLFSSL_SUCCESS); + + ExpectIntEQ(wolfSSL_CTX_EnableOCSPStapling(ctx_c), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_EnableOCSPStapling(ctx_s), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_SetOCSP_OverrideURL(ctx_s, "http://dummy.test"), + WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_EnableOCSP(ctx_s, + WOLFSSL_OCSP_NO_NONCE | WOLFSSL_OCSP_URL_OVERRIDE), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_SetOCSP_Cb(ctx_s, + test_ocsp_ctx_request_cache_io_cb, NULL, NULL), WOLFSSL_SUCCESS); + + for (i = 0; i < 3 && EXPECT_SUCCESS(); i++) { + if (i == 1) { + ExpectIntEQ(test_ocsp_ctx_request_rotate_cert(ctx_s, + "./certs/ocsp/server2-cert.pem", + "./certs/ocsp/server2-key.pem", 0), TEST_SUCCESS); + } + else if (i == 2) { + ExpectIntEQ(test_ocsp_ctx_request_rotate_cert(ctx_s, + "./certs/ocsp/server1-cert.pem", + "./certs/ocsp/server1-key.pem", 1), TEST_SUCCESS); + } + if (i > 0) { + /* Replacing the certificate drops the request built from the old + * one. */ + ExpectNull(ctx_s->certOcspRequest); + } + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0); + ExpectIntEQ(wolfSSL_UseOCSPStapling(ssl_c, WOLFSSL_CSR_OCSP, 0), + WOLFSSL_SUCCESS); + + if (i == 1) { + /* The canned response is for server1, so whether this handshake + * completes is not the point. */ + (void)test_memio_do_handshake(ssl_c, ssl_s, 10, NULL); + } + else { + ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + } + + ExpectIntEQ(test_ocsp_ctx_request_cache_cb_cnt, i + 1); + /* The responder was asked about the certificate in use: pass 1 sent + * something other than pass 0, pass 2 the same as pass 0 again. */ + if (i > 0) { + ExpectIntEQ(test_ocsp_ctx_request_cache_differs, i == 1); + } + /* The request now cached is for the certificate in use. */ + ExpectNotNull(ctx_s->certOcspRequest); + if (ctx_s->certOcspRequest != NULL) { + ExpectIntEQ(ctx_s->certOcspRequest->serialSz, 1); + if (ctx_s->certOcspRequest->serialSz == 1) { + ExpectIntEQ(ctx_s->certOcspRequest->serial[0], serials[i]); + } + } + + /* The connections share the CTX certificate, so they have to be gone + * before it is replaced. */ + wolfSSL_free(ssl_c); + ssl_c = NULL; + wolfSSL_free(ssl_s); + ssl_s = NULL; + } + + wolfSSL_free(ssl_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); + + return EXPECT_RESULT(); +} #else int test_ocsp_ctx_request_cache(void) { return TEST_SKIPPED; } +int test_ocsp_ctx_request_cache_cert_change(void) +{ + return TEST_SKIPPED; +} #endif #if defined(HAVE_OCSP_RESPONDER) && defined(WOLFSSL_ASN_TEMPLATE) && \ diff --git a/tests/api/test_ocsp.h b/tests/api/test_ocsp.h index 43431890751..93b52c01a8d 100644 --- a/tests/api/test_ocsp.h +++ b/tests/api/test_ocsp.h @@ -67,6 +67,7 @@ int test_dtls13_nonblock_ocsp_unfragmented_cert(void); int test_ocsp_status_verify_cb_would_block(void); int test_dtls12_nonblock_ocsp(void); int test_ocsp_ctx_request_cache(void); +int test_ocsp_ctx_request_cache_cert_change(void); int test_ocsp_responder(void); int test_ocsp_ancestor_responder_rejected(void); int test_ocsp_forged_responder_cert_rejected(void); diff --git a/wolfssl/internal.h b/wolfssl/internal.h index 623df4f7569..4539530734d 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -7885,6 +7885,13 @@ WOLFSSL_LOCAL int SendCertificateRequest(WOLFSSL* ssl); WOLFSSL_LOCAL int CreateOcspResponse(WOLFSSL* ssl, OcspRequest** ocspRequest, buffer* response, byte* ctxOwnsRequest); #endif +#if !defined(NO_WOLFSSL_SERVER) && \ + (defined(HAVE_CERTIFICATE_STATUS_REQUEST) || \ + defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2)) +WOLFSSL_LOCAL void FreeCtxOcspRequestCache(WOLFSSL_CTX* ctx); +#else + #define FreeCtxOcspRequestCache(ctx) WC_DO_NOTHING +#endif #if defined(HAVE_SECURE_RENEGOTIATION) && \ !defined(NO_WOLFSSL_SERVER) WOLFSSL_LOCAL int SendHelloRequest(WOLFSSL* ssl); From 02d44b79d80e87af434758f48c4311e0ffd1ed59 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Thu, 5 Nov 2026 18:32:54 +0000 Subject: [PATCH 2/7] Share CTX certificates and keys with SSL objects by reference count OPENSSL_ALL turns on WOLFSSL_COPY_CERT and WOLFSSL_COPY_KEY so that an SSL keeps the certificate, chain and key its CTX had when the SSL was made, even if the CTX replaces or frees them later, as OpenSSL does. SetSSL_CTX_CertsAndKeys() did that by copying all three into every WOLFSSL, and with OPENSSL_EXTRA they stay for the life of the connection. On a Cortex-M33 server that is 0.9 KB per connection with an ECC leaf and 4.7 KB with an RSA leaf and two intermediates. Nothing changes a certificate, chain or key DerBuffer once it is on the CTX: every setter builds a new buffer and frees the old one. So with WOLFSSL_DER_REFCOUNT a DerBuffer carries a reference count and the SSL holds a reference to the CTX's buffers instead of a copy. ShareDer() takes the reference, or makes a copy where the count is not compiled in, and FreeDer() frees, and zeroes a private key, at the last release. WOLFSSL_DER_REFCOUNT is on with WOLFSSL_COPY_CERT or WOLFSSL_COPY_KEY when atomic operations are available or the build is single-threaded; otherwise each SSL still gets a copy. With WOLFSSL_BLIND_PRIVATE_KEY the key is still copied, as each SSL blinds it with its own mask and unblinds it in place. The SSL owns its reference (weOwnCert, weOwnCertChain, weOwnKey), so the free paths, the OCSP request cache and the SNI and certificate setup callback checks behave as they did with the copy. SSL_CtxResourceFree() and wolfSSL_UnloadCertsKeys() no longer zero the key before FreeDer(), which would wipe a key other connections still use; FreeDer() already zeroes private keys. wolfSSL_set_SSL_CTX() now calls SetSSL_CTX_CertsAndKeys() instead of keeping its own copy of it, which also takes the alternative key's id, label and device id from the new CTX. wolfSSL_get_certificate() compares the buffers by pointer first. test_tls_ssl_keeps_ctx_certs() replaces the CTX certificate, chain and key after wolfSSL_new(), frees the CTX and another SSL made from it, and checks that the SSL still holds the same bytes and completes the handshake on them. Server wolfSSL_new() on an STM32U585 with OPENSSL_ALL, bytes: ECC leaf 4922 -> 4064 ECC leaf, 2 intermediates 6500 -> 4064 RSA leaf, 2 intermediates 8740 -> 4064 OPENSSL_EXTRA is unchanged (3876). --- .github/workflows/ocsp.yml | 3 +- src/internal.c | 37 ++++---- src/ssl.c | 101 ++------------------- src/ssl_api_cert.c | 19 ++-- tests/api.c | 38 +++++++- tests/api/test_ocsp.c | 8 +- tests/api/test_tls.c | 156 +++++++++++++++++++++++++++++++++ tests/api/test_tls.h | 2 + wolfcrypt/src/asn.c | 51 +++++++++++ wolfssl/internal.h | 3 + wolfssl/wolfcrypt/asn.h | 1 + wolfssl/wolfcrypt/asn_public.h | 14 +++ wolfssl/wolfcrypt/settings.h | 16 ++-- 13 files changed, 312 insertions(+), 137 deletions(-) diff --git a/.github/workflows/ocsp.yml b/.github/workflows/ocsp.yml index 09cabf9a887..f65fcbbb40f 100644 --- a/.github/workflows/ocsp.yml +++ b/.github/workflows/ocsp.yml @@ -72,7 +72,8 @@ jobs: # reused by every later connection on it, with the CTX owning it. None of the # jobs above reach that cache: it is only populated when the SSL shares the # CTX certificate buffer (ssl->buffers.weOwnCert == 0), and OPENSSL_ALL - # implies WOLFSSL_COPY_CERT, which gives every SSL its own copy instead. + # implies WOLFSSL_COPY_CERT, which gives every SSL its own reference to, or + # copy of, it instead. ocsp_ctx_request_cache: name: ocsp ctx request cache (${{ matrix.name }}) if: ${{ (github.repository_owner == 'wolfssl') && (github.event_name != 'pull_request' || github.event.pull_request.draft == false) }} diff --git a/src/internal.c b/src/internal.c index 9b2145c8bdb..3b8987568be 100644 --- a/src/internal.c +++ b/src/internal.c @@ -3563,17 +3563,14 @@ void SSL_CtxResourceFree(WOLFSSL_CTX* ctx) #endif /* SINGLE_THREADED */ #ifndef NO_CERTS - if (ctx->privateKey != NULL && ctx->privateKey->buffer != NULL) { - ForceZero(ctx->privateKey->buffer, ctx->privateKey->length); - } + /* FreeDer() zeroes a private key when the last holder lets go of it. An + * SSL made from this CTX can still hold it (WOLFSSL_DER_REFCOUNT), so it + * must not be zeroed here. */ FreeDer(&ctx->privateKey); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ctx->privateKeyMask); #endif #ifdef WOLFSSL_DUAL_ALG_CERTS - if (ctx->altPrivateKey != NULL && ctx->altPrivateKey->buffer != NULL) { - ForceZero(ctx->altPrivateKey->buffer, ctx->altPrivateKey->length); - } FreeDer(&ctx->altPrivateKey); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ctx->altPrivateKeyMask); @@ -8282,18 +8279,22 @@ static int SetSSL_CTX_CheckVersion(WOLFSSL* ssl, WOLFSSL_CTX* ctx) #endif /* OPENSSL_EXTRA */ #ifndef NO_CERTS -/* Copy the certificate, certificate chain and private key buffers from the - * CTX into the SSL object. Returns 0 on success. */ -static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx) +/* Give the SSL object the certificate, certificate chain and private key of + * the CTX. The SSL must not hold any of its own yet. + * + * With WOLFSSL_COPY_CERT and WOLFSSL_COPY_KEY the SSL keeps what it is given + * even if the CTX replaces or frees them later, as with OpenSSL: it holds a + * reference to the CTX's buffer (WOLFSSL_DER_REFCOUNT) or a copy, and owns + * that. Otherwise it points at the CTX's buffers and owns nothing. + * + * Returns 0 on success. */ +int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx) { int ret = 0; #ifdef WOLFSSL_COPY_CERT - /* If WOLFSSL_COPY_CERT is defined, always copy the cert */ if (ctx->certificate != NULL) { - ret = AllocCopyDer(&ssl->buffers.certificate, ctx->certificate->buffer, - ctx->certificate->length, ctx->certificate->type, - ctx->certificate->heap); + ret = ShareDer(&ssl->buffers.certificate, ctx->certificate); if (ret != 0) { return ret; } @@ -8301,9 +8302,7 @@ static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx) ssl->buffers.weOwnCert = 1; } if (ctx->certChain != NULL) { - ret = AllocCopyDer(&ssl->buffers.certChain, ctx->certChain->buffer, - ctx->certChain->length, ctx->certChain->type, - ctx->certChain->heap); + ret = ShareDer(&ssl->buffers.certChain, ctx->certChain); if (ret != 0) { return ret; } @@ -8319,12 +8318,10 @@ static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx) #ifndef WOLFSSL_BLIND_PRIVATE_KEY #ifdef WOLFSSL_COPY_KEY if (ctx->privateKey != NULL) { - if (ssl->buffers.key != NULL) { + if (ssl->buffers.weOwnKey) { FreeDer(&ssl->buffers.key); } - ret = AllocCopyDer(&ssl->buffers.key, ctx->privateKey->buffer, - ctx->privateKey->length, ctx->privateKey->type, - ctx->privateKey->heap); + ret = ShareDer(&ssl->buffers.key, ctx->privateKey); if (ret != 0) { return ret; } diff --git a/src/ssl.c b/src/ssl.c index 3773badb1d9..562e90faadb 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -225,8 +225,10 @@ static struct SystemCryptoPolicy crypto_policy; * WOLFSSL_CALLBACKS: Handshake monitoring callbacks default: off * NO_HANDSHAKE_DONE_CB: Disable handshake completion callback default: off * WOLFSSL_SHUTDOWNONCE: Send close_notify only once default: off - * WOLFSSL_COPY_CERT: Copy certificate buffer (own copy) default: off - * WOLFSSL_COPY_KEY: Copy private key buffer (own copy) default: off + * WOLFSSL_COPY_CERT: SSL keeps CTX cert if CTX replaces it default: off + * (on with OPENSSL_ALL) + * WOLFSSL_COPY_KEY: SSL keeps CTX key if CTX replaces it default: off + * (on with OPENSSL_ALL) * WOLF_PRIVATE_KEY_ID: Reference private keys by ID default: off * WOLFSSL_REFCNT_ERROR_RETURN: * Return errors on ref counting failures default: off @@ -9070,74 +9072,10 @@ WOLFSSL_CTX* wolfSSL_set_SSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx) /* Release the certificate, chain and keys this SSL owns so that a ctx * without them leaves none of the old ones behind. */ wolfSSL_certs_clear(ssl); -#ifdef WOLFSSL_COPY_CERT - /* If WOLFSSL_COPY_CERT defined, always make new copy of cert from ctx */ - if (ctx->certificate != NULL) { - ret = AllocCopyDer(&ssl->buffers.certificate, ctx->certificate->buffer, - ctx->certificate->length, ctx->certificate->type, - ctx->certificate->heap); - if (ret != 0) { - return NULL; - } - - ssl->buffers.weOwnCert = 1; - } - if (ctx->certChain != NULL) { - ret = AllocCopyDer(&ssl->buffers.certChain, ctx->certChain->buffer, - ctx->certChain->length, ctx->certChain->type, - ctx->certChain->heap); - if (ret != 0) { - return NULL; - } - - ssl->buffers.weOwnCertChain = 1; - } -#else - /* ctx owns certificate, certChain and key */ - ssl->buffers.certificate = ctx->certificate; - ssl->buffers.certChain = ctx->certChain; -#endif - ssl->buffers.certChainCnt = ctx->certChainCnt; -#ifndef WOLFSSL_BLIND_PRIVATE_KEY -#ifdef WOLFSSL_COPY_KEY - if (ctx->privateKey != NULL) { - ret = AllocCopyDer(&ssl->buffers.key, ctx->privateKey->buffer, - ctx->privateKey->length, ctx->privateKey->type, - ctx->privateKey->heap); - if (ret != 0) { - return NULL; - } - ssl->buffers.weOwnKey = 1; - } - else { - ssl->buffers.key = ctx->privateKey; - } -#else - ssl->buffers.key = ctx->privateKey; -#endif -#else - if (ctx->privateKey != NULL) { - ret = AllocCopyDer(&ssl->buffers.key, ctx->privateKey->buffer, - ctx->privateKey->length, ctx->privateKey->type, - ctx->privateKey->heap); - if (ret != 0) { - return NULL; - } - ssl->buffers.weOwnKey = 1; - /* Blind the private key for the SSL with new random mask. */ - wolfssl_priv_der_blind_toggle(ssl->buffers.key, ctx->privateKeyMask); - ret = wolfssl_priv_der_blind(ssl->rng, ssl->buffers.key, - &ssl->buffers.keyMask); - if (ret != 0) { - return NULL; - } + /* Same certificate, chain and keys as an SSL made from this ctx. */ + if (SetSSL_CTX_CertsAndKeys(ssl, ctx) != 0) { + return NULL; } -#endif - ssl->buffers.keyType = ctx->privateKeyType; - ssl->buffers.keyId = ctx->privateKeyId; - ssl->buffers.keyLabel = ctx->privateKeyLabel; - ssl->buffers.keySz = ctx->privateKeySz; - ssl->buffers.keyDevId = ctx->privateKeyDevId; /* flags indicating what certs/keys are available */ ssl->options.haveRSA = ctx->haveRSA; ssl->options.haveDH = ctx->haveDH; @@ -9147,31 +9085,6 @@ WOLFSSL_CTX* wolfSSL_set_SSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx) ssl->options.haveFalconSig = ctx->haveFalconSig; ssl->options.haveMlDsaSig = ctx->haveMlDsaSig; ssl->options.haveSlhDsaSig = ctx->haveSlhDsaSig; -#ifdef WOLFSSL_DUAL_ALG_CERTS -#ifndef WOLFSSL_BLIND_PRIVATE_KEY - ssl->buffers.altKey = ctx->altPrivateKey; -#else - if (ctx->altPrivateKey != NULL) { - ret = AllocCopyDer(&ssl->buffers.altKey, ctx->altPrivateKey->buffer, - ctx->altPrivateKey->length, ctx->altPrivateKey->type, - ctx->altPrivateKey->heap); - if (ret != 0) { - return NULL; - } - ssl->buffers.weOwnAltKey = 1; - /* Blind the private key for the SSL with new random mask. */ - wolfssl_priv_der_blind_toggle(ssl->buffers.altKey, - ctx->altPrivateKeyMask); - ret = wolfssl_priv_der_blind(ssl->rng, ssl->buffers.altKey, - &ssl->buffers.altKeyMask); - if (ret != 0) { - return NULL; - } - } -#endif - ssl->buffers.altKeySz = ctx->altPrivateKeySz; - ssl->buffers.altKeyType = ctx->altPrivateKeyType; -#endif /* WOLFSSL_DUAL_ALG_CERTS */ #endif #ifdef WOLFSSL_SESSION_ID_CTX diff --git a/src/ssl_api_cert.c b/src/ssl_api_cert.c index 15781db1732..4b6f55055bf 100644 --- a/src/ssl_api_cert.c +++ b/src/ssl_api_cert.c @@ -1126,12 +1126,11 @@ int wolfSSL_UnloadCertsKeys(WOLFSSL* ssl) ssl->buffers.weOwnCertChain = 0; } + /* FreeDer() zeroes a private key when the last holder lets go of it. + * The key can be shared with the CTX (WOLFSSL_DER_REFCOUNT), so it + * must not be zeroed here. */ if (ssl->buffers.weOwnKey) { WOLFSSL_MSG("Unloading key"); - if ((ssl->buffers.key != NULL) && - (ssl->buffers.key->buffer != NULL)) { - ForceZero(ssl->buffers.key->buffer, ssl->buffers.key->length); - } FreeDer(&ssl->buffers.key); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ssl->buffers.keyMask); @@ -1142,11 +1141,6 @@ int wolfSSL_UnloadCertsKeys(WOLFSSL* ssl) #ifdef WOLFSSL_DUAL_ALG_CERTS if (ssl->buffers.weOwnAltKey) { WOLFSSL_MSG("Unloading alt key"); - if ((ssl->buffers.altKey != NULL) && - (ssl->buffers.altKey->buffer != NULL)) { - ForceZero(ssl->buffers.altKey->buffer, - ssl->buffers.altKey->length); - } FreeDer(&ssl->buffers.altKey); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ssl->buffers.altKeyMask); @@ -2114,10 +2108,13 @@ WOLFSSL_X509* wolfSSL_get_certificate(WOLFSSL* ssl) * applications (like nginx OCSP stapling) that use the X509 pointer * from SSL_CTX_use_certificate as a lookup key. */ if ((ssl->ctx != NULL) && (ssl->ctx->ourCert != NULL)) { - /* Compare cert buffers to make sure they are the same */ + /* Compare cert buffers to make sure they are the same. The + * SSL may hold the CTX's own buffer (WOLFSSL_DER_REFCOUNT). */ if ((ssl->buffers.certificate == NULL) || (ssl->buffers.certificate->buffer == NULL) || - ((ssl->buffers.certificate->length == + (ssl->buffers.certificate == ssl->ctx->certificate) || + ((ssl->ctx->certificate != NULL) && + (ssl->buffers.certificate->length == ssl->ctx->certificate->length) && (XMEMCMP(ssl->buffers.certificate->buffer, ssl->ctx->certificate->buffer, diff --git a/tests/api.c b/tests/api.c index fada6a18c44..a2c411b4b6f 100644 --- a/tests/api.c +++ b/tests/api.c @@ -35542,11 +35542,22 @@ static int test_wolfSSL_set_SSL_CTX(void) ExpectIntEQ(XMEMCMP(ssl->sessionCtx, session_id2, 4), 0); #endif #ifdef WOLFSSL_COPY_CERT + /* The SSL holds its own reference to the CTX buffer, or its own copy. */ if (ctx2 != NULL && ctx2->certificate != NULL) { + #ifdef WOLFSSL_DER_REFCOUNT + ExpectTrue(ssl->buffers.certificate == ctx2->certificate); + #else ExpectFalse(ssl->buffers.certificate == ctx2->certificate); + #endif + ExpectIntEQ(ssl->buffers.weOwnCert, 1); } if (ctx2 != NULL && ctx2->certChain != NULL) { + #ifdef WOLFSSL_DER_REFCOUNT + ExpectTrue(ssl->buffers.certChain == ctx2->certChain); + #else ExpectFalse(ssl->buffers.certChain == ctx2->certChain); + #endif + ExpectIntEQ(ssl->buffers.weOwnCertChain, 1); } #else ExpectTrue(ssl->buffers.certificate == ctx2->certificate); @@ -35571,10 +35582,20 @@ static int test_wolfSSL_set_SSL_CTX(void) #ifdef WOLFSSL_INT_H #ifdef WOLFSSL_COPY_CERT if (ctx1 != NULL && ctx1->certificate != NULL) { + #ifdef WOLFSSL_DER_REFCOUNT + ExpectTrue(ssl->buffers.certificate == ctx1->certificate); + #else ExpectFalse(ssl->buffers.certificate == ctx1->certificate); + #endif + ExpectIntEQ(ssl->buffers.weOwnCert, 1); } if (ctx1 != NULL && ctx1->certChain != NULL) { + #ifdef WOLFSSL_DER_REFCOUNT + ExpectTrue(ssl->buffers.certChain == ctx1->certChain); + #else ExpectFalse(ssl->buffers.certChain == ctx1->certChain); + #endif + ExpectIntEQ(ssl->buffers.weOwnCertChain, 1); } #else ExpectTrue(ssl->buffers.certificate == ctx1->certificate); @@ -35584,9 +35605,19 @@ static int test_wolfSSL_set_SSL_CTX(void) ExpectIntGT(ctx1->certChainCnt, 0); ExpectIntEQ(ssl->buffers.certChainCnt, ctx1->certChainCnt); } -#if defined(WOLFSSL_COPY_KEY) || defined(WOLFSSL_BLIND_PRIVATE_KEY) +#if defined(WOLFSSL_BLIND_PRIVATE_KEY) + /* Blinded with its own mask, so always a copy. */ + if (ctx1 != NULL && ctx1->privateKey != NULL) { + ExpectFalse(ssl->buffers.key == ctx1->privateKey); + ExpectIntEQ(ssl->buffers.weOwnKey, 1); + } +#elif defined(WOLFSSL_COPY_KEY) if (ctx1 != NULL && ctx1->privateKey != NULL) { + #ifdef WOLFSSL_DER_REFCOUNT + ExpectTrue(ssl->buffers.key == ctx1->privateKey); + #else ExpectFalse(ssl->buffers.key == ctx1->privateKey); + #endif ExpectIntEQ(ssl->buffers.weOwnKey, 1); } #else @@ -35648,7 +35679,12 @@ static int test_wolfSSL_set_SSL_CTX(void) #ifdef WOLFSSL_INT_H if (ssl != NULL && ctx1 != NULL) { #ifdef WOLFSSL_COPY_CERT + /* Its own reference to the buffer of ctx1, or its own copy. */ + #ifdef WOLFSSL_DER_REFCOUNT + ExpectTrue(ssl->buffers.certificate == ctx1->certificate); + #else ExpectFalse(ssl->buffers.certificate == ctx1->certificate); + #endif ExpectIntEQ(ssl->buffers.weOwnCert, 1); #else ExpectTrue(ssl->buffers.certificate == ctx1->certificate); diff --git a/tests/api/test_ocsp.c b/tests/api/test_ocsp.c index d095633bfcb..b8edee08f9c 100644 --- a/tests/api/test_ocsp.c +++ b/tests/api/test_ocsp.c @@ -5980,10 +5980,10 @@ int test_dtls12_nonblock_ocsp(void) } #endif -/* WOLFSSL_COPY_CERT (implied by OPENSSL_ALL) gives every WOLFSSL its own copy of - * the CTX certificate, which sets ssl->buffers.weOwnCert and takes the CTX - * request cache out of play entirely - there is nothing to test in that - * configuration. */ +/* WOLFSSL_COPY_CERT (implied by OPENSSL_ALL) gives every WOLFSSL its own + * reference to, or copy of, the CTX certificate, which sets + * ssl->buffers.weOwnCert and takes the CTX request cache out of play entirely - + * there is nothing to test in that configuration. */ #if defined(HAVE_OCSP) && defined(HAVE_CERTIFICATE_STATUS_REQUEST) && \ defined(HAVE_SSL_MEMIO_TESTS_DEPENDENCIES) && \ !defined(WOLFSSL_NO_TLS12) && !defined(NO_WOLFSSL_SERVER) && \ diff --git a/tests/api/test_tls.c b/tests/api/test_tls.c index b8b8b29b46c..7a5a72554ca 100644 --- a/tests/api/test_tls.c +++ b/tests/api/test_tls.c @@ -6225,6 +6225,162 @@ int test_tls_ssl_ctx_certs_check(void) return EXPECT_RESULT(); } +#if defined(WOLFSSL_COPY_CERT) && defined(WOLFSSL_COPY_KEY) && \ + defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && !defined(NO_RSA) && \ + defined(HAVE_ECC) && !defined(NO_FILESYSTEM) && \ + defined(WOLFSSL_PEM_TO_DER) + #define TEST_SSL_KEEPS_CTX_CERTS +#endif + +#ifdef TEST_SSL_KEEPS_CTX_CERTS +/* Takes a copy of what "der" holds, to compare against later. */ +static int test_ssl_keeps_ctx_certs_save(const DerBuffer* der, byte** copy, + word32* copySz) +{ + EXPECT_DECLS; + + ExpectNotNull(der); + if (der != NULL) { + ExpectNotNull(*copy = (byte*)XMALLOC(der->length, NULL, + DYNAMIC_TYPE_TMP_BUFFER)); + if (*copy != NULL) { + XMEMCPY(*copy, der->buffer, der->length); + *copySz = der->length; + } + } + return EXPECT_RESULT(); +} + +/* Checks "der" still holds what was saved and frees the saved copy. */ +static int test_ssl_keeps_ctx_certs_check(const DerBuffer* der, byte* copy, + word32 copySz) +{ + EXPECT_DECLS; + + ExpectNotNull(der); + ExpectNotNull(copy); + if ((der != NULL) && (copy != NULL)) { + ExpectIntEQ(der->length, copySz); + if (der->length == copySz) { + ExpectBufEQ(der->buffer, copy, copySz); + } + } + XFREE(copy, NULL, DYNAMIC_TYPE_TMP_BUFFER); + return EXPECT_RESULT(); +} + +static int test_ssl_keeps_ctx_certs_one(method_provider client, + method_provider server) +{ + EXPECT_DECLS; + WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL; + WOLFSSL *ssl_c = NULL, *ssl_s = NULL; + struct test_memio_ctx test_ctx; + byte* cert = NULL; + byte* chain = NULL; + byte* key = NULL; + word32 certSz = 0; + word32 chainSz = 0; + word32 keySz = 0; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, NULL, NULL, + client, server), 0); + /* RSA leaf with its CA as the chain, so the SSL has all three to keep. */ + ExpectIntEQ(wolfSSL_CTX_use_certificate_chain_file(ctx_s, + "./certs/server-cert.pem"), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_use_PrivateKey_file(ctx_s, + "./certs/server-key.pem", WOLFSSL_FILETYPE_PEM), WOLFSSL_SUCCESS); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + client, server), 0); + ExpectNotNull(ssl_s); + + if (ssl_s != NULL) { + #ifdef WOLFSSL_DER_REFCOUNT + /* Shared with the CTX until the CTX lets go of them. */ + ExpectTrue(ssl_s->buffers.certificate == ctx_s->certificate); + ExpectTrue(ssl_s->buffers.certChain == ctx_s->certChain); + #ifndef WOLFSSL_BLIND_PRIVATE_KEY + ExpectTrue(ssl_s->buffers.key == ctx_s->privateKey); + #endif + #endif + ExpectIntEQ(test_ssl_keeps_ctx_certs_save(ssl_s->buffers.certificate, + &cert, &certSz), TEST_SUCCESS); + ExpectIntEQ(test_ssl_keeps_ctx_certs_save(ssl_s->buffers.certChain, + &chain, &chainSz), TEST_SUCCESS); + ExpectIntEQ(test_ssl_keeps_ctx_certs_save(ssl_s->buffers.key, + &key, &keySz), TEST_SUCCESS); + } + /* Another SSL on the same CTX comes and goes. Letting go of what it held + * must leave the buffers it shared with ssl_s alone. */ + wolfSSL_free(wolfSSL_new(ctx_s)); + + /* The CTX moves to an ECC leaf, chain and key, none of which the client + * trusts, and the application then lets go of the CTX. The SSL still + * holds a reference to it. */ + ExpectIntEQ(wolfSSL_CTX_use_certificate_chain_file(ctx_s, + "./certs/intermediate/server-chain-ecc.pem"), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_use_PrivateKey_file(ctx_s, "./certs/ecc-key.pem", + WOLFSSL_FILETYPE_PEM), WOLFSSL_SUCCESS); + wolfSSL_CTX_free(ctx_s); + ctx_s = NULL; + + /* What the SSL holds was neither freed (ASan) nor wiped. */ + if (ssl_s != NULL) { + ExpectIntEQ(test_ssl_keeps_ctx_certs_check(ssl_s->buffers.certificate, + cert, certSz), TEST_SUCCESS); + ExpectIntEQ(test_ssl_keeps_ctx_certs_check(ssl_s->buffers.certChain, + chain, chainSz), TEST_SUCCESS); + ExpectIntEQ(test_ssl_keeps_ctx_certs_check(ssl_s->buffers.key, key, + keySz), TEST_SUCCESS); + } + else { + XFREE(cert, NULL, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(chain, NULL, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(key, NULL, DYNAMIC_TYPE_TMP_BUFFER); + } + + /* And it is what the handshake runs on: the client only trusts the RSA + * CA, and the server has to sign with the RSA key. */ + ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + + wolfSSL_free(ssl_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_c); + return EXPECT_RESULT(); +} +#endif /* TEST_SSL_KEEPS_CTX_CERTS */ + +/* With WOLFSSL_COPY_CERT and WOLFSSL_COPY_KEY an SSL keeps the certificate, + * chain and key its CTX had when the SSL was made, as OpenSSL does, whatever + * the CTX does with them afterwards. With WOLFSSL_DER_REFCOUNT the SSL holds + * the CTX's own buffers, so this also checks that the CTX replacing them and + * being freed neither frees nor wipes them under the SSL. */ +int test_tls_ssl_keeps_ctx_certs(void) +{ + EXPECT_DECLS; +#ifdef TEST_SSL_KEEPS_CTX_CERTS + static const struct { + method_provider client; + method_provider server; + } methods[] = { + #ifndef WOLFSSL_NO_TLS12 + { wolfTLSv1_2_client_method, wolfTLSv1_2_server_method }, + #endif + #ifdef WOLFSSL_TLS13 + { wolfTLSv1_3_client_method, wolfTLSv1_3_server_method }, + #endif + }; + size_t i; + + for (i = 0; i < XELEM_CNT(methods); i++) { + ExpectIntEQ(test_ssl_keeps_ctx_certs_one(methods[i].client, + methods[i].server), TEST_SUCCESS); + } +#endif + return EXPECT_RESULT(); +} + /* Regression test for the post-ALPN_Select PSK-head check. * When ALPN_Select runs before CheckPreSharedKeys (so the per-PSK * binding check has the negotiated ALPN available), TLSX_SetALPN diff --git a/tests/api/test_tls.h b/tests/api/test_tls.h index 0c5a30b1ef4..1a586aa559d 100644 --- a/tests/api/test_tls.h +++ b/tests/api/test_tls.h @@ -98,6 +98,7 @@ int test_tls12_declined_ticket_auth_not_reminted(void); int test_tls13_session_resumption_sni_mismatch(void); int test_tls_cb_ctx_cert_load(void); int test_tls_ssl_ctx_certs_check(void); +int test_tls_ssl_keeps_ctx_certs(void); int test_tls13_resumption_with_alpn(void); int test_tls12_session_id_resumption_alpn_mismatch(void); int test_tls12_old_hello_resumption_binding(void); @@ -214,6 +215,7 @@ int test_tls_hmac_inner_cid_decision_coverage(void); TEST_DECL_GROUP("tls", test_tls13_session_resumption_sni_mismatch), \ TEST_DECL_GROUP("tls", test_tls_cb_ctx_cert_load), \ TEST_DECL_GROUP("tls", test_tls_ssl_ctx_certs_check), \ + TEST_DECL_GROUP("tls", test_tls_ssl_keeps_ctx_certs), \ TEST_DECL_GROUP("tls", test_tls13_resumption_with_alpn), \ TEST_DECL_GROUP("tls", test_tls12_session_id_resumption_alpn_mismatch),\ TEST_DECL_GROUP("tls", test_tls12_old_hello_resumption_binding), \ diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 46090474893..8cb2969e49e 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -27206,6 +27206,9 @@ int AllocDer(DerBuffer** pDer, word32 length, int type, void* heap) if (pDer) { int dynType = 0; DerBuffer* der; + #ifdef WOLFSSL_DER_REFCOUNT + int refErr; + #endif /* Determine dynamic type */ switch (type) { @@ -27232,6 +27235,11 @@ int AllocDer(DerBuffer** pDer, word32 length, int type, void* heap) der->heap = heap; der->buffer = (byte*)der + sizeof(DerBuffer); der->length = length; + #ifdef WOLFSSL_DER_REFCOUNT + /* Cannot fail without a mutex, which WOLFSSL_DER_REFCOUNT excludes. */ + wolfSSL_RefInit(&der->ref, &refErr); + (void)refErr; + #endif ret = 0; /* Success */ } else { ret = BAD_FUNC_ARG; @@ -27250,10 +27258,53 @@ int AllocCopyDer(DerBuffer** pDer, const unsigned char* buff, word32 length, return ret; } +/* Makes *pDer a holder of "src" that is independent of where "src" is held: + * replacing or freeing the original holder's buffer does not affect it. + * + * With WOLFSSL_DER_REFCOUNT this takes a reference to "src" itself, so no + * holder may change the contents of a buffer once it can be shared - a holder + * that needs different contents replaces its buffer instead. Otherwise *pDer + * gets a copy of "src". Either way the caller gives it up with FreeDer(). + * + * Returns 0 on success, BAD_FUNC_ARG or MEMORY_E on failure. + */ +int ShareDer(DerBuffer** pDer, DerBuffer* src) +{ + int ret; + + if ((pDer == NULL) || (src == NULL)) { + return BAD_FUNC_ARG; + } +#ifdef WOLFSSL_DER_REFCOUNT + /* Cannot fail without a mutex, which WOLFSSL_DER_REFCOUNT excludes. */ + wolfSSL_RefInc(&src->ref, &ret); + if (ret == 0) { + *pDer = src; + } +#else + ret = AllocCopyDer(pDer, src->buffer, src->length, src->type, src->heap); +#endif + + return ret; +} + void FreeDer(DerBuffer** pDer) { if (pDer && *pDer) { DerBuffer* der = (DerBuffer*)*pDer; + #ifdef WOLFSSL_DER_REFCOUNT + int isZero = 0; + int err = 0; + + /* Only the last holder frees the buffer. */ + *pDer = NULL; + wolfSSL_RefDec(&der->ref, &isZero, &err); + (void)err; + if (!isZero) { + return; + } + wolfSSL_RefFree(&der->ref); + #endif /* ForceZero private keys */ if (((der->type == PRIVATEKEY_TYPE) || diff --git a/wolfssl/internal.h b/wolfssl/internal.h index 4539530734d..afe88843afa 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -5779,6 +5779,9 @@ typedef struct Buffers { * not reference counted, so a session must not point at the context's. */ WOLFSSL_LOCAL int CopySSL_CTX_DhParams(WOLFSSL* ssl, WOLFSSL_CTX* ctx); #endif +#ifndef NO_CERTS +WOLFSSL_LOCAL int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx); +#endif #if !defined(NO_CERTS) && \ (defined(HAVE_SNI) || defined(WOLFSSL_CERT_SETUP_CB)) #ifdef WOLFSSL_API_PREFIX_MAP diff --git a/wolfssl/wolfcrypt/asn.h b/wolfssl/wolfcrypt/asn.h index 29cd5d62759..4ab1309ae46 100644 --- a/wolfssl/wolfcrypt/asn.h +++ b/wolfssl/wolfcrypt/asn.h @@ -2944,6 +2944,7 @@ WOLFSSL_API int AllocDer(DerBuffer** der, word32 length, int type, void* heap); WOLFSSL_LOCAL int AllocCopyDer(DerBuffer** der, const unsigned char* buff, word32 length, int type, void* heap); +WOLFSSL_LOCAL int ShareDer(DerBuffer** der, DerBuffer* src); WOLFSSL_API void FreeDer(DerBuffer** der); #ifdef WOLFSSL_ASN_PARSE_KEYUSAGE diff --git a/wolfssl/wolfcrypt/asn_public.h b/wolfssl/wolfcrypt/asn_public.h index 63dc5426298..142e0656b2a 100644 --- a/wolfssl/wolfcrypt/asn_public.h +++ b/wolfssl/wolfcrypt/asn_public.h @@ -272,6 +272,17 @@ enum Ctc_Misc { WOLF_ENUM_DUMMY_LAST_ELEMENT(Ctc_Misc) }; +/* DER buffers are reference counted when each SSL object has to keep the + * certificate and key it was made with even if its CTX replaces them + * (WOLFSSL_COPY_CERT, WOLFSSL_COPY_KEY). The SSL then holds a reference to the + * CTX's buffer instead of a copy of it. The count needs atomic operations or a + * single-threaded build; without either, each SSL gets a copy. */ +#if (defined(WOLFSSL_COPY_CERT) || defined(WOLFSSL_COPY_KEY)) && \ + (defined(SINGLE_THREADED) || defined(WOLFSSL_ATOMIC_OPS)) && \ + !defined(NO_CERTS) + #define WOLFSSL_DER_REFCOUNT +#endif + /* DER buffer */ typedef struct DerBuffer { byte* buffer; @@ -279,6 +290,9 @@ typedef struct DerBuffer { word32 length; int type; /* enum CertType */ int dynType; /* DYNAMIC_TYPE_* */ +#ifdef WOLFSSL_DER_REFCOUNT + wolfSSL_Ref ref; /* holders; FreeDer() frees on the last one */ +#endif } DerBuffer; typedef struct WOLFSSL_ASN1_TIME { diff --git a/wolfssl/wolfcrypt/settings.h b/wolfssl/wolfcrypt/settings.h index 6564f4aa52a..0b2733e051c 100644 --- a/wolfssl/wolfcrypt/settings.h +++ b/wolfssl/wolfcrypt/settings.h @@ -5235,17 +5235,21 @@ blinding by defining WC_BLINDING_NO_RNG_ACKNOWLEDGE_WEAKNESS." #undef WOLFSSL_DYNAMIC_PEER_CERT #endif -/* Always copy certificate(s) from SSL CTX to each SSL object on creation, - * if this is not defined then each SSL object shares a pointer to the - * original certificate buffer owned by the SSL CTX. */ +/* Each SSL object keeps the certificate(s) of its SSL CTX as they were when it + * was created, even if the CTX replaces or frees them later, as OpenSSL does. + * The SSL holds a reference to the CTX's buffer, or a copy of it where DER + * buffers cannot be reference counted (see WOLFSSL_DER_REFCOUNT). If this is + * not defined then each SSL object shares a pointer to the original + * certificate buffer owned by the SSL CTX, which must then not be replaced + * while SSL objects made from it exist. */ #if defined(OPENSSL_ALL) && !defined(WOLFSSL_NO_COPY_CERT) #undef WOLFSSL_COPY_CERT #define WOLFSSL_COPY_CERT #endif -/* Always copy private key from SSL CTX to each SSL object on creation, - * if this is not defined then each SSL object shares a pointer to the - * original key buffer owned by the SSL CTX. */ +/* As WOLFSSL_COPY_CERT, for the private key. With WOLFSSL_BLIND_PRIVATE_KEY + * each SSL object always gets its own copy of the key, blinded with its own + * mask. */ #if defined(OPENSSL_ALL) && !defined(WOLFSSL_NO_COPY_KEY) #undef WOLFSSL_COPY_KEY #define WOLFSSL_COPY_KEY From 38d769324c8486a2d26e1828aff05718a124f86f Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Thu, 5 Nov 2026 18:50:20 +0000 Subject: [PATCH 3/7] Allocate the fake chains in the chain tests with AllocDer() test_wolfSSL_CTX_add_extra_chain_cert_counts() and test_wolfSSL_add_to_chain_overflow() built a DerBuffer by hand, which left its new reference count at zero. With WOLFSSL_DER_REFCOUNT, freeing the CTX then took the count below zero without freeing the buffer, and LeakSanitizer reported it. Both tests now allocate the buffer with AllocDer(), and replace and free chains with FreeDer(). Also rename the refErr local in AllocDer() to err, which codespell flagged. --- tests/api.c | 32 ++++++++++---------------------- wolfcrypt/src/asn.c | 6 +++--- 2 files changed, 13 insertions(+), 25 deletions(-) diff --git a/tests/api.c b/tests/api.c index a2c411b4b6f..1436504e900 100644 --- a/tests/api.c +++ b/tests/api.c @@ -5160,24 +5160,18 @@ static int test_wolfSSL_CTX_add_extra_chain_cert_counts(void) /* A rejected append must leave the count alone. Plant a chain long enough * that appending overflows the size calculation. */ if (EXPECT_SUCCESS()) { - fakeChain = (DerBuffer*)XMALLOC(sizeof(DerBuffer) + 16, ctx->heap, - DYNAMIC_TYPE_CERT); - ExpectNotNull(fakeChain); + /* AllocDer() sets up everything FreeDer() relies on, such as the + * reference count with WOLFSSL_DER_REFCOUNT. */ + ExpectIntEQ(AllocDer(&fakeChain, 16, CERT_TYPE, ctx->heap), 0); } if (EXPECT_SUCCESS()) { - XMEMSET(fakeChain, 0, sizeof(DerBuffer) + 16); - fakeChain->buffer = (byte*)(fakeChain + 1); fakeChain->length = WOLFSSL_MAX_32BIT - 2; - fakeChain->type = CERT_TYPE; - fakeChain->dynType = DYNAMIC_TYPE_CERT; /* Replace the real chain with the fake one. */ - if (ctx->certChain != NULL) { - XFREE(ctx->certChain, ctx->heap, DYNAMIC_TYPE_CERT); - } + FreeDer(&ctx->certChain); ctx->certChain = fakeChain; } else { - XFREE(fakeChain, (ctx != NULL) ? ctx->heap : NULL, DYNAMIC_TYPE_CERT); + FreeDer(&fakeChain); } ExpectNotNull(x509 = wolfSSL_X509_load_certificate_file( @@ -5360,24 +5354,18 @@ static int test_wolfSSL_add_to_chain_overflow(void) /* Now ctx->certificate is set, next add goes to certChain via * wolfssl_add_to_chain. Fake a chain whose length is near UINT32_MAX * so the size calculation (len + CERT_HEADER_SZ + certSz) overflows. */ - fakeChain = (DerBuffer*)XMALLOC(sizeof(DerBuffer) + 16, ctx->heap, - DYNAMIC_TYPE_CERT); - ExpectNotNull(fakeChain); + /* AllocDer() sets up everything FreeDer() relies on, such as the + * reference count with WOLFSSL_DER_REFCOUNT. */ + ExpectIntEQ(AllocDer(&fakeChain, 16, CERT_TYPE, ctx->heap), 0); } if (EXPECT_SUCCESS()) { - XMEMSET(fakeChain, 0, sizeof(DerBuffer) + 16); - fakeChain->buffer = (byte*)(fakeChain + 1); fakeChain->length = WOLFSSL_MAX_32BIT - 2; /* will overflow with any cert */ - fakeChain->type = CERT_TYPE; - fakeChain->dynType = DYNAMIC_TYPE_CERT; /* Replace the real chain with our fake one. */ - if (ctx->certChain != NULL) { - XFREE(ctx->certChain, ctx->heap, DYNAMIC_TYPE_CERT); - } + FreeDer(&ctx->certChain); ctx->certChain = fakeChain; } else { - XFREE(fakeChain, ctx ? ctx->heap : NULL, DYNAMIC_TYPE_CERT); + FreeDer(&fakeChain); } /* Try to add another cert - this MUST fail due to overflow guard. */ diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 8cb2969e49e..0237d79490c 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -27207,7 +27207,7 @@ int AllocDer(DerBuffer** pDer, word32 length, int type, void* heap) int dynType = 0; DerBuffer* der; #ifdef WOLFSSL_DER_REFCOUNT - int refErr; + int err; #endif /* Determine dynamic type */ @@ -27237,8 +27237,8 @@ int AllocDer(DerBuffer** pDer, word32 length, int type, void* heap) der->length = length; #ifdef WOLFSSL_DER_REFCOUNT /* Cannot fail without a mutex, which WOLFSSL_DER_REFCOUNT excludes. */ - wolfSSL_RefInit(&der->ref, &refErr); - (void)refErr; + wolfSSL_RefInit(&der->ref, &err); + (void)err; #endif ret = 0; /* Success */ } else { From 857276eb2c13efac094f5d9d0902032e0a1ea203 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Thu, 5 Nov 2026 19:22:10 +0000 Subject: [PATCH 4/7] Zero a key slot's buffer whatever its type at the last release SSL_CtxResourceFree() and wolfSSL_UnloadCertsKeys() zeroed the key buffer before FreeDer() whatever its type. Sharing the CTX keys by reference count dropped that, as it would wipe a key other holders still use, and left the zeroing to FreeDer(), which only zeroes PRIVATEKEY_TYPE and ALT_PRIVATEKEY_TYPE buffers. A key slot can also hold a public key when the private key is on a device, which test_wc_CryptoCb() does, and WOLFSSL_CHECK_MEM_ZERO caught that buffer being freed without being zeroed. FreeKeyDer() frees like FreeDer() but zeroes the buffer whatever its type when the last holder lets go. Every release of ctx->privateKey, ctx->altPrivateKey, ssl->buffers.key and ssl->buffers.altKey now uses it, as with shared buffers any of those can be the last holder. --- src/internal.c | 14 +++++++------- src/ssl.c | 4 ++-- src/ssl_api_cert.c | 10 +++++----- src/ssl_load.c | 24 ++++++++++++------------ src/tls13.c | 2 +- wolfcrypt/src/asn.c | 20 ++++++++++++++++++-- wolfssl/wolfcrypt/asn.h | 1 + 7 files changed, 46 insertions(+), 29 deletions(-) diff --git a/src/internal.c b/src/internal.c index 3b8987568be..a8efbec1167 100644 --- a/src/internal.c +++ b/src/internal.c @@ -3563,15 +3563,15 @@ void SSL_CtxResourceFree(WOLFSSL_CTX* ctx) #endif /* SINGLE_THREADED */ #ifndef NO_CERTS - /* FreeDer() zeroes a private key when the last holder lets go of it. An - * SSL made from this CTX can still hold it (WOLFSSL_DER_REFCOUNT), so it - * must not be zeroed here. */ - FreeDer(&ctx->privateKey); + /* FreeKeyDer() zeroes the key when the last holder lets go of it. An SSL + * made from this CTX can still hold it (WOLFSSL_DER_REFCOUNT), so it must + * not be zeroed here. */ + FreeKeyDer(&ctx->privateKey); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ctx->privateKeyMask); #endif #ifdef WOLFSSL_DUAL_ALG_CERTS - FreeDer(&ctx->altPrivateKey); + FreeKeyDer(&ctx->altPrivateKey); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ctx->altPrivateKeyMask); #endif @@ -8319,7 +8319,7 @@ int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx) #ifdef WOLFSSL_COPY_KEY if (ctx->privateKey != NULL) { if (ssl->buffers.weOwnKey) { - FreeDer(&ssl->buffers.key); + FreeKeyDer(&ssl->buffers.key); } ret = ShareDer(&ssl->buffers.key, ctx->privateKey); if (ret != 0) { @@ -8336,7 +8336,7 @@ int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx) #else if (ctx->privateKey != NULL) { if (ssl->buffers.key != NULL) { - FreeDer(&ssl->buffers.key); + FreeKeyDer(&ssl->buffers.key); } ret = AllocCopyDer(&ssl->buffers.key, ctx->privateKey->buffer, ctx->privateKey->length, ctx->privateKey->type, diff --git a/src/ssl.c b/src/ssl.c index 562e90faadb..e7f6b2e79a7 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -8645,7 +8645,7 @@ void wolfSSL_certs_clear(WOLFSSL* ssl) } #endif if (ssl->buffers.weOwnKey) { - FreeDer(&ssl->buffers.key); + FreeKeyDer(&ssl->buffers.key); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ssl->buffers.keyMask); #endif @@ -8662,7 +8662,7 @@ void wolfSSL_certs_clear(WOLFSSL* ssl) ssl->buffers.keyDevId = 0; #ifdef WOLFSSL_DUAL_ALG_CERTS if (ssl->buffers.weOwnAltKey) { - FreeDer(&ssl->buffers.altKey); + FreeKeyDer(&ssl->buffers.altKey); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ssl->buffers.altKeyMask); #endif diff --git a/src/ssl_api_cert.c b/src/ssl_api_cert.c index 4b6f55055bf..948e4b37c6c 100644 --- a/src/ssl_api_cert.c +++ b/src/ssl_api_cert.c @@ -1126,12 +1126,12 @@ int wolfSSL_UnloadCertsKeys(WOLFSSL* ssl) ssl->buffers.weOwnCertChain = 0; } - /* FreeDer() zeroes a private key when the last holder lets go of it. - * The key can be shared with the CTX (WOLFSSL_DER_REFCOUNT), so it - * must not be zeroed here. */ + /* FreeKeyDer() zeroes the key when the last holder lets go of it. It + * can be shared with the CTX (WOLFSSL_DER_REFCOUNT), so it must not + * be zeroed here. */ if (ssl->buffers.weOwnKey) { WOLFSSL_MSG("Unloading key"); - FreeDer(&ssl->buffers.key); + FreeKeyDer(&ssl->buffers.key); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ssl->buffers.keyMask); #endif @@ -1141,7 +1141,7 @@ int wolfSSL_UnloadCertsKeys(WOLFSSL* ssl) #ifdef WOLFSSL_DUAL_ALG_CERTS if (ssl->buffers.weOwnAltKey) { WOLFSSL_MSG("Unloading alt key"); - FreeDer(&ssl->buffers.altKey); + FreeKeyDer(&ssl->buffers.altKey); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ssl->buffers.altKeyMask); #endif diff --git a/src/ssl_load.c b/src/ssl_load.c index b6b70626f08..011519b754f 100644 --- a/src/ssl_load.c +++ b/src/ssl_load.c @@ -1426,7 +1426,7 @@ static int ProcessBufferPrivKeyHandleDer(WOLFSSL_CTX* ctx, WOLFSSL* ssl, if (ssl != NULL) { /* Dispose of previous key if not context's. */ if (ssl->buffers.weOwnAltKey) { - FreeDer(&ssl->buffers.altKey); + FreeKeyDer(&ssl->buffers.altKey); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ssl->buffers.altKeyMask); #endif @@ -1443,7 +1443,7 @@ static int ProcessBufferPrivKeyHandleDer(WOLFSSL_CTX* ctx, WOLFSSL* ssl, } else if (ctx != NULL) { /* Dispose of previous key. */ - FreeDer(&ctx->altPrivateKey); + FreeKeyDer(&ctx->altPrivateKey); ctx->altPrivateKeyId = 0; ctx->altPrivateKeyLabel = 0; ctx->altPrivateKeyDevId = INVALID_DEVID; @@ -1459,7 +1459,7 @@ static int ProcessBufferPrivKeyHandleDer(WOLFSSL_CTX* ctx, WOLFSSL* ssl, if (ssl != NULL) { /* Dispose of previous key if not context's. */ if (ssl->buffers.weOwnKey) { - FreeDer(&ssl->buffers.key); + FreeKeyDer(&ssl->buffers.key); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ssl->buffers.keyMask); #endif @@ -1476,7 +1476,7 @@ static int ProcessBufferPrivKeyHandleDer(WOLFSSL_CTX* ctx, WOLFSSL* ssl, } else if (ctx != NULL) { /* Dispose of previous key. */ - FreeDer(&ctx->privateKey); + FreeKeyDer(&ctx->privateKey); ctx->privateKeyId = 0; ctx->privateKeyLabel = 0; ctx->privateKeyDevId = INVALID_DEVID; @@ -4578,7 +4578,7 @@ int wolfSSL_CTX_use_PrivateKey_Id(WOLFSSL_CTX* ctx, const unsigned char* id, } /* Dispose of old private key and allocate and copy in id. */ - FreeDer(&ctx->privateKey); + FreeKeyDer(&ctx->privateKey); if (AllocCopyDer(&ctx->privateKey, id, (word32)sz, PRIVATEKEY_TYPE, ctx->heap) != 0) { ret = 0; @@ -4656,7 +4656,7 @@ int wolfSSL_CTX_use_PrivateKey_Label(WOLFSSL_CTX* ctx, const char* label, sz = (word32)XSTRLEN(label) + 1; /* Dispose of old private key and allocate and copy in label. */ - FreeDer(&ctx->privateKey); + FreeKeyDer(&ctx->privateKey); if (AllocCopyDer(&ctx->privateKey, (const byte*)label, (word32)sz, PRIVATEKEY_TYPE, ctx->heap) != 0) { ret = 0; @@ -4697,7 +4697,7 @@ int wolfSSL_CTX_use_AltPrivateKey_Id(WOLFSSL_CTX* ctx, const unsigned char* id, } if (ret == 1) { - FreeDer(&ctx->altPrivateKey); + FreeKeyDer(&ctx->altPrivateKey); if (AllocDer(&ctx->altPrivateKey, (word32)sz, ALT_PRIVATEKEY_TYPE, ctx->heap) != 0) { ret = 0; @@ -4748,7 +4748,7 @@ int wolfSSL_CTX_use_AltPrivateKey_Label(WOLFSSL_CTX* ctx, const char* label, if (ret == 1) { sz = (word32)XSTRLEN(label) + 1; - FreeDer(&ctx->altPrivateKey); + FreeKeyDer(&ctx->altPrivateKey); if (AllocDer(&ctx->altPrivateKey, (word32)sz, ALT_PRIVATEKEY_TYPE, ctx->heap) != 0) { ret = 0; @@ -4991,7 +4991,7 @@ int wolfSSL_use_PrivateKey_Id(WOLFSSL* ssl, const unsigned char* id, /* Dispose of old private key if owned and allocate and copy in id. */ if (ssl->buffers.weOwnKey) { - FreeDer(&ssl->buffers.key); + FreeKeyDer(&ssl->buffers.key); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ssl->buffers.keyMask); #endif @@ -5067,7 +5067,7 @@ int wolfSSL_use_PrivateKey_Label(WOLFSSL* ssl, const char* label, int devId) /* Dispose of old private key if owned and allocate and copy in label. */ if (ssl->buffers.weOwnKey) { - FreeDer(&ssl->buffers.key); + FreeKeyDer(&ssl->buffers.key); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ssl->buffers.keyMask); #endif @@ -5112,7 +5112,7 @@ int wolfSSL_use_AltPrivateKey_Id(WOLFSSL* ssl, const unsigned char* id, long sz, if (ret == 1) { if (ssl->buffers.weOwnAltKey) { - FreeDer(&ssl->buffers.altKey); + FreeKeyDer(&ssl->buffers.altKey); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ssl->buffers.altKeyMask); #endif @@ -5160,7 +5160,7 @@ int wolfSSL_use_AltPrivateKey_Label(WOLFSSL* ssl, const char* label, int devId) if (ret == 1) { sz = (word32)XSTRLEN(label) + 1; if (ssl->buffers.weOwnAltKey) { - FreeDer(&ssl->buffers.altKey); + FreeKeyDer(&ssl->buffers.altKey); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ssl->buffers.altKeyMask); #endif diff --git a/src/tls13.c b/src/tls13.c index 68154208bcf..789fd263b9d 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -11460,7 +11460,7 @@ static int SendTls13CertificateVerify(WOLFSSL* ssl) ssl->buffers.keySz = ssl->buffers.altKeySz; /* If we own it, free key before overriding it. */ if (ssl->buffers.weOwnKey) { - FreeDer(&ssl->buffers.key); + FreeKeyDer(&ssl->buffers.key); #ifdef WOLFSSL_BLIND_PRIVATE_KEY FreeDer(&ssl->buffers.keyMask); #endif diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 0237d79490c..e51b0d56b32 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -27288,7 +27288,9 @@ int ShareDer(DerBuffer** pDer, DerBuffer* src) return ret; } -void FreeDer(DerBuffer** pDer) +/* Gives up *pDer and sets it to NULL. The buffer is freed, and zeroed first if + * "zero" is set or it is a private key, when this was its last holder. */ +static void FreeDerEx(DerBuffer** pDer, int zero) { if (pDer && *pDer) { DerBuffer* der = (DerBuffer*)*pDer; @@ -27307,7 +27309,7 @@ void FreeDer(DerBuffer** pDer) #endif /* ForceZero private keys */ - if (((der->type == PRIVATEKEY_TYPE) || + if ((zero || (der->type == PRIVATEKEY_TYPE) || (der->type == ALT_PRIVATEKEY_TYPE)) && der->buffer != NULL) { ForceZero(der->buffer, der->length); } @@ -27319,6 +27321,20 @@ void FreeDer(DerBuffer** pDer) } } +void FreeDer(DerBuffer** pDer) +{ + FreeDerEx(pDer, 0); +} + +/* As FreeDer(), for a buffer held as a private key: it is zeroed whatever its + * type, as it can hold a public key, or a key id or label, when the private key + * is on a device. With WOLFSSL_DER_REFCOUNT that happens when the last holder + * lets go, so a holder must not zero it before calling this. */ +void FreeKeyDer(DerBuffer** pDer) +{ + FreeDerEx(pDer, 1); +} + #ifndef WOLFSSL_API_PREFIX_MAP int wc_AllocDer(DerBuffer** pDer, word32 length, int type, void* heap) { diff --git a/wolfssl/wolfcrypt/asn.h b/wolfssl/wolfcrypt/asn.h index 4ab1309ae46..199eb221784 100644 --- a/wolfssl/wolfcrypt/asn.h +++ b/wolfssl/wolfcrypt/asn.h @@ -2946,6 +2946,7 @@ WOLFSSL_LOCAL int AllocCopyDer(DerBuffer** der, const unsigned char* buff, word32 length, int type, void* heap); WOLFSSL_LOCAL int ShareDer(DerBuffer** der, DerBuffer* src); WOLFSSL_API void FreeDer(DerBuffer** der); +WOLFSSL_LOCAL void FreeKeyDer(DerBuffer** der); #ifdef WOLFSSL_ASN_PARSE_KEYUSAGE WOLFSSL_LOCAL int ParseKeyUsageStr(const char* value, word16* keyUsage, From b7cd776bf533c1209e5fa9a09b7fda470a137392 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Thu, 5 Nov 2026 19:31:56 +0000 Subject: [PATCH 5/7] Free the saved buffers in test_tls_ssl_keeps_ctx_certs() on failure The check helper freed the saved copies of the certificate, chain and key, but it runs inside ExpectIntEQ(), which skips it once an earlier expectation has failed, so a failing run leaked them. The helper now only compares, and the test frees the copies at the end. --- tests/api/test_tls.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tests/api/test_tls.c b/tests/api/test_tls.c index 7a5a72554ca..77a969b7640 100644 --- a/tests/api/test_tls.c +++ b/tests/api/test_tls.c @@ -6251,9 +6251,9 @@ static int test_ssl_keeps_ctx_certs_save(const DerBuffer* der, byte** copy, return EXPECT_RESULT(); } -/* Checks "der" still holds what was saved and frees the saved copy. */ -static int test_ssl_keeps_ctx_certs_check(const DerBuffer* der, byte* copy, - word32 copySz) +/* Checks "der" still holds what was saved. */ +static int test_ssl_keeps_ctx_certs_check(const DerBuffer* der, + const byte* copy, word32 copySz) { EXPECT_DECLS; @@ -6265,7 +6265,6 @@ static int test_ssl_keeps_ctx_certs_check(const DerBuffer* der, byte* copy, ExpectBufEQ(der->buffer, copy, copySz); } } - XFREE(copy, NULL, DYNAMIC_TYPE_TMP_BUFFER); return EXPECT_RESULT(); } @@ -6334,16 +6333,15 @@ static int test_ssl_keeps_ctx_certs_one(method_provider client, ExpectIntEQ(test_ssl_keeps_ctx_certs_check(ssl_s->buffers.key, key, keySz), TEST_SUCCESS); } - else { - XFREE(cert, NULL, DYNAMIC_TYPE_TMP_BUFFER); - XFREE(chain, NULL, DYNAMIC_TYPE_TMP_BUFFER); - XFREE(key, NULL, DYNAMIC_TYPE_TMP_BUFFER); - } /* And it is what the handshake runs on: the client only trusts the RSA * CA, and the server has to sign with the RSA key. */ ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + /* Freed here, as the checks above are skipped once one fails. */ + XFREE(cert, NULL, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(chain, NULL, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(key, NULL, DYNAMIC_TYPE_TMP_BUFFER); wolfSSL_free(ssl_c); wolfSSL_free(ssl_s); wolfSSL_CTX_free(ctx_c); From f09206bfadd2533fed35a6c4888134602983fa17 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Thu, 5 Nov 2026 19:53:42 +0000 Subject: [PATCH 6/7] Fix the tests for the API guard check and the newer master tests check-api-guards.py requires !NO_FILESYSTEM and !NO_CERTS around calls to wolfSSL_CTX_use_PrivateKey_file() and wolfSSL_CTX_use_certificate_file(). The guards around the new OCSP cache test and test_tls_ssl_keeps_ctx_certs() only implied them, so they now name them. The wolfSSL_UnloadCertsKeys() coverage in test_ssl_cert.c built a key DerBuffer by hand, which left its reference count at zero, so with WOLFSSL_DER_REFCOUNT it was never freed. It now uses AllocDer(). test_internal_nullguard_whitebox.c passes the SSL's CTX to GetCtxOcspLock(), which now takes the CTX. --- tests/api/test_ocsp.c | 3 ++- tests/api/test_ssl_cert.c | 12 +++++------- tests/api/test_tls.c | 7 +++++-- tests/unit-mcdc/test_internal_nullguard_whitebox.c | 14 +++++++------- 4 files changed, 19 insertions(+), 17 deletions(-) diff --git a/tests/api/test_ocsp.c b/tests/api/test_ocsp.c index b8edee08f9c..5dbd379e91c 100644 --- a/tests/api/test_ocsp.c +++ b/tests/api/test_ocsp.c @@ -6103,7 +6103,8 @@ int test_ocsp_status_verify_cb_would_block(void) defined(HAVE_TLS_EXTENSIONS) && !defined(NO_WOLFSSL_SERVER) && \ defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \ !defined(WOLFSSL_NO_TLS12) && !defined(WOLFSSL_COPY_CERT) && \ - !defined(NO_FILESYSTEM) && !defined(NO_RSA) && !defined(NO_SHA) && \ + !defined(NO_FILESYSTEM) && !defined(NO_CERTS) && \ + !defined(NO_RSA) && !defined(NO_SHA) && \ !defined(NO_ASN_TIME) /* Number of times the server's OCSP IO callback has been called. */ diff --git a/tests/api/test_ssl_cert.c b/tests/api/test_ssl_cert.c index 4ff30806b8b..a90d0de6472 100644 --- a/tests/api/test_ssl_cert.c +++ b/tests/api/test_ssl_cert.c @@ -6350,14 +6350,12 @@ int test_ssl_cert_decision_coverage(void) ssl->buffers.key = NULL; (void)wolfSSL_UnloadCertsKeys(ssl); - key = (DerBuffer*)XMALLOC(sizeof(DerBuffer), ssl->heap, - DYNAMIC_TYPE_DER); - ExpectNotNull(key); + /* A key with no buffer. AllocDer() sets up everything FreeDer() + * relies on, such as the reference count with WOLFSSL_DER_REFCOUNT. */ + key = NULL; + ExpectIntEQ(AllocDer(&key, 0, PRIVATEKEY_TYPE, ssl->heap), 0); if (key != NULL) { - XMEMSET(key, 0, sizeof(DerBuffer)); - key->heap = ssl->heap; - key->type = PRIVATEKEY_TYPE; - key->dynType = DYNAMIC_TYPE_DER; + key->buffer = NULL; ssl->buffers.key = key; ssl->buffers.weOwnKey = 1; (void)wolfSSL_UnloadCertsKeys(ssl); diff --git a/tests/api/test_tls.c b/tests/api/test_tls.c index 77a969b7640..0a78d5a2151 100644 --- a/tests/api/test_tls.c +++ b/tests/api/test_tls.c @@ -6227,12 +6227,15 @@ int test_tls_ssl_ctx_certs_check(void) #if defined(WOLFSSL_COPY_CERT) && defined(WOLFSSL_COPY_KEY) && \ defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && !defined(NO_RSA) && \ - defined(HAVE_ECC) && !defined(NO_FILESYSTEM) && \ + defined(HAVE_ECC) && !defined(NO_FILESYSTEM) && !defined(NO_CERTS) && \ defined(WOLFSSL_PEM_TO_DER) #define TEST_SSL_KEEPS_CTX_CERTS #endif -#ifdef TEST_SSL_KEEPS_CTX_CERTS +/* The conditions are spelled out again for check-api-guards.py, which reads + * the guard around each call site. */ +#if defined(TEST_SSL_KEEPS_CTX_CERTS) && !defined(NO_FILESYSTEM) && \ + !defined(NO_CERTS) /* Takes a copy of what "der" holds, to compare against later. */ static int test_ssl_keeps_ctx_certs_save(const DerBuffer* der, byte** copy, word32* copySz) diff --git a/tests/unit-mcdc/test_internal_nullguard_whitebox.c b/tests/unit-mcdc/test_internal_nullguard_whitebox.c index 3ce3c55e638..ebc056e9e49 100644 --- a/tests/unit-mcdc/test_internal_nullguard_whitebox.c +++ b/tests/unit-mcdc/test_internal_nullguard_whitebox.c @@ -145,10 +145,10 @@ static void wb_supported_hash_sig_algo(WbFix* f) /* ---------------------------------------------------------- GetCtxOcspLock * - * `if (ssl->ctx->cm == NULL || ssl->ctx->cm->ocsp_stapling == NULL)`. On a - * real CTX both are non-NULL for the whole life of the object, so neither - * operand can flip from the API. Both are fields of driver-owned structs - * here. */ + * `if (ctx->cm == NULL || ctx->cm->ocsp_stapling == NULL)`, for the SSL's + * CTX. On a real CTX both are non-NULL for the whole life of the object, so + * neither operand can flip from the API. Both are fields of driver-owned + * structs here. */ #if (defined(HAVE_CERTIFICATE_STATUS_REQUEST) || \ defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2)) && !defined(WOLFSSL_NO_TLS12) #ifndef NO_WOLFSSL_SERVER @@ -164,18 +164,18 @@ static void wb_get_ctx_ocsp_lock(WbFix* f) /* operand 0: no cert manager on the CTX at all */ wb_reset(f); f->ctx->cm = NULL; - (void)GetCtxOcspLock(f->ssl); + (void)GetCtxOcspLock(f->ssl->ctx); g_calls++; /* operand 1: a cert manager, but no stapling responder on it */ wb_reset(f); f->cm->ocsp_stapling = NULL; - (void)GetCtxOcspLock(f->ssl); + (void)GetCtxOcspLock(f->ssl->ctx); g_calls++; /* false partner: both present, the lock address is returned */ f->cm->ocsp_stapling = ocsp; - (void)GetCtxOcspLock(f->ssl); + (void)GetCtxOcspLock(f->ssl->ctx); g_calls++; f->cm->ocsp_stapling = NULL; From 7fc95942aa8c8fd4368f8ddfc41a4e9f93b5f9c1 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Thu, 5 Nov 2026 20:28:09 +0000 Subject: [PATCH 7/7] Only use copies of the CTX OCSP request outside the lock GetCtxOcspRequest() read ctx->certOcspRequest under the CTX OCSP lock and handed the CTX's own object to the connection, which then checked it in CheckOcspRequest() without the lock. The connection that cached the request did the same with it after publishing it. That check can block on the responder, and it takes the same lock itself, so it cannot be done under the lock. With FreeCtxOcspRequestCache() dropping the cache when the CTX certificate is replaced, a reload during that check freed the request under the connection. The cache is now only touched under the lock. CreateOcspResponse() caches a copy of the request it built, allocated from the CTX heap that frees it, and GetCtxOcspRequest() returns a copy for the connection to free. Every request a caller holds is now its own, so the ctxOwnsRequest flag is gone. GetCtxOcspRequest() also skips the cache for a connection with a certificate of its own, which it was not built from. test_ocsp_ctx_request_cache_drop_in_use() drops the cache from inside the responder lookup of a handshake whose request came from it, as a certificate reload on another thread would, and FreeCtxOcspRequestCache() is now WOLFSSL_TEST_VIS for that. Without this change ASan reports a use after free in CompareOcspReqResp(). --- src/internal.c | 187 ++++++++++++++++++++++++++---------------- src/tls13.c | 11 +-- tests/api.c | 1 + tests/api/test_ocsp.c | 104 +++++++++++++++++++++++ tests/api/test_ocsp.h | 1 + wolfssl/internal.h | 7 +- 6 files changed, 232 insertions(+), 79 deletions(-) diff --git a/src/internal.c b/src/internal.c index a8efbec1167..a875db8351a 100644 --- a/src/internal.c +++ b/src/internal.c @@ -29614,11 +29614,11 @@ static wolfSSL_Mutex* GetCtxOcspLock(WOLFSSL_CTX* ctx) * responder about, and staple the status of, the certificate the CTX used to * have. The next connection that staples builds and caches a new one. * - * Only connections that share the CTX certificate use the cached request, and - * replacing that certificate frees the buffer they share, so none of them can - * still be using it here. The lock orders this against a connection publishing - * a request. When it cannot be taken the request is still dropped: keeping it - * would be wrong for every connection to come. + * Connections only ever use copies of the cached request, taken under the lock + * (see CopyCtxOcspRequest()), so freeing it cannot pull it out from under one + * that is still checking its request. When the lock cannot be taken the + * request is still dropped: keeping it would be wrong for every connection to + * come. */ void FreeCtxOcspRequestCache(WOLFSSL_CTX* ctx) { @@ -29683,45 +29683,91 @@ int CreateOcspRequest(WOLFSSL* ssl, OcspRequest* request, } +/* Copies a request made by InitOcspRequest() into a new allocation from + * "heap", which the caller frees with FreeOcspRequest() and XFREE(). + * + * The OCSP request cached on the CTX is only ever touched under + * GetCtxOcspLock(): it is stored as a copy and handed out as a copy. A + * connection then never holds the CTX's own request, which + * FreeCtxOcspRequestCache() can free at any time, while it checks the + * request. That check can block on the responder, so it cannot be done under + * the lock. + * + * Returns the copy, or NULL when it cannot be allocated. + */ +static OcspRequest* CopyCtxOcspRequest(const OcspRequest* src, void* heap) +{ + OcspRequest* dst; + + dst = (OcspRequest*)XMALLOC(sizeof(OcspRequest), heap, + DYNAMIC_TYPE_OCSP_REQUEST); + if (dst == NULL) { + return NULL; + } + XMEMCPY(dst, src, sizeof(OcspRequest)); + dst->heap = heap; + dst->serial = NULL; + dst->url = NULL; +#ifdef OPENSSL_EXTRA + /* Not set by InitOcspRequest(). */ + dst->serialInt = NULL; + dst->cid = NULL; +#endif + + if ((src->serial != NULL) && (src->serialSz > 0)) { + dst->serial = (byte*)XMALLOC((size_t)src->serialSz, heap, + DYNAMIC_TYPE_OCSP_REQUEST); + if (dst->serial == NULL) { + goto fail; + } + XMEMCPY(dst->serial, src->serial, (size_t)src->serialSz); + } + if ((src->url != NULL) && (src->urlSz > 0)) { + dst->url = (byte*)XMALLOC((size_t)src->urlSz + 1, heap, + DYNAMIC_TYPE_OCSP_REQUEST); + if (dst->url == NULL) { + goto fail; + } + XMEMCPY(dst->url, src->url, (size_t)src->urlSz); + dst->url[src->urlSz] = '\0'; + } + + return dst; + +fail: + FreeOcspRequest(dst); + XFREE(dst, heap, DYNAMIC_TYPE_OCSP_REQUEST); + return NULL; +} + /* Creates OCSP response and places it in variable "response". Memory * management for "buffer* response" is up to the caller. * * Also creates an OcspRequest in the case that ocspRequest is null or that - * ssl->buffers.weOwnCert is set. A newly created request may be cached on the - * CTX, which then owns it and frees it in wolfSSL_CTX_free(). "ctxOwnsRequest" - * carries that ownership: it tells this function whether the CTX already owns - * the request handed in, and on success it reports whether the CTX owns the - * request handed back. The caller may only free the request when the flag is - * clear. "*ocspRequest" and "*ctxOwnsRequest" are written together on success - * and both left untouched on failure, so the flag always describes the request - * the caller is actually holding. - * - * A request published to the CTX becomes shared with every other connection on - * it and is read without a lock, so it must be treated as immutable from that - * point on. See GetCtxOcspRequest(). + * ssl->buffers.weOwnCert is set, and returns it in "*ocspRequest" for the + * caller to free. A request created from the CTX certificate is also cached on + * the CTX, as a copy that only GetCtxOcspRequest() reads (see + * CopyCtxOcspRequest()). "*ocspRequest" is only written on success. * * Returns 0 on success */ int CreateOcspResponse(WOLFSSL* ssl, OcspRequest** ocspRequest, - buffer* response, byte* ctxOwnsRequest) + buffer* response) { int ret = 0; OcspRequest* request = NULL; byte createdRequest = 0; - byte ctxOwns = 0; /* Zero the output before any exit so that a caller freeing response->buffer * without checking the return code never acts on an indeterminate value. */ if (response != NULL) XMEMSET(response, 0, sizeof(*response)); - if (ssl == NULL || ocspRequest == NULL || response == NULL || - ctxOwnsRequest == NULL) { + if (ssl == NULL || ocspRequest == NULL || response == NULL) { return BAD_FUNC_ARG; } request = *ocspRequest; - ctxOwns = *ctxOwnsRequest; /* unable to fetch status. skip. The stapling object is checked here so that * every use of it below needs no further guarding. */ @@ -29746,40 +29792,50 @@ int CreateOcspResponse(WOLFSSL* ssl, OcspRequest** ocspRequest, ret = MEMORY_E; createdRequest = 1; - ctxOwns = 0; if (ret == 0) { ret = CreateOcspRequest(ssl, request, cert, der->buffer, der->length); } /* Only a request built from a CTX-held certificate may be cached, since - * the cache outlives this connection. + * the cache outlives this connection. The CTX keeps a copy, so this + * connection still owns, and frees, the request it built. * - * Not being able to cache is not an error. ctxOwns stays 0, so this - * connection keeps ownership of the request and frees it as it would - * any other one it built; all that is lost is the reuse by later - * connections on this CTX. Failing the handshake over a missed - * optimization would be the worse outcome. */ + * Not being able to cache is not an error: all that is lost is the + * reuse by later connections on this CTX. Failing the handshake over a + * missed optimization would be the worse outcome. */ if (ret == 0 && !ssl->buffers.weOwnCert) { wolfSSL_Mutex* ocspLock = GetCtxOcspLock(ssl->ctx); + OcspRequest* cached = NULL; /* SSL_CM(ssl) can resolve through ssl->x509_store_pt, so a stapling * object on the CTX manager is not implied by the one checked * above. */ + if (ocspLock != NULL) { + cached = CopyCtxOcspRequest(request, ssl->ctx->heap); + } if (ocspLock == NULL) { WOLFSSL_MSG("No CTX OCSP lock, not caching the request"); } + else if (cached == NULL) { + WOLFSSL_MSG("Couldn't copy the request, not caching it"); + } else if (wc_LockMutex(ocspLock) != 0) { WOLFSSL_MSG("Couldn't lock CTX OCSP mutex, not caching the " "request"); } else { if (ssl->ctx->certOcspRequest == NULL) { - ssl->ctx->certOcspRequest = request; - ctxOwns = 1; + ssl->ctx->certOcspRequest = cached; + cached = NULL; } wc_UnLockMutex(ocspLock); } + /* Another connection cached one first, or it was not cached. */ + if (cached != NULL) { + FreeOcspRequest(cached); + XFREE(cached, ssl->ctx->heap, DYNAMIC_TYPE_OCSP_REQUEST); + } } if (ret != 0) { @@ -29808,7 +29864,7 @@ int CreateOcspResponse(WOLFSSL* ssl, OcspRequest** ocspRequest, } /* free request up if error case found otherwise return it */ - if (ret != 0 && createdRequest && !ctxOwns) { + if (ret != 0 && createdRequest) { FreeOcspRequest(request); XFREE(request, ssl->heap, DYNAMIC_TYPE_OCSP_REQUEST); request = NULL; @@ -29816,12 +29872,9 @@ int CreateOcspResponse(WOLFSSL* ssl, OcspRequest** ocspRequest, exit_cor: - /* The request and the flag describing it are handed back as a pair, so the - * caller never decides ownership against a request it is not holding. On - * failure both of the caller's values are left as they were. */ + /* On failure the caller's request is left as it was. */ if (ret == 0) { *ocspRequest = request; - *ctxOwnsRequest = ctxOwns; } return ret; @@ -30623,36 +30676,36 @@ static int BuildCertificateStatus(WOLFSSL* ssl, byte type, buffer* status, #if defined(HAVE_CERTIFICATE_STATUS_REQUEST) || \ defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2) -/* Reads the OCSP request cached on the CTX. The field is shared by every - * connection on that CTX, so it is read once under GetCtxOcspLock() - the same - * lock the publishing side in CreateOcspResponse() takes - instead of being - * consulted again later. The CTX keeps ownership of what is returned, which - * "ctxOwnsRequest" reports to the caller. +/* Returns a copy of the OCSP request cached on the CTX, for the caller to free. + * The cached request is read under GetCtxOcspLock(), the lock it is published + * under in CreateOcspResponse() and dropped under in FreeCtxOcspRequestCache(), + * and the caller only ever uses its copy (see CopyCtxOcspRequest()). * - * The request handed back is shared with every other connection on the CTX and - * is read by all of them without holding a lock, so it must be treated as - * immutable: no field of it may be written once it has been published. Adding a - * field that the stapling path writes would reintroduce a data race here. + * The cache is built from the CTX certificate, so it is only used when the + * connection shares that certificate. * - * Returns the cached request, or NULL when there is none. + * Returns the copy, or NULL when there is no cached request, the connection has + * a certificate of its own, or the copy cannot be made. The caller then builds + * the request from the certificate. */ -static OcspRequest* GetCtxOcspRequest(WOLFSSL* ssl, byte* ctxOwnsRequest) +static OcspRequest* GetCtxOcspRequest(WOLFSSL* ssl) { wolfSSL_Mutex* ocspLock = GetCtxOcspLock(ssl->ctx); OcspRequest* request = NULL; - *ctxOwnsRequest = 0; - - if (ocspLock == NULL) { + if (ssl->buffers.weOwnCert) { + /* Not the certificate the cache was built from. */ + } + else if (ocspLock == NULL) { WOLFSSL_MSG("No CTX OCSP lock, skipping the request cache"); } else if (wc_LockMutex(ocspLock) != 0) { WOLFSSL_MSG("Couldn't lock CTX OCSP mutex, skipping the request cache"); } else { - request = ssl->ctx->certOcspRequest; - if (request != NULL) - *ctxOwnsRequest = 1; + if (ssl->ctx->certOcspRequest != NULL) { + request = CopyCtxOcspRequest(ssl->ctx->certOcspRequest, ssl->heap); + } wc_UnLockMutex(ocspLock); } @@ -30749,16 +30802,14 @@ int SendCertificateStatus(WOLFSSL* ssl) /* case WOLFSSL_CSR_OCSP: */ case WOLFSSL_CSR2_OCSP: { - byte ctxOwnsRequest = 0; - OcspRequest* request = GetCtxOcspRequest(ssl, &ctxOwnsRequest); + OcspRequest* request = GetCtxOcspRequest(ssl); buffer response; - ret = CreateOcspResponse(ssl, &request, &response, - &ctxOwnsRequest); + ret = CreateOcspResponse(ssl, &request, &response); - /* if a request was successfully created and not stored in - * ssl->ctx then free it */ - if (ret == 0 && request != NULL && !ctxOwnsRequest) { + /* The request is this connection's, whether it is the copy of the + * cached one or one CreateOcspResponse() built. */ + if (request != NULL) { FreeOcspRequest(request); XFREE(request, ssl->heap, DYNAMIC_TYPE_OCSP_REQUEST); request = NULL; @@ -30786,22 +30837,18 @@ int SendCertificateStatus(WOLFSSL* ssl) #if defined HAVE_CERTIFICATE_STATUS_REQUEST_V2 case WOLFSSL_CSR2_OCSP_MULTI: { - /* Three requests with three different owners are in play, so they - * are kept in separate variables. "ctxOwnsRequest" tracks the leaf - * one only. */ - byte ctxOwnsRequest = 0; - OcspRequest* leafRequest = GetCtxOcspRequest(ssl, &ctxOwnsRequest); + /* The leaf and chain requests are kept in separate variables. */ + OcspRequest* leafRequest = GetCtxOcspRequest(ssl); buffer responses[1 + MAX_CHAIN_DEPTH]; word32 i = 0; XMEMSET(responses, 0, sizeof(responses)); - ret = CreateOcspResponse(ssl, &leafRequest, &responses[0], - &ctxOwnsRequest); + ret = CreateOcspResponse(ssl, &leafRequest, &responses[0]); - /* if a request was successfully created and not stored in - * ssl->ctx then free it */ - if (ret == 0 && leafRequest != NULL && !ctxOwnsRequest) { + /* The request is this connection's, whether it is the copy of the + * cached one or one CreateOcspResponse() built. */ + if (leafRequest != NULL) { FreeOcspRequest(leafRequest); XFREE(leafRequest, ssl->heap, DYNAMIC_TYPE_OCSP_REQUEST); leafRequest = NULL; diff --git a/src/tls13.c b/src/tls13.c index 789fd263b9d..95db1060ca0 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -10511,7 +10511,6 @@ static int SetupOcspResp(WOLFSSL* ssl) TLSX* extension = NULL; int ret = 0; OcspRequest* request = NULL; - byte ctxOwnsRequest = 0; extension = TLSX_Find(ssl->extensions, TLSX_STATUS_REQUEST); #ifdef WOLFSSL_POST_HANDSHAKE_AUTH @@ -10589,12 +10588,10 @@ static int SetupOcspResp(WOLFSSL* ssl) } } request = &csr->request.ocsp[0]; - ret = CreateOcspResponse(ssl, &request, &csr->responses[0], - &ctxOwnsRequest); - /* Only a successful call replaces "request", and only a request the CTX did - * not take ownership of is ours to free. Both are checked, matching the - * SendCertificateStatus() callers. */ - if (ret == 0 && request != &csr->request.ocsp[0] && !ctxOwnsRequest) { + ret = CreateOcspResponse(ssl, &request, &csr->responses[0]); + /* Only a successful call replaces "request", with one it built for us to + * free. */ + if (ret == 0 && request != &csr->request.ocsp[0]) { /* request was allocated in CreateOcspResponse() */ FreeOcspRequest(request); XFREE(request, ssl->heap, DYNAMIC_TYPE_OCSP_REQUEST); diff --git a/tests/api.c b/tests/api.c index 1436504e900..942677131af 100644 --- a/tests/api.c +++ b/tests/api.c @@ -47498,6 +47498,7 @@ TEST_CASE testCases[] = { TEST_DECL_GROUP("ocsp", test_dtls12_nonblock_ocsp), TEST_DECL_GROUP("ocsp", test_ocsp_ctx_request_cache), TEST_DECL_GROUP("ocsp", test_ocsp_ctx_request_cache_cert_change), + TEST_DECL_GROUP("ocsp", test_ocsp_ctx_request_cache_drop_in_use), TEST_DECL_GROUP("ocsp", test_ocsp_responder), TEST_DECL_GROUP("ocsp", test_wolfIO_DecodeUrl_crlf_reject), TEST_DECL_GROUP("ocsp", test_wolfIO_DecodeUrl_host_bounds), diff --git a/tests/api/test_ocsp.c b/tests/api/test_ocsp.c index 5dbd379e91c..f71187a898e 100644 --- a/tests/api/test_ocsp.c +++ b/tests/api/test_ocsp.c @@ -6441,6 +6441,106 @@ int test_ocsp_ctx_request_cache_cert_change(void) return EXPECT_RESULT(); } + +/* When set, the IO callback below drops the cached request of this CTX. */ +static WOLFSSL_CTX* test_ocsp_ctx_request_drop_ctx; + +/* Drops the CTX's cached request from inside the responder lookup, as + * replacing the CTX certificate does, while a connection is still checking the + * request it took from the cache. With two threads that is a certificate reload + * while a lookup blocks on the responder. The certificate itself is left alone: + * the connection borrows it from the CTX in these builds. */ +static int test_ocsp_ctx_request_drop_io_cb(void* ioCtx, const char* url, + int urlSz, unsigned char* req, int reqSz, unsigned char** respBuf) +{ + if (test_ocsp_ctx_request_drop_ctx != NULL) { + FreeCtxOcspRequestCache(test_ocsp_ctx_request_drop_ctx); + } + return test_ocsp_ctx_request_cache_io_cb(ioCtx, url, urlSz, req, reqSz, + respBuf); +} + +/* The CTX can drop its cached request while a connection is checking the one + * it got from the cache: connections only use a copy. The second handshake's + * request comes from the cache, and the cache is dropped from inside that + * handshake's responder lookup. The connection has to finish its check, after + * the lookup, on a request that is still there (ASan), and the handshake has + * to complete. */ +int test_ocsp_ctx_request_cache_drop_in_use(void) +{ + EXPECT_DECLS; + WOLFSSL_CTX* ctx_c = NULL; + WOLFSSL_CTX* ctx_s = NULL; + WOLFSSL* ssl_c = NULL; + WOLFSSL* ssl_s = NULL; + int i; + struct test_memio_ctx test_ctx; + + test_ocsp_ctx_request_cache_cb_cnt = 0; + test_ocsp_ctx_request_cache_firstSz = 0; + test_ocsp_ctx_request_cache_differs = 0; + test_ocsp_ctx_request_drop_ctx = NULL; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, NULL, NULL, + wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0); + ExpectIntEQ(wolfSSL_CTX_use_certificate_chain_file(ctx_s, + "./certs/ocsp/server1-chain-noroot.pem"), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_use_PrivateKey_file(ctx_s, + "./certs/ocsp/server1-key.pem", WOLFSSL_FILETYPE_PEM), + WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_load_verify_locations(ctx_s, + "./certs/ocsp/root-ca-cert.pem", NULL), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_load_verify_locations(ctx_s, + "./certs/ocsp/intermediate1-ca-cert.pem", NULL), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_load_verify_locations(ctx_c, + "./certs/ocsp/root-ca-cert.pem", NULL), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_load_verify_locations(ctx_c, + "./certs/ocsp/intermediate1-ca-cert.pem", NULL), WOLFSSL_SUCCESS); + + ExpectIntEQ(wolfSSL_CTX_EnableOCSPStapling(ctx_c), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_EnableOCSPStapling(ctx_s), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_SetOCSP_OverrideURL(ctx_s, "http://dummy.test"), + WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_EnableOCSP(ctx_s, + WOLFSSL_OCSP_NO_NONCE | WOLFSSL_OCSP_URL_OVERRIDE), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_CTX_SetOCSP_Cb(ctx_s, + test_ocsp_ctx_request_drop_io_cb, NULL, NULL), WOLFSSL_SUCCESS); + + for (i = 0; i < 2 && EXPECT_SUCCESS(); i++) { + /* Pass 0 builds and caches the request, pass 1 uses the cache. */ + test_ocsp_ctx_request_drop_ctx = (i == 1) ? ctx_s : NULL; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0); + ExpectIntEQ(wolfSSL_UseOCSPStapling(ssl_c, WOLFSSL_CSR_OCSP, 0), + WOLFSSL_SUCCESS); + ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + ExpectIntEQ(test_ocsp_ctx_request_cache_cb_cnt, i + 1); + if (i == 0) { + ExpectNotNull(ctx_s->certOcspRequest); + } + else { + /* Dropped from inside the lookup, and not cached again: this + * connection's request came from the cache. */ + ExpectNull(ctx_s->certOcspRequest); + } + + wolfSSL_free(ssl_c); + ssl_c = NULL; + wolfSSL_free(ssl_s); + ssl_s = NULL; + } + test_ocsp_ctx_request_drop_ctx = NULL; + + wolfSSL_free(ssl_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); + + return EXPECT_RESULT(); +} #else int test_ocsp_ctx_request_cache(void) { @@ -6450,6 +6550,10 @@ int test_ocsp_ctx_request_cache_cert_change(void) { return TEST_SKIPPED; } +int test_ocsp_ctx_request_cache_drop_in_use(void) +{ + return TEST_SKIPPED; +} #endif #if defined(HAVE_OCSP_RESPONDER) && defined(WOLFSSL_ASN_TEMPLATE) && \ diff --git a/tests/api/test_ocsp.h b/tests/api/test_ocsp.h index 93b52c01a8d..b134499c5bc 100644 --- a/tests/api/test_ocsp.h +++ b/tests/api/test_ocsp.h @@ -68,6 +68,7 @@ int test_ocsp_status_verify_cb_would_block(void); int test_dtls12_nonblock_ocsp(void); int test_ocsp_ctx_request_cache(void); int test_ocsp_ctx_request_cache_cert_change(void); +int test_ocsp_ctx_request_cache_drop_in_use(void); int test_ocsp_responder(void); int test_ocsp_ancestor_responder_rejected(void); int test_ocsp_forged_responder_cert_rejected(void); diff --git a/wolfssl/internal.h b/wolfssl/internal.h index afe88843afa..23d4fe87282 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -7886,12 +7886,15 @@ WOLFSSL_LOCAL int SendCertificateRequest(WOLFSSL* ssl); #if defined(HAVE_CERTIFICATE_STATUS_REQUEST) \ || defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2) WOLFSSL_LOCAL int CreateOcspResponse(WOLFSSL* ssl, OcspRequest** ocspRequest, - buffer* response, byte* ctxOwnsRequest); + buffer* response); #endif #if !defined(NO_WOLFSSL_SERVER) && \ (defined(HAVE_CERTIFICATE_STATUS_REQUEST) || \ defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2)) -WOLFSSL_LOCAL void FreeCtxOcspRequestCache(WOLFSSL_CTX* ctx); +#ifdef WOLFSSL_API_PREFIX_MAP + #define FreeCtxOcspRequestCache wolfSSL_FreeCtxOcspRequestCache +#endif +WOLFSSL_TEST_VIS void FreeCtxOcspRequestCache(WOLFSSL_CTX* ctx); #else #define FreeCtxOcspRequestCache(ctx) WC_DO_NOTHING #endif