Document blocking potential of deleting UnexportableKeys

UnexportableKeyProvider::DeleteSigningKey can block on Mac, but there
is no indication of this in the code.

This renames the method to DeleteSigningKeySlowly (as is the
convention with other methods in the same class), adds a comment about
it, and adds a ScopedBlockingCall instance to its Mac implementation.

Change-Id: Icbd5a12f23dff62fc2abbc0bf9b164e482d66978
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5491304
Reviewed-by: Adam Langley <agl@chromium.org>
Reviewed-by: Sébastien Lalancette <seblalancette@chromium.org>
Commit-Queue: Ken Buchanan <kenrb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1293209}

chrome/browser/enterprise/connectors/device_trust/key_management/core/ec_signing_key.cc

@@ -125,7 +125,7 @@ ECSigningKeyProvider::FromWrappedSigningKeySlowly(
   return std::make_unique<ECSigningKey>(wrapped_key);
 }
 
-bool ECSigningKeyProvider::DeleteSigningKey(
+bool ECSigningKeyProvider::DeleteSigningKeySlowly(
     base::span<const uint8_t> wrapped_key) {
   // Software keys are stateless.
   return true;

chrome/browser/enterprise/connectors/device_trust/key_management/core/ec_signing_key.h

@@ -29,7 +29,7 @@ class ECSigningKeyProvider : public crypto::UnexportableKeyProvider {
           acceptable_algorithms) override;
   std::unique_ptr<crypto::UnexportableSigningKey> FromWrappedSigningKeySlowly(
       base::span<const uint8_t> wrapped_key) override;
-  bool DeleteSigningKey(base::span<const uint8_t> wrapped_key) override;
+  bool DeleteSigningKeySlowly(base::span<const uint8_t> wrapped_key) override;
 };
 
 }  // namespace enterprise_connectors

chrome/browser/webauthn/enclave_manager.cc

@@ -3137,7 +3137,7 @@ void EnclaveManager::ClearRegistration() {
       base::BindOnce(
           [](std::vector<uint8_t> wrapped_hardware_private_key) {
             if (auto provider = GetUnexportableKeyProvider()) {
-              provider->DeleteSigningKey(wrapped_hardware_private_key);
+              provider->DeleteSigningKeySlowly(wrapped_hardware_private_key);
             }
           },
           ToVector(user_->wrapped_hardware_private_key())));

crypto/unexportable_key.h

@@ -144,7 +144,10 @@ class CRYPTO_EXPORT UnexportableKeyProvider {
   // key on such implementations. For stateless implementations, this is a
   // no-op.
   // Returns true on successful deletion, false otherwise.
-  virtual bool DeleteSigningKey(base::span<const uint8_t> wrapped_key) = 0;
+  // This can sometimes block, and therefore must not be called from the UI
+  // thread.
+  virtual bool DeleteSigningKeySlowly(
+      base::span<const uint8_t> wrapped_key) = 0;
 };
 
 // This is an experimental API as it uses an unofficial Windows API.

crypto/unexportable_key_mac.h

@@ -56,7 +56,7 @@ class UnexportableKeyProviderMac : public UnexportableKeyProvider {
           acceptable_algorithms) override;
   std::unique_ptr<UnexportableSigningKey> FromWrappedSigningKeySlowly(
       base::span<const uint8_t> wrapped_key) override;
-  bool DeleteSigningKey(base::span<const uint8_t> wrapped_key) override;
+  bool DeleteSigningKeySlowly(base::span<const uint8_t> wrapped_key) override;
 
  private:
   struct ObjCStorage;

crypto/unexportable_key_mac.mm

@@ -2,6 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "crypto/unexportable_key.h"
+
+#import <CoreFoundation/CoreFoundation.h>
+#import <CryptoTokenKit/CryptoTokenKit.h>
+#import <Foundation/Foundation.h>
+#include <LocalAuthentication/LocalAuthentication.h>
+#import <Security/Security.h>
+
 #include <algorithm>
 #include <iterator>
 #include <memory>
@@ -9,12 +17,6 @@
 #include <utility>
 #include <vector>
 
-#import <CoreFoundation/CoreFoundation.h>
-#import <CryptoTokenKit/CryptoTokenKit.h>
-#import <Foundation/Foundation.h>
-#include <LocalAuthentication/LocalAuthentication.h>
-#import <Security/Security.h>
-
 #include "base/apple/bridging.h"
 #include "base/apple/foundation_util.h"
 #include "base/apple/scoped_cftyperef.h"
@@ -25,11 +27,11 @@
 #include "base/memory/scoped_policy.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/sys_string_conversions.h"
+#include "base/threading/scoped_blocking_call.h"
 #include "crypto/apple_keychain_util.h"
 #include "crypto/apple_keychain_v2.h"
 #include "crypto/features.h"
 #include "crypto/signature_verifier.h"
-#include "crypto/unexportable_key.h"
 #include "crypto/unexportable_key_mac.h"
 #include "third_party/boringssl/src/include/openssl/bn.h"
 #include "third_party/boringssl/src/include/openssl/bytestring.h"
