Repository navigation
Conversation
|
|
Fix confirmed by customer |
|
retest this please |
|
@claude review |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved DNS and mailbox validation gaps allow supported single-label hostnames and malformed email names to bypass name constraints, including a critical CN-fallback path that can authenticate localhost outside a CA’s permitted scope.
2 open issues
Review effort: Balanced
| return 0; | ||
| } | ||
|
|
||
| return hasDot; |
| if (!Rfc822LocalPartOk(name, atPos)) { | ||
| WOLFSSL_MSG("rfc822Name has an unquoted '@' in its local-part"); | ||
| return ASN_PARSE_E; |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (c == '.' && cn[i - 1] != '-' && cn[i - 1] != '.' && | ||
| cn[i + 1] != '.') { | ||
| hasDot = 1; | ||
| continue; | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| return hasDot; |
There was a problem hiding this comment.
🔴 Leaves under a DNS-constrained CA can now get past permitted/excluded dNSName subtrees with a dotless subject CN such as "localhost" or "intranet". CnIsDnsName returns hasDot, so any single-label CN skips the CN fallback in ConfirmNameConstraints. CheckHostName still matches that CN against the peer hostname. Fix: apply the fallback to every CN that MatchDomainName could match as a host name, including dotless LDH labels and a lone '*', and skip only CNs with non-host-name characters such as spaces.
Why this was flagged
A CA has nameConstraints permitted DNS:example.com or excluded DNS: (empty). It issues a leaf with no dNSName SAN and CN=intranet. ConfirmNameConstraints (asn.c:20855) now calls CnIsDnsName, which returns 0 because the CN has no '.' (asn.c:20816), so the CN is never checked against the DNS subtrees and the chain verifies. During TLS, CheckHostName (src/internal.c:15425-15427) falls back to MatchDomainName on subjectCN and accepts hostname "intranet" or "localhost". On the base branch the CN was checked for every non-CA leaf without a dNSName SAN and was rejected as outside example.com. The PR's goal of skipping person-name CNs like "John Doe" is met by the character check alone. The at-least-one-dot rule, copied from OpenSSL cn2dnsid, also exempts real single-label host names, which reopens a constraint bypass this change set out to close.
Verification: The new CnIsDnsName (wolfcrypt/src/asn.c, around lines 20781-20817) ends with return hasDot;. A dotless label-only CN therefore returns 0. The DNS constraint is never applied and the chain verifies. On the base branch the gate was only cert->subjectCN != NULL && !cert->isCA. So this is a regression.
| if ((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || | ||
| (c >= '0' && c <= '9') || c == '_' || c == '*') { | ||
| continue; | ||
| } | ||
| /* '-' and '.' may not be first or last. */ | ||
| if (i == 0 || i == cnLen - 1) { | ||
| return 0; | ||
| } | ||
| if (c == '-') { | ||
| continue; | ||
| } | ||
| /* A '.' may not follow '-' or '.', nor precede '.'. */ | ||
| if (c == '.' && cn[i - 1] != '-' && cn[i - 1] != '.' && | ||
| cn[i + 1] != '.') { | ||
| hasDot = 1; | ||
| continue; | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
🔴 Name-constrained sub-CAs can now issue no-SAN leaves whose CN is an IPv6 literal, and clients verifying that IP accept them. CnIsDnsName rejects ':', so ConfirmNameConstraints skips the CN fallback. Yet MatchDomainName (src/internal.c:15149) still matches an IPv6 CN through MatchIPv6. Fix: keep every CN the hostname matcher can match under constraints, e.g. treat a CN that parses as an IP address as constrained (reject it when DNS or IP subtrees exist). This differs from the dotless-CN finding: dropping the dot requirement still leaves ':' rejected.
Why this was flagged
Trigger: a CA with nameConstraints permitted DNS:example.com (optionally plus permitted iPAddress subtrees) issues a non-CA leaf with no SAN and subject CN=2001:db8::1. CnIsDnsName (asn.c:20796-20813) returns 0 for the ':' bytes, so the condition at asn.c:20855-20856 skips the CN fallback. No DNS constraint is applied, and iPAddress constraints are never applied to the CN. On the base branch the CN was checked against permitted DNS:example.com and the chain was rejected. After this change ConfirmNameConstraints returns 1. In ProcessPeerCerts (src/internal.c:20494-20500), wolfSSL_check_domain_name("2001:db8::1") with no altNames falls back to MatchDomainName on the CN. MatchDomainName calls MatchIPv6 first (src/internal.c:15147-15150, built with WOLFSSL_IP_ALT_NAME, which OPENSSL_ALL requires), so the handshake succeeds for an IP the CA was never allowed to certify. The isIP skip in CheckHostName (internal.c:15425) does not cover this path.
Verification: CnIsDnsName (asn.c:20781-20816) only accepts letters, digits, '_', '*', '-' and '.'. So the new condition at asn.c:20855-20856 leaves subjectDnsName empty. On the base branch the CN was checked against permitted DNS:example.com and the chain was rejected. MatchDomainName first tries MatchIPv6 (internal.c:15147-15150).
| return 0; | ||
| } |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Pre-existing, security-relevant: a sub-CA under excluded: email:evil.com can still issue an accepted cert for the mailbox ".x@ evil.com". The early name[0] == '.' return at asn.c:19965 runs before the RFC822 split, so a leading-dot local-part reports "no match", and an excluded subtree treats that as outside. Fix: apply the leading-dot rejection only to DNS names, and for RFC822 compare the host-part after the last '@' whatever the local-part starts with (as OpenSSL's nc_email does), in both the native and compat paths. The PR closes the quoted-'@' escape but leaves this sibling escape.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
An intermediate has nameConstraints excluded: email:evil.com. It issues a leaf with SAN rfc822Name .x@ evil.com, or subject emailAddress .x@ evil.com. SetDNSEntry (asn.c:21258) stores the name unvalidated. IsInExcludedList (asn.c:20733) calls wolfssl_local_MatchBaseName(ASN_RFC822_TYPE, ...). That function returns 0 at asn.c:19965-19966 before the '@' split at asn.c:20022 and before the new Rfc822LocalPartOk check, so mRet is 0, not ASN_PARSE_E. ConfirmNameConstraints then accepts the leaf. The OpenSSL-compat wolfSSL_NAME_CONSTRAINTS_check_name hits the same function through MatchNameConstraint (src/x509.c:6107) and also returns 1. The base branch behaves the same way, so this is pre-existing. It is the same "malformed mailbox counts as no match, so it escapes the exclusion" pattern the PR fixes for "x@ y"@ evil.com, and the new negative-result mechanism does not reach it because the early return comes first. A permitted subtree fails closed for the same input, so only excluded subtrees are bypassed.
Verification: Trigger: a CA has an excluded rfc822Name subtree such as email:evil.com and issues a leaf whose rfc822Name starts with '.', for example ".x@ evil.com". In wolfcrypt/src/asn.c, the check at 19965-19967 (if (name[0] == '.') { return 0; }) runs before the last-'@' split. In IsInExcludedList 0 leaves ret = 0. The base already has the same line.
| /* An empty dNSName base covers all DNS names. Report a match so that an | ||
| * excluded empty subtree excludes everything. */ | ||
| if (type == ASN_DNS_TYPE && DnsBaseIsEmpty(base, baseSz)) { | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: In --enable-asn=original builds, a CA whose nameConstraints hold a zero-length dNSName still fails to parse. The new empty-base handling is unreachable there, and that CA's S/MIME and client-auth chains stay rejected. asn_orig.c:4107 treats GetLength() returning 0 as "invalid length" and fails the whole extension with ASN_PARSE_E. Fix: in DecodeSubtree (asn_orig.c), accept a zero length (test < 0) so the empty entry is stored as the template decoder already does at asn.c:22651-22661.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
In non-template builds (WOLFSSL_ASN_TEMPLATE off, asn_orig.c included at asn.c:41941), DecodeSubtree reads each GeneralName length with if (GetLength(input, &nameIdx, &strLength, sz) <= 0) at asn_orig.c:4107. GetLength_ex returns the parsed length itself (asn.c:2810), which is 0 for the CA/Browser Forum excluded: DNS: encoding. DecodeSubtree returns ASN_PARSE_E, DecodeNameConstraints fails (asn_orig.c:4204-4209), and DecodeExtensionType sets ret = ASN_PARSE_E (asn.c:23649-23651), so the CA certificate does not load. The empty-base code this PR adds in wolfssl_local_MatchBaseName (asn.c:19969-19973) and wolfssl_local_MatchDnsConstraintWildcard (asn.c:20408-20412) never runs in this build. The CnIsDnsName change, meant to keep S/MIME and client-auth leaves under such a CA valid, has no effect either. This fails closed, which is the same as on the base branch. Template builds store the zero-length entry (asn.c:22651-22661) and behave as the PR describes.
Verification: pre-existing. In wolfcrypt/src/asn_orig.c, DecodeSubtree has if (GetLength(input, &nameIdx, &strLength, sz) <= 0) { WOLFSSL_MSG("\tinvalid length"); return ASN_PARSE_E; }. GetLength_ex ends with return (int)length;, so a zero-length GeneralName gives 0 and the subtree decode fails. The diff does not touch asn_orig.c. The result is fail-closed: the whole CA is rejected, not bypassed.
Description
Two inputs let a certificate escape an excluded name constraint subtree:
excluded: DNS:(zero-length), the CA/Browser Forum encoding for a sub-CA that may not issue DNS names, was treated as matching nothing. Per RFC 5280 4.2.1.10 it now matches every DNS name, in the native and OpenSSL compat paths. A lone"."base is treated the same way unlessOPENSSL_COMPATIBLE_DEFAULTSisdefined.
"x@y"@evil.comescapedexcluded: email:evil.combecause two '@' characters made the name "invalid". The mailbox is now split at its last '@'. An '@' outside a quoted local-part makes the name uncomparable, and the certificate is rejected on both permitted and excluded subtrees.The subject CN fallback for DNS constraints now applies only to CNs with host name syntax, so an empty excluded dNSName does not reject S/MIME or client-auth leaves whose CN is a person's name.
Fixes zd22724
Testing
tests/api/test_asn.c,tests/api/test_ossl_x509_ext.candtests/unit-mcdc/test_asn_ext_whitebox.c.asnandossl_x509_extgroups and the white-box smoke run pass with--enable-alland with--disable-openssl-compatible-defaults.Checklist