Fixes for OCSP stapling, cert manager, and certificate_status_request_v2 handling - #11027
Open
Frauschi wants to merge 4 commits into
Open
Fixes for OCSP stapling, cert manager, and certificate_status_request_v2 handling#11027Frauschi wants to merge 4 commits into
Frauschi wants to merge 4 commits into
Conversation
|
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
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
self-requested a review
July 31, 2026 15:09
philljj
reviewed
Jul 31, 2026
philljj
reviewed
Jul 31, 2026
philljj
self-requested a review
July 31, 2026 17:10
philljj
requested changes
Jul 31, 2026
philljj
left a comment
philljj
left a comment
Contributor
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three independent fixes:
EXT_NOT_ALLOWEDelsewhere), and align certificate checks withAddCA(). Pooled signers now useWOLFSSL_CHAIN_CA, sowolfSSL_CertManagerUnloadIntermediateCerts()can remove them. Fixes F-7227.OcspRequest.sslback-pointer that concurrent handshakes raced on, pass the connection explicitly, move publication of the CTX-cached request intoCreateOcspResponse()behind an explicitctxOwnsRequestflag, and key the cache lock offssl->ctx->cm. Plus a leak fix on theWOLFSSL_CSR2_OCSP_MULTIallocation-failure paths. Fixes F-7230 and F-7231.caLock; free a half-built object after the unlock, sinceFreeCRL()can join the monitor thread, which takescrlLockbeforecaLock. Fixes F-7235.ABI:
struct OcspRequestloses its trailingvoid* ssl. It is fully defined in an installed header and re-exported asOCSP_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 aBAD_MUTEX_Ereturn.Tests: new
test_TLSX_CSR2_tls13_msg_type_validationandtest_ocsp_ctx_request_cache, the latter run by a newocsp.ymljob in plain-stapling,--enable-all -DWOLFSSL_NO_COPY_CERT, and ASan builds. Zero failures across four local configurations; removing the!ctxOwnsRequestguard makes the cache test die under ASan, so it can actually fail.