1571677 - Name Constraints validation: CN treated as DNS name even when syntactically invalid as DNS name

Status: ASSIGNED

(NSS :: Libraries, defect, P2)

Description

[Fraser Tweedale]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

Client certificate verification, where client cert has "CN=IPA RA,O=IPA.LOCAL", EKU with id-kp-serverAuth and id-kp-clientAuth, and no SAN. Issuer has DNS Name Constraint permittedSubtree of "ipa.local".

Actual results:

Certificate verification fails due to name constraint violation.

Expected results:

This bug is on similar lines to https://bugzilla.mozilla.org/show_bug.cgi?id=1523484, but a different issue. The CN is syntactically not a valid DNS name. Therefore the "treat CN as a DNS name" heuristic should not apply in this case. Certificate verification should succeed.

Comment 1

[Fraser Tweedale]

Attachment: nss-ftweedal-0000-Only-treat-CN-as-DNS-name-if-it-looks-like-one.patch

Patch attached. This is probably wrong / incomplete and of course there are no tests yet.
Also I believe both libpkix and the "legacy" validation routine are affected, and I don't think
this patch fixes both.

So this needs the attention of an NSS dev, but I attach the patch in the hope that it will be useful
(particularly the helper function).

Comment 2

[Ryan Sleevi]

Thanks. It is desirable that this function not perform the syntax checks you propose, because the library will accept these as reference name inputs. For example, permitting underscores is just one example that the name resolution libraries support.

Comment 3

[Hubert Kario]

Actually, from my testing it looks like there is inconsistency between libpkix and legacy verifier as far as Name Constraints are concerned.
The legacy verifier does ignore CN of client certificates but the libkpkix verifier doesn't.

Reproducer:

# get a helper library
git clone https://github.com/redhat-qe-security/certgen.git
. certgen/certgen/lib.sh
# generate the certificates
x509KeyGen ca
x509KeyGen server
x509KeyGen client
x509SelfSign --ncExclude '.com' --ncPermit localhost ca

x509CertSign --CA ca server
x509CertSign --CA ca -t webclient \
--extendedKeyUsage serverAuth,clientAuth,emailProtection client
# print the generated certificates
x509DumpCert ca
x509DumpCert server
x509DumpCert client

# set up the NSS databases for client and server
mkdir nssdb-srv
mkdir nssdb-clnt
certutil -N -d nssdb-srv/ --empty-password
certutil -N -d nssdb-clnt --empty-password
certutil -A -n ca -i $(x509Cert ca) -t 'cTC,cTC,' -a -d nssdb-srv
certutil -A -n ca -i $(x509Cert ca) -t 'cTC,cTC,' -a -d nssdb-clnt
pk12util -d nssdb-srv -i $(x509Key --with-cert --pkcs12 server) -W ''
pk12util -d nssdb-clnt -i $(x509Key --with-cert --pkcs12 client) -W ''

# start server that requires client certificates
selfserv -d nssdb-srv -p 4433 -V tls1.2: \
-H 1 -n server -rr -v

#!! run in separate console, same directory
# client that will trust the server cert and has a certificate trusted by 
# server:
tstclnt -d nssdb-clnt -h localhost -p 4433 -n client <<<"GET / HTTP/1.0

"

The connection is successful with selfserv env having NSS_ENABLE_PKIX_VERIFY undefined:

selfserv: About to call accept.
selfserv: Subject: CN=John Smith
selfserv: Issuer : O=Example CA
selfserv: -- SSL3: Certificate Validated.
selfserv: 0 cache hits; 2 cache misses, 0 cache not reusable
          0 stateless resumes, 0 ticket parse failures
selfserv: SSL version 3.4 using 128-bit AES-GCM with 128-bit AEAD MAC
selfserv: Server Auth: 2048-bit TLS 1.3, Key Exchange: 255-bit TLS 1.3
          Compression: NULL, Extended Master Secret: Yes
