Cast device revocation checking.

components/cast_certificate/cast_crl.cc

+// 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/15 22:52:48]

nit: TODO(ryanchung)
(not sure if that is normative or not)

Why aren't we adding it yet again? Because it hasn't been generated?

[ryanchung, 2016/07/18 23:39:08]

The root hasn't been generated due to administrative reasons...
We could delay submitting this CL until it is ready if that is preferred.
Fixed the nit.

[eroman, 2016/07/19 01:54:58]

That's fine, can submit a follow-up CL, and include a test verifying with that
root too.

+    // 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.
+net::der::GeneralizedTime convertTimeExploded(

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

(1) convertTimeExploded --> ConvertTimeExploded
(2) Note that we now have net::der::EncodeTimeAsGeneralizedTime(), which could
perhaps be used (assuming you change the API to take a base::Time instead).

[ryanchung, 2016/07/18 23:39:08]

Went with (2)
I've updated the API in cast_crl.h and cast_cert_validator.h to use base::Time
instead of base::Time::ExplodedTime.

+    const base::Time::Exploded& exploded) {
+  net::der::GeneralizedTime result;
+  result.year = exploded.year;
+  result.month = exploded.month;
+  result.day = exploded.day_of_month;
+  result.hours = exploded.hour;
+  result.minutes = exploded.minute;
+  result.seconds = exploded.second;
+  return result;
+}
+
+// Converts a uint64_t utc time to net::der::GeneralizedTime.

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

not sure that "utc time" is the right terminology. How about "unix timestamp",
or "seconds since unix epoch" instead?

[ryanchung, 2016/07/18 23:39:08]

Done.

