991783 - Add generic mechanism to add name constraints to built-in certificates

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Camilo Viecco (:cviecco)]

There is a need to append name constraints to root cerificates AFTER they have been inserted into the trust database. There is a hardcoding way but this bug is to track a way to do it by modifying the trust database.

Comment 1

[Robert Relyea]

The easiest way to do this would be to add new TRUST Attributes that hold additional certificate extensions. When we do the trust lookups for the cert, we can also add these extensions to the CERTCertExtensions array (cert->extensions), or to the NSSCertificate (where we can add fields because the datastructure is private). If we do the former, we need to make sure we protect the cert->extensions field update from thread issues. If we do the latter, we then need to update cert_FindExtensionByOID() (nss/lib/cerdb/certxutl.c) to search both the extensions array and the extended one in NSSCertificate.

Comment 2

[Ryan Sleevi]

Bob, that actually seems quite a bit more complex.

Wouldn't the better path be to modify the few (two? three?) places in libpkix and legacy that fetch the constraint set to fetch from the trust record? Camilo has already done as much to fetch the certs' constraints in libpkix.

This avoids the threading interactions or the complexity of the NSSCertificate (which has an untold number of side-effects), while offering a simple way to facilitate additional inputs into the standard RFC5280 chain building algorithm (as used by libpkix and mozilla::pkix)

Comment 3

[Robert Relyea]

I don't think so, though I wouldn't mind seeing a patch that does that. 

One of the problems is you probably don't want to fetch these values each time you validate the cert, so you would want to cache, which would put you back into the thread issues again.

The other is in the long term constraints are the only place we may want to tweek with external extensions. Adding extensions to the trust object lets us handle multiple types of extension additions.

There is no panecia, but certainly what I proposed is not exactly complex (either to implement or to get right).

bob

Comment 4

[Richard Barnes [:rbarnes]]

Attachment: bug-991783.patch

This is a POC patch that just refactors the existing ANSSI constraints into a separate header file with a lookup method.  I realize that this is not the most elegant way of doing things, but it does seem minimally invasive.

Bob, Keeler: Does this look like roughly an acceptable arrangement of things? 

Obviously, I will refactor this into independent patches for Firefox and NSS once we agree on the approach.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8532685 [details] [diff] [review]
bug-991783.patch

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