@@ -304,8 +306,10 @@ UnexportableKeyProviderMac::FromWrappedSigningKeySlowly(
                                                      key_attributes);
 }
 
-bool UnexportableKeyProviderMac::DeleteSigningKey(
+bool UnexportableKeyProviderMac::DeleteSigningKeySlowly(
     base::span<const uint8_t> wrapped_key) {
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::WILL_BLOCK);
   NSDictionary* query = @{
     CFToNSPtrCast(kSecClass) : CFToNSPtrCast(kSecClassKey),
     CFToNSPtrCast(kSecAttrKeyType) :

crypto/unexportable_key_mac_unittest.mm

@@ -49,7 +49,7 @@ TEST_F(UnexportableKeyMacTest, DeleteSigningKey) {
       provider->GenerateSigningKeySlowly(kAcceptableAlgos);
   ASSERT_TRUE(key);
   ASSERT_TRUE(provider->FromWrappedSigningKeySlowly(key->GetWrappedKey()));
-  EXPECT_TRUE(provider->DeleteSigningKey(key->GetWrappedKey()));
+  EXPECT_TRUE(provider->DeleteSigningKeySlowly(key->GetWrappedKey()));
   EXPECT_FALSE(provider->FromWrappedSigningKeySlowly(key->GetWrappedKey()));
   EXPECT_TRUE(scoped_fake_apple_keychain_.keychain()->items().empty());
 }
@@ -57,7 +57,7 @@ TEST_F(UnexportableKeyMacTest, DeleteSigningKey) {
 TEST_F(UnexportableKeyMacTest, DeleteUnknownSigningKey) {
   std::unique_ptr<UnexportableKeyProvider> provider =
       GetUnexportableKeyProvider(config);
-  EXPECT_FALSE(provider->DeleteSigningKey(std::vector<uint8_t>{1, 2, 3}));
+  EXPECT_FALSE(provider->DeleteSigningKeySlowly(std::vector<uint8_t>{1, 2, 3}));
 }
 
 TEST_F(UnexportableKeyMacTest, GetSecKeyRef) {

crypto/unexportable_key_metrics.cc

@@ -242,7 +242,7 @@ void MeasureTpmOperationsInternal(UnexportableKeyProvider::Config config) {
   }
 
   auto delete_key = [&provider](UnexportableSigningKey* key) {
-    provider->DeleteSigningKey(key->GetWrappedKey());
+    provider->DeleteSigningKeySlowly(key->GetWrappedKey());
     delete key;
   };
   base::ElapsedTimer key_creation_timer;

crypto/unexportable_key_metrics_unittest.cc

@@ -50,8 +50,8 @@ class MockTrackingUnexportableKeyProvider : public UnexportableKeyProvider {
         << "Attempted to delete non existing key";
     return key_provider_->FromWrappedSigningKeySlowly(wrapped_key);
   }
-  bool DeleteSigningKey(base::span<const uint8_t> wrapped_key) override {
-    key_provider_->DeleteSigningKey(wrapped_key);
+  bool DeleteSigningKeySlowly(base::span<const uint8_t> wrapped_key) override {
+    key_provider_->DeleteSigningKeySlowly(wrapped_key);
     return keys_.erase(
         std::vector<uint8_t>(wrapped_key.begin(), wrapped_key.end()));
   }

crypto/unexportable_key_software_unsecure.cc

@@ -118,7 +118,7 @@ class SoftwareProvider : public UnexportableKeyProvider {
     return std::make_unique<SoftwareECDSA>(std::move(key));
   }
 
-  bool DeleteSigningKey(base::span<const uint8_t> wrapped_key) override {
+  bool DeleteSigningKeySlowly(base::span<const uint8_t> wrapped_key) override {
     // Unexportable software keys are stateless.
     return true;
   }

crypto/unexportable_key_unittest.cc

@@ -134,6 +134,6 @@ TEST_P(UnexportableKeySigningTest, RoundTrip) {
   verifier2.VerifyUpdate(msg);
   ASSERT_TRUE(verifier2.VerifyFinal());
 
-  EXPECT_TRUE(provider->DeleteSigningKey(wrapped));
+  EXPECT_TRUE(provider->DeleteSigningKeySlowly(wrapped));
 }
 }  // namespace

crypto/unexportable_key_win.cc

@@ -486,7 +486,7 @@ class UnexportableKeyProviderWin : public UnexportableKeyProvider {
     return nullptr;
   }
 
-  bool DeleteSigningKey(base::span<const uint8_t> wrapped) override {
+  bool DeleteSigningKeySlowly(base::span<const uint8_t> wrapped) override {
     // Unexportable keys are stateless on Windows.
     return true;
   }

crypto/user_verifying_key_mac.mm

@@ -143,7 +143,7 @@ bool DoDeleteKey(std::vector<uint8_t> wrapped_key,
   if (!key_provider) {
     return false;
   }
-  return key_provider->DeleteSigningKey(wrapped_key);
+  return key_provider->DeleteSigningKeySlowly(wrapped_key);
 }
 
 class UserVerifyingKeyProviderMac : public UserVerifyingKeyProvider {