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..470e24c7bc0e6cfd72a7f213879a6b71a583dfc2
--- /dev/null
+++ b/net/cert/internal/verify_signed_data.cc
@@ -0,0 +1,296 @@
+// 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/x509.h>
+
+#include "crypto/openssl_util.h"
+#include "crypto/scoped_openssl_types.h"
+#include "net/der/input.h"
+#include "net/der/parser.h"
+
+namespace net {
+
+namespace {
+
+// 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();

[davidben, 2015/07/20 19:21:59]

#include <openssl/digest.h>

[eroman, 2015/07/20 20:03:40]

Done.

+      break;
+    case DigestAlgorithm::Sha256:
+      *out = EVP_sha256();
+      break;
+    case DigestAlgorithm::Sha384:
+      *out = EVP_sha384();
+      break;
+    case DigestAlgorithm::Sha512:
+      *out = EVP_sha512();
+      break;
+  }
+
+  return *out != nullptr;

[davidben, 2015/07/20 19:21:59]

This assumes that *out was initialized to nullptr (which it isn't in line 71).

[eroman, 2015/07/20 20:03:40]

Done. Good catch.

+}
+
+// Sets the RSASSA-PSS parameters on |pctx|. Returns true on success.
+WARN_UNUSED_RESULT 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()));

[davidben, 2015/07/20 19:21:59]

In BoringSSL, these can only return 0 or 1.

[eroman, 2015/07/20 20:03:40]

Done.

+}
+
+// Parses an RSA public key from SPKI to an EVP_PKEY.
+//
+// Returns true on success.
+//
+// There are two flavors of RSA public key that this function recognizes from

[davidben, 2015/07/20 19:21:59]

Currently, d2i_RSA_PUBKEY also accepts 2.5.8.1.1.

[eroman, 2015/07/20 20:03:40]

Acknowledged. Note that this is covered by my tests:

VerifySignedDataTest.DISABLED_RsaPss_Sha1_Salt20_UsingPssKeyNoParams
VerifySignedDataTest.DISABLED_RsaPss_Sha256_Salt10_UsingPssKeyWithParams

Which for obvious reasons is disabled :)

+// RFC 5912:
+//
+//     pk-rsa PUBLIC-KEY ::= {
+//      IDENTIFIER rsaEncryption
+//      KEY RSAPublicKey
+//      PARAMS TYPE NULL ARE absent
+//      -- Private key format not in this module --
+//      CERT-KEY-USAGE {digitalSignature, nonRepudiation,
+//      keyEncipherment, dataEncipherment, keyCertSign, cRLSign}
+//     }
+//
+//  ...
+//
+//     pk-rsaSSA-PSS PUBLIC-KEY ::= {

[davidben, 2015/07/20 19:21:59]

d2i_RSA_PUBKEY doesn't actually recognize this one. This goes back again to the
question of what we want to do about usage-specific SPKIs.

Probably simplest to, for the first version, just not support them.

[eroman, 2015/07/20 20:03:40]

Acknowledged. Same response as above.

I will add a TODO() to make this more obvious.

+//         IDENTIFIER id-RSASSA-PSS
+//         KEY RSAPublicKey
+//         PARAMS TYPE RSASSA-PSS-params ARE optional
+//          -- Private key format not in this module --
+//         CERT-KEY-USAGE { nonRepudiation, digitalSignature,
+//                              keyCertSign, cRLSign }
+//     }
+//
+// Any RSA signature algorithm can accept a "pk-rsa" (rsaEncryption). However a
+// "pk-rsaSSA-PSS" key is only accepted if the signature algorithm was for PSS
+// mode:
+//
+//     sa-rsaSSA-PSS SIGNATURE-ALGORITHM ::= {
+//         IDENTIFIER id-RSASSA-PSS
+//         PARAMS TYPE RSASSA-PSS-params ARE required
+//         HASHES { mda-sha1 | mda-sha224 | mda-sha256 | mda-sha384
+//                      | mda-sha512 }
+//         PUBLIC-KEYS { pk-rsa | pk-rsaSSA-PSS }
+//         SMIME-CAPS { IDENTIFIED BY id-RSASSA-PSS }
+//     }
+//
+// Moreover, if a "pk-rsaSSA-PSS" key was used and it optionally provided
+// parameters for the algorithm, they must match those of the signature
+// algorithm.

