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

RESOLVED FIXED in Firefox 29

Status

()

Core
Security: PSM
P1
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 fixed, firefox30 fixed)

Details

Attachments

(3 attachments)

Comment hidden (empty)
Blocks: 921892
Depends on: 921887
Depends on: 895601, 877376
Depends on: 921890
Severity: normal → enhancement
Priority: -- → P1
Target Milestone: --- → mozilla29
Created attachment 8363501 [details] [diff] [review]
Part 1: Add insanity::pkix::Result
Attachment #8363501 - Flags: review?(dkeeler)
Attachment #8363501 - Flags: review?(cviecco)
Created attachment 8363502 [details] [diff] [review]
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.
Attachment #8363502 - Flags: review?(dkeeler)
Attachment #8363502 - Flags: review?(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.
Attachment #8363501 - Flags: review?(cviecco) → review+
Comment on attachment 8363502 [details] [diff] [review]
Part 2: Add TrustDomain

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

types look good.
Attachment #8363502 - Flags: review?(cviecco) → review+
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?
Attachment #8363501 - Flags: review?(dkeeler) → review+
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).
Attachment #8363502 - Flags: review?(dkeeler) → review+
(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.
(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.
Created attachment 8370948 [details] [diff] [review]
Part 3: Add basic building and verification
Attachment #8370948 - Flags: review?(dkeeler)
Attachment #8370948 - Flags: review?(cviecco)
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?)
Attachment #8370948 - Flags: review?(dkeeler) → review+
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.
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
Whiteboard: [leave open]
Target Milestone: mozilla29 → mozilla30
Comment on attachment 8363501 [details] [diff] [review]
Part 1: Add insanity::pkix::Result

Review comments addressed, folded with Part 2, and checked in.
Attachment #8363501 - Flags: checkin+
Comment on attachment 8363502 [details] [diff] [review]
Part 2: Add TrustDomain

Review comments addressed, folded with Part 1, and checked in.
Attachment #8363502 - Flags: checkin+
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 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.)
https://hg.mozilla.org/mozilla-central/rev/62bc350bf158
https://hg.mozilla.org/mozilla-central/rev/62bc350bf158
Attachment #8370948 - Flags: review?(cviecco) → review+
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b87641dd2f8
Whiteboard: [leave open]
Attachment #8370948 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/3b87641dd2f8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Comment on attachment 8363501 [details] [diff] [review]
Part 1: Add insanity::pkix::Result

[Approval Request Comment]
See bug 878932 comment 37.
Attachment #8363501 - Flags: approval-mozilla-aurora?
Comment on attachment 8363502 [details] [diff] [review]
Part 2: Add TrustDomain

[Approval Request Comment]
See bug 878932 comment 37.
Attachment #8363502 - Flags: approval-mozilla-aurora?
Comment on attachment 8370948 [details] [diff] [review]
Part 3: Add basic building and verification

[Approval Request Comment]
See bug 878932 comment 37.
Attachment #8370948 - Flags: approval-mozilla-aurora?
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).
Attachment #8363501 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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).
Attachment #8363502 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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).
Attachment #8370948 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/ad6fe1d8cdd6
https://hg.mozilla.org/releases/mozilla-aurora/rev/3bac1caee5a1
status-firefox29: --- → fixed
status-firefox30: --- → fixed
You need to log in before you can comment on or make changes to this bug.