From 262737d63fd10aa96878760ea598571106305f07 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 28 Apr 2026 11:54:46 -0500 Subject: [PATCH] Fixes from review --- tests/api.c | 54 +++++++++++++++++++-- wolfcrypt/src/asn.c | 101 ++++++++++++++++++---------------------- wolfssl/wolfcrypt/asn.h | 48 ++++++++++++++----- 3 files changed, 133 insertions(+), 70 deletions(-) diff --git a/tests/api.c b/tests/api.c index c30ae094bf..4a41d3379e 100644 --- a/tests/api.c +++ b/tests/api.c @@ -22220,6 +22220,26 @@ static word32 build_otherName_san(byte* out, word32 outSz, const char* val7) return (word32)(sizeof(prefix) + 7); } +/* Build a NameConstraints extension value with a single excludedSubtree + * carrying a registeredID GeneralName for OID 1.2.3.4. registeredID is a + * GeneralName form wolfSSL does not enforce, so DecodeSubtree() must + * record it as 'unsupported' and ConfirmNameConstraints() must fail + * closed when the extension is critical (RFC 5280 4.2.1.10). */ +static word32 build_registeredID_nameConstraints(byte* out, word32 outSz) +{ + static const byte ridNc[] = { + 0x30, 0x09, /* SEQUENCE, 9 */ + 0xA1, 0x07, /* [1] excluded, 7 */ + 0x30, 0x05, /* GeneralSubtree, 5 */ + 0x88, 0x03, /* [8] regId, 3 */ + 0x2A, 0x03, 0x04 /* OID 1.2.3.4 */ + }; + if (outSz < sizeof(ridNc)) + return 0; + XMEMCPY(out, ridNc, sizeof(ridNc)); + return (word32)sizeof(ridNc); +} + /* Build a NameConstraints extension value carrying a single subtree of * the given list type ([0] permitted or [1] excluded) for an otherName * UPN whose UTF8 value is the given 7-byte string. */ @@ -22334,9 +22354,11 @@ static int verify_with_otherName_chain(const byte* nameConstraintsDer, goto done; if (wc_SetSubjectKeyIdFromPublicKey_ex(&cert, ECC_TYPE, &leafKey) != 0) goto done; - if (sanDerSz > sizeof(cert.altNames)) goto done; - XMEMCPY(cert.altNames, sanDer, sanDerSz); - cert.altNamesSz = (int)sanDerSz; + if (sanDer != NULL && sanDerSz > 0) { + if (sanDerSz > sizeof(cert.altNames)) goto done; + XMEMCPY(cert.altNames, sanDer, sanDerSz); + cert.altNamesSz = (int)sanDerSz; + } if (wc_MakeCert(&cert, leafDer, FOURK_BUF, NULL, &leafKey, &rng) < 0) goto done; leafDerSz = wc_SignCert(cert.bodySz, cert.sigType, leafDer, FOURK_BUF, @@ -22379,6 +22401,10 @@ done: * (excluded is enforced regardless of criticality) * 4. Critical permitted subtree, leaf SAN matches -> accept * 5. Critical permitted subtree, leaf SAN does NOT match -> reject + * 6. Critical nameConstraints carrying an unsupported form + * (registeredID), leaf has no relevant SAN -> reject + * (RFC 5280 4.2.1.10 fail-closed for unprocessed forms) + * 7. Same as (6) but non-critical -> accept */ static int test_NameConstraints_OtherName(void) { @@ -22392,8 +22418,9 @@ static int test_NameConstraints_OtherName(void) byte sanAllowed[64]; byte ncExcludedBlocked[64]; byte ncPermittedAllowed[64]; + byte ncRegisteredID[16]; word32 sanBlockedSz, sanAllowedSz; - word32 ncExcludedBlockedSz, ncPermittedAllowedSz; + word32 ncExcludedBlockedSz, ncPermittedAllowedSz, ncRegisteredIDSz; sanBlockedSz = build_otherName_san(sanBlocked, sizeof(sanBlocked), "blocked"); @@ -22403,10 +22430,13 @@ static int test_NameConstraints_OtherName(void) ncExcludedBlocked, sizeof(ncExcludedBlocked), 1, "blocked"); ncPermittedAllowedSz = build_otherName_nameConstraints( ncPermittedAllowed, sizeof(ncPermittedAllowed), 0, "allowed"); + ncRegisteredIDSz = build_registeredID_nameConstraints( + ncRegisteredID, sizeof(ncRegisteredID)); ExpectIntGT((int)sanBlockedSz, 0); ExpectIntGT((int)sanAllowedSz, 0); ExpectIntGT((int)ncExcludedBlockedSz, 0); ExpectIntGT((int)ncPermittedAllowedSz, 0); + ExpectIntGT((int)ncRegisteredIDSz, 0); /* (1) Original bypass scenario: critical excluded otherName matches * the leaf's otherName SAN. Must be rejected. */ @@ -22445,6 +22475,22 @@ static int test_NameConstraints_OtherName(void) ncPermittedAllowed, ncPermittedAllowedSz, 1, sanBlocked, sanBlockedSz), WC_NO_ERR_TRACE(ASN_NAME_INVALID_E)); + + /* (6) Critical nameConstraints carrying a GeneralName form wolfSSL + * does not enforce (registeredID). RFC 5280 4.2.1.10 requires the + * verifier to either process the constraint or reject; we reject + * fail-closed. The leaf needs no SAN to exercise this path. */ + ExpectIntEQ(verify_with_otherName_chain( + ncRegisteredID, ncRegisteredIDSz, 1, NULL, 0), + WC_NO_ERR_TRACE(ASN_NAME_INVALID_E)); + + /* (7) Same as (6) but non-critical: RFC 5280 only mandates the + * fail-closed reject when the extension is critical, so a + * non-critical unsupported constraint form is silently ignored + * and verification succeeds. */ + ExpectIntEQ(verify_with_otherName_chain( + ncRegisteredID, ncRegisteredIDSz, 0, NULL, 0), + 0); #endif return EXPECT_RESULT(); } diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index f2737c9857..631a3b8255 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -12139,6 +12139,8 @@ void FreeDecodedCert(DecodedCert* cert) FreeAltNames(cert->altEmailNames, cert->heap); if (cert->altDirNames) FreeAltNames(cert->altDirNames, cert->heap); + if (cert->altOtherNamesRaw) + FreeAltNames(cert->altOtherNamesRaw, cert->heap); if (cert->permittedNames) FreeNameSubtrees(cert->permittedNames, cert->heap); if (cert->excludedNames) @@ -17622,6 +17624,19 @@ int wolfssl_local_MatchIpSubnet(const byte* ip, int ipSz, return match; } +/* RFC 5280 4.2.1.10: otherName matching is byte-exact comparison of the + * full OtherName encoding (OID || [0] EXPLICIT value). Both the leaf SAN + * (cert->altOtherNamesRaw) and the constraint subtree (Base_entry from + * DecodeSubtree) store the same form, so a memcmp suffices. */ +static int MatchOtherNameConstraint(DNS_entry* name, Base_entry* current) +{ + if (name == NULL || current == NULL) + return 0; + if (name->len != current->nameSz) + return 0; + return XMEMCMP(name->name, current->name, (size_t)current->nameSz) == 0; +} + /* Search through the list to find if the name is permitted. * name The DNS name to search for * dnsList The list to search through @@ -17656,21 +17671,7 @@ static int PermittedListOk(DNS_entry* name, Base_entry* dnsList, byte nameType) } } else if (nameType == ASN_OTHER_TYPE) { - /* RFC 5280 4.2.1.10: otherName matching is byte-exact - * comparison of the full OtherName encoding. The FPKI/SEP - * path also stores entries that contain only the parsed - * UPN/FASCN value and have oidSum != 0; those are not - * byte-comparable with the OID || [0] EXPLICIT value form - * stored for the constraint, so we explicitly skip them. - * Without that guard, a coincidental length match could - * mis-validate. */ - if ( - #ifdef WOLFSSL_FPKI - name->oidSum == 0 && - #endif - name->len == current->nameSz && - XMEMCMP(name->name, current->name, - (size_t)current->nameSz) == 0) { + if (MatchOtherNameConstraint(name, current)) { match = 1; break; } @@ -17723,14 +17724,7 @@ static int IsInExcludedList(DNS_entry* name, Base_entry* dnsList, byte nameType) } } else if (nameType == ASN_OTHER_TYPE) { - /* See note in PermittedListOk about byte-exact matching. */ - if ( - #ifdef WOLFSSL_FPKI - name->oidSum == 0 && - #endif - name->len == current->nameSz && - XMEMCMP(name->name, current->name, - (size_t)current->nameSz) == 0) { + if (MatchOtherNameConstraint(name, current)) { ret = 1; break; } @@ -17825,15 +17819,12 @@ static int ConfirmNameConstraints(Signer* signer, DecodedCert* cert) name = cert->altNames; break; case ASN_OTHER_TYPE: - /* otherName SAN entries are stored on cert->altNames with - * type ASN_OTHER_TYPE. Match by byte-exact comparison of - * the OtherName encoding (OID || [0] EXPLICIT value). For - * FPKI/SEP builds, altNames may also contain entries that - * hold only the parsed UPN/FASCN value (oidSum != 0); the - * explicit oidSum guard in IsInExcludedList / - * PermittedListOk skips those so a coincidental length - * match cannot mis-validate. */ - name = cert->altNames; + /* otherName SAN entries are stored on cert->altOtherNamesRaw + * (kept separate from altNames so the public altNames view + * is unaffected). Each entry holds the raw OtherName + * encoding (OID || [0] EXPLICIT value) and is byte-matched + * against the issuing CA's subtree. */ + name = cert->altOtherNamesRaw; break; default: return 0; @@ -18192,37 +18183,37 @@ static int DecodeGeneralName(const byte* input, word32* inOutIdx, byte tag, } #endif /* WOLFSSL_RID_ALT_NAME */ #endif /* IGNORE_NAME_CONSTRAINTS */ -#if defined(WOLFSSL_SEP) || defined(WOLFSSL_FPKI) - /* GeneralName choice: otherName */ +#ifndef IGNORE_NAME_CONSTRAINTS + /* GeneralName choice: otherName. + * Store the raw OtherName encoding (OID || [0] EXPLICIT value) on a + * dedicated internal list so ConfirmNameConstraints() can byte-match + * it against the issuing CA's nameConstraints subtree (RFC 5280 + * 4.2.1.10). The raw form is kept separate from cert->altNames so + * the public altNames view (used by OpenSSL-compat APIs) reflects + * exactly what the SAN extension carries. */ else if (tag == (ASN_CONTEXT_SPECIFIC | ASN_CONSTRUCTED | ASN_OTHER_TYPE)) { - /* TODO: test data for code path */ - #ifndef IGNORE_NAME_CONSTRAINTS - /* Store the raw OtherName encoding so ConfirmNameConstraints() can - * byte-match it against the issuing CA's subtree (RFC 5280 - * 4.2.1.10). DecodeOtherName() may also add an entry that holds - * only the parsed UPN/FASCN value with oidSum != 0; the explicit - * oidSum guard in IsInExcludedList()/PermittedListOk() ensures - * those parsed entries are skipped during byte-comparison. */ ret = SetDNSEntry(cert->heap, (const char*)(input + idx), len, - ASN_OTHER_TYPE, &cert->altNames); + ASN_OTHER_TYPE, &cert->altOtherNamesRaw); if (ret != 0) { return ret; } - #endif + #if defined(WOLFSSL_SEP) || defined(WOLFSSL_FPKI) + /* FPKI/SEP also OID-decode the otherName into a separate altNames + * entry that holds the parsed UPN/FASCN value (with oidSum != 0). + * That parsed entry is consumed by wc_GetUUIDFromCert / + * wc_GetFASCNFromCert; ConfirmNameConstraints() does not look at + * it - it iterates altOtherNamesRaw instead. */ ret = DecodeOtherName(cert, input, &idx, len); + #else + idx += (word32)len; + #endif } -#elif !defined(IGNORE_NAME_CONSTRAINTS) - /* GeneralName choice: otherName. - * No OID-specific decoding in this build, but we store the raw - * OtherName encoding (OID || [0] EXPLICIT value) on altNames so - * ConfirmNameConstraints() can byte-match it against the issuing CA's - * nameConstraints subtree (RFC 5280 4.2.1.10). */ +#elif defined(WOLFSSL_SEP) || defined(WOLFSSL_FPKI) + /* No name constraints support in the build, but FPKI/SEP still need + * the parsed otherName entry for wc_GetUUIDFromCert / + * wc_GetFASCNFromCert. */ else if (tag == (ASN_CONTEXT_SPECIFIC | ASN_CONSTRUCTED | ASN_OTHER_TYPE)) { - ret = SetDNSEntry(cert->heap, (const char*)(input + idx), len, - ASN_OTHER_TYPE, &cert->altNames); - if (ret == 0) { - idx += (word32)len; - } + ret = DecodeOtherName(cert, input, &idx, len); } #endif /* GeneralName choice: dNSName, x400Address, ediPartyName */ diff --git a/wolfssl/wolfcrypt/asn.h b/wolfssl/wolfcrypt/asn.h index 3e1028275b..2af93bfd90 100644 --- a/wolfssl/wolfcrypt/asn.h +++ b/wolfssl/wolfcrypt/asn.h @@ -1760,6 +1760,13 @@ struct DecodedCert { #ifndef IGNORE_NAME_CONSTRAINTS DNS_entry* altEmailNames; /* alt names list of RFC822 entries */ DNS_entry* altDirNames; /* alt names list of DIR entries */ + /* Raw OtherName GeneralName encodings (OID || [0] EXPLICIT value) + * for any otherName SAN seen on this certificate. Used internally by + * ConfirmNameConstraints() for byte-exact matching against the + * issuing CA's nameConstraints subtrees (RFC 5280 4.2.1.10). Kept + * separate from altNames so OpenSSL-compat APIs that iterate + * altNames see exactly the entries the SAN extension carries. */ + DNS_entry* altOtherNamesRaw; Base_entry* permittedNames; /* Permitted name bases */ Base_entry* excludedNames; /* Excluded name bases */ #endif /* IGNORE_NAME_CONSTRAINTS */ @@ -2062,11 +2069,22 @@ struct DecodedCert { WC_BITFIELD extSubjAltNameCrit:1; WC_BITFIELD extAuthKeyIdCrit:1; #ifndef IGNORE_NAME_CONSTRAINTS + /*! + * \brief Set when the certificate's nameConstraints extension was + * present and marked critical. + */ WC_BITFIELD extNameConstraintCrit:1; - /* Set when DecodeSubtree encountered a constraint form (e.g. - * registeredID, x400Address, ediPartyName) we cannot enforce. Used - * together with extNameConstraintCrit to implement the RFC 5280 - * 4.2.1.10 fail-closed requirement. */ + /*! + * \brief Set when decoding the nameConstraints extension encountered + * at least one permittedSubtrees or excludedSubtrees entry whose + * GeneralName form (e.g. registeredID, x400Address, + * ediPartyName) wolfSSL does not enforce. + * + * During verification, ConfirmNameConstraints() implements the RFC + * 5280 4.2.1.10 fail-closed requirement: when both this flag and + * extNameConstraintCrit are set, the chain is rejected rather than + * the unsupported constraint form being silently ignored. + */ WC_BITFIELD extNameConstraintHasUnsupported:1; #endif WC_BITFIELD extSubjKeyIdCrit:1; @@ -2136,13 +2154,21 @@ struct Signer { word16 maxPathLen; WC_BITFIELD selfSigned:1; #ifndef IGNORE_NAME_CONSTRAINTS - /* Mirror of DecodedCert::extNameConstraintCrit and - * extNameConstraintHasUnsupported so ConfirmNameConstraints can - * implement the RFC 5280 4.2.1.10 fail-closed requirement when a - * critical nameConstraints extension imposes a constraint form we - * cannot fully enforce. Co-located with selfSigned to share its - * bitfield storage word and avoid growing sizeof(Signer), which is - * load-bearing for PERSIST_CERT_CACHE. */ + /*! + * \brief Mirrors DecodedCert::extNameConstraintCrit and + * DecodedCert::extNameConstraintHasUnsupported so the + * nameConstraints state survives onto the CA Signer and is + * available during chain verification. + * + * ConfirmNameConstraints() uses these flags to implement the RFC 5280 + * 4.2.1.10 fail-closed requirement: when extNameConstraintCrit is set + * and extNameConstraintHasUnsupported is also set, verification fails + * rather than the unsupported constraint form being silently ignored. + * + * Co-located with selfSigned to share its bitfield storage word and + * avoid growing sizeof(Signer), which is load-bearing for + * PERSIST_CERT_CACHE. + */ WC_BITFIELD extNameConstraintCrit:1; WC_BITFIELD extNameConstraintHasUnsupported:1; #endif