Hook up Chrome Cast sender to Cast CRL.

extensions/browser/api/cast_channel/cast_auth_util.cc

Index: extensions/browser/api/cast_channel/cast_auth_util.cc
diff --git a/extensions/browser/api/cast_channel/cast_auth_util.cc b/extensions/browser/api/cast_channel/cast_auth_util.cc
index 0e5c38844cbe1e84e1e8210a4c4bbd56a9ac34ea..ababaa82fb3723cf2fd2f181ecbaf1e8a92e37e9 100644
--- a/extensions/browser/api/cast_channel/cast_auth_util.cc
+++ b/extensions/browser/api/cast_channel/cast_auth_util.cc
@@ -8,9 +8,12 @@
 
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/singleton.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "components/cast_certificate/cast_cert_validator.h"
+#include "components/cast_certificate/cast_crl.h"
+#include "crypto/sha2.h"
 #include "extensions/browser/api/cast_channel/cast_message_util.h"
 #include "extensions/common/api/cast_channel/cast_channel.pb.h"
 #include "net/cert/x509_certificate.h"
@@ -26,6 +29,10 @@ const char* const kParseErrorPrefix = "Failed to parse auth message: ";
 // The maximum number of days a cert can live for.
 const int kMaxSelfSignedCertLifetimeInDays = 4;
 
+// Enforce certificate revocation when set to true.
+// If set to false, then any revocation failures are ignored.
+const bool kEnforceRevocationCheck = false;
+
 namespace cast_crypto = ::cast_certificate;
 
 // Extracts an embedded DeviceAuthMessage payload from an auth challenge reply
@@ -126,17 +133,72 @@ AuthResult AuthenticateChallengeReply(const CastMessage& challenge_reply,
   return VerifyCredentials(response, peer_cert_der);
 }
 
+// Singleton for the Cast CRL cache.
+class CastCRLCache {
+ public:
+  static CastCRLCache* GetInstance() {
+    return base::Singleton<CastCRLCache,
+                           base::LeakySingletonTraits<CastCRLCache>>::get();
+  }
+
+  static cast_crypto::CastCRL* Get(const std::string& crl_bundle) {
+    if (crl_bundle.empty())
+      return nullptr;
+    CastCRLCache* cache = GetInstance();
+    std::string crl_bundle_hash = crypto::SHA256HashString(crl_bundle);
+    if (cache->hash_.compare(crl_bundle_hash) == 0) {
+      return cache->crl_.get();
+    } else {
+      return nullptr;
+    }
+  }
+
+  static cast_crypto::CastCRL* Put(const std::string& crl_bundle,
+                                   std::unique_ptr<cast_crypto::CastCRL> crl) {
+    if (crl_bundle.empty() || crl == nullptr)
+      return nullptr;
+    CastCRLCache* cache = GetInstance();
+    cache->hash_ = crypto::SHA256HashString(crl_bundle);
+    cache->crl_ = std::move(crl);
+    return cache->crl_.get();
+  }
+
+ private:
+  friend struct base::DefaultSingletonTraits<CastCRLCache>;
+
+  CastCRLCache() : crl_(nullptr), hash_() {}
+
+  std::unique_ptr<cast_crypto::CastCRL> crl_;
+  std::string hash_;
+  DISALLOW_COPY_AND_ASSIGN(CastCRLCache);
+};
+
 // This function does the following
 //
 // * Verifies that the certificate chain |response.client_auth_certificate| +
 //   |response.intermediate_certificate| is valid and chains to a trusted
-//   Cast root.
+//   Cast root. The list of trusted Cast roots can be overrided by providing a
+//   non-nullptr |cast_trust_store|. The certificate is verified at
+//   |cert_verification_time|.
+//
+// * Verifies that none of the certificates in the chain are revoked based on
+//   the CRL provided in the response |response.crl|. The CRL is verified to be
+//   valid and its issuer certificate chains to a trusted Cast CRL root. The
+//   list of trusted Cast CRL roots can be overrided by providing a non-nullptr
+//   |crl_trust_store|. If |crl_policy| is CRL_OPTIONAL then the result of
+//   revocation checking is ignored. The CRL is verified at
+//   |crl_verification_time|.
 //
 // * Verifies that |response.signature| matches the signature
 //   of |signature_input| by |response.client_auth_certificate|'s public
 //   key.
