net: enable CRL sets behind a command line flag.

net/base/x509_certificate_nss.cc

Index: net/base/x509_certificate_nss.cc
diff --git a/net/base/x509_certificate_nss.cc b/net/base/x509_certificate_nss.cc
index 039edad0a6989172c0530843872db6b526804ae8..9f5cbe205768c9c36915c3b1e0491b8d0c8f504e 100644
--- a/net/base/x509_certificate_nss.cc
+++ b/net/base/x509_certificate_nss.cc
@@ -21,8 +21,10 @@
 #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"
@@ -228,6 +230,69 @@ bool IsKnownRoot(CERTCertificate* root) {
                      "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);
@@ -689,7 +754,8 @@ void X509Certificate::GetSubjectAltName(
 
 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)
@@ -721,7 +787,34 @@ int X509Certificate::VerifyInternal(const std::string& hostname,
     // 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