LibNss Bug 962760 affects usability of Chrome
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
NSS |
Fix Released
|
Medium
|
|||
nss (Ubuntu) |
Fix Released
|
Undecided
|
Unassigned |
Bug Description
I'm affected by https:/
The Chrome browser at version 37 is showing a name constraints violation, which shouldn't be there.
Could you please consider upgrading the library or backporting the fix?
Thanks.
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #2 |
Created attachment 8363934
fix-bug-962760
In Mozilla Bugzilla #962760, Ryan-sleevi (ryan-sleevi) wrote : | #3 |
Comment on attachment 8363934
fix-bug-962760
Review of attachment 8363934:
-------
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://
For the forward building case - which is an optimization strategy - http://
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #4 |
thanks for the review.
So it is ok to modify the signature of PKIX_PL_
In Mozilla Bugzilla #962760, Ryan-sleevi (ryan-sleevi) wrote : | #5 |
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)
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #6 |
Created attachment 8364695
fix-bug-962760-b
This patch, suggested by rsleevi prevents intermediates from, being evaluated for being a CN. This solves the case for SSL server.
In Mozilla Bugzilla #962760, Ryan-sleevi (ryan-sleevi) wrote : | #7 |
Comment on attachment 8364695
fix-bug-962760-b
Review of attachment 8364695:
-------
Can you add a unit test - eg: to chains.sh - that demonstrates failure before and success afterwards?
::: security/
@@ +1270,5 @@
> * "nameConstraints"
> * Address of CertNameConstraints that need to be satisfied.
> + * "includeSubject
> + * 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_
> PKIX_PL_Cert *cert,
> PKIX_PL_
> + PKIX_Boolean includeSubjectC
treatCommonName
The reason is that the subject CN should always be subject to directoryName constraints, it's the dNSName constraint thats special here.
::: security/
@@ +425,5 @@
> PKIX_COMCERTSEL
>
> if (nameConstraints != NULL) {
>
> PKIX_CHECK(
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.
*/
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #8 |
Created attachment 8366316
fix-bug-962760
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #9 |
Created attachment 8366317
add-tests-962760
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #10 |
Comment on attachment 8366317
add-tests-962760
Review of attachment 8366317:
-------
Added this set of tests. The test with server6 fails with the current code, passes with the fix.
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #11 |
Comment on attachment 8366316
fix-bug-962760
Review of attachment 8366316:
-------
Comments addressed (now patch in nss name-space)
In Mozilla Bugzilla #962760, Ryan-sleevi (ryan-sleevi) wrote : | #12 |
Comment on attachment 8366317
add-tests-962760
Review of attachment 8366317:
-------
::: tests/chains/
@@ +19,5 @@
> verify NameConstraints
> cert NameConstraints
> result pass
>
> +verify NameConstraints
Can you add basic comments for what each of these tests does, similar to http://
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.
Works
Server 2 - Subject of "C=Foo, ST=Bar, O=Baz, CN=bat.
Works
Server 3 - Subject of "C=Foo, O=Baz, OU=Bat, CN=bat.
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.
Fails [Int 3 violates NC, as does EE]
Server 8 - Subject of "C=Foo, ST=Bar, O=Baz, CN=bat.
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.
In Mozilla Bugzilla #962760, Ryan-sleevi (ryan-sleevi) wrote : | #13 |
Comment on attachment 8366316
fix-bug-962760
Review of attachment 8366316:
-------
r=ryan.sleevi (mod test comments)
In Mozilla Bugzilla #962760, Ryan-sleevi (ryan-sleevi) wrote : | #14 |
Comment on attachment 8366317
add-tests-962760
Review of attachment 8366317:
-------
r- due to request for even more tests, though I think the existing tests are good.
In Mozilla Bugzilla #962760, Wan-Teh Chang (wtc-google) wrote : | #15 |
Comment on attachment 8366316
fix-bug-962760
Review of attachment 8366316:
-------
r=wtc.
::: lib/libpkix/
@@ +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.
> + * "treatCommonNam
Nit: I think we can name this argument "includeSubject
Or name it "treatCommonNam
::: lib/libpkix/
@@ +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/
@@ +192,4 @@
> PKIX_CERTCHECKN
> }
>
> if (state-
Maybe we should save the result of state->
::: lib/libpkix/
@@ +3151,5 @@
> if (arena == NULL) {
> PKIX_ERROR(
> }
>
> /* This NSS call returns both Subject and Subject Alt Names */
This comment needs to be updated because of the change.
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #16 |
Created attachment 8369553
add-tests-962760 (v2)
Added the new tests and commented the already existing.
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #17 |
Comment on attachment 8369553
add-tests-962760 (v2)
Review of attachment 8369553:
-------
remove request as something I missed on the folding of patches.
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #18 |
Comment on attachment 8369553
add-tests-962760 (v2)
Review of attachment 8369553:
-------
actuallt there was no issue. This is the test patch
In Mozilla Bugzilla #962760, Ryan-sleevi (ryan-sleevi) wrote : | #19 |
Comment on attachment 8369553
add-tests-962760 (v2)
Review of attachment 8369553:
-------
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/
@@ +7,5 @@
> db trustanchors
>
> import NameConstraints
>
> +#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
> cert NameConstraints
> 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.
Subject
@@ +90,5 @@
> + cert NameConstraints
> + 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
> + result fail
> +
> +##
> +# Intermediate 5: signed by intt 3
Intermediate 3
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #20 |
Created attachment 8369584
add-tests-962760 (v 2.1)
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #21 |
Comment on attachment 8369584
add-tests-962760 (v 2.1)
Review of attachment 8369584:
-------
Thanks for the speedy reviews. Here is a cleanup.
In Mozilla Bugzilla #962760, Ryan-sleevi (ryan-sleevi) wrote : | #22 |
Comment on attachment 8369584
add-tests-962760 (v 2.1)
Review of attachment 8369584:
-------
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/
@@ +28,5 @@
> verify NameConstraints
> cert NameConstraints
> 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
> + cert NameConstraints
> + 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
> + result pass
> +
> +# Subject: "C=US, O=Foo, CN=bat.
> +# 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
> + 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
> + 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....
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #23 |
Created attachment 8369629
add-tests-962760 (v2.2)
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #24 |
Created attachment 8369630
add-tests-962760 (v2.3)
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #25 |
Comment on attachment 8369630
add-tests-962760 (v2.3)
Review of attachment 8369630:
-------
Thanks, Being punctilious with reviews is ok with me, as long as the comments are actuable :).
In Mozilla Bugzilla #962760, Ryan-sleevi (ryan-sleevi) wrote : | #26 |
Comment on attachment 8369630
add-tests-962760 (v2.3)
Review of attachment 8369630:
-------
r+ with one miiiiinor comment :)
Thanks so much for the tests.
::: tests/chains/
@@ +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/
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #27 |
Comment on attachment 8366316
fix-bug-962760
Review of attachment 8366316:
-------
::: lib/libpkix/
@@ +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.
> + * "treatCommonNam
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/
@@ +192,4 @@
> PKIX_CERTCHECKN
> }
>
> if (state-
Agreed and one. Local variable name: lastCert (OK?)
::: lib/libpkix/
@@ +3151,5 @@
> if (arena == NULL) {
> PKIX_ERROR(
> }
>
> /* This NSS call returns both Subject and Subject Alt Names */
/* This NSS call returns both Directory names and DNS names*/ ?
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #28 |
Created attachment 8369682
add-tests-962760 (v 2.4)
Keeping r+ from ryan.sleevi
In Mozilla Bugzilla #962760, Wan-Teh Chang (wtc-google) wrote : | #29 |
Comment on attachment 8366316
fix-bug-962760
Review of attachment 8366316:
-------
::: lib/libpkix/
@@ +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.
> + * "treatCommonNam
Ah, I didn't know that. Yes, you can keep Ryan's suggestion, perhaps just changing "DNS" to "DNSName".
::: lib/libpkix/
@@ +192,4 @@
> PKIX_CERTCHECKN
> }
>
> if (state-
Yes, "lastCert" or "isLastCert" is OK.
::: lib/libpkix/
@@ +3151,5 @@
> if (arena == NULL) {
> PKIX_ERROR(
> }
>
> /* This NSS call returns both Subject and Subject Alt Names */
This comment should say something like:
This NSS call returns Subject Alt Names. If treatCommonName
it also returns the Subject Common Name.
or:
This NSS call doesn't return the Subject Common Name unless
treatCommonNa
In Mozilla Bugzilla #962760, Cviecco (cviecco) wrote : | #30 |
Created attachment 8369728
fix-bug-962760 (v2)
keeping r+ from wtc and ryan.sleevi.
In Mozilla Bugzilla #962760, Wan-Teh Chang (wtc-google) wrote : | #31 |
Comment on attachment 8369728
fix-bug-962760 (v2)
Review of attachment 8369728:
-------
r=wtc.
In Mozilla Bugzilla #962760, 2-brian (2-brian) wrote : | #32 |
Checked in both patches together in
http://
Changed in nss: | |
importance: | Unknown → Medium |
status: | Unknown → Fix Released |
Marc Deslauriers (mdeslaur) wrote : | #33 |
NSS has now been updated to 3.17 in all supported releases:
http://
As such, I am closing this bug. Feel free to reopen it if the update didn't solve the issue.
Changed in nss (Ubuntu): | |
status: | New → Fix Released |
Dominik Röttsches (drott) wrote : | #34 |
Works for me, thanks for the update.
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.