selfserv: subject DN: CN=John Smith
selfserv: issuer  DN: O=Example CA

and is aborted with NSS_ENABLE_PKIX_VERIFY set to "1":

selfserv: About to call accept.
selfserv: Subject: CN=John Smith
selfserv: Issuer : O=Example CA
selfserv: -- SSL3: Certificate Invalid, err -8080.
The Certifying Authority for this certificate is not permitted to issue a certificate with this name.
selfserv: HDX PR_Read returned error -8080:
The Certifying Authority for this certificate is not permitted to issue a certificate with this name.

Comment 4

[J.C. Jones [:jcj] (he/him)]

Ryan, can I assign this to you?

Comment 5

[Ryan Sleevi]

I'm opinionated in not breaking it for id-kp-serverAuth (at least not until we're off libpkix), since it was important to address https://nameconstraints.bettertls.com/

While the fix was "obviously" wrong, libpkix is already supposed to have the 'right' logic, by way of CERT_GetConstrainedCertificateNames passing includeSubjectCommonName as part of the constraint check, which libpkix passes through from PKIX_PL_Cert_CheckNameConstraints, and which it computes based on whether the terminal cert being verified contains id-kp-serverAuth.

This is the correct behaviour; when considering whether a name constraint is violated, one doesn't do this by trying to determine the set-intersection of the given chain. Instead, if any subordinate certificate violates the parent certificate constraint, it's an invalid chain, and doesn't even get to name validation.

libpkix is ensuring that, if the cert is capable of being used as a server cert, it's actually compatible with that constraint. In this case, the absence of the SANs, and the presence of id-kp-serverAuth, it does the check.

The specific logic is https://dxr.mozilla.org/mozilla-central/rev/8ac0a35be0d35f9f5aba38ed841cb3d03a2ae3c8/security/nss/lib/libpkix/pkix/checker/pkix_nameconstraintschecker.c#191

If you wanted to change that, so it only enforces that check if verifying as a server cert (which I think is both less consistent with how other nameConstraints are processed and more of a security risk), changing how pkix_NameConstraintsChecker_Check is invoked, so that it can know the client's desired use case, would be how to do it. I think that would be a security issue, though :)

Comment 6

[Robert Relyea]

Attachment: Patch to implement Hubert's suggestion.

This patch make libpkix and the internal validator have the same semantics.

We still need to decide which direction is best, but at least we have potential patches for both of the expected directions.

Comment 7

[Fraser Tweedale]

Let me clarify my earlier comment about both verifiers being affected (I was incorrect).
I was using the vfychain program to perform verification. An NSSDB contains the constrained
CA cert with trust 'CT,C,C', then I execute:

/usr/lib64/nss/unsupported-tools/vfychain -d . -a ipara.pem

With or without NSS_ENABLE_PKIX_VERIFY, verification failed due to name constraints. But I was not
supplying '-u 0' to indicate client usage. Adding that flag, I observe that the legacy verifier succeeds
and libpkix verifier fails.

I am happy with rrelyea's approach if that is preferred by NSS devs.

Comment 8

[Hubert Kario]

Reading https://tools.ietf.org/html/rfc6125#section-6.4.4 now, it looks like the verification library really should check if the CN is a DNS name before applying Name Constraints on it:

Therefore, if and only if the presented identifiers do not include a
DNS-ID, SRV-ID, URI-ID, or any application-specific identifier types
supported by the client, then the client MAY as a last resort check
for a string whose form matches that of a fully qualified DNS domain
name in a Common Name field of the subject field (i.e., a CN-ID).

now, given the importance of this code, what's needed to fix the specific issue at hand, how long we lived with the buggy code and fragility of DNS name detection, I think it would be safer to check for few characters that cannot be in DNS names (a space alone should catch most of the problematic certs) and not test CN for Name Constraints in such case, than to verify if the CN is properly formatted DNS name before applying the Name Constraints to it (fail safe vs fail open)

Comment 9

