743700 - Fix handling of name constraints in root CA certificates

Status: RESOLVED FIXED

(NSS :: CA Certificates Code, task)

Description

[Kathleen Wilson]

When a root certificate includes name constraints, NSS should apply those name constraints.

Note: In bug #711594, the HARICA root certificate was included which has nameConstraints (marked as non critical)

Comment 1

[Gervase Markham [:gerv]]

For bonus points, we should add an additional field to our cert store which allows us to add name constraints to existing roots where the constraints are not in the cert itself.

Gerv

Comment 2

[Kathleen Wilson]

Rather than using name constraints that are included in root certificates, we should have a way to apply our own name constraints to root certificates.

Reasons:
1) The contents of a root cert are basically ignored.
2) We should be able to apply name constraints to a root cert independent of the contents of the root cert.
3) We should be able to apply name constraints to a root cert without having to update the root cert itself.

Comment 3

[Gervase Markham [:gerv]]

bsmith: here's a bug on name constraining roots. Could you look into which of the options we discussed (e.g. hacking the roots in the store / adding special code) is the simplest to implement? That doesn't commit you to implementing it :-)

Gerv

Comment 5

[Camilo Viecco (:cviecco)]

Attachment: fix-nameconstraints

Comment 6

[Camilo Viecco (:cviecco)]

Attachment: name-constraints-root-libpkix

Comment 7

[Camilo Viecco (:cviecco)]

Comment on attachment 8348919 [details] [diff] [review]
name-constraints-root-libpkix

Review of attachment 8348919 [details] [diff] [review]:
-----------------------------------------------------------------

This one is not as clean, There are several issues that I dont like:

1. The change in pkix_build.c is needed because  PKIX_TrustAnchor_CreateWithCert does not fill the nameConstraints field for the anchor. Should I change that one instead?
2. I use a different heuristic for including the CN is a certnamelist in this patch I only add if it it is not a CA in the 'classic' mode the check depends on the usage and the location of the cert in the verification chain (only ee). This is 'better' than the current behavior as the current behavior makes intermediates that are signed by a name constrained entity to usually fail the name check.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8348919 [details] [diff] [review]
name-constraints-root-libpkix

Review of attachment 8348919 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c
@@ +3149,5 @@
>  
>                  arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
>                  if (arena == NULL) {
>                          PKIX_ERROR(PKIX_OUTOFMEMORY);
>                  }

1. This should have a comment explaining why this this condition is used.

2. If includeSubjectCommonName is true, and the certificate contains subjectAltNames, then does CERT_GetConstrainedCertificateNames include the common name or not? AFAICT, it should not, to match the classic code.

Comment 9

[Camilo Viecco (:cviecco)]

Attachment: name-contrain-ANSSI (wip)

This is a work in progress patch (*works for me).

Comment 10

[Camilo Viecco (:cviecco)]

Attachment: name-contrain-ANSSI (wip)

Comment 11

[Camilo Viecco (:cviecco)]

Attachment: name-constraints-root-libpkix (v2)

Comment 12

[Kai Engert (:KaiE:)]

Let's clarify the purpose of this bug.

In my understanding, NSS is assumed to respect name constraints that are already embedded in a root certificate.

That assumption was stated in a past NSS meeting, around the time when we discussed the addition of the HARICA root.

If I understand the patches in this bug correctly, Camilo discovered issues with the current processing of name constraints in certificates, and proposes to fix them.

I'm adjusting the summary of this bug.

In addition to correctly handling name constraints, we also desire a mechanism to technically limit a root CA to a set of arbitrary domains, but a set that isn't included in the certificate. Implementing such an enhancement is the subject of bug 952572.

Comment 13

[Kai Engert (:KaiE:)]

Comment on attachment 8347460 [details] [diff] [review]
fix-nameconstraints

It seems you are moving this block further up.

Can you please explain, why this is necessary?

Which scenario are you fixing?

Comment 14

[Kai Engert (:KaiE:)]

Comment on attachment 8350705 [details] [diff] [review]
name-constraints-root-libpkix (v2)

I'd appreciate explanations what's broken and what you're trying to fix, because the bug doesn't say yet, and one has to guess based on your patch.

What's the state of name constraints checks in libpkix?

Does your comment
  "PKIX_TrustAnchor_GetNameConstraints always returns nil"
imply that it's nonworking as of today?
Or is it mostly working, and you're fixing a specific scenario, only? Which one?

Please fix the comment typos in "constrints stricture".

Your change to skip applying the name constraints to the CN of a CA seems reasonable. (As a general advice, in C, I'd avoid comparing a boolean value explicitly against the numeric true value. Rather, I'd compare it isn't false, or is nonzero. I don't know if other libpkix code also does explicit comparisons == PKIX_TRUE, but personally I'd write 
  "if (basicConstraints && basicConstraints->isCA)"
).

