pkcs7: address second-round review (no-malloc, test guards, hardening)

claude · claude · commit 47d5b2cdec5a · 2026-06-20T09:33:38.000Z
- Replace the mandatory heap allocation of the signer attribute array with a hybrid: an inline array (sized MAX_SIGNED_ATTRIBS_SZ, the historical footprint) is used for the common case with no heap use, and a heap buffer is allocated only when the attribute count exceeds it. Restores WOLFSSL_NO_MALLOC support (no allocation in the common path; >inline returns BUFFER_E without a heap, matching the historical cap) and keeps the per-ESD footprint unchanged. - wc_PKCS7_GetSignedAttribValue(): reject a value that still carries an outer SET wrapper (defends against an unexpected decoded shape) and guard the returned length against INT overflow. - Multi-certificate decode bound: clamp certSetEnd to pkiMsg2Sz to guard against overflow/over-long length, and correct the comment about the streaming idx. - Tests: guard the 3-certificate certs-only and multi-cert tests with MAX_PKCS7_CERTS >= 3, and the 9-attribute test with (!WOLFSSL_NO_MALLOC || MAX_SIGNED_ATTRIBS_SZ >= 9), so constrained builds do not fail spuriously. - ChangeLog: document the non-streaming multi-certificate decode fix. Verified across default (streaming), NO_PKCS7_STREAM, and WOLFSSL_SMALL_STACK builds; pkcs7.c also compiles cleanly with -DWOLFSSL_NO_MALLOC. wolfcrypt and unit tests pass and valgrind is clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EprPgb9PzJkPp8LXuFk1yR
diff --git a/ChangeLog.md b/ChangeLog.md
@@ -19,6 +19,14 @@
   decoded-attribute value shape is documentation of existing behavior and does
   not change it.
 
+* **PKCS#7 multi-certificate decode fix (non-streaming)**: Fixed the additional
+  -certificate loop in `wc_PKCS7_VerifySignedData` to bound against the absolute
+  end of the certificate set rather than its relative length. In `NO_PKCS7_STREAM`
+  builds this previously stopped short and dropped trailing certificates (and,
+  with a large eContent preceding the certificate set, could drop all but the
+  first certificate, causing signature verification to fail when the signer
+  certificate was among those dropped). Streaming builds were unaffected.
+
 * **Behavioral change (RSA-PSS trailerField enforcement)**: `DecodeRsaPssParams`
   (and its public wrapper `wc_DecodeRsaPssParams`) now enforces RFC 8017 A.2.3,
   which mandates `trailerField == trailerFieldBC(1)`.  In the default build
diff --git a/wolfcrypt/src/pkcs7.c b/wolfcrypt/src/pkcs7.c
@@ -1571,8 +1571,15 @@ int wc_PKCS7_GetSignedAttribValue(wc_PKCS7* pkcs7, const byte* oid,
         return ASN_PARSE_E;
     }
 