[Ryan Sleevi]

a space alone should catch most of the problematic certs.

I appreciate this perspective, but I don't believe it's supported by the 'real world' evidence. I'm not arguing that spaces should be valid in domain identifiers - but they are happily accepted by the name resolution libraries. This is why Chrome and Firefox have been rather aggressive in trying to better enforce modern, sensible controls around invalid names. Functionally, the proposal in Comment #8 is not safe nor secure, because it disconnects the name validation part (of the commonName) from the nameConstraint validation. Going that approach could be made more secure, by parsing/validating that the CN is in a form you can canonicalize as a hostname in the same way you'd do so when doing a reference name / presented name check, but that'd be a much more invasive change.

The approach in Comment #6 is what I was describing in Comment #5. It's a weaker guarantee of constraints, and not consistent with how nameConstraints are applied, but it seems unlikely that NSS devs will say "This is unsupported" / "Remove the serverAuth EKU" (either of which would resolve this). An allow-list of usages allowed to skip the check, vs the deny-list implemented, is likely more secure if future methods are introduced, like IPSec was, and is certainly our preference to avoid breaking things or introducing more security issues for downstream consumers like Chrome :)

Comment 10

[Hubert Kario]

but they are happily accepted by the name resolution libraries.

and in what scenario would Firefox actually try to resolve a hostname with a space in it?

Comment 11

[Ryan Sleevi]

in what scenario would Firefox actually try to resolve a hostname with a space in it?

To be clear, we're not just talking about Firefox, but all NSS using applications.

Both Chrome and Firefox have implemented additional checks on top of the name resolution libraries precisely because they'll be happily passed to name resolution libraries and on-to-the-wire. It's not safe nor secure to assume the preferred name syntax in a library like NSS.

There have literally been browser security vulnerabilities related to spaces in host names being passed down to name resolution libraries, because it's a perfectly valid domain name, even though not in preferred name syntax. You can see an example bug at https://crbug.com/695474 .

Many bits have been spilled debating whether it should be that way, and I realize this may be a new conversation, but my hope is that by trying to give productive suggestions about less-problematic approaches, in Comment #9, we can end up with something that doesn't rely on unsound assumptions :)

Comment 12

[Robert Relyea]

So I've of two minds on this.

The patch I created make the libpkix verifier and the nss legacy verifier function the same way. I could go either way on the 'deny' vs 'allow' usage list. The way it's coded now is simply because there are less usages that trigger the CN check than there are usages that don't.

I'm not a fan of heuristic's checks because there is always corner cases and either someone wants to you handle the corner case that looks safe, or you miss the corner case that opens you up for attack. That being said, there was a case where you have a dual use certificate (email and ipsec). The CN probably has the users name. His DNS name and email are in the SAN. My patch doesn't handle this case, but the hueristic does. It also is easier to justify from a SPEC perspective (you consistently create and validate the chain independent of usage, uses the CN exception Hubert pointed out).

On balance I'm clearly OK with the patch I suggested, or with the same patch, but the list of usages that don't care about CN verses the list that does. It sounds like fraser is onboard with that solution, Ryan can tolerate it;).

1. Anyone else want to argue strongly for the heuristic approach?
2. Did my second paragraph make anyone change their mind on this?
3. Do we need any other stakeholders in this conversation?

bob

Comment 13

[Hubert Kario]

(In reply to Ryan Sleevi from comment #11)

in what scenario would Firefox actually try to resolve a hostname with a space in it?
You can see an example bug at https://crbug.com/695474 .

I'm getting "Permission denied" on that bug. Are you sure it's public?

Comment 14

[Ryan Sleevi]

Fixed 😎

Comment 15

[Robert Relyea]

Attachment: Bug 1571677 Name Constraints validation: CN treated as DNS name even when syntactically invalid as DNS name

This patch makes libpkix treat name contraints the same the NSS cert verifier.
This proposal available for review for 9 months without objection.

Time to make this official