If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

libpkix includes the cn of CA as dns names when evaluating name constraints

RESOLVED FIXED in 3.16

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: cviecco, Assigned: cviecco)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Assignee)

Description

4 years ago
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

4 years ago
Assignee: nobody → cviecco
(Assignee)

Comment 1

4 years ago
Created attachment 8363934 [details] [diff] [review]
fix-bug-962760

Comment 2

4 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

4 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

4 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

4 years ago
Created attachment 8364695 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8364695 - Flags: review?(ryan.sleevi)
(Assignee)

Updated

4 years ago
Blocks: 743700

Comment 6

4 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

4 years ago
Created attachment 8366316 [details] [diff] [review]
fix-bug-962760
Attachment #8363934 - Attachment is obsolete: true
Attachment #8364695 - Attachment is obsolete: true
Attachment #8364695 - Flags: review?(ryan.sleevi)
(Assignee)

Comment 8

4 years ago
Created attachment 8366317 [details] [diff] [review]
add-tests-962760
(Assignee)

Comment 9

4 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

4 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

4 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

4 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

4 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

4 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

4 years ago
Created attachment 8369553 [details] [diff] [review]
add-tests-962760 (v2)

Added the new tests and commented the already existing.
Attachment #8366317 - Attachment is obsolete: true
Attachment #8369553 - Flags: review?(ryan.sleevi)
(Assignee)

Comment 16

4 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

4 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

4 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

4 years ago
Created attachment 8369584 [details] [diff] [review]
add-tests-962760 (v 2.1)
Attachment #8369553 - Attachment is obsolete: true
(Assignee)

Comment 20

4 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

4 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

4 years ago
Created attachment 8369629 [details] [diff] [review]
add-tests-962760 (v2.2)
Attachment #8369584 - Attachment is obsolete: true
(Assignee)

Comment 23

4 years ago
Created attachment 8369630 [details] [diff] [review]
add-tests-962760 (v2.3)
Attachment #8369629 - Attachment is obsolete: true
(Assignee)

Comment 24

4 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

4 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

4 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

4 years ago
Created attachment 8369682 [details] [diff] [review]
add-tests-962760 (v 2.4)

Keeping r+ from ryan.sleevi
Attachment #8369630 - Attachment is obsolete: true
Attachment #8369682 - Flags: review+

Comment 28

4 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

4 years ago
Created attachment 8369728 [details] [diff] [review]
fix-bug-962760 (v2)

keeping r+ from wtc and ryan.sleevi.
Attachment #8366316 - Attachment is obsolete: true
Attachment #8369728 - Flags: review+

Comment 30

4 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+
Attachment #8369682 - Flags: checked-in+
Attachment #8369728 - Flags: checked-in+
Checked in both patches together in
http://hg.mozilla.org/projects/nss/rev/02928cea0b12
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.16

Updated

4 years ago
Priority: -- → P1

Updated

3 years ago
You need to log in before you can comment on or make changes to this bug.