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)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cviecco, Assigned: cviecco)

References

Details

Attachments

(2 files, 8 obsolete files)

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: nobody → cviecco
Attached patch fix-bug-962760 (obsolete) — Splinter Review
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-
thanks for the review.
So it is ok to modify the signature of PKIX_PL_Cert_GetNameConstraints of you prefer a new funcion?
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)
Attached patch fix-bug-962760-b (obsolete) — Splinter Review
This patch, suggested by rsleevi prevents intermediates from, being evaluated for being a CN. This solves the case for SSL server.
Attachment #8364695 - Flags: review?(ryan.sleevi)
Blocks: 743700
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.
 */
Attached patch fix-bug-962760 (obsolete) — Splinter Review
Attachment #8363934 - Attachment is obsolete: true
Attachment #8364695 - Attachment is obsolete: true
Attachment #8364695 - Flags: review?(ryan.sleevi)
Attached patch add-tests-962760 (obsolete) — Splinter Review
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)
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 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 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 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 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+
Attached patch add-tests-962760 (v2) (obsolete) — Splinter Review
Added the new tests and commented the already existing.
Attachment #8366317 - Attachment is obsolete: true
Attachment #8369553 - Flags: review?(ryan.sleevi)
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)
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 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-
Attached patch add-tests-962760 (v 2.1) (obsolete) — Splinter Review
Attachment #8369553 - Attachment is obsolete: true
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 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-
Attached patch add-tests-962760 (v2.2) (obsolete) — Splinter Review
Attachment #8369584 - Attachment is obsolete: true
Attached patch add-tests-962760 (v2.3) (obsolete) — Splinter Review
Attachment #8369629 - Attachment is obsolete: true
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 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+
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)
Keeping r+ from ryan.sleevi
Attachment #8369630 - Attachment is obsolete: true
Attachment #8369682 - Flags: review+
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)
keeping r+ from wtc and ryan.sleevi.
Attachment #8366316 - Attachment is obsolete: true
Attachment #8369728 - Flags: review+
Comment on attachment 8369728 [details] [diff] [review]
fix-bug-962760 (v2)

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

r=wtc.
Attachment #8369728 - Flags: review+
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
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: