921891 - Add path building and verification of non-extension certificate fields to insanity::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Comment 1

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

Attachment: Part 1: Add insanity::pkix::Result

Comment 2

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

Attachment: Part 2: Add TrustDomain

Note: Part 1 has some Windows line endings, I guess. This patch corrects that. I will check Part 1 and Part 2 in at the same time, so don't worry about that part.

Comment 3

[Camilo Viecco (:cviecco)]

Comment on attachment 8363501 [details] [diff] [review]
Part 1: Add insanity::pkix::Result

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

Looks like I have already seen this.

Comment 4

[Camilo Viecco (:cviecco)]

Comment on attachment 8363502 [details] [diff] [review]
Part 2: Add TrustDomain

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

types look good.

Comment 5

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

Comment on attachment 8363501 [details] [diff] [review]
Part 1: Add insanity::pkix::Result

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

Looks like parts of this patch is redundant with some of the others, but as long as that gets resolved, this looks fine.

::: security/insanity/lib/pkixutil.h
@@ +20,5 @@
> +
> +#include "insanity/pkixtypes.h"
> +#include "seccomon.h"
> +#include "secerr.h"
> +#include "prerror.h"

nit: include sorting

@@ +24,5 @@
> +#include "prerror.h"
> +
> +namespace insanity { namespace pkix {
> +
> +enum Result

Wasn't this already in another file? Anyway, as long as we don't end up with multiple definitions of this type, it's fine.

@@ +64,5 @@
> +      return FatalError;
> +  }
> +
> +  // TODO: PORT_Assert(false); // we haven't classified the error yet
> +  return RecoverableError;

Shouldn't this be FatalError?

Comment 6

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

Comment on attachment 8363502 [details] [diff] [review]
Part 2: Add TrustDomain

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

I think this is a good interface. I'm wondering how issues like blocking on OCSP fetching is going to fit into this (or is that what you mean by non-extension certificate fields?), but maybe I just haven't seen that code yet.

::: security/insanity/include/insanity/pkixtypes.h
@@ +32,5 @@
>  typedef ScopedPtr<CERTCertList, CERT_DestroyCertList> ScopedCERTCertList;
>  typedef ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey>
>          ScopedSECKEYPublicKey;
>  
> +typedef unsigned int KeyUsages;

It doesn't look like this is being used (yet?)

@@ +83,5 @@
> +  TrustDomain() { }
> +
> +private:
> +  TrustDomain(const TrustDomain&) /* = delete */;
> +  void operator=(const TrustDomain&) /* = delete */;

Again, I think there's a MOZ_DELETE that might be useful here (although, I guess if you're trying to not depend on mfbt, nevermind).

Comment 7

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

