Implement the ability to whitelist certs from specific issuers

net/cert/cert_verify_proc.cc

 // Copyright (c) 2012 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/cert/cert_verify_proc.h"
 
 #include <stdint.h>
 
 #include "base/basictypes.h"
 #include "base/metrics/histogram.h"
 #include "base/sha1.h"
 #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"
 #include "net/cert/cert_status_flags.h"
 #include "net/cert/cert_verifier.h"
 #include "net/cert/cert_verify_result.h"
 #include "net/cert/crl_set.h"
 #include "net/cert/x509_certificate.h"
 #include "url/url_canon.h"
 
 #if defined(USE_NSS) || defined(OS_IOS)
 #include "net/cert/cert_verify_proc_nss.h"
 #elif defined(USE_OPENSSL_CERTS) && !defined(OS_ANDROID)
 #include "net/cert/cert_verify_proc_openssl.h"
 #elif defined(OS_ANDROID)
 #include "net/cert/cert_verify_proc_android.h"
 #elif defined(OS_MACOSX)
 #include "net/cert/cert_verify_proc_mac.h"
 #elif defined(OS_WIN)
 #include "net/cert/cert_verify_proc_win.h"
 #else
 #error Implement certificate verification.
 #endif
 
 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";
 const char kIntermediateCert[] = "Intermediate";
 const char kRootCert[] = "Root";
 // Matches the order of X509Certificate::PublicKeyType
 const char* const kCertTypeStrings[] = {
     "Unknown",
     "RSA",
     "DSA",
@@ skipping 105 matching lines @@
           size_bits,
           type);
     }
     if (!weak_key && IsWeakKey(type, size_bits))
       weak_key = true;
   }
 
   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
 CertVerifyProc* CertVerifyProc::CreateDefault() {
 #if defined(USE_NSS) || defined(OS_IOS)
   return new CertVerifyProcNSS();
 #elif defined(USE_OPENSSL_CERTS) && !defined(OS_ANDROID)
   return new CertVerifyProcOpenSSL();
 #elif defined(OS_ANDROID)
   return new CertVerifyProcAndroid();
@@ skipping 51 matching lines @@
   std::vector<std::string> dns_names, ip_addrs;
   cert->GetSubjectAltName(&dns_names, &ip_addrs);
   if (HasNameConstraintsViolation(verify_result->public_key_hashes,
                                   cert->subject().common_name,
                                   dns_names,
                                   ip_addrs)) {
     verify_result->cert_status |= CERT_STATUS_NAME_CONSTRAINT_VIOLATION;
     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);
 
   if (weak_key) {
     verify_result->cert_status |= CERT_STATUS_WEAK_KEY;
     // Avoid replacing a more serious error, such as an OS/library failure,
     // by ensuring that if verification failed, it failed with a certificate
     // error.
     if (rv == OK || IsCertificateError(rv))
@@ skipping 414 matching lines @@
   if (start >= time_2012_07_01 && month_diff > 60)
     return true;
 
   // For certificates issued after 1 April 2015: 39 months.
   if (start >= time_2015_04_01 && month_diff > 39)
     return true;
 
   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