-AuthResult VerifyCredentials(const AuthResponse& response,
-                             const std::string& signature_input) {
+AuthResult VerifyCredentialsImpl(const AuthResponse& response,
+                                 const std::string& signature_input,
+                                 const cast_crypto::CRLPolicy& crl_policy,
+                                 net::TrustStore* cast_trust_store,
+                                 net::TrustStore* crl_trust_store,
+                                 const base::Time& cert_verification_time,
+                                 const base::Time& crl_verification_time) {
   // Verify the certificate
   std::unique_ptr<cast_crypto::CertVerificationContext> verification_context;
 
@@ -147,19 +209,64 @@ AuthResult VerifyCredentials(const AuthResponse& response,
                     response.intermediate_certificate().begin(),
                     response.intermediate_certificate().end());
 
-  // Use the current time when checking certificate validity.
-  base::Time now = base::Time::Now();
+  // Parse the CRL.
+  cast_crypto::CastCRL* crl = CastCRLCache::Get(response.crl());
+  if (crl == nullptr) {
+    std::unique_ptr<cast_crypto::CastCRL> crl_parsed;
+    if (crl_trust_store) {
+      crl_parsed = cast_crypto::ParseAndVerifyCRLUsingCustomTrustStore(
+          response.crl(), crl_verification_time, crl_trust_store);
+    } else {
+      crl_parsed =
+          cast_crypto::ParseAndVerifyCRL(response.crl(), crl_verification_time);
+    }
 
-  // CRL should not be enforced until it is served.
-  cast_crypto::CastDeviceCertPolicy device_policy;
-  if (!cast_crypto::VerifyDeviceCert(
-          cert_chain, now, &verification_context, &device_policy, nullptr,
-          cast_certificate::CRLPolicy::CRL_OPTIONAL)) {
-    // TODO(eroman): The error information was lost; this error is ambiguous.
-    return AuthResult("Failed verifying cast device certificate",
-                      AuthResult::ERROR_CERT_NOT_SIGNED_BY_TRUSTED_CA);
+    crl = CastCRLCache::Put(response.crl(), std::move(crl_parsed));

[eroman, 2016/09/10 01:03:23]

I don't understand the caching behavior here.
 * What happens when it expires?
 * This is not thread-safe, is that expected?
 * The cache is shared by custom and-non custom trust store invocations. Does
this mean a caller with bogus trust store can populate a cache entry that will
then be used when this is called again with default trust store, and possibly
consider that CRL to be trusted?

[ryanchung, 2016/09/14 18:53:40]

* What happens when it expires?
This is meant to be a dumb cache. It just stores the last successfully parsed
CRL and does not track expiry. The CastCRL itself tracks the expiry of the
information and will cause a failure in VerifyDeviceCert() when appropriate.

The cached CastCRL will only be used if it matches the one passed in |response|.
So I guess it is a little confusing here because if an expired CRL from
|response| is not cached, then it'll fail to ParseAndVerifyCRL(), but if it is
already cached, then we'll fail to VerifyDeviceCert(). 

* This is not thread-safe, is that expected?
Ah you're right, that's a problem. The cache owns the parsed CastCRL. Not sure
how to make this work without using some ref-counting and maybe AutoLock for
Get() and Put().

* The cache is shared by custom and-non custom trust store invocations.
Good point. The cache should not be used in the custom trust store scenario
since the state of the trust store is hard to track.

Thinking about this whole thing, I'm no longer sure if it is worthwhile to cache
the parsed CRL.
The use case is that the device will always send a copy of the CRL with its
certificate during device auth. The idea was to save some compute power by not
processing the CRL if it is the same since all devices share the same CRL. But
I'm not sure its worth the complexity of having a global state. What would you
suggest?

[eroman, 2016/09/14 19:46:42]

My suggestion would be to not use a cache unless it is needed. "if it is needed"
is best answered by a benchmark.

From a code-review perspective if you do decide to add a cache I think it is
worth proposing as a separate CL for a more thorough review (and then we can
land the uncontentious parts first).

Here is my thinking:

If this isn't on a critical path, then the advantage of not caching is like you
said code simplicity, and also a smaller memory footprint (since there won't be
lingering memory usage for the CRL cache). Since the CRL is being provided
always anyway, the time saving is just the parsing and verification of it, and
not an actual fetch time, so meh.

I haven't run numbers, but my gut feeling is that verifying the CRL is probably
around 50% of the overall cost of doing the device cert verification. It depends
on the length of the respective certificate chains, but the dominant cost will
likely be the RSA signature verification (so count up how many
VerifySignedData() calls each does for an approximate cost model).

If it turns out that performance of VerifyDeviceCert() is too slow, we may want
to consider broader caching not just for CRL part, but for the target
certificate too, so a more general cache may be what we want instead.

Assuming we do decide on a cache, my first thought is to introduce a sort of
"context" object, that encapsulates the trust store + caches. Instead of passing
in a custom trust store as we do today, tests would pass in a custom "context"
object instead (which contains the trust store).

The default context when unspecified could be a global (much how the default
trust store is a global today). If we don't anticipate multi-threaded callers,
then adding some assertions to ensure the entry points (VerifyDeviceCert())
aren't called on other threads should be sufficient.

I dunno, just thinking out loud.

[ryanchung, 2016/09/14 23:35:48]

I agree. Thanks for the insights. I under-estimated the complexity of adding a
cache. Will remove the cache for now and re-evaluate if it is needed.

This code is on a critical path. The device must be authenticated before casting
can begin. My feeling is that the cost is negligible considering the performance
of any modern computer. Maybe I should do some benchmarks to check. However,
there are such a large range of computers out there I'm not sure what threshold
would be considered too slow.

We also have metrics for time-to-launch Cast session, so I supposed that can be
monitored.

+  }
+  if (!crl && crl_policy == cast_crypto::CRLPolicy::CRL_REQUIRED) {
+    // CRL is invalid.
+    return AuthResult("Failed verifying Cast CRL.",
+                      AuthResult::ERROR_CRL_INVALID);
   }
 
+  cast_crypto::CastDeviceCertPolicy device_policy;
+  bool verification_success;
+  if (cast_trust_store) {
+    verification_success = cast_crypto::VerifyDeviceCertUsingCustomTrustStore(
+        cert_chain, cert_verification_time, &verification_context,
+        &device_policy, crl, crl_policy, cast_trust_store);
+  } else {
+    verification_success = cast_crypto::VerifyDeviceCert(
+        cert_chain, cert_verification_time, &verification_context,
+        &device_policy, crl, crl_policy);
+  }
+  if (!verification_success) {
+    // TODO(ryanchung): Once this feature is completely rolled-out, remove the
+    // reverification step and use error reporting to get verification errors
+    // for metrics.
+    bool verification_no_crl_success;
+    if (cast_trust_store) {
+      verification_no_crl_success =
+          cast_crypto::VerifyDeviceCertUsingCustomTrustStore(
+              cert_chain, cert_verification_time, &verification_context,
+              &device_policy, nullptr, cast_crypto::CRLPolicy::CRL_OPTIONAL,
+              cast_trust_store);
+    } else {
+      verification_no_crl_success = cast_crypto::VerifyDeviceCert(
+          cert_chain, cert_verification_time, &verification_context,
+          &device_policy, nullptr, cast_crypto::CRLPolicy::CRL_OPTIONAL);
+    }
+    if (!verification_no_crl_success) {
+      // TODO(eroman): The error information was lost; this error is ambiguous.
+      return AuthResult("Failed verifying cast device certificate",
+                        AuthResult::ERROR_CERT_NOT_SIGNED_BY_TRUSTED_CA);
+    }
+    if (crl_policy == cast_crypto::CRLPolicy::CRL_REQUIRED) {
+      // Device is revoked.
+      return AuthResult("Failed certificate revocation check.",
+                        AuthResult::ERROR_CERT_REVOKED);
+    }
+  }
   if (!verification_context->VerifySignatureOverData(response.signature(),
                                                      signature_input)) {
     return AuthResult("Failed verifying signature over data",
@@ -181,6 +288,29 @@ AuthResult VerifyCredentials(const AuthResponse& response,
   return success;
 }
 
+AuthResult VerifyCredentials(const AuthResponse& response,
+                             const std::string& signature_input) {
+  base::Time now = base::Time::Now();
+  cast_crypto::CRLPolicy policy = cast_crypto::CRLPolicy::CRL_REQUIRED;
+  if (!kEnforceRevocationCheck) {
+    policy = cast_crypto::CRLPolicy::CRL_OPTIONAL;
+  }
+  return VerifyCredentialsImpl(response, signature_input, policy, nullptr,
+                               nullptr, now, now);
+}
+
+AuthResult VerifyCredentialsForTest(const AuthResponse& response,
+                                    const std::string& signature_input,
+                                    const cast_crypto::CRLPolicy& crl_policy,
+                                    net::TrustStore* cast_trust_store,
+                                    net::TrustStore* crl_trust_store,
+                                    const base::Time& cert_verification_time,
+                                    const base::Time& crl_verification_time) {
+  return VerifyCredentialsImpl(response, signature_input, crl_policy,
+                               cast_trust_store, crl_trust_store,
+                               cert_verification_time, crl_verification_time);
+}
+
 }  // namespace cast_channel
 }  // namespace api
 }  // namespace extensions