platform_keys: Add error code for input too long

When 'raw' signing (of pre-hashed data) is performed, report a
designated error code when the input data is too long.
Also add browser tests for 'raw' signing and clarify some docs.

Bug: 1158363
Change-Id: Ica61543985068e57752060e95b1d28f4765bbfa0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2595282
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Michael Ershov <miersh@google.com>
Cr-Commit-Position: refs/heads/master@{#838655}

chrome/browser/chromeos/platform_keys/platform_keys.cc

@@ -55,6 +55,8 @@ std::string StatusToString(Status status) {
       return "Algorithm not supported.";
     case Status::kErrorCertificateNotFound:
       return "Certificate could not be found.";
+    case Status::kErrorInputTooLong:
+      return "Input too long.";
     case Status::kErrorGrantKeyPermissionForExtension:
       return "Tried to grant permission for a key although prohibited (either "
              "key is a corporate key or this account is managed).";

chrome/browser/chromeos/platform_keys/platform_keys.h

@@ -43,6 +43,7 @@ enum class Status {
   kSuccess,
   kErrorAlgorithmNotSupported,
   kErrorCertificateNotFound,
+  kErrorInputTooLong,
   kErrorGrantKeyPermissionForExtension,
   kErrorInternal,
   kErrorKeyAttributeRetrievalFailed,

chrome/browser/chromeos/platform_keys/platform_keys_service.h

@@ -156,11 +156,12 @@ class PlatformKeysService : public KeyedService {
                                   SignCallback callback) = 0;
 
   // Applies PKCS1 padding and afterwards signs the data with the private key
-  // matching |public_key_spki_der|. |data| is not digested. If the key is not
-  // found in that |token_id| (or in none of the available tokens if |token_id|
-  // is not specified), the operation aborts. The size of |data| (number of
-  // octets) must be smaller than k - 11, where k is the key size in octets.
-  // |callback| will be invoked with the signature or an error status.
+  // matching |public_key_spki_der|. |data| is not digested, PKCS1 DigestInfo is
+  // not prepended. If the key is not found in that |token_id| (or in none of
+  // the available tokens if |token_id| is not specified), the operation aborts.
+  // The size of |data| (number of octets) must be smaller than k - 11, where k
+  // is the key size in octets. |callback| will be invoked with the signature or
+  // an error status.
   virtual void SignRSAPKCS1Raw(base::Optional<TokenId> token_id,
                                const std::string& data,
                                const std::string& public_key_spki_der,

chrome/browser/chromeos/platform_keys/platform_keys_service_browsertest.cc

@@ -16,7 +16,10 @@
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "base/run_loop.h"
+#include "base/stl_util.h"
+#include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_piece.h"
 #include "base/strings/stringprintf.h"
 #include "base/task/post_task.h"
 #include "base/threading/thread_restrictions.h"
@@ -47,6 +50,7 @@
 #include "content/public/test/browser_test.h"
 #include "crypto/nss_key_util.h"
 #include "crypto/scoped_nss_types.h"
+#include "crypto/sha2.h"
 #include "crypto/signature_verifier.h"
 #include "net/cert/nss_cert_database.h"
 #include "net/cert/x509_certificate.h"
@@ -95,6 +99,24 @@ struct TestConfigPerToken {
   TokenId token_id;
 };
 
+// Returns |hash| prefixed with DER-encoded PKCS#1 DigestInfo with
+// AlgorithmIdentifier=id-sha256.
+// This is useful for testing PlatformKeysService::SignRSAPKCS1Raw which only
+// appends PKCS#1 v1.5 padding before signing.
+std::string PrependSHA256DigestInfo(base::StringPiece hash) {
+  // DER-encoded PKCS#1 DigestInfo "prefix" with
+  // AlgorithmIdentifier=id-sha256.
+  // The encoding is taken from https://tools.ietf.org/html/rfc3447#page-43
+  const uint8_t kDigestInfoSha256DerData[] = {
+      0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01,
+      0x65, 0x03, 0x04, 0x02, 0x01, 0x05, 0x00, 0x04, 0x20};
+  const base::StringPiece kDigestInfoSha256Der(
+      reinterpret_cast<const char*>(kDigestInfoSha256DerData),
+      base::size(kDigestInfoSha256DerData));
+
+  return base::StrCat({kDigestInfoSha256Der, hash});
+}
+
 }  // namespace
 
 class PlatformKeysServiceBrowserTestBase
@@ -171,17 +193,23 @@ class PlatformKeysServiceBrowserTestBase
 
   // Generates a key pair in the given |token_id| using platform keys service
   // and returns the SubjectPublicKeyInfo string encoded in DER format.
-  std::string GenerateKeyPair(TokenId token_id) {
-    const unsigned int kKeySize = 2048;
-
+  std::string GenerateKeyPair(TokenId token_id, unsigned int key_size) {
     test_util::GenerateKeyExecutionWaiter generate_key_waiter;
-    platform_keys_service()->GenerateRSAKey(token_id, kKeySize,
+    platform_keys_service()->GenerateRSAKey(token_id, key_size,
                                             generate_key_waiter.GetCallback());
     generate_key_waiter.Wait();
 
     return generate_key_waiter.public_key_spki_der();
   }
 
+  // Generates a key pair with a default size in the given |token_id| using
+  // platform keys service and returns the SubjectPublicKeyInfo string encoded
+  // in DER format.
+  std::string GenerateKeyPair(TokenId token_id) {
+    const unsigned int kDefaultKeySize = 2048;
+    return GenerateKeyPair(token_id, kDefaultKeySize);
+  }
+
   // Imports the certificate and key described by the |cert_filename| and
   // |key_filename| files in |source_dir| into the Token |token_id|, then stores
   // the resulting certificate in *|out_cert| and the SPKI of the public key in
@@ -426,6 +454,79 @@ IN_PROC_BROWSER_TEST_P(PlatformKeysServicePerTokenBrowserTest,
   EXPECT_TRUE(signature_verifier.VerifyFinal());
 }
 
+// Generates a Rsa key pair and tests signing using the SignRSAPKCS1Raw
+// function.
+IN_PROC_BROWSER_TEST_P(PlatformKeysServicePerTokenBrowserTest,
+                       GenerateRsaAndSignRaw) {
+  const unsigned int kKeySize = 2048;
+  const TokenId token_id = GetParam().token_id;
+
+  // SignRSAPKCS1Raw only performs PKCS#1.5 padding. To get a correct PKCS#1
+  // signature of |kDataToSign|, it is necessary to pass
+  // (DigestInfo + hash(kDataToSign)) to SignRSAPKCS1Raw, where DigestInfo
+  // describes the hash function.
+  const std::string kDataToSign = "test";
+  const std::string kDataToSignHash = crypto::SHA256HashString(kDataToSign);
+  const std::string kDigestInfoAndDataToSignHash =
+      PrependSHA256DigestInfo(kDataToSignHash);
+
+  const crypto::SignatureVerifier::SignatureAlgorithm kSignatureAlgorithm =
+      crypto::SignatureVerifier::RSA_PKCS1_SHA256;
+
+  const std::string public_key_spki_der = GenerateKeyPair(token_id, kKeySize);
+
+  test_util::SignExecutionWaiter sign_waiter;
+  platform_keys_service()->SignRSAPKCS1Raw(
+      token_id, kDigestInfoAndDataToSignHash, public_key_spki_der,
+      sign_waiter.GetCallback());
+  sign_waiter.Wait();
+  EXPECT_EQ(sign_waiter.status(), Status::kSuccess);
+
+  crypto::SignatureVerifier signature_verifier;
+  ASSERT_TRUE(signature_verifier.VerifyInit(
+      kSignatureAlgorithm,
+      base::as_bytes(base::make_span(sign_waiter.signature())),
+      base::as_bytes(base::make_span(public_key_spki_der))));
+  signature_verifier.VerifyUpdate(base::as_bytes(base::make_span(kDataToSign)));
+  EXPECT_TRUE(signature_verifier.VerifyFinal());
+}
+
+// Generates a Rsa key pair and tests expected limits of the input length of the
+// SignRSAPKCS1Raw function.
+IN_PROC_BROWSER_TEST_P(PlatformKeysServicePerTokenBrowserTest,
+                       SignRawInputTooLong) {
+  const unsigned int kKeySize = 2048;
+  const TokenId token_id = GetParam().token_id;
+
+  const std::string public_key_spki_der = GenerateKeyPair(token_id, kKeySize);
+
+  // SignRSAPKCS1Raw performs PKCS#11 padding which adds at least 11 bytes.
+  {
+    // An input of |kKeySize in bytes - 11| should be fine.
+    std::string data_to_sign;
+    data_to_sign.resize(kKeySize / 8 - 11);
+
+    test_util::SignExecutionWaiter sign_waiter;
+    platform_keys_service()->SignRSAPKCS1Raw(
+        token_id, data_to_sign, public_key_spki_der, sign_waiter.GetCallback());
+    sign_waiter.Wait();
+    EXPECT_EQ(sign_waiter.status(), Status::kSuccess);
+  }
+
+  {
+    // An input of |kKeySize in bytes - 10| should be too long.
+    std::string data_to_sign_too_long;
+    data_to_sign_too_long.resize(kKeySize / 8 - 10);
+
+    test_util::SignExecutionWaiter sign_waiter;
+    platform_keys_service()->SignRSAPKCS1Raw(token_id, data_to_sign_too_long,
+                                             public_key_spki_der,
+                                             sign_waiter.GetCallback());
+    sign_waiter.Wait();
+    EXPECT_EQ(sign_waiter.status(), Status::kErrorInputTooLong);
+  }
+}
+
 IN_PROC_BROWSER_TEST_P(PlatformKeysServicePerTokenBrowserTest,
                        SetAndGetKeyAttribute) {
   // The attribute type to be set and retrieved using platform keys service.

chrome/browser/chromeos/platform_keys/platform_keys_service_nss.cc

@@ -826,6 +826,29 @@ void GenerateECKeyWithDB(std::unique_ptr<GenerateECKeyState> state,
       base::BindOnce(&GenerateECKeyOnWorkerThread, std::move(state)));
 }
 
+// Checks whether |input_length| is lower or equal to the maximum input length
+// for a RSA PKCS#1 v1.5 signature generated using |private_key| with PK11_Sign.
+// Returns false if |input_length| is too large.
+// If the maximum input length can not be determined (which is possible because
+// it queries the PKCS#11 module), returns true and logs a warning.
+bool CheckRSAPKCS1SignRawInputLength(SECKEYPrivateKey* private_key,
+                                     size_t input_length) {
+  // For RSA keys, PK11_Sign will perform PKCS#1 v1.5 padding, which needs at
+  // least 11 bytes. RSA Sign can process an input of max. modulus length.
+  // Thus the maximum input length for the sign operation is
+  // |modulus_length - 11|.
+  int modulus_length_bytes = PK11_GetPrivateModulusLen(private_key);
+  if (modulus_length_bytes <= 0) {
+    LOG(WARNING) << "Could not determine modulus length";
+    return true;
+  }
+  size_t max_input_length_after_padding =
+      static_cast<size_t>(modulus_length_bytes);
+  // PKCS#1 v1.5 Padding needs at least this many bytes.
+  size_t kMinPaddingLength = 11u;
+  return input_length + kMinPaddingLength <= max_input_length_after_padding;
+}
+
 // Performs "raw" PKCS1 v1.5 padding + signing by calling PK11_Sign on
 // |rsa_key|.
 void SignRSAPKCS1RawOnWorkerThread(std::unique_ptr<SignState> state,
@@ -848,6 +871,17 @@ void SignRSAPKCS1RawOnWorkerThread(std::unique_ptr<SignState> state,
   std::vector<unsigned char> signature(signature_len);
   SECItem signature_output = {siBuffer, signature.data(), signature.size()};
   if (PK11_Sign(rsa_key.get(), &signature_output, &input) != SECSuccess) {
+    // Input size is checked after a failure - obtaining max input size
+    // involves extracting key modulus length which is not a free operation, so
+    // don't bother if signing succeeded.
+    // Note: It would be better if this could be determined from some library
+    // return code (e.g. PORT_GetError), but this was not possible with
+    // NSS+chaps at this point.
+    if (!CheckRSAPKCS1SignRawInputLength(rsa_key.get(), state->data_.size())) {
+      LOG(ERROR) << "Couldn't sign - input too long.";
+      state->OnError(FROM_HERE, Status::kErrorInputTooLong);
+      return;
+    }
     LOG(ERROR) << "Couldn't sign.";
     state->OnError(FROM_HERE, Status::kErrorInternal);
     return;