LGTM in general.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +93,5 @@
>        }
>  
>        bool keepGoing;
> +      SECItem nameConstraints;
> +      SECStatus ss = CERT_GetNameConstraintsBuiltIn(&encodedIssuerNameSECItem,

Sometimes we use srv for variables like this.

@@ +95,5 @@
>        bool keepGoing;
> +      SECItem nameConstraints;
> +      SECStatus ss = CERT_GetNameConstraintsBuiltIn(&encodedIssuerNameSECItem,
> +                                                    &nameConstraints);
> +      if (ss == SECSuccess) {

Be sure to handle the fatal error case as well:

} else if (PR_GetError() != SEC_ERROR_EXTENSION_NOT_FOUND) {
  return Result::FATAL_ERROR_LIBRARY_FAILURE;
}

::: security/certverifier/moz.build
@@ +23,5 @@
>  
>  LOCAL_INCLUDES += [
>      '../manager/boot/src',
>      '../manager/ssl/src',
> +    '../nss/lib/certdb/',

cert.h should already be visible to NSSCertDBTrustDomain.cpp, so this shouldn't be necessary.

::: security/nss/lib/certdb/manifest.mn
@@ +7,5 @@
>  EXPORTS = \
>  	cert.h \
>  	certt.h \
>  	certdb.h \
> +  ncons.h \

nit: I guess this file uses tabs, not spaces

::: security/nss/lib/certdb/ncons.h
@@ +1,1 @@
> +#ifndef _NCONS_H_

Maybe we could call this file something a little less cryptic like 'nameconstraints.h'?

@@ +1,5 @@
> +#ifndef _NCONS_H_
> +#define _NCONS_H_
> +
> +/*
> + * This file defines built-in name constraints for CAs included in NSS.

I would add something like "when they're not already present in the certificate itself".

@@ +7,5 @@
> +
> +#include "seccomon.h"
> +#include "secerr.h"
> +
> +#define CONSTRAINT_ARRAY_SIZE 2

It would probably be best to use things like PR_ARRAY_SIZE.

@@ +8,5 @@
> +#include "seccomon.h"
> +#include "secerr.h"
> +
> +#define CONSTRAINT_ARRAY_SIZE 2
> +#define CONSTRAINT_ENTRY_SIZE 256

Doesn't look like this is used.

@@ +91,5 @@
> + * (Most importantly, root certificates)
> + */
> +static SECStatus
> +CERT_GetNameConstraintsBuiltIn(SECItem *derSubject,
> +                               SECItem *extensions)

extensions should probably be a pointer to a SECItem*. That way, it's more clear that the caller is responsible for the memory that gets allocated as a result of the SECITEM_CopyItem (otherwise, we get this strange mix of stack-allocated SECItems that have pointers to heap-allocated data (it's hard to use RAII types for that - see http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#853)).

@@ +105,5 @@
> +  PORT_SetError(SEC_ERROR_EXTENSION_NOT_FOUND);
> +  return SECFailure;
> +}
> +
> +#endif /* ndef __CONSTRAINTS_H__ */

This comment doesn't match the guard at the top of the file.

Comment 6

[Robert Relyea]

Comment on attachment 8532685 [details] [diff] [review]
bug-991783.patch

Though our list is starting to get a little long. It may good to invest some time to create PKCS #11 attributes and put them in the libnssckbi. 

bob

Comment 7

[Richard Barnes [:rbarnes]]

Attachment: bug-991783.1.full.patch

I think this one is pretty much good to go.  

The main uncertainty is with regard to this feedback:

> extensions should probably be a pointer to a SECItem*. That way, 
> it's more clear that the caller is responsible for the memory that 
> gets allocated as a result of the SECITEM_CopyItem (otherwise, we 
> get this strange mix of stack-allocated SECItems that have pointers 
> to heap-allocated data (it's hard to use RAII types for that).

I decided that it would be simpler just to have the SECItem point to the global, so that you can just use a stack-allocated SECItem and pass in a pointer to it.  It seems like this avoids any worries about freeing things.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8539733 [details] [diff] [review]
bug-991783.1.full.patch

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

LGTM, but this will need to be two separate patches (and probably even two separate bugs): one for NSS and one for PSM. r=me for the PSM bits.

::: config/external/nss/nss.def
@@ +111,5 @@
>  CERT_GetDefaultCertDB
>  CERT_GetFirstEmailAddress
>  CERT_GetGeneralNameTypeFromString
>  CERT_GetLocalityName
> +CERT_GetNameConstraintsBuiltIn

I think you'll need to update the nss.def file in the actual nss tree as well.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +93,5 @@
>      }
>  
> +    const SECItem encodedIssuerNameSECItem = {
> +      siBuffer,
> +      (unsigned char*) encodedIssuerName.UnsafeGetData(),

nit: let's use c++ style casts: const_cast<unsigned char*>(encodedIssuerName.UnsafeGetData()) (although that I guess that might not work since this is actually both a const cast and a reinterpret cast from const uint8_t to unsigned char)

@@ +108,4 @@
>          return Result::FATAL_ERROR_LIBRARY_FAILURE;
>        }
> +      rv = checker.Check(certDER, &nameConstraintsInput, keepGoing);
> +    } else if (PR_GetError() != SEC_ERROR_EXTENSION_NOT_FOUND) {

nit: check the error case first, please (i.e. directly after the CERT_GetNameConstraintsBuiltIn call, check if srv != SECSuccess and PR_GetError() != SEC_ERROR_EXTENSION_NOT_FOUND)

::: security/nss/lib/certdb/nameconstraints.h
@@ +1,1 @@
> +#ifndef _NAMECONSTRAINTS_H_

I don't know what NSS's style is for guards (or if there even is one), but we should be sure we're following it

@@ +18,5 @@
> + *
> + * Both of these are stored in DER-encoded form so that they can easily
> + * be consumed by validation code.
> + */
> +static const SECItem NAME_CONSTRAINTS[1][2] = {

Hmmm - does the compiler require we specify the dimensions of the array here? If we can, let's just specify the size of the second dimension (because it will always be 2) and not the first.

@@ +33,5 @@
> +      "\x31\x0E\x30\x0C\x06\x03\x55\x04\x0B\x13\x05" "DCSSI"    /* OU */
> +      "\x31\x0E\x30\x0C\x06\x03\x55\x04\x03\x13\x05" "IGC/A"    /* CN */
> +      "\x31\x23\x30\x21\x06\x09\x2A\x86\x48\x86\xF7\x0D\x01\x09\x01"
> +                      "\x16\x14" "[e-mail]", /* emailAddress */
> +      136

Let's not hand-roll these lengths - that can easily go wrong. Let's do something like:

static const unsigned char* ANSSI_SUBJECT_DN = "...";

And then in NAME_CONSTRAINTS:

{
  siBuffer,
  (unsigned char*)ANSSI_SUBJECT_DN,
  sizeof(ANSSI_SUBJECT_DN) - 1
}

@@ +61,5 @@
> +/* Add name constraints to certain certs that do not include name constraints
> + * (Most importantly, root certificates)
> + *
> + * The SECItem data for the constraints is populated in *extensions, but
> + * points to the stack-allocated data.

This comment could be a bit more clear. I would say something like: "If a matching subject is found, the data in extensions will point to memory that the caller does not own. Do not free it."

@@ +68,5 @@
> +CERT_GetNameConstraintsBuiltIn(const SECItem *derSubject,
> +                               SECItem *extensions)
> +{
> +  if (!extensions) {
> +    return SECFailure;

Add a PR_SetError(SEC_ERROR_INVALID_ARGS, 0);

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 8539733 [details] [diff] [review]
bug-991783.1.full.patch

r- for an organizational nit regarding your change to nss.def

Please always add new exported functions at the end of the file, in particular, inside a section with the correct version number. You must use the number of the first NSS version that will contain the new function, right now that's 3.18

Comment 10

[Wan-Teh Chang]

Comment on attachment 8539733 [details] [diff] [review]
bug-991783.1.full.patch

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

Richard: Thank you for writing the patch. The new function is not defined in the right
file. I suggest the following:

1. Don't add the new nameconstraints.h header.

2. Declare the new CERT_GetNameConstraintsBuiltIn function in cert.h, without "static".

3. Move the definition of the static NAME_CONSTRAINTS array to genname.c. Rename it
to something like "builtInNameConstraints".

4 Define CERT_GetNameConstraintsBuiltIn in genname.c, without "static".

::: security/nss/lib/certdb/nameconstraints.h
@@ +1,1 @@
> +#ifndef _NAMECONSTRAINTS_H_

1. IMPORTANT: we probably should not add a new public NSS header just for this
functionality. I think one of the existing public headers in this directory
should be suitable for this.

2. If we need to add this file (possibly as an internal header), it needs a
MPL2 header.

@@ +18,5 @@
> + *
> + * Both of these are stored in DER-encoded form so that they can easily
> + * be consumed by validation code.
> + */
> +static const SECItem NAME_CONSTRAINTS[1][2] = {

It's uncommon to define a static array in a .h file. If two .c files include
this header, each of them will instantiate a copy of this array with file
static scope. That would be bad.

@@ +64,5 @@
> + * The SECItem data for the constraints is populated in *extensions, but
> + * points to the stack-allocated data.
> + */
> +static SECStatus
> +CERT_GetNameConstraintsBuiltIn(const SECItem *derSubject,

You intend to export this function (it's listed in nss.def), so this
function must not be marked as static.

Comment 11

[Richard Barnes [:rbarnes]]

Attachment: bug-991783.2.patch

This patch does a few things:
1. Removes the gecko parts (to Bug 1121982)
2. Refactors as suggested by Wan-Teh
3. Correctly updates nss.def as suggested by Kai

Comment 12

[Wan-Teh Chang]

Comment on attachment 8549950 [details] [diff] [review]
bug-991783.2.patch

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

Richard: this is much better. Many of my suggested changes are
on coding styles. But there are two important design issues.

1. Whether it is sufficient to identify a certificate using
just |derSubject|. I think the public key is also important,
because a subject may have multiple certificates.

2. Whether the function should copy the extension data.

::: lib/certdb/cert.h
@@ +1177,5 @@
>  
>  extern CERTGeneralName *
>  CERT_GetPrevGeneralName(CERTGeneralName *current);
>  
> +/* Add name constraints to certain certs that do not include name constraints

This function does not add the name constraints, right?

@@ +1181,5 @@
> +/* Add name constraints to certain certs that do not include name constraints
> +* (Most importantly, root certificates)
> +*
> +* If a matching subject is found, the data in extensions will point to memory
> +* that the caller does not own. Do not free it.

Is the data pointed to by |extensions| guaranteed to not change?
Alternatively, the function can copy the data using SECITEM_CopyItem
as the original code does.

In what form is the data? How should the caller decode it? Is the
encoded form the most convenient?

@@ +1182,5 @@
> +* (Most importantly, root certificates)
> +*
> +* If a matching subject is found, the data in extensions will point to memory
> +* that the caller does not own. Do not free it.
> +*/

Nit: format the comment block as follows:

/* First line
 * Second line
 * ...
 */

Note the space before the second through the last line.

@@ +1184,5 @@
> +* If a matching subject is found, the data in extensions will point to memory
> +* that the caller does not own. Do not free it.
> +*/
> +SECStatus
> +CERT_GetNameConstraintsBuiltIn(const SECItem *derSubject, SECItem *extensions);

1. IMPORTANT: it seems that the subjectPublicKeyInfo is also necessary to uniquely identify the built-in certificate. Providing only |derSubject| doesn't seem enough.

2. Nit: the function name could be improved. Ideally we want a name that
suggests the name constraints are not part of the certificate. Perhaps
CERT_GetExternalNameConstraints or CERT_GetImposedNameConstraints?
CERT_GetBuiltInNameConstraints would also work, except that it implies
the name constraints are built into NSS and would preclude a new function
that allows imposing additional name constraints at run time.

::: lib/certdb/genname.c
@@ +1559,5 @@
> +/*
> +* Here we define a list of name constraints to be imposed on
> +* certain certificates, most importantly root certificates.
> +*
> +* Each entry in the name constraints list is constructe with this

constructe => constructed

@@ +1560,5 @@
> +* Here we define a list of name constraints to be imposed on
> +* certain certificates, most importantly root certificates.
> +*
> +* Each entry in the name constraints list is constructe with this
> +* macro.  An entry contains two SECItems, which have have names in

have have => have

@@ +1569,5 @@
> +*  * ${CA}_NAME_CONSTRAINTS - The name constraints extension
> +*
> +* Both of these are stored in DER-encoded form so that they can easily
> +* be consumed by validation code.
> +*/

Nit: format the comment block as follows:

/*
 * Foo
 * Bar
 * ...
 */

Note the space before the second through the last line.

@@ +1580,5 @@
> +  STRING_TO_SECITEM(CA ## _SUBJECT_DN), \
> +  STRING_TO_SECITEM(CA ## _NAME_CONSTRAINTS) \
> +}
> +
> +/* ANSSI */

Nit: provide the full name of ANSSI.

@@ +1591,5 @@
> +"\x31\x10\x30\x0E\x06\x03\x55\x04\x0A\x13\x07" "PM/SGDN"  /* O */    \
> +"\x31\x0E\x30\x0C\x06\x03\x55\x04\x0B\x13\x05" "DCSSI"    /* OU */   \
> +"\x31\x0E\x30\x0C\x06\x03\x55\x04\x03\x13\x05" "IGC/A"    /* CN */   \
> +"\x31\x23\x30\x21\x06\x09\x2A\x86\x48\x86\xF7\x0D\x01\x09\x01"       \
> +"\x16\x14" "[e-mail]" /* emailAddress */ \

Nit: I probably would indent lines 1587-1595 (with four spaces, the
indentation level in NSS). Similarly for the other three macro definitions.

@@ +1609,5 @@
> +"\x30\x05\x82\x03" ".pf" \
> +"\x30\x05\x82\x03" ".nc" \
> +"\x30\x05\x82\x03" ".tf" \
> +
> +static const SECItem builtInNameConstraints[1][2] = {

Nit: dkeeler suggested using [][2]. Does that work?

@@ +1617,1 @@
>  /* Add name constraints to certain certs that do not include name constraints

This function does not add name constraints.
You can move this comment before the builtInNameConstraints array,
or just remove the comment (because it is very similar to the comment
on lines 1560-1561).

@@ +1621,5 @@
> +* that the caller does not own. Do not free it.
> +*/
> +SECStatus
> +CERT_GetNameConstraintsBuiltIn(const SECItem *derSubject,
> +SECItem *extensions)

If this argument doesn't fit on the previous line, please align
it on the left with the first argument.

@@ +1623,5 @@
> +SECStatus
> +CERT_GetNameConstraintsBuiltIn(const SECItem *derSubject,
> +SECItem *extensions)
> +{
> +  if (!extensions) {

NSS uses an indentation level of four spaces. Please fix the
entire function.

@@ +1658,5 @@
>      if (rv != SECSuccess) {
>          if (PORT_GetError() != SEC_ERROR_EXTENSION_NOT_FOUND) {
>              return rv;
>          }
> +        rv = CERT_GetNameConstraintsBuiltIn(&cert->derSubject,  

Nit: the code review tool shows a space at the end of this line.

Comment 13

[Kai Engert (:KaiE:)]

Comment on attachment 8549950 [details] [diff] [review]
bug-991783.2.patch

I assume you'll attach a new patch with the requests from Wan-Teh addressed.

Comment 14

[Richard Barnes [:rbarnes]]

Attachment: bug-991783.2-3.interdiff

Comment 15

[Richard Barnes [:rbarnes]]

Attachment: bug-991783.3.patch

(In reply to Wan-Teh Chang from comment #12)
> Comment on attachment 8549950 [details] [diff] [review]
> bug-991783.2.patch
> 
> Review of attachment 8549950 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Richard: this is much better. Many of my suggested changes are
> on coding styles. But there are two important design issues.

Thanks for the review.  I have fixed the style issues.  I don't think the design issues require a change here, for the reasons stated below.

Meta-point: This patch is just generalizing what has already been in NSS for 9 months:
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/certdb/genname.c#1619

> 1. Whether it is sufficient to identify a certificate using
> just |derSubject|. I think the public key is also important,
> because a subject may have multiple certificates.

I think using just |derSubject| is OK, for a few reasons:

1. It's what we do today.

2. All of the certificates in the NSS root store have distinct names, so there's no collision at that level.  The main risk of collision is from cross-certifications, where the public key is going to be the same anyway.

3. If you really want uniqueness, you have to pass in the cert.  And at least one of the potential callers of this method can't do that (it only has the subject name).
https://dxr.mozilla.org/mozilla-central/source/security/certverifier/NSSCertDBTrustDomain.cpp#129

3. It seems to me to be the right semantic -- we're not constraining what a key can do, we want to constraint what the *CA* can do.

4. If we need a different selection criterion, we can add something like CERT_GetImposedNameConstraintsByCert() later.


> 2. Whether the function should copy the extension data.

Given that we're currently always returning static data (and have no plans to do otherwise), it seems like just returning a pointer to that static data makes things simpler and faster.  If we have a need in the future to have more dynamic provisioning of name constraints, we just need to make sure that the provider of the list manages the memory.


> 2. Nit: the function name could be improved. Ideally we want a name that
> suggests the name constraints are not part of the certificate. Perhaps
> CERT_GetExternalNameConstraints or CERT_GetImposedNameConstraints?
> CERT_GetBuiltInNameConstraints would also work, except that it implies
> the name constraints are built into NSS and would preclude a new function
> that allows imposing additional name constraints at run time.

Changed to CERT_GetImposedNameConstraints.

Comment 16

[Robert Relyea]

Hi richard, my preliminary review is the basic idea is fine. If wtc doesn't give you an r+ by Wednesday, I'll give you a full review then.

bob

Comment 17

[Wan-Teh Chang]

Comment on attachment 8558690 [details] [diff] [review]
bug-991783.3.patch

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

r=wtc. You can go ahead and check this in after making the changes
I suggested. Sorry about the late review.

I have some high-level comments.

1. In the comment for CERT_FindNameConstraintsExten, please add a
note to point out that the function will return imposed name
constraints.

2. I assume you are adding the new CERT_GetImposedNameConstraints
function for mozilla::pkix. If mozilla::pkix is using the
CERT_FindNameConstraintsExten, then the new function is not
needed. If mozilla::pkix is calling

    CERT_FindCertExtension(cert, SEC_OID_X509_NAME_CONSTRAINTS,
                           &constraintsExtension)

to get the binary-encoded name constraints extension, then an
alternative solution is to change CERT_FindCertExtension() to
use the imposed name constraints. I am worried that can be
confusing because we won't be able to check if a certificate
has a name constraints extension. So this is probably a bad
idea.

::: lib/certdb/cert.h
@@ +1177,5 @@
>  
>  extern CERTGeneralName *
>  CERT_GetPrevGeneralName(CERTGeneralName *current);
>  
> +/* Provide name constraints for some certs that do not include name constraints

Nit: "provide" could be misinterpreted to mean "add" or "impose".

@@ +1180,5 @@
>  
> +/* Provide name constraints for some certs that do not include name constraints
> + * (Most importantly, root certificates)
> + *
> + * If a matching subject is found, extensions will be populated with a DER-encoded

Nit: here and on line 1187, quote "extensions" to make it clear it is referring to the argument. You can use either |extensions| (gradually being adopted by NSS) or 'extensions' (the original NSS style).

@@ +1187,5 @@
> + * The data in extensions will point to memory that the caller does not own. Do
> + * not free it.
> + */
> +SECStatus
> +CERT_GetImposedNameConstraints(const SECItem *derSubject, SECItem *extensions);

When I read this, I am wondering: what function should an application call to impose name constraints on a certificate (or rather, certificate subject)? If all the imposed name constraints are built into NSS, it would be nice to document that.

::: lib/certdb/genname.c
@@ +1557,5 @@
>  }
>  
> +/*
> + * Here we define a list of name constraints to be imposed on
> + * certain certificates, most importantly root certificates.

Nit: it would be nice to explain why this list identifies a certificate by the subject name rather than some unique identifier, such as the [issuer name, serial number] pair or certificate hash.

If the reason is that we want to easily cover all certificates of a CA, can you explain (briefly) why the key ID is not a good identifier? I guess the key ID is more appropriate for a blacklist of compromised or mis-issued certificates, and here we want to constrain all good, valid certificates of a CA, so the subject name is more appropriate. Correct?

@@ +1568,5 @@
> + *                       should be applied
> + *  * ${CA}_NAME_CONSTRAINTS - The name constraints extension
> + *
> + * Both of these are stored in DER-encoded form so that they can easily
> + * be consumed by validation code.

This comment (lines 1571-1572) is more appropriate for the header file. I wondered about how I could use the returned extension in binary-encoded form when I read the function declaration. So, I suggest we move this comment to cert.h.

@@ +1625,2 @@
>  
> +    size_t i;

IMPORTANT: please declare this variable at the beginning of the function (assuming a pre-C99 compiler).

::: lib/nss/nss.def
@@ +1061,5 @@
>  PK11_PrivDecrypt;
>  ;+    local:
>  ;+       *;
>  ;+};
> +;+NSS_3.18 { 	# NSS 3.18 release

Please change this to "NSS_3.18.1" and "NSS 3.18.1". Sorry about my late review.

Comment 18

[Richard Barnes [:rbarnes]]

Attachment: bug-991783.4.patch r=wtc

(In reply to Wan-Teh Chang from comment #17)
> 1. In the comment for CERT_FindNameConstraintsExten, please add a
> note to point out that the function will return imposed name
> constraints.

Updated in cert.h and genname.c.


> 2. I assume you are adding the new CERT_GetImposedNameConstraints
> function for mozilla::pkix. If mozilla::pkix is using the
> CERT_FindNameConstraintsExten, then the new function is not
> needed. If mozilla::pkix is calling
>
>     CERT_FindCertExtension(cert, SEC_OID_X509_NAME_CONSTRAINTS,
>                            &constraintsExtension)
>
> to get the binary-encoded name constraints extension, then an
> alternative solution is to change CERT_FindCertExtension() to
> use the imposed name constraints. I am worried that can be
> confusing because we won't be able to check if a certificate
> has a name constraints extension. So this is probably a bad
> idea.

I agree.  I have my doubts about even CERT_FindNameConstraintsExten, but that method is already loading the ANSSI name constraints, so it seems harmless to continue that pattern.

My plan is to call CERT_GetImposedNameConstraints in mozilla::pkix, in bug 1121982, where the ANSSI constraints get inserted today.
https://dxr.mozilla.org/mozilla-central/source/security/certverifier/NSSCertDBTrustDomain.cpp#127


> ::: lib/certdb/cert.h
> @@ +1177,5 @@
> >
> >  extern CERTGeneralName *
> >  CERT_GetPrevGeneralName(CERTGeneralName *current);
> >
> > +/* Provide name constraints for some certs that do not include name constraints
>
> Nit: "provide" could be misinterpreted to mean "add" or "impose".

Changed to "look up".


> @@ +1187,5 @@
> > + * The data in extensions will point to memory that the caller does not own. Do
> > + * not free it.
> > + */
> > +SECStatus
> > +CERT_GetImposedNameConstraints(const SECItem *derSubject, SECItem *extensions);
>
> When I read this, I am wondering: what function should an application call
> to impose name constraints on a certificate (or rather, certificate
> subject)? If all the imposed name constraints are built into NSS, it would
> be nice to document that.

Added a comment to clarify that the only imposed constraints are built in.


> ::: lib/certdb/genname.c
> @@ +1557,5 @@
> >  }
> >
> > +/*
> > + * Here we define a list of name constraints to be imposed on
> > + * certain certificates, most importantly root certificates.
>
> Nit: it would be nice to explain why this list identifies a certificate by
> the subject name rather than some unique identifier, such as the [issuer
> name, serial number] pair or certificate hash.
>
> If the reason is that we want to easily cover all certificates of a CA, can
> you explain (briefly) why the key ID is not a good identifier? I guess the
> key ID is more appropriate for a blacklist of compromised or mis-issued
> certificates, and here we want to constrain all good, valid certificates of
> a CA, so the subject name is more appropriate. Correct?

Pretty much.  I got here because subject names were how we were doing things already, but it still makes sense to me.  Added a comment.

Comment 19

[Richard Barnes [:rbarnes]]

Attachment: bug-991783.3-4.interdiff

Comment 20

[Martin Thomson [:mt:]]

Comment on attachment 8591252 [details] [diff] [review]
bug-991783.4.patch r=wtc

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

Where are the unit tests?

(I can check this in on Monday if no one else does.)

::: lib/certdb/genname.c
@@ +1643,3 @@
>  }
>  
> +/* 

Trailing space.

Comment 21

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/07da6d86695a

Comment 22

[Wan-Teh Chang]

Attachment: Change the symbol version of CERT_GetImposedNameConstraints to NSS_3.19

Kai, you can check in this patch for me if it looks good. Thanks.

Comment 23

[Kai Engert (:KaiE:)]

Comment on attachment 8599020 [details] [diff] [review]
Change the symbol version of CERT_GetImposedNameConstraints to NSS_3.19

Wan-Teh, thanks a lot for noticing!

r=kaie

Comment 24

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/8a805656e9a3