Add insanity::pkix as certificate verification option.

RESOLVED FIXED in Firefox 29

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: cviecco, Assigned: briansmith)

Tracking

unspecified
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29 fixed, firefox30 fixed)

Details

(Whiteboard: [tests in bug 971178 and elsewhere])

Attachments

(2 attachments, 13 obsolete attachments)

2.38 KB, patch
cviecco
: review+
Details | Diff | Splinter Review
30.04 KB, patch
briansmith
: review+
cviecco
: review+
Details | Diff | Splinter Review
We need a modern certificate verification library to replace  the 'classic' verification code (be rfc 5280 compliant)
Comment on attachment 757623 [details] [diff] [review]
part2: import insanity::pkix into the tree

This is just an import of insanity (with changes of NULL to nullptr) and some mozilla specific build files (moz.build + Makefiles)
Posted file part 9: create insanitytrustdomain (obsolete) —
Attachment #757705 - Attachment is obsolete: true
Attachment #757690 - Flags: review?(bsmith)
Comment on attachment 757636 [details] [diff] [review]
part 3: Allow insanity to build on old gcc versions

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

::: security/manager/insanity/include/insanity/ScopedPtr.h
@@ +2,5 @@
>  #define insanity__ScopedPtr_h
>  
>  #include "plarena.h"
>  
> +//GCC does not understand nullptr until 4.6 so

space after //

@@ +4,5 @@
>  #include "plarena.h"
>  
> +//GCC does not understand nullptr until 4.6 so
> +#ifdef __GNUC__
> +#if ((__GNUC__ * 100 + __GNUC_MINOR__ < 406 ))

remove extra space after "406". (also elsewhere).

@@ +5,5 @@
>  
> +//GCC does not understand nullptr until 4.6 so
> +#ifdef __GNUC__
> +#if ((__GNUC__ * 100 + __GNUC_MINOR__ < 406 ))
> +  #define nullptr __null

don't indent this line.
Attachment #757636 - Flags: review+
(In reply to Brian Smith (:bsmith) from comment #15)
> ::: security/manager/insanity/include/insanity/ScopedPtr.h
> @@ +2,5 @@
> >  #define insanity__ScopedPtr_h
> >  
> >  #include "plarena.h"
> >  
> > +//GCC does not understand nullptr until 4.6 so
> 
> space after //
> 
> @@ +4,5 @@
> >  #include "plarena.h"
> >  
> > +//GCC does not understand nullptr until 4.6 so
> > +#ifdef __GNUC__
> > +#if ((__GNUC__ * 100 + __GNUC_MINOR__ < 406 ))
> 
> remove extra space after "406". (also elsewhere).
> 
> @@ +5,5 @@
> >  
> > +//GCC does not understand nullptr until 4.6 so
> > +#ifdef __GNUC__
> > +#if ((__GNUC__ * 100 + __GNUC_MINOR__ < 406 ))
> > +  #define nullptr __null
> 
> don't indent this line.

I made all these corrections already. no action needed.
Comment on attachment 757690 [details] [diff] [review]
part 8: NSS patch to support getting only ocsp cache data.

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

cviecco and I talked about this before. We're going to do the caching a different way.
Attachment #757690 - Flags: review?(brian)
So what's the plan here, are these patches waiting on review or are they obsolete, or what do we need to finish up this bug?
Flags: needinfo?(brian)
Clearing the NEEDINFO flag. We've already discussed this offline and also we're going to have a burndown list for this soon that will get posted to dev-tech-crypto probably next week.
Flags: needinfo?(brian)
Attachment #757621 - Attachment is obsolete: true
Attachment #757623 - Attachment is obsolete: true
Attachment #757707 - Attachment is obsolete: true
Attachment #757706 - Attachment is obsolete: true
No longer depends on: 916629
(In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) from comment #19)
> Clearing the NEEDINFO flag. We've already discussed this offline and also
> we're going to have a burndown list for this soon that will get posted to
> dev-tech-crypto probably next week.

Can you post a link to either the d-t-c thread or wherever you've put the burndown list?  Would be nice to have it here in the tracking bug.
Flags: needinfo?(brian)
No longer blocks: 915931
No longer blocks: 915931
Depends on: 915931
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #20)
> Can you post a link to either the d-t-c thread or wherever you've put the
> burndown list?  Would be nice to have it here in the tracking bug.

As we discussed last week, the bug tree for bug 915930 is basically the entire burn-down list for insanity::pkix. Bug 921907 has a more comprehensive bug tree.
Flags: needinfo?(brian)
Thanks Brian, just wanted to make sure that info was available in this bug since it seems like a tracking bug and since you mentioned there might be a thread in dev-tech-crypto, I want to capture a link somewhere (looks like bug 915930 is the right spot) for reference when it's available.
No longer depends on: 921887
Summary: Add insanity::pkix as certificate verifcation option. → Add insanity::pkix as certificate verification option.
No longer depends on: 915931
Attachment #757636 - Attachment is obsolete: true
Attachment #757642 - Attachment is obsolete: true
Attachment #757688 - Attachment is obsolete: true
Attachment #757677 - Attachment is obsolete: true
Attachment #757687 - Attachment is obsolete: true
Comment on attachment 757690 [details] [diff] [review]
part 8: NSS patch to support getting only ocsp cache data.

Camilo, I will do the OCSP caching in bug 915931.
Attachment #757690 - Attachment is obsolete: true
Assignee: nobody → brian
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla30
v2 fixes some nits that WARNINS_AS_ERRORS caught on tryserver but not locally.
Attachment #8373163 - Attachment is obsolete: true
Attachment #8373163 - Flags: review?(dkeeler)
Attachment #8373163 - Flags: review?(cviecco)
Attachment #8373224 - Flags: review?(dkeeler)
Attachment #8373224 - Flags: review?(cviecco)
Comment on attachment 8373224 [details] [diff] [review]
Part 1: Add insanity::pkix as a CertVerifier option [v2]

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

::: security/certverifier/CertVerifier.cpp
@@ +224,5 @@
> +                   const SECCertificateUsage usage,
> +                   const PRTime time,
> +                   void* pinArg,
> +                   const Flags flags,
> +  /*optional out*/ insanity::pkix::ScopedCERTCertList* validationChain)