-    /* attrib->value holds the SET contents; parse the first AttributeValue
-     * TLV (tag + length + content) and return a pointer spanning it */
+    /* attrib->value must be the SET contents with the outer SET tag stripped
+     * (the documented shape). Reject a value that still carries a SET wrapper,
+     * which would indicate the value is not in the expected form. */
+    if (attrib->value[0] == (ASN_SET | ASN_CONSTRUCTED)) {
+        return ASN_PARSE_E;
+    }
+
+    /* parse the first AttributeValue TLV (tag + length + content) and return a
+     * pointer spanning it */
     idx = 1; /* skip the value tag */
     if (GetLength(attrib->value, &idx, &length, attrib->valueSz) < 0) {
         return ASN_PARSE_E;
@@ -1584,6 +1591,9 @@ int wc_PKCS7_GetSignedAttribValue(wc_PKCS7* pkcs7, const byte* oid,
 
     *out   = attrib->value;
     *outSz = idx + (word32)length;
+    if (*outSz > (word32)WC_MAX_SINT_OF(int)) {
+        return BUFFER_E;
+    }
     return (int)*outSz;
 }
 
@@ -1671,11 +1681,13 @@ typedef struct ESD {
                         byte digEncAlgoId[MAX_ALGO_SZ];
 #endif
                         byte signedAttribSet[MAX_SET_SZ];
-                            /* signedAttribs is allocated at encode time, sized
-                             * to the actual attribute count, so it is not
-                             * bounded by MAX_SIGNED_ATTRIBS_SZ and adds no
-                             * fixed per-ESD footprint. signedAttribsCap holds
-                             * its allocated entry count. */
+                            /* Working attribute array. signedAttribs points at
+                             * the inline buffer for the common case (no heap
+                             * use, important for no-malloc/static-memory builds)
+                             * and is redirected to a heap allocation only when
+                             * the attribute count exceeds MAX_SIGNED_ATTRIBS_SZ.
+                             * signedAttribsCap holds the usable entry count. */
+                            EncodedAttrib signedAttribsInline[MAX_SIGNED_ATTRIBS_SZ];
                             EncodedAttrib* signedAttribs;
                         byte signerDigest[MAX_OCTET_STR_SZ];
     word32 signedAttribsCap;
@@ -3326,30 +3338,50 @@ static int PKCS7_EncodeSigned(wc_PKCS7* pkcs7,
         }
         signerInfoSz += esd->digEncAlgoIdSz;
 
-        /* Allocate the working attribute array, sized to the actual number of
-         * attributes: user-supplied plus up to the CMS auto-defaults
+        /* Point the working attribute array at the inline buffer, sized for the
+         * actual attribute count: user-supplied plus the CMS auto-defaults
          * (contentType, messageDigest, and, unless NO_ASN_TIME, signingTime).
-         * This avoids a fixed per-ESD array and any MAX_SIGNED_ATTRIBS_SZ cap. */
+         * Only fall back to a heap allocation when more attributes are needed
+         * than fit inline, so the common case stays allocation-free. */
         {
         #ifdef NO_ASN_TIME
             word32 defaultAttribCap = 2;
         #else
             word32 defaultAttribCap = 3;
         #endif
-            esd->signedAttribsCap = defaultAttribCap + pkcs7->signedAttribsSz;
-            /* detect addition and multiplication overflow */
-            if (esd->signedAttribsCap < pkcs7->signedAttribsSz ||
-                esd->signedAttribsCap >
-                    ((word32)WC_MAX_SINT_OF(int) / (word32)sizeof(EncodedAttrib))) {
+            word32 neededCap = defaultAttribCap + pkcs7->signedAttribsSz;
+
+            /* detect addition overflow */
+            if (neededCap < pkcs7->signedAttribsSz) {
                 idx = BUFFER_E;
                 goto out;
             }
-            esd->signedAttribs = (EncodedAttrib*)XMALLOC(
-                esd->signedAttribsCap * (word32)sizeof(EncodedAttrib),
-                pkcs7->heap, DYNAMIC_TYPE_TMP_BUFFER);
-            if (esd->signedAttribs == NULL) {
-                idx = MEMORY_E;
+
+            if (neededCap <= MAX_SIGNED_ATTRIBS_SZ) {
+                esd->signedAttribs    = esd->signedAttribsInline;
+                esd->signedAttribsCap = MAX_SIGNED_ATTRIBS_SZ;
+            }
+            else {
+            #ifdef WOLFSSL_NO_MALLOC
+                /* cannot grow beyond the inline array without a heap */
+                idx = BUFFER_E;
                 goto out;
+            #else
+                /* detect multiplication overflow */
+                if (neededCap > ((word32)WC_MAX_SINT_OF(int) /
+                                 (word32)sizeof(EncodedAttrib))) {
+                    idx = BUFFER_E;
+                    goto out;
+                }
+                esd->signedAttribs = (EncodedAttrib*)XMALLOC(
+                    neededCap * (word32)sizeof(EncodedAttrib),
+                    pkcs7->heap, DYNAMIC_TYPE_PKCS7);
+                if (esd->signedAttribs == NULL) {
+                    idx = MEMORY_E;
+                    goto out;
+                }
+                esd->signedAttribsCap = neededCap;
+            #endif
             }
             XMEMSET(esd->signedAttribs, 0,
                 esd->signedAttribsCap * (word32)sizeof(EncodedAttrib));
@@ -3783,13 +3815,17 @@ static int PKCS7_EncodeSigned(wc_PKCS7* pkcs7,
 
     XFREE(flatSignedAttribs, pkcs7->heap, DYNAMIC_TYPE_PKCS7);
 
-    /* free working attribute array (allocated above) before freeing esd. In
-     * small-stack builds esd is heap-allocated and may be NULL here. */
+    /* free the working attribute array only if it was heap-allocated (i.e. it
+     * is not the inline buffer) before freeing esd. In small-stack builds esd
+     * is heap-allocated and may be NULL here. */
 #ifdef WOLFSSL_SMALL_STACK
     if (esd != NULL)
 #endif
     {
-        XFREE(esd->signedAttribs, pkcs7->heap, DYNAMIC_TYPE_TMP_BUFFER);
+        if (esd->signedAttribs != NULL &&
+            esd->signedAttribs != esd->signedAttribsInline) {
+            XFREE(esd->signedAttribs, pkcs7->heap, DYNAMIC_TYPE_PKCS7);
+        }
         esd->signedAttribs = NULL;
     }
 
@@ -7219,14 +7255,18 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf,
                         int sz = 0;
                         int i;
                         /* Absolute end of the certificate set within pkiMsg2.
-                         * idx is the start of the set: 0 in streaming mode
-                         * (the set was copied to a standalone buffer), or the
-                         * absolute offset into the message in non-streaming
-                         * mode. The set spans [idx, idx + length). Bounding the
-                         * loop with the relative length alone stops short by
-                         * idx bytes in non-streaming mode and can drop the last
-                         * certificate. */
+                         * 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). */
                         word32 certSetEnd = idx + (word32)length;
+                        if (certSetEnd < idx || certSetEnd > pkiMsg2Sz)
+                            certSetEnd = pkiMsg2Sz;
 
                         pkcs7->cert[0]   = cert;
                         pkcs7->certSz[0] = (word32)certSz;
diff --git a/wolfcrypt/test/test.c b/wolfcrypt/test/test.c
@@ -67507,6 +67507,7 @@ static wc_test_ret_t pkcs7_certsonly_test(byte* cert1, word32 cert1Sz,
     XFREE(out, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER);
     out = NULL;
 
+#if MAX_PKCS7_CERTS >= 3
     /* --- three certificates, in input order --- */
     pkcs7 = wc_PKCS7_New(HEAP_HINT, devId);
     if (pkcs7 == NULL)
@@ -67559,6 +67560,9 @@ static wc_test_ret_t pkcs7_certsonly_test(byte* cert1, word32 cert1Sz,
         if (!found1 || !found2 || !found3)
             ERROR_OUT(WC_TEST_RET_ENC_NC, out);
     }
+#else
+    (void)cert2; (void)cert2Sz; (void)cert3; (void)cert3Sz;
+#endif /* MAX_PKCS7_CERTS >= 3 */
 
     ret = 0;
 
@@ -67572,9 +67576,13 @@ static wc_test_ret_t pkcs7_certsonly_test(byte* cert1, word32 cert1Sz,
 #endif /* !NO_RSA */
 
 #if !defined(NO_RSA) && !defined(NO_SHA256)
+/* The nine-attribute case below needs more slots than the inline array holds
+ * (MAX_SIGNED_ATTRIBS_SZ), which requires a heap allocation; skip it on
+ * no-malloc builds unless the inline array was enlarged at build time. */
+#if !defined(WOLFSSL_NO_MALLOC) || (MAX_SIGNED_ATTRIBS_SZ >= 9)
 /* Test that a SignedData carrying six user signed attributes plus the three
- * CMS auto-defaults (nine total) encodes and verifies. This exceeds the old
- * MAX_SIGNED_ATTRIBS_SZ default of 7. */
+ * CMS auto-defaults (nine total) encodes and verifies. This exceeds the
+ * default inline capacity (MAX_SIGNED_ATTRIBS_SZ == 7). */
 static wc_test_ret_t pkcs7_signed_attribs_count_test(byte* cert, word32 certSz,
                                                      byte* key, word32 keySz)
 {
@@ -67676,6 +67684,7 @@ static wc_test_ret_t pkcs7_signed_attribs_count_test(byte* cert, word32 certSz,
 
     return ret;
 }
+#endif /* !WOLFSSL_NO_MALLOC || MAX_SIGNED_ATTRIBS_SZ >= 9 */
 
 /* Test that decoded signed-attribute values follow the documented, stable
  * shape (the contents of the SET OF AttributeValue, outer SET tag stripped)
@@ -67818,6 +67827,12 @@ static wc_test_ret_t pkcs7_signed_multi_cert_test(
         byte* cert1, word32 cert1Sz, byte* key, word32 keySz,
         byte* cert2, word32 cert2Sz, byte* cert3, word32 cert3Sz)
 {
+#if MAX_PKCS7_CERTS < 3
+    /* needs at least three certificate slots to exercise the bound */
+    (void)cert1; (void)cert1Sz; (void)key; (void)keySz;
+    (void)cert2; (void)cert2Sz; (void)cert3; (void)cert3Sz;
+    return 0;
+#else
     wc_test_ret_t ret = 0;
     wc_PKCS7* pkcs7 = NULL;
     WC_RNG rng;
@@ -67900,6 +67915,7 @@ static wc_test_ret_t pkcs7_signed_multi_cert_test(
     XFREE(content, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER);
 
     return ret;
+#endif /* MAX_PKCS7_CERTS < 3 */
 }
 #endif /* !NO_RSA && !NO_SHA256 */
 
@@ -68044,11 +68060,13 @@ WOLFSSL_TEST_SUBROUTINE wc_test_ret_t pkcs7signed_test(void)
 #endif
 
 #if !defined(NO_RSA) && !defined(NO_SHA256)
-    /* signed-attribute count beyond the old MAX_SIGNED_ATTRIBS_SZ default */
+#if !defined(WOLFSSL_NO_MALLOC) || (MAX_SIGNED_ATTRIBS_SZ >= 9)
+    /* signed-attribute count beyond the inline capacity */
     if (ret >= 0)
         ret = pkcs7_signed_attribs_count_test(
                             rsaClientCertBuf, (word32)rsaClientCertBufSz,
                             rsaClientPrivKeyBuf, (word32)rsaClientPrivKeyBufSz);
+#endif
 
     /* decoded signed-attribute value shape */
     if (ret >= 0)
