Implement the ability to whitelist certs from specific issuers

net/cert/cert_verify_proc.cc

Index: net/cert/cert_verify_proc.cc
diff --git a/net/cert/cert_verify_proc.cc b/net/cert/cert_verify_proc.cc
index d987e3dc04c079d67078a53df9e6a35e1161fb65..13096d81fbed30f6c4a8ac15e55edc9a0d2cec8b 100644
--- a/net/cert/cert_verify_proc.cc
+++ b/net/cert/cert_verify_proc.cc
@@ -12,6 +12,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
+#include "crypto/sha2.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
@@ -39,6 +40,7 @@
 namespace net {
 
 namespace {
+#include "net/cert/cert_verify_proc_whitelist-inc.cc"

[davidben, 2015/03/30 21:59:14]

Maybe cert_verify_proc_whitelist.h or cert_verify_proc_whitelist_data.h? The
hyphen and .cc are weird. (.h is also off, but there's precedence with
net_error_list.h and transport_security_state_static.h)

[Ryan Sleevi, 2015/03/30 22:27:05]

net_error_list makes some sense because it's included in multiple files.
transport_security_state_static doesn't, and it's bothered me it's a .h

I did -inc, matching registry controlled domains, but it looks like it should be
.inc (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Self_containe...
)

 
 // Constants used to build histogram names
 const char kLeafCert[] = "Leaf";
@@ -164,6 +166,12 @@ bool ExaminePublicKeys(const scoped_refptr<X509Certificate>& cert,
   return weak_key;
 }
 
+int CompareHashValueToRawHash(const void* key, const void* element) {
+  const SHA256HashValue* search_key =
+      reinterpret_cast<const SHA256HashValue*>(key);
+  return memcmp(search_key->data, element, sizeof(search_key->data));
+}
+
 }  // namespace
 
 // static
@@ -235,6 +243,12 @@ int CertVerifyProc::Verify(X509Certificate* cert,
     rv = MapCertStatusToNetError(verify_result->cert_status);
   }
 
+  if (IsNonWhitelistedCert(*verify_result->verified_cert,
+                           verify_result->public_key_hashes)) {
+    verify_result->cert_status |= CERT_STATUS_AUTHORITY_INVALID;
+    rv = MapCertStatusToNetError(verify_result->cert_status);
+  }

[davidben, 2015/03/30 21:59:14]

Just to confirm this is fine: were the underlying OS to completely remove some
root we apply this treatment to, the whitelist will become void and we won't
trust anything from it, whitelist or no. Also if the OS does something similar,
the net whitelist will be the intersection.

(I think this is fine, but wanted to confirm.)

[Ryan Sleevi, 2015/03/30 22:27:05]

Yup

+
   // Check for weak keys in the entire verified chain.
   bool weak_key = ExaminePublicKeys(verify_result->verified_cert,
                                     verify_result->is_issued_by_known_root);
@@ -669,4 +683,28 @@ bool CertVerifyProc::HasTooLongValidity(const X509Certificate& cert) {
   return false;
 }
 
+// static
+bool CertVerifyProc::IsNonWhitelistedCert(const X509Certificate& cert,
+                                          const HashValueVector& hashes) {
+  for (size_t i = 0; i < arraysize(kWhitelistedIssuers); ++i) {
+    for (const auto& hash : hashes) {
+      if (hash.tag == HASH_VALUE_SHA256 &&
+          memcmp(hash.data(), kWhitelistedIssuers[i].public_key,
+                 crypto::kSHA256Length) == 0) {
+        const SHA256HashValue leaf_hash =
+            X509Certificate::CalculateFingerprint256(cert.os_cert_handle());
+        void* result =
+            bsearch(&leaf_hash, kWhitelistedIssuers[i].whitelist,

[davidben, 2015/03/30 21:59:14]

Optional: std::lower_bound might save you some casts and inlines the
element-size computation. Though you'll have to do an extra check, so I dunno.

[Ryan Sleevi, 2015/03/30 22:27:05]

Adds an extra compare and the extra check. Also ends up with bigger code-size
hit

+                    kWhitelistedIssuers[i].whitelist_size,
+                    sizeof(leaf_hash.data), CompareHashValueToRawHash);

[davidben, 2015/03/30 21:59:14]

Nit: Probably clearer to do sizeof(leaf_data.data) ->
sizeof(kWhitelistIssuers[i].whitelist[0])

(static_assert? This code assumes those two are equal, as are
crypto::kSHA256Length and sizeof(kWhitelistedIssuers[i].public_key). All of
which are blatantly true, of course. I guess if nothing else, this way you won't
accidentally forget to update things if the hash changes.)

[Ryan Sleevi, 2015/03/30 22:27:05]

I don't believe that's guaranteed to optimize the same. It certainly should
raise red-flags for reviewers "what if whitelist_size is zero" and having to
reason about the compile-time correctness.
I actually lean against that entirely and favoured just an explicit 32. The size
of a SHA-2 hash isn't going to change :/

[davidben, 2015/03/30 22:29:22]

Oh, I just meant if you were to switch to a different hash function. Though I
suppose tests would notice that anyway, so *shrug*.

+        if (result == nullptr)
+          return true;
+        return false;
+      }
+    }
+  }
+
+  return false;
+}
+
 }  // namespace net