Closed
Bug 962760
Opened 10 years ago
Closed 10 years ago
libpkix includes the cn of CA as dns names when evaluating name constraints
Categories
(NSS :: Libraries, defect, P1)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.16
People
(Reporter: cviecco, Assigned: cviecco)
References
Details
Attachments
(2 files, 8 obsolete files)
28.02 KB,
patch
|
cviecco
:
review+
briansmith
:
checked-in+
|
Details | Diff | Splinter Review |
6.68 KB,
patch
|
cviecco
:
review+
wtc
:
review+
briansmith
:
checked-in+
|
Details | Diff | Splinter Review |
When evaluating NC libpkix always includes the CN as a dns name. Therefore when you have a name constrained subCA that issues another subCA the nameconstraints check produces a failure.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → cviecco
Assignee | ||
Comment 1•10 years ago
|
||
Comment 2•10 years ago
|
||
Comment on attachment 8363934 [details] [diff] [review] fix-bug-962760 Review of attachment 8363934 [details] [diff] [review]: ----------------------------------------------------------------- Using isCA isn't sufficient, since it's legitimate for a CA cert to be used as an end-entity/server certificate. You really want to have the reverse name checker (the one that starts at the root and builds to the EE cert) pass along whether or not remaining certs == 0. http://mxr.mozilla.org/nss/source/lib/libpkix/pkix/checker/pkix_nameconstraintschecker.c#154 is the entry point for that pass (state->certsRemaining) For the forward building case - which is an optimization strategy - http://mxr.mozilla.org/nss/source/lib/libpkix/pkix/certsel/pkix_certselector.c#429 - you can simply *always* disable the CN check.
Attachment #8363934 -
Flags: review-
Assignee | ||
Comment 3•10 years ago
|
||
thanks for the review. So it is ok to modify the signature of PKIX_PL_Cert_GetNameConstraints of you prefer a new funcion?
Comment 4•10 years ago
|
||
It's fine to modify any of the PKIX_PL functions - they're all internal to NSS (that is, the libpkix API is not publicly exposed, beyond the basic cert_pi* bits)
Assignee | ||
Comment 5•10 years ago
|
||
This patch, suggested by rsleevi prevents intermediates from, being evaluated for being a CN. This solves the case for SSL server.
Assignee | ||
Updated•10 years ago
|
Attachment #8364695 -
Flags: review?(ryan.sleevi)
Comment 6•10 years ago
|
||
Comment on attachment 8364695 [details] [diff] [review] fix-bug-962760-b Review of attachment 8364695 [details] [diff] [review]: ----------------------------------------------------------------- Can you add a unit test - eg: to chains.sh - that demonstrates failure before and success afterwards? ::: security/nss/lib/libpkix/include/pkix_pl_pki.h @@ +1270,5 @@ > * "nameConstraints" > * Address of CertNameConstraints that need to be satisfied. > + * "includeSubjectCommonName" > + * Whether to include or not the subject common name for the name > + * constraints evaluation. PKIX_TRUE if the subject common name should be considered a dNSName when evaluating name constraints. @@ +1284,5 @@ > PKIX_Error * > PKIX_PL_Cert_CheckNameConstraints( > PKIX_PL_Cert *cert, > PKIX_PL_CertNameConstraints *nameConstraints, > + PKIX_Boolean includeSubjectCommonName, treatCommonNameAsDNS ? The reason is that the subject CN should always be subject to directoryName constraints, it's the dNSName constraint thats special here. ::: security/nss/lib/libpkix/pkix/certsel/pkix_certselector.c @@ +425,5 @@ > PKIX_COMCERTSELPARAMSGETNAMECONSTRAINTSFAILED); > > if (nameConstraints != NULL) { > > PKIX_CHECK(PKIX_PL_Cert_CheckNameConstraints Let's include a comment here explaining why. /* As only the end-entity certificate should have * the common name constrained as if it was a dNSName, * do not constrain the common name when building a * forward path. */
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8363934 -
Attachment is obsolete: true
Attachment #8364695 -
Attachment is obsolete: true
Attachment #8364695 -
Flags: review?(ryan.sleevi)
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8366317 [details] [diff] [review] add-tests-962760 Review of attachment 8366317 [details] [diff] [review]: ----------------------------------------------------------------- Added this set of tests. The test with server6 fails with the current code, passes with the fix.
Attachment #8366317 -
Flags: review?(ryan.sleevi)
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8366316 [details] [diff] [review] fix-bug-962760 Review of attachment 8366316 [details] [diff] [review]: ----------------------------------------------------------------- Comments addressed (now patch in nss name-space)
Attachment #8366316 -
Flags: review?(ryan.sleevi)
Comment 11•10 years ago
|
||
Comment on attachment 8366317 [details] [diff] [review] add-tests-962760 Review of attachment 8366317 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/chains/scenarios/nameconstraints.cfg @@ +19,5 @@ > verify NameConstraints.server3:x > cert NameConstraints.intermediate:x > result pass > > +verify NameConstraints.server4:x Can you add basic comments for what each of these tests does, similar to http://mxr.mozilla.org/nss/source/tests/chains/scenarios/trustanchors.cfg ? Also, I don't think these tests fully cover the code under test. I realize this is messy, given the permutations, but I want to make sure we don't regress on existing constraints (eg: directoryNames) with this CL, so I'm hoping you can add more tests. That is, a case where CA - no NC Int1 - NC to a DirectoryName of "C=Foo, ST=Bar, O=Baz" and dNSName constraint of "foo.example" Int2 - Subject of "C=Foo, ST=Bar, O=Baz, OU=Bat", no NC present, signed by Int 1 [All signed by Int 2] Server1 - Subject of "C=Foo, ST=Bar, O=Baz, OU=Bat, CN=bat.foo.example", no SAN Works Server 2 - Subject of "C=Foo, ST=Bar, O=Baz, CN=bat.foo.example", no SAN Works Server 3 - Subject of "C=Foo, O=Baz, OU=Bat, CN=bat.foo.example", no SAN Fail [missing ST in subject = violates NC] Server 4 - Subject of "C=Foo, ST=Bar, O=Baz, OU=Bat, CN=bar.example", no SAN Fail [CN doesn't NC] Server 5 - Subject of "C=Foo, ST=Bar, O=Baz, CN=site.example", SAN of "foo.example" Works [SAN present = ignores CN constraint violation] Server 6 - Subject of "C=Foo, ST=Bar, O=Baz, CN=Honest Achmed", no SAN Fails [CN doesn't NC, even though it's not DNS-shaped] Int 3 - Subject of "C=Foo, ST=Bar, O=Dummy", signed by Int 1 [All signed by Int 3] Server 7 - Subject of "C=Foo, ST=Bar, O=Dummy, CN=bat.foo.example", no SAN Fails [Int 3 violates NC, as does EE] Server 8 - Subject of "C=Foo, ST=Bar, O=Baz, CN=bat.foo.example", no SAN Fails [Int 3 violates NC, even though EE doesn't] As you can see, I'm trying to ensure that the permutations possible here are covered and non-regressing.
Comment 12•10 years ago
|
||
Comment on attachment 8366316 [details] [diff] [review] fix-bug-962760 Review of attachment 8366316 [details] [diff] [review]: ----------------------------------------------------------------- r=ryan.sleevi (mod test comments)
Attachment #8366316 -
Flags: review?(ryan.sleevi) → review+
Comment 13•10 years ago
|
||
Comment on attachment 8366317 [details] [diff] [review] add-tests-962760 Review of attachment 8366317 [details] [diff] [review]: ----------------------------------------------------------------- r- due to request for even more tests, though I think the existing tests are good.
Attachment #8366317 -
Flags: review?(ryan.sleevi) → review-
Comment 14•10 years ago
|
||
Comment on attachment 8366316 [details] [diff] [review] fix-bug-962760 Review of attachment 8366316 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. ::: lib/libpkix/include/pkix_pl_pki.h @@ +1268,5 @@ > * Address of Cert whose subject names are to be checked. > * Must be non-NULL. > * "nameConstraints" > * Address of CertNameConstraints that need to be satisfied. > + * "treatCommonNameAsDNS" Nit: I think we can name this argument "includeSubjectCommonName", which is the name of the corresponding argument to CERT_GetConstrainedCertificateNames. Or name it "treatCommonNameAsDNSName". Please make the same change to the function definition in pkix_pl_cert.c. ::: lib/libpkix/pkix/certsel/pkix_certselector.c @@ +428,5 @@ > + /* As only the end-entity certificate should have > + * the common name constrained as if it was a dNSName, > + * do not constrain the common name when building a > + * forward path. > + */ Nit: I think it's more precise to be verbose and say "subject common name" because the Issuer field also has a common name. Nit: the comment block should be aligned with the "PKIX_CHECK" on the next line. (This may be an artifact of the code review tool.) ::: lib/libpkix/pkix/checker/pkix_nameconstraintschecker.c @@ +192,4 @@ > PKIX_CERTCHECKNAMECONSTRAINTSFAILED); > } > > if (state->certsRemaining != 0) { Maybe we should save the result of state->certsRemaining == 0 in a local variable, because this expression (including the negation) is used three times. ::: lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c @@ +3151,5 @@ > if (arena == NULL) { > PKIX_ERROR(PKIX_OUTOFMEMORY); > } > > /* This NSS call returns both Subject and Subject Alt Names */ This comment needs to be updated because of the change.
Attachment #8366316 -
Flags: review+
Assignee | ||
Comment 15•10 years ago
|
||
Added the new tests and commented the already existing.
Attachment #8366317 -
Attachment is obsolete: true
Attachment #8369553 -
Flags: review?(ryan.sleevi)
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8369553 [details] [diff] [review] add-tests-962760 (v2) Review of attachment 8369553 [details] [diff] [review]: ----------------------------------------------------------------- remove request as something I missed on the folding of patches.
Attachment #8369553 -
Flags: review?(ryan.sleevi)
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8369553 [details] [diff] [review] add-tests-962760 (v2) Review of attachment 8369553 [details] [diff] [review]: ----------------------------------------------------------------- actuallt there was no issue. This is the test patch
Attachment #8369553 -
Flags: review?(ryan.sleevi)
Comment 18•10 years ago
|
||
Comment on attachment 8369553 [details] [diff] [review] add-tests-962760 (v2) Review of attachment 8369553 [details] [diff] [review]: ----------------------------------------------------------------- Test wise, this is r+, but the comments show a wild degree of inconsistency internally and with spelling. Do you mind taking a pass through again and make sure they all read well? :) I've highlighted a few of the examples ::: tests/chains/scenarios/nameconstraints.cfg @@ +7,5 @@ > db trustanchors > > import NameConstraints.ca:x:CT,C,C > > +#int1 - NC to dNSName constraint of ".example" s/intN/Intermediate N/g (throughout) Inconsistent spacing after "-" (two spaces, elsewhere you use one or a colon) @@ +10,5 @@ > > +#int1 - NC to dNSName constraint of ".example" > + > +# Subject: C=US, ST=California, L=Mountain View, O=BOGUS NSS, CN=test.invalid > +# altDns : test.invalid You need to pick a notation and stick with it. Compare lines 14, 27, 35, and 86 - each their own style. Please read all comments again for sanity (and spelling) @@ +22,5 @@ > verify NameConstraints.server2:x > cert NameConstraints.intermediate:x > result fail > > +# Subject C=US, ST=California, L=Mountain View, O=BOGUS NSS, CN=test.example Subject: @@ +55,5 @@ > + > +#Int3 - NC to a DirectoryName of "C = us, ST = ca, O = Foo" and dNSName constraint of "foo.example" > +#Int2 - Subject of "C=US, ST=CA, O=Foo, CN=NSS Intermediate CA 2", no NC present, signed by Int 4 > + > +#ubject of "C=US, ST=CA, O=Foo, OU=bar, CN=bat.foo.example", no SAN Subject @@ +90,5 @@ > + cert NameConstraints.intermediate3:x > + result pass > + > +# Subject: C=US, ST=CA, O=Foo, CN=Honest Achmed > +# Fail: CN doest not NC even tough is not DNS shaped Fail: CN does not pass NC even though it is not DNS shaped @@ +97,5 @@ > + cert NameConstraints.intermediate3:x > + result fail > + > +## > +# Intermediate 5: signed by intt 3 Intermediate 3
Attachment #8369553 -
Flags: review?(ryan.sleevi) → review-
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8369553 -
Attachment is obsolete: true
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8369584 [details] [diff] [review] add-tests-962760 (v 2.1) Review of attachment 8369584 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the speedy reviews. Here is a cleanup.
Attachment #8369584 -
Flags: review?(ryan.sleevi)
Comment 21•10 years ago
|
||
Comment on attachment 8369584 [details] [diff] [review] add-tests-962760 (v 2.1) Review of attachment 8369584 [details] [diff] [review]: ----------------------------------------------------------------- r- because more grammar fixes :) Sorry for being so pedantic on this, but given the complexity of the tests, want to make sure it's long-term readable. ::: tests/chains/scenarios/nameconstraints.cfg @@ +28,5 @@ > verify NameConstraints.server3:x > cert NameConstraints.intermediate:x > result pass > > +# Intermediate 2: No name constraints, signed by intermediate 1(inherits name constraints) Intermediate 2: No name constraints, signed by Intermediate 1 (inherits name constraints) ": N" -> ": N" [matches lines 56 and 11] "intermediate" -> "Intermediate" [matches line 11] "1(inherits" -> "1 (inherits" [grammar] @@ +52,5 @@ > + cert NameConstraints.intermediate2:x > + cert NameConstraints.intermediate:x > + result pass > + > +# Intermediate 3: Name constrained to a permitted DirectoryName of "C = us, ST = ca, O = Foo" us -> US ca -> CA (matches line 58/63/etc) @@ +54,5 @@ > + result pass > + > +# Intermediate 3: Name constrained to a permitted DirectoryName of "C = us, ST = ca, O = Foo" > +# and a permitted DNSName of "foo.example" > +# Subject: "C=US, ST=California, L=Mountain View, O=BOGUS NSS, CN=NSS Intermediate CA3" The description here is inconsistent with the format you used on lines 60/61, namely the order in which the subject appears For my own preference, I'm a fan of Intermediate 3: Subject: ... Name constrained to a permitted DirectoryName of "C=US, ST=CA, O=Foo" and a permitted DNSName of "foo.example" Intermediate 4: Subject: ... No name constraints present Signed by Intermediate 3 (inheriting the name constraints) @@ +72,5 @@ > + cert NameConstraints.intermediate3:x > + result pass > + > +# Subject: "C=US, O=Foo, CN=bat.foo.example", no SAN > +# Fail: missing ST in name constraints Fail: ST is missing in subject, thus not matching name constraints @@ +87,5 @@ > + result fail > + > +# Subject: "C=US, ST=CA, O=Foo, CN=site.example" > +# altDNS:foo.example > +# Pass: ignore CN constraint name validation Pass: Ignores CN name constraint violation because DNS SAN is present @@ +94,5 @@ > + cert NameConstraints.intermediate3:x > + result pass > + > +# Subject: "C=US, ST=CA, O=Foo, CN=Honest Achmed" > +# Fail: CN doest not name constrained even tough is not DNS shaped You didn't update this sentence: "Fail: CN does not match DNS name constraints - even though it's not 'DNS shaped'" @@ +101,5 @@ > + cert NameConstraints.intermediate3:x > + result fail > + > +## > +# Intermediate 5: signed by intermediate 3 (subject not in signed name constrained space) If you update lines 58, update this line for consistency as well Intermediate 5: Subject: ... Signed by Intermediate 3 Intermediate 5's subject is not in Intermediate 3's permitted names, so all certs issued by it are invalid @@ +105,5 @@ > +# Intermediate 5: signed by intermediate 3 (subject not in signed name constrained space) > +# Subject: "C=US, ST=CA, O=OtherOrg, CN=NSS Intermediate CA 2" > + > +# Subject: "C=US, ST=CA, O=OtherOrg, CN=bat.foo.example" > +# Fail: Org is not in name constrained space (as does its signer) "as does its signer" is not grammatically consistent with the preceding remark "Fail: Org matches Intermediate 5's name constraints, but does not match Intermediate 3's name constraints" @@ +112,5 @@ > + cert NameConstraints.intermediate3:x > + result fail > + > +# Subject: "C=US, ST=CA, O=Foo, CN=another.foo.example" > +# Fail: While server14 is on intermediate5 name space, its signer it not. Fail: Matches Intermediate 5's name constraints, but fails because Intermediate 5 does not match Intermediate 3's name constraints
Attachment #8369584 -
Flags: review?(ryan.sleevi) → review-
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8369584 -
Attachment is obsolete: true
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8369629 -
Attachment is obsolete: true
Assignee | ||
Comment 24•10 years ago
|
||
Comment on attachment 8369630 [details] [diff] [review] add-tests-962760 (v2.3) Review of attachment 8369630 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Being punctilious with reviews is ok with me, as long as the comments are actuable :).
Attachment #8369630 -
Flags: review?(ryan.sleevi)
Comment 25•10 years ago
|
||
Comment on attachment 8369630 [details] [diff] [review] add-tests-962760 (v2.3) Review of attachment 8369630 [details] [diff] [review]: ----------------------------------------------------------------- r+ with one miiiiinor comment :) Thanks so much for the tests. ::: tests/chains/scenarios/nameconstraints.cfg @@ +58,5 @@ > +# and a permitted DNSName of "foo.example" > + > +# Intermediate 4: Subject: "C=US, ST=CA, O=Foo, CN=NSS Intermediate CA 2" > +# No name constraints present > +# Signed by Intermediate 3 (Inherits name constraints) s/Inherits/inherits/ (line 32 as comparison)
Attachment #8369630 -
Flags: review?(ryan.sleevi) → review+
Assignee | ||
Comment 26•10 years ago
|
||
Comment on attachment 8366316 [details] [diff] [review] fix-bug-962760 Review of attachment 8366316 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/libpkix/include/pkix_pl_pki.h @@ +1268,5 @@ > * Address of Cert whose subject names are to be checked. > * Must be non-NULL. > * "nameConstraints" > * Address of CertNameConstraints that need to be satisfied. > + * "treatCommonNameAsDNS" wtc. This is the name I had on my first patch. But changed to address a comment by ryan. My plan is to keep ryan's suggestion unless you reiterate about this change. ::: lib/libpkix/pkix/checker/pkix_nameconstraintschecker.c @@ +192,4 @@ > PKIX_CERTCHECKNAMECONSTRAINTSFAILED); > } > > if (state->certsRemaining != 0) { Agreed and one. Local variable name: lastCert (OK?) ::: lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c @@ +3151,5 @@ > if (arena == NULL) { > PKIX_ERROR(PKIX_OUTOFMEMORY); > } > > /* This NSS call returns both Subject and Subject Alt Names */ /* This NSS call returns both Directory names and DNS names*/ ?
Attachment #8366316 -
Flags: feedback?(wtc)
Assignee | ||
Comment 27•10 years ago
|
||
Keeping r+ from ryan.sleevi
Attachment #8369630 -
Attachment is obsolete: true
Attachment #8369682 -
Flags: review+
Comment 28•10 years ago
|
||
Comment on attachment 8366316 [details] [diff] [review] fix-bug-962760 Review of attachment 8366316 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/libpkix/include/pkix_pl_pki.h @@ +1268,5 @@ > * Address of Cert whose subject names are to be checked. > * Must be non-NULL. > * "nameConstraints" > * Address of CertNameConstraints that need to be satisfied. > + * "treatCommonNameAsDNS" Ah, I didn't know that. Yes, you can keep Ryan's suggestion, perhaps just changing "DNS" to "DNSName". ::: lib/libpkix/pkix/checker/pkix_nameconstraintschecker.c @@ +192,4 @@ > PKIX_CERTCHECKNAMECONSTRAINTSFAILED); > } > > if (state->certsRemaining != 0) { Yes, "lastCert" or "isLastCert" is OK. ::: lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c @@ +3151,5 @@ > if (arena == NULL) { > PKIX_ERROR(PKIX_OUTOFMEMORY); > } > > /* This NSS call returns both Subject and Subject Alt Names */ This comment should say something like: This NSS call returns Subject Alt Names. If treatCommonNameAsDNS is true, it also returns the Subject Common Name. or: This NSS call doesn't return the Subject Common Name unless treatCommonNameAsDNS is true.
Attachment #8366316 -
Flags: feedback?(wtc)
Assignee | ||
Comment 29•10 years ago
|
||
keeping r+ from wtc and ryan.sleevi.
Attachment #8366316 -
Attachment is obsolete: true
Attachment #8369728 -
Flags: review+
Comment 30•10 years ago
|
||
Comment on attachment 8369728 [details] [diff] [review] fix-bug-962760 (v2) Review of attachment 8369728 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc.
Attachment #8369728 -
Flags: review+
Updated•10 years ago
|
Attachment #8369682 -
Flags: checked-in+
Updated•10 years ago
|
Attachment #8369728 -
Flags: checked-in+
Comment 31•10 years ago
|
||
Checked in both patches together in http://hg.mozilla.org/projects/nss/rev/02928cea0b12
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.16
Updated•10 years ago
|
Priority: -- → P1
See Also: → https://launchpad.net/bugs/1358727
You need to log in
before you can comment on or make changes to this bug.
Description
•