[davidben, 2015/07/20 19:21:59]

This isn't implemented.

[eroman, 2015/07/20 20:03:40]

Acknowledged.

+//
+// COMPATIBILITY NOTE: RFC 5912 specifies that the algorithm parameters for
+// pk-rsa (rsaEncryption) are absent. However in practice though it is common
+// for these to be present and NULL.

[davidben, 2015/07/20 19:21:59]

This one says it MUST be NULL.
https://tools.ietf.org/html/rfc3279#section-2.3.1

I'm not familiar with the ASN.1 syntax of RFC 5912, but if "ARE absent" means
what I assume it means, this seems a bug in RFC 5912. (5912 seems to only exist
because "Some developers would like the IETF to use the latest version of ASN.1
in its standards." and... honestly I like the 1988 ASN.1 better. This new one is
hard to read.)


I should not that, we can fix this but, currently, d2i_RSA_PUBKEY will just
ignore the parameters. So if there's garbage in there, it won't notice.


I can add a crypto/bytestring-based SPKI parser within BoringSSL that avoids a
lot of these stupid bits. But I think we need a ruling on how SPKI
responsibilities are supposed to be divided between net/cert/,
boringssl/src/crypto/, and boringssl/src/ssl/.

[eroman, 2015/07/20 20:03:40]

Correct, "ARE absent" in this case means MUST be absent. RFC 5912 uses
"preferredAbsent" for things that SHOULD be absent:

ParamOptions ::= ENUMERATED {
   required,         -- Parameters MUST be encoded in structure
   preferredPresent, -- Parameters SHOULD be encoded in structure
   preferredAbsent,  -- Parameters SHOULD NOT be encoded in structure
   absent,           -- Parameters MUST NOT be encoded in structure
   inheritable,      -- Parameters are inherited if not present
   optional,         -- Parameters MAY be encoded in the structure
   ...
}

It is frustrating that these RFCs are not in agreement on certain things.

How should I proceed with these checks, which RFCs to give precedence to?

Obviously in this case RFC 5912's definition is not helpful, since I didn't
actually come across any where it wasn't NULL.

I have added a reference to 3279 in the compatibility note, but perhaps it would
be better for me to re-write the documentation without using 5912 as the
authoritative source.

[davidben, 2015/07/20 21:17:22]

If RFC 3279 is treated authoritative, it's not even allowed to be omitted. And
mozilla::pkix will reject the SPKI if not NULL. I think RFC 5912 is just wrong
here.
Dunno. A priori, I would think RFC 3279 would be more usefully authoritative,
and any discrepancies in RFC 5912 are just bugs, but at this point you're
probably more familiar with the X.509 landscape than me.

