Tests for CryptohomeKeyDelegate: algs, ext errors

Add more browser tests for the CryptohomeKeyDelegate implementation,
for these scenarios:

* a key that supports two algorithms simultaneously;
* another signature algorithm (PKCS #1 v1.5 SHA-1) besides PKCS #1 v1.5
  SHA-256;
* an unsupported signature algorithm;
* the extension removing the key that it reported before;
* the extension failing the signature operation.

Bug: 826417
Change-Id: Ie298c506a26debfcead64fb8209a2174fedc52b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1706440
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682298}

chrome/browser/chromeos/certificate_provider/test_certificate_provider_extension.cc

@@ -35,6 +35,12 @@
 
 namespace {
 
+// List of algorithms that the extension claims to support for the returned
+// certificates.
+constexpr extensions::api::certificate_provider::Hash kSupportedHashes[] = {
+    extensions::api::certificate_provider::Hash::HASH_SHA256,
+    extensions::api::certificate_provider::Hash::HASH_SHA1};
+
 base::Value ConvertBytesToValue(base::span<const uint8_t> bytes) {
   base::Value value(base::Value::Type::LIST);
   for (auto byte : bytes)
@@ -49,17 +55,17 @@ std::vector<uint8_t> ExtractBytesFromValue(const base::Value& value) {
   return bytes;
 }
 
-base::Value MakeCertInfoValue(
-    const net::X509Certificate& certificate,
-    const std::vector<extensions::api::certificate_provider::Hash>&
-        supported_hashes) {
+base::span<const uint8_t> GetCertDer(const net::X509Certificate& certificate) {
+  return base::as_bytes(base::make_span(
+      net::x509_util::CryptoBufferAsStringPiece(certificate.cert_buffer())));
+}
+
+base::Value MakeCertInfoValue(const net::X509Certificate& certificate) {
   base::Value cert_info_value(base::Value::Type::DICTIONARY);
   cert_info_value.SetKey("certificate",
-                         ConvertBytesToValue(base::as_bytes(base::make_span(
-                             net::x509_util::CryptoBufferAsStringPiece(
-                                 certificate.cert_buffer())))));
+                         ConvertBytesToValue(GetCertDer(certificate)));
   base::Value supported_hashes_value(base::Value::Type::LIST);
-  for (auto supported_hash : supported_hashes) {
+  for (auto supported_hash : kSupportedHashes) {
     supported_hashes_value.GetList().emplace_back(base::Value(
         extensions::api::certificate_provider::ToString(supported_hash)));
   }
@@ -157,23 +163,34 @@ void TestCertificateProviderExtension::Observe(
 
 base::Value TestCertificateProviderExtension::HandleCertificatesRequest() {
   base::Value cert_info_values(base::Value::Type::LIST);
-  cert_info_values.GetList().emplace_back(MakeCertInfoValue(
-      *certificate_,
-      {extensions::api::certificate_provider::Hash::HASH_SHA256}));
+  if (!should_fail_certificate_requests_)
+    cert_info_values.GetList().emplace_back(MakeCertInfoValue(*certificate_));
   return cert_info_values;
 }
 
 base::Value TestCertificateProviderExtension::HandleSignDigestRequest(
     const base::Value& sign_request) {
-  // TODO(crbug.com/826417): Verify the "certificate" field value.
+  CHECK_EQ(*sign_request.FindKey("certificate"),
+           ConvertBytesToValue(GetCertDer(*certificate_)));
 
   const std::vector<uint8_t> digest =
       ExtractBytesFromValue(*sign_request.FindKey("digest"));
-  CHECK_EQ(extensions::api::certificate_provider::Hash::HASH_SHA256,
-           extensions::api::certificate_provider::ParseHash(
-               sign_request.FindKey("hash")->GetString()));
 
+  const extensions::api::certificate_provider::Hash hash =
+      extensions::api::certificate_provider::ParseHash(
+          sign_request.FindKey("hash")->GetString());
+  int openssl_digest_type = 0;
+  if (hash == extensions::api::certificate_provider::Hash::HASH_SHA256)
+    openssl_digest_type = NID_sha256;
+  else if (hash == extensions::api::certificate_provider::Hash::HASH_SHA1)
+    openssl_digest_type = NID_sha1;
+  else
+    LOG(FATAL) << "Unexpected signature request hash: " << hash;
+
+  if (should_fail_sign_digest_requests_)
+    return base::Value();
   std::vector<uint8_t> signature;
-  RsaSignPrehashed(*private_key_, NID_sha256, digest, &signature);
+  CHECK(
+      RsaSignPrehashed(*private_key_, openssl_digest_type, digest, &signature));
   return ConvertBytesToValue(signature);
 }

chrome/browser/chromeos/certificate_provider/test_certificate_provider_extension.h

@@ -34,7 +34,8 @@ class X509Certificate;
 // It subscribes itself for requests from the JavaScript side of the extension,
 // and implements the cryptographic operations using the "client_1" test
 // certificate and private key (see src/net/data/ssl/certificates). The
-// signature algorithm is currently hardcoded to SHA-256.
+// supported signature algorithms are currently hardcoded to PKCS #1 v1.5 with
+// SHA-1 and SHA-256.
 class TestCertificateProviderExtension final
     : public content::NotificationObserver {
  public:
@@ -46,6 +47,20 @@ class TestCertificateProviderExtension final
     return certificate_;
   }
 
+  // Sets whether the extension should respond with a failure to the
+  // onCertificatesRequested requests.
+  void set_should_fail_certificate_requests(
+      bool should_fail_certificate_requests) {
+    should_fail_certificate_requests_ = should_fail_certificate_requests;
+  }
+
+  // Sets whether the extension should respond with a failure to the
+  // onSignDigestRequested requests.
+  void set_should_fail_sign_digest_requests(
+      bool should_fail_sign_digest_requests) {
+    should_fail_sign_digest_requests_ = should_fail_sign_digest_requests;
+  }
+
  private:
   // content::NotificationObserver implementation:
   void Observe(int type,
@@ -57,8 +72,10 @@ class TestCertificateProviderExtension final
 
   content::BrowserContext* const browser_context_;
   const std::string extension_id_;
-  scoped_refptr<net::X509Certificate> certificate_;
-  bssl::UniquePtr<EVP_PKEY> private_key_;
+  const scoped_refptr<net::X509Certificate> certificate_;
+  const bssl::UniquePtr<EVP_PKEY> private_key_;
+  bool should_fail_certificate_requests_ = false;
+  bool should_fail_sign_digest_requests_ = false;
   content::NotificationRegistrar notification_registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(TestCertificateProviderExtension);

chrome/browser/chromeos/dbus/cryptohome_key_delegate_service_provider_browsertest.cc

@@ -143,6 +143,14 @@ class CryptohomeKeyDelegateServiceProviderTest
         &service_provider_);
 
     PrepareExtension();
+
+    // Populate the browser's state with the mapping between the test
+    // certificate provider extension and the certs that it provides, so that
+    // the tested implementation knows where it should send challenges to. In
+    // the real-world usage, this step is done by the Login/Lock Screens while
+    // preparing the parameters that are later used by the cryptohomed daemon
+    // for calling our D-Bus service.
+    RefreshCertsFromCertProviders();
   }
 
   void TearDownOnMainThread() override {
@@ -169,8 +177,11 @@ class CryptohomeKeyDelegateServiceProviderTest
 
   // Calls the tested ChallengeKey D-Bus method, requesting a signature
   // challenge.
-  // Fills |signature| with the data returned by the call.
-  void CallSignatureChallengeKey(std::vector<uint8_t>* signature) {
+  // Returns whether the D-Bus method succeeded, and on success fills
+  // |signature| with the data returned by the call.
+  bool CallSignatureChallengeKey(
+      cryptohome::ChallengeSignatureAlgorithm signature_algorithm,
+      std::vector<uint8_t>* signature) {
     const cryptohome::AccountIdentifier account_identifier =
         cryptohome::CreateAccountIdentifierFromAccountId(EmptyAccountId());
     cryptohome::KeyChallengeRequest request;
@@ -180,7 +191,7 @@ class CryptohomeKeyDelegateServiceProviderTest
     request.mutable_signature_request_data()->set_public_key_spki_der(
         GetCertSpki(*cert_provider_extension_->certificate()));
     request.mutable_signature_request_data()->set_signature_algorithm(
-        cryptohome::CHALLENGE_RSASSA_PKCS1_V1_5_SHA256);
+        signature_algorithm);
 
     dbus::MethodCall method_call(
         cryptohome::kCryptohomeKeyDelegateInterface,
@@ -192,6 +203,8 @@ class CryptohomeKeyDelegateServiceProviderTest
     std::unique_ptr<dbus::Response> dbus_response =
         dbus_service_test_helper_->CallMethod(&method_call);
 
+    if (dbus_response->GetMessageType() == dbus::Message::MESSAGE_ERROR)
+      return false;
     dbus::MessageReader reader(dbus_response.get());
     cryptohome::KeyChallengeResponse response;
     EXPECT_TRUE(reader.PopArrayOfBytesAsProto(&response));
@@ -199,16 +212,17 @@ class CryptohomeKeyDelegateServiceProviderTest
     EXPECT_TRUE(response.signature_response_data().has_signature());
     signature->assign(response.signature_response_data().signature().begin(),
                       response.signature_response_data().signature().end());
+    return true;
   }
 
   // Returns whether the given |signature| is a valid signature of the original
   // data.
-  bool IsSignatureValid(const std::vector<uint8_t>& signature) const {
+  bool IsSignatureValid(crypto::SignatureVerifier::SignatureAlgorithm algorithm,
+                        const std::vector<uint8_t>& signature) const {
     const std::string spki =
         GetCertSpki(*cert_provider_extension_->certificate());
     crypto::SignatureVerifier verifier;
-    if (!verifier.VerifyInit(crypto::SignatureVerifier::RSA_PKCS1_SHA256,
-                             signature,
+    if (!verifier.VerifyInit(algorithm, signature,
                              base::as_bytes(base::make_span(spki)))) {
       return false;
     }
@@ -216,6 +230,10 @@ class CryptohomeKeyDelegateServiceProviderTest
     return verifier.VerifyFinal();
   }
 
+  TestCertificateProviderExtension* cert_provider_extension() {
+    return cert_provider_extension_.get();
+  }
+
  private:
   // Test certificate provider extension:
   const std::string kExtensionId = "ecmhnokcdiianioonpgakiooenfnonid";
@@ -244,19 +262,56 @@ class CryptohomeKeyDelegateServiceProviderTest
   std::unique_ptr<TestCertificateProviderExtension> cert_provider_extension_;
 };
 
-// Verifies that the ChallengeKey method handles the signature challenge request
-// by passing the request to the certificate provider extension.
+// Verifies that the ChallengeKey request with the PKCS #1 v1.5 SHA-256
+// algorithm is handled successfully using the test provider.
 IN_PROC_BROWSER_TEST_F(CryptohomeKeyDelegateServiceProviderTest,
-                       SignatureBasic) {
-  // Populate the browser's state with the mapping between the test certificate
-  // provider extension and the certs that it provides, so that the tested
-  // implementation knows where it should send challenges to. In the real-world
-  // usage, this step is done by the Login/Lock Screens while preparing the
-  // parameters that are later used by the cryptohomed daemon for calling our
-  // D-Bus service.
+                       SignatureSuccessSha256) {
+  std::vector<uint8_t> signature;
+  EXPECT_TRUE(CallSignatureChallengeKey(
+      cryptohome::CHALLENGE_RSASSA_PKCS1_V1_5_SHA256, &signature));
+  EXPECT_TRUE(
+      IsSignatureValid(crypto::SignatureVerifier::RSA_PKCS1_SHA256, signature));
+}
+
+// Verifies that the ChallengeKey request with the PKCS #1 v1.5 SHA-1 algorithm
+// is handled successfully using the test provider.
+IN_PROC_BROWSER_TEST_F(CryptohomeKeyDelegateServiceProviderTest,
+                       SignatureSuccessSha1) {
+  std::vector<uint8_t> signature;
+  EXPECT_TRUE(CallSignatureChallengeKey(
+      cryptohome::CHALLENGE_RSASSA_PKCS1_V1_5_SHA1, &signature));
+  EXPECT_TRUE(
+      IsSignatureValid(crypto::SignatureVerifier::RSA_PKCS1_SHA1, signature));
+}
+
+// Verifies that the ChallengeKey request fails when the requested algorithm
+// isn't supported by the test provider.
+IN_PROC_BROWSER_TEST_F(CryptohomeKeyDelegateServiceProviderTest,
+                       SignatureErrorUnsupportedAlgorithm) {
+  std::vector<uint8_t> signature;
+  EXPECT_FALSE(CallSignatureChallengeKey(
+      cryptohome::CHALLENGE_RSASSA_PKCS1_V1_5_SHA384, &signature));
+}
+
+// Verifies that the ChallengeKey request fails when the used key isn't reported
+// by the test provider anymore.
+IN_PROC_BROWSER_TEST_F(CryptohomeKeyDelegateServiceProviderTest,
+                       SignatureErrorKeyRemoved) {
+  cert_provider_extension()->set_should_fail_certificate_requests(true);
   RefreshCertsFromCertProviders();
 
   std::vector<uint8_t> signature;
-  CallSignatureChallengeKey(&signature);
-  EXPECT_TRUE(IsSignatureValid(signature));
+  EXPECT_FALSE(CallSignatureChallengeKey(
+      cryptohome::CHALLENGE_RSASSA_PKCS1_V1_5_SHA256, &signature));
+}
+
+// Verifies that the ChallengeKey request fails when the test provider returns
+// an error to the signature request.
+IN_PROC_BROWSER_TEST_F(CryptohomeKeyDelegateServiceProviderTest,
+                       SignatureErrorWhileSigning) {
+  cert_provider_extension()->set_should_fail_sign_digest_requests(true);
+
+  std::vector<uint8_t> signature;
+  EXPECT_FALSE(CallSignatureChallengeKey(
+      cryptohome::CHALLENGE_RSASSA_PKCS1_V1_5_SHA256, &signature));
 }

chrome/test/data/extensions/test_certificate_provider/extension/background.js

@@ -54,7 +54,10 @@ chrome.certificateProvider.onSignDigestRequested.addListener(
           JSON.stringify(
               ['onSignDigestRequested', jsonifiableFromSignRequest(request)]),
           response => {
-            const signature = arrayBufferFromByteList(JSON.parse(response));
+            const parsedResponse = JSON.parse(response);
+            const signature = (parsedResponse === null) ?
+                undefined :
+                arrayBufferFromByteList(parsedResponse);
             reportCallback(signature);
           });
     });