Fixes for OCSP stapling, cert manager, and certificate_status_request_v2 handling by Frauschi · Pull Request #11027 · wolfSSL/wolfssl

Frauschi · 2026-07-31T11:20:38Z

Three independent fixes:

certificate_status_request_v2. Enforce RFC 8446 4.4.2.1 on the client (ClientHello only under TLS 1.3, EXT_NOT_ALLOWED elsewhere), and align certificate checks with AddCA(). Pooled signers now use WOLFSSL_CHAIN_CA, so wolfSSL_CertManagerUnloadIntermediateCerts() can remove them. Fixes F-7227.
OCSP stapling request ownership. Drop the shared OcspRequest.ssl back-pointer that concurrent handshakes raced on, pass the connection explicitly, move publication of the CTX-cached request into CreateOcspResponse() behind an explicit ctxOwnsRequest flag, and key the cache lock off ssl->ctx->cm. Plus a leak fix on the WOLFSSL_CSR2_OCSP_MULTI allocation-failure paths. Fixes F-7230 and F-7231.
Cert manager CRL/OCSP init. Build each object in a local and publish only once initialized, under caLock; free a half-built object after the unlock, since FreeCRL() can join the monitor thread, which takes crlLock before caLock. Fixes F-7235.

ABI: struct OcspRequest loses its trailing void* ssl. It is fully defined in an installed header and re-exported as OCSP_REQUEST, so this is a size change for unrebuilt consumers. No other offsets move; no in-tree consumer affected. wolfSSL_CertManagerEnable{CRL,OCSP,OCSPStapling}() gain a BAD_MUTEX_E return.

Tests: new test_TLSX_CSR2_tls13_msg_type_validation and test_ocsp_ctx_request_cache, the latter run by a new ocsp.yml job in plain-stapling, --enable-all -DWOLFSSL_NO_COPY_CERT, and ASan builds. Zero failures across four local configurations; removing the !ctxOwnsRequest guard makes the cache test die under ASan, so it can actually fail.

github-actions · 2026-07-31T11:43:25Z

MemBrowse Memory Report

gcc-arm-cortex-m4-rsa-only

FLASH: .text +128 B (+0.0%, 326,232 B / 1,048,576 B, total: 31% used)

linuxkm-standard

Data: __patchable_function_entries +8 B (+0.0%, 49,008 B)
No memory changes detected for:
gcc-arm-cortex-m0plus
gcc-arm-cortex-m3
gcc-arm-cortex-m4
gcc-arm-cortex-m4-baremetal
gcc-arm-cortex-m4-crypto-only
gcc-arm-cortex-m4-dtls13
gcc-arm-cortex-m4-min-ecc
gcc-arm-cortex-m4-openssl-compat
gcc-arm-cortex-m4-pkcs7
gcc-arm-cortex-m4-pq
gcc-arm-cortex-m4-sp-math
gcc-arm-cortex-m4-tls12
gcc-arm-cortex-m4-tls13
gcc-arm-cortex-m7
gcc-arm-cortex-m7-pq
gcc-arm-cortex-m7-tls13
linuxkm-pie
stm32-sim-stm32h753

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #11027

Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src

No new issues found in the changed files. ✅

philljj · 2026-07-31T15:28:27Z

    byte   nonce[MAX_OCSP_NONCE_SZ];
    int    nonceSz;
    void*  heap;
-    void*  ssl;


This is an installed header so this will be an ABI change. (it may be a necessary change, just making a note)

If we want to prevent an ABI change (if a SO version bump is not required otherwise in the next release anyway), we can re-add that. But technically the pointer is not used in the code anymore.

It looks like a justified change

Agreed it is one. Nothing reads the field any more, so I have left it removed and will get it into the release notes.

philljj · 2026-07-31T15:55:44Z

+     * further checks, so admission has to enforce the same capabilities AddCA()
+     * requires of a chain CA.
+     *
+     * A non-CA is skipped silently rather than reported as an error, because


It's not skipped silently, because it's logged right here in WOLFSSL_MSG?

Dropped the word - the WOLFSSL_MSG does log it. Only meant that it is not an error.

philljj · 2026-07-31T16:27:32Z