Comment 15

[Kai Engert (:KaiE:)]

I'd like to propose that this work should go together with some first step of solving bug 757854 (adding automated testing to the NSS test suite).

Comment 16

[Camilo Viecco (:cviecco)]

Comment on attachment 8347460 [details] [diff] [review]
fix-nameconstraints

Review of attachment 8347460 [details] [diff] [review]:
-----------------------------------------------------------------

removing request until I address Kai's comments.

Comment 17

[Camilo Viecco (:cviecco)]

Comment on attachment 8347460 [details] [diff] [review]
fix-nameconstraints

Review of attachment 8347460 [details] [diff] [review]:
-----------------------------------------------------------------

This patch just moves the checking of name constraints before the block calling CERT_GetCertTrust. In the CERT_GetCertTrust block, the loop will exit if the cert is a trusted CA. And is the reason why the in the current code, we do not evaluate root (or trusted) certificates.

I moved the call to be before the CRL checks as the check is depends only on local data and we can have a performance gain by not blocking here.

Comment 18

[Camilo Viecco (:cviecco)]

(In reply to Kai Engert (:kaie) from comment #14)
> Comment on attachment 8350705 [details] [diff] [review]
> name-constraints-root-libpkix (v2)
> 
> I'd appreciate explanations what's broken and what you're trying to fix,
> because the bug doesn't say yet, and one has to guess based on your patch.
> 
> What's the state of name constraints checks in libpkix?
It is bad:
1. name constraints are not initialized by PKIX_TrustAnchor_GetNameConstraints, therefore they are not evaluated for root/trusted certs (as with 'classic' verification).
2. The only case where NC work correctly is where the end-entity cert inmediatelly follows the name constrained certifiate. If there is another intermediate between the named constrained intermediate and the end-entity certificate the name constraints (will usually) fail as the CN of the non name constrained intermediate is added to the list of names to evaluate. 

> 
> Does your comment
>   "PKIX_TrustAnchor_GetNameConstraints always returns nil"
> imply that it's nonworking as of today?
Correct, PKIX_TrustAnchor_GetNameConstraints, it is not-working as today.

> Or is it mostly working, and you're fixing a specific scenario, only? Which
> one?

This patch only fixes the name-constraints in the pkix_build section of the code, there is another call to to PKIX_TrustAnchor_GetNameConstraints in pkix_validate.c that I do not modify. (The way is called from PSM only uses the pkix_build path and my tests are limited to this path).



> 
> Please fix the comment typos in "constrints stricture".
> 
> Your change to skip applying the name constraints to the CN of a CA seems
> reasonable. (As a general advice, in C, I'd avoid comparing a boolean value
> explicitly against the numeric true value. Rather, I'd compare it isn't
> false, or is nonzero. I don't know if other libpkix code also does explicit
> comparisons == PKIX_TRUE, but personally I'd write 
>   "if (basicConstraints && basicConstraints->isCA)"
> ).

Thanks for both comments (will address).

Comment 19

[Ryan Sleevi]

(In reply to Camilo Viecco (:cviecco) from comment #18)
> (In reply to Kai Engert (:kaie) from comment #14)
> > Comment on attachment 8350705 [details] [diff] [review]
> > name-constraints-root-libpkix (v2)
> > 
> > I'd appreciate explanations what's broken and what you're trying to fix,
> > because the bug doesn't say yet, and one has to guess based on your patch.
> > 
> > What's the state of name constraints checks in libpkix?
> It is bad:
> 1. name constraints are not initialized by
> PKIX_TrustAnchor_GetNameConstraints, therefore they are not evaluated for
> root/trusted certs (as with 'classic' verification).
> 2. The only case where NC work correctly is where the end-entity cert
> inmediatelly follows the name constrained certifiate. If there is another
> intermediate between the named constrained intermediate and the end-entity
> certificate the name constraints (will usually) fail as the CN of the non
> name constrained intermediate is added to the list of names to evaluate. 
> 

Can you please provide a test chain demonstrating this?

When I investigated name constraints, in all cases I tested, ibpkix did the correct thing. I think perhaps you're looking at a different, unused code path, but the way to prove this is with a test cert chain demonstrating any bugs you believe exist.

> > 
> > Does your comment
> >   "PKIX_TrustAnchor_GetNameConstraints always returns nil"
> > imply that it's nonworking as of today?
> Correct, PKIX_TrustAnchor_GetNameConstraints, it is not-working as today.
> 
> > Or is it mostly working, and you're fixing a specific scenario, only? Which
> > one?
> 
> This patch only fixes the name-constraints in the pkix_build section of the
> code, there is another call to to PKIX_TrustAnchor_GetNameConstraints in
> pkix_validate.c that I do not modify. (The way is called from PSM only uses
> the pkix_build path and my tests are limited to this path).
> 
> 
> 
> > 
> > Please fix the comment typos in "constrints stricture".
> > 
> > Your change to skip applying the name constraints to the CN of a CA seems
> > reasonable. (As a general advice, in C, I'd avoid comparing a boolean value
> > explicitly against the numeric true value. Rather, I'd compare it isn't
> > false, or is nonzero. I don't know if other libpkix code also does explicit
> > comparisons == PKIX_TRUE, but personally I'd write 
> >   "if (basicConstraints && basicConstraints->isCA)"
> > ).
> 
> Thanks for both comments (will address).

Comment 20

[Camilo Viecco (:cviecco)]

Attachment: ca-nc-none.der

Comment 21

[Camilo Viecco (:cviecco)]

Attachment: int-nc-perm-example.com-ca-nc-none.der

Comment 22

[Camilo Viecco (:cviecco)]

Attachment: int-nc-none-int-perm-example.com-ca-nc-none.der

Comment 23

[Camilo Viecco (:cviecco)]

Attachment: cn-www.example.com-int-nc-none-int-perm-example.com-ca-nc-none.der

Comment 24

[Camilo Viecco (:cviecco)]

(In reply to Ryan Sleevi from comment #19)
> (In reply to Camilo Viecco (:cviecco) from comment #18)
> > (In reply to Kai Engert (:kaie) from comment #14)
> > > Comment on attachment 8350705 [details] [diff] [review]
> > > name-constraints-root-libpkix (v2)
> > > 
> > > I'd appreciate explanations what's broken and what you're trying to fix,
> > > because the bug doesn't say yet, and one has to guess based on your patch.
> > > 
> > > What's the state of name constraints checks in libpkix?
> > It is bad:
> > 1. name constraints are not initialized by
> > PKIX_TrustAnchor_GetNameConstraints, therefore they are not evaluated for
> > root/trusted certs (as with 'classic' verification).
> > 2. The only case where NC work correctly is where the end-entity cert
> > inmediatelly follows the name constrained certifiate. If there is another
> > intermediate between the named constrained intermediate and the end-entity
> > certificate the name constraints (will usually) fail as the CN of the non
> > name constrained intermediate is added to the list of names to evaluate. 
> > 
> 
> Can you please provide a test chain demonstrating this?
> 
> When I investigated name constraints, in all cases I tested, ibpkix did the
> correct thing. I think perhaps you're looking at a different, unused code
> path, but the way to prove this is with a test cert chain demonstrating any
> bugs you believe exist.
> 

Use the attached certs:
ca-nc-none.der -> a CA with no name constraints
int-nc-perm-example.com-ca-nc-none.der -> a sub CA (issued by ca-nc-none) with DNS name constraints to example.com.
int-nc-none-int-perm-example.com-ca-nc-none.der -> a sub CA (issued by int-nc-perm-example.com-ca-nc-none) with no name constraints.
cn-www.example.com-int-nc-none-int-perm-example.com-ca-nc-none.der -> a EE cert (issued by int-nc-none-int-perm-example.com-ca-nc-none.der) for cn = www.example.com

Results:
vfychain -u 3 -vv -pp    -r int-nc-none-int-perm-example.com-ca-nc-none.der -r int-nc-perm-example.com-ca-nc-none.der  -t ca-nc-none.der 
Chain is bad!
PROBLEM WITH THE CERT CHAIN:
CERT 2. CN=ca-nc-none [Certificate Authority]:
  ERROR -8080: The Certifying Authority for this certificate is not permitted to issue a certificate with this name.

I since there is no dns name here I would expect that nt-nc-none-int-perm-example.com-ca-nc-none is trusted as an SSL CA.

vfychain -u 1 -vv -pp   -r cn-www.example.com-int-nc-none-int-perm-example.com-ca-nc-none.der -r int-nc-none-int-perm-example.com-ca-nc-none.der -r int-nc-perm-example.com-ca-nc-none.der  -t ca-nc-none.der 
Chain is bad!
PROBLEM WITH THE CERT CHAIN:
CERT 3. CN=ca-nc-none [Certificate Authority]:
  ERROR -8080: The Certifying Authority for this certificate is not permitted to issue a certificate with this name.

The nc should match the cert name. This check should pass.

Comment 25

[Ryan Sleevi]

Likely a problem with following call sequence (from quick source scan):
http://mxr.mozilla.org/nss/source/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c#3130
http://mxr.mozilla.org/nss/source/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c#3157
http://mxr.mozilla.org/nss/source/lib/certdb/genname.c#1063
http://mxr.mozilla.org/nss/source/lib/certdb/genname.c#1106

TL;DR: If a cert (intermediate or leaf) lacks DNS-shaped SANs, we'll count the CN as DNS when applying DNS NCs.

Sounds like a separate bug. Are you actively working on this?

Comment 26

[Camilo Viecco (:cviecco)]

Sure let me file a separate bug for this. But I would call the bug differently: libpkix will count the CN of a ca(or subca) as DNS when applying DNS NC.

Comment 27

[Camilo Viecco (:cviecco)]

Attachment: name-constraints-root-libpkix (v3)

Comment 28

[Camilo Viecco (:cviecco)]

Comment on attachment 8347460 [details] [diff] [review]
fix-nameconstraints

Review of attachment 8347460 [details] [diff] [review]:
-----------------------------------------------------------------

Move evaluation of nc before evaluation of trustworthiness of certificate

Comment 29

[Camilo Viecco (:cviecco)]

Comment on attachment 8368047 [details] [diff] [review]
name-constraints-root-libpkix (v3)

Review of attachment 8368047 [details] [diff] [review]:
-----------------------------------------------------------------

This solve the issue, but I think a better solution would be to chage PKIX_TrustAnchor_CreateWithCert so that the nameconstraint fields (and keys,etc) get populated there. (this would also clean up some other logic).

Comment 30

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8347460 [details] [diff] [review]
fix-nameconstraints

Review of attachment 8347460 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/nss/lib/certhigh/certvfy.c
@@ +508,5 @@
>  	}
>  	
> +        /* make sure that the entire chain is within the name space of the
> +        ** current issuer certificate.
> +        */

Nit: fix the comment style to be:

   /*
    *
    */

like the comment below.

Comment 31

[Ryan Sleevi]

Comment on attachment 8368047 [details] [diff] [review]
name-constraints-root-libpkix (v3)

Review of attachment 8368047 [details] [diff] [review]:
-----------------------------------------------------------------

Why not change PKIX_TrustAnchor_CreateWithCert to populate the nameConstraints with the attached constraints to the cert?

You can make the same call to PKIX_PL_Cert_GetNameConstraints, just do it within the trust anchor.

Comment 32

[Camilo Viecco (:cviecco)]

Attachment: name-constraints-root-libpkix-alt

Comment 33

[Camilo Viecco (:cviecco)]

Attachment: tests-libpkix-root-nc

Comment 34

[Camilo Viecco (:cviecco)]

Attachment: name-constraints-root-libpkix-alt (v1.1)

Comment 35

[Camilo Viecco (:cviecco)]

Attachment: tests-libpkix-root-nc (v1.1)

Comment 36

[Camilo Viecco (:cviecco)]

Comment on attachment 8369736 [details] [diff] [review]
name-constraints-root-libpkix-alt (v1.1)

Review of attachment 8369736 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/libpkix/pkix/params/pkix_trustanchor.c
@@ +372,5 @@
> +
> +        PKIX_CHECK(PKIX_PL_Cert_GetNameConstraints
> +                    (anchor->trustedCert, &anchor->nameConstraints, plContext),
> +                    PKIX_CERTGETNAMECONSTRAINTSFAILED);
> +

Yes I have and extra new line here.

Comment 37

[Ryan Sleevi]

Comment on attachment 8369739 [details] [diff] [review]
tests-libpkix-root-nc (v1.1)

Review of attachment 8369739 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/chains/scenarios/nameconstraints.cfg
@@ +125,5 @@
>    cert NameConstraints.intermediate5:x
>    cert NameConstraints.intermediate3:x
>    result fail
>  
> +# Intermediate 6: Subject: "C=US, ST=CA, O=OtherOrg, CN=NSS Intermediate CA3"

Update description? Isn't this 6 now?

::: tests/libpkix/certs/make-nc
@@ +359,5 @@
> +9
> +n
> +CERTSCRIPT
> +
> +certutil -S -z noise -g 1024 -d . -n ica6 -s "CN=NSS Intermediate CA3,O=OtherOrg,ST=CA,C=US" -t ,, -c ncca -m 63 -w -2 -v 120 -1 -2 -5 <<CERTSCRIPT

Also here

Comment 38

[Ryan Sleevi]

Comment on attachment 8369739 [details] [diff] [review]
tests-libpkix-root-nc (v1.1)

Review of attachment 8369739 [details] [diff] [review]:
-----------------------------------------------------------------

Setting r- for the nits, but otherwise quite happy to see more stuff land.

Comment 39

[Camilo Viecco (:cviecco)]

Attachment: tests-libpkix-root-nc (v1.2)

Comment 40

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Is this ready to check in now?

Comment 41

[Camilo Viecco (:cviecco)]

Attachment: evaluate-ns-in-roots-classic (v 1.1)

Keeping r+ from bsmith. Now with the nits addressed is ready for checkin

Comment 42

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

All three patches got folded together into one commit, and were otherwise unchanged:
http://hg.mozilla.org/projects/nss/rev/b57dd6c60b5a