Code review feedback

Author: kareem-wolfssl

tests/api.c

@@ -12268,6 +12268,108 @@ static int test_wc_CheckCertSigPubKey(void)
     return EXPECT_RESULT();
 }
 
+/* Trailing data after a certificate's outer SEQUENCE must be rejected at parse
+ * time on a default (strict) build and tolerated when WOLFSSL_NO_ASN_STRICT is
+ * defined. In the latter case the stored/canonical DER (wolfSSL_X509_get_der /
+ * wolfSSL_i2d_X509) must still be bounded to the certificate itself and never
+ * re-emit the trailing bytes. */
+static int test_cert_trailing_data(void)
+{
+    EXPECT_DECLS;
+#if !defined(NO_CERTS) && !defined(NO_RSA) && !defined(NO_FILESYSTEM) && \
+    defined(WOLFSSL_PEM_TO_DER)
+    const char*  ca_cert = "./certs/ca-cert.pem";
+    byte*        pem = NULL;
+    size_t       pemSz = 0;
+    byte*        der = NULL;
+    word32       derBufSz = 0;
+    int          certSz = 0;
+    byte*        certPlusJunk = NULL;
+    word32       certPlusJunkSz = 0;
+    const word32 junkSz = 24;
+    DecodedCert  decoded;
+
+    /* Load a known-good certificate and convert it to DER. */
+    ExpectIntEQ(load_file(ca_cert, &pem, &pemSz), 0);
+    derBufSz = (word32)pemSz; /* DER is always smaller than its PEM. */
+    ExpectNotNull(der = (byte*)XMALLOC(derBufSz, NULL, DYNAMIC_TYPE_TMP_BUFFER));
+    ExpectIntGT(certSz = wc_CertPemToDer(pem, (int)pemSz, der, (int)derBufSz,
+        CERT_TYPE), 0);
+
+    /* Build a buffer holding the certificate followed by N junk bytes. */
+    if (certSz > 0) {
+        certPlusJunkSz = (word32)certSz + junkSz;
+        ExpectNotNull(certPlusJunk = (byte*)XMALLOC(certPlusJunkSz, NULL,
+            DYNAMIC_TYPE_TMP_BUFFER));
+    }
+    if (certPlusJunk != NULL) {
+        XMEMCPY(certPlusJunk, der, (size_t)certSz);
+        XMEMSET(certPlusJunk + certSz, 0xA5, junkSz);
+    }
+
+    /* Sanity: the clean certificate parses successfully. */
+    if (certSz > 0) {
+        XMEMSET(&decoded, 0, sizeof(decoded));
+        wc_InitDecodedCert(&decoded, der, (word32)certSz, NULL);
+        ExpectIntEQ(wc_ParseCert(&decoded, CERT_TYPE, NO_VERIFY, NULL), 0);
+        wc_FreeDecodedCert(&decoded);
+    }
+
+    /* Certificate with appended junk. */
+    if (certPlusJunk != NULL) {
+        XMEMSET(&decoded, 0, sizeof(decoded));
+        wc_InitDecodedCert(&decoded, certPlusJunk, certPlusJunkSz, NULL);
+#ifdef WOLFSSL_NO_ASN_STRICT
+        /* Opt-out build: trailing data is tolerated at parse time, but srcIdx
+         * must stop at the end of the certificate (not the junk). */
+        ExpectIntEQ(wc_ParseCert(&decoded, CERT_TYPE, NO_VERIFY, NULL), 0);
+        ExpectIntEQ((int)decoded.srcIdx, certSz);
+#else
+        /* Default (strict) build: trailing data is rejected. */
+        ExpectIntEQ(wc_ParseCert(&decoded, CERT_TYPE, NO_VERIFY, NULL),
+            WC_NO_ERR_TRACE(ASN_PARSE_E));
+#endif
+        wc_FreeDecodedCert(&decoded);
+    }
+
+    /* derCert bounding: on a non-strict build a certificate with trailing data
+     * loads into a WOLFSSL_X509, but wolfSSL_X509_get_der / wolfSSL_i2d_X509
+     * must re-emit only the certificate bytes (srcIdx), proving the trailing
+     * junk is never stored or re-serialized. */
+#if defined(OPENSSL_EXTRA) && defined(WOLFSSL_NO_ASN_STRICT)
+    if (certPlusJunk != NULL) {
+        WOLFSSL_X509* x509 = NULL;
+        const byte*   getDer = NULL;
+        int           getDerSz = 0;
+        byte*         i2dDer = NULL;
+        int           i2dSz = 0;
+
+        x509 = wolfSSL_X509_load_certificate_buffer(certPlusJunk,
+            (int)certPlusJunkSz, WOLFSSL_FILETYPE_ASN1);
+        ExpectNotNull(x509);
+
+        getDer = wolfSSL_X509_get_der(x509, &getDerSz);
+        ExpectNotNull(getDer);
+        ExpectIntEQ(getDerSz, certSz);
+
+        ExpectIntEQ((i2dSz = wolfSSL_i2d_X509(x509, &i2dDer)), certSz);
+        if (i2dDer != NULL)
+            XFREE(i2dDer, HEAP_HINT, DYNAMIC_TYPE_OPENSSL);
+
+        wolfSSL_X509_free(x509);
+    }
+#endif /* OPENSSL_EXTRA && WOLFSSL_NO_ASN_STRICT */
+
+    if (certPlusJunk != NULL)
+        XFREE(certPlusJunk, NULL, DYNAMIC_TYPE_TMP_BUFFER);
+    if (der != NULL)
+        XFREE(der, NULL, DYNAMIC_TYPE_TMP_BUFFER);
+    if (pem != NULL)
+        XFREE(pem, NULL, DYNAMIC_TYPE_TMP_BUFFER);
+#endif
+    return EXPECT_RESULT();
+}
+
 static int test_wolfSSL_X509_ext_d2i(void)
 {
     EXPECT_DECLS;
@@ -37009,6 +37111,7 @@ TEST_CASE testCases[] = {
     TEST_DECL(test_wc_GetPubKeyDerFromCert),
     TEST_DECL(test_wc_GetSubjectPubKeyInfoDerFromCert),
     TEST_DECL(test_wc_CheckCertSigPubKey),
+    TEST_DECL(test_cert_trailing_data),
 
     /* wolfCrypt ASN tests */
     TEST_DECL(test_ToTraditional),

wolfcrypt/src/asn.c

@@ -22558,7 +22558,7 @@ static int DecodeCertInternal(DecodedCert* cert, int verify, int* criticalExt,
      * whole template but their callers may pass larger buffers. The TRUSTED
      * CERTIFICATE format legitimately carries auxiliary trust data after the
      * certificate, so allow it when cert->allowTrailing is set. */
-    if ((ret == 0) && (!done) && (!stopAtPubKey) && (!stopAfterPubKey) &&
+    if ((ret == 0) && (!done) &&
             (!cert->allowTrailing) && (cert->srcIdx != cert->maxIdx)
 #ifdef WOLFSSL_CERT_REQ
             && (!cert->isCSR)
@@ -23770,6 +23770,11 @@ int ParseCertRelative(DecodedCert* cert, int type, int verify, void* cm,
     int    ret = 0;
 #ifndef WOLFSSL_ASN_TEMPLATE
     word32 confirmOID = 0;
+#ifndef WOLFSSL_NO_ASN_STRICT
+    /* Full input length before GetCertHeader() rebounds cert->maxIdx to the
+     * certificate's outer SEQUENCE; used to detect trailing data. */
+    word32 origMaxIdx = 0;
+#endif
 #ifdef WOLFSSL_CERT_REQ
     int    len = 0;
 #endif
@@ -23808,6 +23813,12 @@ int ParseCertRelative(DecodedCert* cert, int type, int verify, void* cm,
 
     if (cert->sigCtx.state == SIG_STATE_BEGIN) {
 #ifndef WOLFSSL_ASN_TEMPLATE
+#ifndef WOLFSSL_NO_ASN_STRICT
+        /* Capture the full input length now: DecodeToKey()->GetCertHeader()
+         * rebounds cert->maxIdx to the certificate's outer SEQUENCE, after
+         * which trailing bytes would otherwise be invisible. */
+        origMaxIdx = cert->maxIdx;
+#endif
         cert->badDate = 0;
         cert->criticalExt = 0;
         if ((ret = DecodeToKey(cert, verify)) < 0) {
@@ -24050,6 +24061,27 @@ int ParseCertRelative(DecodedCert* cert, int type, int verify, void* cm,
             WOLFSSL_ERROR_VERBOSE(ASN_SIG_OID_E);
             return ASN_SIG_OID_E;
         }
+
+#ifndef WOLFSSL_NO_ASN_STRICT
+        /* Reject trailing data after the certificate's outer SEQUENCE.
+         *
+         * This is the non-template counterpart to the trailing-data check in
+         * DecodeCertInternal(). GetCertHeader() rebounded cert->maxIdx to the
+         * end of the certificate's outer SEQUENCE, so comparing it against the
+         * original input length (origMaxIdx) detects any appended bytes. The
+         * TRUSTED CERTIFICATE format legitimately carries auxiliary trust data
+         * after the certificate (cert->allowTrailing), and CSRs carry their own
+         * trailing structure, so both are exempt. */
+        if ((cert->maxIdx != origMaxIdx) && (!cert->allowTrailing)
+#ifdef WOLFSSL_CERT_REQ
+                && (!cert->isCSR)
+#endif
+                ) {
+            WOLFSSL_MSG("Trailing data after certificate");
+            WOLFSSL_ERROR_VERBOSE(ASN_PARSE_E);
+            return ASN_PARSE_E;
+        }
+#endif /* !WOLFSSL_NO_ASN_STRICT */
 #else
 #ifdef WOLFSSL_CERT_REQ
         if (cert->isCSR) {

wolfssl/wolfcrypt/asn.h

@@ -2142,10 +2142,6 @@ struct DecodedCert {
 
     /* Option Bits */
     WC_BITFIELD subjectCNStored:1;      /* have we saved a copy we own */
-    WC_BITFIELD allowTrailing:1;        /* permit data after the cert's outer
-                                         * SEQUENCE. Used internally for the
-                                         * TRUSTED CERTIFICATE auxiliary trust
-                                         * info. */
     WC_BITFIELD extSubjKeyIdSet:1;      /* Set when the SKID was read from cert */
     WC_BITFIELD extAuthKeyIdSet:1;      /* Set when the AKID was read from cert */
 #ifndef IGNORE_NAME_CONSTRAINTS
@@ -2214,6 +2210,10 @@ struct DecodedCert {
 #ifdef HAVE_RPK
     WC_BITFIELD isRPK:1;   /* indicate the cert is Raw-Public-Key cert in RFC7250 */
 #endif
+    WC_BITFIELD allowTrailing:1;        /* permit data after the cert's outer
+                                         * SEQUENCE. Used internally for the
+                                         * TRUSTED CERTIFICATE auxiliary trust
+                                         * info. */
 #ifdef WC_ASN_UNKNOWN_EXT_CB
     wc_UnknownExtCallback unknownExtCallback;
     wc_UnknownExtCallbackEx unknownExtCallbackEx;