diff --git a/.github/workflows/ocsp.yml b/.github/workflows/ocsp.yml index 03123f608d..09cabf9a88 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/doc/dox_comments/header_files/ssl.h b/doc/dox_comments/header_files/ssl.h index a6bf5c3fe8..d9f4ca3f26 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/crl.c b/src/crl.c index 709af1c721..db40bb0f64 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/internal.c b/src/internal.c index 838fb57cd2..4dfadf9cdd 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 */ @@ -26371,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); @@ -26395,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); @@ -26420,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; - if (ssl == NULL || ocspRequest == NULL || response == NULL) + /* Zero the output before any exit so that a caller freeing response->buffer + * without checking the return code never acts on an indeterminate value. */ + if (response != NULL) + XMEMSET(response, 0, sizeof(*response)); + + if (ssl == NULL || ocspRequest == NULL || response == NULL || + ctxOwnsRequest == NULL) { 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; @@ -26451,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) { @@ -26475,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. @@ -26491,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; } @@ -27284,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; @@ -27374,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; @@ -27409,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)) { @@ -27432,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 */ @@ -27448,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; @@ -27462,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 @@ -27483,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 @@ -27512,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 7ddc6772a2..06b557490b 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 5fadd9fbe1..0dc9beb723 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,58 @@ 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; + + /* 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"); + /* 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 { + #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 (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 +2194,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,24 +2222,44 @@ 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; + + /* 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"); + /* 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 { + /* Publish only once fully initialized so that + * other threads never see a half-built object. */ + cm->ocsp = ocsp; + } + } } + wc_UnLockMutex(&cm->caLock); } } } @@ -2274,8 +2324,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,24 +2352,44 @@ 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; + + /* 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"); + /* 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 { + /* Publish only once fully initialized so that + * other threads never see a half-built object. */ + cm->ocsp_stapling = ocsp; + } + } } + wc_UnLockMutex(&cm->caLock); } } } @@ -2520,7 +2591,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 6647207776..443871b28b 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 @@ -4255,8 +4243,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) { @@ -4563,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 @@ -18698,9 +18697,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/src/tls13.c b/src/tls13.c index 7674cb7f24..4ae3cce85f 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/src/x509.c b/src/x509.c index 3842b2feb6..1a5bdf0fa6 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, diff --git a/tests/api.c b/tests/api.c index 14f10ca874..9dfd0dd7d4 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), @@ -38850,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 dca2e1c5a8..a77969891a 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 23b26551d1..0c95e07f17 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 f0cb857df2..c5fe3189c3 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/tests/api/test_tls_ext.c b/tests/api/test_tls_ext.c index 6453315505..ec9f5fe7fd 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 429216498e..4f100429fc 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); diff --git a/wolfssl/internal.h b/wolfssl/internal.h index da08ac117a..3b1ce7c8d7 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 054eb7103a..e652584d89 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 8259f0861f..9da78269d3 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,