From 65230fc816d7ce983e096101a38496018d0d7e68 Mon Sep 17 00:00:00 2001 From: Kareem Date: Wed, 29 Jul 2026 17:41:48 -0700 Subject: [PATCH 1/9] Only call XFREE in ShrinkOutputBuffer for dynamic buffers, and fully reset the buffer state. Thanks to Christos Papakonstantinou (Cantina Security) for the report. --- src/internal.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/internal.c b/src/internal.c index 975309ab591..a1a4d965993 100644 --- a/src/internal.c +++ b/src/internal.c @@ -11691,17 +11691,33 @@ static int wolfSSLReceive(WOLFSSL* ssl, byte* buf, word32 sz) } -/* Switch dynamic output buffer back to static, buffer is assumed clear */ +/* Switch dynamic output buffer back to static, discarding any pending output */ void ShrinkOutputBuffer(WOLFSSL* ssl) { WOLFSSL_MSG("Shrinking output buffer"); - XFREE(ssl->buffers.outputBuffer.buffer - ssl->buffers.outputBuffer.offset, - ssl->heap, DYNAMIC_TYPE_OUT_BUFFER); + /* Only the dynamic buffer is heap allocated. When dynamicFlag is clear the + * buffer already is the inline staticBuffer array, and in a + * LARGE_STATIC_BUFFERS build a pending flight can live there, so freeing + * unconditionally would pass an interior pointer of the WOLFSSL struct to + * XFREE. */ + if (ssl->buffers.outputBuffer.dynamicFlag) { + XFREE(ssl->buffers.outputBuffer.buffer - + ssl->buffers.outputBuffer.offset, + ssl->heap, DYNAMIC_TYPE_OUT_BUFFER); + } ssl->buffers.outputBuffer.buffer = ssl->buffers.outputBuffer.staticBuffer; ssl->buffers.outputBuffer.bufferSize = STATIC_BUFFER_LEN; ssl->buffers.outputBuffer.dynamicFlag = 0; ssl->buffers.outputBuffer.offset = 0; - /* idx and length are assumed to be 0. */ + /* Drop idx/length with the buffer they described. bufferSize shrinks to + * STATIC_BUFFER_LEN here, so leaving them set would break the + * idx + length <= bufferSize invariant that the rest of the output + * accounting relies on: CheckAvailableSize()'s word32 space computation + * would underflow instead of growing, and SendBuffered()/GetOutputBuffer() + * would address past the end of staticBuffer. Callers reaching this + * function with output still queued are abandoning that output. */ + ssl->buffers.outputBuffer.idx = 0; + ssl->buffers.outputBuffer.length = 0; } From 688cca5b44f512b910ca7b676976a1530d404b60 Mon Sep 17 00:00:00 2001 From: Kareem Date: Wed, 29 Jul 2026 17:44:28 -0700 Subject: [PATCH 2/9] Correct logic in X509StoreFreeObjList. Thanks to Christos Papakonstantinou (Cantina Security) for the report. --- src/x509_str.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/x509_str.c b/src/x509_str.c index 7bd2eb22f11..c0d99a1c3cb 100644 --- a/src/x509_str.c +++ b/src/x509_str.c @@ -1777,13 +1777,22 @@ static void X509StoreFreeObjList(WOLFSSL_X509_STORE* store, * the numAdded to the store >= is used when comparing to 0. */ i = wolfSSL_sk_X509_OBJECT_num(objs) - 1; while (cnt > 0 && i >= 0) { - /* The inner X509 is owned by somebody else, NULL out the reference */ obj = (WOLFSSL_X509_OBJECT *)wolfSSL_sk_X509_OBJECT_value(objs, i); - if (obj != NULL) { + /* Only certificate objects can be borrowed from store->certs, so only + * they may consume the numAdded budget. Counting any other object type + * would slide this window one slot past the borrowed certificates - + * wolfSSL_X509_STORE_get0_objects() appends a CRL object after them + * under HAVE_CRL - leaving the first borrowed certificate live for the + * pop_free below to release a reference it does not own. Skipping the + * CRL object also lets pop_free drop the reference that + * get0_objects() took on it, rather than leaking it. */ + if (obj != NULL && obj->type == WOLFSSL_X509_LU_X509) { + /* The inner X509 is owned by somebody else, NULL out the reference + */ obj->type = (WOLFSSL_X509_LOOKUP_TYPE)0; obj->data.ptr = NULL; + cnt--; } - cnt--; i--; } From d6009a65dfdd195cd7c6e7bc3cf3dedd5e0dc378 Mon Sep 17 00:00:00 2001 From: Kareem Date: Wed, 29 Jul 2026 17:45:29 -0700 Subject: [PATCH 3/9] Prevent unoffered types from being allowed in the RPK case. Thanks to Christos Papakonstantinou (Cantina Security) for the report. --- src/ssl.c | 15 +++++++++++++++ src/tls.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/src/ssl.c b/src/ssl.c index 0c286cf9171..4d182bf2770 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -5659,6 +5659,21 @@ size_t wolfSSL_get_client_random(const WOLFSSL* ssl, unsigned char* out, ssl->earlyData = no_early_data; ssl->earlyDataSz = 0; #endif + #ifdef HAVE_RPK + { + /* Reset the RPK certificate-type negotiation state. received_* is + * peer-supplied and sending_* is derived from it, so leaving either + * set would carry one peer's certificate-type choice into the next + * handshake on this reused object, where ProcessPeerCertParse() + * would read it as the type negotiated with the new peer. + * isRPKLoaded describes the locally loaded certificate rather than + * the negotiation, so it survives. */ + int rpkLoaded = ssl->options.rpkState.isRPKLoaded; + XMEMSET(&ssl->options.rpkState, 0, + sizeof(ssl->options.rpkState)); + ssl->options.rpkState.isRPKLoaded = rpkLoaded; + } + #endif #if defined(HAVE_TLS_EXTENSIONS) && !defined(NO_TLS) TLSX_FreeAll(ssl->extensions, ssl->heap); diff --git a/src/tls.c b/src/tls.c index 07a48902ee0..9a991700482 100644 --- a/src/tls.c +++ b/src/tls.c @@ -13541,6 +13541,23 @@ static int TLSX_ClientCertificateType_Parse(WOLFSSL* ssl, const byte* input, else if (msgType == server_hello || msgType == encrypted_extensions) { /* parse it in client side */ if (length == 1) { + /* Same offered-vs-received binding as the server_cert_type twin + * above (RFC 7250 4.1, RFC 8446 4.2). An unsolicited value here + * would let the peer pick the form this client presents its own + * credential in. */ + if (ssl->options.rpkState.sending_ClientCertTypeCnt == 0) { + WOLFSSL_MSG("client_cert_type received but never offered"); + WOLFSSL_ERROR_VERBOSE(UNSUPPORTED_EXTENSION); + return UNSUPPORTED_EXTENSION; + } + if (!IsCertTypeListed(*input, + ssl->options.rpkState.sending_ClientCertTypeCnt, + ssl->options.rpkState.sending_ClientCertTypes)) { + WOLFSSL_MSG("client_cert_type value was not offered"); + WOLFSSL_ERROR_VERBOSE(UNSUPPORTED_EXTENSION); + return UNSUPPORTED_EXTENSION; + } + ssl->options.rpkState.received_ClientCertTypeCnt = 1; ssl->options.rpkState.received_ClientCertTypes[0] = *input; } @@ -13741,6 +13758,28 @@ static int TLSX_ServerCertificateType_Parse(WOLFSSL* ssl, const byte* input, if (length != 1) /* length slould be 1 */ return BUFFER_E; + /* RFC 7250 4.1 and RFC 8446 4.2: a server may only answer with a + * certificate type the client offered, and must not send the extension + * at all when the client did not offer it. Enforce that here, because + * the value stored below is what ProcessPeerCertParse() later treats as + * the negotiated certificate type - accepting an unsolicited value lets + * the peer choose it, and a peer-chosen RawPublicKey means + * ParseCertRelative() returns before any chain or trust verification. + * sending_ServerCertTypes[] holds what this client actually offered; + * the count is 0 when no extension was sent. */ + if (ssl->options.rpkState.sending_ServerCertTypeCnt == 0) { + WOLFSSL_MSG("server_cert_type received but never offered"); + WOLFSSL_ERROR_VERBOSE(UNSUPPORTED_EXTENSION); + return UNSUPPORTED_EXTENSION; + } + if (!IsCertTypeListed(*input, + ssl->options.rpkState.sending_ServerCertTypeCnt, + ssl->options.rpkState.sending_ServerCertTypes)) { + WOLFSSL_MSG("server_cert_type value was not offered"); + WOLFSSL_ERROR_VERBOSE(UNSUPPORTED_EXTENSION); + return UNSUPPORTED_EXTENSION; + } + ssl->options.rpkState.received_ServerCertTypeCnt = 1; ssl->options.rpkState.received_ServerCertTypes[0] = *input; } From 1d3ebc06342566c72e886994a01068c4e4818ebd Mon Sep 17 00:00:00 2001 From: Kareem Date: Wed, 29 Jul 2026 17:46:49 -0700 Subject: [PATCH 4/9] Always free TEMP_CAs in wolfSSL_verify_X509. Thanks to Christos Papakonstantinou (Cantina Security) for the report. --- src/x509_str.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/src/x509_str.c b/src/x509_str.c index c0d99a1c3cb..b55c937b913 100644 --- a/src/x509_str.c +++ b/src/x509_str.c @@ -858,7 +858,6 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) { int ret = WC_NO_ERR_TRACE(WOLFSSL_FAILURE); int done = 0; - int added = 0; int i = 0; int numFailedCerts = 0; int depth = 0; @@ -997,11 +996,19 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) &depth, origDepth); continue; } - added = 1; ret = X509StoreVerifyCert(ctx); if (ret != WOLFSSL_SUCCESS) { - if ((origDepth - depth) <= 1) - added = 0; + /* The issuer just added above did not authenticate this + * certificate, so drop it again here. AddCA() does not verify a + * CA certificate's own signature for CA_TYPE additions, so a + * caller-supplied issuer left behind would sit in the shared + * CertManager as a fully usable WOLFSSL_TEMP_CA anchor - + * GetCA()/GetCAByName() do not inspect signer->type, so every + * other consumer of this CertManager (native TLS peer + * verification, OCSP, CRL, CM_VerifyBuffer_ex) would treat it + * like a configured trust root. X509VerifyCertSetupRetry() + * below only removes ctx->current_cert, the child. */ + X509StoreRemoveCa(ctx->store, issuer, WOLFSSL_TEMP_CA); X509VerifyCertSetupRetry(ctx, certs, failedCerts, &depth, origDepth); continue; @@ -1023,13 +1030,11 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) != WOLFSSL_SUCCESS) { /* Could not guarantee the temporary intermediates were * dropped; fail closed rather than risk verifying the current - * certificate against one. Leave `added` set: they are still - * loaded, so the exit cleanup makes a final attempt to drop - * them. */ + * certificate against one. The exit cleanup makes a final + * unconditional attempt to drop them. */ ret = WOLFSSL_FATAL_ERROR; goto exit; } - added = 0; ret = X509StoreVerifyCert(ctx); if (ret != WOLFSSL_SUCCESS) { /* WOLFSSL_PARTIAL_CHAIN may only terminate the chain at a @@ -1149,12 +1154,19 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) } } } - /* Remove intermediates that were added to CM */ + /* Remove intermediates that were added to CM. + * + * Unconditional on purpose: this must hold on every exit path, including + * the early `goto exit`s and the chain-building failure paths. A flag + * tracking "did we add one" is not a safe guard, because the failure paths + * that leave a caller-supplied issuer loaded are exactly the paths that + * would clear it - and anything left resident becomes a WOLFSSL_TEMP_CA + * trust anchor for every other consumer of this shared CertManager, whose + * signer lookups do not check signer->type. Unloading when nothing was + * added is harmless. */ if (ctx != NULL) { if (ctx->store != NULL) { - if (added == 1) { - wolfSSL_CertManagerUnloadTempIntermediateCerts(ctx->store->cm); - } + wolfSSL_CertManagerUnloadTempIntermediateCerts(ctx->store->cm); } if (orig != NULL) { ctx->current_cert = orig; From 26c55a6fd461de1059d30664b110fbcab09d3b51 Mon Sep 17 00:00:00 2001 From: Kareem Date: Fri, 31 Jul 2026 14:52:14 -0700 Subject: [PATCH 5/9] Code review feedback: reduce comment verbosity, add DTLS test --- src/internal.c | 16 ++-------------- tests/api/test_dtls13.c | 39 +++++++++++++++++++++++++++++++++++++++ tests/api/test_dtls13.h | 2 ++ 3 files changed, 43 insertions(+), 14 deletions(-) diff --git a/src/internal.c b/src/internal.c index a1a4d965993..918d426322b 100644 --- a/src/internal.c +++ b/src/internal.c @@ -11695,11 +11695,6 @@ static int wolfSSLReceive(WOLFSSL* ssl, byte* buf, word32 sz) void ShrinkOutputBuffer(WOLFSSL* ssl) { WOLFSSL_MSG("Shrinking output buffer"); - /* Only the dynamic buffer is heap allocated. When dynamicFlag is clear the - * buffer already is the inline staticBuffer array, and in a - * LARGE_STATIC_BUFFERS build a pending flight can live there, so freeing - * unconditionally would pass an interior pointer of the WOLFSSL struct to - * XFREE. */ if (ssl->buffers.outputBuffer.dynamicFlag) { XFREE(ssl->buffers.outputBuffer.buffer - ssl->buffers.outputBuffer.offset, @@ -11709,15 +11704,8 @@ void ShrinkOutputBuffer(WOLFSSL* ssl) ssl->buffers.outputBuffer.bufferSize = STATIC_BUFFER_LEN; ssl->buffers.outputBuffer.dynamicFlag = 0; ssl->buffers.outputBuffer.offset = 0; - /* Drop idx/length with the buffer they described. bufferSize shrinks to - * STATIC_BUFFER_LEN here, so leaving them set would break the - * idx + length <= bufferSize invariant that the rest of the output - * accounting relies on: CheckAvailableSize()'s word32 space computation - * would underflow instead of growing, and SendBuffered()/GetOutputBuffer() - * would address past the end of staticBuffer. Callers reaching this - * function with output still queued are abandoning that output. */ - ssl->buffers.outputBuffer.idx = 0; - ssl->buffers.outputBuffer.length = 0; + ssl->buffers.outputBuffer.idx = 0; + ssl->buffers.outputBuffer.length = 0; } diff --git a/tests/api/test_dtls13.c b/tests/api/test_dtls13.c index c8fdcd65dfd..c7f7f83e0cf 100644 --- a/tests/api/test_dtls13.c +++ b/tests/api/test_dtls13.c @@ -1122,6 +1122,45 @@ int test_dtls13_epochs(void) { return EXPECT_RESULT(); } +int test_dtls13_alert_with_pending_output(void) +{ + EXPECT_DECLS; +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_DTLS13) + WOLFSSL_CTX *ctx_c = NULL; + WOLFSSL_CTX *ctx_s = NULL; + WOLFSSL *ssl_c = NULL; + WOLFSSL *ssl_s = NULL; + struct test_memio_ctx test_ctx; + char msg[1300]; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + XMEMSET(msg, 'A', sizeof(msg)); + + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfDTLSv1_3_client_method, wolfDTLSv1_3_server_method), 0); + ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + + /* Stall the transport, then queue a record big enough that adding the + * alert would exceed the MTU. */ + test_ctx.s_force_want_write = 1; + ExpectIntLT(wolfSSL_write(ssl_s, msg, (int)sizeof(msg)), 0); + ExpectIntGT((int)ssl_s->buffers.outputBuffer.length, 1288); + + /* EndOfEarlyData is not valid in DTLS 1.3 and raises a fatal alert. */ + ExpectIntEQ(Dtls13CheckEpoch(ssl_s, end_of_early_data), SANITY_MSG_E); + + ExpectIntLE((int)(ssl_s->buffers.outputBuffer.idx + + ssl_s->buffers.outputBuffer.length), + (int)ssl_s->buffers.outputBuffer.bufferSize); + + wolfSSL_free(ssl_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); +#endif + return EXPECT_RESULT(); +} + /*-- ack_order (test_dtls.c lines 873,951) ---*/ int test_dtls13_ack_order(void) { diff --git a/tests/api/test_dtls13.h b/tests/api/test_dtls13.h index 2cf31bec279..ac211f1db6c 100644 --- a/tests/api/test_dtls13.h +++ b/tests/api/test_dtls13.h @@ -44,6 +44,7 @@ int test_dtls13_basic_connection_id(void); int test_dtls13_hrr_want_write(void); int test_dtls13_every_write_want_write(void); int test_dtls13_epochs(void); +int test_dtls13_alert_with_pending_output(void); int test_dtls13_ack_order(void); int test_dtls13_ack_overflow(void); int test_dtls13_ack_dup_write_counter(void); @@ -71,6 +72,7 @@ int test_dtls13_reuse_after_clear(void); TEST_DECL_GROUP("dtls13", test_dtls13_hrr_want_write), \ TEST_DECL_GROUP("dtls13", test_dtls13_every_write_want_write), \ TEST_DECL_GROUP("dtls13", test_dtls13_epochs), \ + TEST_DECL_GROUP("dtls13", test_dtls13_alert_with_pending_output), \ TEST_DECL_GROUP("dtls13", test_dtls13_ack_order), \ TEST_DECL_GROUP("dtls13", test_dtls13_ack_overflow), \ TEST_DECL_GROUP("dtls13", test_dtls13_ack_dup_write_counter), \ From 90f3ecc9aa7110df76bc39b108d39b7c39a986fd Mon Sep 17 00:00:00 2001 From: Kareem Date: Fri, 31 Jul 2026 15:28:40 -0700 Subject: [PATCH 6/9] Code review feedback: improve wolfSSL_X509_STORE_get0_objects freeing logic, add test, make comments less verbose --- src/x509_str.c | 41 +++++++++++++++++++++++---------- tests/api/test_ossl_x509_str.c | 42 ++++++++++++++++++++++++++++++++++ tests/api/test_ossl_x509_str.h | 3 +++ 3 files changed, 74 insertions(+), 12 deletions(-) diff --git a/src/x509_str.c b/src/x509_str.c index b55c937b913..51cbf258d07 100644 --- a/src/x509_str.c +++ b/src/x509_str.c @@ -1790,17 +1790,10 @@ static void X509StoreFreeObjList(WOLFSSL_X509_STORE* store, i = wolfSSL_sk_X509_OBJECT_num(objs) - 1; while (cnt > 0 && i >= 0) { obj = (WOLFSSL_X509_OBJECT *)wolfSSL_sk_X509_OBJECT_value(objs, i); - /* Only certificate objects can be borrowed from store->certs, so only - * they may consume the numAdded budget. Counting any other object type - * would slide this window one slot past the borrowed certificates - - * wolfSSL_X509_STORE_get0_objects() appends a CRL object after them - * under HAVE_CRL - leaving the first borrowed certificate live for the - * pop_free below to release a reference it does not own. Skipping the - * CRL object also lets pop_free drop the reference that - * get0_objects() took on it, rather than leaking it. */ + /* Only certificates are borrowed, so only they consume numAdded. The + * CRL object appended after them must not shift this window. */ if (obj != NULL && obj->type == WOLFSSL_X509_LU_X509) { - /* The inner X509 is owned by somebody else, NULL out the reference - */ + /* The inner X509 is owned by somebody else, NULL out the ref */ obj->type = (WOLFSSL_X509_LOOKUP_TYPE)0; obj->data.ptr = NULL; cnt--; @@ -2543,6 +2536,9 @@ WOLF_STACK_OF(WOLFSSL_X509_OBJECT)* wolfSSL_X509_STORE_get0_objects( { WOLFSSL_STACK* ret = NULL; WOLFSSL_STACK* cert_stack = NULL; + /* Set once the certificates have been handed over to "ret". Until then + * cert_stack still owns them and the error path must not free them. */ + byte certsOwned = 0; #if ((defined(WOLFSSL_SIGNER_DER_CERT) && !defined(NO_FILESYSTEM)) || \ (defined(HAVE_CRL))) WOLFSSL_X509_OBJECT* obj = NULL; @@ -2616,6 +2612,7 @@ WOLF_STACK_OF(WOLFSSL_X509_OBJECT)* wolfSSL_X509_STORE_get0_objects( while (wolfSSL_sk_X509_num(cert_stack) > 0) { wolfSSL_sk_X509_pop(cert_stack); } + certsOwned = 1; #endif #ifdef HAVE_CRL @@ -2646,8 +2643,28 @@ WOLF_STACK_OF(WOLFSSL_X509_OBJECT)* wolfSSL_X509_STORE_get0_objects( store->objs = ret; return ret; err_cleanup: - if (ret != NULL) - X509StoreFreeObjList(store, ret); + if (ret != NULL) { + if (certsOwned) { + X509StoreFreeObjList(store, ret); + } + else { + /* cert_stack still owns these certificates. pop_free() below runs + * wolfSSL_X509_OBJECT_free() on each entry, which frees the inner + * X509, so clear the references or they are freed twice. */ + int j; + WOLFSSL_X509_OBJECT* cur; + + for (j = 0; j < wolfSSL_sk_X509_OBJECT_num(ret); j++) { + cur = (WOLFSSL_X509_OBJECT*)wolfSSL_sk_X509_OBJECT_value(ret, + j); + if (cur != NULL) { + cur->type = (WOLFSSL_X509_LOOKUP_TYPE)0; + cur->data.ptr = NULL; + } + } + wolfSSL_sk_X509_OBJECT_pop_free(ret, NULL); + } + } if (cert_stack != NULL) { while (store->numAdded > 0) { wolfSSL_sk_X509_pop(cert_stack); diff --git a/tests/api/test_ossl_x509_str.c b/tests/api/test_ossl_x509_str.c index 2257fa5ed35..73dbfe9734e 100644 --- a/tests/api/test_ossl_x509_str.c +++ b/tests/api/test_ossl_x509_str.c @@ -2568,6 +2568,48 @@ int test_X509_STORE_get0_objects(void) return EXPECT_RESULT(); } +int test_X509_STORE_get0_objects_borrowed_crl(void) +{ + EXPECT_DECLS; +#if defined(OPENSSL_ALL) && defined(HAVE_CRL) && \ + defined(WOLFSSL_SIGNER_DER_CERT) && !defined(NO_FILESYSTEM) && \ + !defined(NO_RSA) + int pass; + + /* pass 0: one get0_objects call. pass 1: a second call, which rebuilds + * the list and so tears the first one down while the store is alive. */ + for (pass = 0; pass < 2 && EXPECT_SUCCESS(); pass++) { + X509_STORE* store = NULL; + X509* borrowed = NULL; + STACK_OF(X509_OBJECT)* objs = NULL; + + /* Not self-signed, so add_cert up_refs it onto store->certs. */ + ExpectNotNull(borrowed = wolfSSL_X509_load_certificate_file(svrCertFile, + WOLFSSL_FILETYPE_PEM)); + ExpectNotNull(store = X509_STORE_new()); + ExpectIntEQ(X509_STORE_add_cert(store, borrowed), 1); + /* Arms cm->crl and puts one decoded CA in the CertManager. */ + ExpectIntEQ(X509_STORE_load_locations(store, caCertFile, NULL), + WOLFSSL_SUCCESS); + + ExpectNotNull(objs = X509_STORE_get0_objects(store)); + /* CM decode + borrowed cert + CRL. */ + ExpectIntEQ(sk_X509_OBJECT_num(objs), 3); + if (pass == 1) { + ExpectNotNull(objs = X509_STORE_get0_objects(store)); + ExpectIntEQ(sk_X509_OBJECT_num(objs), 3); + } + + X509_STORE_free(store); + + /* The store is gone but the caller's reference must have survived. */ + ExpectNotNull(X509_get_subject_name(borrowed)); + X509_free(borrowed); + } +#endif + return EXPECT_RESULT(); +} + int test_wolfSSL_X509_STORE_get1_certs(void) { EXPECT_DECLS; diff --git a/tests/api/test_ossl_x509_str.h b/tests/api/test_ossl_x509_str.h index d320133b627..274b231dda7 100644 --- a/tests/api/test_ossl_x509_str.h +++ b/tests/api/test_ossl_x509_str.h @@ -44,6 +44,7 @@ int test_wolfSSL_X509_STORE_set_flags(void); int test_wolfSSL_X509_STORE(void); int test_wolfSSL_X509_STORE_load_locations(void); int test_X509_STORE_get0_objects(void); +int test_X509_STORE_get0_objects_borrowed_crl(void); int test_wolfSSL_X509_STORE_get1_certs(void); int test_wolfSSL_X509_STORE_set_get_crl(void); int test_wolfSSL_X509_STORE_CTX_set0_crls(void); @@ -82,6 +83,8 @@ int test_wolfSSL_CTX_set_cert_store(void); TEST_DECL_GROUP("ossl_x509_store", \ test_wolfSSL_X509_STORE_load_locations), \ TEST_DECL_GROUP("ossl_x509_store", test_X509_STORE_get0_objects), \ + TEST_DECL_GROUP("ossl_x509_store", \ + test_X509_STORE_get0_objects_borrowed_crl), \ TEST_DECL_GROUP("ossl_x509_store", test_wolfSSL_X509_STORE_get1_certs), \ TEST_DECL_GROUP("ossl_x509_store", test_wolfSSL_X509_STORE_set_get_crl), \ TEST_DECL_GROUP("ossl_x509_store", \ From 2f74a6aeeeca62916592d03e5398841da33a412f Mon Sep 17 00:00:00 2001 From: Kareem Date: Fri, 31 Jul 2026 15:30:19 -0700 Subject: [PATCH 7/9] Code review feedback: Make RPK comments less verbose, add RPK test --- src/ssl.c | 10 +++------- src/tls.c | 20 +++++++------------ tests/api/test_tls13.c | 45 ++++++++++++++++++++++++++++++++++++++++++ tests/api/test_tls13.h | 2 ++ 4 files changed, 57 insertions(+), 20 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index 4d182bf2770..1c43ee4b7df 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -5661,13 +5661,9 @@ size_t wolfSSL_get_client_random(const WOLFSSL* ssl, unsigned char* out, #endif #ifdef HAVE_RPK { - /* Reset the RPK certificate-type negotiation state. received_* is - * peer-supplied and sending_* is derived from it, so leaving either - * set would carry one peer's certificate-type choice into the next - * handshake on this reused object, where ProcessPeerCertParse() - * would read it as the type negotiated with the new peer. - * isRPKLoaded describes the locally loaded certificate rather than - * the negotiation, so it survives. */ + /* Drop the negotiated cert types so one peer's choice cannot carry + * into the next handshake. isRPKLoaded describes the local + * certificate, not the negotiation, so it survives. */ int rpkLoaded = ssl->options.rpkState.isRPKLoaded; XMEMSET(&ssl->options.rpkState, 0, sizeof(ssl->options.rpkState)); diff --git a/src/tls.c b/src/tls.c index 9a991700482..348532e1d3f 100644 --- a/src/tls.c +++ b/src/tls.c @@ -13541,10 +13541,9 @@ static int TLSX_ClientCertificateType_Parse(WOLFSSL* ssl, const byte* input, else if (msgType == server_hello || msgType == encrypted_extensions) { /* parse it in client side */ if (length == 1) { - /* Same offered-vs-received binding as the server_cert_type twin - * above (RFC 7250 4.1, RFC 8446 4.2). An unsolicited value here - * would let the peer pick the form this client presents its own - * credential in. */ + /* Same offered-vs-received binding as server_cert_type: an + * unsolicited value lets the peer pick the form this client + * presents its own credential in. */ if (ssl->options.rpkState.sending_ClientCertTypeCnt == 0) { WOLFSSL_MSG("client_cert_type received but never offered"); WOLFSSL_ERROR_VERBOSE(UNSUPPORTED_EXTENSION); @@ -13758,15 +13757,10 @@ static int TLSX_ServerCertificateType_Parse(WOLFSSL* ssl, const byte* input, if (length != 1) /* length slould be 1 */ return BUFFER_E; - /* RFC 7250 4.1 and RFC 8446 4.2: a server may only answer with a - * certificate type the client offered, and must not send the extension - * at all when the client did not offer it. Enforce that here, because - * the value stored below is what ProcessPeerCertParse() later treats as - * the negotiated certificate type - accepting an unsolicited value lets - * the peer choose it, and a peer-chosen RawPublicKey means - * ParseCertRelative() returns before any chain or trust verification. - * sending_ServerCertTypes[] holds what this client actually offered; - * the count is 0 when no extension was sent. */ + /* RFC 7250 4.1, RFC 8446 4.2: the server may only answer with a type + * the client offered. ProcessPeerCertParse() treats the stored value as + * negotiated, so an unsolicited one lets the peer select RawPublicKey + * and skip chain verification. */ if (ssl->options.rpkState.sending_ServerCertTypeCnt == 0) { WOLFSSL_MSG("server_cert_type received but never offered"); WOLFSSL_ERROR_VERBOSE(UNSUPPORTED_EXTENSION); diff --git a/tests/api/test_tls13.c b/tests/api/test_tls13.c index f0cb857df2d..c133e85caaa 100644 --- a/tests/api/test_tls13.c +++ b/tests/api/test_tls13.c @@ -4078,6 +4078,51 @@ int test_tls13_rpk_trust(void) return EXPECT_RESULT(); } +int test_tls13_rpk_unoffered_cert_type(void) +{ + EXPECT_DECLS; +#if defined(HAVE_RPK) && defined(WOLFSSL_TLS13) && \ + !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER) + int isServerType; + + /* round 0: server_cert_type. round 1: the client_cert_type twin. */ + for (isServerType = 1; isServerType >= 0; isServerType--) { + WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL; + WOLFSSL *ssl_c = NULL, *ssl_s = NULL; + struct test_memio_ctx test_ctx; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_rpk_nopin_setup(&test_ctx, &ctx_c, &ctx_s, + &ssl_c, &ssl_s, wolfTLSv1_3_client_method, + wolfTLSv1_3_server_method), 0); + + /* ClientHello out, then the server's flight carrying the response. */ + ExpectIntNE(wolfSSL_connect(ssl_c), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_get_error(ssl_c, + WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR)), WOLFSSL_ERROR_WANT_READ); + ExpectIntNE(wolfSSL_accept(ssl_s), WOLFSSL_SUCCESS); + + /* Drop the offer, making the response unsolicited. */ + if (ssl_c != NULL) { + if (isServerType) + ssl_c->options.rpkState.sending_ServerCertTypeCnt = 0; + else + ssl_c->options.rpkState.sending_ClientCertTypeCnt = 0; + } + + ExpectIntNE(wolfSSL_connect(ssl_c), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_get_error(ssl_c, + WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR)), UNSUPPORTED_EXTENSION); + + wolfSSL_free(ssl_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); + } +#endif /* HAVE_RPK && WOLFSSL_TLS13 && client && server */ + return EXPECT_RESULT(); +} + #if defined(HAVE_IO_TESTS_DEPENDENCIES) && defined(WOLFSSL_TLS13) && \ defined(WOLFSSL_HAVE_MLKEM) && !defined(WOLFSSL_MLKEM_NO_ENCAPSULATE) && \ diff --git a/tests/api/test_tls13.h b/tests/api/test_tls13.h index 30ac62952fd..922811cfc93 100644 --- a/tests/api/test_tls13.h +++ b/tests/api/test_tls13.h @@ -32,6 +32,7 @@ int test_tls13_rpk_handshake_no_negotiation(void); int test_tls13_pha(void); int test_tls13_rpk_untrusted(void); int test_tls13_rpk_trust(void); +int test_tls13_rpk_unoffered_cert_type(void); int test_tls13_pq_groups(void); int test_tls13_multi_pqc_key_share(void); int test_tls13_early_data(void); @@ -115,6 +116,7 @@ int test_tls13_pha_status_request(void); TEST_DECL_GROUP("tls13", test_tls13_pha), \ TEST_DECL_GROUP("tls13", test_tls13_rpk_untrusted), \ TEST_DECL_GROUP("tls13", test_tls13_rpk_trust), \ + TEST_DECL_GROUP("tls13", test_tls13_rpk_unoffered_cert_type), \ TEST_DECL_GROUP("tls13", test_tls13_pq_groups), \ TEST_DECL_GROUP("tls13", test_tls13_multi_pqc_key_share), \ TEST_DECL_GROUP("tls13", test_tls13_early_data), \ From 9449c686204f437e4840da3c72de7ba2b3312e5d Mon Sep 17 00:00:00 2001 From: Kareem Date: Fri, 31 Jul 2026 15:33:18 -0700 Subject: [PATCH 8/9] Code review feedback: Remove unneeded X509StoreRemoveCa call, reduce comment verbosity, add test --- src/x509_str.c | 26 ++------------- tests/api/test_ossl_x509_str.c | 58 ++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 23 deletions(-) diff --git a/src/x509_str.c b/src/x509_str.c index 51cbf258d07..ed9099a4c6f 100644 --- a/src/x509_str.c +++ b/src/x509_str.c @@ -998,17 +998,6 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) } ret = X509StoreVerifyCert(ctx); if (ret != WOLFSSL_SUCCESS) { - /* The issuer just added above did not authenticate this - * certificate, so drop it again here. AddCA() does not verify a - * CA certificate's own signature for CA_TYPE additions, so a - * caller-supplied issuer left behind would sit in the shared - * CertManager as a fully usable WOLFSSL_TEMP_CA anchor - - * GetCA()/GetCAByName() do not inspect signer->type, so every - * other consumer of this CertManager (native TLS peer - * verification, OCSP, CRL, CM_VerifyBuffer_ex) would treat it - * like a configured trust root. X509VerifyCertSetupRetry() - * below only removes ctx->current_cert, the child. */ - X509StoreRemoveCa(ctx->store, issuer, WOLFSSL_TEMP_CA); X509VerifyCertSetupRetry(ctx, certs, failedCerts, &depth, origDepth); continue; @@ -1030,8 +1019,7 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) != WOLFSSL_SUCCESS) { /* Could not guarantee the temporary intermediates were * dropped; fail closed rather than risk verifying the current - * certificate against one. The exit cleanup makes a final - * unconditional attempt to drop them. */ + * certificate against one. */ ret = WOLFSSL_FATAL_ERROR; goto exit; } @@ -1154,16 +1142,8 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) } } } - /* Remove intermediates that were added to CM. - * - * Unconditional on purpose: this must hold on every exit path, including - * the early `goto exit`s and the chain-building failure paths. A flag - * tracking "did we add one" is not a safe guard, because the failure paths - * that leave a caller-supplied issuer loaded are exactly the paths that - * would clear it - and anything left resident becomes a WOLFSSL_TEMP_CA - * trust anchor for every other consumer of this shared CertManager, whose - * signer lookups do not check signer->type. Unloading when nothing was - * added is harmless. */ + /* Remove intermediates that were added to CM. Unconditional: anything left + * resident anchors verification for every other user of this CM. */ if (ctx != NULL) { if (ctx->store != NULL) { wolfSSL_CertManagerUnloadTempIntermediateCerts(ctx->store->cm); diff --git a/tests/api/test_ossl_x509_str.c b/tests/api/test_ossl_x509_str.c index 73dbfe9734e..42409ce7975 100644 --- a/tests/api/test_ossl_x509_str.c +++ b/tests/api/test_ossl_x509_str.c @@ -1657,6 +1657,60 @@ static int test_untrusted_inter_retry(X509* leaf, X509* inter, sk_X509_free(badOnly); return EXPECT_RESULT(); } + +/* A first-link signature failure must not leave the caller-supplied issuer + * loaded in the store's CertManager. X509_verify_cert() adds it as a + * WOLFSSL_TEMP_CA before checking the child; if the check fails the anchor has + * to go with it. The compat verifier drops TEMP_CAs before its own trust + * check, so residue is only visible through another user of the same + * CertManager - signer lookups there do not filter on type. */ +static int test_untrusted_inter_no_temp_ca_residue(X509* leaf, X509* inter, + X509* root) +{ + EXPECT_DECLS; + X509_STORE* store = NULL; + X509_STORE_CTX* ctx = NULL; + STACK_OF(X509)* untrusted = NULL; + X509* badLeaf = NULL; + unsigned char* der = NULL; + const unsigned char* p = NULL; + int derSz = 0; + + /* Flip a bit in the trailing signature BIT STRING: the leaf still names + * inter as its issuer, so inter is still selected and loaded, but the + * signature check against it now fails. */ + ExpectIntGT(derSz = wolfSSL_i2d_X509(leaf, &der), 0); + ExpectNotNull(der); + if (EXPECT_SUCCESS() && der != NULL) { + der[derSz - 1] ^= 0x01; + p = der; + ExpectNotNull(badLeaf = wolfSSL_d2i_X509(NULL, &p, derSz)); + der[derSz - 1] ^= 0x01; + } + + ExpectNotNull(store = X509_STORE_new()); + ExpectIntEQ(X509_STORE_add_cert(store, root), 1); + ExpectNotNull(untrusted = sk_X509_new_null()); + ExpectIntGT(sk_X509_push(untrusted, inter), 0); + ExpectNotNull(ctx = X509_STORE_CTX_new()); + ExpectIntEQ(X509_STORE_CTX_init(ctx, store, badLeaf, untrusted), 1); + ExpectIntEQ(X509_verify_cert(ctx), 0); + ExpectIntNE(X509_STORE_CTX_get_error(ctx), X509_V_OK); + + /* Only root was ever trusted, so the genuine leaf must not verify through + * the CertManager. It would if inter were still resident as a TEMP_CA. */ + if (EXPECT_SUCCESS() && store != NULL && der != NULL) { + ExpectIntNE(wolfSSL_CertManagerVerifyBuffer(store->cm, der, derSz, + WOLFSSL_FILETYPE_ASN1), WOLFSSL_SUCCESS); + } + + X509_STORE_CTX_free(ctx); + X509_STORE_free(store); + sk_X509_free(untrusted); + X509_free(badLeaf); + XFREE(der, NULL, DYNAMIC_TYPE_OPENSSL); + return EXPECT_RESULT(); +} #endif /* OPENSSL_EXTRA && !NO_RSA && !NO_CERTS && !NO_FILESYSTEM */ int test_X509_verify_cert_untrusted_inter(void) @@ -1682,6 +1736,7 @@ int test_X509_verify_cert_untrusted_inter(void) int depthExhaustRes = 0; int trustedStackCleanupRes = 0; int retryRes = 0; + int noTempCaResidueRes = 0; ExpectNotNull(leaf = untrusted_inter_load(UA_CERT_DIR "leaf-cert.pem")); ExpectNotNull(leafDeep = @@ -1716,6 +1771,8 @@ int test_X509_verify_cert_untrusted_inter(void) trustedStackCleanupRes = test_untrusted_inter_trusted_stack_cleanup( leaf, inter, root); retryRes = test_untrusted_inter_retry(leaf, inter, tamperedInter, root); + noTempCaResidueRes = test_untrusted_inter_no_temp_ca_residue(leaf, + inter, root); ExpectIntEQ(sanityRes, 1); ExpectIntEQ(twoLevelRes, 1); ExpectIntEQ(emptyStoreRes, 1); @@ -1726,6 +1783,7 @@ int test_X509_verify_cert_untrusted_inter(void) ExpectIntEQ(depthExhaustRes, 1); ExpectIntEQ(trustedStackCleanupRes, 1); ExpectIntEQ(retryRes, 1); + ExpectIntEQ(noTempCaResidueRes, 1); } X509_free(leaf); From 5fd6966266a898f84da3601d52279abc47df02e2 Mon Sep 17 00:00:00 2001 From: Kareem Date: Mon, 3 Aug 2026 11:25:09 -0700 Subject: [PATCH 9/9] Fix RPK unit test failing under --enable-all. --- tests/api/test_tls13.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/api/test_tls13.c b/tests/api/test_tls13.c index c133e85caaa..6eab72e1c93 100644 --- a/tests/api/test_tls13.c +++ b/tests/api/test_tls13.c @@ -4096,6 +4096,15 @@ int test_tls13_rpk_unoffered_cert_type(void) &ssl_c, &ssl_s, wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0); + /* Both ends must authenticate the peer. Under + * OPENSSL_COMPATIBLE_DEFAULTS (--enable-all) the CTX defaults to + * WOLFSSL_VERIFY_NONE, and a non-verifying server omits the + * client_certificate_type response entirely, leaving nothing for the + * second round to reject. Set it explicitly so both rounds behave the + * same across build configs. */ + wolfSSL_set_verify(ssl_c, WOLFSSL_VERIFY_PEER, NULL); + wolfSSL_set_verify(ssl_s, WOLFSSL_VERIFY_PEER, NULL); + /* ClientHello out, then the server's flight carrying the response. */ ExpectIntNE(wolfSSL_connect(ssl_c), WOLFSSL_SUCCESS); ExpectIntEQ(wolfSSL_get_error(ssl_c,