Auth work to support new auth protocol

chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc

Index: chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc
diff --git a/chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc b/chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc
index af1e0f362e58c6b20566f067ae2f586be9acaa88..52b6b363db55afe01557cfebd5c3af8c0d98fabd 100644
--- a/chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc
+++ b/chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc
@@ -15,6 +15,9 @@
 #include "chrome/browser/extensions/api/cast_channel/cast_message_util.h"
 #include "crypto/nss_util.h"
 #include "crypto/scoped_nss_types.h"
+#include "net/base/hash_value.h"
+#include "net/cert/asn1_util.h"
+#include "net/cert/x509_certificate.h"
 
 namespace {
 
@@ -44,11 +47,30 @@ static const unsigned char kCAPublicKeyDER[] = {
     0x61, 0x47, 0x9e, 0xab, 0x80, 0xb7, 0xe4, 0x48, 0x80, 0x2a, 0x92, 0xc5,
     0x1b, 0x02, 0x03, 0x01, 0x00, 0x01 };
 
+static const net::SHA1HashValue kICAFingerprints[] = {

[Ryan Sleevi, 2014/04/30 01:32:13]

Note: We don't use fingerprints for any (granting) of security privileges - we
always do a full comparison of the cert if the fingerprints hit, in part because
certs are crazy.

Instead of fingerprints, can you use the three trusted public keys from these
certs?

[Munjal (Google), 2014/05/05 00:01:46]

Let me see if I understand correctly. So you are suggesting a change to the
solution we discussed over email in that instead of just checking finger prints,
we check the whole public key of the intermediate cert?

[Ryan Sleevi, 2014/05/05 19:35:32]

I apologize for the confusion. The goal is to have the same security guarantees
you have now. The fingerprint approach is safe if and only if you're checking
the signature against a root - that is, that there is no chance of attacks on
the fingerprint alg (SHA-1) using the structure of an X.509 certificate.

Since you're not - you're wanting to use the ICA as a trust anchor - you should
be checking using the same method you're already using to check a trust anchor -
that is, the public key.

[Munjal (Google), 2014/05/05 21:00:27]

So are you suggesting that we hard-code a list of public keys?

+  { { 0x57, 0x16, 0xe2, 0xad, 0x73, 0x2e, 0xbe, 0xda, 0xeb, 0x18,
+      0xe8, 0x47, 0x15, 0xa8, 0xde, 0x90, 0x3b, 0x5e, 0x2a, 0xf4 } },
+  { { 0x1b, 0xa2, 0x9e, 0xc9, 0x8e, 0x4e, 0xb3, 0x80, 0xee, 0x55,
+      0xb2, 0x97, 0xfd, 0x2e, 0x2b, 0x2c, 0xb6, 0x8e, 0x0b, 0x2f} },
+  { { 0x97, 0x05, 0xce, 0xf6, 0x3f, 0xa9, 0x5e, 0x0f, 0xe7, 0x61,
+      0xfB, 0x08, 0x44, 0x31, 0xbe, 0xde, 0x01, 0xb8, 0xfB, 0xeb} }
+};
+
 typedef scoped_ptr<
     CERTCertificate,
     crypto::NSSDestroyer<CERTCertificate, CERT_DestroyCertificate> >
         ScopedCERTCertificate;
 
+// Returns whether |fingerprint| matches one of the hard-coded list of
+// allowed ICA fingerprints.
+static bool IsFingerprintAllowed(const net::SHA1HashValue& fingerprint) {
+  for (size_t i = 0; i < arraysize(kICAFingerprints); ++i) {
+    if (fingerprint.Equals(kICAFingerprints[i]))
+      return true;
+  }
+  return false;
+}
+
 // Parses out DeviceAuthMessage from CastMessage
 static bool ParseAuthMessage(
     const extensions::api::cast_channel::CastMessage& challenge_reply,
@@ -81,9 +103,32 @@ static bool ParseAuthMessage(
 // Authenticates the given credentials:
 // 1. |signature| verification of |data| using |certificate|.
 // 2. |certificate| is signed by a trusted CA.
-bool VerifyCredentials(const std::string& certificate,
-                       const std::string& signature,
-                       const std::string& data) {
+bool VerifyCredentials(
+    const extensions::api::cast_channel::AuthResponse& response,
+    const std::string& data) {
+  const std::string& certificate = response.client_auth_certificate();
+  const std::string& signature = response.signature();
+
+  std::string ica_public_key_str;
+  // If the list of intermediates is not empty, then the fingerprint of
+  // the first intermediate should be one of the hard-coded list of
+  // fingerprints.
+  if (response.intermediate_certificate_size() > 0) {

[mark a. foltz, 2014/05/05 19:22:22]

ISTM having two helper functions that extract the public certificate from the
response - one for the intermediates case, one without - would make for cleaner
code.  Each would return (or populate) trusted_ca_key_der_item.

+    const std::string& ica = response.intermediate_certificate(0);
+    scoped_refptr<net::X509Certificate> ica_cert
+        = net::X509Certificate::CreateFromBytes(ica.data(), ica.length());
+    if (!IsFingerprintAllowed(ica_cert->fingerprint())) {
+      VLOG(1) << "Disallowed intermdiate cert: " << ica;

[mark a. foltz, 2014/05/05 19:22:22]

ica looks to be a binary blob that is parsed to yield the certificate.  Please
log something human readable, or failing that, the byte length of the blob. 

+      return false;
+    }
+    base::StringPiece ica_public_key;
+    if (!net::asn1::ExtractSPKIFromDERCert(ica, &ica_public_key)) {
+      VLOG(1) << "Could not extract public key from intermediate cert: " << ica;

[mark a. foltz, 2014/05/05 19:22:22]

Ditto.

+      return false;
+    }
+    ica_public_key_str = ica_public_key.as_string();
+  }

[Munjal (Google), 2014/05/05 00:01:46]

Ryan, does this logic look OK to you?

+
   crypto::EnsureNSSInit();
   SECItem der_cert;
   der_cert.type = siDERCertBuffer;
@@ -103,8 +148,16 @@ bool VerifyCredentials(const std::string& certificate,
   // Check that the certificate is signed by trusted CA.
   SECItem trusted_ca_key_der_item;
   trusted_ca_key_der_item.type = siDERCertBuffer;
-  trusted_ca_key_der_item.data = const_cast<unsigned char*>(kCAPublicKeyDER);
-  trusted_ca_key_der_item.len = sizeof(kCAPublicKeyDER);
+  // Use the hard-coded CA public key if response has no intermediates.
+  if (ica_public_key_str.empty()) {
+    trusted_ca_key_der_item.data = const_cast<unsigned char*>(kCAPublicKeyDER);
+    trusted_ca_key_der_item.len = sizeof(kCAPublicKeyDER);
+  } else {
+    trusted_ca_key_der_item.data = reinterpret_cast<unsigned char*>(

[mark a. foltz, 2014/05/05 19:22:22]

You are taking the data pointer out of ica_public_key_str, casting away the
constness and giving it to a data structure that is passed to another library. 
This is not safe as any modification to the char array will not be reflected in
the state of ica_public_key_str.  IMHO it would be safer to allocate a copy for
the SECItem.

[Ryan Sleevi, 2014/05/05 19:35:32]

This is fine/safe/blessed. NSS is slowly becoming most explicit in its
const-correctness. I'll file an upstream bug to note that both the
CERTSignedData and the CERTSubjectPublicKeyInfo are 'const' (implying they're
safe from modification - even though the SECItem takes a char*)

+        const_cast<char*>(ica_public_key_str.data()));
+    trusted_ca_key_der_item.len = ica_public_key_str.length();
+  }
+
   crypto::ScopedSECKEYPublicKey ca_public_key(
       SECKEY_ImportDERPublicKey(&trusted_ca_key_der_item, CKK_RSA));
   SECStatus verified = CERT_VerifySignedDataWithPublicKey(
@@ -157,9 +210,7 @@ bool AuthenticateChallengeReply(const CastMessage& challenge_reply,
     return false;
 
   const AuthResponse& response = auth_message.response();
-  return VerifyCredentials(response.client_auth_certificate(),
-                           response.signature(),
-                           peer_cert);
+  return VerifyCredentials(response, peer_cert);
 }
 
 }  // namespace cast_channel