From 525f52bd57d4055ae1ee4c9307d5413a8e9dd83f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Fri, 31 Jul 2026 12:45:57 +0200 Subject: [PATCH 1/4] Improve certificate_status_request_v2 handling RFC 8446 Section 4.4.2.1 deprecates the status_request_v2 extension for TLS 1.3. The server side already avoided it; on the client side, reject it in every message type but ClientHello once TLS 1.3 is negotiated, so TLSX_CSR2_Parse() can no longer record it. ClientHello stays allowed because the peer may still negotiate a lower version, where the extension does apply. Also align the pending signer registration in the chain verification loop with the CA checks AddCA() performs on the normal path, so the same conditions apply on both. Register the signer as WOLFSSL_CHAIN_CA rather than CA_TYPE while doing so. TLSX_CSR2_MergePendingCA() promotes it into the certificate manager, and wolfSSL_CertManagerUnloadIntermediateCerts() selects entries by that type, so a chain CA learned over a status_request_v2 multi handshake could never be unloaded again. Add test_TLSX_CSR2_tls13_msg_type_validation, which feeds the extension to TLSX_Parse() in the TLS 1.3 message types that must not carry it. Fixes F-7227. --- src/internal.c | 53 +++++++++++++++++++++++++++++++++++----- src/tls.c | 24 ++++++++++++++---- tests/api.c | 1 + tests/api/test_tls_ext.c | 46 ++++++++++++++++++++++++++++++++++ tests/api/test_tls_ext.h | 1 + 5 files changed, 114 insertions(+), 11 deletions(-) diff --git a/src/internal.c b/src/internal.c index 838fb57cd21..eb3ef1705ef 100644 --- a/src/internal.c +++ b/src/internal.c @@ -16505,6 +16505,32 @@ static int ProcessPeerCertAddPendingCA(WOLFSSL* ssl, buffer* cert) if (ret != 0) { goto exit_req_v2; } + /* Only a certificate that is actually usable as a CA may enter the pending + * signer pool. ParseCertRelative() consults that pool ahead of the + * certificate manager and uses the signer as a verification key without + * further checks, so admission has to enforce the same capabilities AddCA() + * requires of a chain CA. + * + * A non-CA is skipped rather than reported as an error, because + * ProcessPeerCerts() only offers a chain cert to AddCA() when isCA is set: + * such a certificate is already left out of the certificate manager without + * failing the handshake. */ + if (!dCertAdd->isCA) { + WOLFSSL_MSG("Chain cert is not a CA, not adding as pending CA"); + goto exit_req_v2; + } +#ifndef ALLOW_INVALID_CERTSIGN + /* Per RFC 5280 an absent Key Usage extension implies all usages, so only + * enforce certificate signing when the extension is actually present. + * AddCA() rejects such a certificate outright, so report the same error + * here rather than quietly leaving it out of the pool. */ + if (!dCertAdd->selfSigned && dCertAdd->extKeyUsageSet && + (dCertAdd->extKeyUsage & KEYUSE_KEY_CERT_SIGN) == 0) { + WOLFSSL_MSG("Chain cert doesn't have key usage certificate signing"); + ret = NOT_CA_ERROR; + goto exit_req_v2; + } +#endif ret = AllocDer(&derBuffer, cert->length, CA_TYPE, ssl->heap); if (ret != 0 || derBuffer == NULL) { goto exit_req_v2; @@ -16515,7 +16541,11 @@ static int ProcessPeerCertAddPendingCA(WOLFSSL* ssl, buffer* cert) ret = MEMORY_E; goto exit_req_v2; } - ret = FillSigner(s, dCertAdd, CA_TYPE, derBuffer); + /* WOLFSSL_CHAIN_CA, not CA_TYPE: TLSX_CSR2_MergePendingCA() promotes this + * signer into the certificate manager, and the unload path + * (wolfSSL_CertManagerUnloadIntermediateCerts()) selects entries by that + * type. AddCA() records chain CAs the same way. */ + ret = FillSigner(s, dCertAdd, WOLFSSL_CHAIN_CA, derBuffer); if (ret != 0) { goto exit_req_v2; } @@ -17746,14 +17776,25 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, } #endif #if defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2) - if (ret == 0 && addToPendingCAs && !alreadySigner) { - /* skipAddCA is only consulted later on the ret == 0 - * continuation path; on helper failure we goto - * exit_ppc, so setting it up-front is safe. */ + if (ret == 0 && addToPendingCAs && !alreadySigner && + !ssl->options.verifyNone && !skipAddCA) { + /* The verifyNone and skipAddCA conditions mirror the + * guards on the AddCA() call below. A certificate the + * surrounding code has already declined to admit as a + * CA must not enter the pending pool either, and must + * not be failed against CA rules. skipAddCA is set here + * so AddCA() is not also run for a certificate that did + * enter the pool; it is only consulted later on the + * ret == 0 continuation path, so setting it up-front is + * safe. */ skipAddCA = 1; ret = ProcessPeerCertAddPendingCA(ssl, &args->certs[args->certIdx]); - if (ret != 0) + /* A certificate turned away for not being a usable CA + * falls through to the shared error handling below so + * that it fails the handshake the same way a rejection + * from AddCA() does. */ + if (ret != 0 && ret != WC_NO_ERR_TRACE(NOT_CA_ERROR)) goto exit_ppc; } #endif /* HAVE_CERTIFICATE_STATUS_REQUEST_V2 */ diff --git a/src/tls.c b/src/tls.c index 6647207776a..be43262d528 100644 --- a/src/tls.c +++ b/src/tls.c @@ -4255,8 +4255,19 @@ static int TLSX_CSR2_Parse(WOLFSSL* ssl, const byte* input, word16 length, if (!isRequest) { #ifndef NO_WOLFSSL_CLIENT - TLSX* extension = TLSX_Find(ssl->extensions, TLSX_STATUS_REQUEST_V2); - CertificateStatusRequestItemV2* csr2 = extension ? + TLSX* extension; + CertificateStatusRequestItemV2* csr2; + + /* RFC 8446 Section 4.4.2.1: a TLS 1.3 client must not act upon the + * presence of, or the information in, this extension. Return before any + * extension state is touched. TLSX_Parse() already rejects it for every + * TLS 1.3 message type that reaches this branch, so this is defence in + * depth rather than the load bearing check. */ + if (IsAtLeastTLSv1_3(ssl->version)) + return length ? BUFFER_ERROR : 0; /* extension_data MUST be empty. */ + + extension = TLSX_Find(ssl->extensions, TLSX_STATUS_REQUEST_V2); + csr2 = extension ? (CertificateStatusRequestItemV2*)extension->data : NULL; if (!csr2) { @@ -18698,9 +18709,12 @@ WOLFSSL_TEST_VIS int TLSX_Parse(WOLFSSL* ssl, const byte* input, word16 length, #if defined(WOLFSSL_TLS13) && defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2) if (IsAtLeastTLSv1_3(ssl->version)) { - if (msgType != client_hello && - msgType != certificate_request && - msgType != certificate) + /* RFC 8446 Section 4.4.2.1: a TLS 1.3 server must not send + * this extension in EncryptedExtensions, CertificateRequest + * or Certificate. ClientHello stays allowed because the + * peer may still negotiate a lower version, where the + * extension does apply. */ + if (msgType != client_hello) return EXT_NOT_ALLOWED; } else diff --git a/tests/api.c b/tests/api.c index 14f10ca8745..395a87574ea 100644 --- a/tests/api.c +++ b/tests/api.c @@ -38654,6 +38654,7 @@ TEST_CASE testCases[] = { TEST_DECL(test_TLSX_TCA_Find), TEST_DECL(test_TLSX_SNI_GetSize_overflow), TEST_DECL(test_TLSX_ECH_msg_type_validation), + TEST_DECL(test_TLSX_CSR2_tls13_msg_type_validation), TEST_DECL(test_TLSX_SRTP_msg_type_validation), TEST_DECL(test_TLSX_ALPN_server_response_count), TEST_DECL(test_TLSX_SupportedCurve_empty_or_unsupported), diff --git a/tests/api/test_tls_ext.c b/tests/api/test_tls_ext.c index 6453315505f..ec9f5fe7fd2 100644 --- a/tests/api/test_tls_ext.c +++ b/tests/api/test_tls_ext.c @@ -1806,6 +1806,52 @@ int test_wolfSSL_custom_ext_ticket_fallback(void) return EXPECT_RESULT(); } +/* RFC 8446 Section 4.4.2.1: status_request_v2 is not used in TLS 1.3. Only a + * ClientHello may still carry it, since the peer can negotiate a lower version + * where it does apply - every other message type is rejected, so that + * TLSX_CSR2_Parse() never gets to set ssl->status_request_v2 on a TLS 1.3 + * connection. */ +int test_TLSX_CSR2_tls13_msg_type_validation(void) +{ + EXPECT_DECLS; +#if defined(WOLFSSL_TLS13) && defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2) && \ + !defined(NO_WOLFSSL_CLIENT) && !defined(NO_TLS) + WOLFSSL_CTX* ctx = NULL; + WOLFSSL* ssl = NULL; + /* type = TLSX_STATUS_REQUEST_V2 (0x0011), size = 0x0000 */ + const byte extBytes[] = { 0x00, 0x11, 0x00, 0x00 }; + Suites suites; + + XMEMSET(&suites, 0, sizeof(suites)); + + ExpectNotNull(ctx = wolfSSL_CTX_new(wolfTLSv1_3_client_method())); + ExpectNotNull(ssl = wolfSSL_new(ctx)); + + ExpectIntEQ(TLSX_Parse(ssl, extBytes, (word16)sizeof(extBytes), + encrypted_extensions, NULL), + WC_NO_ERR_TRACE(EXT_NOT_ALLOWED)); + /* certificate_request is parsed as a request, so it needs a suites list. */ + ExpectIntEQ(TLSX_Parse(ssl, extBytes, (word16)sizeof(extBytes), + certificate_request, &suites), + WC_NO_ERR_TRACE(EXT_NOT_ALLOWED)); + /* In a Certificate message the earlier RFC 8446 4.4.2 gate fires first, + * because nothing offered status_request_v2 in the ClientHello. Either way + * it never reaches TLSX_CSR2_Parse(). */ + ExpectIntEQ(TLSX_Parse(ssl, extBytes, (word16)sizeof(extBytes), + certificate, NULL), + WC_NO_ERR_TRACE(UNSUPPORTED_EXTENSION)); + + /* Nothing was recorded on the way out. */ + if (ssl != NULL) { + ExpectIntEQ(ssl->status_request_v2, 0); + } + + wolfSSL_free(ssl); + wolfSSL_CTX_free(ctx); +#endif + return EXPECT_RESULT(); +} + /* use_srtp is only valid in ClientHello/ServerHello (pre-TLS 1.3) or * ClientHello/EncryptedExtensions (TLS 1.3) per RFC 5764. Feeding it in a * Finished message must be rejected with EXT_NOT_ALLOWED. */ diff --git a/tests/api/test_tls_ext.h b/tests/api/test_tls_ext.h index 429216498e3..4f100429fc3 100644 --- a/tests/api/test_tls_ext.h +++ b/tests/api/test_tls_ext.h @@ -38,6 +38,7 @@ int test_certificate_authorities_client_hello(void); int test_TLSX_TCA_Find(void); int test_TLSX_SNI_GetSize_overflow(void); int test_TLSX_ECH_msg_type_validation(void); +int test_TLSX_CSR2_tls13_msg_type_validation(void); int test_TLSX_SRTP_msg_type_validation(void); int test_TLSX_ALPN_server_response_count(void); int test_TLSX_SupportedCurve_empty_or_unsupported(void); From 1d15e1f27f8866a37de8f1099f56db16b961b7dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Fri, 31 Jul 2026 12:45:57 +0200 Subject: [PATCH 2/4] Make OCSP stapling request ownership explicit The OcspRequest carried a "void* ssl" back-pointer that the stapling paths wrote just before handing the request to the OCSP layer. For the request cached on the WOLFSSL_CTX that field is shared by every connection using it, so concurrent handshakes raced on it. Drop the field and pass the connection to CheckOcspRequest() and CheckOcspResponse() as an argument instead, which is the only thing it was ever read for. Ownership of the cached request was equally implicit. Publication moves out of CreateOcspRequest() into CreateOcspResponse(), and callers now learn whether the CTX took ownership from a "ctxOwnsRequest" flag rather than by comparing pointers against ssl->ctx->certOcspRequest, which was read without the lock that guards it. The flag and the request are handed back together on success and both left untouched on failure, so a caller never decides ownership against a request it is not holding. The cache is a field of the WOLFSSL_CTX, so serialize it with a lock scoped to the CTX. SSL_CM(ssl) can resolve to a per-SSL cert manager when WOLFSSL_LOCAL_X509_STORE is defined, which left two connections on one CTX taking different locks for a check-then-set on the same pointer. GetCtxOcspLock() keys off ssl->ctx->cm for both the reader and the publisher, and a failure to take it is logged instead of silently disabling the cache. CheckOcspRequest() also loses its heap argument. It was only ever the hint for the response buffer it hands back, which the caller frees against the connection, so take it from the connection rather than from a parameter every caller had to keep in step with its own free. Smaller fixes in the same paths: zero the caller's response buffer before the argument check can return, since SendCertificateStatus() frees it without checking the return code; fold the ocsp_stapling NULL check into the single early skip so the later uses need no guard; gate the SetupOcspResp() free on success like the other two callers; split the three differently owned requests in the WOLFSSL_CSR2_OCSP_MULTI case into separate variables; and let that case's allocation failures fall through to its shared cleanup instead of returning, which leaked an already built leaf response. Add test_ocsp_ctx_request_cache, which runs three handshakes over one CTX pair and checks that the later ones reuse the cached request rather than building another. The responder callback answers with a canned good response, so stapling runs all the way through and the ownership decision each connection makes is actually acted on: a connection that freed the shared request shows up as a use after free on the next pass and a double free at CTX teardown. The cached request is marked before the last pass and the encoded request the callback sees is compared, since a request rebuilt from the same certificate would otherwise be identical byte for byte. The test is gated on !WOLFSSL_COPY_CERT: OPENSSL_ALL implies it, and it gives every WOLFSSL its own certificate copy, which takes the cache out of play. A new ocsp.yml job covers the plain stapling build, an --enable-all build with the copy turned back off, and an ASan build. Also gate test_tls13_pha_status_request on KEEP_PEER_CERT. It checks the received client certificate with wolfSSL_get_peer_certificate(), which is only built when that macro is defined, so a post-handshake auth build with stapling but without the OpenSSL compatibility layer failed to link tests/unit.test. Fixes F-7230 and F-7231. --- .github/workflows/ocsp.yml | 60 +++++++++ src/internal.c | 257 ++++++++++++++++++++++++++----------- src/ocsp.c | 28 ++-- src/ssl_certman.c | 2 +- src/tls.c | 22 +--- src/tls13.c | 12 +- tests/api.c | 1 + tests/api/test_ocsp.c | 199 ++++++++++++++++++++++++++++ tests/api/test_ocsp.h | 1 + tests/api/test_tls13.c | 6 + wolfssl/internal.h | 5 +- wolfssl/ocsp.h | 4 +- wolfssl/wolfcrypt/asn.h | 1 - 13 files changed, 482 insertions(+), 116 deletions(-) diff --git a/.github/workflows/ocsp.yml b/.github/workflows/ocsp.yml index 03123f608d9..09cabf9a887 100644 --- a/.github/workflows/ocsp.yml +++ b/.github/workflows/ocsp.yml @@ -68,6 +68,66 @@ jobs: ./tests/unit.test -test_wolfIO_OcspDestAllowed | tee out.txt grep -Eq 'test_wolfIO_OcspDestAllowed[^_].*: passed' out.txt + # The leaf OCSP request built for stapling is cached on the WOLFSSL_CTX and + # 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. + 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) }} + runs-on: ubuntu-24.04 + timeout-minutes: 20 + strategy: + fail-fast: false + matrix: + include: + # Plain stapling build: no OPENSSL_ALL, so no WOLFSSL_COPY_CERT and + # the cache is live. + - name: default + config: --enable-ocsp --enable-ocspstapling --enable-ocspstapling2 + # The same cache under --enable-all, which pulls in OPENSSL_ALL and + # with it the compatibility-layer code paths around the cert manager. + # OPENSSL_ALL would otherwise force WOLFSSL_COPY_CERT and take the + # cache out of play entirely, so that is turned back off explicitly - + # which is what this entry is really here to prove. + - name: all, no cert copy + config: --enable-all CPPFLAGS=-DWOLFSSL_NO_COPY_CERT + # The cache hands one OcspRequest to many connections, so the failure + # mode of an ownership mistake is a double free or a use after free at + # CTX teardown rather than a wrong answer. ASan is what turns that into + # a test failure. + - name: asan + config: --enable-ocsp --enable-ocspstapling --enable-ocspstapling2 CFLAGS='-fsanitize=address -g' LDFLAGS='-fsanitize=address' + steps: + - name: workaround high-entropy ASLR + # Needed for the ASan build on this runner image; harmless for the rest. + run: sudo sysctl vm.mmap_rnd_bits=28 + + - name: Checkout wolfSSL + uses: actions/checkout@v5 + + - name: Build wolfSSL + run: autoreconf -ivf && ./configure ${{ matrix.config }} && make + + # Assert on the counters rather than grepping the test name for "passed": + # the handshake under test logs to the same stream and splits the name and + # the result across lines. Running the one test on its own makes 0/0/1/1 + # exact, and a build where the cache is compiled out reports 0/1/0/1 + # instead - so a config change that quietly disables this cannot pass as + # green. + # + # Leak detection is off because wolfSSL's own unit.test has no verified + # clean LSan baseline; the double free and use after free this is here to + # catch are reported either way. + - name: Run the CTX OCSP request cache test + env: + ASAN_OPTIONS: detect_leaks=0 + run: | + set -o pipefail + ./tests/unit.test -test_ocsp_ctx_request_cache | tee out.txt + grep -Eq 'Failed/Skipped/Passed/All: 0/0/1/1' out.txt + ocsp_ssrf_screen_fallback: name: ocsp responder SSRF screening (gethostbyname fallback) 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 eb3ef1705ef..4dfadf9cddc 100644 --- a/src/internal.c +++ b/src/internal.c @@ -26412,23 +26412,37 @@ int SendFinished(WOLFSSL* ssl) (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". In the case - * of !ssl->buffers.weOwnCert, ssl->ctx->certOcspRequest gets set to "request". +/* 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 + * scope is the CTX. SSL_CM(ssl) must not be used to reach that lock: with + * WOLFSSL_LOCAL_X509_STORE a connection can carry its own cert manager, which + * would leave two connections on one CTX taking different locks while doing a + * check-then-set on the very same field. + * + * 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) +{ + if (ssl->ctx->cm == NULL || ssl->ctx->cm->ocsp_stapling == NULL) + return NULL; + + return &ssl->ctx->cm->ocsp_stapling->ocspLock; +} + +/* Parses and decodes the certificate then initializes "request". * * Returns 0 on success */ int CreateOcspRequest(WOLFSSL* ssl, OcspRequest* request, - DecodedCert* cert, byte* certData, word32 length, - byte *ctxOwnsRequest) + DecodedCert* cert, byte* certData, word32 length) { int ret; if (request != NULL) XMEMSET(request, 0, sizeof(OcspRequest)); - if (ctxOwnsRequest!= NULL) - *ctxOwnsRequest = 0; - InitDecodedCert(cert, certData, length, ssl->heap); /* TODO: Setup async support here */ ret = ParseCertRelative(cert, CERT_TYPE, NO_VERIFY, SSL_CM(ssl), NULL); @@ -26436,20 +26450,6 @@ int CreateOcspRequest(WOLFSSL* ssl, OcspRequest* request, WOLFSSL_MSG("ParseCert failed"); if (ret == 0) ret = InitOcspRequest(request, cert, 0, ssl->heap); - if (ret == 0) { - /* make sure ctx OCSP request is updated */ - if (!ssl->buffers.weOwnCert && SSL_CM(ssl) != NULL) { - wolfSSL_Mutex* ocspLock = &SSL_CM(ssl)->ocsp_stapling->ocspLock; - if (wc_LockMutex(ocspLock) == 0) { - if (ssl->ctx->certOcspRequest == NULL) { - ssl->ctx->certOcspRequest = request; - if (ctxOwnsRequest!= NULL) - *ctxOwnsRequest = 1; - } - wc_UnLockMutex(ocspLock); - } - } - } FreeDecodedCert(cert); @@ -26461,30 +26461,48 @@ int CreateOcspRequest(WOLFSSL* ssl, OcspRequest* request, * 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. In those cases managing ocspRequest free'ing - * is up to the caller. NOTE: in OcspCreateRequest ssl->ctx->certOcspRequest can - * be set to point to "ocspRequest" and it then should not be free'd since - * wolfSSL_CTX_free will take care of it. + * 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(). * * Returns 0 on success */ int CreateOcspResponse(WOLFSSL* ssl, OcspRequest** ocspRequest, - buffer* response) + buffer* response, byte* ctxOwnsRequest) { int ret = 0; OcspRequest* request = NULL; byte createdRequest = 0; - byte ctxOwnsRequest = 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) + if (ssl == NULL || ocspRequest == NULL || response == NULL || + ctxOwnsRequest == NULL) { return BAD_FUNC_ARG; + } - XMEMSET(response, 0, sizeof(*response)); request = *ocspRequest; + ctxOwns = *ctxOwnsRequest; - /* unable to fetch status. skip. */ - if (SSL_CM(ssl) == NULL || SSL_CM(ssl)->ocspStaplingEnabled == 0) - return 0; + /* unable to fetch status. skip. The stapling object is checked here so that + * every use of it below needs no further guarding. */ + if (SSL_CM(ssl) == NULL || SSL_CM(ssl)->ocspStaplingEnabled == 0 || + SSL_CM(ssl)->ocsp_stapling == NULL) { + goto exit_cor; + } if (request == NULL || ssl->buffers.weOwnCert) { DerBuffer* der = ssl->buffers.certificate; @@ -26492,19 +26510,50 @@ int CreateOcspResponse(WOLFSSL* ssl, OcspRequest** ocspRequest, /* unable to fetch status. skip. */ if (der->buffer == NULL || der->length == 0) - return 0; + goto exit_cor; WC_ALLOC_VAR_EX(cert, DecodedCert, 1, ssl->heap, DYNAMIC_TYPE_DCERT, - return MEMORY_E); + { ret = MEMORY_E; goto exit_cor; }); request = (OcspRequest*)XMALLOC(sizeof(OcspRequest), ssl->heap, DYNAMIC_TYPE_OCSP_REQUEST); if (request == NULL) ret = MEMORY_E; createdRequest = 1; + ctxOwns = 0; if (ret == 0) { ret = CreateOcspRequest(ssl, request, cert, der->buffer, - der->length, &ctxOwnsRequest); + der->length); + } + + /* Only a request built from a CTX-held certificate may be cached, since + * the cache outlives this connection. + * + * 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. */ + if (ret == 0 && !ssl->buffers.weOwnCert) { + wolfSSL_Mutex* ocspLock = GetCtxOcspLock(ssl); + + /* 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) { + WOLFSSL_MSG("No CTX OCSP lock, not caching the request"); + } + 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; + } + wc_UnLockMutex(ocspLock); + } } if (ret != 0) { @@ -26516,9 +26565,8 @@ int CreateOcspResponse(WOLFSSL* ssl, OcspRequest** ocspRequest, } if (ret == 0) { - request->ssl = ssl; ret = CheckOcspRequest(SSL_CM(ssl)->ocsp_stapling, request, response, - ssl->heap); + ssl); /* Suppressing soft-fail responder errors. OCSP_CERT_REVOKED is an * explicit positive assertion of revocation and must not be ignored. @@ -26532,13 +26580,21 @@ int CreateOcspResponse(WOLFSSL* ssl, OcspRequest** ocspRequest, } /* free request up if error case found otherwise return it */ - if (ret != 0 && createdRequest && !ctxOwnsRequest) { + if (ret != 0 && createdRequest && !ctxOwns) { FreeOcspRequest(request); XFREE(request, ssl->heap, DYNAMIC_TYPE_OCSP_REQUEST); + request = NULL; } - if (ret == 0) +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. */ + if (ret == 0) { *ocspRequest = request; + *ctxOwnsRequest = ctxOwns; + } return ret; } @@ -27325,6 +27381,42 @@ 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. + * + * 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. + * + * Returns the cached request, or NULL when there is none. + */ +static OcspRequest* GetCtxOcspRequest(WOLFSSL* ssl, byte* ctxOwnsRequest) +{ + wolfSSL_Mutex* ocspLock = GetCtxOcspLock(ssl); + OcspRequest* request = NULL; + + *ctxOwnsRequest = 0; + + 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; + wc_UnLockMutex(ocspLock); + } + + return request; +} + static int BuildCertificateStatusWithStatusCB(WOLFSSL* ssl, byte status_type) { WOLFSSL_OCSP *ocsp; @@ -27415,14 +27507,16 @@ int SendCertificateStatus(WOLFSSL* ssl) /* case WOLFSSL_CSR_OCSP: */ case WOLFSSL_CSR2_OCSP: { - OcspRequest* request = ssl->ctx->certOcspRequest; + byte ctxOwnsRequest = 0; + OcspRequest* request = GetCtxOcspRequest(ssl, &ctxOwnsRequest); buffer response; - ret = CreateOcspResponse(ssl, &request, &response); + ret = CreateOcspResponse(ssl, &request, &response, + &ctxOwnsRequest); /* if a request was successfully created and not stored in * ssl->ctx then free it */ - if (ret == 0 && request != ssl->ctx->certOcspRequest) { + if (ret == 0 && request != NULL && !ctxOwnsRequest) { FreeOcspRequest(request); XFREE(request, ssl->heap, DYNAMIC_TYPE_OCSP_REQUEST); request = NULL; @@ -27450,22 +27544,27 @@ int SendCertificateStatus(WOLFSSL* ssl) #if defined HAVE_CERTIFICATE_STATUS_REQUEST_V2 case WOLFSSL_CSR2_OCSP_MULTI: { - OcspRequest* request = ssl->ctx->certOcspRequest; - buffer responses[1 + MAX_CHAIN_DEPTH]; + /* 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); + buffer responses[1 + MAX_CHAIN_DEPTH]; word32 i = 0; XMEMSET(responses, 0, sizeof(responses)); - ret = CreateOcspResponse(ssl, &request, &responses[0]); + ret = CreateOcspResponse(ssl, &leafRequest, &responses[0], + &ctxOwnsRequest); /* if a request was successfully created and not stored in * ssl->ctx then free it */ - if (ret == 0 && request != ssl->ctx->certOcspRequest) { - FreeOcspRequest(request); - XFREE(request, ssl->heap, DYNAMIC_TYPE_OCSP_REQUEST); - request = NULL; + if (ret == 0 && leafRequest != NULL && !ctxOwnsRequest) { + FreeOcspRequest(leafRequest); + XFREE(leafRequest, ssl->heap, DYNAMIC_TYPE_OCSP_REQUEST); + leafRequest = NULL; } + /* leafRequest is done; the chain below has its own. */ if (ret == 0 && (!ssl->ctx->chainOcspRequest[0] || ssl->buffers.weOwnCertChain)) { @@ -27473,14 +27572,20 @@ int SendCertificateStatus(WOLFSSL* ssl) word32 idx = 0; WC_DECLARE_VAR(cert, DecodedCert, 1, 0); DerBuffer* chain; + /* Scratch owned by this block, reused for each chain cert */ + OcspRequest* chainRequest = NULL; + /* Allocation failures record the error and fall through to the + * cleanup at the end of the case. Returning here would leak the + * leaf response that CreateOcspResponse() has already built. */ WC_ALLOC_VAR_EX(cert, DecodedCert, 1, ssl->heap, - DYNAMIC_TYPE_DCERT, return MEMORY_E); - request = (OcspRequest*)XMALLOC(sizeof(OcspRequest), ssl->heap, - DYNAMIC_TYPE_OCSP_REQUEST); - if (request == NULL) { - WC_FREE_VAR_EX(cert, ssl->heap, DYNAMIC_TYPE_DCERT); - return MEMORY_E; + DYNAMIC_TYPE_DCERT, ret = MEMORY_E); + if (ret == 0) { + chainRequest = (OcspRequest*)XMALLOC(sizeof(OcspRequest), + ssl->heap, DYNAMIC_TYPE_OCSP_REQUEST); + if (chainRequest == NULL) { + ret = MEMORY_E; + } } /* use certChain if available, otherwise use certificate */ @@ -27489,7 +27594,7 @@ int SendCertificateStatus(WOLFSSL* ssl) chain = ssl->buffers.certificate; } - if (chain && chain->buffer) { + if (ret == 0 && chain && chain->buffer) { while (ret == 0 && idx + OPAQUE24_LEN < chain->length) { c24to32(chain->buffer + idx, &der.length); idx += OPAQUE24_LEN; @@ -27503,12 +27608,11 @@ int SendCertificateStatus(WOLFSSL* ssl) ret = MAX_CERT_EXTENSIONS_ERR; break; } - ret = CreateOcspRequest(ssl, request, cert, der.buffer, - der.length, &ctxOwnsRequest); + ret = CreateOcspRequest(ssl, chainRequest, cert, + der.buffer, der.length); if (ret == 0) { - request->ssl = ssl; ret = CheckOcspRequest(SSL_CM(ssl)->ocsp_stapling, - request, &responses[i + 1], ssl->heap); + chainRequest, &responses[i + 1], ssl); /* Suppressing soft-fail responder errors. * OCSP_CERT_REVOKED is an explicit positive @@ -27524,21 +27628,21 @@ int SendCertificateStatus(WOLFSSL* ssl) i++; - if (!ctxOwnsRequest) - FreeOcspRequest(request); + FreeOcspRequest(chainRequest); } } } - if (!ctxOwnsRequest) - XFREE(request, ssl->heap, DYNAMIC_TYPE_OCSP_REQUEST); + XFREE(chainRequest, ssl->heap, DYNAMIC_TYPE_OCSP_REQUEST); WC_FREE_VAR_EX(cert, ssl->heap, DYNAMIC_TYPE_DCERT); } else { + /* These are owned by the CTX and freed in wolfSSL_CTX_free() */ + OcspRequest* cachedRequest; + while (ret == 0 && i < MAX_CHAIN_DEPTH && - NULL != (request = ssl->ctx->chainOcspRequest[i])) { - request->ssl = ssl; + NULL != (cachedRequest = ssl->ctx->chainOcspRequest[i])) { ret = CheckOcspRequest(SSL_CM(ssl)->ocsp_stapling, - request, &responses[++i], ssl->heap); + cachedRequest, &responses[++i], ssl); /* Suppressing soft-fail responder errors. * OCSP_CERT_REVOKED is an explicit positive assertion of @@ -27553,17 +27657,18 @@ int SendCertificateStatus(WOLFSSL* ssl) } } - if (responses[0].buffer) { - if (ret == 0) { - ret = BuildCertificateStatus(ssl, status_type, responses, + if (ret == 0 && responses[0].buffer) { + ret = BuildCertificateStatus(ssl, status_type, responses, i + 1); - } + } - for (i = 0; i < 1 + MAX_CHAIN_DEPTH; i++) { - if (responses[i].buffer) { - XFREE(responses[i].buffer, ssl->heap, - DYNAMIC_TYPE_OCSP_REQUEST); - } + /* Not gated on the leaf response: the chain loop can have stored + * responses of its own even when the leaf produced none. */ + for (i = 0; i < 1 + MAX_CHAIN_DEPTH; i++) { + if (responses[i].buffer) { + XFREE(responses[i].buffer, ssl->heap, + DYNAMIC_TYPE_OCSP_REQUEST); + responses[i].buffer = NULL; } } diff --git a/src/ocsp.c b/src/ocsp.c index 7ddc6772a27..06b557490b1 100644 --- a/src/ocsp.c +++ b/src/ocsp.c @@ -88,7 +88,7 @@ int wc_CheckCertOcspResponse(WOLFSSL_OCSP *ocsp, DecodedCert *cert, if (InitOcspRequest(ocspRequest, cert, ocsp->cm->ocspSendNonce, ocsp->cm->heap) == 0) { ret = CheckOcspResponse(ocsp, response, responseSz, NULL, NULL, NULL, - ocspRequest, heap); + ocspRequest, heap, NULL); FreeOcspRequest(ocspRequest); } @@ -212,8 +212,7 @@ int CheckCertOCSP_ex(WOLFSSL_OCSP* ocsp, DecodedCert* cert, WOLFSSL* ssl) if (InitOcspRequest(ocspRequest, cert, ocsp->cm->ocspSendNonce, ocsp->cm->heap) == 0) { - ocspRequest->ssl = ssl; - ret = CheckOcspRequest(ocsp, ocspRequest, NULL, NULL); + ret = CheckOcspRequest(ocsp, ocspRequest, NULL, ssl); FreeOcspRequest(ocspRequest); } @@ -333,11 +332,13 @@ static int GetOcspStatus(WOLFSSL_OCSP* ocsp, OcspRequest* request, * entry The OCSP entry for this certificate. * ocspRequest Request corresponding to response. * heap Heap hint used for responseBuffer + * ssl Connection the request belongs to, may be NULL. * returns OCSP_LOOKUP_FAIL when the response is bad and 0 otherwise. */ int CheckOcspResponse(WOLFSSL_OCSP *ocsp, byte *response, int responseSz, WOLFSSL_BUFFER_INFO *responseBuffer, CertStatus *status, - OcspEntry *entry, OcspRequest *ocspRequest, void* heap) + OcspEntry *entry, OcspRequest *ocspRequest, void* heap, + WOLFSSL* ssl) { #ifdef WOLFSSL_SMALL_STACK CertStatus* newStatus; @@ -373,10 +374,11 @@ int CheckOcspResponse(WOLFSSL_OCSP *ocsp, byte *response, int responseSz, InitOcspResponse(ocspResponse, newSingle, newStatus, response, (word32)responseSz, ocsp->cm->heap); #if defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2) && !defined(NO_TLS) - if (ocspRequest != NULL && ocspRequest->ssl != NULL && - TLSX_CSR2_IsMulti(((WOLFSSL*)ocspRequest->ssl)->extensions)) { - ocspResponse->pendingCAs = TLSX_CSR2_GetPendingSigners(((WOLFSSL*)ocspRequest->ssl)->extensions); + if (ssl != NULL && TLSX_CSR2_IsMulti(ssl->extensions)) { + ocspResponse->pendingCAs = TLSX_CSR2_GetPendingSigners(ssl->extensions); } +#else + (void)ssl; #endif ret = OcspResponseDecode(ocspResponse, ocsp->cm, ocsp->cm->heap, 0, 0); if (ret != 0) { @@ -480,7 +482,7 @@ int CheckOcspResponse(WOLFSSL_OCSP *ocsp, byte *response, int responseSz, #define OCSP_MAX_REQUEST_SZ 2048 #endif int CheckOcspRequest(WOLFSSL_OCSP* ocsp, OcspRequest* ocspRequest, - buffer* responseBuffer, void* heap) + buffer* responseBuffer, WOLFSSL* ssl) { OcspEntry* entry = NULL; CertStatus* status = NULL; @@ -491,8 +493,11 @@ int CheckOcspRequest(WOLFSSL_OCSP* ocsp, OcspRequest* ocspRequest, const char* url = NULL; int urlSz = 0; int ret = -1; - WOLFSSL* ssl; void* ioCtx; + /* Hint for responseBuffer only, which the caller frees against the same + * connection, so take it from there rather than have every caller pass a + * heap it has to keep in step with its own free. */ + void* heap = (ssl != NULL) ? ssl->heap : NULL; WOLFSSL_ENTER("CheckOcspRequest"); @@ -518,8 +523,7 @@ int CheckOcspRequest(WOLFSSL_OCSP* ocsp, OcspRequest* ocspRequest, responseBuffer->buffer = NULL; } - /* get SSL and IOCtx */ - ssl = (WOLFSSL*)ocspRequest->ssl; + /* get IOCtx */ ioCtx = (ssl && ssl->ocspIOCtx != NULL) ? ssl->ocspIOCtx : ocsp->cm->ocspIOCtx; @@ -565,7 +569,7 @@ int CheckOcspRequest(WOLFSSL_OCSP* ocsp, OcspRequest* ocspRequest, if (responseSz >= 0 && response) { ret = CheckOcspResponse(ocsp, response, responseSz, responseBuffer, status, - entry, ocspRequest, heap); + entry, ocspRequest, heap, ssl); } if (response != NULL && ocsp->cm->ocspRespFreeCb) diff --git a/src/ssl_certman.c b/src/ssl_certman.c index 5fadd9fbe13..500216863d0 100644 --- a/src/ssl_certman.c +++ b/src/ssl_certman.c @@ -2520,7 +2520,7 @@ int wolfSSL_CertManagerCheckOCSPResponse(WOLFSSL_CERT_MANAGER *cm, if ((ret == 0) && cm->ocspEnabled) { /* Check OCSP response with OCSP object from certificate manager. */ ret = CheckOcspResponse(cm->ocsp, response, responseSz, responseBuffer, - status, entry, ocspRequest, NULL); + status, entry, ocspRequest, NULL, NULL); } return (ret == 0) ? WOLFSSL_SUCCESS : ret; diff --git a/src/tls.c b/src/tls.c index be43262d528..443871b28b8 100644 --- a/src/tls.c +++ b/src/tls.c @@ -3682,7 +3682,6 @@ int ProcessChainOCSPRequest(WOLFSSL* ssl) buffer der; int i = 1; int ret = 0; - byte ctxOwnsRequest = 0; /* use certChain if available, otherwise use peer certificate */ chain = ssl->buffers.certChain; @@ -3722,22 +3721,12 @@ int ProcessChainOCSPRequest(WOLFSSL* ssl) request = &csr->request.ocsp[i]; if (ret == 0) { ret = CreateOcspRequest(ssl, request, cert, - der.buffer, der.length, &ctxOwnsRequest); - if (ctxOwnsRequest) { - wolfSSL_Mutex* ocspLock = - &SSL_CM(ssl)->ocsp_stapling->ocspLock; - if (wc_LockMutex(ocspLock) == 0) { - /* the request is ours */ - ssl->ctx->certOcspRequest = NULL; - } - wc_UnLockMutex(ocspLock); - } + der.buffer, der.length); } if (ret == 0) { - request->ssl = ssl; ret = CheckOcspRequest(SSL_CM(ssl)->ocsp_stapling, - request, &csr->responses[i], ssl->heap); + request, &csr->responses[i], ssl); /* Suppressing soft-fail responder errors. OCSP_CERT_REVOKED * is an explicit positive assertion of revocation and must * not be ignored. OCSP_NO_URL just means there is no @@ -4020,9 +4009,8 @@ int TLSX_CSR_ForceRequest(WOLFSSL* ssl) case WOLFSSL_CSR_OCSP: if (SSL_CM(ssl)->ocspEnabled) { int ret; - csr->request.ocsp[0].ssl = ssl; ret = CheckOcspRequest(SSL_CM(ssl)->ocsp, - &csr->request.ocsp[0], NULL, NULL); + &csr->request.ocsp[0], NULL, ssl); /* This is the client's fallback leaf lookup on the * verification instance, so honor the no-responder policy * just like the non-stapling leaf path. Default stays @@ -4574,9 +4562,9 @@ int TLSX_CSR2_ForceRequest(WOLFSSL* ssl) case WOLFSSL_CSR2_OCSP_MULTI: if (SSL_CM(ssl)->ocspEnabled && csr2->requests >= 1) { int ret; - csr2->request.ocsp[csr2->requests-1].ssl = ssl; ret = CheckOcspRequest(SSL_CM(ssl)->ocsp, - &csr2->request.ocsp[csr2->requests-1], NULL, NULL); + &csr2->request.ocsp[csr2->requests-1], + NULL, ssl); /* This is the client's fallback leaf lookup on the * verification instance, so honor the no-responder policy * just like the non-stapling leaf path. Default stays diff --git a/src/tls13.c b/src/tls13.c index 7674cb7f24c..4ae3cce85ff 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -9634,6 +9634,7 @@ 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 @@ -9711,10 +9712,13 @@ static int SetupOcspResp(WOLFSSL* ssl) } } request = &csr->request.ocsp[0]; - ret = CreateOcspResponse(ssl, &request, &csr->responses[0]); - if (request != &csr->request.ocsp[0] && - ssl->buffers.weOwnCert) { - /* request will be allocated in CreateOcspResponse() */ + 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) { + /* 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 395a87574ea..9dfd0dd7d40 100644 --- a/tests/api.c +++ b/tests/api.c @@ -38851,6 +38851,7 @@ TEST_CASE testCases[] = { TEST_DECL(test_ocsp_cert_unknown_crl_fallback_nonleaf), TEST_DECL(test_ocsp_no_url_policy), TEST_DECL(test_tls13_nonblock_ocsp_low_mfl), + TEST_DECL(test_ocsp_ctx_request_cache), TEST_DECL(test_ocsp_responder), TEST_DECL(test_wolfIO_DecodeUrl_crlf_reject), TEST_TLS_DECLS, diff --git a/tests/api/test_ocsp.c b/tests/api/test_ocsp.c index dca2e1c5a8a..a77969891a8 100644 --- a/tests/api/test_ocsp.c +++ b/tests/api/test_ocsp.c @@ -20,6 +20,7 @@ */ #include +#include #include #include @@ -1845,6 +1846,204 @@ int test_tls13_nonblock_ocsp_low_mfl(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. */ +#if defined(HAVE_OCSP) && defined(HAVE_CERTIFICATE_STATUS_REQUEST) && \ + 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) + +/* Number of times the server's OCSP IO callback has been called. */ +static int test_ocsp_ctx_request_cache_cb_cnt; +/* The encoded request seen by the first call, kept to compare later ones + * against. 512 bytes is well past the size of an OCSP request for one cert. */ +static byte test_ocsp_ctx_request_cache_first[512]; +static int test_ocsp_ctx_request_cache_firstSz; +/* Set when a later call encoded something other than what the first did. */ +static int test_ocsp_ctx_request_cache_differs; + +/* Answers with the canned good response for server1, so that the stapling path + * runs to completion and the ownership decision it makes about the request is + * actually acted on. The encoded request is kept so the caller can tell which + * OcspRequest object produced it. + */ +static int test_ocsp_ctx_request_cache_io_cb(void* ioCtx, const char* url, + int urlSz, unsigned char* req, int reqSz, unsigned char** respBuf) +{ + (void)ioCtx; + (void)url; + (void)urlSz; + + if (test_ocsp_ctx_request_cache_cb_cnt == 0) { + if (req != NULL && reqSz > 0 && + reqSz <= (int)sizeof(test_ocsp_ctx_request_cache_first)) { + XMEMCPY(test_ocsp_ctx_request_cache_first, req, (size_t)reqSz); + test_ocsp_ctx_request_cache_firstSz = reqSz; + } + } + else if (test_ocsp_ctx_request_cache_firstSz > 0) { + test_ocsp_ctx_request_cache_differs = + (reqSz != test_ocsp_ctx_request_cache_firstSz) || + (XMEMCMP(req, test_ocsp_ctx_request_cache_first, + (size_t)reqSz) != 0); + } + + test_ocsp_ctx_request_cache_cb_cnt++; + + /* Static blob - the NULL free callback registered alongside this one keeps + * it from being freed. */ + *respBuf = (unsigned char*)resp_server1_cert; + return (int)sizeof(resp_server1_cert); +} + +/* The leaf OCSP request that stapling builds is cached on the WOLFSSL_CTX, and + * every later connection on that CTX reuses it. The CTX owns it from that point + * on and frees it in wolfSSL_CTX_free(), so no connection may free it. + * + * Runs three handshakes over one CTX pair: + * pass 0 - builds the request and publishes it on the CTX, + * pass 1 - reuses the published one, + * pass 2 - reuses it again, after it has been marked so that reuse can be + * told apart from a rebuild off the same certificate. + * + * The responder answers with a good response on every pass, so stapling runs + * all the way through and the ownership decision each connection makes about + * the request is actually acted on. That is what puts the rules themselves + * under test: a connection that frees the cached request shows up as a use + * after free on the next pass and a double free at CTX teardown, both of which + * the ASan CI job turns into a failure. + */ +int test_ocsp_ctx_request_cache(void) +{ + EXPECT_DECLS; + WOLFSSL_CTX* ctx_c = NULL; + WOLFSSL_CTX* ctx_s = NULL; + WOLFSSL* ssl_c = NULL; + WOLFSSL* ssl_s = NULL; + OcspRequest* cached = 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; + + /* Build the CTX pair on its own first: the certificate has to be in place + * before any WOLFSSL is made from the CTX, since each one latches the + * certificate buffer at wolfSSL_new(). */ + 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); + + /* server1 is the certificate the canned OCSP response answers for, sent + * with its intermediate so the client can build a path to the root. */ + 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); + /* The server needs the issuing CAs to verify the response it staples. */ + 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); + /* The client needs the intermediate too: it has to verify the responder + * certificate inside the stapled response, and a build whose default verify + * mode is NONE (OPENSSL_COMPATIBLE_DEFAULTS) never registers the chain + * certificates the handshake carried. */ + 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); + /* The test certificate carries no AuthInfo, so point the lookup at a dummy + * responder to get past the no-URL check. */ + 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); + /* NULL free callback: the response points at a static array. */ + 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++) { + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + /* The CTXs already exist, so this only makes the connection pair - all + * three handshakes run on the same CTXs, which is what puts the cache + * in play. */ + 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 < 2) { + /* A good response is stapled and accepted, so the handshake has to + * complete. */ + ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + } + else { + /* The marked request no longer matches the response, so this pass + * is only about which object got encoded. */ + (void)test_memio_do_handshake(ssl_c, ssl_s, 10, NULL); + } + + /* Every pass went out to the responder, so every pass really did run + * the stapling path. */ + ExpectIntEQ(test_ocsp_ctx_request_cache_cb_cnt, i + 1); + + if (i == 0) { + /* First pass builds the request and publishes it on the CTX. */ + ExpectNotNull(cached = ctx_s->certOcspRequest); + } + else { + /* Later passes reuse that one instead of publishing another, which + * is what leaves the CTX as the single owner. Reaching this with + * the object intact is the point: a connection that had freed it + * would have left a dangling pointer here. */ + ExpectPtrEq(ctx_s->certOcspRequest, cached); + } + + if (i == 1) { + /* Mark the cached request so the next pass can be told apart from + * one that rebuilt the request off the same certificate: both would + * otherwise encode byte for byte the same. */ + if (cached != NULL && cached->serial != NULL && + cached->serialSz > 0) { + cached->serial[0] = (byte)(cached->serial[0] ^ 0xFF); + } + } + else if (i == 2) { + /* It really was the cached object that went to the responder, not a + * fresh request that happens to be cached alongside it. */ + ExpectIntEQ(test_ocsp_ctx_request_cache_differs, 1); + } + + 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; +} +#endif + #if defined(HAVE_OCSP_RESPONDER) && defined(WOLFSSL_ASN_TEMPLATE) && \ !defined(NO_SHA) && !defined(NO_RSA) /* Structure to hold test configuration */ diff --git a/tests/api/test_ocsp.h b/tests/api/test_ocsp.h index 23b26551d15..0c95e07f17f 100644 --- a/tests/api/test_ocsp.h +++ b/tests/api/test_ocsp.h @@ -35,6 +35,7 @@ int test_ocsp_cert_unknown_crl_fallback(void); int test_ocsp_cert_unknown_crl_fallback_nonleaf(void); int test_ocsp_no_url_policy(void); int test_tls13_nonblock_ocsp_low_mfl(void); +int test_ocsp_ctx_request_cache(void); int test_ocsp_responder(void); int test_ocsp_ancestor_responder_rejected(void); int test_wolfIO_DecodeUrl_crlf_reject(void); diff --git a/tests/api/test_tls13.c b/tests/api/test_tls13.c index f0cb857df2d..c5fe3189c36 100644 --- a/tests/api/test_tls13.c +++ b/tests/api/test_tls13.c @@ -8708,6 +8708,7 @@ int test_tls13_KeyUpdate_sender_limit(void) #if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \ defined(WOLFSSL_TLS13) && defined(WOLFSSL_POST_HANDSHAKE_AUTH) && \ defined(HAVE_CERTIFICATE_STATUS_REQUEST) && defined(HAVE_OCSP) && \ + defined(KEEP_PEER_CERT) && \ !defined(NO_CERTS) && !defined(NO_RSA) && \ !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER) /* Mock OCSP I/O callback that yields no response. It lets stapling be enabled @@ -8750,6 +8751,10 @@ static void test_pha_ocsp_resp_free_cb(void* ioCtx, unsigned char* resp) * without client authentication, followed by a server-initiated PHA * exchange. The server expects to receive (and verify) the client * certificate even though no OCSP staple is supplied. + * + * KEEP_PEER_CERT is part of the guard because the check for the received + * client certificate uses wolfSSL_get_peer_certificate(), which is only + * built when that macro is defined. */ int test_tls13_pha_status_request(void) { @@ -8757,6 +8762,7 @@ int test_tls13_pha_status_request(void) #if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \ defined(WOLFSSL_TLS13) && defined(WOLFSSL_POST_HANDSHAKE_AUTH) && \ defined(HAVE_CERTIFICATE_STATUS_REQUEST) && defined(HAVE_OCSP) && \ + defined(KEEP_PEER_CERT) && \ !defined(NO_CERTS) && !defined(NO_RSA) && \ !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER) struct test_memio_ctx test_ctx; diff --git a/wolfssl/internal.h b/wolfssl/internal.h index da08ac117a7..3b1ce7c8d71 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -3489,8 +3489,7 @@ WOLFSSL_LOCAL int ProcessChainOCSPRequest(WOLFSSL* ssl); #if defined(HAVE_CERTIFICATE_STATUS_REQUEST) || \ defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2) WOLFSSL_LOCAL int CreateOcspRequest(WOLFSSL* ssl, OcspRequest* request, - DecodedCert* cert, byte* certData, word32 length, - byte *ctxOwnsRequest); + DecodedCert* cert, byte* certData, word32 length); #endif /** Certificate Status Request v2 - RFC 6961 */ #ifdef HAVE_CERTIFICATE_STATUS_REQUEST_V2 @@ -7045,7 +7044,7 @@ 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); + buffer* response, byte* ctxOwnsRequest); #endif #if defined(HAVE_SECURE_RENEGOTIATION) && \ !defined(NO_WOLFSSL_SERVER) diff --git a/wolfssl/ocsp.h b/wolfssl/ocsp.h index 054eb7103a7..e652584d897 100644 --- a/wolfssl/ocsp.h +++ b/wolfssl/ocsp.h @@ -68,12 +68,12 @@ WOLFSSL_LOCAL int CheckCertOCSP_ex(WOLFSSL_OCSP* ocsp, DecodedCert* cert, WOLFSSL* ssl); WOLFSSL_LOCAL int CheckOcspRequest(WOLFSSL_OCSP* ocsp, OcspRequest* ocspRequest, WOLFSSL_BUFFER_INFO* responseBuffer, - void* heap); + WOLFSSL* ssl); WOLFSSL_LOCAL int OcspNoUrlPolicy(WOLFSSL_CERT_MANAGER* cm); WOLFSSL_LOCAL int CheckOcspResponse(WOLFSSL_OCSP *ocsp, byte *response, int responseSz, WOLFSSL_BUFFER_INFO *responseBuffer, CertStatus *status, OcspEntry *entry, OcspRequest *ocspRequest, - void* heap); + void* heap, WOLFSSL* ssl); #ifndef CheckOcspResponder WOLFSSL_LOCAL int CheckOcspResponder(OcspResponse *bs, byte* subjectNameHash, diff --git a/wolfssl/wolfcrypt/asn.h b/wolfssl/wolfcrypt/asn.h index 8259f0861f9..9da78269d36 100644 --- a/wolfssl/wolfcrypt/asn.h +++ b/wolfssl/wolfcrypt/asn.h @@ -3099,7 +3099,6 @@ struct OcspRequest { byte nonce[MAX_OCSP_NONCE_SZ]; int nonceSz; void* heap; - void* ssl; }; WOLFSSL_LOCAL void InitOcspResponse(OcspResponse* resp, OcspEntry* single, From d5893aa9fd1e2671b31574af46808471eb13e2d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Fri, 31 Jul 2026 12:45:57 +0200 Subject: [PATCH 3/4] Initialize cert manager CRL and OCSP objects before publishing The lazy creation paths in wolfSSL_CertManagerEnableCRL, wolfSSL_CertManagerEnableOCSP and wolfSSL_CertManagerEnableOCSPStapling stored the freshly allocated object in the shared certificate manager before zeroing and initializing it. A certificate manager is shared by every WOLFSSL created from a CTX, so another thread could observe the non-NULL pointer and operate on uninitialized memory, for example by taking crl->crlLock before InitCRL had created it. Build each object in a local, initialize it, and store it in the certificate manager only on success. The CRL lookup callback is set on the local as well, so a thread that picks the object up cannot find it without one and fall back to CRL_MISSING. Serialize the creation with caLock and re-check the pointer after locking, so that two concurrent Enable calls cannot both allocate and leak one of the objects. This does not make every writer of the pointer safe: wolfSSL_X509_STORE_add_crl() still publishes cm->crl with no lock, and readers observe it without one. Dispose of a half-built object after releasing caLock rather than under it. Neither free can actually block here: InitCRL() sets tid to INVALID_THREAD_VAL before any of its failure returns so FreeCRL() skips the monitor join, and FreeOCSP() takes no lock at all. The point is to keep the critical section down to the decision of what to publish, and to keep caLock out of the CRL free path as a rule: FreeCRL() on a published object joins the CRL monitor thread, which takes crlLock, while the verification path already takes crlLock (CheckCertCRLList()) before caLock (GetCA()). Fixes F-7235. --- doc/dox_comments/header_files/ssl.h | 3 + src/ssl_certman.c | 207 +++++++++++++++++++++------- 2 files changed, 159 insertions(+), 51 deletions(-) diff --git a/doc/dox_comments/header_files/ssl.h b/doc/dox_comments/header_files/ssl.h index a6bf5c3fe8c..d9f4ca3f26e 100644 --- a/doc/dox_comments/header_files/ssl.h +++ b/doc/dox_comments/header_files/ssl.h @@ -11366,6 +11366,7 @@ int wolfSSL_SetOCSP_Cb(WOLFSSL* ssl, CbOCSPIO ioCb, CbOCSPRespFree respFreeCb, memory during execution of the function. \return SSL_FAILURE returned if the crl member of the WOLFSSL_CERT_MANAGER fails to initialize correctly. + \return BAD_MUTEX_E returned if locking the certificate manager failed. \return NOT_COMPILED_IN wolfSSL was not compiled with the HAVE_CRL option. \param ctx a pointer to a WOLFSSL_CTX structure, created using @@ -11495,6 +11496,7 @@ int wolfSSL_CTX_SetCRL_Cb(WOLFSSL_CTX* ctx, CbMissingCRL cb); \return SSL_SUCCESS is returned upon success. \return SSL_FAILURE is returned upon failure. + \return BAD_MUTEX_E returned if locking the certificate manager failed. \return NOT_COMPILED_IN is returned when this function has been called, but OCSP support was not enabled when wolfSSL was compiled. @@ -11617,6 +11619,7 @@ int wolfSSL_CTX_SetOCSP_Cb(WOLFSSL_CTX* ctx, \return MEMORY_E returned if there was an issue allocating memory. \return SSL_FAILURE returned if the initialization of the OCSP structure failed. + \return BAD_MUTEX_E returned if locking the certificate manager failed. \return NOT_COMPILED_IN returned if wolfSSL was not compiled with HAVE_CERTIFICATE_STATUS_REQUEST option. diff --git a/src/ssl_certman.c b/src/ssl_certman.c index 500216863d0..85b89428a8f 100644 --- a/src/ssl_certman.c +++ b/src/ssl_certman.c @@ -1744,6 +1744,7 @@ int CM_GetCertCacheMemSize(WOLFSSL_CERT_MANAGER* cm) * @return WOLFSSL_SUCCESS on success. * @return WOLFSSL_FAILURE when initializing the CRL object fails. * @return BAD_FUNC_ARG when cm is NULL. + * @return BAD_MUTEX_E when locking the certificate manager fails. * @return MEMORY_E when dynamic memory allocation fails. * @return NOT_COMPILED_IN when the CRL feature is disabled. */ @@ -1777,30 +1778,75 @@ int wolfSSL_CertManagerEnableCRL(WOLFSSL_CERT_MANAGER* cm, int options) #else /* Create CRL object if not present. */ if (cm->crl == NULL) { - /* Allocate memory for CRL object. */ - cm->crl = (WOLFSSL_CRL*)XMALLOC(sizeof(WOLFSSL_CRL), cm->heap, - DYNAMIC_TYPE_CRL); - if (cm->crl == NULL) { - ret = MEMORY_E; + WOLFSSL_CRL* crl; + /* Half-built object to dispose of once the lock is released. */ + WOLFSSL_CRL* crlFree = NULL; + + /* Serialize creation so that concurrent callers cannot both + * allocate a CRL object. */ + if (wc_LockMutex(&cm->caLock) != 0) { + WOLFSSL_MSG("wc_LockMutex on caLock failed"); + ret = BAD_MUTEX_E; } - if (ret == WOLFSSL_SUCCESS) { - /* Reset fields of CRL object. */ - XMEMSET(cm->crl, 0, sizeof(WOLFSSL_CRL)); - /* Initialize CRL object. */ - if (InitCRL(cm->crl, cm) != 0) { - WOLFSSL_MSG("Init CRL failed"); - /* Dispose of CRL object - indicating dynamically allocated. - */ - FreeCRL(cm->crl, 1); - cm->crl = NULL; - ret = WOLFSSL_FAILURE; + else { + /* Another thread may have created the object already. */ + if (cm->crl == NULL) { + /* Allocate memory for CRL object. */ + crl = (WOLFSSL_CRL*)XMALLOC(sizeof(WOLFSSL_CRL), cm->heap, + DYNAMIC_TYPE_CRL); + if (crl == NULL) { + ret = MEMORY_E; + } + else { + /* Reset fields of CRL object. */ + XMEMSET(crl, 0, sizeof(WOLFSSL_CRL)); + /* Initialize CRL object. */ + if (InitCRL(crl, cm) != 0) { + WOLFSSL_MSG("Init CRL failed"); + /* Nothing here needs caLock, so dispose of the + * object once it has been released and keep the + * critical section to the publish decision. + * + * This particular free cannot block: InitCRL() + * sets tid to INVALID_THREAD_VAL before any of its + * failure returns, so FreeCRL() skips the monitor + * join and takes no lock. Keep frees out of caLock + * anyway, because FreeCRL() on a published object + * does join the CRL monitor thread, which needs + * crlLock, and the verification path already takes + * crlLock (CheckCertCRLList()) before caLock + * (GetCA()). Holding caLock across a CRL free + * would be the wrong order. */ + crlFree = crl; + ret = WOLFSSL_FAILURE; + } + else { + #if defined(HAVE_CRL_IO) && defined(USE_WOLFSSL_IO) + /* Set before publishing: a thread that picks the + * object up must not find it without a lookup + * callback and fall back to CRL_MISSING. */ + crl->crlIOCb = EmbedCrlLookup; + #endif + /* Publish only once fully initialized so that + * other threads never see a half-built object. */ + cm->crl = crl; + } + } + } + wc_UnLockMutex(&cm->caLock); + + if (crlFree != NULL) { + /* Indicate dynamically allocated. */ + FreeCRL(crlFree, 1); } } } if (ret == WOLFSSL_SUCCESS) { #if defined(HAVE_CRL_IO) && defined(USE_WOLFSSL_IO) - /* Use built-in callback to lookup CRL from URL. */ + /* Redundant for an object created above, but the CRL object can + * also have been published by wolfSSL_X509_STORE_add_crl(), which + * does not set the lookup callback. */ cm->crl->crlIOCb = EmbedCrlLookup; #endif #if defined(OPENSSL_COMPATIBLE_DEFAULTS) @@ -2165,8 +2211,9 @@ int wolfSSL_CertManagerLoadCRLFile(WOLFSSL_CERT_MANAGER* cm, const char* file, * WOLFSSL_OCSP_CHECKALL, * WOLFSSL_OCSP_FAIL_IF_NOT_SUPPORTED. * @return WOLFSSL_SUCCESS on success. - * @return 0 when initializing the OCSP object fails. + * @return WOLFSSL_FAILURE when initializing the OCSP object fails. * @return BAD_FUNC_ARG when cm is NULL. + * @return BAD_MUTEX_E when locking the certificate manager fails. * @return MEMORY_E when dynamic memory allocation fails. * @return NOT_COMPILED_IN when the OCSP feature is disabled. */ @@ -2192,23 +2239,51 @@ int wolfSSL_CertManagerEnableOCSP(WOLFSSL_CERT_MANAGER* cm, int options) if (ret == WOLFSSL_SUCCESS) { /* Check whether OCSP object is available. */ if (cm->ocsp == NULL) { - /* Allocate memory for OCSP object. */ - cm->ocsp = (WOLFSSL_OCSP*)XMALLOC(sizeof(WOLFSSL_OCSP), cm->heap, - DYNAMIC_TYPE_OCSP); - if (cm->ocsp == NULL) { - ret = MEMORY_E; + WOLFSSL_OCSP* ocsp; + /* Half-built object to dispose of once the lock is released. */ + WOLFSSL_OCSP* ocspFree = NULL; + + /* Serialize creation so that concurrent callers cannot both + * allocate an OCSP object. */ + if (wc_LockMutex(&cm->caLock) != 0) { + WOLFSSL_MSG("wc_LockMutex on caLock failed"); + ret = BAD_MUTEX_E; } - if (ret == WOLFSSL_SUCCESS) { - /* Reset the fields of the OCSP object. */ - XMEMSET(cm->ocsp, 0, sizeof(WOLFSSL_OCSP)); - /* Initialize the OCSP object. */ - if (InitOCSP(cm->ocsp, cm) != 0) { - WOLFSSL_MSG("Init OCSP failed"); - /* Dispose of OCSP object - indicating dynamically - * allocated. */ - FreeOCSP(cm->ocsp, 1); - cm->ocsp = NULL; - ret = 0; + else { + /* Another thread may have created the object already. */ + if (cm->ocsp == NULL) { + /* Allocate memory for OCSP object. */ + ocsp = (WOLFSSL_OCSP*)XMALLOC(sizeof(WOLFSSL_OCSP), + cm->heap, DYNAMIC_TYPE_OCSP); + if (ocsp == NULL) { + ret = MEMORY_E; + } + else { + /* Reset the fields of the OCSP object. */ + XMEMSET(ocsp, 0, sizeof(WOLFSSL_OCSP)); + /* Initialize the OCSP object. */ + if (InitOCSP(ocsp, cm) != 0) { + WOLFSSL_MSG("Init OCSP failed"); + /* FreeOCSP() takes no lock, so unlike the CRL case + * above this cannot deadlock. Dispose of the object + * after the lock has been released anyway, to keep + * the critical section down to the decision of + * what to publish. */ + ocspFree = ocsp; + ret = WOLFSSL_FAILURE; + } + else { + /* Publish only once fully initialized so that + * other threads never see a half-built object. */ + cm->ocsp = ocsp; + } + } + } + wc_UnLockMutex(&cm->caLock); + + if (ocspFree != NULL) { + /* Indicate dynamically allocated. */ + FreeOCSP(ocspFree, 1); } } } @@ -2274,8 +2349,9 @@ int wolfSSL_CertManagerDisableOCSP(WOLFSSL_CERT_MANAGER* cm) * WOLFSSL_OCSP_URL_OVERRIDE, WOLFSSL_OCSP_NO_NONCE, * WOLFSSL_OCSP_CHECKALL. * @return WOLFSSL_SUCCESS on success. - * @return 0 when initializing the OCSP stapling object fails. + * @return WOLFSSL_FAILURE when initializing the OCSP stapling object fails. * @return BAD_FUNC_ARG when cm is NULL. + * @return BAD_MUTEX_E when locking the certificate manager fails. * @return MEMORY_E when dynamic memory allocation fails. * @return NOT_COMPILED_IN when the OCSP stapling feature is disabled. */ @@ -2301,23 +2377,52 @@ int wolfSSL_CertManagerEnableOCSPStapling(WOLFSSL_CERT_MANAGER* cm) if (ret == WOLFSSL_SUCCESS) { /* Check whether OCSP object is available. */ if (cm->ocsp_stapling == NULL) { - /* Allocate memory for OCSP stapling object. */ - cm->ocsp_stapling = (WOLFSSL_OCSP*)XMALLOC(sizeof(WOLFSSL_OCSP), - cm->heap, DYNAMIC_TYPE_OCSP); - if (cm->ocsp_stapling == NULL) { - ret = MEMORY_E; + WOLFSSL_OCSP* ocsp; + /* Half-built object to dispose of once the lock is released. */ + WOLFSSL_OCSP* ocspFree = NULL; + + /* Serialize creation so that concurrent callers cannot both + * allocate an OCSP stapling object. */ + if (wc_LockMutex(&cm->caLock) != 0) { + WOLFSSL_MSG("wc_LockMutex on caLock failed"); + ret = BAD_MUTEX_E; } - if (ret == WOLFSSL_SUCCESS) { - /* Reset the fields of the OCSP object. */ - XMEMSET(cm->ocsp_stapling, 0, sizeof(WOLFSSL_OCSP)); - /* Initialize the OCSP stapling object. */ - if (InitOCSP(cm->ocsp_stapling, cm) != 0) { - WOLFSSL_MSG("Init OCSP failed"); - /* Dispose of OCSP stapling object - indicating dynamically - * allocated. */ - FreeOCSP(cm->ocsp_stapling, 1); - cm->ocsp_stapling = NULL; - ret = 0; + else { + /* Another thread may have created the object already. */ + if (cm->ocsp_stapling == NULL) { + /* Allocate memory for OCSP stapling object. */ + ocsp = (WOLFSSL_OCSP*)XMALLOC(sizeof(WOLFSSL_OCSP), + cm->heap, DYNAMIC_TYPE_OCSP); + if (ocsp == NULL) { + ret = MEMORY_E; + } + else { + /* Reset the fields of the OCSP object. */ + XMEMSET(ocsp, 0, sizeof(WOLFSSL_OCSP)); + /* Initialize the OCSP stapling object. */ + if (InitOCSP(ocsp, cm) != 0) { + WOLFSSL_MSG("Init OCSP failed"); + /* FreeOCSP() takes no lock, so unlike the CRL case + * in wolfSSL_CertManagerEnableCRL() this cannot + * deadlock. Dispose of the object after the lock + * has been released anyway, to keep the critical + * section down to the decision of what to + * publish. */ + ocspFree = ocsp; + ret = WOLFSSL_FAILURE; + } + else { + /* Publish only once fully initialized so that + * other threads never see a half-built object. */ + cm->ocsp_stapling = ocsp; + } + } + } + wc_UnLockMutex(&cm->caLock); + + if (ocspFree != NULL) { + /* Indicate dynamically allocated. */ + FreeOCSP(ocspFree, 1); } } } From 3e7f1c8172b54bf741b315fb9f10a7472039f08d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Fri, 31 Jul 2026 22:22:26 +0200 Subject: [PATCH 4/4] Do not free CRL and OCSP objects whose initialization failed wolfSSL_CertManagerEnableCRL, wolfSSL_CertManagerEnableOCSP, wolfSSL_CertManagerEnableOCSPStapling and wolfSSL_d2i_X509_CRL called FreeCRL() or FreeOCSP() on an object that InitCRL() or InitOCSP() had just failed to initialize. The other callers of those two functions, wolfSSL_X509_crl_new(), wolfSSL_X509_CRL_new(), SwapLists() and wc_NewOCSP(), only dispose of the memory, which is the contract the init functions are written to. Freeing the object is not harmless. InitCRL() releases the read/write lock and the condition variable itself before returning an error from the reference count path, so FreeCRL() destroys both a second time, and on the two earlier failure paths it destroys primitives that were never created at all. InitOCSP() only fails when it cannot create its mutex, which FreeOCSP() then destroys. Destroying a synchronization object twice, or one that was never created, is undefined behaviour on every platform and a real double free on the ports where the object holds a handle to allocated storage, such as vSemaphoreDelete() on FreeRTOS and CloseHandle() for Windows condition variables. Dispose of only the memory in all four places. In the certificate manager that also removes the reason to defer the disposal until after caLock is released, so the extra local and the second free block go with it. Complete InitCRL()'s own cleanup while here: when wc_InitRwLock() fails it returned without releasing the condition variable it had created just above, leaking it for every caller. --- src/crl.c | 5 ++++ src/ssl_certman.c | 64 +++++++++++------------------------------------ src/x509.c | 6 +++++ 3 files changed, 26 insertions(+), 49 deletions(-) diff --git a/src/crl.c b/src/crl.c index 709af1c7210..db40bb0f647 100644 --- a/src/crl.c +++ b/src/crl.c @@ -86,6 +86,11 @@ int InitCRL(WOLFSSL_CRL* crl, WOLFSSL_CERT_MANAGER* cm) #endif if (wc_InitRwLock(&crl->crlLock) != 0) { WOLFSSL_MSG("Init Mutex failed"); + #ifdef HAVE_CRL_MONITOR + /* Undo the condition variable created above: a failed InitCRL() must + * leave nothing behind, since callers only free the memory. */ + wolfSSL_CondFree(&crl->cond); + #endif return BAD_MUTEX_E; } #ifdef OPENSSL_ALL diff --git a/src/ssl_certman.c b/src/ssl_certman.c index 85b89428a8f..0dc9beb723f 100644 --- a/src/ssl_certman.c +++ b/src/ssl_certman.c @@ -1779,8 +1779,6 @@ int wolfSSL_CertManagerEnableCRL(WOLFSSL_CERT_MANAGER* cm, int options) /* Create CRL object if not present. */ if (cm->crl == NULL) { WOLFSSL_CRL* crl; - /* Half-built object to dispose of once the lock is released. */ - WOLFSSL_CRL* crlFree = NULL; /* Serialize creation so that concurrent callers cannot both * allocate a CRL object. */ @@ -1803,21 +1801,11 @@ int wolfSSL_CertManagerEnableCRL(WOLFSSL_CERT_MANAGER* cm, int options) /* Initialize CRL object. */ if (InitCRL(crl, cm) != 0) { WOLFSSL_MSG("Init CRL failed"); - /* Nothing here needs caLock, so dispose of the - * object once it has been released and keep the - * critical section to the publish decision. - * - * This particular free cannot block: InitCRL() - * sets tid to INVALID_THREAD_VAL before any of its - * failure returns, so FreeCRL() skips the monitor - * join and takes no lock. Keep frees out of caLock - * anyway, because FreeCRL() on a published object - * does join the CRL monitor thread, which needs - * crlLock, and the verification path already takes - * crlLock (CheckCertCRLList()) before caLock - * (GetCA()). Holding caLock across a CRL free - * would be the wrong order. */ - crlFree = crl; + /* A failed InitCRL() has already released whatever + * it managed to take, so only the memory is left + * to dispose of. FreeCRL() would free the lock and + * the condition variable a second time. */ + XFREE(crl, cm->heap, DYNAMIC_TYPE_CRL); ret = WOLFSSL_FAILURE; } else { @@ -1834,11 +1822,6 @@ int wolfSSL_CertManagerEnableCRL(WOLFSSL_CERT_MANAGER* cm, int options) } } wc_UnLockMutex(&cm->caLock); - - if (crlFree != NULL) { - /* Indicate dynamically allocated. */ - FreeCRL(crlFree, 1); - } } } @@ -2240,8 +2223,6 @@ int wolfSSL_CertManagerEnableOCSP(WOLFSSL_CERT_MANAGER* cm, int options) /* Check whether OCSP object is available. */ if (cm->ocsp == NULL) { WOLFSSL_OCSP* ocsp; - /* Half-built object to dispose of once the lock is released. */ - WOLFSSL_OCSP* ocspFree = NULL; /* Serialize creation so that concurrent callers cannot both * allocate an OCSP object. */ @@ -2264,12 +2245,11 @@ int wolfSSL_CertManagerEnableOCSP(WOLFSSL_CERT_MANAGER* cm, int options) /* Initialize the OCSP object. */ if (InitOCSP(ocsp, cm) != 0) { WOLFSSL_MSG("Init OCSP failed"); - /* FreeOCSP() takes no lock, so unlike the CRL case - * above this cannot deadlock. Dispose of the object - * after the lock has been released anyway, to keep - * the critical section down to the decision of - * what to publish. */ - ocspFree = ocsp; + /* InitOCSP() only fails when it could not create + * the lock, so there is nothing to release but the + * memory. FreeOCSP() would destroy a mutex that was + * never initialized. */ + XFREE(ocsp, cm->heap, DYNAMIC_TYPE_OCSP); ret = WOLFSSL_FAILURE; } else { @@ -2280,11 +2260,6 @@ int wolfSSL_CertManagerEnableOCSP(WOLFSSL_CERT_MANAGER* cm, int options) } } wc_UnLockMutex(&cm->caLock); - - if (ocspFree != NULL) { - /* Indicate dynamically allocated. */ - FreeOCSP(ocspFree, 1); - } } } } @@ -2378,8 +2353,6 @@ int wolfSSL_CertManagerEnableOCSPStapling(WOLFSSL_CERT_MANAGER* cm) /* Check whether OCSP object is available. */ if (cm->ocsp_stapling == NULL) { WOLFSSL_OCSP* ocsp; - /* Half-built object to dispose of once the lock is released. */ - WOLFSSL_OCSP* ocspFree = NULL; /* Serialize creation so that concurrent callers cannot both * allocate an OCSP stapling object. */ @@ -2402,13 +2375,11 @@ int wolfSSL_CertManagerEnableOCSPStapling(WOLFSSL_CERT_MANAGER* cm) /* Initialize the OCSP stapling object. */ if (InitOCSP(ocsp, cm) != 0) { WOLFSSL_MSG("Init OCSP failed"); - /* FreeOCSP() takes no lock, so unlike the CRL case - * in wolfSSL_CertManagerEnableCRL() this cannot - * deadlock. Dispose of the object after the lock - * has been released anyway, to keep the critical - * section down to the decision of what to - * publish. */ - ocspFree = ocsp; + /* InitOCSP() only fails when it could not create + * the lock, so there is nothing to release but the + * memory. FreeOCSP() would destroy a mutex that was + * never initialized. */ + XFREE(ocsp, cm->heap, DYNAMIC_TYPE_OCSP); ret = WOLFSSL_FAILURE; } else { @@ -2419,11 +2390,6 @@ int wolfSSL_CertManagerEnableOCSPStapling(WOLFSSL_CERT_MANAGER* cm) } } wc_UnLockMutex(&cm->caLock); - - if (ocspFree != NULL) { - /* Indicate dynamically allocated. */ - FreeOCSP(ocspFree, 1); - } } } } diff --git a/src/x509.c b/src/x509.c index 3842b2feb60..1a5bdf0fa67 100644 --- a/src/x509.c +++ b/src/x509.c @@ -9480,6 +9480,12 @@ WOLFSSL_X509_CRL* wolfSSL_d2i_X509_CRL(WOLFSSL_X509_CRL** crl, ret = InitCRL(newcrl, NULL); if (ret < 0) { WOLFSSL_MSG("Init tmp CRL failed"); + /* A failed InitCRL() has already released whatever it took, + * so dispose of the memory here and keep this object away + * from the wolfSSL_X509_CRL_free() below, which would destroy + * the lock and the condition variable a second time. */ + XFREE(newcrl, NULL, DYNAMIC_TYPE_CRL); + newcrl = NULL; } else { ret = BufferLoadCRL(newcrl, in, len, WOLFSSL_FILETYPE_ASN1,