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

net/cert/internal/verify_signed_data.cc

+// 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

[davidben, 2015/07/21 16:26:27]

Nit: NOTIMPLEMENTED()

[eroman, 2015/07/21 19:24:28]

Done.

+  return false;
+}
+
+}  // namespace net
+
+#else
+
+#include <openssl/digest.h>
+#include <openssl/ec.h>
+#include <openssl/ec_key.h>
+#include <openssl/evp.h>
+#include <openssl/rsa.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) {

[davidben, 2015/07/21 16:26:27]

Optional: Could make this return const EVP_MD* and just nullptr on failure. That
would also resolve the "modifies output parameters on failure" thing.

[davidben, 2015/07/21 16:26:27]

#include "base/compiler_specific.h"

[eroman, 2015/07/21 19:24:28]

My thinking is to consistently return "bool" since I expect to change the
signature to an error result as a follow-up (once I have thought through more
carefully what the errors should be and their type).

I also had a slight paranoia in this specific instance that some BoringSSL
functions might treat a null EVP_MD as having a special meaning, and didn't want
the error case to inadvertently select some default behavior.

[eroman, 2015/07/21 19:24:28]

Done.

+  *out = nullptr;
+
+  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 != nullptr;
+}
+
+// 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 (EVP_PKEY_CTX_set_rsa_padding(pctx, RSA_PKCS1_PSS_PADDING) &&

[davidben, 2015/07/21 16:26:27]

Nit: Unnecessary parentheses. (Chromium's not very consistent, but without seems
more common.)

https://code.google.com/p/chromium/codesearch#search/&q=return%5C%20%5C(.*&am...
https://code.google.com/p/chromium/codesearch#search/&q=return%5C%20%5B%5E(%5...

[eroman, 2015/07/21 19:24:28]

Done.

+          EVP_PKEY_CTX_set_rsa_mgf1_md(pctx, mgf1_hash) &&
+          EVP_PKEY_CTX_set_rsa_pss_saltlen(pctx,
+                                           salt_length_bytes_int.ValueOrDie()));
+}
+
+WARN_UNUSED_RESULT bool ImportPkeyFromSpki(const der::Input& spki,
+                                           int expected_pkey_id,
+                                           crypto::ScopedEVP_PKEY* pkey) {

[davidben, 2015/07/21 16:26:27]

Optional: Could return crypto::ScopedEVP_PKEY rather than use an out-param. That
would also resolve the "modifies output parameters on failure" thing.

[eroman, 2015/07/21 19:24:28]

My preference is for a bool, as it will likely be changed to a result code soon.

+  crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
+
+  const uint8_t* ptr = spki.UnsafeData();
+  pkey->reset(d2i_PUBKEY(nullptr, &ptr, spki.Length()));

[davidben, 2015/07/21 16:26:27]

This probably needs a TODO to fix the strictness (not in this CL, yeah),
probably by switching to a dedicated parser. It's not just for id-RSASSA-PSS,
but also to actually enforce all the things we're supposed to enforce. BER, not
using the bogus alternative RSA OID, and actually checking id-rsaEncryption
params. (Or would you rather I go fix that in BoringSSL? We'll want to do that
anyway, and if that happens, I suppose we don't need to switch parsers until
id-RSASSA-PSS is added.)

[eroman, 2015/07/21 19:24:28]

Done.

+  if (!pkey->get() || ptr != spki.UnsafeData() + spki.Length() ||
+      EVP_PKEY_id(pkey->get()) != expected_pkey_id) {
+    pkey->reset();
+    return false;
+  }
+
+  return true;
+}
+
+// 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 should recognize
+// from RFC 5912 (however note that pk-rsaSSA-PSS is not supported in the
+// current implementation).
+// TODO(eroman): Support id-RSASSA-PSS and its associated parameters.
+//
+//     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 ::= {
+//         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.
+//
+// COMPATIBILITY NOTE: RFC 5912 specifies that the algorithm parameters for
+// pk-rsa (rsaEncryption) are absent. However this is in contradiction with
+// RFC 3279 section 2.3.1 which says it should be NULL:

[davidben, 2015/07/21 16:26:27]

Nit: should -> MUST

(I.e. it's not that RFC 3279 allows both and RFC 5912 insists on the other.
They're flat-out contradictory.)

[eroman, 2015/07/21 19:24:28]

Done.

+//
+//     The rsaEncryption OID is intended to be used in the algorithm field
+//     of a value of type AlgorithmIdentifier.  The parameters field MUST
+//     have ASN.1 type NULL for this algorithm identifier.
+//
+// In practice implementations follow the rules from RFC 3279 and not RFC 5912.
+WARN_UNUSED_RESULT bool ParseRsaKeyFromSpki(const der::Input& public_key_spki,
+                                            crypto::ScopedEVP_PKEY* pkey) {

[davidben, 2015/07/21 16:26:27]

Optional: Could return crypto::ScopedEVP_PKEY rather than use an out-param. That
would also resolve the "modifies output parameters on failure" thing.

[eroman, 2015/07/21 19:24:28]

Same response as earlier.

+  return ImportPkeyFromSpki(public_key_spki, EVP_PKEY_RSA, pkey);
+}
+
+// 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 ||

[davidben, 2015/07/21 16:26:27]

#include "base/logging.h"

[eroman, 2015/07/21 19:24:28]

Done.

+         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 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) {

[davidben, 2015/07/21 16:26:27]

Optional: Could return crypto::ScopedEVP_PKEY rather than use an out-param.

[eroman, 2015/07/21 19:24:28]

Same response as earlier.

+  if (!ImportPkeyFromSpki(public_key_spki, EVP_PKEY_EC, pkey))
+    return false;
+
+  // Enforce policy on allowed curves in case ImportPkeyFromSpki() were to
+  // recognize and allow use of a weak curve.
+  crypto::ScopedEC_KEY ec(EVP_PKEY_get1_EC_KEY(pkey->get()));
+  if (!ec.get())
+    return false;  // Unexpected.
+
+  int curve_nid = EC_GROUP_get_curve_name(EC_KEY_get0_group(ec.get()));
+  return IsAllowedCurveName(curve_nid);
+}
+
+}  // 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:
+      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