Implement VerifySignedData() for ECDSA, RSA PKCS#1 and RSA PSS.

net/cert/internal/verify_signed_data.cc

Index: net/cert/internal/verify_signed_data.cc
diff --git a/net/cert/internal/verify_signed_data.cc b/net/cert/internal/verify_signed_data.cc
new file mode 100644
index 0000000000000000000000000000000000000000..76addd7e72b14744cea19e2cd0dc3f3d33e2d294
--- /dev/null
+++ b/net/cert/internal/verify_signed_data.cc
@@ -0,0 +1,185 @@
+// Copyright 2015 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 "net/cert/internal/verify_signed_data.h"
+
+#include "net/cert/internal/signature_algorithm.h"
+
+// TODO(eroman): There is no intention to implement this for non-OpenSSL. Remove
+// this branch once the migration is complete. This could have been done as a
+// conditional file (_openssl.cc) in the build file instead, but that is likely
+// not worth the effort at this point.
+
+#if !defined(USE_OPENSSL)
+
+namespace net {
+
+bool VerifySignedData(const SignatureAlgorithm& signature_algorithm,
+                      const der::Input& signed_data,
+                      const der::Input& signature_value,
+                      const der::Input& public_key) {
+  // Not implemented
+  return false;
+}
+
+}  // namespace net
+
+#else
+
+#include <openssl/evp.h>
+#include <openssl/pkcs12.h>

[davidben, 2015/07/07 18:03:56]

What's this include for?

[eroman, 2015/07/07 18:34:43]

I was apparently relying on it for its transitive include of x509.h (needed for
d2i_PUBKEY()).

Changed to included x509.h directly instead.

+#include <openssl/rand.h>

[eroman, 2015/07/07 18:34:43]

This similarly was not needed, and removed.

