878932 - Add insanity::pkix as certificate verification option.

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Camilo Viecco (:cviecco)]

We need a modern certificate verification library to replace  the 'classic' verification code (be rfc 5280 compliant)

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: part 1. NSS-patch-to-allow-insanity::pkix

Comment 2

[Camilo Viecco (:cviecco)]

Attachment: part2: import insanity::pkix into the tree

Comment 3

[Camilo Viecco (:cviecco)]

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)

Comment 4

[Camilo Viecco (:cviecco)]

Attachment: part 3: Allow insanity to build on old gcc versions

Comment 5

[Camilo Viecco (:cviecco)]

Attachment: part 4: update insanity::pkix, do extension checks for all certs.

Comment 6

[Camilo Viecco (:cviecco)]

Attachment: part 5: insanity do basic constrains checks for v3plus only certs

Comment 7

[Camilo Viecco (:cviecco)]

Attachment: part 6: version exception only for root certificates.

Comment 8

[Camilo Viecco (:cviecco)]

Attachment: part 7: insanity::pkix fix cert extension loading

Comment 9

[Camilo Viecco (:cviecco)]

Attachment: part 8: NSS patch to support getting only ocsp cache data.

Comment 10

[Camilo Viecco (:cviecco)]

Attachment: part 9: create insanitytrustdomain

Comment 11

[Camilo Viecco (:cviecco)]

Attachment: part 9: create insanitytrustdomain

Comment 12

[Camilo Viecco (:cviecco)]

Attachment: part 10: integration with certverify

Comment 13

[Camilo Viecco (:cviecco)]

https://tbpl.mozilla.org/?tree=Try&rev=5698d40755e9

Comment 14

[Camilo Viecco (:cviecco)]

https://tbpl.mozilla.org/?tree=Try&rev=d7f95a3a649c

Comment 15

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

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.

Comment 16

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

(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 17

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

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.

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

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?

Comment 19

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

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.

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Brian Smith (:briansmith, was :[e-mail]) 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.

Comment 21

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

(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.

Comment 22

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 23

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

https://tbpl.mozilla.org/?tree=Try&rev=5b76e49ef39e

Comment 24

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

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.

Comment 25

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

Attachment: Part 1: Add insanity::pkix as a CertVerifier option

Comment 26

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

Attachment: Part 2: Make CertVerifier pref dynamic

Comment 27

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

Attachment: Part 1: Add insanity::pkix as a CertVerifier option [v2]

v2 fixes some nits that WARNINS_AS_ERRORS caught on tryserver but not locally.

Comment 28

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

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 29

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

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 {.

Comment 30

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

Comment on attachment 8373224 [details] [diff] [review]
Part 1: Add insanity::pkix as a CertVerifier option [v2]

Whoops - wrong flag.

Comment 31

[Camilo Viecco (: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

Comment 32

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

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.

Comment 33

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

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.

Comment 34

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

Attachment: Part 1: Add insanity::pkix as a CertVerifier option [v3]

Comment 35

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5eece3c778aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/e10dee58dd74

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5eece3c778aa
https://hg.mozilla.org/mozilla-central/rev/e10dee58dd74

Comment 37

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

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

Comment 38

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

Comment on attachment 8373827 [details] [diff] [review]
Part 1: Add insanity::pkix as a CertVerifier option [v3]

[Approval Request Comment]
See comment 37.

Comment 39

[Sylvestre Ledru [:Sylvestre]]

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).

Comment 40

[Sylvestre Ledru [:Sylvestre]]

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).

Comment 41

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/676c7d391cd5

Comment 42

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/0939d42b51df

Comment 43

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

This got checked in with the wrong bug number:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f47a37f59752
The actual bug is bug 915931.

Comment 44

[Carsten Book [:Tomcat]]

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

Comment 45

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

(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.

Comment 46

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

(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