NOTE: revocation support and dealing with cert error overrides will happen in a subsequent patch.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +944,2 @@
>    bool crlDownloading = Preferences::GetBool("security.CRL_download.enabled",
>                                               false);

aiaDownloadEnabled needs to go here too.
Comment on attachment 8373224 [details] [diff] [review]
Part 1: Add insanity::pkix as a CertVerifier option [v2]

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

No major concerns.

::: security/certverifier/CertVerifier.cpp
@@ +226,5 @@
> +                   void* pinArg,
> +                   const Flags flags,
> +  /*optional out*/ insanity::pkix::ScopedCERTCertList* validationChain)
> +{
> +

nit: unnecessary empty line

@@ +228,5 @@
> +  /*optional out*/ insanity::pkix::ScopedCERTCertList* validationChain)
> +{
> +
> +  PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
> +         ("VerifyCert: Top of insanityPKIXVerifyCert \n"));

This log string looks out of date (e.g. "InsanityVerifyCert", not "insanityPKIXVerifyCert")

@@ +251,5 @@
> +                          SEC_OID_EXT_KEY_USAGE_CLIENT_AUTH,
> +                          builtChain);
> +      break;
> +    }
> + 

nit: trailing whitespace

@@ +313,5 @@
> +                          builtChain);
> +      break;
> +    }
> +
> +    case certificateUsageVerifyCA: 

nit: trailing whitespace

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +20,5 @@
>  #include "secmod.h"
>  
>  using namespace insanity::pkix;
>  
> +#ifdef MOZ_LOGGING

MOZ_LOGGING or PR_LOGGING? I remember there were was an issue recently with this.

@@ +46,5 @@
> +//  , mOCSPStrict(ocspStrict)
> +  , mPinArg(pinArg)
> +{
> +}
> + 

nit: trailing whitespace

@@ +77,5 @@
> +    return SECFailure;
> +  }
> +
> +  // XXX: CERT_GetCertTrust's result is ambiguous. Does SECFailure mean that
> +  // it failed, or that there is no trust record?

Looks like "no trust record". But I suppose it could also indicate an internal state error:
stanpcertdb.c line 89:
89 SECStatus
90 CERT_GetCertTrust(const CERTCertificate *cert, CERTCertTrust *trust)
91 {
92     SECStatus rv;
93     CERT_LockCertTrust(cert);
94     if ( cert->trust == NULL ) {
95 	rv = SECFailure;
96     } else {
97 	*trust = *cert->trust;
98 	rv = SECSuccess;
99     }
100     CERT_UnlockCertTrust(cert);
101     return(rv);
102 }

