Certificate Transparency: Require SCTs for EV certificates.

net/cert/cert_policy_enforcer.cc

+// Copyright 2014 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_policy_enforcer.h"
+
+#include <algorithm>
+
+#include "base/metrics/histogram.h"
+#include "base/numerics/safe_conversions.h"
+#include "net/cert/ct_ev_whitelist.h"
+#include "net/cert/ct_verify_result.h"
+#include "net/cert/signed_certificate_timestamp.h"
+#include "net/cert/x509_certificate.h"
+
+namespace net {
+
+namespace {
+bool IsEmbeddedSCT(const scoped_refptr<ct::SignedCertificateTimestamp>& sct) {
+  return sct->origin == ct::SignedCertificateTimestamp::SCT_EMBEDDED;
+}
+}
+
+CertPolicyEnforcer::CertPolicyEnforcer(size_t num_ct_logs,
+                                       bool require_ct_for_ev)
+    : num_ct_logs_(num_ct_logs), enforce_ct_requirement_(require_ct_for_ev) {
+}
+
+CertPolicyEnforcer::~CertPolicyEnforcer() {
+}
+
+bool CertPolicyEnforcer::DoesConformToCTEVPolicy(
+    X509Certificate* cert,
+    const ct::EVCertsWhitelist* ev_whitelist,
+    const ct::CTVerifyResult& ct_result) {
+  bool cert_in_ev_whitelist = false;
+  if (ev_whitelist && ev_whitelist->IsValid()) {
+    const SHA256HashValue fingerprint(
+        X509Certificate::CalculateFingerprint256(cert->os_cert_handle()));
+
+    cert_in_ev_whitelist = ev_whitelist->ContainsCertificateHash(
+        std::string(reinterpret_cast<const char*>(fingerprint.data), 8));
+  }
+
+  UMA_HISTOGRAM_BOOLEAN("Net.SSL_EVCertificateInWhitelist",
+                        cert_in_ev_whitelist);
+
+  if (!enforce_ct_requirement_)
+    return true;
+

[Ryan Sleevi, 2014/11/06 00:16:43]

Note that this code is missing the build date freshness check

see
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/transport...

This is also where you can place the Finch flag (which is part of //base as
well)

[Eran Messeri, 2014/11/20 11:49:56]

Added (by shamelessly copying it from TransportSecurityState class. Can you
suggest somewhere else for it to live?)

+  if (cert_in_ev_whitelist)
+    return true;
+
+  size_t num_valid_scts = ct_result.verified_scts.size();
+  size_t num_embedded_scts =
+      std::count_if(ct_result.verified_scts.begin(),
+                    ct_result.verified_scts.end(), IsEmbeddedSCT);

[Ryan Sleevi, 2014/11/11 02:50:18]

So, from the ct-policy discussion, I should have paid closer attention to the
policy.

This allows certificates that were logged BEFORE the log is included (per the
policy) to be accepted as if they were valid embedded certs. It's a BUG because
the code doesn't match the stated policy.

I think our internal database of verified_scts needs to include the approval
date for verification of it being embedded (in the CTVerifier or possibly
CTLogVerifier - not sure where this policy flag should land)

[Eran Messeri, 2014/11/20 11:49:56]

See my previous reply - IIUC that's to avoid distrusted logs rather than logs
which we've yet to fully accept because we're  monitoring them (so there's no
evidence of misbehaviour yet).

+
+  // TODO(eranm): Count the number of *independent* SCTs once the information
+  // about log operators is available, crbug.com/425174
+  size_t num_non_embedded_scts = num_valid_scts - num_embedded_scts;
+  if (num_non_embedded_scts >= 2) {

[Ryan Sleevi, 2014/11/06 00:16:43]

net code omits braces for single-line conditionals (lines 62, 66), but uses them
for multi-line (lines 70-73 are fine) or, traditionally, for complex cases
(lines 80-87 are fine)

[Eran Messeri, 2014/11/20 11:49:56]

Done per your guidelines.

+    return true;
+  }
+
+  if ((num_non_embedded_scts == 1) && (num_embedded_scts > 0)) {
+    return true;
+  }
+
+  if (cert->valid_start().is_null() || cert->valid_expiry().is_null()) {
+    // Will not be able to calculate the certificate's validity period.
+    return false;
+  }
+
+  base::TimeDelta expiry_period = cert->valid_expiry() - cert->valid_start();
+  uint32_t expiry_in_months_approx = expiry_period.InDays() / 30.14;

[Ryan Sleevi, 2014/11/06 00:16:43]

Does it make more sense to adopt math similar to that on
https://codereview.chromium.org/20628006/diff/289001/net/cert/cert_verify_pro...

(See lines 627-644)

[Eran Messeri, 2014/11/20 11:49:56]

Yes, used similar logic here.
I considered adding the functionality to TImeDelta but unlike other time units
there, it would be an approximation rather than exact value, so I'll follow up
with an owner to see if that makes sense.

+  // At most 5 SCTs are required - for certificate with lifetime of over
+  // 39 months.
+  size_t num_required_embedded_scts;
+  if (expiry_in_months_approx > 39) {
+    num_required_embedded_scts = 5;
+  } else if (expiry_in_months_approx > 27) {
+    num_required_embedded_scts = 4;
+  } else if (expiry_in_months_approx >= 15) {
+    num_required_embedded_scts = 3;
+  } else {
+    num_required_embedded_scts = 2;
+  }
+
+  size_t min_acceptable_logs = std::max(static_cast<size_t>(2), num_ct_logs_);

[Ryan Sleevi, 2014/11/06 00:16:43]

just do 2U and it should be sufficient, right?

Or place num_ct_logs_ as the first arg?

[Eran Messeri, 2014/11/20 11:49:56]

Tried both, the compiler was equally unhappy - I have to explicitly cast to
size_t.

+  return num_embedded_scts >=
+         std::min(num_required_embedded_scts, min_acceptable_logs);
+}
+
+}  // namespace net