Defer data hashing in cert provider service

Refactor CertificateProviderService to defer the input data hashing till
the very last point before sending the signature request to the
extension. As a consequence, change the CryptohomeKeyDelegate D-Bus
service to pass the unhashed data. Also kill the call in the
CertStoreInstance Mojo service that was passing pre-hashed data (that
service will anyway be deleted in one of follow-ups - see b/119914122).

This is a preparation step for introducing a new version of the
chrome.certificateProvider API that will pass the unhashed inputs to the
extension.

There's no use-visible changes expected. All changes, except the one of
CertStoreInstance, are backwards-compatible, and the CertStoreInstance
is an unlaunched functionality that will be deleted soon (see
b/119914122).

Bug: 1078761
Change-Id: I656f46c41fdd5fd779e27abe76ee6c01ea2349f2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2246177
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Fabian Sommer <fabiansommer@chromium.org>
Reviewed-by: Polina Bondarenko <pbond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#783089}

chrome/browser/chromeos/arc/enterprise/cert_store/security_token_operation_bridge.cc

@@ -93,10 +93,8 @@ void SecurityTokenOperationBridge::SignDigest(
                             base::nullopt);
     return;
   }
-  certificate_provider_service_->RequestSignatureBySpki(
-      subject_public_key_info, crypto_algorithm.value(), data, base::nullopt,
-      base::BindOnce(&SecurityTokenOperationBridge::OnSignCompleted,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+  // TODO(b/119914122): Call RequestSignatureBySpki().
+  std::move(callback).Run(mojom::SignatureResult::kFailed, base::nullopt);
 }
 
 void SecurityTokenOperationBridge::OnSignCompleted(

chrome/browser/chromeos/arc/enterprise/cert_store/security_token_operation_bridge_unittest.cc

@@ -181,10 +181,7 @@ TEST_P(SecurityTokenOperationBridgeTest, BasicTest) {
 INSTANTIATE_TEST_SUITE_P(
     SecurityTokenOperationBridgeTest,
     SecurityTokenOperationBridgeTest,
-    ::testing::Values(std::make_tuple(mojom::SignatureResult::kOk,
-                                      kCertFileName,
-                                      std::vector<uint8_t>(1)),
-                      std::make_tuple(mojom::SignatureResult::kFailed,
+    ::testing::Values(std::make_tuple(mojom::SignatureResult::kFailed,
                                       kCertFileName,
                                       std::vector<uint8_t>()),
                       std::make_tuple(mojom::SignatureResult::kFailed,

chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc

@@ -24,8 +24,6 @@
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "chrome/browser/chromeos/certificate_provider/certificate_provider.h"
 #include "net/base/net_errors.h"
-#include "third_party/boringssl/src/include/openssl/digest.h"
-#include "third_party/boringssl/src/include/openssl/ssl.h"
 
 namespace chromeos {
 
@@ -184,25 +182,13 @@ void CertificateProviderService::SSLPrivateKey::Sign(
   // PostSignResult().
   callback = base::BindOnce(&PostSignResult, std::move(callback));
 
-  // The extension expects the input to be hashed ahead of time.
-  const EVP_MD* md = SSL_get_signature_algorithm_digest(algorithm);
-  uint8_t digest[EVP_MAX_MD_SIZE];
-  unsigned digest_len;
-  if (!md || !EVP_Digest(input.data(), input.size(), digest, &digest_len, md,
-                         nullptr)) {
-    std::move(callback).Run(net::ERR_SSL_CLIENT_AUTH_SIGNATURE_FAILED,
-                            /*signature=*/{});
-    return;
-  }
-
   if (!service_) {
     std::move(callback).Run(net::ERR_FAILED, /*signature=*/{});
     return;
   }
 
   service_->RequestSignatureFromExtension(
-      extension_id_, cert_info_.certificate, algorithm,
-      base::make_span(digest, digest_len),
+      extension_id_, cert_info_.certificate, algorithm, input,
       /*authenticating_user_account_id=*/{}, std::move(callback));
 }
 
@@ -331,7 +317,7 @@ void CertificateProviderService::OnExtensionUnloaded(
 void CertificateProviderService::RequestSignatureBySpki(
     const std::string& subject_public_key_info,
     uint16_t algorithm,
-    base::span<const uint8_t> digest,
+    base::span<const uint8_t> input,
     const base::Optional<AccountId>& authenticating_user_account_id,
     net::SSLPrivateKey::SignCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -347,7 +333,7 @@ void CertificateProviderService::RequestSignatureBySpki(
   }
 
   RequestSignatureFromExtension(extension_id, info.certificate, algorithm,
-                                digest, authenticating_user_account_id,
+                                input, authenticating_user_account_id,
                                 std::move(callback));
 }
 
@@ -454,7 +440,7 @@ void CertificateProviderService::RequestSignatureFromExtension(
     const std::string& extension_id,
     const scoped_refptr<net::X509Certificate>& certificate,
     uint16_t algorithm,
-    base::span<const uint8_t> digest,
+    base::span<const uint8_t> input,
     const base::Optional<AccountId>& authenticating_user_account_id,
     net::SSLPrivateKey::SignCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -470,7 +456,7 @@ void CertificateProviderService::RequestSignatureFromExtension(
   pin_dialog_manager_.AddSignRequestId(extension_id, sign_request_id,
                                        authenticating_user_account_id);
   if (!delegate_->DispatchSignRequestToExtension(
-          extension_id, sign_request_id, algorithm, certificate, digest)) {
+          extension_id, sign_request_id, algorithm, certificate, input)) {
     // TODO(crbug.com/1046860): Remove logging after stabilizing the feature.
     VLOG(1) << "Failed to dispatch signature request to extension "
             << extension_id << " id " << sign_request_id;

chrome/browser/chromeos/certificate_provider/certificate_provider_service.h

@@ -91,7 +91,7 @@ class CertificateProviderService : public KeyedService {
         int sign_request_id,
         uint16_t algorithm,
         const scoped_refptr<net::X509Certificate>& certificate,
-        base::span<const uint8_t> digest) = 0;
+        base::span<const uint8_t> input) = 0;
 
    private:
     DISALLOW_COPY_AND_ASSIGN(Delegate);
@@ -144,7 +144,7 @@ class CertificateProviderService : public KeyedService {
   // not the sign request id alone but only the pair (extension id, sign request
   // id) is unambiguous.
   // If the signature could be calculated by the extension, |signature| is
-  // provided in the reply and should be the signature of the digest sent in the
+  // provided in the reply and should be the signature of the data sent in the
   // sign request. Otherwise, in case of a failure, |signature| must be empty.
   // The call is ignored if |sign_request_id| is not referring to a pending
   // request.
@@ -180,14 +180,14 @@ class CertificateProviderService : public KeyedService {
   void OnExtensionUnloaded(const std::string& extension_id);
 
   // Requests the extension which provided the certificate identified by
-  // |subject_public_key_info| to sign |digest| with the corresponding private
-  // key. |algorithm| is a TLS 1.3 SignatureScheme value. See net::SSLPrivateKey
-  // for details. |callback| will be run with the reply of the extension or an
-  // error.
+  // |subject_public_key_info| to sign the unhashed |input| with the
+  // corresponding private key. |algorithm| is a TLS 1.3 SignatureScheme value.
+  // See net::SSLPrivateKey for details. |callback| will be run with the reply
+  // of the extension or an error.
   void RequestSignatureBySpki(
       const std::string& subject_public_key_info,
       uint16_t algorithm,
-      base::span<const uint8_t> digest,
+      base::span<const uint8_t> input,
       const base::Optional<AccountId>& authenticating_user_account_id,
       net::SSLPrivateKey::SignCallback callback);
 
@@ -230,16 +230,15 @@ class CertificateProviderService : public KeyedService {
   // are processed.
   void TerminateCertificateRequest(int cert_request_id);
 
-  // Requests extension with |extension_id| to sign |digest| with the private
-  // key certified by |certificate|. |algorithm| is a TLS 1.3 SignatureScheme
-  // value. See net::SSLPrivateKey for details. |digest| was created by
-  // |algorithm|'s prehash.  |callback| will be run with the reply of the
-  // extension or an error.
+  // Requests extension with |extension_id| to sign the unhashed |input| with
+  // the private key certified by |certificate|. |algorithm| is a TLS 1.3
+  // SignatureScheme value. See net::SSLPrivateKey for details. |callback| will
+  // be run with the reply of the extension or an error.
   void RequestSignatureFromExtension(
       const std::string& extension_id,
       const scoped_refptr<net::X509Certificate>& certificate,
       uint16_t algorithm,
-      base::span<const uint8_t> digest,
+      base::span<const uint8_t> input,
       const base::Optional<AccountId>& authenticating_user_account_id,
       net::SSLPrivateKey::SignCallback callback);
 

chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc

@@ -9,6 +9,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/containers/span.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/singleton.h"
@@ -28,6 +29,7 @@
 #include "net/cert/x509_certificate.h"
 #include "net/cert/x509_util.h"
 #include "net/ssl/ssl_private_key.h"
+#include "third_party/boringssl/src/include/openssl/digest.h"
 #include "third_party/boringssl/src/include/openssl/ssl.h"
 
 namespace chromeos {
@@ -56,7 +58,7 @@ class DefaultDelegate : public CertificateProviderService::Delegate,
       int request_id,
       uint16_t algorithm,
       const scoped_refptr<net::X509Certificate>& certificate,
-      base::span<const uint8_t> digest) override;
+      base::span<const uint8_t> input) override;
 
   // extensions::EventRouter::Observer:
   void OnListenerAdded(const extensions::EventListenerInfo& details) override {}
@@ -113,12 +115,13 @@ bool DefaultDelegate::DispatchSignRequestToExtension(
     int request_id,
     uint16_t algorithm,
     const scoped_refptr<net::X509Certificate>& certificate,
-    base::span<const uint8_t> digest) {
+    base::span<const uint8_t> input) {
   const std::string event_name(api_cp::OnSignDigestRequested::kEventName);
   if (!event_router_->ExtensionHasEventListener(extension_id, event_name))
     return false;
 
   api_cp::SignRequest request;
+
   request.sign_request_id = request_id;
   switch (algorithm) {
     case SSL_SIGN_RSA_PKCS1_MD5_SHA1:
@@ -140,11 +143,20 @@ bool DefaultDelegate::DispatchSignRequestToExtension(
       LOG(ERROR) << "Unknown signature algorithm";
       return false;
   }
-  request.digest.assign(digest.begin(), digest.end());
   base::StringPiece cert_der =
       net::x509_util::CryptoBufferAsStringPiece(certificate->cert_buffer());
   request.certificate.assign(cert_der.begin(), cert_der.end());
 
+  // The extension expects the input to be hashed ahead of time.
+  request.digest.resize(EVP_MAX_MD_SIZE);
+  const EVP_MD* md = SSL_get_signature_algorithm_digest(algorithm);
+  unsigned digest_len;
+  if (!md || !EVP_Digest(input.data(), input.size(), request.digest.data(),
+                         &digest_len, md, /*ENGINE *impl=*/nullptr)) {
+    return false;
+  }
+  request.digest.resize(digest_len);
+
   std::unique_ptr<base::ListValue> internal_args(new base::ListValue);
   internal_args->AppendInteger(request_id);
   internal_args->Append(request.ToValue());

chrome/browser/chromeos/dbus/cryptohome_key_delegate_service_provider.cc

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "base/containers/span.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "chrome/browser/chromeos/certificate_provider/certificate_provider_service.h"
@@ -22,7 +23,6 @@
 #include "components/account_id/account_id.h"
 #include "dbus/message.h"
 #include "net/base/net_errors.h"
-#include "third_party/boringssl/src/include/openssl/digest.h"
 #include "third_party/boringssl/src/include/openssl/ssl.h"
 #include "third_party/cros_system_api/dbus/cryptohome/dbus-constants.h"
 
@@ -55,26 +55,6 @@ bool ChallengeSignatureAlgorithmToSslAlgorithm(
   }
 }
 
-// Builds the digest of the given input, using the hashing algorithm that is
-// required for the given signature algorithm.
-bool BuildDigestToSign(const std::string& input,
-                       uint16_t ssl_algorithm,
-                       std::vector<uint8_t>* digest) {
-  DCHECK(!input.empty());
-  const EVP_MD* md = SSL_get_signature_algorithm_digest(ssl_algorithm);
-  if (!md)
-    return false;
-  digest->resize(EVP_MAX_MD_SIZE);
-  unsigned digest_len = 0;
-  if (!EVP_Digest(input.data(), input.size(), digest->data(), &digest_len, md,
-                  nullptr)) {
-    digest->clear();
-    return false;
-  }
-  digest->resize(digest_len);
-  return true;
-}
-
 // Completes the "ChallengeKey" D-Bus call of the |CHALLENGE_TYPE_SIGNATURE|
 // type with the given signature, or with an error if the signature wasn't
 // successfully generated.
@@ -172,17 +152,9 @@ void HandleSignatureKeyChallenge(
     return;
   }
 
-  std::vector<uint8_t> digest;
-  if (!BuildDigestToSign(challenge_request_data.data_to_sign(), ssl_algorithm,
-                         &digest)) {
-    std::move(response_sender)
-        .Run(dbus::ErrorResponse::FromMethodCall(method_call, DBUS_ERROR_FAILED,
-                                                 "Failed to build digest"));
-    return;
-  }
-
   certificate_provider_service->RequestSignatureBySpki(
-      challenge_request_data.public_key_spki_der(), ssl_algorithm, digest,
+      challenge_request_data.public_key_spki_der(), ssl_algorithm,
+      base::as_bytes(base::make_span(challenge_request_data.data_to_sign())),
       account_id,
       base::BindOnce(&CompleteSignatureKeyChallenge,
                      base::Unretained(method_call),