(In reply to David Keeler (:keeler) from comment #5)
> @@ +24,5 @@
> > +#include "prerror.h"
> > +
> > +namespace insanity { namespace pkix {
> > +
> > +enum Result
> 
> Wasn't this already in another file? Anyway, as long as we don't end up with
> multiple definitions of this type, it's fine.

insanity::der::Result (2 states) is different than insanity::pkix::Result (3 states). insanity::der::Result doesn't have the concept of "recoverable error".

> 
> @@ +64,5 @@
> > +      return FatalError;
> > +  }
> > +
> > +  // TODO: PORT_Assert(false); // we haven't classified the error yet
> > +  return RecoverableError;
> 
> Shouldn't this be FatalError?

I filed follow-up bug 965011 for that. Right now, there are a lot of recoverable errors that aren't listed. We'd need to do some research to more closely classify which errors are fatal vs. recoverable.

Comment 8

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

(In reply to David Keeler (:keeler) from comment #6)
> I think this is a good interface. I'm wondering how issues like blocking on
> OCSP fetching is going to fit into this (or is that what you mean by
> non-extension certificate fields?), but maybe I just haven't seen that code
> yet.

Right. There will be a separate patch for each extension. OCSP URIs are listed in the AIA extension. OCSP is a series of patches. Just like in CERT_VerifyCert and CERT_PKIXVerifyCert, OCSP fetching will block the entire verification. That is why we have the SSLServerCertVerification thread pool. A non-blocking interface for revocation would be great, but it would make the code an unreadable mess; attempting to create such a non-blocking interface is part of what makes libpkix so difficult to read.

> ::: security/insanity/include/insanity/pkixtypes.h
> @@ +32,5 @@
> >  typedef ScopedPtr<CERTCertList, CERT_DestroyCertList> ScopedCERTCertList;
> >  typedef ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey>
> >          ScopedSECKEYPublicKey;
> >  
> > +typedef unsigned int KeyUsages;
> 
> It doesn't look like this is being used (yet?)

Yes, I should have moved it into a separate bug, but I don't want to spend all our time doing these minor refactorings.

> Again, I think there's a MOZ_DELETE that might be useful here (although, I
> guess if you're trying to not depend on mfbt, nevermind).

Right, I'm trying to avoid dependencies other than NSPR and NSS inside security/insanity.

Comment 9

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

Attachment: Part 3: Add basic building and verification

Comment 10

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

Comment on attachment 8370948 [details] [diff] [review]
Part 3: Add basic building and verification

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

This looks good to me.

::: security/certverifier/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  UNIFIED_SOURCES += [
> +    '../insanity/lib/pkixbuild.cpp',
> +    '../insanity/lib/pkixcheck.cpp',

This seems a little weird, but it's definitely something we can fix in a follow-up: shouldn't insanity take care of building itself? (i.e. have its own moz.build, etc.)

::: security/insanity/include/insanity/pkix.h
@@ +23,4 @@
>  
>  namespace insanity { namespace pkix {
>  
> +SECStatus BuildCertChain(TrustDomain& trustDomain,

A bit of documentation would be good, I think.

::: security/insanity/lib/pkixbuild.cpp
@@ +76,5 @@
> +      if (*out) {
> +        // Duplicate extension
> +        return Fail(RecoverableError, SEC_ERROR_EXTENSION_VALUE_INVALID);
> +      }
> +      *out = &ext->value;

This whole function is subtle. I think I understand it, but we should be careful here and make sure we have good test coverage.

@@ +133,5 @@
> +
> +  unsigned int newSubCACount = subCACount;
> +  if (endEntityOrCA == MustBeCA) {
> +    newSubCACount = subCACount + 1;
> +  } else {

What's the other option here? "MustBeEE"? Maybe we should make it explicit.

@@ +190,5 @@
> +    results = CERT_NewCertList();
> +    if (!results) {
> +      return FatalError;
> +    }
> +    rv = subject.PrependNSSCertToList(results.get());

Does the auto-pointer class not have the right operator/whatever magic to auto-convert results to a CERTCertList*?

@@ +236,5 @@
> +    return SECFailure;
> +  }
> +
> +  // The only non-const operation on the cert we are allowed to do is
> +  // CERT_DupCertificate.

So can we have it be a const CERTCertificate* and cast away the const-ness when we call CERT_DupCertificate? Or is that the less desirable option?

@@ +267,5 @@
> +{
> +  if (!arena) {
> +    arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
> +  }
> +  return arena.get();

Again, do we not get auto-conversion of the pointer? (Am I making this up? Was this ever a thing?)

Comment 11

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

Comment on attachment 8370948 [details] [diff] [review]
Part 3: Add basic building and verification

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

::: security/certverifier/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  UNIFIED_SOURCES += [
> +    '../insanity/lib/pkixbuild.cpp',
> +    '../insanity/lib/pkixcheck.cpp',

I agree it is unusual, but it is less work for us to maintain since we don't need the separate moz.build. Also, it should be slightly faster w.r.t. build times.

::: security/insanity/include/insanity/pkix.h
@@ +23,4 @@
>  
>  namespace insanity { namespace pkix {
>  
> +SECStatus BuildCertChain(TrustDomain& trustDomain,

The later patches change this interface. In order to avoid a bunch of merge conflicts caused by the added documentation, I will add the documentation after those patches land; I filed bug 968451 for that.

::: security/insanity/lib/pkixbuild.cpp
@@ +133,5 @@
> +
> +  unsigned int newSubCACount = subCACount;
> +  if (endEntityOrCA == MustBeCA) {
> +    newSubCACount = subCACount + 1;
> +  } else {

I could add a PR_ASSERT(endEntityOrCA == MustBeEndEntity). However, I think those assertions would quickly get annoying. I chose the endEntityOrCA name to hint that this is a binary choice. I avoided using bool because it wouldn't be obvious to the callers whether "true" ment isCA or isEndEntity. Do you think it isn't clear enough?

@@ +190,5 @@
> +    results = CERT_NewCertList();
> +    if (!results) {
> +      return FatalError;
> +    }
> +    rv = subject.PrependNSSCertToList(results.get());

The insanity::pkix ScopedPtr<T> class was designed to be a subset of std::unique_ptr. std::unique_ptr doesn't have an automatic conversion. See also bug 896648 and probably other duplicate bugs.

@@ +236,5 @@
> +    return SECFailure;
> +  }
> +
> +  // The only non-const operation on the cert we are allowed to do is
> +  // CERT_DupCertificate.

I think we should just leave it as it is. It is ugly but all the stuff dealing with CERTCertificate* is ugly. It would be better to spend time on removing the rest of the CERTCertificate dependencies.

Comment 12

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

I (accidentally) pushed Part 1 and Part 2, folded together, to inbound prematurely. But, it should be OK to just leave it:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62bc350bf158

Comment 13

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

Comment on attachment 8363501 [details] [diff] [review]
Part 1: Add insanity::pkix::Result

Review comments addressed, folded with Part 2, and checked in.

Comment 14

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

Comment on attachment 8363502 [details] [diff] [review]
Part 2: Add TrustDomain

Review comments addressed, folded with Part 1, and checked in.

Comment 15

[Camilo Viecco (:cviecco)]

Comment on attachment 8370948 [details] [diff] [review]
Part 3: Add basic building and verification

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

::: security/insanity/lib/pkixbuild.cpp
@@ +49,5 @@
> +        ext->id.data[0] == 0x55 && ext->id.data[1] == 0x1d) {
> +      // { id-ce x }
> +      switch (ext->id.data[2]) {
> +        case 14: out = &dummyEncodedSubjectKeyIdentifier; break; // bug 965136
> +        case 35: out = &dummyEncodedAuthorityKeyIdentifier; break; // bug 965136

what about other values? default should be an error?

@@ +61,5 @@
> +      switch (ext->id.data[8]) {
> +        // We should remember the value of the encoded AIA extension here, but
> +        // since our TrustDomain implementations get the OCSP URI using
> +        // CERT_GetOCSPAuthorityInfoAccessLocation, we currently don't need to.
> +        case 1: out = &dummyEncodedAuthorityInfoAccess; break;

Same here, default should be error correct?

@@ +66,5 @@
> +      }
> +    } else if (ext->critical.data && ext->critical.len > 0) {
> +      // The only valid explicit value of the critical flag is TRUE because
> +      // it is defined as BOOLEAN DEFAULT FALSE, so we just assume it is true.
> +      return Fail(RecoverableError, SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION);

recoverable? I tought that should be fatal (at least for EE certs)

@@ +254,5 @@
> +
> +  // Build the cert chain even if the cert is expired, because we would
> +  // rather report the untrusted issuer error than the expired error.
> +  if (CheckTimes(ee.GetNSSCert(), time) != Success) {
> +    PR_SetError(SEC_ERROR_EXPIRED_CERTIFICATE, 0);

What about cerits that are not valid yet?

::: security/insanity/lib/pkixcheck.cpp
@@ +25,5 @@
> +CheckTimes(const CERTCertificate* cert, PRTime time)
> +{
> +  PR_ASSERT(cert);
> +
> +  SECCertTimeValidity validity = CERT_CheckCertValidTimes(cert, time, false);

CERT_CheckCertValidTimes checks if there is an override in the cert. Is that what we want? should we use  CERT_GetCertTimes instead?

Comment 16

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

Comment on attachment 8370948 [details] [diff] [review]
Part 3: Add basic building and verification

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

::: security/insanity/lib/pkixbuild.cpp
@@ +49,5 @@
> +        ext->id.data[0] == 0x55 && ext->id.data[1] == 0x1d) {
> +      // { id-ce x }
> +      switch (ext->id.data[2]) {
> +        case 14: out = &dummyEncodedSubjectKeyIdentifier; break; // bug 965136
> +        case 35: out = &dummyEncodedAuthorityKeyIdentifier; break; // bug 965136

The default should be an error if the extension is marked "critical". Otherwise, we should ignore extensions we don't understand.

Note that later patches in this queue fill this in with KU, EKU, etc.

Note that, in particular, we "understand" SKI and AKI even though we ignore them. Ignoring SKI and AKI is a valid way of processing them, since they are purely a performance optimization. The only reason we need to handle them specially here is to avoid failing when they are marked "critical".

@@ +61,5 @@
> +      switch (ext->id.data[8]) {
> +        // We should remember the value of the encoded AIA extension here, but
> +        // since our TrustDomain implementations get the OCSP URI using
> +        // CERT_GetOCSPAuthorityInfoAccessLocation, we currently don't need to.
> +        case 1: out = &dummyEncodedAuthorityInfoAccess; break;

We assume that the TrustDomain  will, eventually, use CERT_GetOCSPAuthorityInfoAccessLocation and so we "understand" AIA.

It may have been better for me to put this handling of AIA into bug 915931, however since we're going to land bug 915931 before using insanity::pkix for anything that matters, I suggest we avoid shuffling lines of these patches around unnecessarily.

@@ +66,5 @@
> +      }
> +    } else if (ext->critical.data && ext->critical.len > 0) {
> +      // The only valid explicit value of the critical flag is TRUE because
> +      // it is defined as BOOLEAN DEFAULT FALSE, so we just assume it is true.
> +      return Fail(RecoverableError, SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION);

Good question. I should document this. If an unknown critical extension is found in a CA certificate, then we should keep trying to find an alternate path, so this needs to be RecoverableError. If an unknown critical extension is found in an EE certificate, we won't keep going so it doesn't matter if this is RecoverableError or FatalError. In general, the distinction between RecoverableError and FatalError doesn't matter when we're checking an end-entity certificate since RecoverableError is treated identically to FatalError there, but the distinction is important for CA certificates.

@@ +254,5 @@
> +
> +  // Build the cert chain even if the cert is expired, because we would
> +  // rather report the untrusted issuer error than the expired error.
> +  if (CheckTimes(ee.GetNSSCert(), time) != Success) {
> +    PR_SetError(SEC_ERROR_EXPIRED_CERTIFICATE, 0);

That will also result in SEC_ERROR_EXPIRED_CERTIFICATE. I filed a follow-up bug, bug 968560, to enhance this. It doesn't seem like something that needs to be done urgently.

::: security/insanity/lib/pkixcheck.cpp
@@ +25,5 @@
> +CheckTimes(const CERTCertificate* cert, PRTime time)
> +{
> +  PR_ASSERT(cert);
> +
> +  SECCertTimeValidity validity = CERT_CheckCertValidTimes(cert, time, false);

CERT_CheckCertValidTimes only allows overrides if the third parameter is true. Since we don't want overrides, I pass false to avoid the override logic.

(Eventually, insanity::pkix should be made to avoid any CERT_* functions. Not a priority now.)

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62bc350bf158

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62bc350bf158

Comment 19

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

(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #11)
> ::: security/certverifier/moz.build
> @@ +5,5 @@
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> >  UNIFIED_SOURCES += [
> > +    '../insanity/lib/pkixbuild.cpp',
> > +    '../insanity/lib/pkixcheck.cpp',
> 
> I agree it is unusual, but it is less work for us to maintain since we don't
> need the separate moz.build. Also, it should be slightly faster w.r.t. build
> times.

Ok.

> ::: security/insanity/include/insanity/pkix.h
> @@ +23,4 @@
> >  
> >  namespace insanity { namespace pkix {
> >  
> > +SECStatus BuildCertChain(TrustDomain& trustDomain,
> 
> The later patches change this interface. In order to avoid a bunch of merge
> conflicts caused by the added documentation, I will add the documentation
> after those patches land; I filed bug 968451 for that.

Sounds good.

> ::: security/insanity/lib/pkixbuild.cpp
> @@ +133,5 @@
> > +
> > +  unsigned int newSubCACount = subCACount;
> > +  if (endEntityOrCA == MustBeCA) {
> > +    newSubCACount = subCACount + 1;
> > +  } else {
> 
> I could add a PR_ASSERT(endEntityOrCA == MustBeEndEntity). However, I think
> those assertions would quickly get annoying. I chose the endEntityOrCA name
> to hint that this is a binary choice. I avoided using bool because it
> wouldn't be obvious to the callers whether "true" ment isCA or isEndEntity.
> Do you think it isn't clear enough?

Eh, nevermind. I think it's fine.

> @@ +236,5 @@
> > +    return SECFailure;
> > +  }
> > +
> > +  // The only non-const operation on the cert we are allowed to do is
> > +  // CERT_DupCertificate.
> 
> I think we should just leave it as it is. It is ugly but all the stuff
> dealing with CERTCertificate* is ugly. It would be better to spend time on
> removing the rest of the CERTCertificate dependencies.

Ok.

Comment 20

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b87641dd2f8

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b87641dd2f8

Comment 22

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

Comment on attachment 8363501 [details] [diff] [review]
Part 1: Add insanity::pkix::Result

[Approval Request Comment]
See bug 878932 comment 37.

Comment 23

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

Comment on attachment 8363502 [details] [diff] [review]
Part 2: Add TrustDomain

[Approval Request Comment]
See bug 878932 comment 37.

Comment 24

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

Comment on attachment 8370948 [details] [diff] [review]
Part 3: Add basic building and verification

[Approval Request Comment]
See bug 878932 comment 37.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8363501 [details] [diff] [review]
Part 1: Add insanity::pkix::Result

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 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8363502 [details] [diff] [review]
Part 2: Add TrustDomain

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 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8370948 [details] [diff] [review]
Part 3: Add basic building and verification

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 28

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/ad6fe1d8cdd6
https://hg.mozilla.org/releases/mozilla-aurora/rev/3bac1caee5a1