@@ +88,5 @@
> +    PRUint32 relevantTrustBit = endEntityOrCA == MustBeCA ? CERTDB_TRUSTED_CA
> +                                                          : CERTDB_TRUSTED;
> +    if (((flags & (relevantTrustBit|CERTDB_TERMINAL_RECORD)))
> +            == CERTDB_TERMINAL_RECORD) {
> +      *trustLevel = ActivelyDistrusted;

If I'm reading this correctly, we're saying distrust is indicated by the absence of the appropriate trust bit. I think the comment could be a bit more clear in saying this.

::: security/certverifier/NSSCertDBTrustDomain.h
@@ +48,5 @@
> +
> +public:
> +  NSSCertDBTrustDomain(SECTrustType certDBTrustType,
> +                       bool ocspDownloadEnabled, bool ocspStrict,
> +                       void * pinArg);

nit: void*

@@ +57,5 @@
> +                /*out*/ insanity::pkix::ScopedCERTCertList& results);
> +
> +  virtual SECStatus GetCertTrust(insanity::pkix::EndEntityOrCA endEntityOrCA,
> +                                 const CERTCertificate* candidateCert,
> +                         /*out*/ TrustLevel * trustLevel);

nit: TrustLevel*

@@ +65,5 @@
> +
> +private:
> +  const SECTrustType mCertDBTrustType;
> +//  const bool mOCSPDownloadEnabled;
> +//  const bool mOCSPStrict;

I'm assuming these are anticipating OCSP support?

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +979,1 @@
>  #ifndef NSS_NO_LIBPKIX

Would a compiler ever complain about an empty else clause body? If so, maybe move the #ifndef NSS_NO_LIBPKIX to before the } else {.
Attachment #8373224 - Flags: review?(cviecco) → review+
Comment on attachment 8373224 [details] [diff] [review]
Part 1: Add insanity::pkix as a CertVerifier option [v2]

Whoops - wrong flag.
Attachment #8373224 - Flags: review?(dkeeler) → review?(cviecco)
Comment on attachment 8373224 [details] [diff] [review]
Part 1: Add insanity::pkix as a CertVerifier option [v2]

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

r- for the verifylog. BuildCertChainForOneKeyUsage could be better too.

::: security/certverifier/CertVerifier.cpp
@@ +193,5 @@
> +static SECStatus
> +BuildCertChainForOneKeyUsage(TrustDomain& trustDomain, CERTCertificate* cert,
> +                             PRTime time, KeyUsages ku1, KeyUsages ku2,
> +                             KeyUsages ku3, SECOidTag eku,
> +                             ScopedCERTCertList& builtChain)

Why dont you make it:
BuildCertChainForKeyUsageField(TrustDomain& trustDomain, CERTCertificate* cert,
                             PRTime time,  KeyUsageField ku
                             SECOidTag eku, ScopedCERTCertList& builtChain) {

 SECStatus rv;
 for (i=KU_DIGITAL_SIGNATURE; i!=0; i=i>>1) {
   if( i & ku == ku) {
     rv = BuildCertChain(trustDomain, cert, time, MustBeEndEntity,
		i, eku, builtChain)
     if (rv == SECSuccess) {
       return rv;
     }
   }
 }
 PR_SetError(SEC_ERROR_INADEQUATE_KEY_USAGE, 0);
 return rv;
}

(however I think this will be removed soon).

@@ +361,5 @@
> +
> +  if (validationChain && rv == SECSuccess) {
> +    *validationChain = builtChain.release();
> +  }
> +