+WARN_UNUSED_RESULT bool ParseRsaKeyFromSpki(const der::Input& public_key_spki,
+                                            crypto::ScopedEVP_PKEY* pkey) {
+  crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
+
+  const uint8_t* ptr = public_key_spki.UnsafeData();
+  crypto::ScopedRSA rsa(

[davidben, 2015/07/20 19:21:59]

#include <openssl/rsa.h>

[eroman, 2015/07/20 20:03:40]

Done.

+      d2i_RSA_PUBKEY(nullptr, &ptr, public_key_spki.Length()));
+
+  if (!rsa || ptr != public_key_spki.UnsafeData() + public_key_spki.Length())
+    return false;
+
+  // Create a corresponding EVP_PKEY.
+  pkey->reset(EVP_PKEY_new());
+  if (!pkey || !EVP_PKEY_set1_RSA(pkey->get(), rsa.get()))

[davidben, 2015/07/20 19:21:59]

This is slightly odd since d2i_RSA_PUBKEY will actually get you an EVP_PKEY and
then dismantle it. And then this is creating a wrapper anew.

(This is part of the usual "anything based on the legacy crypto/x509 stack is
extremely shoddy in quality and probably should be ditched".)

[eroman, 2015/07/20 20:03:40]

Do you want me to go back to using d2i_PUBKEY() as in the earlier patchset? (and
test the key type for ECDSA vs RSA of course).

[davidben, 2015/07/20 21:17:22]

*shrug* That's all d2i_RSA_PUBKEY is doing.

But if the plan is to parse SPKIs here and not lean on BoringSSL at all, I
suppose it doesn't really matter since it'll get replaced with der::Input
anyway. (Any reason not to just do it now? You can even use
RSA_public_key_from_bytes which will save you the &ptr thing, and the check that
you consumed everything.)

https://boringssl.googlesource.com/boringssl/+/master/include/openssl/rsa.h#333

On the EC side, it's slightly more effort because the parameters include things,
but you o2i_ECPublicKey (hrm, the documentation is slightly off; it's not
parsing a DER structure) and pass in an EC_KEY that has the group already set.
I'll add a EC_KEY_public_from_bytes(EC_GROUP*, const uint8_t*, size_t) later.

https://boringssl.googlesource.com/boringssl/+/master/include/openssl/ec_key....

[eroman, 2015/07/20 22:28:08]

Done, changed to use d2i_PUBKEY().
My preference is to explore any custom parsing of the spki as a separate CL.

The straightforward call to d2i_PUBKEY() gets us pretty far, and in fact likely
supports most if not all of the cases we care about for the initial milestone
(the only unsupported functionality is specifying a *key* algorithm as
RSASSA-PSS).

Custom parsing for RSASSA-PSS spki and validating against the signature
algorithm is enough work that I consider it a lower-priority follow-up. Mozilla
pkix for instance does not even support RSA-PSS, let alone RSA-PSS as the public
key algorithm.

Too many more patchsets on this CL risks review fatigue, but of course I defer
to my reviewers :)

[davidben, 2015/07/21 15:29:05]

Fair enough. Though RSASSA-PSS SPKI isn't the only thing that we get out of
doing the SPKI parsing ourselves. We also get strictness (the weird extra OID is
supported, BER is supported, etc). I was actually thinking just implement
id-rsaEncryption manually which shouldn't be much work. But, yeah, let's leave
it as a follow-up.

+    return false;
+
+  return true;
+}
+
+// Does signature verification using either RSA or ECDSA.
+WARN_UNUSED_RESULT bool DoVerify(const SignatureAlgorithm& algorithm,
+                                 const der::Input& signed_data,
+                                 const der::Input& signature_value,
+                                 EVP_PKEY* public_key) {
+  DCHECK(algorithm.algorithm() == SignatureAlgorithmId::RsaPkcs1 ||
+         algorithm.algorithm() == SignatureAlgorithmId::RsaPss ||
+         algorithm.algorithm() == SignatureAlgorithmId::Ecdsa);
+
+  crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
+
+  crypto::ScopedEVP_MD_CTX ctx(EVP_MD_CTX_create());
+  EVP_PKEY_CTX* pctx = nullptr;  // Owned by |ctx|.
+
+  const EVP_MD* digest;
+  if (!GetDigest(algorithm.digest(), &digest))
+    return false;
+
+  if (!EVP_DigestVerifyInit(ctx.get(), &pctx, digest, nullptr, 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());
+}
+
+// Returns true if the given curve is allowed for ECDSA. The input is a
+// BoringSSL NID.
+//
+// TODO(eroman): Extract policy decisions such as allowed curves, hashes, RSA
+// modulus size, to somewhere more central.
+WARN_UNUSED_RESULT bool IsAllowedCurveName(int curve_nid) {
+  switch (curve_nid) {
+    case NID_X9_62_prime256v1:
+    case NID_secp384r1:
+    case NID_secp521r1:
+      return true;
+  }
+  return false;
+}
+
+// Parses an EC public key from SPKI to an EVP_PKEY.
+//
+// Returns true on success.
+//
+// RFC 5912 describes all the ECDSA signature algorithms as requiring a public
+// key of type "pk-ec":
+//
+//     pk-ec PUBLIC-KEY ::= {
+//      IDENTIFIER id-ecPublicKey
+//      KEY ECPoint
+//      PARAMS TYPE ECParameters ARE required
+//      -- Private key format not in this module --
+//      CERT-KEY-USAGE { digitalSignature, nonRepudiation, keyAgreement,
+//                           keyCertSign, cRLSign }
+//     }
+//
+// Moreover RFC 5912 stipulates what curves are allowed. The ECParameters
+// MUST NOT use an implicitCurve or specificCurve for PKIX:
+//
+//     ECParameters ::= CHOICE {
+//      namedCurve      CURVE.&id({NamedCurve})
+//      -- implicitCurve   NULL
+//        -- implicitCurve MUST NOT be used in PKIX
+//      -- specifiedCurve  SpecifiedCurve
+//        -- specifiedCurve MUST NOT be used in PKIX
+//        -- Details for specifiedCurve can be found in [X9.62]
+//        -- Any future additions to this CHOICE should be coordinated
+//        -- with ANSI X.9.
+//     }
+//     -- If you need to be able to decode ANSI X.9 parameter structures,
+//     -- uncomment the implicitCurve and specifiedCurve above, and also
+//     -- uncomment the following:
+//     --(WITH COMPONENTS {namedCurve PRESENT})
+//
+// The namedCurves are extensible. The ones described by RFC 5912 are:
+//
+//     NamedCurve CURVE ::= {
+//     { ID secp192r1 } | { ID sect163k1 } | { ID sect163r2 } |
+//     { ID secp224r1 } | { ID sect233k1 } | { ID sect233r1 } |
+//     { ID secp256r1 } | { ID sect283k1 } | { ID sect283r1 } |
+//     { ID secp384r1 } | { ID sect409k1 } | { ID sect409r1 } |
+//     { ID secp521r1 } | { ID sect571k1 } | { ID sect571r1 },
+//     ... -- Extensible
+//     }
+WARN_UNUSED_RESULT bool ParseEcKeyFromSpki(const der::Input& public_key_spki,
+                                           crypto::ScopedEVP_PKEY* pkey) {
+  crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
+
+  const uint8_t* ptr = public_key_spki.UnsafeData();
+  crypto::ScopedEC_KEY ec(
+      d2i_EC_PUBKEY(nullptr, &ptr, public_key_spki.Length()));
+
+  if (!ec || ptr != public_key_spki.UnsafeData() + public_key_spki.Length())
+    return false;
+
+  // Enforce policy on allowed curves in case d2i_EC_PUBKEY() were to recognize
+  // and allow use of a weak curve.
+  if (!IsAllowedCurveName(EC_GROUP_get_curve_name(EC_KEY_get0_group(ec.get()))))

[davidben, 2015/07/20 19:21:59]

#include <openssl/ec.h>
#include <openssl/ec_key.h>

[eroman, 2015/07/20 20:03:40]

Done.

+    return false;
+
+  // Create a corresponding EVP_PKEY.
+  pkey->reset(EVP_PKEY_new());
+  if (!pkey || !EVP_PKEY_set1_EC_KEY(pkey->get(), ec.get()))

[davidben, 2015/07/20 19:21:59]

(Ditto about this dismantling an EVP_PKEY, only to reassemble it.)

[eroman, 2015/07/20 22:28:08]

Done.

+    return false;
+
+  return true;
+}
+
+}  // namespace
+
+bool VerifySignedData(const SignatureAlgorithm& signature_algorithm,
+                      const der::Input& signed_data,
+                      const der::Input& signature_value,
+                      const der::Input& public_key_spki) {
+  crypto::ScopedEVP_PKEY public_key;
+
+  // Parse the SPKI to an EVP_PKEY appropriate for the signature algorithm.
+  switch (signature_algorithm.algorithm()) {
+    case SignatureAlgorithmId::RsaPkcs1:
+    case SignatureAlgorithmId::RsaPss:
+      // TODO(eroman): This does not enforce that the SPKI's algorithm matches
+      // the signature algorithm. For instance it would not forbid a key with
+      // PSS parameters being used with PKCS1 padding.
+      if (!ParseRsaKeyFromSpki(public_key_spki, &public_key))
+        return false;
+      break;
+    case SignatureAlgorithmId::Ecdsa:
+      if (!ParseEcKeyFromSpki(public_key_spki, &public_key))
+        return false;
+      break;
+  }
+
+  return DoVerify(signature_algorithm, signed_data, signature_value,
+                  public_key.get());
+}
+
+}  // namespace net
+
+#endif