+
+#include "crypto/openssl_util.h"
+#include "crypto/scoped_openssl_types.h"
+#include "net/der/input.h"
+#include "net/der/parser.h"
+
+namespace net {
+
+namespace {
+
+// Creates a EVP_PKEY given the der-encoded bytes of the SPKI. This function
+// does not do any sort of validation of the resultant key. The consumer is
+// responsible for verifying the key's type and validity of its parameters.
+WARN_UNUSED_RESULT bool ImportUnverifiedPkeyFromSpki(
+    const der::Input& spki,
+    crypto::ScopedEVP_PKEY* pkey) {
+  crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
+
+  const uint8_t* ptr = spki.UnsafeData();
+  pkey->reset(d2i_PUBKEY(nullptr, &ptr, spki.Length()));

[davidben, 2015/07/07 18:03:56]

This needs x509.h. I'd rather we not use x509.h, but it's true that pub_encode /
pub_decode hooks on the EVP_PKEY aren't currently exposed anywhere. Not sure yet
what I want to do with those.

We could use this for now, and I could get you folks a fresh API that doesn't
use the legacy ASN.1 stack. But the question there is, like with
AlgorithmIdentifier in signatures, do we want that within BoringSSL or do we
want that in net/cert? net/cert is in a better position to enforce things like:

- RFC 4055 defines an id-RSASSA-PSS for the SPKI to make the key only allow
RSA-PSS? I'm not sure BoringSSL even accepts it at all right now.
https://tools.ietf.org/html/rfc4055#section-3.3

- RFC 3279 says RSA keys are identified by rsaEncryption. d2i_PUBKEY will accept
either rsaEncryption or 2.5.8.1.1 apparently. I dunno whether compatibility
requires we accept both, but it's certainly easier to change a new thing than an
old; the way it's plumbed into BoringSSL is pretty invasive.

- SPKIs for EC keys may use id-ecPublicKey or id-ecDH. The latter is useless for
us, but we shouldn't allow those for ECDSA. BoringSSL currently doesn't parse
id-ecDH at all (and I doubt it ever will), but that's not necessarily in the
contract of a "SPKI -> EVP_PKEY" function. At least right now, an EVP_PKEY
doesn't include any constraints on it. It's a pretty thin abstraction over EC
and RSA keys. (And DSA, but that only half-works.)

- Speaking of DSA, d2i_PUBKEY will happily decode DSA keys right now. They'll
decode, but operations on them won't work. (It's the bare minimum needed thus
far to appease some internal consumers. Hopefully it'll go away again later, but
it does need to be routed up for now.)

Then again, processing SPKIs may be a generally useful thing. Certainly the SSL
stack needs to extract the key even in a post crypto/x509 world. I don't have
strong opinions as to whether that should work by parsing a tiny X.509 subset in
ssl/ or if the "here is a bunch of certificates I got over the wire; do they
look good?" callback should also have the embedder give back the public key. The
latter seems kinda tempting actually. Nice to not have things being parsed
twice.


I currently lean towards having net/cert parse those and instead call
lower-level functions like d2i_RSAPublicKey (soon to no longer use the legacy
ASN.1 stack[*], so I'll have a nicer API for you later). But I'm also not the
person who will be implementing all that.


[*] Note: right now, this does use the legacy ASN.1 stack which really parses
BER, not DER. It's absurdly lenient; we didn't even notice this:
https://boringssl.googlesource.com/boringssl/+/cb41d7702980776e61f46c535cfdef...

However, fairly soon those will be using crypto/bytestring. Then they will be,
as far as I know, completely strict w.r.t. DER. So that's something we'll need
to sniff at for compat once that's in.

[eroman, 2015/07/07 18:34:43]

Thanks David!

So my take-away is:

  (1) Switch to d2i_RSAPublicKey() / d2i_EC_PUBKEY()
  (2) Longer term we want to either expose different API in boringssl, or move
the parsing into net/cert

The same issue applies to WebCrypto, which to be spec compliant should just
implement the WebCrypto version of parsing and writing.

If we decide against keeping that logic in boringssl, I can implement the needed
parsing here for WebCrypto and net/cert. However my gut feeling is that it
probably should just stay in boringssl which as you say still has a need to
parse SPKI, so seems like it should be the canonical place for that.

[davidben, 2015/07/07 18:53:21]

Oh, good point. I hadn't thought about WebCrypto.

That does beg the question of what a "parse SPKI" API should look like though.
If BoringSSL internally parses SPKI, we'll need to figure out the right story
for enforcing all the things we want to enforce. Either by pushing things into
EVP_PKEY or having {net/cert,WebCrypto} extract things out of the AlgID in
parallel or a bit of both.

Ryan, do you have opinions here?

+  if (!pkey->get() || ptr != spki.UnsafeData() + spki.Length())
+    return false;
+
+  return true;
+}
+
+// Converts a DigestAlgorithm to an equivalent EVP_MD*.
+WARN_UNUSED_RESULT bool GetDigest(DigestAlgorithm digest, const EVP_MD** out) {
+  switch (digest) {
+    case DigestAlgorithm::Sha1:
+      *out = EVP_sha1();
+      break;
+    case DigestAlgorithm::Sha256:
+      *out = EVP_sha256();
+      break;
+    case DigestAlgorithm::Sha384:
+      *out = EVP_sha384();
+      break;
+    case DigestAlgorithm::Sha512:
+      *out = EVP_sha512();
+      break;
+  }
+
+  return *out;

[davidben, 2015/07/07 18:03:57]

I believe MSVC will grump about this and want !!*out or *out != nullptr.

[eroman, 2015/07/07 18:34:43]

Done.

+}
+
+// Sets the RSASSA-PSS parameters on |pctx|. Returns true on success.
+bool ApplyRsaPssOptions(const RsaPssParameters* params, EVP_PKEY_CTX* pctx) {
+  // BoringSSL takes a signed int for the salt length, and interprets
+  // negative values in a special manner. Make sure not to silently underflow.
+  base::CheckedNumeric<int> salt_length_bytes_int(params->salt_length());
+  if (!salt_length_bytes_int.IsValid())
+    return false;
+
+  const EVP_MD* mgf1_hash;
+  if (!GetDigest(params->mgf1_hash(), &mgf1_hash))
+    return false;
+
+  return (1 == EVP_PKEY_CTX_set_rsa_padding(pctx, RSA_PKCS1_PSS_PADDING) &&
+          1 == EVP_PKEY_CTX_set_rsa_mgf1_md(pctx, mgf1_hash) &&
+          1 == EVP_PKEY_CTX_set_rsa_pss_saltlen(
+                   pctx, salt_length_bytes_int.ValueOrDie()));
+}
+
+bool RsaVerify(const SignatureAlgorithm& algorithm,
+               const der::Input& signed_data,
+               const der::Input& signature_value,
+               EVP_PKEY* public_key) {
+  // TODO(eroman): Is it necessary to verify that the public key's parameters
+  // are congruent with the signature algorithm, or will BoringSSL already do
+  // the needed checks?

[Ryan Sleevi, 2015/07/07 14:07:31]

David: Look here

[davidben, 2015/07/07 18:03:56]

I don't believe BoringSSL is even passed the signature algorithm here, so it
couldn't do it if we wanted to. But, no, an EVP_PKEY has no constraints attached
to it. It's just a thin wrapper over RSA and EC_KEY, nothing more.

[eroman, 2015/07/07 18:34:43]

Thanks!

Indeed, seems the checks are in fact quite necessary.
My code as written would happily inflate a non RSA-key from SPKI, and then do
the verification using that key type.

I will fix this and update the tests.

Sadly I mistakenly thought I was already testing for this with:
  rsa-using-ec-key.pem
  ecdsa-using-rsa-key.pem

But the test construction for those is such that I swapped out the key of
otherwise valid parameters, whereas swapping the algorithm would have made for a
better test.

+
+  crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
+  crypto::ScopedEVP_MD_CTX ctx(EVP_MD_CTX_create());
+  EVP_PKEY_CTX* pctx = NULL;  // Owned by |ctx|.
+
+  const EVP_MD* digest;
+  if (!GetDigest(algorithm.digest(), &digest))
+    return false;
+
+  if (!EVP_DigestVerifyInit(ctx.get(), &pctx, digest, NULL, public_key))
+    return false;
+
+  // Set the RSASSA-PSS specific options.
+  if (algorithm.algorithm() == SignatureAlgorithmId::RsaPss &&
+      !ApplyRsaPssOptions(algorithm.ParamsForRsaPss(), pctx)) {
+    return false;
+  }
+
+  if (!EVP_DigestVerifyUpdate(ctx.get(), signed_data.UnsafeData(),
+                              signed_data.Length())) {
+    return false;
+  }
+
+  return 1 == EVP_DigestVerifyFinal(ctx.get(), signature_value.UnsafeData(),
+                                    signature_value.Length());
+}
+
+bool EcdsaVerify(const SignatureAlgorithm& algorithm,
+                 const der::Input& signed_data,
+                 const der::Input& signature_value,
+                 EVP_PKEY* public_key) {
+  // TODO(eroman): Is it necessary to verify that the public key's parameters
+  // are congruent with the signature algorithm, or will BoringSSL already do
+  // the needed checks?

[Ryan Sleevi, 2015/07/07 14:07:31]

David: And here

[davidben, 2015/07/07 18:03:56]

Ditto.

+
+  crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
+  crypto::ScopedEVP_MD_CTX ctx(EVP_MD_CTX_create());
+
+  const EVP_MD* digest;
+  if (!GetDigest(algorithm.digest(), &digest))
+    return false;
+
+  if (!EVP_DigestVerifyInit(ctx.get(), NULL, digest, NULL, public_key) ||
+      !EVP_DigestVerifyUpdate(ctx.get(), signed_data.UnsafeData(),
+                              signed_data.Length())) {
+    return false;
+  }
+
+  return 1 == EVP_DigestVerifyFinal(ctx.get(), signature_value.UnsafeData(),
+                                    signature_value.Length());
+}
+
+}  // namespace
+
+bool VerifySignedData(const SignatureAlgorithm& signature_algorithm,
+                      const der::Input& signed_data,
+                      const der::Input& signature_value,
+                      const der::Input& public_key_spki) {
+  crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
+
+  // The validity checks on the public key are done in the algorithm-specific
+  // verification functions.
+  crypto::ScopedEVP_PKEY public_key;
+  if (!ImportUnverifiedPkeyFromSpki(public_key_spki, &public_key))
+    return false;
+
+  switch (signature_algorithm.algorithm()) {
+    case SignatureAlgorithmId::RsaPkcs1:
+    case SignatureAlgorithmId::RsaPss:
+      return RsaVerify(signature_algorithm, signed_data, signature_value,
+                       public_key.get());
+    case SignatureAlgorithmId::Ecdsa:
+      return EcdsaVerify(signature_algorithm, signed_data, signature_value,
+                         public_key.get());
+  }
+
+  return false;  // Unsupported algorithm.
+}
+
+}  // namespace net
+
+#endif