Implement crypto signature verification routines using OpenSSL.

extensions/browser/api/cast_channel/cast_auth_util_openssl.cc

Index: extensions/browser/api/cast_channel/cast_auth_util_openssl.cc
diff --git a/extensions/browser/api/cast_channel/cast_auth_util_openssl.cc b/extensions/browser/api/cast_channel/cast_auth_util_openssl.cc
index 3ebceed1f107a848a36cfdeb2424c08e2592e85e..05b1425b7d510254c2557459019f2027aac250e3 100644
--- a/extensions/browser/api/cast_channel/cast_auth_util_openssl.cc
+++ b/extensions/browser/api/cast_channel/cast_auth_util_openssl.cc
@@ -4,15 +4,167 @@
 
 #include "extensions/browser/api/cast_channel/cast_auth_util.h"
 
+#include <openssl/evp.h>
+#include <openssl/rsa.h>
+#include <openssl/x509.h>
+#include <stddef.h>
+
 #include "base/logging.h"
+#include "crypto/scoped_openssl_types.h"
+#include "extensions/browser/api/cast_channel/cast_auth_ica.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"
 
 namespace extensions {
 namespace core_api {
 namespace cast_channel {
+namespace {
+
+typedef crypto::ScopedOpenSSL<X509, X509_free>::Type ScopedX509;
+
+}  // namespace
+
+// This function does the following
+// * Verifies that |auth_message.response.client_auth_certificate| is signed

[davidben, 2014/10/30 18:37:23]

Nit: s/auth_message.//

[Kevin M, 2014/10/31 21:39:37]

Done.

+//   by the trusted CA public key passed as |ca_public_key_bytes|.

[davidben, 2014/10/30 18:37:23]

Nit: ca_public_key_bytes is no longer a parameter.

[Kevin M, 2014/10/31 21:39:37]

Done.

+// * Verifies that |auth_message.response.signature| matches the signature
+//   of |peer_cert| by |auth_message.response.client_auth_certificate|'s public
+//   key.
+AuthResult VerifyCredentials(const AuthResponse& response,
+                             const std::string& peer_cert) {

[davidben, 2014/10/30 18:37:23]

This should include crypto/openssl_util.h and add a

   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);

line at the top. This'll ensure that the OpenSSL error queue gets cleared on
function exit. And it'll print the errors on debug builds or when logging is
enabled, which is probably convenient.

[Kevin M, 2014/10/31 21:39:37]

That's neat, Done.

+  // Get the public key of the ICA that was used to sign the client's cert.
+  base::StringPiece ca_public_key_bytes;
+  if (response.intermediate_certificate().size() <= 0) {
+    ca_public_key_bytes = GetDefaultTrustedICAPublicKey();
+  } else {
+    ca_public_key_bytes =
+        GetTrustedICAPublicKey(response.intermediate_certificate(0));
+    if (ca_public_key_bytes.empty()) {
+      LOG(ERROR) << "Couldn't find trusted ICA.";
+      return AuthResult::Create(

[mark a. foltz, 2014/10/31 06:49:11]

The AuthResult API has evolved with vadimgo's patch, you may need to
reconstitute this method.

[Kevin M, 2014/10/31 21:39:37]

Done.

+          "failed to verify credentials: cert not signed by trusted CA",
+          AuthResult::ERROR_NSS_CERT_NOT_SIGNED_BY_TRUSTED_CA);
+    }
+  }
+
+  // Parse the CA public key.
+  const uint8_t* ca_ptr =
+      reinterpret_cast<const uint8_t*>(ca_public_key_bytes.data());
+  const uint8_t* ca_public_key_end = ca_ptr + ca_public_key_bytes.size();

[mark a. foltz, 2014/10/31 06:49:11]

Why are you calling size() here and length() below?

[mark a. foltz, 2014/10/31 06:49:11]

What is ca_public_key_end pointing to?  It seems to be pointing just past the
end of ca_public_key_bytes_data - but there can be no valid key contents there?

[Kevin M, 2014/10/31 21:39:37]

It's the expected value of cert_ptr after d2i_X509, which increments the cert
data ptr on successful read.

[Kevin M, 2014/10/31 21:39:37]

Good catch, done

[Kevin M, 2014/10/31 21:39:37]

Done.

+  crypto::ScopedRSA ca_public_key_rsa(
+      d2i_RSAPublicKey(NULL, &ca_ptr, ca_public_key_bytes.length()));
+  if (!ca_public_key_rsa || ca_ptr != ca_public_key_end) {
+    LOG(ERROR) << "Failed to import trusted public key.";
+    return AuthResult::Create(
+        "failed to import trusted public key.",
+        AuthResult::ERROR_NSS_CERT_PARSING_FAILED);
+  }
+  crypto::ScopedEVP_PKEY ca_public_key(EVP_PKEY_new());
+  if (!ca_public_key ||

[mark a. foltz, 2014/10/31 06:49:11]

What type is ca_public_key - I assume it has a !operator?

[Kevin M, 2014/10/31 21:39:37]

It's a scoped_ptr. I can't find an operator! defined for scoped_ptr, but I've
seen it being used this way in other code that uses OpenSSL.

https://code.google.com/p/chromium/codesearch#search/&q=scopedevp_pkey%20%22!...

+      !EVP_PKEY_set1_RSA(ca_public_key.get(), ca_public_key_rsa.get())) {
+    LOG(ERROR) << "Failed to initialize EVP_PKEY";
+    return AuthResult::Create(
+        "failed to initialize EVP_PKEY.",
+        AuthResult::ERROR_NSS_CANNOT_EXTRACT_PUBLIC_KEY);
+  }
+
+  // Parse the client auth certificate.
+  const uint8_t* client_cert_ptr = reinterpret_cast<const uint8_t*>(
+      response.client_auth_certificate().data());
+  const uint8_t* client_cert_end =

[mark a. foltz, 2014/10/31 06:49:11]

Similar questions about the use of client_cert_end

[Kevin M, 2014/10/31 21:39:37]

Acknowledged.

+      client_cert_ptr +
+      response.client_auth_certificate().size();
+  const ScopedX509 client_cert(
+      d2i_X509(NULL, &client_cert_ptr,
+               response.client_auth_certificate().size()));
+  if (!client_cert || client_cert_ptr != client_cert_end) {
+    LOG(ERROR) << "Failed to parse certificate.";
+    return AuthResult::Create(
+        "failed to parse client_auth_certificate.",
+        AuthResult::ERROR_NSS_CERT_PARSING_FAILED);
+  }
+
+  // Verify that the client auth certificate was signed by a trusted CA.
+  if (X509_verify(client_cert.get(), ca_public_key.get()) <= 0) {

[mark a. foltz, 2014/10/31 06:49:11]

Does X509_verify return specific error codes?  If so they should be passed with
the AuthResult.

[Kevin M, 2014/10/31 21:39:37]

From what I could tell, it's one or zero "except in certain exceptional
circumstances" (according to the OpenSSL wiki).

But, from David's comment in this CL: "Going forward, BoringSSL functions are
going to try to avoid being tri-state like that, but it'll take a while to go
through all the legacy functions."

[davidben, 2014/11/01 00:19:34]

For stuff in crypto/, whether it's 0 or -1 is almost always uninteresting and is
going to be redundant with the error queue.

(As it turns out, it looks like X509_verify in BoringSSL already isn't tri-state
anymore.)

+    LOG(ERROR) << "Certificate is not issued by a trusted CA.";
+      return AuthResult::Create(
+          "cert not signed by trusted CA",
+          AuthResult::ERROR_NSS_CERT_NOT_SIGNED_BY_TRUSTED_CA);
+  }
+
+  // Get the client auth certificate's public key.
+  const crypto::ScopedEVP_PKEY client_public_key(
+      X509_get_pubkey(client_cert.get()));
+  const int client_public_key_type = EVP_PKEY_id(client_public_key.get());
+  if (client_public_key_type != EVP_PKEY_RSA) {
+    LOG(ERROR) << "Expected RSA key type for client certificate, got "
+               << client_public_key_type << " instead.";
+    return AuthResult::Create(
+        "couldn't extract public_key from client cert.",
+        AuthResult::ERROR_NSS_CANNOT_EXTRACT_PUBLIC_KEY);

[mark a. foltz, 2014/10/31 06:49:11]

Is there an OpenSSL equivalent to PORT_GetError() that would allow us to pass a
specific failure reason back through AuthResult for all of these library calls? 
We want to make sure to include them in our channel event logs and OnError
events.

+  }
+
+  // Check that the SSL peer certificate was signed using the client's public
+  // key.
+  const crypto::ScopedEVP_MD_CTX ctx(EVP_MD_CTX_create());
+  if (!ctx) {
+    LOG(ERROR) << "Unable to allocate EVP_MD_CTX.";
+    return AuthResult::Create(

[davidben, 2014/10/30 18:37:23]

You could probably just chain these few together with ||s and let
OpenSSLErrStackTracer provide the context needed, but this is fine too.

[Kevin M, 2014/10/31 21:39:37]

Having conditional blocks for each step of the way allows us to add more
detailed logging info to AuthResult, which is useful for us.

+        "unable to allocate EVP_MD_CTX.",
+        AuthResult::ERROR_UNEXPECTED_AUTH_LIBRARY_RESULT);
+  }
+  if (!EVP_DigestVerifyInit(ctx.get(), NULL, EVP_sha1(), NULL,
+                            client_public_key.get())) {
+    LOG(ERROR) << "EVP_DigestVerifyInit failed.";
+    return AuthResult::Create(
+        "EVP_DigestVerifyInit failed.",
+        AuthResult::ERROR_UNEXPECTED_AUTH_LIBRARY_RESULT);
+  }
+  if (!EVP_DigestVerifyUpdate(ctx.get(), peer_cert.data(), peer_cert.size())) {
+    LOG(ERROR) << "EVP_DigestVerifyUpdate failed.";
+    return AuthResult::Create(
+        "EVP_DigestVerifyUpdate failed.",
+        AuthResult::ERROR_UNEXPECTED_AUTH_LIBRARY_RESULT);
+  }
+  const std::string& signature = response.signature();
+  int result = EVP_DigestVerifyFinal(

[davidben, 2014/10/30 18:37:22]

Actually, as of https://codereview.chromium.org/664663004/ this can just be

  if (!EVP_DigestVerifyFinal(

There was only one case left where it could return -1 and that's gone. (Going
forward, BoringSSL functions are going to try to avoid being tri-state like
that, but it'll take a while to go through all the legacy functions.)

[Kevin M, 2014/10/31 21:39:38]

Done.

[davidben, 2014/11/01 00:19:34]

Oops. It occurred to me that we probably want to leave merging this as an
option. Mind switching that back? Apologies for the churn.

[Kevin M, 2014/11/03 18:31:46]

Done.

+      ctx.get(),
+      reinterpret_cast<uint8_t*>(const_cast<char*>(signature.data())),
+      signature.size());
+  if (result <= 0) {
+    return AuthResult::Create(
+        "payload verification failed.",
+        AuthResult::ERROR_NSS_SIGNED_BLOBS_MISMATCH);
+  }
+  return AuthResult();
+}
 
 AuthResult AuthenticateChallengeReply(const CastMessage& challenge_reply,

[mark a. foltz, 2014/10/31 06:49:11]

This appears to be identical to the same method in _nss.cc.  Hoist it out into a
utility class and eliminate the duplicated code?

[Kevin M, 2014/10/31 21:39:37]

Done.

                                       const std::string& peer_cert) {
-  NOTREACHED();
+  if (peer_cert.empty()) {
+    AuthResult result = AuthResult::Create("Peer cert was empty.",
+                                           AuthResult::ERROR_PEER_CERT_EMPTY);
+    VLOG(1) << result.error_message;
+    return result;
+  }
+
+  VLOG(1) << "Challenge reply: " << CastMessageToString(challenge_reply);
+  DeviceAuthMessage auth_message;
+  AuthResult result = ParseAuthMessage(challenge_reply, &auth_message);
+  if (!result.success()) {
+    VLOG(1) << result.error_message;
+    return result;
+  }
+
+  const AuthResponse& response = auth_message.response();
+  result = VerifyCredentials(response, peer_cert);
+  if (!result.success()) {
+    VLOG(1) << result.error_message
+            << ", error type: " << result.error_type;
+    return result;
+  }
+
   return AuthResult();
 }