790809 - Add libpkix certificate verification callback framework to Certverify

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

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

+++ This bug was initially created as a clone of Bug #744204 +++

In bug 764973, we are adding a callback mechanism to libpkix that allows us to augment libpkix's certificate chain checking with our own additional checks. This is needed by bug 744204, bug 360126, and other bugs, so Camilo is going to split out a dummy "return SECSuccess;" callback from his patches in bug 744204 so that all the dependent work can be done concurrently.

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: interface-part1

assumes patches for bug 764973 to be applied for this patch to work. It also assumes that we always call the callback in a silly if(1==1) condition.

Comment 2

[Camilo Viecco (:cviecco)]

Attachment: interface-part2

Silly me I was using externs, but these are not needed (at least it works on Linux and compiles on the rest) see:  https://tbpl.mozilla.org/?tree=Try&rev=470209c8aff7

Comment 3

[Camilo Viecco (:cviecco)]

Attachment: full-implementation-removed last ref to pinning

Separation of psm interface from the other implementaiton. The only issue that could be troublesome is the use of a 'if(1==1)' condition to help document the case where callback is not needed.

Comment 4

[Camilo Viecco (:cviecco)]

Attachment: proposed-patch-v2

Comment 5

[Camilo Viecco (:cviecco)]

Attachment: proposed-patch-v2.1

Comment 6

[Camilo Viecco (:cviecco)]

Attachment: psm-chainvalitate-interface v 3 (fix bitrot)

Comment 7

[Camilo Viecco (:cviecco)]

After taking a look at Bug 481656 and Bug 650307. It seems like we should have the callback available from other places of the code. So maybe the callback should reside within 

nsCERTValInParamWrapper.cpp

where a 'constructed' nsCERTValInParamWrapper would have three (optional) params:
hostname, timetocheck, and isEV. 

This way we can al last guarantee that we will be using the same chain wherever we calculate it.

Comment 8

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

(In reply to Camilo Viecco (:cviecco) from comment #7)
> After taking a look at Bug 481656 and Bug 650307. It seems like we should
> have the callback available from other places of the code. So maybe the
> callback should reside within 
> 
> nsCERTValInParamWrapper.cpp
> 
> where a 'constructed' nsCERTValInParamWrapper would have three (optional)
> params:
> hostname, timetocheck, and isEV. 
> 
> This way we can al last guarantee that we will be using the same chain
> wherever we calculate it.

The idea of nsCERTValInParamWrapper is that it is just a cache of the preferences that affect certificate validation. It turns out it wasn't a good idea to cache those values by pre-constructing the CERTValInParams. Instead, we should just store the preferences themselves, and construct a CERTValInParams on the stack before we call CERT_VerifyCert.

Once that is done, then we can create a single VerifyCert function that has the necessary parameters (like the ones you suggested, but not hostname, because we don't always have a hostname when we're validating an SSL certificate; e.g. when we're viewing an SSL certificate from the certificate manager), and which decides whether to call CERT_VerifyCert or CERT_PKIXVerifyCert based on the libpkix preference. Then, we can add the callback logic to just that one VerifyCert function.

Comment 9

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

Comment on attachment 676236 [details] [diff] [review]
psm-chainvalitate-interface v 3 (fix bitrot)

After bug 813418, we will have only one place where we call CERT_PKIXVerifyCert. So, the patch definitely needs to be updated for that.

Within CertVerifier::VerifyCert, we will need to have a callback that is called for every verification to do things like key size checks.

I don't think it is a good idea to create a new CertVerifier for every verification that requires a callback. However, it may be reasonable to create a new CertVerifier instance sometimes, such as for the key pinning stuff; e.g. by creating a new CertVerifier constructor like this:

   CertVerifier(const CertVerifier & base, const KeyPinningInfo & pinningInfo)
       : mOCSPDownloadEnabled(base.mOCSPDownloadEnabled)
       , ....
       , mPinningInfo(pinningInfo)
   {
   }

So, generally the design seems reasonable but we should avoid adding the new copy-constructor-like constructor until we actually need it (e.g. in the key pinning bug).

Comment 10

[Camilo Viecco (:cviecco)]

Attachment: interface post centralization

Comment 11

[Camilo Viecco (:cviecco)]

Comment on attachment 711018 [details] [diff] [review]
interface post centralization

This version does not have a fake constructor, instead the state is kept on the stack.

Comment 12

[Florian Bender]

Camillo, are you still working on this? Any ETA?

Comment 13

[Camilo Viecco (:cviecco)]

Hello Florian, the goal now is to stop depending on libpkix and use insanity::pkix. Depending on the timing of insanity this will be done or will be marked not to do.

Comment 14

[Camilo Viecco (:cviecco)]

Attachment: add-callback-for-final-check-verify-libpkix

Comment 15

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

Comment on attachment 8367649 [details] [diff] [review]
add-callback-for-final-check-verify-libpkix

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

Looks good. Just a few items:
- let's skip the state argument for now if we can
- type*, not type *

::: security/certverifier/CertVerifier.cpp
@@ +91,5 @@
>    return SECSuccess;
>  }
>  
> +typedef struct {
> +   //just an example for now

Can we just skip this for now and have the state argument be null until we need it?

@@ +96,5 @@
> +   char *inHostname;
> +}chainValidationCallbackState_t;
> +
> +SECStatus chainValidationCallback(void *state, const CERTCertList *certList,
> +                                  PRBool *chainOK)

void*, CERTCertList*, etc.

@@ +99,5 @@
> +SECStatus chainValidationCallback(void *state, const CERTCertList *certList,
> +                                  PRBool *chainOK)
> +{
> +  chainValidationCallbackState_t *inState = nullptr;
> +  inState = (chainValidationCallbackState_t *)state;

Let's just skip the state argument for now.

@@ +104,5 @@
> +  *chainOK = PR_FALSE;
> +
> +  PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, ("verifycert: Inside the Callback \n"));
> +
> +  //on Sanity failure we fail closed.

nit: space after //

@@ +105,5 @@
> +
> +  PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, ("verifycert: Inside the Callback \n"));
> +
> +  //on Sanity failure we fail closed.
> +  if (NULL == certList) {

nullptr, not NULL. Also, I prefer (certList == nullptr)

@@ +107,5 @@
> +
> +  //on Sanity failure we fail closed.
> +  if (NULL == certList) {
> +    PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, ("verifycert: Short circuit, callback, "
> +                                      "sanity check failed \n"));

indentation/alignment

@@ +120,5 @@
> +  }
> +  *chainOK = PR_TRUE;
> +  return SECSuccess;
> +}
> +

extra empty line

Comment 16

[Camilo Viecco (:cviecco)]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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