From 04625b2f2416ec1bdb1b64118b40dccb1d0059d3 Mon Sep 17 00:00:00 2001 From: Kareem Date: Mon, 22 Jun 2026 17:14:11 -0700 Subject: [PATCH 1/7] Fix DER buffer size in test_wc_CheckCertSigPubKey. --- tests/api.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/api.c b/tests/api.c index 5ba7f90b8f..88283845c5 100644 --- a/tests/api.c +++ b/tests/api.c @@ -12826,6 +12826,10 @@ static int test_wc_CheckCertSigPubKey(void) ExpectNotNull(cert_der = (byte*)malloc(cert_dersz)); ExpectIntGE(ret = wc_CertPemToDer(cert_buf, (int)cert_sz, cert_der, (int)cert_dersz, CERT_TYPE), 0); + /* Use the actual DER length, not the (larger) PEM buffer size, otherwise + * the decoded cert would have trailing bytes after its outer SEQUENCE. */ + if (ret > 0) + cert_dersz = (word32)ret; wc_InitDecodedCert(&decoded, cert_der, cert_dersz, NULL); ExpectIntEQ(wc_ParseCert(&decoded, CERT_TYPE, NO_VERIFY, NULL), 0); From 7193765f8437c200f128c69025d59214288e34e0 Mon Sep 17 00:00:00 2001 From: Kareem Date: Mon, 22 Jun 2026 17:17:41 -0700 Subject: [PATCH 2/7] Reject certificates with trailing data, except for trusted certificates. In addition, only copy the certificate itself to WOLFSSL_X509, omitting trailing bytes. Thanks to Lucca Hirschi (Inria, France) and the tlspuffin team for the report. --- src/internal.c | 27 +++++++++++++++++++++------ src/x509.c | 11 ++++++----- wolfcrypt/src/asn.c | 40 ++++++++++++++++++++++++++++++++++++++++ wolfssl/wolfcrypt/asn.h | 4 ++++ 4 files changed, 71 insertions(+), 11 deletions(-) diff --git a/src/internal.c b/src/internal.c index 271c941e98..4a23bc7c60 100644 --- a/src/internal.c +++ b/src/internal.c @@ -15290,9 +15290,19 @@ int CopyDecodedToX509(WOLFSSL_X509* x509, DecodedCert* dCert) /* if der contains original source buffer then store for potential * retrieval */ if (dCert->source != NULL && dCert->maxIdx > 0) { - if (AllocDer(&x509->derCert, dCert->maxIdx, CERT_TYPE, x509->heap) - == 0) { - XMEMCPY(x509->derCert->buffer, dCert->source, dCert->maxIdx); + /* Store only the certificate itself, bounded by the end of its outer + * SEQUENCE (dCert->srcIdx), never any trailing bytes that may follow in + * the source buffer. This keeps wolfSSL_i2d_X509 / wolfSSL_X509_get_der + * / wolfSSL_X509_digest canonical - they operate on derCert - even on + * builds/paths that do not reject trailing data (e.g. + * WOLFSSL_NO_ASN_STRICT). It removes only bytes after the certificate, + * so the signed certificate bytes are preserved verbatim. */ + word32 derCertSz = dCert->maxIdx; + if ((dCert->srcIdx > 0) && (dCert->srcIdx < derCertSz)) { + derCertSz = dCert->srcIdx; + } + if (AllocDer(&x509->derCert, derCertSz, CERT_TYPE, x509->heap) == 0) { + XMEMCPY(x509->derCert->buffer, dCert->source, derCertSz); } else { ret = MEMORY_E; @@ -15497,9 +15507,14 @@ int CopyDecodedAcertToX509(WOLFSSL_X509_ACERT* x509, DecodedAcert* dAcert) /* if der contains original source buffer then store for potential * retrieval */ if (dAcert->source != NULL && dAcert->maxIdx > 0) { - if (AllocDer(&x509->derCert, dAcert->maxIdx, CERT_TYPE, x509->heap) - == 0) { - XMEMCPY(x509->derCert->buffer, dAcert->source, dAcert->maxIdx); + /* Bound to the end of the attribute certificate's outer SEQUENCE so no + * trailing bytes after it are ever re-emitted by i2d / get_der. */ + word32 derCertSz = dAcert->maxIdx; + if ((dAcert->srcIdx > 0) && (dAcert->srcIdx < derCertSz)) { + derCertSz = dAcert->srcIdx; + } + if (AllocDer(&x509->derCert, derCertSz, CERT_TYPE, x509->heap) == 0) { + XMEMCPY(x509->derCert->buffer, dAcert->source, derCertSz); } else { ret = MEMORY_E; diff --git a/src/x509.c b/src/x509.c index a8f0690b80..9e1a2f8092 100644 --- a/src/x509.c +++ b/src/x509.c @@ -6181,17 +6181,18 @@ static WOLFSSL_X509* loadX509orX509REQFromBuffer( /* ready to be decoded. */ if (der != NULL && der->buffer != NULL) { WC_DECLARE_VAR(cert, DecodedCert, 1, 0); - /* For TRUSTED_CERT_TYPE, the DER buffer contains the certificate - * followed by auxiliary trust info. ParseCertRelative expects CERT_TYPE - * and will parse only the certificate portion, ignoring the rest. */ - int parseType = (type == TRUSTED_CERT_TYPE) ? CERT_TYPE : type; WC_ALLOC_VAR_EX(cert, DecodedCert, 1, NULL, DYNAMIC_TYPE_DCERT, ret=MEMORY_ERROR); if (WC_VAR_OK(cert)) { InitDecodedCert(cert, der->buffer, der->length, NULL); - ret = ParseCertRelative(cert, parseType, 0, NULL, NULL); + /* For TRUSTED_CERT_TYPE the DER buffer holds the certificate + * followed by auxiliary trust info. ParseCertRelative() recognizes + * the type: it parses only the certificate, permits the trailing + * aux data, and treats it as CERT_TYPE for verification. The DER is + * trimmed to the certificate below. */ + ret = ParseCertRelative(cert, type, 0, NULL, NULL); if (ret == 0) { /* For TRUSTED_CERT_TYPE, truncate the DER buffer to exclude * auxiliary trust data. ParseCertRelative sets srcIdx to the diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 8e6c7560ee..830366cae6 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -22865,6 +22865,35 @@ static int DecodeCertInternal(DecodedCert* cert, int verify, int* criticalExt, } } +#ifndef WOLFSSL_NO_ASN_STRICT + /* Reject trailing data after the certificate's outer SEQUENCE. + * + * The template parser (GetASN_Items) only verifies that constructed items + * nested under the top-level item are fully consumed - it never checks that + * the outermost SEQUENCE spans all the way to maxIdx. Without this check, + * arbitrary bytes appended after a certificate are silently accepted and + * then returned/hashed verbatim by wolfSSL_i2d_X509 / wolfSSL_X509_get_der / + * wolfSSL_X509_digest (which operate on the stored source buffer of length + * maxIdx). + * + * cert->srcIdx points just past the certificate's outer SEQUENCE after the + * x509CertASN parse above. Only enforce this on a full parse; the + * pubkey-only paths (stopAtPubKey/stopAfterPubKey) intentionally parse the + * 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) && + (!cert->allowTrailing) && (cert->srcIdx != cert->maxIdx) +#ifdef WOLFSSL_CERT_REQ + && (!cert->isCSR) +#endif + ) { + WOLFSSL_MSG("Trailing data after certificate"); + WOLFSSL_ERROR_VERBOSE(ASN_PARSE_E); + ret = ASN_PARSE_E; + } +#endif /* !WOLFSSL_NO_ASN_STRICT */ + if ((ret == 0) && (!done) && (badDate != 0)) { /* Parsed whole certificate fine but return any date errors. */ ret = badDate; @@ -24147,6 +24176,17 @@ int ParseCertRelative(DecodedCert* cert, int type, int verify, void* cm, return BAD_FUNC_ARG; } + /* TRUSTED CERTIFICATE blobs (RFC/OpenSSL "TRUSTED CERTIFICATE") carry + * auxiliary trust data after the certificate. Permit that trailing data and + * parse only the certificate prefix; treat it as a normal certificate for + * all verification/path-length logic below. Doing this here (rather than in + * a single caller) means any caller of wc_ParseCert()/ParseCertRelative() + * that passes TRUSTED_CERT_TYPE gets the correct, consistent behavior. */ + if (type == TRUSTED_CERT_TYPE) { + cert->allowTrailing = 1; + type = CERT_TYPE; + } + #ifdef WOLFSSL_CERT_REQ if (type == CERTREQ_TYPE) cert->isCSR = 1; diff --git a/wolfssl/wolfcrypt/asn.h b/wolfssl/wolfcrypt/asn.h index 42c94e0940..2c8735e09f 100644 --- a/wolfssl/wolfcrypt/asn.h +++ b/wolfssl/wolfcrypt/asn.h @@ -2161,6 +2161,10 @@ 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 From 13f9194e61ffc425fc5fbf01958040813be0d85b Mon Sep 17 00:00:00 2001 From: Kareem Date: Thu, 25 Jun 2026 17:38:59 -0700 Subject: [PATCH 3/7] Gate new cert trailing rejection logic behind an opt-in macro to avoid breaking existing use cases. --- .wolfssl_known_macro_extras | 1 + src/internal.c | 8 ++++++-- src/x509.c | 7 ++++--- wolfcrypt/src/asn.c | 17 ++++++++++------- wolfssl/wolfcrypt/asn.h | 2 ++ 5 files changed, 23 insertions(+), 12 deletions(-) diff --git a/.wolfssl_known_macro_extras b/.wolfssl_known_macro_extras index ce340519d8..90aac5fbc3 100644 --- a/.wolfssl_known_macro_extras +++ b/.wolfssl_known_macro_extras @@ -845,6 +845,7 @@ WOLFSSL_CAAM_BLACK_KEY_AESCCM WOLFSSL_CAAM_BLACK_KEY_SM WOLFSSL_CAAM_NO_BLACK_KEY WOLFSSL_CALLBACKS +WOLFSSL_CERT_REJECT_TRAILING WOLFSSL_CHECK_DESKEY WOLFSSL_CHIBIOS WOLFSSL_CLANG_TIDY diff --git a/src/internal.c b/src/internal.c index 4a23bc7c60..ae7cc1d6fb 100644 --- a/src/internal.c +++ b/src/internal.c @@ -15290,6 +15290,8 @@ int CopyDecodedToX509(WOLFSSL_X509* x509, DecodedCert* dCert) /* if der contains original source buffer then store for potential * retrieval */ if (dCert->source != NULL && dCert->maxIdx > 0) { + word32 derCertSz = dCert->maxIdx; +#ifdef WOLFSSL_CERT_REJECT_TRAILING /* Store only the certificate itself, bounded by the end of its outer * SEQUENCE (dCert->srcIdx), never any trailing bytes that may follow in * the source buffer. This keeps wolfSSL_i2d_X509 / wolfSSL_X509_get_der @@ -15297,10 +15299,10 @@ int CopyDecodedToX509(WOLFSSL_X509* x509, DecodedCert* dCert) * builds/paths that do not reject trailing data (e.g. * WOLFSSL_NO_ASN_STRICT). It removes only bytes after the certificate, * so the signed certificate bytes are preserved verbatim. */ - word32 derCertSz = dCert->maxIdx; if ((dCert->srcIdx > 0) && (dCert->srcIdx < derCertSz)) { derCertSz = dCert->srcIdx; } +#endif if (AllocDer(&x509->derCert, derCertSz, CERT_TYPE, x509->heap) == 0) { XMEMCPY(x509->derCert->buffer, dCert->source, derCertSz); } @@ -15507,12 +15509,14 @@ int CopyDecodedAcertToX509(WOLFSSL_X509_ACERT* x509, DecodedAcert* dAcert) /* if der contains original source buffer then store for potential * retrieval */ if (dAcert->source != NULL && dAcert->maxIdx > 0) { + word32 derCertSz = dAcert->maxIdx; +#ifdef WOLFSSL_CERT_REJECT_TRAILING /* Bound to the end of the attribute certificate's outer SEQUENCE so no * trailing bytes after it are ever re-emitted by i2d / get_der. */ - word32 derCertSz = dAcert->maxIdx; if ((dAcert->srcIdx > 0) && (dAcert->srcIdx < derCertSz)) { derCertSz = dAcert->srcIdx; } +#endif if (AllocDer(&x509->derCert, derCertSz, CERT_TYPE, x509->heap) == 0) { XMEMCPY(x509->derCert->buffer, dAcert->source, derCertSz); } diff --git a/src/x509.c b/src/x509.c index 9e1a2f8092..a0005d67de 100644 --- a/src/x509.c +++ b/src/x509.c @@ -6189,9 +6189,10 @@ static WOLFSSL_X509* loadX509orX509REQFromBuffer( InitDecodedCert(cert, der->buffer, der->length, NULL); /* For TRUSTED_CERT_TYPE the DER buffer holds the certificate * followed by auxiliary trust info. ParseCertRelative() recognizes - * the type: it parses only the certificate, permits the trailing - * aux data, and treats it as CERT_TYPE for verification. The DER is - * trimmed to the certificate below. */ + * the type: it parses only the certificate and treats it as + * CERT_TYPE for verification (and, under WOLFSSL_CERT_REJECT_TRAILING, + * permits the trailing aux data). The DER is trimmed to the + * certificate below. */ ret = ParseCertRelative(cert, type, 0, NULL, NULL); if (ret == 0) { /* For TRUSTED_CERT_TYPE, truncate the DER buffer to exclude diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 830366cae6..9680484a7c 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -22865,7 +22865,7 @@ static int DecodeCertInternal(DecodedCert* cert, int verify, int* criticalExt, } } -#ifndef WOLFSSL_NO_ASN_STRICT +#ifdef WOLFSSL_CERT_REJECT_TRAILING /* Reject trailing data after the certificate's outer SEQUENCE. * * The template parser (GetASN_Items) only verifies that constructed items @@ -22892,7 +22892,7 @@ static int DecodeCertInternal(DecodedCert* cert, int verify, int* criticalExt, WOLFSSL_ERROR_VERBOSE(ASN_PARSE_E); ret = ASN_PARSE_E; } -#endif /* !WOLFSSL_NO_ASN_STRICT */ +#endif /* WOLFSSL_CERT_REJECT_TRAILING */ if ((ret == 0) && (!done) && (badDate != 0)) { /* Parsed whole certificate fine but return any date errors. */ @@ -24177,13 +24177,16 @@ int ParseCertRelative(DecodedCert* cert, int type, int verify, void* cm, } /* TRUSTED CERTIFICATE blobs (RFC/OpenSSL "TRUSTED CERTIFICATE") carry - * auxiliary trust data after the certificate. Permit that trailing data and - * parse only the certificate prefix; treat it as a normal certificate for - * all verification/path-length logic below. Doing this here (rather than in - * a single caller) means any caller of wc_ParseCert()/ParseCertRelative() - * that passes TRUSTED_CERT_TYPE gets the correct, consistent behavior. */ + * auxiliary trust data after the certificate. Parse only the certificate + * prefix and treat it as a normal certificate for all verification / + * path-length logic below. Doing this here (rather than in a single caller) + * means any caller of wc_ParseCert()/ParseCertRelative() that passes + * TRUSTED_CERT_TYPE gets the correct, consistent behavior. */ if (type == TRUSTED_CERT_TYPE) { +#ifdef WOLFSSL_CERT_REJECT_TRAILING + /* Opt out of the trailing-data rejection for the aux trust info. */ cert->allowTrailing = 1; +#endif type = CERT_TYPE; } diff --git a/wolfssl/wolfcrypt/asn.h b/wolfssl/wolfcrypt/asn.h index 2c8735e09f..a5e54f3b44 100644 --- a/wolfssl/wolfcrypt/asn.h +++ b/wolfssl/wolfcrypt/asn.h @@ -2161,10 +2161,12 @@ struct DecodedCert { /* Option Bits */ WC_BITFIELD subjectCNStored:1; /* have we saved a copy we own */ +#ifdef WOLFSSL_CERT_REJECT_TRAILING WC_BITFIELD allowTrailing:1; /* permit data after the cert's outer * SEQUENCE. Used internally for the * TRUSTED CERTIFICATE auxiliary trust * info. */ +#endif 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 From c25168179b2fa1774159652f16957273f0d2903d Mon Sep 17 00:00:00 2001 From: Kareem Date: Mon, 6 Jul 2026 16:02:10 -0700 Subject: [PATCH 4/7] Remove WOLFSSL_CERT_REJECT_TRAILING. Reject certs with trailing data by default, allow opt-out via WOLFSSL_NO_ASN_STRICT. --- .wolfssl_known_macro_extras | 1 - src/internal.c | 8 ++------ src/x509.c | 7 +++---- wolfcrypt/src/asn.c | 17 +++++++---------- wolfssl/wolfcrypt/asn.h | 2 -- 5 files changed, 12 insertions(+), 23 deletions(-) diff --git a/.wolfssl_known_macro_extras b/.wolfssl_known_macro_extras index 90aac5fbc3..ce340519d8 100644 --- a/.wolfssl_known_macro_extras +++ b/.wolfssl_known_macro_extras @@ -845,7 +845,6 @@ WOLFSSL_CAAM_BLACK_KEY_AESCCM WOLFSSL_CAAM_BLACK_KEY_SM WOLFSSL_CAAM_NO_BLACK_KEY WOLFSSL_CALLBACKS -WOLFSSL_CERT_REJECT_TRAILING WOLFSSL_CHECK_DESKEY WOLFSSL_CHIBIOS WOLFSSL_CLANG_TIDY diff --git a/src/internal.c b/src/internal.c index ae7cc1d6fb..4a23bc7c60 100644 --- a/src/internal.c +++ b/src/internal.c @@ -15290,8 +15290,6 @@ int CopyDecodedToX509(WOLFSSL_X509* x509, DecodedCert* dCert) /* if der contains original source buffer then store for potential * retrieval */ if (dCert->source != NULL && dCert->maxIdx > 0) { - word32 derCertSz = dCert->maxIdx; -#ifdef WOLFSSL_CERT_REJECT_TRAILING /* Store only the certificate itself, bounded by the end of its outer * SEQUENCE (dCert->srcIdx), never any trailing bytes that may follow in * the source buffer. This keeps wolfSSL_i2d_X509 / wolfSSL_X509_get_der @@ -15299,10 +15297,10 @@ int CopyDecodedToX509(WOLFSSL_X509* x509, DecodedCert* dCert) * builds/paths that do not reject trailing data (e.g. * WOLFSSL_NO_ASN_STRICT). It removes only bytes after the certificate, * so the signed certificate bytes are preserved verbatim. */ + word32 derCertSz = dCert->maxIdx; if ((dCert->srcIdx > 0) && (dCert->srcIdx < derCertSz)) { derCertSz = dCert->srcIdx; } -#endif if (AllocDer(&x509->derCert, derCertSz, CERT_TYPE, x509->heap) == 0) { XMEMCPY(x509->derCert->buffer, dCert->source, derCertSz); } @@ -15509,14 +15507,12 @@ int CopyDecodedAcertToX509(WOLFSSL_X509_ACERT* x509, DecodedAcert* dAcert) /* if der contains original source buffer then store for potential * retrieval */ if (dAcert->source != NULL && dAcert->maxIdx > 0) { - word32 derCertSz = dAcert->maxIdx; -#ifdef WOLFSSL_CERT_REJECT_TRAILING /* Bound to the end of the attribute certificate's outer SEQUENCE so no * trailing bytes after it are ever re-emitted by i2d / get_der. */ + word32 derCertSz = dAcert->maxIdx; if ((dAcert->srcIdx > 0) && (dAcert->srcIdx < derCertSz)) { derCertSz = dAcert->srcIdx; } -#endif if (AllocDer(&x509->derCert, derCertSz, CERT_TYPE, x509->heap) == 0) { XMEMCPY(x509->derCert->buffer, dAcert->source, derCertSz); } diff --git a/src/x509.c b/src/x509.c index a0005d67de..9e1a2f8092 100644 --- a/src/x509.c +++ b/src/x509.c @@ -6189,10 +6189,9 @@ static WOLFSSL_X509* loadX509orX509REQFromBuffer( InitDecodedCert(cert, der->buffer, der->length, NULL); /* For TRUSTED_CERT_TYPE the DER buffer holds the certificate * followed by auxiliary trust info. ParseCertRelative() recognizes - * the type: it parses only the certificate and treats it as - * CERT_TYPE for verification (and, under WOLFSSL_CERT_REJECT_TRAILING, - * permits the trailing aux data). The DER is trimmed to the - * certificate below. */ + * the type: it parses only the certificate, permits the trailing + * aux data, and treats it as CERT_TYPE for verification. The DER is + * trimmed to the certificate below. */ ret = ParseCertRelative(cert, type, 0, NULL, NULL); if (ret == 0) { /* For TRUSTED_CERT_TYPE, truncate the DER buffer to exclude diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 9680484a7c..830366cae6 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -22865,7 +22865,7 @@ static int DecodeCertInternal(DecodedCert* cert, int verify, int* criticalExt, } } -#ifdef WOLFSSL_CERT_REJECT_TRAILING +#ifndef WOLFSSL_NO_ASN_STRICT /* Reject trailing data after the certificate's outer SEQUENCE. * * The template parser (GetASN_Items) only verifies that constructed items @@ -22892,7 +22892,7 @@ static int DecodeCertInternal(DecodedCert* cert, int verify, int* criticalExt, WOLFSSL_ERROR_VERBOSE(ASN_PARSE_E); ret = ASN_PARSE_E; } -#endif /* WOLFSSL_CERT_REJECT_TRAILING */ +#endif /* !WOLFSSL_NO_ASN_STRICT */ if ((ret == 0) && (!done) && (badDate != 0)) { /* Parsed whole certificate fine but return any date errors. */ @@ -24177,16 +24177,13 @@ int ParseCertRelative(DecodedCert* cert, int type, int verify, void* cm, } /* TRUSTED CERTIFICATE blobs (RFC/OpenSSL "TRUSTED CERTIFICATE") carry - * auxiliary trust data after the certificate. Parse only the certificate - * prefix and treat it as a normal certificate for all verification / - * path-length logic below. Doing this here (rather than in a single caller) - * means any caller of wc_ParseCert()/ParseCertRelative() that passes - * TRUSTED_CERT_TYPE gets the correct, consistent behavior. */ + * auxiliary trust data after the certificate. Permit that trailing data and + * parse only the certificate prefix; treat it as a normal certificate for + * all verification/path-length logic below. Doing this here (rather than in + * a single caller) means any caller of wc_ParseCert()/ParseCertRelative() + * that passes TRUSTED_CERT_TYPE gets the correct, consistent behavior. */ if (type == TRUSTED_CERT_TYPE) { -#ifdef WOLFSSL_CERT_REJECT_TRAILING - /* Opt out of the trailing-data rejection for the aux trust info. */ cert->allowTrailing = 1; -#endif type = CERT_TYPE; } diff --git a/wolfssl/wolfcrypt/asn.h b/wolfssl/wolfcrypt/asn.h index a5e54f3b44..2c8735e09f 100644 --- a/wolfssl/wolfcrypt/asn.h +++ b/wolfssl/wolfcrypt/asn.h @@ -2161,12 +2161,10 @@ struct DecodedCert { /* Option Bits */ WC_BITFIELD subjectCNStored:1; /* have we saved a copy we own */ -#ifdef WOLFSSL_CERT_REJECT_TRAILING WC_BITFIELD allowTrailing:1; /* permit data after the cert's outer * SEQUENCE. Used internally for the * TRUSTED CERTIFICATE auxiliary trust * info. */ -#endif 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 From 03a81d8181ac50756d019cb217c6011548fa44a5 Mon Sep 17 00:00:00 2001 From: Kareem Date: Tue, 14 Jul 2026 18:07:00 -0700 Subject: [PATCH 5/7] Code review feedback --- tests/api.c | 103 ++++++++++++++++++++++++++++++++++++++++ wolfcrypt/src/asn.c | 34 ++++++++++++- wolfssl/wolfcrypt/asn.h | 8 ++-- 3 files changed, 140 insertions(+), 5 deletions(-) diff --git a/tests/api.c b/tests/api.c index 88283845c5..f8ed3ded11 100644 --- a/tests/api.c +++ b/tests/api.c @@ -12871,6 +12871,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; @@ -38937,6 +39039,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), diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 830366cae6..ee7d8d6bf6 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -22882,7 +22882,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) @@ -24156,6 +24156,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 @@ -24194,6 +24199,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) { @@ -24436,6 +24447,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) { diff --git a/wolfssl/wolfcrypt/asn.h b/wolfssl/wolfcrypt/asn.h index 2c8735e09f..308acc14d5 100644 --- a/wolfssl/wolfcrypt/asn.h +++ b/wolfssl/wolfcrypt/asn.h @@ -2161,10 +2161,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 @@ -2233,6 +2229,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; From 8009964ae8c8052f8f5519c536be9540dfe8efdd Mon Sep 17 00:00:00 2001 From: Kareem Date: Thu, 23 Jul 2026 16:58:53 -0700 Subject: [PATCH 6/7] Code review feedback --- wolfcrypt/src/asn.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index ee7d8d6bf6..1e365d463a 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -22883,11 +22883,7 @@ static int DecodeCertInternal(DecodedCert* cert, int verify, int* criticalExt, * CERTIFICATE format legitimately carries auxiliary trust data after the * certificate, so allow it when cert->allowTrailing is set. */ if ((ret == 0) && (!done) && - (!cert->allowTrailing) && (cert->srcIdx != cert->maxIdx) -#ifdef WOLFSSL_CERT_REQ - && (!cert->isCSR) -#endif - ) { + (!cert->allowTrailing) && (cert->srcIdx != cert->maxIdx)) { WOLFSSL_MSG("Trailing data after certificate"); WOLFSSL_ERROR_VERBOSE(ASN_PARSE_E); ret = ASN_PARSE_E; From 5fb8a29bd0deba7a26fa41c2e1617664930a87af Mon Sep 17 00:00:00 2001 From: Kareem Date: Mon, 10 Aug 2026 16:54:43 -0700 Subject: [PATCH 7/7] Code review feedback --- src/internal.c | 56 ++++++++++++++-------------- src/x509.c | 11 +----- tests/api.c | 91 +++++++++++++++++++++++++++++++++++++++------ wolfcrypt/src/asn.c | 66 +++++++++++++++++++------------- 4 files changed, 150 insertions(+), 74 deletions(-) diff --git a/src/internal.c b/src/internal.c index 4a23bc7c60..42c0f9c97d 100644 --- a/src/internal.c +++ b/src/internal.c @@ -14761,6 +14761,27 @@ static void CopyDateToASN1_TIME(const byte* srcDate, int srcDateLen, dst->length = 0; } } + +/* Store the DER object at the start of source in a new DerBuffer, bounded by + * the end of the object rather than maxIdx (the full input length). With + * WOLFSSL_ASN_TEMPLATE, srcIdx is written only when the object's outer + * SEQUENCE parses and so marks its end. The original parser instead rebounds + * maxIdx to the outer SEQUENCE itself, while its srcIdx advances step by step + * and can rest inside the object, so there it is not a valid bound. */ +static int AllocCopyDecodedDer(DerBuffer** der, const byte* source, + word32 maxIdx, word32 srcIdx, void* heap) +{ + word32 derSz = maxIdx; + +#ifdef WOLFSSL_ASN_TEMPLATE + if ((srcIdx > 0) && (srcIdx < derSz)) { + derSz = srcIdx; + } +#else + (void)srcIdx; +#endif + return AllocCopyDer(der, source, derSz, CERT_TYPE, heap); +} #endif /* KEEP_PEER_CERT || SESSION_CERTS || OPENSSL_EXTRA || * OPENSSL_EXTRA_X509_SMALL || WOLFSSL_ACERT */ @@ -15288,23 +15309,11 @@ int CopyDecodedToX509(WOLFSSL_X509* x509, DecodedCert* dCert) ret = copyRet; /* if der contains original source buffer then store for potential - * retrieval */ + * retrieval. Store only the certificate itself, never trailing bytes that + * may follow it in the source buffer. */ if (dCert->source != NULL && dCert->maxIdx > 0) { - /* Store only the certificate itself, bounded by the end of its outer - * SEQUENCE (dCert->srcIdx), never any trailing bytes that may follow in - * the source buffer. This keeps wolfSSL_i2d_X509 / wolfSSL_X509_get_der - * / wolfSSL_X509_digest canonical - they operate on derCert - even on - * builds/paths that do not reject trailing data (e.g. - * WOLFSSL_NO_ASN_STRICT). It removes only bytes after the certificate, - * so the signed certificate bytes are preserved verbatim. */ - word32 derCertSz = dCert->maxIdx; - if ((dCert->srcIdx > 0) && (dCert->srcIdx < derCertSz)) { - derCertSz = dCert->srcIdx; - } - if (AllocDer(&x509->derCert, derCertSz, CERT_TYPE, x509->heap) == 0) { - XMEMCPY(x509->derCert->buffer, dCert->source, derCertSz); - } - else { + if (AllocCopyDecodedDer(&x509->derCert, dCert->source, dCert->maxIdx, + dCert->srcIdx, x509->heap) != 0) { ret = MEMORY_E; } } @@ -15505,18 +15514,11 @@ int CopyDecodedAcertToX509(WOLFSSL_X509_ACERT* x509, DecodedAcert* dAcert) } /* if der contains original source buffer then store for potential - * retrieval */ + * retrieval. Store only the attribute certificate itself, never trailing + * bytes that may follow it in the source buffer. */ if (dAcert->source != NULL && dAcert->maxIdx > 0) { - /* Bound to the end of the attribute certificate's outer SEQUENCE so no - * trailing bytes after it are ever re-emitted by i2d / get_der. */ - word32 derCertSz = dAcert->maxIdx; - if ((dAcert->srcIdx > 0) && (dAcert->srcIdx < derCertSz)) { - derCertSz = dAcert->srcIdx; - } - if (AllocDer(&x509->derCert, derCertSz, CERT_TYPE, x509->heap) == 0) { - XMEMCPY(x509->derCert->buffer, dAcert->source, derCertSz); - } - else { + if (AllocCopyDecodedDer(&x509->derCert, dAcert->source, dAcert->maxIdx, + dAcert->srcIdx, x509->heap) != 0) { ret = MEMORY_E; } } diff --git a/src/x509.c b/src/x509.c index 9e1a2f8092..f39e276c2f 100644 --- a/src/x509.c +++ b/src/x509.c @@ -6190,17 +6190,10 @@ static WOLFSSL_X509* loadX509orX509REQFromBuffer( /* For TRUSTED_CERT_TYPE the DER buffer holds the certificate * followed by auxiliary trust info. ParseCertRelative() recognizes * the type: it parses only the certificate, permits the trailing - * aux data, and treats it as CERT_TYPE for verification. The DER is - * trimmed to the certificate below. */ + * aux data, and treats it as CERT_TYPE for verification. + * CopyDecodedToX509() bounds the stored DER to the certificate. */ ret = ParseCertRelative(cert, type, 0, NULL, NULL); if (ret == 0) { - /* For TRUSTED_CERT_TYPE, truncate the DER buffer to exclude - * auxiliary trust data. ParseCertRelative sets srcIdx to the - * end of the certificate, so we adjust cert->maxIdx accordingly. */ - if (type == TRUSTED_CERT_TYPE && cert->srcIdx < cert->maxIdx) { - cert->maxIdx = cert->srcIdx; - } - x509 = (WOLFSSL_X509*)XMALLOC(sizeof(WOLFSSL_X509), NULL, DYNAMIC_TYPE_X509); if (x509 != NULL) { diff --git a/tests/api.c b/tests/api.c index f8ed3ded11..f30ff0edae 100644 --- a/tests/api.c +++ b/tests/api.c @@ -12824,7 +12824,7 @@ static int test_wc_CheckCertSigPubKey(void) ExpectIntEQ(load_file(ca_cert, &cert_buf, &cert_sz), 0); cert_dersz = (word32)cert_sz; /* DER will be smaller than PEM */ ExpectNotNull(cert_der = (byte*)malloc(cert_dersz)); - ExpectIntGE(ret = wc_CertPemToDer(cert_buf, (int)cert_sz, cert_der, + ExpectIntGT(ret = wc_CertPemToDer(cert_buf, (int)cert_sz, cert_der, (int)cert_dersz, CERT_TYPE), 0); /* Use the actual DER length, not the (larger) PEM buffer size, otherwise * the decoded cert would have trailing bytes after its outer SEQUENCE. */ @@ -12875,13 +12875,16 @@ static int test_wc_CheckCertSigPubKey(void) * 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) + * re-emit the trailing bytes. TRUSTED CERTIFICATE blobs are the exception: + * their auxiliary trust data after the certificate parses on every build, with + * the stored DER again bounded to the certificate itself. */ +static int test_wc_ParseCert_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"; + const char* trusted_cert = "./certs/test/ossl-trusted-cert.pem"; byte* pem = NULL; size_t pemSz = 0; byte* der = NULL; @@ -12890,6 +12893,10 @@ static int test_cert_trailing_data(void) byte* certPlusJunk = NULL; word32 certPlusJunkSz = 0; const word32 junkSz = 24; + DerBuffer* trustedDer = NULL; + word32 trustedCertSz = 0; + EncryptedInfo info; + int keyFormat = 0; DecodedCert decoded; /* Load a known-good certificate and convert it to DER. */ @@ -12912,7 +12919,6 @@ static int test_cert_trailing_data(void) /* 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); @@ -12920,7 +12926,6 @@ static int test_cert_trailing_data(void) /* 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 @@ -12935,11 +12940,44 @@ static int test_cert_trailing_data(void) 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) + /* TRUSTED CERTIFICATE: the certificate is followed by auxiliary trust + * data. TRUSTED_CERT_TYPE permits it on every build and srcIdx must still + * stop at the end of the certificate. */ + XMEMSET(&info, 0, sizeof(info)); + if (pem != NULL) { + XFREE(pem, NULL, DYNAMIC_TYPE_TMP_BUFFER); + pem = NULL; + } + ExpectIntEQ(load_file(trusted_cert, &pem, &pemSz), 0); + ExpectIntEQ(wc_PemToDer(pem, (long)pemSz, TRUSTED_CERT_TYPE, &trustedDer, + NULL, &info, &keyFormat), 0); + if (trustedDer != NULL) { + wc_InitDecodedCert(&decoded, trustedDer->buffer, trustedDer->length, + NULL); + ExpectIntEQ(wc_ParseCert(&decoded, TRUSTED_CERT_TYPE, NO_VERIFY, NULL), + 0); + trustedCertSz = decoded.srcIdx; + ExpectIntGT((int)trustedCertSz, 0); + /* The fixture must actually carry auxiliary data after the certificate + * for this test to prove anything. */ + ExpectIntLT((int)trustedCertSz, (int)trustedDer->length); + wc_FreeDecodedCert(&decoded); + +#ifndef WOLFSSL_NO_ASN_STRICT + /* The same blob parsed as a plain certificate is rejected. */ + wc_InitDecodedCert(&decoded, trustedDer->buffer, trustedDer->length, + NULL); + ExpectIntEQ(wc_ParseCert(&decoded, CERT_TYPE, NO_VERIFY, NULL), + WC_NO_ERR_TRACE(ASN_PARSE_E)); + wc_FreeDecodedCert(&decoded); +#endif + } + +#ifdef OPENSSL_EXTRA + /* derCert bounding: wolfSSL_X509_get_der / wolfSSL_i2d_X509 must re-emit + * only the certificate bytes, proving data after the certificate is never + * stored or re-serialized. */ +#ifdef WOLFSSL_NO_ASN_STRICT if (certPlusJunk != NULL) { WOLFSSL_X509* x509 = NULL; const byte* getDer = NULL; @@ -12961,8 +12999,37 @@ static int test_cert_trailing_data(void) wolfSSL_X509_free(x509); } -#endif /* OPENSSL_EXTRA && WOLFSSL_NO_ASN_STRICT */ +#else + /* Default (strict) build: a certificate with trailing junk does not load + * as DER at all. */ + if (certPlusJunk != NULL) { + ExpectNull(wolfSSL_X509_load_certificate_buffer(certPlusJunk, + (int)certPlusJunkSz, WOLFSSL_FILETYPE_ASN1)); + } +#endif /* WOLFSSL_NO_ASN_STRICT */ +#ifndef NO_BIO + /* PEM_read_bio_X509_AUX consumes the TRUSTED CERTIFICATE format; the + * X509's DER must hold only the certificate, not the auxiliary data. */ + if ((pem != NULL) && (trustedCertSz > 0)) { + WOLFSSL_BIO* bio = NULL; + WOLFSSL_X509* x509 = NULL; + const byte* auxDer = NULL; + int auxDerSz = 0; + + ExpectNotNull(bio = wolfSSL_BIO_new_mem_buf(pem, (int)pemSz)); + ExpectNotNull(x509 = wolfSSL_PEM_read_bio_X509_AUX(bio, NULL, NULL, + NULL)); + auxDer = wolfSSL_X509_get_der(x509, &auxDerSz); + ExpectNotNull(auxDer); + ExpectIntEQ(auxDerSz, (int)trustedCertSz); + wolfSSL_X509_free(x509); + wolfSSL_BIO_free(bio); + } +#endif /* !NO_BIO */ +#endif /* OPENSSL_EXTRA */ + + wc_FreeDer(&trustedDer); if (certPlusJunk != NULL) XFREE(certPlusJunk, NULL, DYNAMIC_TYPE_TMP_BUFFER); if (der != NULL) @@ -39039,7 +39106,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), + TEST_DECL(test_wc_ParseCert_trailing_data), /* wolfCrypt ASN tests */ TEST_DECL(test_ToTraditional), diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 1e365d463a..934a98cb6d 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -22557,6 +22557,7 @@ static int DecodeCertInternal(DecodedCert* cert, int verify, int* criticalExt, #if defined(HAVE_RPK) /* try to parse the cert as Raw Public Key cert */ + word32 rpkStartIdx = cert->srcIdx; DECL_ASNGETDATA(RPKdataASN, RPKCertASN_Length); CALLOC_ASNGETDATA(RPKdataASN, RPKCertASN_Length, ret, cert->heap); GetASN_OID(&RPKdataASN[RPKCERTASN_IDX_SPUBKEYINFO_ALGO_OID], @@ -22596,10 +22597,22 @@ static int DecodeCertInternal(DecodedCert* cert, int verify, int* criticalExt, FREE_ASNGETDATA(RPKdataASN, cert->heap); if (ret == 0) { +#ifndef WOLFSSL_NO_ASN_STRICT + /* Data after the SubjectPublicKeyInfo is rejected the same way as + * data after a certificate's outer SEQUENCE below. */ + if ((!stopAtPubKey) && (!stopAfterPubKey) && (!cert->allowTrailing) && + (cert->srcIdx != cert->maxIdx)) { + WOLFSSL_MSG("Trailing data after certificate"); + WOLFSSL_ERROR_VERBOSE(ASN_PARSE_E); + return ASN_PARSE_E; + } +#endif /* !WOLFSSL_NO_ASN_STRICT */ return ret; } else { - ret = 0; /* proceed to the original x509 parsing */ + /* Restore the index and proceed to the original x509 parsing. */ + cert->srcIdx = rpkStartIdx; + ret = 0; } #endif /* HAVE_RPK */ @@ -22630,6 +22643,19 @@ static int DecodeCertInternal(DecodedCert* cert, int verify, int* criticalExt, } #endif } +#ifndef WOLFSSL_NO_ASN_STRICT + /* cert->srcIdx is now just past the certificate's outer SEQUENCE. Reject + * any data after it, with two exceptions: the pubkey-only paths + * (stopAtPubKey/stopAfterPubKey), whose callers may pass larger buffers, + * and the TRUSTED CERTIFICATE format (cert->allowTrailing), which carries + * auxiliary trust data after the certificate. */ + if ((ret == 0) && (!stopAtPubKey) && (!stopAfterPubKey) && + (!cert->allowTrailing) && (cert->srcIdx != cert->maxIdx)) { + WOLFSSL_MSG("Trailing data after certificate"); + WOLFSSL_ERROR_VERBOSE(ASN_PARSE_E); + ret = ASN_PARSE_E; + } +#endif /* !WOLFSSL_NO_ASN_STRICT */ /* Check version is valid/supported - can't be negative. */ if ((ret == 0) && (version > MAX_X509_VERSION)) { WOLFSSL_MSG("Unexpected certificate version"); @@ -22865,31 +22891,6 @@ static int DecodeCertInternal(DecodedCert* cert, int verify, int* criticalExt, } } -#ifndef WOLFSSL_NO_ASN_STRICT - /* Reject trailing data after the certificate's outer SEQUENCE. - * - * The template parser (GetASN_Items) only verifies that constructed items - * nested under the top-level item are fully consumed - it never checks that - * the outermost SEQUENCE spans all the way to maxIdx. Without this check, - * arbitrary bytes appended after a certificate are silently accepted and - * then returned/hashed verbatim by wolfSSL_i2d_X509 / wolfSSL_X509_get_der / - * wolfSSL_X509_digest (which operate on the stored source buffer of length - * maxIdx). - * - * cert->srcIdx points just past the certificate's outer SEQUENCE after the - * x509CertASN parse above. Only enforce this on a full parse; the - * pubkey-only paths (stopAtPubKey/stopAfterPubKey) intentionally parse the - * 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) && - (!cert->allowTrailing) && (cert->srcIdx != cert->maxIdx)) { - WOLFSSL_MSG("Trailing data after certificate"); - WOLFSSL_ERROR_VERBOSE(ASN_PARSE_E); - ret = ASN_PARSE_E; - } -#endif /* !WOLFSSL_NO_ASN_STRICT */ - if ((ret == 0) && (!done) && (badDate != 0)) { /* Parsed whole certificate fine but return any date errors. */ ret = badDate; @@ -24217,6 +24218,15 @@ int ParseCertRelative(DecodedCert* cert, int type, int verify, void* cm, WOLFSSL_MSG("Parsed Past Key"); #if defined(HAVE_RPK) if (cert->isRPK) { +#ifndef WOLFSSL_NO_ASN_STRICT + /* A Raw Public Key cert is only the SubjectPublicKeyInfo; reject + * data after it like data after a certificate's outer SEQUENCE. */ + if ((!cert->allowTrailing) && (cert->srcIdx != cert->maxIdx)) { + WOLFSSL_MSG("Trailing data after certificate"); + WOLFSSL_ERROR_VERBOSE(ASN_PARSE_E); + return ASN_PARSE_E; + } +#endif /* !WOLFSSL_NO_ASN_STRICT */ return ret; } #endif /* HAVE_RPK */ @@ -35849,6 +35859,10 @@ static int OcspCheckCert(OcspResponse *resp, int noVerify, #endif InitDecodedCert(cert, resp->cert, resp->certSz, heap); + /* The response's certs field may hold a chain (RFC 6960: SEQUENCE OF + * Certificate) and certSz spans all of it; only the first certificate is + * parsed and used, so permit the data that follows it. */ + cert->allowTrailing = 1; ret = ParseCertRelative(cert, CERT_TYPE, noVerify ? NO_VERIFY : VERIFY_OCSP_CERT, cm, resp->pendingCAs);