Error is not inserted into the verifylog.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +88,5 @@
> +    PRUint32 relevantTrustBit = endEntityOrCA == MustBeCA ? CERTDB_TRUSTED_CA
> +                                                          : CERTDB_TRUSTED;
> +    if (((flags & (relevantTrustBit|CERTDB_TERMINAL_RECORD)))
> +            == CERTDB_TERMINAL_RECORD) {
> +      *trustLevel = ActivelyDistrusted;

+1
Attachment #8373224 - Flags: review?(cviecco) → review-
Comment on attachment 8373224 [details] [diff] [review]
Part 1: Add insanity::pkix as a CertVerifier option [v2]

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

I already said above that the cert error override logic is coming up in another bug.

The cert error override logic isn't needed to testing the stuff that is needed by the apps stuff, which is blocked on this bug. So, let's defer the cert error override logic to another bug.

In particular, my plan for the insanity::pkix cert error override logic is to NOT use a verify log. It will be clearer when you see the patches.
Attachment #8373224 - Flags: review- → review?(cviecco)
Attachment #8373164 - Flags: review?(cviecco) → review+
Comment on attachment 8373224 [details] [diff] [review]
Part 1: Add insanity::pkix as a CertVerifier option [v2]

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

::: security/certverifier/CertVerifier.cpp
@@ +193,5 @@
> +static SECStatus
> +BuildCertChainForOneKeyUsage(TrustDomain& trustDomain, CERTCertificate* cert,
> +                             PRTime time, KeyUsages ku1, KeyUsages ku2,
> +                             KeyUsages ku3, SECOidTag eku,
> +                             ScopedCERTCertList& builtChain)

I think your last comment is the thing. This is ugly no matter how we do it, and the proper solution is to have SSLServerCertVerification/libssl and other callers tell us which bits it wants us to check, so we don't have to do this mapping at all. I filed bug 970760 to fix the more general problem, which will result in this code being removed.

@@ +361,5 @@
> +
> +  if (validationChain && rv == SECSuccess) {
> +    *validationChain = builtChain.release();
> +  }
> +

Will be handled in a later bug.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +77,5 @@
> +    return SECFailure;
> +  }
> +
> +  // XXX: CERT_GetCertTrust's result is ambiguous. Does SECFailure mean that
> +  // it failed, or that there is no trust record?

Thanks. I verified this and updated the comment.

::: security/certverifier/NSSCertDBTrustDomain.h
@@ +65,5 @@
> +
> +private:
> +  const SECTrustType mCertDBTrustType;
> +//  const bool mOCSPDownloadEnabled;
> +//  const bool mOCSPStrict;

Yes, these are for OCSP support, and this is an artifact of refactoring and landing WARNINGS_AS_ERRORS in the middle of this series of patches. Will be fixed a couple patches from now.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +979,1 @@
>  #ifndef NSS_NO_LIBPKIX

I think things are OK. I will push to try.
Attachment #8373827 - Flags: review?(cviecco)
Attachment #8373827 - Flags: review?(cviecco) → review+
Flags: in-testsuite+
Whiteboard: [tests in bug 971178 and elsewhere]
Comment on attachment 8373164 [details] [diff] [review]
Part 2: Make CertVerifier pref dynamic

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This uplift is very important so that CAs can test the new certificate verification code in the release before it becomes the default. Asking them to switch to a prerelease channel is not realistic (based on previous interactions). Additionally, as discussed on Vidyo, we want to uplift bug 896620, which depends on this bug.

User impact if declined: Added risk to Firefox 30 release when this code will be enabled by default, due to lack of testing by CAs and others.

Testing completed (on m-c, etc.): We created a set of unit/regression tests that are either already checked in or will be checked in along with the uplift.

Risk to taking this patch (and alternatives if risky): There is very low risk to uplifting the patches in these bugs because all the code, except for the small amount of integration code in this bug, is pref'd off by default.

String or IDL/UUID changes made by this patch: None
Attachment #8373164 - Flags: approval-mozilla-aurora?
Comment on attachment 8373827 [details] [diff] [review]
Part 1: Add insanity::pkix as a CertVerifier option [v3]

[Approval Request Comment]
See comment 37.
Attachment #8373827 - Flags: approval-mozilla-aurora?
Comment on attachment 8373827 [details] [diff] [review]
Part 1: Add insanity::pkix as a CertVerifier option [v3]

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 #8373827 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8373164 [details] [diff] [review]
Part 2: Make CertVerifier pref dynamic

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 #8373164 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This got checked in with the wrong bug number:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f47a37f59752
The actual bug is bug 915931.
(In reply to Carsten Book [:Tomcat] from comment #44)
> https://hg.mozilla.org/mozilla-central/rev/f47a37f59752

This got checked in with the wrong bug number. The actual bug is bug 915931.
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #45)
> This got checked in with the wrong bug number. The actual bug is bug 915931.

Sorry, I forgot to change that wrong bug number in the commit message when I uplifted that patch to Mozilla-Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/4d6c9d219ee3
You need to log in before you can comment on or make changes to this bug.