+                        /* Initialize CRL object. */
+                        if (InitCRL(crl, cm) != 0) {
+                            WOLFSSL_MSG("Init CRL failed");
+                            /* Dispose of it outside the critical section:


Calling FreeCRL() inside the critical section can deadlock?

If that's what the comment is referring to, then would prefer to state that more plainly.

Checked it: no, it cannot deadlock. crlLock -> caLock exists (CheckCertCRLList(), then GetCA()) but nothing goes the other way, and InitCRL() sets tid to INVALID_THREAD_VAL before every failure return, so FreeCRL() never reaches the monitor join. Comment and commit message corrected.

It did expose a real bug, now a separate commit: the three Enable* functions and wolfSSL_d2i_X509_CRL() called FreeCRL() / FreeOCSP() after a failed init, double-destroying the lock and condition variable. Fixing it dropped the deferred free with it.

philljj · 2026-07-31T16:28:07Z

+                        /* Initialize the OCSP object. */
+                        if (InitOCSP(ocsp, cm) != 0) {
+                            WOLFSSL_MSG("Init OCSP failed");
+                            /* Dispose of it outside the critical section, for


Calling FreeOCSP inside critical section can deadlock? Same comment as above.

Not the same reason - FreeOCSP() takes no lock. Both comments say that now, and the free is a plain XFREE at this point anyway.

philljj · 2026-07-31T16:56:06Z

+            wolfSSL_Mutex* ocspLock = GetCtxOcspLock(ssl);
+
+            if (ocspLock == NULL) {
+                WOLFSSL_MSG("No CTX OCSP lock, not caching the request");


Does this if or else if warrant returning an error?

No. ctxOwns stays 0, so this connection frees the request as usual and only cross-connection reuse is lost. The NULL branch is reachable, not padding: SSL_CM(ssl) can resolve through ssl->x509_store_pt. Both noted in the comment.

philljj · 2026-07-31T16:59:19Z

 #endif
 int CheckOcspRequest(WOLFSSL_OCSP* ocsp, OcspRequest* ocspRequest,
-                     buffer* responseBuffer, void* heap)
+                     buffer* responseBuffer, void* heap, WOLFSSL* ssl)


this heap argument seems redundant now.

Agreed, dropped. It was only the hint for responseBuffer->buffer, and every caller with a real one passed ssl->heap, so deriving it inside is identical at all six sites. CheckOcspResponse() keeps its heap arg for wc_CheckCertOcspResponse(), which has no ssl.

philljj · 2026-07-31T17:01:11Z

+                        /* Initialize the OCSP stapling object. */
+                        if (InitOCSP(ocsp, cm) != 0) {
+                            WOLFSSL_MSG("Init OCSP failed");
+                            /* Dispose of it outside the critical section, for


Not the same reason - FreeOCSP() takes no lock. Both comments say that now, and the free is a plain XFREE at this point anyway.

philljj · 2026-07-31T17:01:25Z

-        request->ssl = ssl;
        ret = CheckOcspRequest(SSL_CM(ssl)->ocsp_stapling, request, response,
-                               ssl->heap);
+                               ssl->heap, ssl);


Looks like we're passing ssl->heap, ssl) everywhere in CheckOcspRequest() now. The separate heap arg is redundant now.

Agreed, dropped. It was only the hint for responseBuffer->buffer, and every caller with a real one passed ssl->heap, so deriving it inside is identical at all six sites. CheckOcspResponse() keeps its heap arg for wc_CheckCertOcspResponse(), which has no ssl.

philljj · 2026-07-31T17:04:38Z

+ * 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,


Is there a way to interleave these tests to make them less serial?

The serial order is what is under test: publish, then clean reuse, then marked reuse. Run concurrently, a wrong ownership decision stops being observable. It is three memio handshakes, no sockets or sleeps.

philljj

Forgot to select Request changes

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.

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.

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.

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.

Frauschi self-assigned this Jul 31, 2026

Frauschi requested a review from wolfSSL-Fenrir-bot July 31, 2026 12:17

wolfSSL-Fenrir-bot reviewed Jul 31, 2026

View reviewed changes

Frauschi assigned wolfSSL-Bot and unassigned Frauschi Jul 31, 2026

philljj self-requested a review July 31, 2026 15:09

philljj reviewed Jul 31, 2026

View reviewed changes

philljj self-requested a review July 31, 2026 17:10

philljj requested changes Jul 31, 2026

View reviewed changes

philljj assigned Frauschi and unassigned wolfSSL-Bot Jul 31, 2026

Frauschi added 4 commits July 31, 2026 22:01

Frauschi force-pushed the fenrir_2 branch from 3e69857 to 3e7f1c8 Compare July 31, 2026 20:33

Frauschi requested a review from philljj July 31, 2026 21:06

Frauschi assigned philljj and unassigned Frauschi Jul 31, 2026

Uh oh!

Conversation

Frauschi commented Jul 31, 2026

Uh oh!

github-actions Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

MemBrowse Memory Report

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #11027

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philljj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Jul 31, 2026 •

edited

Loading