From bd1c52f6a3166c44a7fefd4cf5e75e73a8a1c156 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Mon, 5 Oct 2026 15:03:15 +0200 Subject: [PATCH 1/7] Drop stale PKCS#7 content before freeing the stream copy After a streaming wc_PKCS7_VerifySignedData() of a message that carries content, pkcs7->content points at pkcs7->stream->content, which wc_PKCS7_ResetStream() keeps so the caller can read the content. Verifying another message with the same object frees that buffer in stage 2 but left pkcs7->content pointing at it. When the next message is detached, for example a certificates-only bundle, wc_PKCS7_HandleOctetStrings() copies the detached content from pkcs7->content and so read freed memory. Builds with NO_PKCS7_STREAM leave pkcs7->content pointing into the previous input buffer instead, so a detached verify that follows hashes that buffer whether or not the caller has freed it. Record the pointer a successful verify leaves in pkcs7->content in a new wc_PKCS7 member, verifyContent. When the next verify starts, clear pkcs7->content if it still holds that pointer or pkcs7->contentDynamic, and contentSz with it unless the call is a header/footer verify, which takes contentSz from the caller. Content the caller sets in between is a different pointer and is kept. Nothing in the new message is then parsed against the previous one, including the detached-signature check and the stream size bookkeeping in stage 2. The pkcs7->contentDynamic copy a verify leaves behind was never released by the next one: stage 3 overwrote it, which leaked it when a detached verify followed a verify of a message with content, and stage 5 copied from it at the size of the new content. Free it when the next verify starts. Stage 5 also asked for a zero-byte copy when the message had no content, which fails on allocators that return NULL for size 0; skip the copy then. A detached signature hit the same problem: the verify replaced the content the caller set with its stream copy, so a second detached verify with the same object also read the freed copy. Hand back the caller's pointer instead, as builds with NO_PKCS7_STREAM already do, so that verify keeps using the caller's content. A verify that fails now also leaves pkcs7->content and contentSz as the caller set them, in both builds. Streaming left them on a library buffer, and builds without streaming lost them once stage 5 had reset the structure. PKCS7_SIGNEEDS_CHECK is not a failure here: it hands the message content back for the caller to check, like a successful verify. A call that finds the structure in another operation's state fails without touching pkcs7->content. --- doc/dox_comments/header_files/pkcs7.h | 10 + tests/api/test_pkcs7.c | 312 ++++++++++++++++++++++++++ tests/api/test_pkcs7.h | 4 +- wolfcrypt/src/pkcs7.c | 60 ++++- wolfssl/wolfcrypt/pkcs7.h | 2 + 5 files changed, 386 insertions(+), 2 deletions(-) diff --git a/doc/dox_comments/header_files/pkcs7.h b/doc/dox_comments/header_files/pkcs7.h index 09ac05672ac..e64e179e3ec 100644 --- a/doc/dox_comments/header_files/pkcs7.h +++ b/doc/dox_comments/header_files/pkcs7.h @@ -328,6 +328,11 @@ int wc_PKCS7_EncodeSignedData_ex(wc_PKCS7* pkcs7, const byte* hashBuf, extracts the certificate list and certificate revocation list, and then verifies the signature. It stores the extracted content in the given PKCS7 structure. + Content that a verify leaves in pkcs7->content can point into the + input buffer, so it stays valid only while that buffer does and until + the next verify or wc_PKCS7_Free() on the same structure. A verify that + fails with an error other than PKCS7_SIGNEEDS_CHECK leaves pkcs7->content + as the caller set it. \return 0 Returned on successfully extracting the information from the message @@ -405,6 +410,11 @@ int wc_PKCS7_VerifySignedData(wc_PKCS7* pkcs7, hash/header/footer, then extracts the certificate list and certificate revocation list, and then verifies the signature. It stores the extracted content in the given PKCS7 structure. + Content that a verify leaves in pkcs7->content can point into the + input buffer, so it stays valid only while that buffer does and until + the next verify or wc_PKCS7_Free() on the same structure. A verify that + fails with an error other than PKCS7_SIGNEEDS_CHECK leaves pkcs7->content + as the caller set it. \return 0 Returned on successfully extracting the information from the message diff --git a/tests/api/test_pkcs7.c b/tests/api/test_pkcs7.c index 568654ecba7..ffa7ed11e35 100644 --- a/tests/api/test_pkcs7.c +++ b/tests/api/test_pkcs7.c @@ -9266,3 +9266,315 @@ int test_wc_PKCS7_CertFooterDecisionCoverage(void) #endif /* HAVE_PKCS7 && !NO_RSA && !NO_SHA256 */ return EXPECT_RESULT(); } + +#if defined(HAVE_PKCS7) && !defined(NO_RSA) && !defined(NO_SHA256) && \ + defined(USE_CERT_BUFFERS_2048) +/* Encode a SignedData carrying count copies of the client certificate, either + * certificates-only or signed with the client key. */ +static int pkcs7_encode_cert_set(byte* out, word32 outSz, int count, + int certsOnly, WC_RNG* rng) +{ + static const byte data[] = { 0x01, 0x02, 0x03 }; + PKCS7* pkcs7; + int ret = 0; + int i; + + pkcs7 = wc_PKCS7_New(HEAP_HINT, testDevId); + if (pkcs7 == NULL) + return MEMORY_E; + + if (!certsOnly) { + ret = wc_PKCS7_InitWithCert(pkcs7, (byte*)client_cert_der_2048, + sizeof_client_cert_der_2048); + } + for (i = 0; ret == 0 && i < count; i++) { + ret = wc_PKCS7_AddCertificate(pkcs7, (byte*)client_cert_der_2048, + sizeof_client_cert_der_2048); + } + if (ret == 0 && certsOnly) { + ret = wc_PKCS7_SetSignerIdentifierType(pkcs7, DEGENERATE_SID); + pkcs7->detached = 1; + } + if (ret == 0 && !certsOnly) { + pkcs7->content = (byte*)data; + pkcs7->contentSz = (word32)sizeof(data); + pkcs7->hashOID = SHA256h; + pkcs7->encryptOID = RSAk; + pkcs7->privateKey = (byte*)client_key_der_2048; + pkcs7->privateKeySz = sizeof_client_key_der_2048; + pkcs7->rng = rng; + } + if (ret == 0) { + pkcs7->contentOID = DATA; + ret = wc_PKCS7_EncodeSignedData(pkcs7, out, outSz); + } + + wc_PKCS7_Free(pkcs7); + return ret; +} +#endif + +/* Verifying again with an object whose last message carried content must not + * read that content after it is freed. */ +int test_wc_PKCS7_VerifySignedData_ReuseAfterContent(void) +{ + EXPECT_DECLS; +#if defined(HAVE_PKCS7) && !defined(NO_RSA) && !defined(NO_SHA256) && \ + defined(USE_CERT_BUFFERS_2048) + static const byte data[] = { 0x01, 0x02, 0x03 }; + PKCS7* pkcs7 = NULL; + PKCS7* other = NULL; + WC_RNG rng; + byte* signedMsg = NULL; + byte* certsOnly = NULL; + byte* detached = NULL; + byte* badMsg = NULL; + byte* msgCopy = NULL; + byte* head = NULL; + byte* foot = NULL; + word32 headSz = 0; + word32 footSz = 0; + byte hash[WC_SHA256_DIGEST_SIZE]; + byte bigData[64]; + word32 msgMax = 2 * sizeof_client_cert_der_2048 + FOURK_BUF; + int signedSz = 0; + int certsOnlySz = 0; + int detachedSz = 0; + int noContentRet = 0; + int i; + + XMEMSET(&rng, 0, sizeof(WC_RNG)); + XMEMSET(bigData, 0x5a, sizeof(bigData)); + ExpectIntEQ(wc_InitRng(&rng), 0); + ExpectNotNull(signedMsg = (byte*)XMALLOC(msgMax, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + ExpectNotNull(certsOnly = (byte*)XMALLOC(msgMax, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + ExpectNotNull(detached = (byte*)XMALLOC(msgMax, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + ExpectNotNull(badMsg = (byte*)XMALLOC(msgMax, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + ExpectIntGT((signedSz = pkcs7_encode_cert_set(signedMsg, msgMax, 0, 0, + &rng)), 0); + ExpectIntGT((certsOnlySz = pkcs7_encode_cert_set(certsOnly, msgMax, 1, 1, + &rng)), 0); + + ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_InitWithCert(other, (byte*)client_cert_der_2048, + sizeof_client_cert_der_2048), 0); + if (other != NULL) { + other->content = (byte*)data; + other->contentSz = (word32)sizeof(data); + other->contentOID = DATA; + other->hashOID = SHA256h; + other->encryptOID = RSAk; + other->privateKey = (byte*)client_key_der_2048; + other->privateKeySz = sizeof_client_key_der_2048; + other->rng = &rng; + other->detached = 1; + } + ExpectIntGT((detachedSz = wc_PKCS7_EncodeSignedData(other, detached, + msgMax)), 0); + wc_PKCS7_Free(other); + other = NULL; + + ExpectNotNull(pkcs7 = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, signedMsg, + (word32)signedSz), 0); + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, certsOnly, + (word32)certsOnlySz), 0); + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, signedMsg, + (word32)signedSz), 0); + if (pkcs7 != NULL) { + ExpectIntEQ(pkcs7->contentSz, 3); + } + + /* Detached content set by the caller is kept for the next call. */ + if (pkcs7 != NULL) { + pkcs7->content = (byte*)data; + pkcs7->contentSz = (word32)sizeof(data); + } + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, detached, + (word32)detachedSz), 0); + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, detached, + (word32)detachedSz), 0); + if (pkcs7 != NULL) { + ExpectPtrEq(pkcs7->content, data); + } + wc_PKCS7_Free(pkcs7); + pkcs7 = NULL; + + /* Content left by a verify that failed part way is not used either. */ + if (EXPECT_SUCCESS()) { + XMEMCPY(badMsg, signedMsg, (size_t)signedSz); + for (i = 0; i + 16 < signedSz; i++) { + if (XMEMCMP(badMsg + i, client_cert_der_2048, 16) == 0) { + badMsg[i + 2]++; + break; + } + } + ExpectIntLT(i + 16, signedSz); + } + ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); + if (other != NULL) { + noContentRet = wc_PKCS7_VerifySignedData(other, detached, + (word32)detachedSz); + } + ExpectIntLT(noContentRet, 0); + wc_PKCS7_Free(other); + other = NULL; + ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntLT(wc_PKCS7_VerifySignedData(other, badMsg, (word32)signedSz), 0); + ExpectIntEQ(wc_PKCS7_VerifySignedData(other, detached, + (word32)detachedSz), noContentRet); + wc_PKCS7_Free(other); + other = NULL; + + /* A failed verify leaves the caller's content in place. */ + ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); + if (other != NULL) { + other->content = (byte*)data; + other->contentSz = (word32)sizeof(data); + } + ExpectIntLT(wc_PKCS7_VerifySignedData(other, badMsg, (word32)signedSz), 0); + if (other != NULL) { + ExpectPtrEq(other->content, data); + ExpectIntEQ(other->contentSz, sizeof(data)); + } + ExpectIntEQ(wc_PKCS7_VerifySignedData(other, detached, + (word32)detachedSz), 0); + wc_PKCS7_Free(other); + other = NULL; + + /* Content a verify handed back is not used by the next verify, even when + * it points into a message buffer the caller has freed since. */ + ExpectNotNull(msgCopy = (byte*)XMALLOC(msgMax, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + if (msgCopy != NULL) { + XMEMCPY(msgCopy, signedMsg, (size_t)signedSz); + } + ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_VerifySignedData(other, msgCopy, (word32)signedSz), + 0); + if (msgCopy != NULL) { + XMEMSET(msgCopy, 0, (size_t)signedSz); + } + XFREE(msgCopy, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + ExpectIntEQ(wc_PKCS7_VerifySignedData(other, detached, + (word32)detachedSz), noContentRet); + wc_PKCS7_Free(other); + other = NULL; + + /* A message without certificates hands its content back for the caller + * to check, and the next verify drops it like any other. */ + ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_InitWithCert(other, (byte*)client_cert_der_2048, + sizeof_client_cert_der_2048), 0); + ExpectIntEQ(wc_PKCS7_SetNoCerts(other, 1), 0); + if (other != NULL) { + other->content = (byte*)data; + other->contentSz = (word32)sizeof(data); + other->contentOID = DATA; + other->hashOID = SHA256h; + other->encryptOID = RSAk; + other->privateKey = (byte*)client_key_der_2048; + other->privateKeySz = sizeof_client_key_der_2048; + other->rng = &rng; + } + ExpectIntGT((signedSz = wc_PKCS7_EncodeSignedData(other, signedMsg, + msgMax)), 0); + wc_PKCS7_Free(other); + other = NULL; + ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_InitWithCert(other, NULL, 0), 0); + ExpectIntEQ(wc_PKCS7_VerifySignedData(other, signedMsg, (word32)signedSz), + WC_NO_ERR_TRACE(PKCS7_SIGNEEDS_CHECK)); + if (other != NULL) { + ExpectIntEQ(other->contentSz, sizeof(data)); + ExpectNotNull(other->content); + if (other->content != NULL) { + ExpectBufEQ(other->content, data, sizeof(data)); + } + } + ExpectIntEQ(wc_PKCS7_VerifySignedData(other, detached, + (word32)detachedSz), noContentRet); + wc_PKCS7_Free(other); + other = NULL; + + /* A header/footer verify after a full one keeps the contentSz the caller + * set even though the old content pointer is still in place. */ + ExpectNotNull(head = (byte*)XMALLOC(msgMax, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + ExpectNotNull(foot = (byte*)XMALLOC(msgMax, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + ExpectIntEQ(wc_Sha256Hash(bigData, (word32)sizeof(bigData), hash), 0); + headSz = msgMax; + footSz = msgMax; + ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_InitWithCert(other, (byte*)client_cert_der_2048, + sizeof_client_cert_der_2048), 0); + if (other != NULL) { + other->contentSz = (word32)sizeof(bigData); + other->hashOID = SHA256h; + other->encryptOID = RSAk; + other->privateKey = (byte*)client_key_der_2048; + other->privateKeySz = sizeof_client_key_der_2048; + other->rng = &rng; + } + ExpectIntEQ(wc_PKCS7_EncodeSignedData_ex(other, hash, sizeof(hash), head, + &headSz, foot, &footSz), 0); + wc_PKCS7_Free(other); + other = NULL; + ExpectIntGT((signedSz = pkcs7_encode_cert_set(signedMsg, msgMax, 0, 0, + &rng)), 0); + ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_VerifySignedData(other, signedMsg, (word32)signedSz), + 0); + if (other != NULL) { + other->contentSz = (word32)sizeof(bigData); + } + ExpectIntEQ(wc_PKCS7_VerifySignedData_ex(other, hash, sizeof(hash), head, + headSz, foot, footSz), 0); + wc_PKCS7_Free(other); + other = NULL; + + /* Larger detached content than the previous verify left behind. */ + ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_InitWithCert(other, (byte*)client_cert_der_2048, + sizeof_client_cert_der_2048), 0); + if (other != NULL) { + other->content = bigData; + other->contentSz = (word32)sizeof(bigData); + other->contentOID = DATA; + other->hashOID = SHA256h; + other->encryptOID = RSAk; + other->privateKey = (byte*)client_key_der_2048; + other->privateKeySz = sizeof_client_key_der_2048; + other->rng = &rng; + other->detached = 1; + } + ExpectIntGT((detachedSz = wc_PKCS7_EncodeSignedData(other, detached, + msgMax)), 0); + wc_PKCS7_Free(other); + other = NULL; + ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_VerifySignedData(other, signedMsg, (word32)signedSz), + 0); + if (other != NULL) { + other->content = bigData; + other->contentSz = (word32)sizeof(bigData); + } + ExpectIntEQ(wc_PKCS7_VerifySignedData(other, detached, + (word32)detachedSz), 0); + wc_PKCS7_Free(other); + + XFREE(foot, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(head, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(badMsg, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(detached, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(certsOnly, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(signedMsg, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + wc_FreeRng(&rng); +#endif + return EXPECT_RESULT(); +} diff --git a/tests/api/test_pkcs7.h b/tests/api/test_pkcs7.h index 1dee47e6825..c41644ca401 100644 --- a/tests/api/test_pkcs7.h +++ b/tests/api/test_pkcs7.h @@ -102,6 +102,7 @@ int test_wc_PKCS7_VerifySignedData_DegenerateNonEmptyDigestAlgos(void); int test_wc_PKCS7_VerifySignedData_DegenerateWithCert(void); int test_wc_PKCS7_VerifySignedData_NoDegenerateAcceptsRealSigner(void); int test_wc_PKCS7_VerifySignedData_NoDigestParams(void); +int test_wc_PKCS7_VerifySignedData_ReuseAfterContent(void); #define TEST_PKCS7_DECLS \ @@ -167,7 +168,8 @@ int test_wc_PKCS7_VerifySignedData_NoDigestParams(void); TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_DegenerateNonEmptyDigestAlgos), \ TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_DegenerateWithCert), \ TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_NoDegenerateAcceptsRealSigner), \ - TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_NoDigestParams) + TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_NoDigestParams), \ + TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_ReuseAfterContent) #define TEST_PKCS7_ENCRYPTED_DATA_DECLS \ TEST_DECL_GROUP("pkcs7_ed", test_wc_PKCS7_DecodeEnvelopedData_stream), \ diff --git a/wolfcrypt/src/pkcs7.c b/wolfcrypt/src/pkcs7.c index 10eba731316..e6057d3f773 100644 --- a/wolfcrypt/src/pkcs7.c +++ b/wolfcrypt/src/pkcs7.c @@ -155,6 +155,7 @@ struct PKCS7State { byte* aad; /* additional data for AEAD algos */ byte* tag; /* tag data for AEAD algos */ byte* content; + byte* callerContent; /* pkcs7->content set by the caller */ byte* buffer; /* main internal read buffer */ wc_HashAlg hashAlg; @@ -182,6 +183,7 @@ struct PKCS7State { word32 fragCarrySz; /* bytes held in fragCarry */ word32 contentCap; /* allocated size of reassembled content */ word32 contentSz; + word32 callerContentSz; word32 currContIdx; /* index of current content */ word32 currContSz; /* size of current content */ word32 currContRmnSz; /* remaining size of current content */ @@ -7033,6 +7035,23 @@ static int wc_PKCS7_HandleOctetStrings(wc_PKCS7* pkcs7, byte* in, word32 inSz, return ret; } #endif /* !NO_PKCS7_STREAM */ + +/* Drop content that a previous verify left in pkcs7->content. keepSz keeps + * contentSz, which a header/footer verify takes from the caller. */ +static void wc_PKCS7_DropVerifyContent(wc_PKCS7* pkcs7, int keepSz) +{ + if (pkcs7->content != NULL && + (pkcs7->content == pkcs7->verifyContent || + pkcs7->content == pkcs7->contentDynamic)) { + pkcs7->content = NULL; + if (!keepSz) + pkcs7->contentSz = 0; + } + pkcs7->verifyContent = NULL; + XFREE(pkcs7->contentDynamic, pkcs7->heap, DYNAMIC_TYPE_PKCS7); + pkcs7->contentDynamic = NULL; +} + /* Finds the certificates in the message and saves it. By default allows * degenerate cases which can have no signer. * @@ -7077,7 +7096,11 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, enum wc_HashType hashType = WC_HASH_TYPE_NONE; byte* src = NULL; word32 srcSz; +#else + word32 callerContentSz; #endif + byte* callerContent = NULL; + byte restoreContent = 1; byte* pkiMsg2 = in2; word32 pkiMsg2Sz = in2Sz; (void)keepContent; @@ -7098,6 +7121,11 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, return BAD_FUNC_ARG; } idx = 0; +#ifdef NO_PKCS7_STREAM + wc_PKCS7_DropVerifyContent(pkcs7, in2 != NULL && in2Sz > 0); + callerContent = pkcs7->content; + callerContentSz = pkcs7->contentSz; +#endif #ifdef ASN_BER_TO_DER if (pkcs7->derSz > 0 && pkcs7->der) { @@ -7116,6 +7144,10 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, switch (pkcs7->state) { case WC_PKCS7_START: #ifndef NO_PKCS7_STREAM + wc_PKCS7_DropVerifyContent(pkcs7, in2 != NULL && in2Sz > 0); + pkcs7->stream->callerContent = pkcs7->content; + pkcs7->stream->callerContentSz = pkcs7->contentSz; + /* The expected size calculation originally assumed digest OID * with NULL params, -2 to also accept with absent params */ if ((ret = wc_PKCS7_AddDataToStream(pkcs7, in, inSz, (MAX_SEQ_SZ + @@ -8053,7 +8085,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, #endif /* Save dynamic content before freeing PKCS7 struct */ - if (pkcs7->contentDynamic != NULL) { + if (pkcs7->contentDynamic != NULL && contentSz > 0) { contentDynamic = (byte*)XMALLOC((word32)contentSz, pkcs7->heap, DYNAMIC_TYPE_PKCS7); if (contentDynamic == NULL) { @@ -8414,6 +8446,14 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, pkcs7->content = content; pkcs7->contentSz = (word32)contentSz; + #ifndef NO_PKCS7_STREAM + /* keep the caller's detached content, not the stream copy */ + if (pkcs7->stream->detached && + pkcs7->stream->callerContent != NULL) { + pkcs7->content = pkcs7->stream->callerContent; + pkcs7->contentSz = pkcs7->stream->callerContentSz; + } + #endif if (ret == 0) { #if !defined(NO_PKCS7_STREAM) && defined(ASN_BER_TO_DER) @@ -8478,14 +8518,32 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, default: WOLFSSL_MSG("PKCS7 Unknown verify state"); ret = BAD_FUNC_ARG; + restoreContent = 0; } if (ret != 0 && ret != WC_NO_ERR_TRACE(WC_PKCS7_WANT_READ_E)) { + /* a failed verify leaves the caller's content as it was */ + if (restoreContent && ret != WC_NO_ERR_TRACE(PKCS7_SIGNEEDS_CHECK)) { + #ifndef NO_PKCS7_STREAM + pkcs7->content = pkcs7->stream->callerContent; + pkcs7->contentSz = pkcs7->stream->callerContentSz; + #else + pkcs7->content = callerContent; + pkcs7->contentSz = callerContentSz; + #endif + } #ifndef NO_PKCS7_STREAM wc_PKCS7_ResetStream(pkcs7); #endif wc_PKCS7_ChangeState(pkcs7, WC_PKCS7_START); } + if (ret == 0 || ret == WC_NO_ERR_TRACE(PKCS7_SIGNEEDS_CHECK)) { + #ifndef NO_PKCS7_STREAM + callerContent = pkcs7->stream->callerContent; + #endif + if (pkcs7->content != callerContent) + pkcs7->verifyContent = pkcs7->content; + } return ret; } diff --git a/wolfssl/wolfcrypt/pkcs7.h b/wolfssl/wolfcrypt/pkcs7.h index 350748f4e65..1d0a203b8f4 100644 --- a/wolfssl/wolfcrypt/pkcs7.h +++ b/wolfssl/wolfcrypt/pkcs7.h @@ -428,6 +428,8 @@ struct wc_PKCS7 { #ifdef WC_ASN_UNKNOWN_EXT_CB wc_UnknownExtCallback32 unknownExtCallback32; #endif + byte* verifyContent; /* pkcs7->content set by the last verify */ + /* !! NEW DATA MEMBERS MUST BE ADDED AT END !! */ }; From 53868c614eb8441846f01de5fb137f37391b8f42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Mon, 5 Oct 2026 14:29:44 +0200 Subject: [PATCH 2/7] Refuse certs-only PKCS#7 bundles that overflow MAX_PKCS7_CERTS wc_PKCS7_VerifySignedData() stores the certificates of a SignedData in pkcs7->cert[], which holds MAX_PKCS7_CERTS entries (4 by default, 15 with OPENSSL_ALL). Certificates past that were dropped without an error, so a caller reading cert[] got a short list and a success return. In a certificates-only (degenerate) bundle the certificates are the whole payload, for example an RFC 7030 /cacerts or a SCEP GetCACert response, so the truncation silently loses trust anchor material. Return BUFFER_E when a degenerate bundle holds more certificates than cert[] can store. A bundle is only known to be degenerate once the signerInfos SET has been read, so the overflow is recorded while the certificate set is parsed and checked there. The flag is kept in the stream state so it survives a streaming call that returns WC_PKCS7_WANT_READ_E in between. A bundle that wc_PKCS7_AllowDegenerate() disallows still fails with PKCS7_NO_SIGNER_E. Signed messages keep the current behaviour. Their signer certificate is looked up in cert[], and a message carrying more than MAX_PKCS7_CERTS certificates verifies today when the signer is among the first ones, so refusing it would break existing users. A debug message notes that the extra certificates were not stored. --- doc/dox_comments/header_files/pkcs7.h | 6 +- tests/api/test_ossl_p7p12.c | 73 +++++++++++ tests/api/test_ossl_p7p12.h | 2 + tests/api/test_pkcs7.c | 176 ++++++++++++++++++++++++++ tests/api/test_pkcs7.h | 4 +- wolfcrypt/src/pkcs7.c | 26 ++++ 6 files changed, 284 insertions(+), 3 deletions(-) diff --git a/doc/dox_comments/header_files/pkcs7.h b/doc/dox_comments/header_files/pkcs7.h index e64e179e3ec..f035a7acbc3 100644 --- a/doc/dox_comments/header_files/pkcs7.h +++ b/doc/dox_comments/header_files/pkcs7.h @@ -345,7 +345,8 @@ int wc_PKCS7_EncodeSignedData_ex(wc_PKCS7* pkcs7, const byte* hashBuf, \return RSA_BUFFER_E Returned if buffer error, output too small or input too large \return BUFFER_E Returned if the given buffer is not large enough to - hold the encoded certificate + hold the encoded certificate, or if a certificates-only (degenerate) + message holds more than MAX_PKCS7_CERTS certificates \return MP_INIT_E may be returned if there is an error generating the signature \return MP_READ_E may be returned if there is an error generating @@ -427,7 +428,8 @@ int wc_PKCS7_VerifySignedData(wc_PKCS7* pkcs7, \return RSA_BUFFER_E Returned if buffer error, output too small or input too large \return BUFFER_E Returned if the given buffer is not large enough to - hold the encoded certificate + hold the encoded certificate, or if a certificates-only (degenerate) + message holds more than MAX_PKCS7_CERTS certificates \return MP_INIT_E may be returned if there is an error generating the signature \return MP_READ_E may be returned if there is an error generating diff --git a/tests/api/test_ossl_p7p12.c b/tests/api/test_ossl_p7p12.c index 60eca703497..f2b0ddb1c93 100644 --- a/tests/api/test_ossl_p7p12.c +++ b/tests/api/test_ossl_p7p12.c @@ -187,6 +187,79 @@ int test_wolfSSL_PKCS7_certs(void) return EXPECT_RESULT(); } +/* d2i_PKCS7() loads a certificates-only bundle that fills cert[] and refuses + * one with more certificates instead of returning a truncated stack. */ +int test_wolfSSL_PKCS7_certs_over_limit(void) +{ + EXPECT_DECLS; +#if defined(OPENSSL_ALL) && !defined(NO_CERTS) && !defined(NO_BIO) && \ + !defined(NO_RSA) && defined(HAVE_PKCS7) && defined(USE_CERT_BUFFERS_2048) + STACK_OF(X509)* sk = NULL; + STACK_OF(X509)* certs = NULL; + X509* x509 = NULL; + PKCS7* p7 = NULL; + BIO* bio = NULL; + const byte* p = NULL; + const byte* der = NULL; + int buflen = 0; + int count; + int i; + + for (count = MAX_PKCS7_CERTS; count <= MAX_PKCS7_CERTS + 1; count++) { + ExpectNotNull(p7 = PKCS7_new()); + if (p7 != NULL) { + p7->version = 1; + #ifdef NO_SHA + p7->hashOID = SHA256h; + #else + p7->hashOID = SHAh; + #endif + } + ExpectNotNull(sk = sk_X509_new_null()); + for (i = 0; EXPECT_SUCCESS() && i < count; i++) { + der = client_cert_der_2048; + ExpectNotNull(x509 = d2i_X509(NULL, &der, + sizeof_client_cert_der_2048)); + ExpectIntGT(sk_X509_push(sk, x509), 0); + if (EXPECT_FAIL()) { + X509_free(x509); + } + x509 = NULL; + } + ExpectNotNull(bio = BIO_new(BIO_s_mem())); + ExpectIntEQ(wolfSSL_PKCS7_encode_certs(p7, sk, bio), 1); + /* encode_certs takes sk only on success. */ + if (EXPECT_SUCCESS()) { + sk = NULL; + } + else if (sk != NULL) { + sk_X509_pop_free(sk, X509_free); + sk = NULL; + } + ExpectIntGT((buflen = BIO_get_mem_data(bio, &p)), 0); + PKCS7_free(p7); + p7 = NULL; + + if (count == MAX_PKCS7_CERTS) { + ExpectNotNull(p7 = d2i_PKCS7(NULL, &p, buflen)); + ExpectNotNull(certs = wolfSSL_PKCS7_to_stack(p7)); + ExpectIntEQ(sk_X509_num(certs), count); + } + else { + ExpectNull(p7 = d2i_PKCS7(NULL, &p, buflen)); + } + + /* PKCS7_free frees the certs */ + PKCS7_free(p7); + p7 = NULL; + certs = NULL; + BIO_free(bio); + bio = NULL; + } +#endif + return EXPECT_RESULT(); +} + int test_wolfSSL_PKCS7_sign(void) { EXPECT_DECLS; diff --git a/tests/api/test_ossl_p7p12.h b/tests/api/test_ossl_p7p12.h index 1f1e283f528..0a18f9c0cb8 100644 --- a/tests/api/test_ossl_p7p12.h +++ b/tests/api/test_ossl_p7p12.h @@ -26,6 +26,7 @@ int test_wolfssl_PKCS7(void); int test_wolfSSL_PKCS7_certs(void); +int test_wolfSSL_PKCS7_certs_over_limit(void); int test_wolfSSL_PKCS7_sign(void); int test_wolfSSL_PKCS7_verify_signer_forgery(void); int test_wolfSSL_PKCS7_verify_sid_binding(void); @@ -42,6 +43,7 @@ int test_wolfSSL_PKCS12_create_mac(void); #define TEST_OSSL_PKCS7_DECLS \ TEST_DECL_GROUP("ossl_p7", test_wolfssl_PKCS7), \ TEST_DECL_GROUP("ossl_p7", test_wolfSSL_PKCS7_certs), \ + TEST_DECL_GROUP("ossl_p7", test_wolfSSL_PKCS7_certs_over_limit), \ TEST_DECL_GROUP("ossl_p7", test_wolfSSL_PKCS7_sign), \ TEST_DECL_GROUP("ossl_p7", test_wolfSSL_PKCS7_verify_signer_forgery), \ TEST_DECL_GROUP("ossl_p7", test_wolfSSL_PKCS7_verify_sid_binding), \ diff --git a/tests/api/test_pkcs7.c b/tests/api/test_pkcs7.c index ffa7ed11e35..be011112977 100644 --- a/tests/api/test_pkcs7.c +++ b/tests/api/test_pkcs7.c @@ -9578,3 +9578,179 @@ int test_wc_PKCS7_VerifySignedData_ReuseAfterContent(void) #endif return EXPECT_RESULT(); } + +#if defined(HAVE_PKCS7) && !defined(NO_RSA) && !defined(NO_SHA256) && \ + defined(USE_CERT_BUFFERS_2048) && !defined(NO_PKCS7_STREAM) +/* Feed a message to wc_PKCS7_VerifySignedData() one byte per call. */ +static int pkcs7_verify_bytewise(PKCS7* pkcs7, byte* in, int inSz) +{ + int ret = WC_NO_ERR_TRACE(WC_PKCS7_WANT_READ_E); + int z; + + for (z = 0; z < inSz && ret == WC_NO_ERR_TRACE(WC_PKCS7_WANT_READ_E); + z++) { + ret = wc_PKCS7_VerifySignedData(pkcs7, in + z, 1); + } + return ret; +} +#endif + +#if defined(HAVE_PKCS7) && !defined(NO_RSA) && !defined(NO_SHA256) && \ + defined(USE_CERT_BUFFERS_2048) +/* Write a DER tag and definite length, or only size them when out is NULL. */ +static word32 pkcs7_der_hdr(byte* out, byte tag, word32 len) +{ + word32 lenBytes = 0; + word32 n; + word32 i; + + if (len >= 0x80) { + for (n = len; n > 0; n >>= 8) + lenBytes++; + } + if (out != NULL) { + out[0] = tag; + if (lenBytes == 0) { + out[1] = (byte)len; + } + else { + out[1] = (byte)(ASN_LONG_LENGTH | lenBytes); + for (i = 0; i < lenBytes; i++) + out[2 + i] = (byte)(len >> (8 * (lenBytes - 1 - i))); + } + } + return 2 + lenBytes; +} + +/* Encode a certificates-only SignedData holding count copies of the client + * certificate, listing SHA-256 in digestAlgorithms when digestAlg is set. */ +static int pkcs7_encode_raw_cert_set(byte* out, word32 outSz, int count, + int digestAlg) +{ + static const byte signedDataOid[] = { + 0x06, 0x09, 0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x07, 0x02 + }; + static const byte version[] = { 0x02, 0x01, 0x01 }; + static const byte noDigestAlgs[] = { 0x31, 0x00 }; + static const byte sha256DigestAlgs[] = { + 0x31, 0x0F, 0x30, 0x0D, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, + 0x03, 0x04, 0x02, 0x01, 0x05, 0x00 + }; + static const byte dataContentInfo[] = { + 0x30, 0x0B, 0x06, 0x09, 0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, + 0x07, 0x01 + }; + static const byte noSigners[] = { 0x31, 0x00 }; + const byte* digestAlgs = digestAlg ? sha256DigestAlgs : noDigestAlgs; + word32 digestAlgsSz = digestAlg ? (word32)sizeof(sha256DigestAlgs) : + (word32)sizeof(noDigestAlgs); + word32 setSz; + word32 sdSz; + word32 expSz; + word32 ciSz; + word32 idx = 0; + int i; + + setSz = (word32)count * (word32)sizeof_client_cert_der_2048; + sdSz = (word32)sizeof(version) + digestAlgsSz + + (word32)sizeof(dataContentInfo) + pkcs7_der_hdr(NULL, 0, setSz) + + setSz + (word32)sizeof(noSigners); + expSz = pkcs7_der_hdr(NULL, 0, sdSz) + sdSz; + ciSz = (word32)sizeof(signedDataOid) + pkcs7_der_hdr(NULL, 0, expSz) + + expSz; + if (pkcs7_der_hdr(NULL, 0, ciSz) + ciSz > outSz) + return BUFFER_E; + + idx += pkcs7_der_hdr(out + idx, ASN_CONSTRUCTED | ASN_SEQUENCE, ciSz); + XMEMCPY(out + idx, signedDataOid, sizeof(signedDataOid)); + idx += (word32)sizeof(signedDataOid); + idx += pkcs7_der_hdr(out + idx, ASN_CONSTRUCTED | ASN_CONTEXT_SPECIFIC, + expSz); + idx += pkcs7_der_hdr(out + idx, ASN_CONSTRUCTED | ASN_SEQUENCE, sdSz); + XMEMCPY(out + idx, version, sizeof(version)); + idx += (word32)sizeof(version); + XMEMCPY(out + idx, digestAlgs, digestAlgsSz); + idx += digestAlgsSz; + XMEMCPY(out + idx, dataContentInfo, sizeof(dataContentInfo)); + idx += (word32)sizeof(dataContentInfo); + idx += pkcs7_der_hdr(out + idx, ASN_CONSTRUCTED | ASN_CONTEXT_SPECIFIC, + setSz); + for (i = 0; i < count; i++) { + XMEMCPY(out + idx, client_cert_der_2048, sizeof_client_cert_der_2048); + idx += (word32)sizeof_client_cert_der_2048; + } + XMEMCPY(out + idx, noSigners, sizeof(noSigners)); + idx += (word32)sizeof(noSigners); + + return (int)idx; +} +#endif + +/* A certificates-only bundle with more certificates than cert[] holds is + * refused instead of being truncated; a signed message still verifies. */ +int test_wc_PKCS7_VerifySignedData_CertSetOverflow(void) +{ + EXPECT_DECLS; +#if defined(HAVE_PKCS7) && !defined(NO_RSA) && !defined(NO_SHA256) && \ + defined(USE_CERT_BUFFERS_2048) + PKCS7* pkcs7 = NULL; + WC_RNG rng; + byte* bundle = NULL; + byte* fit = NULL; + word32 bundleMax = (MAX_PKCS7_CERTS + 2) * sizeof_client_cert_der_2048 + + FOURK_BUF; + int bundleSz = 0; + int fitSz = 0; + + XMEMSET(&rng, 0, sizeof(WC_RNG)); + ExpectIntEQ(wc_InitRng(&rng), 0); + ExpectNotNull(bundle = (byte*)XMALLOC(bundleMax, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + ExpectNotNull(fit = (byte*)XMALLOC(bundleMax, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + + ExpectIntGT((fitSz = pkcs7_encode_cert_set(fit, bundleMax, + MAX_PKCS7_CERTS, 1, &rng)), 0); + ExpectNotNull(pkcs7 = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, fit, (word32)fitSz), 0); + if (pkcs7 != NULL) { + ExpectNotNull(pkcs7->cert[MAX_PKCS7_CERTS - 1]); + } + + /* The same object accepts a bundle that fits after each refusal. */ + ExpectIntGT((bundleSz = pkcs7_encode_cert_set(bundle, bundleMax, + MAX_PKCS7_CERTS + 1, 1, &rng)), 0); + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, bundle, (word32)bundleSz), + WC_NO_ERR_TRACE(BUFFER_E)); + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, fit, (word32)fitSz), 0); + + /* With degenerate bundles disallowed, the policy error is reported. */ + ExpectIntGT((bundleSz = pkcs7_encode_raw_cert_set(bundle, bundleMax, + MAX_PKCS7_CERTS + 1, 1)), 0); + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, bundle, (word32)bundleSz), + WC_NO_ERR_TRACE(BUFFER_E)); + wc_PKCS7_AllowDegenerate(pkcs7, 0); + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, bundle, (word32)bundleSz), + WC_NO_ERR_TRACE(PKCS7_NO_SIGNER_E)); + wc_PKCS7_AllowDegenerate(pkcs7, 1); + ExpectIntGT((bundleSz = pkcs7_encode_cert_set(bundle, bundleMax, + MAX_PKCS7_CERTS + 1, 1, &rng)), 0); + +#ifndef NO_PKCS7_STREAM + ExpectIntEQ(pkcs7_verify_bytewise(pkcs7, bundle, bundleSz), + WC_NO_ERR_TRACE(BUFFER_E)); + ExpectIntEQ(pkcs7_verify_bytewise(pkcs7, fit, fitSz), 0); +#endif + + ExpectIntGT((bundleSz = pkcs7_encode_cert_set(bundle, bundleMax, + MAX_PKCS7_CERTS + 1, 0, &rng)), 0); + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, bundle, (word32)bundleSz), 0); + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, fit, (word32)fitSz), 0); + wc_PKCS7_Free(pkcs7); + + XFREE(fit, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(bundle, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + wc_FreeRng(&rng); +#endif + return EXPECT_RESULT(); +} diff --git a/tests/api/test_pkcs7.h b/tests/api/test_pkcs7.h index c41644ca401..5ac515c7693 100644 --- a/tests/api/test_pkcs7.h +++ b/tests/api/test_pkcs7.h @@ -103,6 +103,7 @@ int test_wc_PKCS7_VerifySignedData_DegenerateWithCert(void); int test_wc_PKCS7_VerifySignedData_NoDegenerateAcceptsRealSigner(void); int test_wc_PKCS7_VerifySignedData_NoDigestParams(void); int test_wc_PKCS7_VerifySignedData_ReuseAfterContent(void); +int test_wc_PKCS7_VerifySignedData_CertSetOverflow(void); #define TEST_PKCS7_DECLS \ @@ -169,7 +170,8 @@ int test_wc_PKCS7_VerifySignedData_ReuseAfterContent(void); TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_DegenerateWithCert), \ TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_NoDegenerateAcceptsRealSigner), \ TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_NoDigestParams), \ - TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_ReuseAfterContent) + TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_ReuseAfterContent), \ + TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_CertSetOverflow) #define TEST_PKCS7_ENCRYPTED_DATA_DECLS \ TEST_DECL_GROUP("pkcs7_ed", test_wc_PKCS7_DecodeEnvelopedData_stream), \ diff --git a/wolfcrypt/src/pkcs7.c b/wolfcrypt/src/pkcs7.c index e6057d3f773..6d1a096cec9 100644 --- a/wolfcrypt/src/pkcs7.c +++ b/wolfcrypt/src/pkcs7.c @@ -204,6 +204,7 @@ struct PKCS7State { WC_BITFIELD indefLen:1; /* flag to indicate indef-length encoding used */ WC_BITFIELD indefEci:1; /* EncryptedContentInfo is indefinite */ WC_BITFIELD indefContent:1; /* encryptedContent is fragmented */ + WC_BITFIELD certSetOverflow:1; /* certs set exceeds MAX_PKCS7_CERTS */ }; @@ -292,6 +293,7 @@ static void wc_PKCS7_ResetStream(wc_PKCS7* pkcs7) pkcs7->stream->varThree = 0; pkcs7->stream->noContent = 0; pkcs7->stream->indefLen = 0; + pkcs7->stream->certSetOverflow = 0; pkcs7->stream->cntIdfCnt = 0; pkcs7->stream->currContIdx = 0; pkcs7->stream->currContSz = 0; @@ -7077,6 +7079,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, word32 localIdx, start; word32 certIdx, certIdx2; byte degenerate = 0; + byte certSetOverflow = 0; byte detached = 0; byte noContent = 0; byte tag = 0; @@ -8179,6 +8182,18 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, certIdx += (word32)sz; } } + + /* cert[] is full and another certificate follows */ + if (ret == 0 && i == MAX_PKCS7_CERTS && + certIdx + 1 < pkiMsg2Sz && + certIdx + 1 < certSetEnd && + pkiMsg2[certIdx] == + (ASN_CONSTRUCTED | ASN_SEQUENCE)) { + certSetOverflow = 1; + } + #ifndef NO_PKCS7_STREAM + pkcs7->stream->certSetOverflow = (certSetOverflow != 0); + #endif } } idx += (word32)length; @@ -8348,9 +8363,20 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, degenerate = (length == 0) ? 1 : 0; #ifndef NO_PKCS7_STREAM pkcs7->stream->degenerate = (degenerate != 0); + certSetOverflow = pkcs7->stream->certSetOverflow; #endif } + if (ret == 0 && degenerate && certSetOverflow && + !pkcs7->noDegenerate) { + WOLFSSL_MSG("Certificates-only bundle holds more than " + "MAX_PKCS7_CERTS certificates"); + ret = BUFFER_E; + } + else if (ret == 0 && certSetOverflow) { + WOLFSSL_MSG("Certificates past MAX_PKCS7_CERTS are not stored"); + } + if (ret != 0) break; #ifndef NO_PKCS7_STREAM From 46010730aab3df3cdc27d7cd456351242f359c68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Mon, 5 Oct 2026 15:08:14 +0200 Subject: [PATCH 3/7] Skip non-X.509 CertificateChoices in the PKCS#7 certificate set The loop collecting the certificates of a SignedData only read the length of SEQUENCE elements. For any other CertificateChoices alternative of RFC 5652, such as a [2] attribute certificate, it moved on by the tag byte alone and still used up a cert[] slot. It then walked the inside of that element one byte at a time, left NULL holes in cert[], and could take a 0x30 byte inside it for a certificate. Since the previous change counts slots to detect an overflowing certificates-only bundle, such a bundle could be refused although it held no more than MAX_PKCS7_CERTS certificates. Read the tag and length of every element and step over the ones that are not X.509 certificates. Only stored certificates use a slot, and cert[] is filled from the first free slot, so a set that starts with an attribute certificate puts its first X.509 certificate in cert[0]. wolfSSL_PKCS7_to_stack() stops at the first NULL entry and so returned no certificates for such a set. The overflow is now flagged when an X.509 certificate arrives with cert[] already full, wherever it sits in the set. Elements are bounded by the end of the certificate set rather than the end of the buffer, which in builds without streaming is the whole message. An element whose length runs past the set, or a lone tag byte at the end of the set, now fails with ASN_PARSE_E in every build instead of being stepped over. So does an element with an indefinite length: GetLength() reports it as zero bytes long, so the loop would walk into its contents and store the SEQUENCEs nested in it as certificates, which could also flag a false overflow. --- tests/api/test_pkcs7.c | 101 ++++++++++++++++++++++++++++++++++++++--- tests/api/test_pkcs7.h | 4 +- wolfcrypt/src/pkcs7.c | 47 +++++++------------ 3 files changed, 113 insertions(+), 39 deletions(-) diff --git a/tests/api/test_pkcs7.c b/tests/api/test_pkcs7.c index be011112977..7ece4136981 100644 --- a/tests/api/test_pkcs7.c +++ b/tests/api/test_pkcs7.c @@ -9623,9 +9623,10 @@ static word32 pkcs7_der_hdr(byte* out, byte tag, word32 len) } /* Encode a certificates-only SignedData holding count copies of the client - * certificate, listing SHA-256 in digestAlgorithms when digestAlg is set. */ + * certificate, listing SHA-256 in digestAlgorithms when digestAlg is set and + * placing the attrSz bytes of attr before copy attrPos. */ static int pkcs7_encode_raw_cert_set(byte* out, word32 outSz, int count, - int digestAlg) + int digestAlg, const byte* attr, word32 attrSz, int attrPos) { static const byte signedDataOid[] = { 0x06, 0x09, 0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x07, 0x02 @@ -9651,7 +9652,7 @@ static int pkcs7_encode_raw_cert_set(byte* out, word32 outSz, int count, word32 idx = 0; int i; - setSz = (word32)count * (word32)sizeof_client_cert_der_2048; + setSz = attrSz + (word32)count * (word32)sizeof_client_cert_der_2048; sdSz = (word32)sizeof(version) + digestAlgsSz + (word32)sizeof(dataContentInfo) + pkcs7_der_hdr(NULL, 0, setSz) + setSz + (word32)sizeof(noSigners); @@ -9675,9 +9676,16 @@ static int pkcs7_encode_raw_cert_set(byte* out, word32 outSz, int count, idx += (word32)sizeof(dataContentInfo); idx += pkcs7_der_hdr(out + idx, ASN_CONSTRUCTED | ASN_CONTEXT_SPECIFIC, setSz); - for (i = 0; i < count; i++) { - XMEMCPY(out + idx, client_cert_der_2048, sizeof_client_cert_der_2048); - idx += (word32)sizeof_client_cert_der_2048; + for (i = 0; i <= count; i++) { + if (i == attrPos) { + XMEMCPY(out + idx, attr, attrSz); + idx += attrSz; + } + if (i < count) { + XMEMCPY(out + idx, client_cert_der_2048, + sizeof_client_cert_der_2048); + idx += (word32)sizeof_client_cert_der_2048; + } } XMEMCPY(out + idx, noSigners, sizeof(noSigners)); idx += (word32)sizeof(noSigners); @@ -9726,7 +9734,7 @@ int test_wc_PKCS7_VerifySignedData_CertSetOverflow(void) /* With degenerate bundles disallowed, the policy error is reported. */ ExpectIntGT((bundleSz = pkcs7_encode_raw_cert_set(bundle, bundleMax, - MAX_PKCS7_CERTS + 1, 1)), 0); + MAX_PKCS7_CERTS + 1, 1, NULL, 0, -1)), 0); ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, bundle, (word32)bundleSz), WC_NO_ERR_TRACE(BUFFER_E)); wc_PKCS7_AllowDegenerate(pkcs7, 0); @@ -9754,3 +9762,82 @@ int test_wc_PKCS7_VerifySignedData_CertSetOverflow(void) #endif return EXPECT_RESULT(); } + +/* A certificate set may hold other CertificateChoices; they are stepped over + * and only X.509 certificates are stored and counted. */ +int test_wc_PKCS7_VerifySignedData_CertSetAttrCert(void) +{ + EXPECT_DECLS; +#if defined(HAVE_PKCS7) && !defined(NO_RSA) && !defined(NO_SHA256) && \ + defined(USE_CERT_BUFFERS_2048) + static const byte attrCert[] = { 0xA2, 0x03, 0x30, 0x01, 0x00 }; + static const byte badAttrCert[] = { 0xA2, 0x7F }; + static const byte longAttrCert[] = { 0xA2, 0x03, 0x30, 0x01 }; + static const byte strayTag[] = { 0xA2 }; + static const byte indefCert[] = { 0x30, 0x80, 0x30, 0x00, 0x00, 0x00 }; + const byte* bad[] = { badAttrCert, longAttrCert, strayTag, indefCert }; + const word32 badSz[] = { sizeof(badAttrCert), sizeof(longAttrCert), + sizeof(strayTag), sizeof(indefCert) }; + PKCS7* pkcs7 = NULL; + byte* bundle = NULL; + word32 bundleMax = (MAX_PKCS7_CERTS + 2) * sizeof_client_cert_der_2048 + + FOURK_BUF; + int bundleSz = 0; + int pos; + int p; + + ExpectNotNull(bundle = (byte*)XMALLOC(bundleMax, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + + /* Attribute certificate first, second and last. */ + for (p = 0; p < 3; p++) { + pos = (p < 2) ? p : MAX_PKCS7_CERTS; + ExpectIntGT((bundleSz = pkcs7_encode_raw_cert_set(bundle, bundleMax, + MAX_PKCS7_CERTS, 0, attrCert, sizeof(attrCert), pos)), 0); + ExpectNotNull(pkcs7 = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, bundle, + (word32)bundleSz), 0); + if (pkcs7 != NULL) { + ExpectIntEQ(pkcs7->certSz[0], sizeof_client_cert_der_2048); + ExpectIntEQ(pkcs7->certSz[MAX_PKCS7_CERTS - 1], + sizeof_client_cert_der_2048); + } + #ifndef NO_PKCS7_STREAM + ExpectIntEQ(pkcs7_verify_bytewise(pkcs7, bundle, bundleSz), 0); + #endif + wc_PKCS7_Free(pkcs7); + pkcs7 = NULL; + + pos = (p < 2) ? p : MAX_PKCS7_CERTS + 1; + ExpectIntGT((bundleSz = pkcs7_encode_raw_cert_set(bundle, bundleMax, + MAX_PKCS7_CERTS + 1, 0, attrCert, sizeof(attrCert), pos)), 0); + ExpectNotNull(pkcs7 = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, bundle, + (word32)bundleSz), WC_NO_ERR_TRACE(BUFFER_E)); + #ifndef NO_PKCS7_STREAM + ExpectIntEQ(pkcs7_verify_bytewise(pkcs7, bundle, bundleSz), + WC_NO_ERR_TRACE(BUFFER_E)); + #endif + wc_PKCS7_Free(pkcs7); + pkcs7 = NULL; + } + + /* Truncated, overlong and indefinite-length elements are parse errors. */ + for (p = 0; p < (int)(sizeof(badSz) / sizeof(badSz[0])); p++) { + ExpectIntGT((bundleSz = pkcs7_encode_raw_cert_set(bundle, bundleMax, + 1, 0, bad[p], badSz[p], 1)), 0); + ExpectNotNull(pkcs7 = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_VerifySignedData(pkcs7, bundle, + (word32)bundleSz), WC_NO_ERR_TRACE(ASN_PARSE_E)); + #ifndef NO_PKCS7_STREAM + ExpectIntEQ(pkcs7_verify_bytewise(pkcs7, bundle, bundleSz), + WC_NO_ERR_TRACE(ASN_PARSE_E)); + #endif + wc_PKCS7_Free(pkcs7); + pkcs7 = NULL; + } + + XFREE(bundle, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); +#endif + return EXPECT_RESULT(); +} diff --git a/tests/api/test_pkcs7.h b/tests/api/test_pkcs7.h index 5ac515c7693..c0b80441f59 100644 --- a/tests/api/test_pkcs7.h +++ b/tests/api/test_pkcs7.h @@ -104,6 +104,7 @@ int test_wc_PKCS7_VerifySignedData_NoDegenerateAcceptsRealSigner(void); int test_wc_PKCS7_VerifySignedData_NoDigestParams(void); int test_wc_PKCS7_VerifySignedData_ReuseAfterContent(void); int test_wc_PKCS7_VerifySignedData_CertSetOverflow(void); +int test_wc_PKCS7_VerifySignedData_CertSetAttrCert(void); #define TEST_PKCS7_DECLS \ @@ -171,7 +172,8 @@ int test_wc_PKCS7_VerifySignedData_CertSetOverflow(void); TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_NoDegenerateAcceptsRealSigner), \ TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_NoDigestParams), \ TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_ReuseAfterContent), \ - TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_CertSetOverflow) + TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_CertSetOverflow), \ + TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_CertSetAttrCert) #define TEST_PKCS7_ENCRYPTED_DATA_DECLS \ TEST_DECL_GROUP("pkcs7_ed", test_wc_PKCS7_DecodeEnvelopedData_stream), \ diff --git a/wolfcrypt/src/pkcs7.c b/wolfcrypt/src/pkcs7.c index 6d1a096cec9..9d9e265617d 100644 --- a/wolfcrypt/src/pkcs7.c +++ b/wolfcrypt/src/pkcs7.c @@ -8139,16 +8139,8 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, if (ret == 0 && MAX_PKCS7_CERTS > 0) { int sz = 0; int i; - /* Absolute end of the certificate set within pkiMsg2. - * idx is the start of the set, so the set spans - * [idx, idx + length). In non-streaming mode idx is the - * absolute offset into the message; in streaming mode it - * is typically 0 (the set was copied to a standalone - * buffer). Bounding the loop with the relative length - * alone stops short by idx bytes in non-streaming mode - * and can drop the last certificate. Clamp to pkiMsg2Sz - * to guard against overflow/over-long length (reads stay - * bounded by the certIdx + 1 < pkiMsg2Sz check below). */ + /* End of the set in pkiMsg2, clamped to pkiMsg2Sz + * against an overlong or wrapping length. */ word32 certSetEnd = idx + (word32)length; if (certSetEnd < idx || certSetEnd > pkiMsg2Sz) certSetEnd = pkiMsg2Sz; @@ -8156,40 +8148,33 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, pkcs7->cert[0] = cert; pkcs7->certSz[0] = (word32)certSz; certIdx = idx + (word32)certSz; + i = (cert != NULL) ? 1 : 0; - for (i = 1; i < MAX_PKCS7_CERTS && - certIdx + 1 < pkiMsg2Sz && - certIdx + 1 < certSetEnd; i++) { + while (certIdx < certSetEnd) { localIdx = certIdx; - if (ret == 0 && GetASNTag(pkiMsg2, &certIdx, &tag, - pkiMsg2Sz) < 0) { + if (GetASNTag(pkiMsg2, &certIdx, &tag, + certSetEnd) < 0 || + GetLength(pkiMsg2, &certIdx, &sz, + certSetEnd) < 0 || + (sz == 0 && pkiMsg2[certIdx - 1] == + ASN_INDEF_LENGTH)) { ret = ASN_PARSE_E; break; } - if (ret == 0 && - tag == (ASN_CONSTRUCTED | ASN_SEQUENCE)) { - if (GetLength(pkiMsg2, &certIdx, &sz, - pkiMsg2Sz) < 0) { - ret = ASN_PARSE_E; + /* other CertificateChoices are skipped */ + if (tag == (ASN_CONSTRUCTED | ASN_SEQUENCE)) { + if (i == MAX_PKCS7_CERTS) { + certSetOverflow = 1; break; } - pkcs7->cert[i] = &pkiMsg2[localIdx]; pkcs7->certSz[i] = (word32)sz + (certIdx - localIdx); - certIdx += (word32)sz; + i++; } - } - - /* cert[] is full and another certificate follows */ - if (ret == 0 && i == MAX_PKCS7_CERTS && - certIdx + 1 < pkiMsg2Sz && - certIdx + 1 < certSetEnd && - pkiMsg2[certIdx] == - (ASN_CONSTRUCTED | ASN_SEQUENCE)) { - certSetOverflow = 1; + certIdx += (word32)sz; } #ifndef NO_PKCS7_STREAM pkcs7->stream->certSetOverflow = (certSetOverflow != 0); From 838b5c10cd0d210743546be8192e42c1ea171db4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Tue, 6 Oct 2026 09:35:44 +0200 Subject: [PATCH 4/7] Raise the default MAX_PKCS7_CERTS from 4 to 8 wc_PKCS7_VerifySignedData() now refuses a certificates-only bundle that holds more certificates than cert[] can store, where it used to drop the extra ones silently. With the old default of 4, a CA bundle carrying a root, a couple of intermediates and an RA certificate already hits that limit. OPENSSL_ALL builds keep 15. The limit only sizes the cert[] and certSz[] arrays in wc_PKCS7, so the structure grows by 12 bytes per slot on 64-bit targets and 8 on 32-bit ones. Both arrays sit in the middle of the structure, so applications must be rebuilt against the new header, as for any change to it. --- wolfssl/wolfcrypt/pkcs7.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wolfssl/wolfcrypt/pkcs7.h b/wolfssl/wolfcrypt/pkcs7.h index 1d0a203b8f4..d68dc205be9 100644 --- a/wolfssl/wolfcrypt/pkcs7.h +++ b/wolfssl/wolfcrypt/pkcs7.h @@ -52,7 +52,7 @@ #ifdef OPENSSL_ALL #define MAX_PKCS7_CERTS 15 #else - #define MAX_PKCS7_CERTS 4 + #define MAX_PKCS7_CERTS 8 #endif #endif From bf6dd0d521d37b20f5221699a88b6a58727dabae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Thu, 5 Nov 2026 09:47:25 +0100 Subject: [PATCH 5/7] Address review feedback PKCS7_VerifySignedData() tested for a header/footer verify in three ways: in2 alone, in2 with a hash, and in2Sz with hashSz. Name the two meanings once, hasFooter for where the footer is read from and hashedFooter for a verify whose content is left out of the message, and use them throughout. The content drop at verify start and the message size bookkeeping in the START case and stage 2 now use hashedFooter, so contentSz is only kept and added when the content really is missing from the message. A footer passed without a hash no longer keeps a contentSz left by an earlier verify. Builds with NO_PKCS7_STREAM dropped the content of the previous verify and captured the caller's content before the state switch. A call that found the structure in another operation's state then lost that content and returned BAD_FUNC_ARG without restoring it, while streaming builds kept it. Do both in the START case, in both builds. Document that a footer is only supported together with a hash, and that a header/footer verify needs contentSz set before each call, since a value left by an earlier verify on the same structure cannot be told apart from one the caller set. The reuse test now also checks that a footer without a hash gives the same result after a full verify as on a fresh object, and that a verify called in another operation's state keeps the content the last verify handed back. --- doc/dox_comments/header_files/pkcs7.h | 5 ++- tests/api/test_pkcs7.c | 54 +++++++++++++++++++++++++++ wolfcrypt/src/pkcs7.c | 48 ++++++++++++------------ 3 files changed, 82 insertions(+), 25 deletions(-) diff --git a/doc/dox_comments/header_files/pkcs7.h b/doc/dox_comments/header_files/pkcs7.h index f035a7acbc3..77c40f2e458 100644 --- a/doc/dox_comments/header_files/pkcs7.h +++ b/doc/dox_comments/header_files/pkcs7.h @@ -416,6 +416,9 @@ int wc_PKCS7_VerifySignedData(wc_PKCS7* pkcs7, the next verify or wc_PKCS7_Free() on the same structure. A verify that fails with an error other than PKCS7_SIGNEEDS_CHECK leaves pkcs7->content as the caller set it. + Set pkcs7->contentSz to the content length before each header/footer + verify, since a value left by an earlier verify on the same structure is + kept. \return 0 Returned on successfully extracting the information from the message @@ -460,7 +463,7 @@ int wc_PKCS7_VerifySignedData(wc_PKCS7* pkcs7, to verify and decode \param pkiMsgHeadSz size of the signed message header \param pkiMsgFoot pointer to the buffer containing the signed message footer - to verify and decode + to verify and decode, only supported together with hashBuf \param pkiMsgFootSz size of the signed message footer _Example_ diff --git a/tests/api/test_pkcs7.c b/tests/api/test_pkcs7.c index 7ece4136981..d4c64480ed7 100644 --- a/tests/api/test_pkcs7.c +++ b/tests/api/test_pkcs7.c @@ -9332,6 +9332,7 @@ int test_wc_PKCS7_VerifySignedData_ReuseAfterContent(void) byte* msgCopy = NULL; byte* head = NULL; byte* foot = NULL; + byte* full = NULL; word32 headSz = 0; word32 footSz = 0; byte hash[WC_SHA256_DIGEST_SIZE]; @@ -9538,6 +9539,58 @@ int test_wc_PKCS7_VerifySignedData_ReuseAfterContent(void) wc_PKCS7_Free(other); other = NULL; + /* A footer without a hash gives the same result after a full verify as on + * a fresh object. */ + ExpectNotNull(full = (byte*)XMALLOC(headSz + sizeof(bigData) + footSz, + HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER)); + if (full != NULL) { + XMEMCPY(full, head, headSz); + XMEMCPY(full + headSz, bigData, sizeof(bigData)); + XMEMCPY(full + headSz + sizeof(bigData), foot, footSz); + } + for (i = 0; i < 2; i++) { + byte* in2 = (i == 0) ? foot : bigData; + word32 in2Sz = (i == 0) ? footSz : (word32)sizeof(bigData); + int freshRet = 0; + + ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); + if (other != NULL) { + freshRet = wc_PKCS7_VerifySignedData_ex(other, NULL, 0, full, + headSz + (word32)sizeof(bigData) + footSz, in2, in2Sz); + } + wc_PKCS7_Free(other); + other = NULL; + ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_VerifySignedData(other, signedMsg, + (word32)signedSz), 0); + ExpectIntEQ(wc_PKCS7_VerifySignedData_ex(other, NULL, 0, full, + headSz + (word32)sizeof(bigData) + footSz, in2, in2Sz), freshRet); + wc_PKCS7_Free(other); + other = NULL; + } + + /* A verify called in another operation's state leaves the content the + * last verify handed back. */ + ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_VerifySignedData(other, signedMsg, (word32)signedSz), + 0); + if (other != NULL) { + other->state = WC_PKCS7_AUTHENV_7; + } + ExpectIntEQ(wc_PKCS7_VerifySignedData(other, signedMsg, (word32)signedSz), + WC_NO_ERR_TRACE(BAD_FUNC_ARG)); + if (other != NULL) { + ExpectIntEQ(other->contentSz, sizeof(data)); + ExpectNotNull(other->content); + if (other->content != NULL) { + ExpectBufEQ(other->content, data, sizeof(data)); + } + } + ExpectIntEQ(wc_PKCS7_VerifySignedData(other, signedMsg, (word32)signedSz), + 0); + wc_PKCS7_Free(other); + other = NULL; + /* Larger detached content than the previous verify left behind. */ ExpectNotNull(other = wc_PKCS7_New(HEAP_HINT, testDevId)); ExpectIntEQ(wc_PKCS7_InitWithCert(other, (byte*)client_cert_der_2048, @@ -9568,6 +9621,7 @@ int test_wc_PKCS7_VerifySignedData_ReuseAfterContent(void) (word32)detachedSz), 0); wc_PKCS7_Free(other); + XFREE(full, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); XFREE(foot, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); XFREE(head, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); XFREE(badMsg, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); diff --git a/wolfcrypt/src/pkcs7.c b/wolfcrypt/src/pkcs7.c index 9d9e265617d..a0e654c89f0 100644 --- a/wolfcrypt/src/pkcs7.c +++ b/wolfcrypt/src/pkcs7.c @@ -7089,6 +7089,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, byte* der; #endif int multiPart = 0, keepContent; + int hasFooter, hashedFooter; int contentLen = 0; byte* pkiMsg = in; @@ -7100,7 +7101,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, byte* src = NULL; word32 srcSz; #else - word32 callerContentSz; + word32 callerContentSz = 0; #endif byte* callerContent = NULL; byte restoreContent = 1; @@ -7123,12 +7124,9 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, if ((hashSz > 0 && hashBuf == NULL) || (pkiMsg2Sz > 0 && pkiMsg2 == NULL)) { return BAD_FUNC_ARG; } + hasFooter = (in2 != NULL && in2Sz > 0); + hashedFooter = (hasFooter && hashBuf != NULL && hashSz > 0); idx = 0; -#ifdef NO_PKCS7_STREAM - wc_PKCS7_DropVerifyContent(pkcs7, in2 != NULL && in2Sz > 0); - callerContent = pkcs7->content; - callerContentSz = pkcs7->contentSz; -#endif #ifdef ASN_BER_TO_DER if (pkcs7->derSz > 0 && pkcs7->der) { @@ -7146,8 +7144,11 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, switch (pkcs7->state) { case WC_PKCS7_START: - #ifndef NO_PKCS7_STREAM - wc_PKCS7_DropVerifyContent(pkcs7, in2 != NULL && in2Sz > 0); + wc_PKCS7_DropVerifyContent(pkcs7, hashedFooter); + #ifdef NO_PKCS7_STREAM + callerContent = pkcs7->content; + callerContentSz = pkcs7->contentSz; + #else pkcs7->stream->callerContent = pkcs7->content; pkcs7->stream->callerContentSz = pkcs7->contentSz; @@ -7177,7 +7178,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, /* determine total message size */ totalSz = pkiMsgSz; - if (pkiMsg2 && pkiMsg2Sz > 0) { + if (hashedFooter) { totalSz += pkiMsg2Sz + pkcs7->contentSz; } @@ -7221,7 +7222,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, inSz; totalSz = pkiMsgSz; - if (pkiMsg2 && pkiMsg2Sz > 0) { + if (hashedFooter) { totalSz += pkiMsg2Sz + pkcs7->contentSz; } @@ -7336,7 +7337,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, if ((ret = wc_PKCS7_StreamEndCase(pkcs7, &stateIdx, &idx)) != 0) { break; } - if (pkiMsg2 && pkiMsg2Sz > 0) { + if (hashedFooter) { pkcs7->stream->maxLen += pkiMsg2Sz + pkcs7->contentSz; } wc_PKCS7_StreamStoreVar(pkcs7, totalSz, 0, 0); @@ -7586,7 +7587,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, /* update idx if successful */ if (ret == 0) { /* support using header and footer without content */ - if (pkiMsg2 && pkiMsg2Sz > 0 && hashBuf && hashSz > 0) { + if (hashedFooter) { localIdx = 0; #ifndef NO_PKCS7_STREAM pkcs7->stream->noContent = 1; @@ -7658,7 +7659,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, if (multiPart) { idx = pkcs7->stream->currContIdx; } - if (in2Sz > 0 && hashSz > 0) { + if (hashedFooter) { /* seems no content included */ pkcs7->stream->expected = + ASN_TAG_SZ + MAX_LENGTH_SZ; } @@ -7696,7 +7697,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, FALL_THROUGH; case WC_PKCS7_VERIFY_STAGE3: - keepContent = !(pkiMsg2 && pkiMsg2Sz > 0 && hashBuf && hashSz > 0); + keepContent = !hashedFooter; #ifndef NO_PKCS7_STREAM ret = wc_PKCS7_HandleOctetStrings(pkcs7, in, inSz, &stateIdx, &idx, keepContent); @@ -7761,8 +7762,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, /* get parts of content */ if (ret == 0 && multiPart) { int i = 0; - keepContent = !(pkiMsg2 && pkiMsg2Sz > 0 && hashBuf && - hashSz > 0); + keepContent = !hashedFooter; if (keepContent) { /* Create a buffer to hold content of OCTET_STRINGs. */ @@ -7804,7 +7804,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, contentSz = length; /* support using header and footer without content */ - if (pkiMsg2 && pkiMsg2Sz > 0 && hashBuf && hashSz > 0) { + if (hashedFooter) { /* Content not provided, use provided pkiMsg2 footer */ content = NULL; localIdx = 0; @@ -7863,7 +7863,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, case WC_PKCS7_VERIFY_STAGE4: #ifndef NO_PKCS7_STREAM - if (in2 && in2Sz > 0) { + if (hasFooter) { src = in2; srcSz = in2Sz; pkiMsg2 = in2; @@ -7962,7 +7962,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, break; } #ifndef NO_PKCS7_STREAM - if (in2 && in2Sz > 0 && hashBuf && hashSz > 0) { + if (hashedFooter) { stateIdx = idx; /* case where all data was read from in2 */ } @@ -7987,7 +7987,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, case WC_PKCS7_VERIFY_STAGE5: #ifndef NO_PKCS7_STREAM - if (in2 && in2Sz > 0) { + if (hasFooter) { src = in2; srcSz = in2Sz; pkiMsg2 = in2; @@ -8022,7 +8022,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, /* store current index to get the signerInfo index later */ certIdx2 = idx; /* store certificate if needed */ - if (length > 0 && in2Sz == 0) { + if (length > 0 && !hasFooter) { /* free tmpCert if not NULL */ XFREE(pkcs7->stream->tmpCert, pkcs7->heap, DYNAMIC_TYPE_PKCS7); pkcs7->stream->tmpCert = (byte*)XMALLOC((word32)length, @@ -8232,7 +8232,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, case WC_PKCS7_VERIFY_STAGE6: #ifndef NO_PKCS7_STREAM - if (in2 && in2Sz > 0) { + if (hasFooter) { src = in2; srcSz = in2Sz; pkiMsg2 = in2; @@ -8373,7 +8373,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, } wc_PKCS7_StreamStoreVar(pkcs7, pkiMsg2Sz, 0, length); - if (in2 && in2Sz > 0 && hashBuf && hashSz > 0) { + if (hashedFooter) { if (length > 0) { pkcs7->stream->expected = (word32)length; } @@ -8402,7 +8402,7 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, case WC_PKCS7_VERIFY_STAGE7: #ifndef NO_PKCS7_STREAM - if (in2 && in2Sz > 0) { + if (hasFooter) { src = in2; srcSz = in2Sz; pkiMsg2 = in2; From b96cd6ddd0b76dda86d8b2ab1297c35447f12aa4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Thu, 5 Nov 2026 09:50:06 +0100 Subject: [PATCH 6/7] Reset the PKCS#7 state when an AuthEnvelopedData decode fails wc_PKCS7_DecodeAuthEnvelopedData() reset the stream on failure but left pkcs7->state where the decode stopped, unlike the EnvelopedData and EncryptedData decoders. The next operation on the same structure then resumed in that state. A verify returned BAD_FUNC_ARG from its unknown state branch. Another AuthEnvelopedData decode resumed at the tag stage with no stream data: builds with NO_PKCS7_STREAM failed it, and streaming builds copied the nonce from the freed stream->nonce, which the reset sets to NULL while keeping nonceSz, and crashed on the NULL pointer. Return to WC_PKCS7_START on every failure other than WC_PKCS7_WANT_READ_E, in both builds. The test decodes a message with a corrupted authentication tag and then the intact message with the same structure. --- tests/api/test_pkcs7.c | 50 ++++++++++++++++++++++++++++++++++++++++++ tests/api/test_pkcs7.h | 2 ++ wolfcrypt/src/pkcs7.c | 5 +++-- 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/tests/api/test_pkcs7.c b/tests/api/test_pkcs7.c index d4c64480ed7..e9304ba8eb3 100644 --- a/tests/api/test_pkcs7.c +++ b/tests/api/test_pkcs7.c @@ -4543,6 +4543,56 @@ int test_wc_PKCS7_DecodeAuthEnvelopedData_truncated(void) } /* END test_wc_PKCS7_DecodeAuthEnvelopedData_truncated() */ +/* A failed AuthEnvelopedData decode leaves the object ready for the next + * operation. */ +int test_wc_PKCS7_DecodeAuthEnvelopedData_ReuseAfterError(void) +{ + EXPECT_DECLS; +#if defined(HAVE_PKCS7) && defined(HAVE_AESGCM) && !defined(NO_RSA) && \ + !defined(NO_AES) && defined(WOLFSSL_AES_128) + PKCS7* pkcs7 = NULL; + byte enveloped[2048]; + byte badTag[2048]; + byte decoded[256]; + byte data[] = "authEnvelopedData reuse test"; + int encSz = 0; + + ExpectNotNull(pkcs7 = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_InitWithCert(pkcs7, (byte*)client_cert_der_2048, + sizeof_client_cert_der_2048), 0); + if (pkcs7 != NULL) { + pkcs7->content = data; + pkcs7->contentSz = (word32)sizeof(data); + pkcs7->contentOID = DATA; + pkcs7->encryptOID = AES128GCMb; + } + ExpectIntGT(encSz = wc_PKCS7_EncodeAuthEnvelopedData(pkcs7, enveloped, + sizeof(enveloped)), 0); + wc_PKCS7_Free(pkcs7); + pkcs7 = NULL; + if (encSz > 0) { + XMEMCPY(badTag, enveloped, (size_t)encSz); + badTag[encSz - 1] ^= 0x01; + } + + ExpectNotNull(pkcs7 = wc_PKCS7_New(HEAP_HINT, testDevId)); + ExpectIntEQ(wc_PKCS7_InitWithCert(pkcs7, (byte*)client_cert_der_2048, + sizeof_client_cert_der_2048), 0); + if (pkcs7 != NULL) { + pkcs7->privateKey = (byte*)client_key_der_2048; + pkcs7->privateKeySz = sizeof_client_key_der_2048; + } + ExpectIntLT(wc_PKCS7_DecodeAuthEnvelopedData(pkcs7, badTag, + (word32)encSz, decoded, sizeof(decoded)), 0); + ExpectIntEQ(wc_PKCS7_DecodeAuthEnvelopedData(pkcs7, enveloped, + (word32)encSz, decoded, sizeof(decoded)), (int)sizeof(data)); + ExpectIntEQ(XMEMCMP(decoded, data, sizeof(data)), 0); + wc_PKCS7_Free(pkcs7); +#endif + return EXPECT_RESULT(); +} /* END test_wc_PKCS7_DecodeAuthEnvelopedData_ReuseAfterError() */ + + #if defined(HAVE_PKCS7) && !defined(NO_RSA) && !defined(NO_AES) && \ defined(WOLFSSL_AES_128) && (defined(HAVE_AESGCM) || defined(HAVE_AESCCM)) /* Encode an AuthEnvelopedData bundle, then cut its tag down to tagSz bytes diff --git a/tests/api/test_pkcs7.h b/tests/api/test_pkcs7.h index c0b80441f59..b065a16023a 100644 --- a/tests/api/test_pkcs7.h +++ b/tests/api/test_pkcs7.h @@ -86,6 +86,7 @@ int test_wc_PKCS7_DecodeAuthEnvelopedData_fragmented(void); int test_wc_PKCS7_DecodeOpenSslStream(void); int test_wc_PKCS7_DecodeEnvelopedData_split(void); int test_wc_PKCS7_DecodeAuthEnvelopedData_truncated(void); +int test_wc_PKCS7_DecodeAuthEnvelopedData_ReuseAfterError(void); int test_wc_PKCS7_DecodeAuthEnvelopedData_shortTag(void); int test_wc_PKCS7_DecodeAuthEnvelopedData_shortTagCcm(void); int test_wc_PKCS7_DecodeAuthEnvelopedData_shortTagChunked(void); @@ -203,6 +204,7 @@ int test_wc_PKCS7_VerifySignedData_CertSetAttrCert(void); TEST_DECL_GROUP("pkcs7_ed", test_wc_PKCS7_DecodeOpenSslStream), \ TEST_DECL_GROUP("pkcs7_ed", test_wc_PKCS7_DecodeEnvelopedData_split), \ TEST_DECL_GROUP("pkcs7_ed", test_wc_PKCS7_DecodeAuthEnvelopedData_truncated), \ + TEST_DECL_GROUP("pkcs7_ed", test_wc_PKCS7_DecodeAuthEnvelopedData_ReuseAfterError), \ TEST_DECL_GROUP("pkcs7_ed", test_wc_PKCS7_DecodeAuthEnvelopedData_shortTag), \ TEST_DECL_GROUP("pkcs7_ed", test_wc_PKCS7_DecodeAuthEnvelopedData_shortTagCcm), \ TEST_DECL_GROUP("pkcs7_ed", test_wc_PKCS7_DecodeAuthEnvelopedData_shortTagChunked), \ diff --git a/wolfcrypt/src/pkcs7.c b/wolfcrypt/src/pkcs7.c index a0e654c89f0..419894dc61a 100644 --- a/wolfcrypt/src/pkcs7.c +++ b/wolfcrypt/src/pkcs7.c @@ -17246,11 +17246,12 @@ int wc_PKCS7_DecodeAuthEnvelopedData(wc_PKCS7* pkcs7, byte* in, } } -#ifndef NO_PKCS7_STREAM if (ret != 0 && ret != WC_NO_ERR_TRACE(WC_PKCS7_WANT_READ_E)) { + #ifndef NO_PKCS7_STREAM wc_PKCS7_ResetStream(pkcs7); + #endif + wc_PKCS7_ChangeState(pkcs7, WC_PKCS7_START); } -#endif return ret; From 223e7e0f4f4d6b3b9c690a019f6a5a3cb35054e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Thu, 5 Nov 2026 10:00:48 +0100 Subject: [PATCH 7/7] Clear the AEAD sizes when resetting the PKCS#7 stream wc_PKCS7_ResetStream() freed stream->aad, stream->tag and stream->nonce but kept aadSz, tagSz and nonceSz. The AuthEnvelopedData tag stage copies the saved nonce and tag whenever their sizes are non-zero, so a decode that resumed there after a reset copied from a NULL pointer, and took a NULL aad with a non-zero size. Clear the three sizes together with their buffers. The previous commit stops a failed decode from leaving the structure in that stage. The test now also forces the tag stage after a failed decode and expects an error instead of a crash. --- tests/api/test_pkcs7.c | 7 +++++++ wolfcrypt/src/pkcs7.c | 3 +++ 2 files changed, 10 insertions(+) diff --git a/tests/api/test_pkcs7.c b/tests/api/test_pkcs7.c index e9304ba8eb3..5c4cba98b5c 100644 --- a/tests/api/test_pkcs7.c +++ b/tests/api/test_pkcs7.c @@ -4584,6 +4584,13 @@ int test_wc_PKCS7_DecodeAuthEnvelopedData_ReuseAfterError(void) } ExpectIntLT(wc_PKCS7_DecodeAuthEnvelopedData(pkcs7, badTag, (word32)encSz, decoded, sizeof(decoded)), 0); + /* Resuming at the tag stage after the stream was reset finds no saved + * nonce, tag or attributes. */ + if (pkcs7 != NULL) { + pkcs7->state = WC_PKCS7_AUTHENV_6; + } + ExpectIntLT(wc_PKCS7_DecodeAuthEnvelopedData(pkcs7, enveloped, + (word32)encSz, decoded, sizeof(decoded)), 0); ExpectIntEQ(wc_PKCS7_DecodeAuthEnvelopedData(pkcs7, enveloped, (word32)encSz, decoded, sizeof(decoded)), (int)sizeof(data)); ExpectIntEQ(XMEMCMP(decoded, data, sizeof(data)), 0); diff --git a/wolfcrypt/src/pkcs7.c b/wolfcrypt/src/pkcs7.c index 419894dc61a..56369c662be 100644 --- a/wolfcrypt/src/pkcs7.c +++ b/wolfcrypt/src/pkcs7.c @@ -276,6 +276,9 @@ static void wc_PKCS7_ResetStream(wc_PKCS7* pkcs7) pkcs7->stream->buffer = NULL; pkcs7->stream->bufferPt = NULL; pkcs7->stream->key = NULL; + pkcs7->stream->aadSz = 0; + pkcs7->stream->tagSz = 0; + pkcs7->stream->nonceSz = 0; /* reset values, note that content and tmpCert are saved */ pkcs7->stream->maxLen = 0;