Cast device revocation checking.

components/cast_certificate/cast_crl.cc

Index: components/cast_certificate/cast_crl.cc
diff --git a/components/cast_certificate/cast_crl.cc b/components/cast_certificate/cast_crl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..938881f5cb5bf3c4c620c0972aed229a3b9557d2
--- /dev/null
+++ b/components/cast_certificate/cast_crl.cc
@@ -0,0 +1,374 @@
+// Copyright 2016 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 "components/cast_certificate/cast_crl.h"
+
+#include <unordered_map>
+#include <unordered_set>
+
+#include "base/base64.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/singleton.h"
+#include "components/cast_certificate/proto/revocation.pb.h"
+#include "crypto/sha2.h"
+#include "net/cert/internal/parse_certificate.h"
+#include "net/cert/internal/parsed_certificate.h"
+#include "net/cert/internal/path_builder.h"
+#include "net/cert/internal/signature_algorithm.h"
+#include "net/cert/internal/signature_policy.h"
+#include "net/cert/internal/trust_store.h"
+#include "net/cert/internal/verify_certificate_chain.h"
+#include "net/cert/internal/verify_signed_data.h"
+#include "net/cert/x509_certificate.h"
+#include "net/der/input.h"
+#include "net/der/parser.h"
+#include "net/der/parse_values.h"
+
+namespace cast_certificate {
+namespace {
+
+enum CrlVersion {
+  // version 0: Spki Hash Algorithm = SHA-256
+  //            Signature Algorithm = RSA-PKCS1 V1.5 with SHA-256
+  CRL_VERSION_0 = 0,
+};
+
+// -------------------------------------------------------------------------
+// Cast CRL trust anchors.
+// -------------------------------------------------------------------------
+
+// There is one trusted root for Cast CRL certificate chains:
+//
+//   (1) CN=Cast CRL Root CA    (kCastCRLRootCaDer)
+//
+// These constants are defined by the file included next:
+
+#include "components/cast_certificate/cast_crl_root_ca_cert_der-inc.h"
+
+// Singleton for the Cast CRL trust store.
+class CastCRLTrustStore {
+ public:
+  static CastCRLTrustStore* GetInstance() {
+    return base::Singleton<CastCRLTrustStore, base::LeakySingletonTraits<
+                                                  CastCRLTrustStore>>::get();
+  }
+
+  static net::TrustStore& Get() { return GetInstance()->store_; }
+
+ private:
+  friend struct base::DefaultSingletonTraits<CastCRLTrustStore>;
+
+  CastCRLTrustStore() {
+    // Initialize the trust store with the root certificate.
+    // TODO (ryanchung): Add official Cast CRL Root here

[eroman, 2016/07/12 21:22:01]

Why is this a TODO() rather than added here?

[ryanchung, 2016/07/14 16:15:25]

The root certificate has not been generated yet.

+    // scoped_refptr<net::ParsedCertificate> root = net::ParsedCertificate::
+    //     net::ParsedCertificate::CreateFromCertificateData(
+    //         kCastCRLRootCaDer, sizeof(kCastCRLRootCaDer),
+    //         net::ParsedCertificate::DataSource::EXTERNAL_REFERENCE, {});
+    // CHECK(root);
+    // store_.AddTrustedCertificate(std::move(root));
+  }
+
+  net::TrustStore store_;
+  DISALLOW_COPY_AND_ASSIGN(CastCRLTrustStore);
+};
+
+// Converts a base::Time::Exploded to a net::der::GeneralizedTime.

[eroman, 2016/07/12 21:22:00]

side-note: This seems like something we should extract to a lower layer -- I
will propose a change for this.

+net::der::GeneralizedTime ConvertExplodedTime(
+    const base::Time::Exploded& exploded) {
+  // Add 500 milliseconds and strip the milliseconds.
+  // Effectively rounding the original time to the nearest second.
+  base::Time rounded_time = base::Time::FromUTCExploded(exploded);

[eroman, 2016/07/12 21:22:00]

This isn't generally safe pattern, since FromUTCExploded() can fail.

See for instance crbug.com/601900

[ryanchung, 2016/07/14 16:15:25]

Removed the rounding and removed this. Thanks for the heads up!

+  rounded_time += base::TimeDelta::FromMilliseconds(500);

[eroman, 2016/07/12 21:22:00]

Why is this rounding desired?
We don't for instance do this in cast_cert_validator.cc

[ryanchung, 2016/07/14 16:15:25]

Removed rounding.
Originally, this was because the precision of the CRL's expiration was in
milliseconds so the rounding was added to be conservative due to the difference
in precision. We've now changed the precision of the CRL's expiration to seconds
to avoid the confusion here.

+  base::Time::Exploded rounded_exploded_time;
+  rounded_time.UTCExplode(&rounded_exploded_time);
+
+  net::der::GeneralizedTime result;
+  result.year = rounded_exploded_time.year;
+  result.month = rounded_exploded_time.month;
+  result.day = rounded_exploded_time.day_of_month;
+  result.hours = rounded_exploded_time.hour;
+  result.minutes = rounded_exploded_time.minute;
+  result.seconds = rounded_exploded_time.second;
+  return result;
+}
+
+// Converts a ner::der::GeneralizedTime to base::Time::Exploded
+base::Time::Exploded ConvertGeneralizedTime(
+    const net::der::GeneralizedTime generalized) {
+  base::Time::Exploded result = {0};
+  result.year = generalized.year;
+  result.month = generalized.month;
+  result.day_of_month = generalized.day;
+  result.hour = generalized.hours;
+  result.minute = generalized.minutes;
+  result.second = generalized.seconds;
+  return result;
+}
+
+// Specifies the signature verification policy.
+std::unique_ptr<net::SignaturePolicy> CreateCastSignaturePolicy() {
+  return base::WrapUnique(new net::SimpleSignaturePolicy(2048));

[eroman, 2016/07/12 21:22:00]

Can you add a comment explaining what algorithms are needed?
i.e. RSA SSA PKCS#1 v.1.5 with SHA-256, using RSA keys 2048-bits or longer.

[ryanchung, 2016/07/14 16:15:25]

Done.

+}
+
+// Verifies the CRL is signed by a trusted CRL authority at the time the CRL
+// was issued. Verifies the signature of |tbs_crl| is valid based on the
+// certificate and signature in |crl|. The validity of |tbs_crl| is verified
+// at |time|. The validity period of the CRL is adjusted to be the earliest
+// of the issuer certificate chain's expiration and the CRL's expiration and
+// the result is stored in |overall_validity_end|.
+bool VerifyCRL(const Crl& crl,
+               const TbsCrl& tbs_crl,
+               const base::Time::Exploded& time,
+               base::Time* overall_validity_end) {
+  // Verify the trust of the CRL authority.
+  scoped_refptr<net::ParsedCertificate> parsed_cert =
+      net::ParsedCertificate::CreateFromCertificateData(
+          reinterpret_cast<const uint8_t*>(crl.signer_cert().data()),
+          crl.signer_cert().size(),
+          net::ParsedCertificate::DataSource::EXTERNAL_REFERENCE, {});
+  if (parsed_cert == nullptr) {
+    VLOG(2) << "CRL - Issuer certificate parsing failed.";
+    return false;
+  }
+
+  // Parse the signature.
+  std::string signature = crl.signature();

[eroman, 2016/07/12 21:22:00]

can this be a reference instead (to avoid copying the signature data).

Or rather, in the line below you can write:

net::der::BitString(net::der::Input(base::StringPiece(crl.signature())), 0);

[ryanchung, 2016/07/14 16:15:25]

Done.

+  net::der::BitString signature_value_bit_string =
+      net::der::BitString(net::der::Input(&signature), 0);
+
+  // Verify the signature.
+  std::unique_ptr<net::SignaturePolicy> policy =
+      base::WrapUnique(new net::SimpleSignaturePolicy(2048));

[eroman, 2016/07/12 21:22:00]

Should this be using CreateCastSignaturePolicy() instead?

[ryanchung, 2016/07/14 16:15:25]

Done.

+  std::unique_ptr<net::SignatureAlgorithm> signature_algorithm_type =
+      net::SignatureAlgorithm::CreateRsaPkcs1(net::DigestAlgorithm::Sha256);
+  if (!VerifySignedData(*signature_algorithm_type,
+                        net::der::Input(&crl.tbs_crl()),
+                        signature_value_bit_string, parsed_cert->tbs().spki_tlv,
+                        policy.get())) {
+    VLOG(2) << "CRL - Signature verification failed.";
+    return false;
+  }
+
+  // Verify the issuer certificate.
+  auto signature_policy = CreateCastSignaturePolicy();
+  net::CertPathBuilder::Result result;
+  net::CertPathBuilder path_builder(
+      parsed_cert.get(), &CastCRLTrustStore::Get(), signature_policy.get(),
+      ConvertExplodedTime(time), &result);
+  net::CompletionStatus rv = path_builder.Run(base::Closure());
+  DCHECK_EQ(rv, net::CompletionStatus::SYNC);
+  if (!result.is_success() || result.paths.empty() ||
+      !result.paths[result.best_result_index]->is_success()) {
+    VLOG(2) << "CRL - Issuer certificate verification failed.";
+    return false;
+  }
+
+  // Verify the CRL is still valid.
+  base::Time utc_time = base::Time::FromUTCExploded(time);
+  base::Time validity_start =
+      base::Time::UnixEpoch() +
+      base::TimeDelta::FromMilliseconds(tbs_crl.issuance_time_millis());
+  base::Time validity_end =
+      base::Time::UnixEpoch() +
+      base::TimeDelta::FromMilliseconds(tbs_crl.issuance_time_millis() +
+                                        tbs_crl.validity_period_millis());
+  if ((utc_time < validity_start) || (utc_time >= validity_end)) {
+    VLOG(2) << "CRL - Not time-valid.";
+    return false;
+  }
+
+  // Set CRL expiry to the earliest of the cert chain expiry and CRL expiry.
+  *overall_validity_end = validity_end;
+  for (const auto cert : result.paths[result.best_result_index]->path) {

[eroman, 2016/07/12 21:22:00]

const auto&

[ryanchung, 2016/07/14 16:15:25]

Done.

+    base::Time::Exploded cert_expiry_exploded =
+        ConvertGeneralizedTime(cert->tbs().validity_not_after);
+    base::Time cert_time;
+    if (!base::Time::FromUTCExploded(cert_expiry_exploded, &cert_time)) {
+      VLOG(2) << "Unexpected error: Malformed certificate not-after time.";
+      return false;
+    }
+    // Certificate expiry time uses ASN.1 Generalized time accurate to the
+    // second and has a precision of +/- 500ms. Conservatively add 500ms when
+    // converted to base::Time.
+    cert_time += base::TimeDelta::FromMilliseconds(500);

[eroman, 2016/07/12 21:22:01]

Same comment as earlier. I don't think there is real value in doing this.

[ryanchung, 2016/07/14 16:15:25]

Removed the rounding. Updated all instances of time to have a precision of
seconds to avoid this.

+    if (cert_time < *overall_validity_end)
+      *overall_validity_end = cert_time;
+  }
+
+  // Perform sanity check on serial numbers.
+  for (const auto& range : tbs_crl.revoked_serial_number_ranges()) {
+    uint64_t first_serial_number = range.first_serial_number();
+    uint64_t last_serial_number = range.last_serial_number();
+    if (last_serial_number < first_serial_number) {
+      VLOG(2) << "CRL - Malformed serial number range.";
+      return false;
+    }
+  }
+  return true;
+}
+
+class CastCRLImpl : public CastCRL {
+ public:
+  CastCRLImpl(const TbsCrl& tbs_crl, const base::Time& overall_validity_end);
+  ~CastCRLImpl() override;
+
+  bool CheckRevocation(
+      const std::vector<scoped_refptr<net::ParsedCertificate>>& trusted_chain,
+      const base::Time::Exploded& time) const override;
+
+ private:
+  base::Time validity_start;
+  base::Time validity_end;
+
+  // Hashes of all revoked public keys.

[eroman, 2016/07/12 21:22:00]

document what the hash used is.

[ryanchung, 2016/07/14 16:15:25]

Done.

+  std::set<std::string> revoked_hashes_;
+
+  // Revoked serial number ranges indexed by issuer public key hash.
+  std::unordered_map<std::string, std::set<std::pair<uint64_t, uint64_t>>>
+      revoked_serial_numbers_;
+  DISALLOW_COPY_AND_ASSIGN(CastCRLImpl);
+};
+
+CastCRLImpl::CastCRLImpl(const TbsCrl& tbs_crl,
+                         const base::Time& overall_validity_end) {
+  // Parse the validity information.
+  validity_start =
+      base::Time::UnixEpoch() +
+      base::TimeDelta::FromMilliseconds(tbs_crl.issuance_time_millis());

[eroman, 2016/07/12 21:22:00]

This is done elsewhere too, suggest extracting to a helper.
Maybe TimeFromMillis() or something?

[ryanchung, 2016/07/14 16:15:25]

Done.

+  validity_end =
+      base::Time::UnixEpoch() +
+      base::TimeDelta::FromMilliseconds(tbs_crl.issuance_time_millis() +

[eroman, 2016/07/12 21:22:00]

Note that adding the two numbers in this fashion could result in an integer
overflow.
Instead convert them to TimeDelta's separately and then add those (addition of
TimeDelta saturates rather than overflow)

[ryanchung, 2016/07/14 16:15:25]

Done. The CRL proto has been updated to use not_before and not_after to avoid
adding time.

+                                        tbs_crl.validity_period_millis());
+  if (overall_validity_end < validity_end)
+    validity_end = overall_validity_end;
+
+  // Parse the revoked hashes.
+  for (const auto& hash : tbs_crl.revoked_public_key_hashes()) {
+    revoked_hashes_.insert(hash);
+  }
+
+  // Parse the revoked serial ranges.
+  for (const auto& range : tbs_crl.revoked_serial_number_ranges()) {
+    std::string issuer_hash = range.issuer_public_key_hash();
+
+    auto issuer_iter = revoked_serial_numbers_.find(issuer_hash);
+    if (issuer_iter == revoked_serial_numbers_.end()) {
+      std::set<std::pair<uint64_t, uint64_t>> serials;
+      std::pair<std::string, std::set<std::pair<uint64_t, uint64_t>>>
+          revocation_entry = std::make_pair(issuer_hash, serials);
+      issuer_iter = revoked_serial_numbers_.insert(revocation_entry).first;
+    }
+
+    uint64_t first_serial_number = range.first_serial_number();
+    uint64_t last_serial_number = range.last_serial_number();
+    std::pair<uint64_t, uint64_t> revocation_range(first_serial_number,
+                                                   last_serial_number);
+    issuer_iter->second.insert(revocation_range);
+  }
+}
+
+CastCRLImpl::~CastCRLImpl() {}
+
+// Verifies the revocation status of the certificate chain, at the specified
+// time.
+bool CastCRLImpl::CheckRevocation(
+    const std::vector<scoped_refptr<net::ParsedCertificate>>& trusted_chain,
+    const base::Time::Exploded& time) const {
+  if (trusted_chain.empty())
+    return false;
+
+  // Check the validity of the CRL at the specified time.
+  base::Time utc_time = base::Time::FromUTCExploded(time);
+  if ((utc_time < validity_start) || (utc_time >= validity_end)) {

[eroman, 2016/07/12 21:22:00]

validity_end corresponds with notAfter, so shouldn't this be > and not >= ?

[ryanchung, 2016/07/14 16:15:25]

Done.

+    VLOG(2) << "CRL not time-valid. Perform hard fail.";
+    return false;
+  }
+
+  // Check revocation.
+  std::string issuer_key_hash;
+  for (int i = trusted_chain.size() - 1; i >= 0; --i) {
+    // Check public key revocation.
+    scoped_refptr<net::ParsedCertificate> parsed_cert = trusted_chain[i];

[eroman, 2016/07/12 21:22:00]

can you use a const reference instead?

[ryanchung, 2016/07/14 16:15:25]

Done.

+    if (parsed_cert == nullptr) {
+      VLOG(2) << "Certificate chain not available.";

[eroman, 2016/07/12 21:22:00]

This shouldn't be reachable.

[ryanchung, 2016/07/14 16:15:25]

Done.

+      return false;
+    }
+    // Calculate the public key's hash.
+    std::string spki_hash =
+        crypto::SHA256HashString(parsed_cert->tbs().spki_tlv.AsString());
+    if (revoked_hashes_.find(spki_hash) != revoked_hashes_.end()) {
+      VLOG(2) << "Public key is revoked.";
+      return false;
+    }
+
+    // Check serial range revocation.
+    if (!issuer_key_hash.empty()) {
+      auto issuer_iter = revoked_serial_numbers_.find(issuer_key_hash);
+      if (issuer_iter != revoked_serial_numbers_.end()) {
+        uint64_t serial_number;
+        if (!net::der::ParseUint64(parsed_cert->tbs().serial_number,

[eroman, 2016/07/12 21:22:00]

Why is this sufficient?

Serial numbers can be 160 bits long.

And in fact based on
https://bugs.chromium.org/p/chromium/issues/detail?id=621645 it was determined
that they may not even parse as valid numbers

[ryanchung, 2016/07/14 16:15:25]

For serial range revocation, we will only revoke Google-generate certificates.
In this case, serials will be guaranteed to be less than 64 bits in length.

Other certificates are expected to be revoked through revocation of the whole
ICA.

[eroman, 2016/07/15 22:52:48]

Thanks for the explanation, that sounds reasonable.

And certainly the proto can always be extended later if this design requirement
changes and we want to support larger serial numbers, so this isn't a forever
commitment.

+                                   &serial_number)) {
+          continue;
+        }
+        for (auto const& revoked_serial : issuer_iter->second) {
+          if (revoked_serial.first <= serial_number &&
+              revoked_serial.second >= serial_number) {
+            VLOG(2) << "Serial number is revoked";
+            return false;
+          }
+        }
+      }
+    }
+    issuer_key_hash = spki_hash;
+  }
+  return true;
+}
+
+}  // namespace
+
+std::unique_ptr<CastCRL> ParseAndVerifyCRL(const std::string& crl_proto,
+                                           const base::Time::Exploded& time) {
+  CrlBundle crl_bundle;
+  if (!crl_bundle.ParseFromString(crl_proto)) {
+    LOG(ERROR) << "CRL - Binary could not be parsed.";
+    return nullptr;
+  }
+  for (auto const& crl : crl_bundle.crls()) {
+    TbsCrl tbs_crl;
+    if (!tbs_crl.ParseFromString(crl.tbs_crl())) {
+      LOG(WARNING) << "Binary TBS CRL could not be parsed.";
+      continue;
+    }
+    if (tbs_crl.version() != CRL_VERSION_0) {
+      continue;
+    }
+    base::Time overall_validity_end;
+    if (!VerifyCRL(crl, tbs_crl, time, &overall_validity_end)) {
+      LOG(ERROR) << "CRL - Verification failed.";
+      return nullptr;
+    }
+    return base::WrapUnique(new CastCRLImpl(tbs_crl, overall_validity_end));
+  }
+  LOG(ERROR) << "No supported version of revocation data.";
+  return nullptr;
+}
+
+bool SetCRLTrustAnchorForTest(const uint8_t* data, size_t length) {
+  scoped_refptr<net::ParsedCertificate> anchor(
+      net::ParsedCertificate::CreateFromCertificateData(
+          data, length, net::ParsedCertificate::DataSource::EXTERNAL_REFERENCE,
+          {}));
+  if (!anchor)
+    return false;
+  CastCRLTrustStore::Get().Clear();
+  CastCRLTrustStore::Get().AddTrustedCertificate(std::move(anchor));
+  return true;
+}
+
+void ClearCRLTrustAnchorForTest() {
+  CastCRLTrustStore::Get().Clear();
+}
+
+}  // namespace cast_certificate