886471 - Add Preloaded CRLSet mechanism

Status: RESOLVED DUPLICATE of bug 1024809

(Core :: Security: PSM, defect)

Description

[Mohammed Almeshekah [:meshekah]]

We need to find better ways to do revocation to avoid the security and performance limitations for the CRLs and OCSPs. This some what be similar to what Google implemented in Chrome.

Comment 1

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet

Comment 2

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet

Comment 3

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet

Comment 4

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet

Comment 5

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet V5

Comment 6

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet V6

Comment 7

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet V7

Comment 8

[Camilo Viecco (:cviecco)]

Comment on attachment 766879 [details] [diff] [review]
Implementation of the CRLSet V7

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

Good work

Things to do:
- Add some actual crls in your set to make sure your code actually works.
- Start thinking on how would you test this.
- Please fold your patches into a single patch for next review.

::: security/manager/ssl/src/CRLSet.cpp
@@ +47,5 @@
> +  }
> +  // Can this be a for loop
> +  node = CERT_LIST_HEAD(certList);
> +  parent = CERT_LIST_NEXT(node);
> +  while (!CERT_LIST_END(parent, certList)) {

Change while to for:

for(CERTCERTLISTNODE *node = CERT_LIST_HEAD(list); !CERT_LIST_END(CERT_LIST_NEXT(node),certlist); node=CERT_LIST_NEXT(node) {

@@ +48,5 @@
> +  // Can this be a for loop
> +  node = CERT_LIST_HEAD(certList);
> +  parent = CERT_LIST_NEXT(node);
> +  while (!CERT_LIST_END(parent, certList)) {
> +    rv = HASH_HashBuf(HASH_AlgSHA256, issuerHash,

Who allocates issuerHash?

@@ +50,5 @@
> +  parent = CERT_LIST_NEXT(node);
> +  while (!CERT_LIST_END(parent, certList)) {
> +    rv = HASH_HashBuf(HASH_AlgSHA256, issuerHash,
> +		      parent.derPublicKey.data,
> +		      parent.derPublicKey.len);

You need to check if rv is success.

@@ +54,5 @@
> +		      parent.derPublicKey.len);
> +    toConvert.type = siBuffer;
> +    toConvert.len = ISSUER_HASH_LENGTH;
> +    toConvert.data = issuerHash;
> +    NSSBase64_EncodeItem(NULL, issuerHashBase64, ISSUER_HASH_BASE64, &toConvert);

where is issuerHashBase64 allocated?

@@ +59,5 @@
> +    certSerial = node.serialNumber.data;
> +    issuerList = (struct CRLSetEntry *) bsearch(issuerHashBase64,
> +						CRLSetObject->listOfIssuer,
> +						CRLSetObject->numOfIssuers,
> +						sizeof(struct CRLSetEntry), compareIssuers);

If you change the loop to node then here you can do:

if(!issuerList) {
  continue;
}

to simplify logic.

@@ +64,5 @@
> +    if (issuerList != NULL) {
> +      revokedCert = (char *) bsearch(certSerial, issuerList->revokedSerialNums,
> +				     issuerList->numOfRevokedCerts, sizeof(char *),
> +				     compareSerials);
> +      if (revokedCert != NULL) {

We prefer to use:
'if (revokedCert)... '  and 'if (!revokedCert)...' instead of : 'if (revokedCert != nullptr) ...' and 'if (revokedCert == nullptr)...'
Also since it is c++ you should use nullptr instead of NULL

Comment 9

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet V8

Comment 10

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet V9

Comment 11

[Camilo Viecco (:cviecco)]

Comment on attachment 767467 [details] [diff] [review]
Implementation of the CRLSet V9

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

Getting better. Beware of reading the file format.

::: security/manager/ssl/src/CRLSet.cpp
@@ +42,5 @@
> +     return SECFailure;
> +  }
> +
> +  char line[CRLSet_MAX_CERT_SERIAL_LENGTH];
> +  ifstream myFile ("CRLSet.txt");

Where is CRLSet.txt, where is the format defined?

@@ +52,5 @@
> +  if (!myFile.good()) {
> +    return SECFailure;
> +  }
> +  lNumOfIssuers = atoi(line);
> +  lIssuers = new CRLSetEntry[lNumOfIssuers];

what if the file is corrupted and NumOfIssuers is completely bogus (too big, negative or zero)?, you also need to check that new was successful.

@@ +65,5 @@
> +    }
> +  }
> +  lVersion = atoi(line);
> +
> +  for (int i = 0; i < lNumOfIssuers; i++) {

What happens if the file is shorter than the numberof issuers?

@@ +91,5 @@
> +}
> +
> +SECStatus
> +CRLSet::Init()
> +{

Can, (should this be called once?)

::: security/manager/ssl/src/CertVerifier.cpp
@@ +109,5 @@
>    if (rv == SECFailure) {
>      return SECFailure;
>    }
> +  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: getting chain in 'classic' \n"));
> +  localValidationChain = CERT_GetCertChainFromCert(cert, time, enumUsage);

Do this compiles? ie where is localValidationChain defined?

@@ +115,5 @@
>      return SECFailure;
>    }
> +  CRLSet::Init();
> +  rv = CRLSet::Check(localValidationChain);
> +  rv = SECSuccess;

Always success?

@@ +407,3 @@
>    }
> +  
> +  //Checking the CRLSet for revocation for the whole chain.

You need to have a similar part on the first call of pkix as you are missing the fallback (think case where EV path in revoked but non EV path is not revoked)

@@ +407,5 @@
>    }
> +  
> +  //Checking the CRLSet for revocation for the whole chain.
> +  if (rv == SECSuccess) {
> +    CRLSet::Init();

you should not call this all the time, since this will read from a file and parse it. You need a mutex and a flag to make sure the file is only read once. There are multiple threads calling this.

Comment 12

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet V10

Comment 13

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet V11

Comment 14

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet V12

Comment 15

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet V13

Comment 16

[Camilo Viecco (:cviecco)]

Comment on attachment 770927 [details] [diff] [review]
Implementation of the CRLSet V13

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

better, but you have spin locks and the comparison function is overly complicated.

::: security/manager/ssl/src/CRLSet.cpp
@@ +33,5 @@
>  /* Comparison function for bsearch() */
>  static int compareSerials( const void *a, const void *b) {
> +  CRLSetRevokedEntry *serial1 = (CRLSetRevokedEntry *) a;
> +  CRLSetRevokedEntry *serial2 = (CRLSetRevokedEntry *) b;
> +  int smallerSize = (int) (serial1->len <= serial2->len ? serial1->len : serial2->len);

replace with: 
if (sizes are different) { 
  return serial1->len < serial2->len
}
return memcmp(serial1->revokedSerial,serial2->revokedSerial, serial1->len);

@@ +201,2 @@
>    if (CRLSet::isInit == true) {
> +    CRLSet::CRLSetMutex->Unlock();

dont need it to setup init

@@ +210,5 @@
>      return SECFailure;
>    }
>  
> +  while (true) {
> +    CRLSet::CRLSetMutex->Lock();

another spin lock.

@@ +247,5 @@
>      return SECSuccess;
>    }
>  
> +  while (true) {
> +    CRLSet::CRLSetMutex->Lock();

this is a spin lock-> bad (think what happens when there is one thread using the struct)

::: security/manager/ssl/src/CRLSet.h
@@ +4,5 @@
>  
>  #ifndef mozilla_psm__CRLSet_h
>  #define mozilla_psm__CRLSet_h
>  #define CRLSet_ISSUER_HASH_LENGTH 33
>  #define CRLSet_ISSUER_LENGTH 45

why not make these 44 and 32?

@@ +18,5 @@
>  
>  typedef struct {
> +  char *revokedSerial;
> +  uint8_t len;
> +} CRLSetRevokedEntry;

make it a packed struct (reduce memory footprint) and put the len first. (then you could use a single memcpy call in your compare)

Comment 17

[Mohammed Almeshekah [:meshekah]]

Attachment: CRLSet Implementation for Preloaded Set

This is the CRLSet Implementation for the preloaded revoked certificates in file "CRLSetPreload.inc"

Comment 18

[Camilo Viecco (:cviecco)]

Comment on attachment 781265 [details] [diff] [review]
CRLSet Implementation for Preloaded Set

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

Remove the pem files from your

::: CRLSet.moz
@@ +1,1 @@
> +1

Is this file needed?

::: security/certverifier/CRLSet.cpp
@@ +68,5 @@
> +
> +/* This is called to initilize the CRLSet. It assumes that the local CRLSet file is already */
> +/*   packaged with firefox.                                                                 */
> +nsresult
> +CRLSet::Init() {  

you dont really need an init, as all your stuff is built in.

@@ +87,5 @@
> +
> +/* This will check whether a particular certificate is revoked (i.e. in the CRLSet) given the */
> +/*    certifciate and the certificate of the issuer.                                          */
> +SECStatus
> +CRLSet::CheckCertRevoked(const CERTCertListNode *parent, const CERTCertListNode *node) {

General: this function should return tree values:
1. I now is revoked
2. I now is NOT revoked
3. I cannot tell, 

The interface does not allow you to distinguish between cases 2 and 3. Should we care? what should be the fallback?

you should proably use the actual certs instead of the nodes (readability)
you should also rename parent to issuer.

@@ +96,5 @@
> +  CRLSetEntry *issuerList = nullptr;
> +  CRLSetEntry tempIssuerEntry;
> +
> +  memset(issuerHash,0x00,SHA256_LENGTH);
> +  rv = SECSuccess;

dont do this, always assume failure. Until you prove me otherwise

::: security/certverifier/CRLSet.h
@@ +29,5 @@
> +class CRLSet : public AtomicRefCounted<CRLSet>
> +{
> + public:
> +  SECStatus CheckChainRevoked(const CERTCertList *certChain);
> +  SECStatus CheckCertRevoked(const CERTCertListNode *issuer, const CERTCertListNode *cert);

Why is CheckCertRevoked public?

@@ +36,5 @@
> +
> + private:
> +  int numOfIssuers = -1;
> +  CRLSetEntry *listOfIssuers = nullptr;
> +  int version = -1;

No version is needed if using a built-in list , also initializing in the declaration is frowned here.
Actually I dont think you need any of these private variables.

::: security/certverifier/CRLSetPreload.inc
@@ +178,5 @@
> +const PreloadCRLSetRevokedEntry cert8_ca[] = {
> +	{ cert8_caSerial0, 2 }
> +};
> +
> +const PreloadCRLSetRevokedEntry chase_parent[] = {

This should not be here ->

::: security/certverifier/CertVerifier.cpp
@@ +160,5 @@
> +  localValidationChain = CERT_GetCertChainFromCert(cert, time, enumUsage);
> +  if (!localValidationChain) {
> +    return SECFailure;
> +  }
> +  if (!crlSet->is_Init()) {

you are assuming crlSet is Not null -> potential fail.

::: security/certverifier/ExtendedValidation.cpp
@@ +10,5 @@
>  #include "pk11pub.h"
>  #include "secerr.h"
>  #include "prerror.h"
>  #include "prinit.h"
> +#include <algorithm>

why this?

::: security/certverifier/PreLoadedCRLSetGen/generatePreLoadedCRLSet.py
@@ +144,5 @@
> +			revoked = crl.get_revoked()
> +			if revoked is None:
> +				print "CRL [" + CRLURI + "] has no revoked certificates"
> +				continue;
> +			

trailing space

@@ +154,5 @@
> +			found = 0
> +			for rev in revoked:
> +				reason = rev.get_reason()
> +				reason = str(reason).strip().lower().replace(" ", "")
> +				if reason == "cacompromise" or reason == "2":

are there any other values this thing returns?

@@ +174,5 @@
> +			issuer.append(revokedList)
> +			issuersList.append(issuer)
> +		
> +		##################
> +		## Testing issuer

This should NOT be included by default.

::: security/certverifier/library/EmbeddedCertVerifier.cpp
@@ +4,5 @@
>  #include "nss.h"
>  #include "secerr.h"
>  #include "prerror.h"
>  #include "prlog.h"
> +#include <fstream>

why fstream?

::: security/manager/ssl/src/SharedSSLState.cpp
@@ +18,5 @@
>  #include "PublicSSL.h"
>  #include "ssl.h"
>  #include "nsNetCID.h"
>  #include "mozilla/unused.h"
> +#include "CRLSet.h"

where is this used?

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1017,3 @@
>          }
>        }
> +      

remove all this extra white spaces, as I actually think the file does not differ,

::: security/manager/ssl/src/nsNSSComponent.h
@@ +172,5 @@
>    NS_IMETHOD IsNSSInitialized(bool *initialized);
>  
>    ::mozilla::TemporaryRef<mozilla::psm::CertVerifier>
>      GetDefaultCertVerifier() MOZ_OVERRIDE;
> +  

extra space

::: security/manager/ssl/tests/gtest/test_crlset.cpp
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

This file does not test anything. Remove

::: security/manager/ssl/tests/unit/test_crlset/generate_certs.py
@@ +1,1 @@
> +#!/usr/bin/python

This file does not test anything.

Comment 19

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet Preloaded mechanism with XPCshell tests

Comment 20

[Camilo Viecco (:cviecco)]

Comment on attachment 787097 [details] [diff] [review]
Implementation of the CRLSet Preloaded mechanism with XPCshell tests

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

Where is ""bad_issuer_key.key"

::: security/certverifier/CRLSet.h
@@ +10,5 @@
> +#include <hasht.h>
> +
> +namespace mozilla { namespace psm {
> +
> +typedef char*  RevokedSerialEntry;

I would not add a typedef for a char*

::: security/certverifier/PreLoadedCRLSetGen/generatePreLoadedCRLSet.py
@@ +162,5 @@
> +			else:
> +				print "\t-> " + str(len(revokedList)) + " certificates have been added."
> +			Total += len(revokedList)
> +			issuer.append(revokedList)
> +			issuersList.append(issuer)		

whitespace

@@ +164,5 @@
> +			Total += len(revokedList)
> +			issuer.append(revokedList)
> +			issuersList.append(issuer)		
> +
> +		#######################################

remove this section(chase)

@@ +195,5 @@
> +		## Generated File Content
> +		Content = GenFileHeader
> +		Content += "const int PreloadNumOfIssuers = " + str(len(issuersList)) + ";\n\n"
> +
> +		for x in issuersList:

plase add into the generated file a comment (in the generated source) that includes the subject of the CRL issuer

::: security/manager/ssl/tests/unit/test_crlset/generate_certs.py
@@ +6,5 @@
> +import tempfile, os, subprocess
> +
> +db = ""
> +srcdir = ""
> +preloadDirLoc = "/Users/malmeshekah/hg/mozilla-central/security/certverifier/PreLoadedCRLSetGen"

This is a no-no : all locations should be relative (But actually seems like you dont use this) remove it.

@@ +9,5 @@
> +srcdir = ""
> +preloadDirLoc = "/Users/malmeshekah/hg/mozilla-central/security/certverifier/PreLoadedCRLSetGen"
> +
> +CA_basic_constraints = "basicConstraints=critical,CA:TRUE\n"
> +CA_limited_basic_constraints = "basicConstraints=critical,CA:TRUE, pathlen:0\n"

you dont use this one, remove it

@@ +15,5 @@
> +CA_min_ku = "keyUsage=critical, keyCertSign\n"
> +EE_full_ku = "keyUsage=digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement\n"
> +
> +## This is the list of the certificates to generate
> +## [cert_name, CA = 1 | Int = 2 | EE = 3, issuer_file_name, use_bad_key (YES = 1 | No = 0), serial]

since you have an issuer the name I would actually change the params to that instead of the location in the chain you specify is the cert is EE or CA. (in would be issuer name not null and CA)

@@ +55,5 @@
> +    else:
> +        key_name = cert_key
> +
> +    csr_name = db + "/"+ name + ".csr"
> +    pid = subprocess.Popen("openssl req -new -key "+ key_name + " -days 3650 -extensions v3_ca -batch -out " + csr_name + " -utf8 -subj '/CN=" + name + "'", stderr=subprocess.PIPE, shell=True)

I notices you changed the "system" balc for Popen, why? (I dont have a strong preference, this is just an observation as this code looks more complex)

@@ +79,5 @@
> +
> +def generate_certs():
> +    ee_ext_text = EE_basic_constraints + EE_full_ku
> +    ca_ext_text = CA_basic_constraints + CA_min_ku
> +    

whitespaces....

Comment 21

[Mohammed Almeshekah [:meshekah]]

(In reply to Camilo Viecco (:cviecco) from comment #20)
> Comment on attachment 787097 [details] [diff] [review]
> Implementation of the CRLSet Preloaded mechanism with XPCshell tests
> 
> Review of attachment 787097 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Where is ""bad_issuer_key.key"
> 
> ::: security/certverifier/CRLSet.h
> @@ +10,5 @@
> > +#include <hasht.h>
> > +
> > +namespace mozilla { namespace psm {
> > +
> > +typedef char*  RevokedSerialEntry;
> 
> I would not add a typedef for a char*
> 
> ::: security/certverifier/PreLoadedCRLSetGen/generatePreLoadedCRLSet.py
> @@ +162,5 @@
> > +			else:
> > +				print "\t-> " + str(len(revokedList)) + " certificates have been added."
> > +			Total += len(revokedList)
> > +			issuer.append(revokedList)
> > +			issuersList.append(issuer)		
> 
> whitespace
> 
> @@ +164,5 @@
> > +			Total += len(revokedList)
> > +			issuer.append(revokedList)
> > +			issuersList.append(issuer)		
> > +
> > +		#######################################
> 
> remove this section(chase)
> 
> @@ +195,5 @@
> > +		## Generated File Content
> > +		Content = GenFileHeader
> > +		Content += "const int PreloadNumOfIssuers = " + str(len(issuersList)) + ";\n\n"
> > +
> > +		for x in issuersList:
> 
> plase add into the generated file a comment (in the generated source) that
> includes the subject of the CRL issuer
> 
> ::: security/manager/ssl/tests/unit/test_crlset/generate_certs.py
> @@ +6,5 @@
> > +import tempfile, os, subprocess
> > +
> > +db = ""
> > +srcdir = ""
> > +preloadDirLoc = "/Users/malmeshekah/hg/mozilla-central/security/certverifier/PreLoadedCRLSetGen"
> 
> This is a no-no : all locations should be relative (But actually seems like
> you dont use this) remove it.
> 
> @@ +9,5 @@
> > +srcdir = ""
> > +preloadDirLoc = "/Users/malmeshekah/hg/mozilla-central/security/certverifier/PreLoadedCRLSetGen"
> > +
> > +CA_basic_constraints = "basicConstraints=critical,CA:TRUE\n"
> > +CA_limited_basic_constraints = "basicConstraints=critical,CA:TRUE, pathlen:0\n"
> 
> you dont use this one, remove it

which one? I use all of the usages information.

> 
> @@ +15,5 @@
> > +CA_min_ku = "keyUsage=critical, keyCertSign\n"
> > +EE_full_ku = "keyUsage=digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement\n"
> > +
> > +## This is the list of the certificates to generate
> > +## [cert_name, CA = 1 | Int = 2 | EE = 3, issuer_file_name, use_bad_key (YES = 1 | No = 0), serial]
> 
> since you have an issuer the name I would actually change the params to that
> instead of the location in the chain you specify is the cert is EE or CA.
> (in would be issuer name not null and CA)

I am not sure I got this comment.

> 
> @@ +55,5 @@
> > +    else:
> > +        key_name = cert_key
> > +
> > +    csr_name = db + "/"+ name + ".csr"
> > +    pid = subprocess.Popen("openssl req -new -key "+ key_name + " -days 3650 -extensions v3_ca -batch -out " + csr_name + " -utf8 -subj '/CN=" + name + "'", stderr=subprocess.PIPE, shell=True)
> 
> I notices you changed the "system" balc for Popen, why? (I dont have a
> strong preference, this is just an observation as this code looks more
> complex)

The main reason is that I can pipe all the output (including stderr) instead of showing them in the command line which makes the output of the code much cleaner.

> 
> @@ +79,5 @@
> > +
> > +def generate_certs():
> > +    ee_ext_text = EE_basic_constraints + EE_full_ku
> > +    ca_ext_text = CA_basic_constraints + CA_min_ku
> > +    
> 
> whitespaces....

Comment 22

[Mohammed Almeshekah [:meshekah]]

Attachment: Implementation of the CRLSet Preloaded mechanism with XPCshell tests 2

Comment 23

[Camilo Viecco (:cviecco)]

Comment on attachment 787682 [details] [diff] [review]
Implementation of the CRLSet Preloaded mechanism with XPCshell tests 2

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

Almost there.

::: security/certverifier/PreLoadedCRLSetGen/generatePreLoadedCRLSet.py
@@ +161,5 @@
> +			else:
> +				print "\t-> " + str(len(revokedList)) + " certificates have been added."
> +			Total += len(revokedList)
> +			issuer.append(revokedList)
> +			issuersList.append(issuer)

You have still not added the comment on the actual subject of the cert. This will affect the generated file

@@ +165,5 @@
> +			issuersList.append(issuer)
> +
> +		## This is revocation information for the automated testing
> +		print "Adding the automated testing certificates"
> +		Testhash = "f9863ff2e834febf83ebb8a3ec0333d1bc5fba8045ba034d4051eeef921b50d1"

Add comment on how to get this number

::: security/manager/ssl/tests/unit/test_crlset/generate_certs.py
@@ +14,5 @@
> +CA_min_ku = "keyUsage=critical, keyCertSign\n"
> +EE_full_ku = "keyUsage=digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement\n"
> +
> +## This is the list of the certificates to generate
> +## [cert_name, CA = 1 | Int = 2 | EE = 3, issuer_file_name, use_bad_key (YES = 1 | No = 0), serial]

Please chahge the logic so that it is:
cert_name, is_ca (yes_no), issuer_file_name, use_bad_key, serial 
In this case: 

             |       has_issuer_file_name
             |     0          |        1
 ----------------------------------------------------
  is_ca  0   | EE-self-signed | EE   
         ------------------------------------------ 
         1   | Root_ca        | int CA

as you have it is is possible to have inconsistent state.

Comment 24

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

This became OneCRL.