+net::der::GeneralizedTime ConvertTimeSeconds(uint64_t seconds) {
+  base::Time utc_time =
+      base::Time::UnixEpoch() + base::TimeDelta::FromSeconds(seconds);

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

This has the possibility of overflow -- FromSeconds takes an int64_t but we are
passing it a uint64_t, so it might wrap around to negative.

This may not cause problems but is technically relying on undefined behavior
which is undesirable (integer overflow for signed types is undefined).

Can you use base::saturated_cast<> instead?

[ryanchung, 2016/07/18 23:39:08]

Done.

+  base::Time::Exploded exploded;
+  utc_time.UTCExplode(&exploded);
+  return convertTimeExploded(exploded);
+}
+
+// Specifies the signature verification policy.
+// The required algorithms are:
+// RSASSA PKCS#1 v1.5 with SHA-256, using RSA keys 2048-bits or longer.
+std::unique_ptr<net::SignaturePolicy> CreateCastSignaturePolicy() {
+  return base::WrapUnique(new net::SimpleSignaturePolicy(2048));
+}
+
+// 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_not_after|.
+bool VerifyCRL(const Crl& crl,
+               const TbsCrl& tbs_crl,
+               const base::Time::Exploded& time,
+               net::der::GeneralizedTime* overall_not_after) {
+  // 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.

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

nti: "Parse the signature" --> "Wrap the signature in a BitString"

(No parsing is actually being done here, just encapsulating the bytes in a
different type.. because X.509 is silly and uses a BitString rather than octet
string).

[ryanchung, 2016/07/18 23:39:08]

Done.

+  net::der::BitString signature_value_bit_string = net::der::BitString(
+      net::der::Input(base::StringPiece(crl.signature())), 0);
+
+  // Verify the signature.
+  auto signature_policy = CreateCastSignaturePolicy();
+  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,
+                        signature_policy.get())) {
+    VLOG(2) << "CRL - Signature verification failed.";
+    return false;
+  }
+
+  // Verify the issuer certificate.
+  net::CertPathBuilder::Result result;
+  net::CertPathBuilder path_builder(
+      parsed_cert.get(), &CastCRLTrustStore::Get(), signature_policy.get(),
+      convertTimeExploded(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;
+  }

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

Please add a comment about why no checks on the leaf certificate are necessary
(i.e. there are no requirements placed here on the leaf certificate having any
particular keyUsages).

[ryanchung, 2016/07/18 23:39:08]

Done.
Ahhh just realized after uploading Patch23... I added the comment on line 131.
I'll move it down here in the next patch.

+
+  // Verify the CRL is still valid.
+  net::der::GeneralizedTime verification_time = convertTimeExploded(time);
+  net::der::GeneralizedTime not_before =
+      ConvertTimeSeconds(tbs_crl.not_before_seconds());

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

What is the role of setting a notBefore on the CRL?

Presumably once something is deemed revoked, it should be revoked immediately
and forever.
I don't think there is harm in having a notBefore, just wondering if it will
actually be useful.

In this case CRL verification fails if the current time is before not_before

[ryanchung, 2016/07/18 23:39:08]

That's a very good point.
If a certificate expires, its revocation information will no longer be
distributed in the CRL (to maybe reduce the size...). If a verifier's clock is
slow and is in the past, the revoked certificate is still time-valid (to this
verfier) but the CRL no longer lists the certificate as being revoked.

+  net::der::GeneralizedTime not_after =
+      ConvertTimeSeconds(tbs_crl.not_after_seconds());
+  if ((verification_time < not_before) || (verification_time > not_after)) {
+    VLOG(2) << "CRL - Not time-valid.";
+    return false;
+  }
+
+  // Set CRL expiry to the earliest of the cert chain expiry and CRL expiry.
+  *overall_not_after = not_after;
+  for (const auto& cert : result.paths[result.best_result_index]->path) {
+    net::der::GeneralizedTime cert_not_after = cert->tbs().validity_not_after;
+    if (cert_not_after < *overall_not_after)
+      *overall_not_after = cert_not_after;
+  }
+
+  // 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 net::der::GeneralizedTime& overall_not_after);
+  ~CastCRLImpl() override;
+
+  bool CheckRevocation(const net::ParsedCertificateList& trusted_chain,
+                       const base::Time::Exploded& time) const override;
+
+ private:
+  net::der::GeneralizedTime not_before;

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

name private variables with trailing underscore.

[ryanchung, 2016/07/18 23:39:08]

Done.

+  net::der::GeneralizedTime not_after;

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

same

[ryanchung, 2016/07/18 23:39:08]

Done.

+
+  // Revoked public key hashes.
+  // The values consist of the SHA256 hash of the SubjectPublicKeyInfo.
+  std::set<std::string> revoked_hashes_;
+
+  // Revoked serial number ranges indexed by issuer public key hash.
+  // The key is the SHA256 hash of issuer's SubjectPublicKeyInfo.
+  // The value is a list of revoked serial number ranges.
+  std::unordered_map<std::string, std::set<std::pair<uint64_t, uint64_t>>>

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

For the second part, I suggest using a std::vector<std::pair<,,>> rather than a
std::set<std::pair<,,>>.

It is basically just iterated over O(n) anyway, so a vector will be more
efficient.
The only advantage of a set is de-duplication if the server publishes redundant
ranges.
Not sure that is something we should worry about on the client side.

[ryanchung, 2016/07/18 23:39:07]

Done.

+      revoked_serial_numbers_;
+  DISALLOW_COPY_AND_ASSIGN(CastCRLImpl);
+};
+
+CastCRLImpl::CastCRLImpl(const TbsCrl& tbs_crl,
+                         const net::der::GeneralizedTime& overall_not_after) {
+  // Parse the validity information.
+  not_before = ConvertTimeSeconds(tbs_crl.not_before_seconds());
+  not_after = ConvertTimeSeconds(tbs_crl.not_after_seconds());
+  if (overall_not_after < not_after)
+    not_after = overall_not_after;
+
+  // 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);

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

How about using operator[], which combines the "get if it exists, otherwise add
and default construct".
Could look something like this:

auto& serial_number_range = revoked_serial_numbers_[issuer_hash];
serial_number_range.insert(std::make_pair(first_serial_number,
last_serial_number));

[ryanchung, 2016/07/18 23:39:08]

Done.

+    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,

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

Can you define a struct for this instead?

struct SerialNumberRange {
  .. first;
  .. last;
};

I find std::pair<> hard to read.

[ryanchung, 2016/07/18 23:39:08]

Done.

+                                                   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 net::ParsedCertificateList& trusted_chain,
+    const base::Time::Exploded& time) const {
+  if (trusted_chain.empty())
+    return false;
+
+  // Check the validity of the CRL at the specified time.
+  net::der::GeneralizedTime verification_time = convertTimeExploded(time);
+  if ((verification_time < not_before) || (verification_time > not_after)) {
+    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) {

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

style: I am generally fearful of this sort of loop construct, since changing the
loop counter to "size_t" becomes a bug (and technically the initial assignment
to "i" is an unchecked cast that could underflow).

I prefer either of these constructs instead:

// Iterate the chain in reverse
for (auto it = trusted_chain.rbegin(); it != trusted_chain.rend(); ++it) {
  const auto& parsed_cert = *it;
  ....
}

Or alternately:

  for (size_t i = 0; i < trusted_chain.size(); ++i) {
    // Iterate in reverse order
    const auto& parsed_cert = trusted_chain[trusted_chain.size() - 1 - i];
  }


Or another way is to restructure this so you iterate the chain in the "forward"
direction.
Instead of saving "issuer_key_hash", you could do it as follows:

  for (size_t i = 0; i < trusted_chain.size(); ++i) {
      ....

      // Check if the subordinate certificate was revoked by serial number
      if (i > 0) {
         const auto& subordinate = trusted_chain[i - 1];
         // Lookup in revoked_serial_numbers by (spki_hash,
subordinate->tbs().serial_number)..
      }
   }

[ryanchung, 2016/07/18 23:39:08]

Done. Forward direction sounds good. Thanks!

+    // Check public key revocation.
+    const scoped_refptr<net::ParsedCertificate> parsed_cert = trusted_chain[i];
+    // 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;
+        // Only Google generated device certificates will be revoked by range.
+        // These will always be less than 64 bits in length.
+        if (!net::der::ParseUint64(parsed_cert->tbs().serial_number,
+                                   &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;
+    }
+    net::der::GeneralizedTime overall_not_after;
+    if (!VerifyCRL(crl, tbs_crl, time, &overall_not_after)) {
+      LOG(ERROR) << "CRL - Verification failed.";
+      return nullptr;

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

Failing here seems like a fine choice.
However it does perhaps seem inconsistent with the code above which "continues"
when failing to parse the protobuffer.

No change required, just observing to confirm this was intentional.

[ryanchung, 2016/07/18 23:39:08]

The CRL bundle will contain at max a single copy for each CRL version.
When a CRL fails to parse or is the wrong version, there could be other CRLs in
the bundle that is supported.
However, since only one version of the CRL is supported at this time, a
supported CRL that fails to verify indicates definitively that nothing in the
CRL bundle is usable.

+    }
+    return base::WrapUnique(new CastCRLImpl(tbs_crl, overall_not_after));
+  }
+  LOG(ERROR) << "No supported version of revocation data.";
+  return nullptr;
+}
+
+bool SetCRLTrustAnchorForTest(const std::string& cert) {
+  scoped_refptr<net::ParsedCertificate> anchor(
+      net::ParsedCertificate::CreateFromCertificateCopy(cert, {}));
+  CastCRLTrustStore::Get().Clear();
+  if (!anchor)
+    return false;
+  CastCRLTrustStore::Get().AddTrustedCertificate(std::move(anchor));
+  return true;
+}
+
+}  // namespace cast_certificate