net: enable CRL sets behind a command line flag.

net/base/x509_certificate_nss.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/x509_certificate.h"
 
 #include <cert.h>
 #include <cryptohi.h>
 #include <nss.h>
 #include <pk11pub.h>
 #include <prerror.h>
 #include <prtime.h>
 #include <secder.h>
 #include <secerr.h>
 #include <sechash.h>
 #include <sslerr.h>
 
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/pickle.h"
 #include "base/time.h"
 #include "crypto/nss_util.h"
 #include "crypto/rsa_private_key.h"
+#include "net/base/asn1_util.h"
 #include "net/base/cert_status_flags.h"
 #include "net/base/cert_verify_result.h"
+#include "net/base/crl_set.h"
 #include "net/base/ev_root_ca_metadata.h"
 #include "net/base/net_errors.h"
 #include "net/base/x509_util_nss.h"
 
 namespace net {
 
 namespace {
 
 class ScopedCERTCertificatePolicies {
  public:
@@ skipping 185 matching lines @@
 bool IsKnownRoot(CERTCertificate* root) {
   if (!root->slot)
     return false;
 
   // This magic name is taken from
   // http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ckfw/builtins/constants.c&rev=1.13&mark=86,89#79
   return 0 == strcmp(PK11_GetSlotName(root->slot),
                      "NSS Builtin Objects");
 }
 
+enum CRLSetResult {
+  kCRLSetRevoked,
+  kCRLSetOk,
+  kCRLSetError,
+};
+
+// CheckRevocationWithCRLSet attempts to check each element of |cert_list|
+// against |crl_set|. It returns:
+//   kCRLSetRevoked: if any element of the chain is known to have been revoked.
+//   kCRLSetError: if an error occurs in processing.
+//   kCRLSetOk: if no element in the chain is known to have been revoked.
+CRLSetResult CheckRevocationWithCRLSet(CERTCertList* cert_list,
+                                       CERTCertificate* root,
+                                       CRLSet* crl_set) {
+  std::vector<CERTCertificate*> certs;
+  for (CERTCertListNode* node = CERT_LIST_HEAD(cert_list);
+       !CERT_LIST_END(node, cert_list);
+       node = CERT_LIST_NEXT(node)) {
+    certs.push_back(node->cert);
+  }
+  certs.push_back(root);
+
+  CERTCertificate* last = NULL;

[wtc, 2011/10/21 23:17:31]

Nit: |prev| would be better because |last| may also mean the
very last item.

[agl, 2011/10/24 20:44:27]

Done.

+  for (std::vector<CERTCertificate*>::iterator i = certs.begin();
+       i != certs.end(); i++) {

[wtc, 2011/10/21 23:17:31]

Nit: ++i

[agl, 2011/10/24 20:44:27]

Done.

+    CERTCertificate* cert = *i;
+    CERTCertificate* child = last;
+    last = cert;
+    if (child == NULL)
+      continue;
+
+    base::StringPiece der(reinterpret_cast<char*>(cert->derCert.data),
+                          cert->derCert.len);
+
+    base::StringPiece spki;
+    if (!asn1::ExtractSPKIFromDERCert(der, &spki)) {
+      NOTREACHED();
+      return kCRLSetError;
+    }
+
+    std::string serial_number(
+        reinterpret_cast<char*>(child->serialNumber.data),
+        child->serialNumber.len);
+    // Remove leading zeros.
+    while (serial_number.size() > 1 && serial_number[0] == 0)
+      serial_number = serial_number.substr(1, serial_number.size() - 1);

[wtc, 2011/10/21 23:17:31]

I'm sorry that I'm bringing this up again.

We should not remove the leading zeros from certificate serial
numbers, unless it is advantageous.  The reason is that it
may be misinterpreted as us not knowing how ASN.1 INTEGERs
are encoded.

[agl, 2011/10/24 20:44:27]

This has been addressed in http://codereview.chromium.org/8381017/

+
+    CRLSet::Result result = crl_set->CheckCertificate(serial_number, spki);
+
+    switch (result) {
+      case CRLSet::REVOKED:
+        return kCRLSetRevoked;
+      case CRLSet::UNKNOWN:

[wtc, 2011/10/21 23:17:31]

IMPORTANT: are you sure we should ignore CRLSet::UNKNOWN?
I think we should return kCRLSetError in that case.

[agl, 2011/10/24 20:44:27]

Yes. The intended semantics are that, unless a serial number is listed, then
it's not revoked. (If we accept that soft-fail, on-line checks are worthless in
any reasonable scenario then there's no harm in that. I'd be happy to try and
convince you if that if you wish.)

+      case CRLSet::GOOD:
+        continue;
+      default:
+        NOTREACHED();

[wtc, 2011/10/21 23:17:31]

BUG (corner case): We should return kCRLSetError in this case.

[agl, 2011/10/24 20:44:27]

Done.

+    }
+  }
+
+  return kCRLSetOk;
+}
+
 void ParsePrincipal(CERTName* name,
                     CertPrincipal* principal) {
   typedef char* (*CERTGetNameFunc)(CERTName* name);
 
   // TODO(jcampan): add business_category and serial_number.
   // TODO(wtc): NSS has the CERT_GetOrgName, CERT_GetOrgUnitName, and
   // CERT_GetDomainComponentName functions, but they return only the most
   // general (the first) RDN.  NSS doesn't have a function for the street
   // address.
   static const SECOidTag kOIDs[] = {
@@ skipping 441 matching lines @@
     }
     name = CERT_GetNextGeneralName(name);
     if (name == alt_name_list)
       break;
   }
   PORT_FreeArena(arena, PR_FALSE);
 }
 
 int X509Certificate::VerifyInternal(const std::string& hostname,
                                     int flags,
-                                    CertVerifyResult* verify_result) const {
+                                    CertVerifyResult* verify_result,
+                                    CRLSet* crl_set) const {
   // Make sure that the hostname matches with the common name of the cert.
   SECStatus status = CERT_VerifyCertName(cert_handle_, hostname.c_str());
   if (status != SECSuccess)
     verify_result->cert_status |= CERT_STATUS_COMMON_NAME_INVALID;
 
   // Make sure that the cert is valid now.
   SECCertTimeValidity validity = CERT_CheckCertValidTimes(
       cert_handle_, PR_Now(), PR_TRUE);
   if (validity != secCertTimeValid)
     verify_result->cert_status |= CERT_STATUS_DATE_INVALID;
@@ skipping 11 matching lines @@
   cvout[cvout_index].type = cert_po_end;
   ScopedCERTValOutParam scoped_cvout(cvout);
 
   bool check_revocation = (flags & VERIFY_REV_CHECKING_ENABLED);
   if (check_revocation) {
     verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
   } else {
     // EV requires revocation checking.
     flags &= ~VERIFY_EV_CERT;
   }
-  status = PKIXVerifyCert(cert_handle_, check_revocation, NULL, 0, cvout);
+
+  status = SECSuccess;

[wtc, 2011/10/21 23:17:31]

This assignment is not necessary.

[agl, 2011/10/24 20:44:27]

Done.

+  if (check_revocation && crl_set) {
+    // We have a CRLSet so we build a chain without revocation checking in
+    // order to try and check it ourselves.
+    status = PKIXVerifyCert(cert_handle_, false /* no revocation checking */,
+                            NULL, 0, cvout);
+    if (status == SECSuccess) {
+      CRLSetResult crl_set_result = CheckRevocationWithCRLSet(
+          cvout[cvout_cert_list_index].value.pointer.chain,
+          cvout[cvout_trust_anchor_index].value.pointer.cert,
+          crl_set);
+      if (crl_set_result == kCRLSetError) {
+        // An error occured during processing so we fall back to standard
+        // revocation checking.
+        status = PKIXVerifyCert(cert_handle_, check_revocation, NULL, 0, cvout);
+      } else {
+        DCHECK(crl_set_result == kCRLSetRevoked || crl_set_result == kCRLSetOk);
+        verify_result->cert_status |= CERT_STATUS_USED_CRL_SET;
+        if (crl_set_result == kCRLSetRevoked)
+          verify_result->cert_status |= CERT_STATUS_REVOKED;

[wtc, 2011/10/21 23:17:31]

BUG: we need to set |status| to SECFailure and set the NSS
error code here:
      if (crl_set_result == kCRLSetRevoked) {
        PORT_SetError(SEC_ERROR_REVOKED_CERTIFICATE);
        status = SECFailure;
      }
and then rely on the error code path on lines 818-834.

Alternatively, we can return right here:
      if (crl_set_result == kCRLSetRevoked) {
        LOG(ERROR) << ...;  // Not sure if this is necessary
        verify_result->cert_status |= CERT_STATUS_REVOKED;
        return ERR_CERT_REVOKED;
      }

[agl, 2011/10/24 20:44:27]

Ah, yes. Thank you. Done.

+      }
+    }
+  } else {
+    status = PKIXVerifyCert(cert_handle_, check_revocation,
+                            NULL, 0, cvout);
+  }
+
   if (status != SECSuccess) {
     int err = PORT_GetError();
     LOG(ERROR) << "CERT_PKIXVerifyCert for " << hostname
                << " failed err=" << err;
     // CERT_PKIXVerifyCert rerports the wrong error code for
     // expired certificates (NSS bug 491174)
     if (err == SEC_ERROR_CERT_NOT_VALID &&
         (verify_result->cert_status & CERT_STATUS_DATE_INVALID))
       err = SEC_ERROR_EXPIRED_CERTIFICATE;
     CertStatus cert_status = MapCertErrorToCertStatus(err);
@@ skipping 191 matching lines @@
 
 // static
 bool X509Certificate::WriteOSCertHandleToPickle(OSCertHandle cert_handle,
                                                 Pickle* pickle) {
   return pickle->WriteData(
       reinterpret_cast<const char*>(cert_handle->derCert.data),
       cert_handle->derCert.len);
 }
 
 }  // namespace net