921892 - Verify certificate basic constraints extension in insanity::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Verify certificate basic constraints extension in insanity::pkix

Comment 2

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

Comment on attachment 8370951 [details] [diff] [review]
Verify certificate basic constraints extension in insanity::pkix

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

Just a couple of nits.

::: security/insanity/lib/pkixcheck.cpp
@@ +67,5 @@
> +    // constraints as CAs.
> +    //
> +    // TODO: add check for self-signedness?
> +    if (endEntityOrCA == MustBeCA && isTrustAnchor) {
> +      const CERTCertificate * nssCert = cert.GetNSSCert();

nit: CERTCertificate*

@@ +70,5 @@
> +    if (endEntityOrCA == MustBeCA && isTrustAnchor) {
> +      const CERTCertificate * nssCert = cert.GetNSSCert();
> +
> +      der::Input versionDer;
> +      if (versionDer.Init(nssCert->version.data, nssCert->version.len)) {

!= Success?

@@ +74,5 @@
> +      if (versionDer.Init(nssCert->version.data, nssCert->version.len)) {
> +        return RecoverableError;
> +      }
> +      uint8_t version;
> +      if (der::OptionalVersion(versionDer, version) || der::End(versionDer)) {

!= Success, etc.

@@ +78,5 @@
> +      if (der::OptionalVersion(versionDer, version) || der::End(versionDer)) {
> +        return RecoverableError;
> +      }
> +      if (version == 1) {
> +        basicConstraints.isCA = false;

Shouldn't this be isCA = true?

@@ +107,5 @@
> +    return Fail(RecoverableError, SEC_ERROR_CA_CERT_INVALID);
> +  }
> +
> +  // TODO: subCACount >  basicConstraints.pathLenConstraint or
> +  //       subCACount >= basicConstraints.pathLenConstraint?

Yes, let's not forget this.

@@ +121,5 @@
> +
> +// Checks extensions that apply to both EE and intermediate certs,
> +// except for AIA, CRL, and AKI/SKI, which are handled elsewhere.
> +Result
> +CheckExtensions(BackCert & cert,

nit: BackCert&

@@ +129,5 @@
> +{
> +  // 4.2.1.1. Authority Key Identifier dealt with as part of path building
> +  // 4.2.1.2. Subject Key Identifier dealt with as part of path building
> +
> +  PLArenaPool * arena = cert.GetArena();

nit: PLArenaPool*

@@ +139,5 @@
> +
> +  // 4.2.1.3. Key Usage
> +  // 4.2.1.4. Certificate Policies
> +  // 4.2.1.5. Policy Mappings are rejected in BackCert::Init()
> +  // 4.2.1.6. Subject Alternative Name dealt with elsewhere

in path building?

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8370951 [details] [diff] [review]
Verify certificate basic constraints extension in insanity::pkix

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

::: security/insanity/lib/pkixcheck.cpp
@@ +107,5 @@
> +    return Fail(RecoverableError, SEC_ERROR_CA_CERT_INVALID);
> +  }
> +
> +  // TODO: subCACount >  basicConstraints.pathLenConstraint or
> +  //       subCACount >= basicConstraints.pathLenConstraint?

David, what do you think the answer is?

@@ +139,5 @@
> +
> +  // 4.2.1.3. Key Usage
> +  // 4.2.1.4. Certificate Policies
> +  // 4.2.1.5. Policy Mappings are rejected in BackCert::Init()
> +  // 4.2.1.6. Subject Alternative Name dealt with elsewhere

In CheckNameConstraints (bug 921896).

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment 3.

Comment 5

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

I believe it's 'subCACount >  basicConstraints.pathLenConstraint' as you have it. From my reading of the spec, the pathLenConstraint is basically the number of allowed intermediates between the current intermediate and the EE cert. From what I can see subCACount only gets incremented after the first intermediate in the chain. So, if an EE is directly issued, a pathLenConstraint of 0 works. If there is 1 intermediate, subCACount will be 1 when the root cert is checked, which results in 'subCACount > basicConstraints.pathLenConstraint' being the right check, and so on.

Comment 6

[Camilo Viecco (:cviecco)]

Comment on attachment 8370951 [details] [diff] [review]
Verify certificate basic constraints extension in insanity::pkix

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

::: security/insanity/lib/pkixcheck.cpp
@@ +43,5 @@
> +Result
> +CheckBasicConstraints(const BackCert& cert,
> +                      EndEntityOrCA endEntityOrCA,
> +                      bool isTrustAnchor,
> +                      unsigned int subCACount)

This name is a little unintuitive, but I am strugling for better name:
- currentCertTrustDepth?
- certPathDepth?
- intermediateCount?

@@ +69,5 @@
> +    // TODO: add check for self-signedness?
> +    if (endEntityOrCA == MustBeCA && isTrustAnchor) {
> +      const CERTCertificate * nssCert = cert.GetNSSCert();
> +
> +      der::Input versionDer;

Where is this der processor?

::: security/insanity/lib/pkixutil.h
@@ +102,5 @@
> +  // match against the issuer's key identifier, if it has one. |*result|
> +  // will be null if this cert does not have the auth key identifier.
> +  // The result is owned by this BackCert object and will be destroyed
> +  // when this BackCert object is destroyed.
> +  Result GetAuthorityKeyIdentifier(const SECItem** result);

Where is this?

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8370951 [details] [diff] [review]
Verify certificate basic constraints extension in insanity::pkix

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

::: security/insanity/lib/pkixcheck.cpp
@@ +43,5 @@
> +Result
> +CheckBasicConstraints(const BackCert& cert,
> +                      EndEntityOrCA endEntityOrCA,
> +                      bool isTrustAnchor,
> +                      unsigned int subCACount)

Since we use "CA" elsewhere, I think it is OK to use "subCA" here. I agree intermediateCount is also a reasonable name.

"Depth" and variations on it are bad because "depth" is ambiguous and unclear about whether or not the EE certificate is included. (I originally had named this "depth" and I renamed it after finding a bug about including EE certificates.)

@@ +69,5 @@
> +    // TODO: add check for self-signedness?
> +    if (endEntityOrCA == MustBeCA && isTrustAnchor) {
> +      const CERTCertificate * nssCert = cert.GetNSSCert();
> +
> +      der::Input versionDer;

security/insanity/pkix/lib/pikixder.[ch].

@@ +74,5 @@
> +      if (versionDer.Init(nssCert->version.data, nssCert->version.len)) {
> +        return RecoverableError;
> +      }
> +      uint8_t version;
> +      if (der::OptionalVersion(versionDer, version) || der::End(versionDer)) {

Why "etc."? Are there more places where I'm missing the != der::Success?

Note that with the explicit "!= der::Success" these lines wrap to two lines now.

@@ +78,5 @@
> +      if (der::OptionalVersion(versionDer, version) || der::End(versionDer)) {
> +        return RecoverableError;
> +      }
> +      if (version == 1) {
> +        basicConstraints.isCA = false;

I think so. I filed bug 969188 for adding a test. Adding the test for it is harder than I expected because nothing generates v1 certificates anymore, AFAICT.

@@ +107,5 @@
> +    return Fail(RecoverableError, SEC_ERROR_CA_CERT_INVALID);
> +  }
> +
> +  // TODO: subCACount >  basicConstraints.pathLenConstraint or
> +  //       subCACount >= basicConstraints.pathLenConstraint?

I removed the comment. I agree with your comment that the code is correct. I also verified that the code is correct by changing the "subCACount >" to "subCACount >="; when I did that, the test_intermediate_basic_constraints.js test fails.

::: security/insanity/lib/pkixutil.h
@@ +102,5 @@
> +  // match against the issuer's key identifier, if it has one. |*result|
> +  // will be null if this cert does not have the auth key identifier.
> +  // The result is owned by this BackCert object and will be destroyed
> +  // when this BackCert object is destroyed.
> +  Result GetAuthorityKeyIdentifier(const SECItem** result);

I removed this. It was an artifact of code that I deleted.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Verify certificate basic constraints extension in insanity::pkix [v2, r=keeler]

Besides the things I noted in my previous comment, I also removed the unused #include <limit>.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1829adc0388

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c1829adc0388

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8372006 [details] [diff] [review]
Verify certificate basic constraints extension in insanity::pkix [v2, r=keeler]

[Approval Request Comment]
See bug 878932 comment 37.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8372006 [details] [diff] [review]
Verify certificate basic constraints extension in insanity::pkix [v2, r=keeler]

Uplifted granted to the patches relative to the new feature: "Add insanity::pkix as certificate verification option"
Lukas and I discussed with Brian and we think it is important to have this feature for 29 (but disabled by default).
It is early in the aurora process and they have plenty of tests for these feature (and to make sure that the current behaviors are still performing correctly).

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac6d341c6d17