From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Samuel Attard <sattard@salesforce.com>
Date: Thu, 29 Sep 2022 16:58:47 -0700
Subject: feat: ensure mas builds of the same application can use safestorage

This change ensures that MAS builds of applications with an equivilant darwin build that share the same name do not fight over access to the same Safe Storage account.

Specifically this changes the account name for app "My App" from "My App" to "My App AppStore" if the app is using a MAS build of Electron.

We attempt to migrate the safe storage key from the old account, if that migration succeeds we delete the old key and move on.

Existing apps that aren't built for the app store should be unimpacted, there is one edge case where a user uses BOTH an AppStore and a darwin build of the same app only one will keep it's access to the safestorage key as during the migration we delete the old account.  This is an acceptable edge case as no one should be actively using two versions of the same app.

diff --git a/components/os_crypt/sync/keychain_password_mac.mm b/components/os_crypt/sync/keychain_password_mac.mm
index 3bcbb33700b2b9349795c05c12e44b4fafcc0370..95ca7ef0986f6681885c78f53c35250971d04fa4 100644
--- a/components/os_crypt/sync/keychain_password_mac.mm
+++ b/components/os_crypt/sync/keychain_password_mac.mm
@@ -27,6 +27,12 @@
 using KeychainNameContainerType = const base::NoDestructor<std::string>;
 #endif
 
+#if IS_MAS_BUILD()
+const char kAccountNameSuffix[] = " App Store Key";
+#else
+const char kAccountNameSuffix[] = " Key";
+#endif
+
 namespace {
 
 // These two strings ARE indeed user facing.  But they are used to access
@@ -96,11 +102,49 @@
                                   uma_result);
   };
 
+  const std::string account_name_suffix = kAccountNameSuffix;
+  const std::string suffixed_account_name = GetAccountName() + account_name_suffix;
   auto password =
-      keychain_->FindGenericPassword(GetServiceName(), GetAccountName());
+      keychain_->FindGenericPassword(GetServiceName(), suffixed_account_name);
+
+  if (password.has_value()) {
+    return std::string(base::as_string_view(*password));
+  }
+
+  // If the error was anything other than "it does not exist" we should error out here
+  // This normally means the account exists but we were denied access to it
+  if (password.error() != errSecItemNotFound) {
+    OSSTATUS_LOG(ERROR, password.error()) << "Keychain lookup for suffixed key failed";
+    return std::string();
+  }
+
+  // If the suffixed account didn't exist, we should check if the legacy non-suffixed account
+  // exists. If it does we can use that key and migrate it to the new account
+  base::apple::ScopedCFTypeRef<SecKeychainItemRef> item_ref;
+  password =
+      keychain_->FindGenericPassword(GetServiceName(), GetAccountName(),
+                                     item_ref.InitializeInto());
 
   if (password.has_value()) {
     uma_result = FindGenericPasswordResult::kPasswordFound;
+
+    // If we found the legacy account name we should copy it over to
+    // the new suffixed account
+    OSStatus error = keychain_->AddGenericPassword(
+        GetServiceName(), suffixed_account_name, *password);
+
+    if (error == noErr) {
+      // If we successfully made the suffixed account we can delete the old
+      // account to ensure new apps don't try to use it and run into IAM
+      // issues
+      error = keychain_->ItemDelete(item_ref.get());
+      if (error != noErr) {
+        OSSTATUS_DLOG(ERROR, error) << "Keychain delete for legacy key failed";
+      }
+    } else {
+      OSSTATUS_DLOG(ERROR, error) << "Keychain add for suffixed key failed";
+    }
+
     return std::string(base::as_string_view(*password));
   }
 
diff --git a/crypto/apple/keychain.h b/crypto/apple/keychain.h
index 2438c3947f330fb496886b85c8eaf49767c68413..dc7da2fd2819eb64f2083de722a1b2771380abd5 100644
--- a/crypto/apple/keychain.h
+++ b/crypto/apple/keychain.h
@@ -18,6 +18,14 @@
 
 namespace crypto::apple {
 
+// TODO(smaddock): Migrate to SecItem* as part of
+// https://issues.chromium.org/issues/40233280
+#if BUILDFLAG(IS_IOS)
+using AppleSecKeychainItemRef = void*;
+#else
+using AppleSecKeychainItemRef = SecKeychainItemRef;
+#endif
+
 // DEPRECATED: use `KeychainV2` instead.
 // Wraps the KeychainServices API in a very thin layer, to allow it to be
 // mocked out for testing.
@@ -46,13 +54,18 @@ class CRYPTO_EXPORT Keychain {
   // std::vector<uint8_t> arm is populated instead.
   virtual base::expected<std::vector<uint8_t>, OSStatus> FindGenericPassword(
       std::string_view service_name,
-      std::string_view account_name) const = 0;
+      std::string_view account_name,
+      AppleSecKeychainItemRef* item = nullptr) const = 0;
 
   virtual OSStatus AddGenericPassword(
       std::string_view service_name,
       std::string_view account_name,
       base::span<const uint8_t> password) const = 0;
 
+#if BUILDFLAG(IS_MAC)
+  virtual OSStatus ItemDelete(AppleSecKeychainItemRef item) const = 0;
+#endif  // !BUILDFLAG(IS_MAC)
+
  protected:
   Keychain();
 };
diff --git a/crypto/apple/keychain_secitem.h b/crypto/apple/keychain_secitem.h
index eb74282adaba24ebd667f0ab3fc34dbe4cd8b527..7b91eb27489cece38eca719986255c5ec01c4bac 100644
--- a/crypto/apple/keychain_secitem.h
+++ b/crypto/apple/keychain_secitem.h
@@ -17,12 +17,17 @@ class CRYPTO_EXPORT KeychainSecItem : public Keychain {
 
   base::expected<std::vector<uint8_t>, OSStatus> FindGenericPassword(
       std::string_view service_name,
-      std::string_view account_name) const override;
+      std::string_view account_name,
+      AppleSecKeychainItemRef* item) const override;
 
   OSStatus AddGenericPassword(
       std::string_view service_name,
       std::string_view account_name,
       base::span<const uint8_t> password) const override;
+
+#if BUILDFLAG(IS_MAC)
+  OSStatus ItemDelete(AppleSecKeychainItemRef item) const override;
+#endif  // !BUILDFLAG(IS_MAC)
 };
 
 }  // namespace crypto::apple
diff --git a/crypto/apple/keychain_secitem.mm b/crypto/apple/keychain_secitem.mm
index d2b9526f9a0bfa9d12a594c35c71499810cb6bb0..aef0eb3508fc37e3a0e9e1c27a71e1aa1f568231 100644
--- a/crypto/apple/keychain_secitem.mm
+++ b/crypto/apple/keychain_secitem.mm
@@ -161,7 +161,8 @@
 
 base::expected<std::vector<uint8_t>, OSStatus>
 KeychainSecItem::FindGenericPassword(std::string_view service_name,
-                                     std::string_view account_name) const {
+                                     std::string_view account_name,
+                                     AppleSecKeychainItemRef* item) const {
   base::apple::ScopedCFTypeRef<CFDictionaryRef> query =
       MakeGenericPasswordQuery(service_name, account_name);
 
@@ -203,4 +204,13 @@
   return base::ToVector(base::apple::CFDataToSpan(password_data));
 }
 
+#if BUILDFLAG(IS_MAC)
+OSStatus KeychainSecItem::ItemDelete(AppleSecKeychainItemRef item) const {
+  // TODO(smaddock): AppleSecKeychainItemRef aliases the deprecated
+  // SecKeychainItemRef. Need to update this to accept a CFDictionary in the
+  // case of SecItemDelete.
+  return noErr;
+}
+#endif
+
 }  // namespace crypto::apple
diff --git a/crypto/apple/keychain_seckeychain.cc b/crypto/apple/keychain_seckeychain.cc
index 4ad5acbf29607fe2ea28637f5470599984e317db..61056948062f3fcb5138e309b9f5019c96f9ce00 100644
--- a/crypto/apple/keychain_seckeychain.cc
+++ b/crypto/apple/keychain_seckeychain.cc
@@ -25,14 +25,15 @@ KeychainSecKeychain::~KeychainSecKeychain() = default;
 
 base::expected<std::vector<uint8_t>, OSStatus>
 KeychainSecKeychain::FindGenericPassword(std::string_view service_name,
-                                         std::string_view account_name) const {
+                                         std::string_view account_name,
+                                         AppleSecKeychainItemRef* item) const {
   base::AutoLock lock(GetMacSecurityServicesLock());
   uint32_t password_length = 0;
   void* password_data = nullptr;
   OSStatus status = SecKeychainFindGenericPassword(
       nullptr, service_name.length(), service_name.data(),
       account_name.length(), account_name.data(), &password_length,
-      &password_data, nullptr);
+      &password_data, item);
   if (status != noErr) {
     return base::unexpected(status);
   }
@@ -58,6 +59,11 @@ OSStatus KeychainSecKeychain::AddGenericPassword(
       password.data(), nullptr);
 }
 
+OSStatus KeychainSecKeychain::ItemDelete(AppleSecKeychainItemRef item) const {
+  base::AutoLock lock(GetMacSecurityServicesLock());
+  return SecKeychainItemDelete(item);
+}
+
 #pragma clang diagnostic pop
 
 }  // namespace crypto::apple
diff --git a/crypto/apple/keychain_seckeychain.h b/crypto/apple/keychain_seckeychain.h
index 991b2c8debaa1812812fb04e7ab6bf437c874691..191ea7532d269eae7a975229b12227cd32b56ede 100644
--- a/crypto/apple/keychain_seckeychain.h
+++ b/crypto/apple/keychain_seckeychain.h
@@ -20,12 +20,17 @@ class CRYPTO_EXPORT KeychainSecKeychain : public Keychain {
 
   base::expected<std::vector<uint8_t>, OSStatus> FindGenericPassword(
       std::string_view service_name,
-      std::string_view account_name) const override;
+      std::string_view account_name,
+      AppleSecKeychainItemRef* item) const override;
 
   OSStatus AddGenericPassword(
       std::string_view service_name,
       std::string_view account_name,
       base::span<const uint8_t> password) const override;
+
+#if !BUILDFLAG(IS_IOS)
+  OSStatus ItemDelete(SecKeychainItemRef itemRef) const override;
+#endif  // !BUILDFLAG(IS_IOS)
 };
 
 }  // namespace crypto::apple
diff --git a/crypto/apple/mock_keychain.cc b/crypto/apple/mock_keychain.cc
index 080806aaf3fc10548b160850ad36ef3519ea2b6f..21f04059d67ba41118face6ee9327aa05e854362 100644
--- a/crypto/apple/mock_keychain.cc
+++ b/crypto/apple/mock_keychain.cc
@@ -32,7 +32,8 @@ MockKeychain::~MockKeychain() = default;
 
 base::expected<std::vector<uint8_t>, OSStatus>
 MockKeychain::FindGenericPassword(std::string_view service_name,
-                                  std::string_view account_name) const {
+                                  std::string_view account_name,
+                                  AppleSecKeychainItemRef* item) const {
   IncrementKeychainAccessHistogram();
 
   // When simulating |noErr|, return canned |passwordData| and
@@ -56,6 +57,10 @@ OSStatus MockKeychain::AddGenericPassword(
   return noErr;
 }
 
+OSStatus MockKeychain::ItemDelete(SecKeychainItemRef itemRef) const {
+  return noErr;
+}
+
 std::string MockKeychain::GetEncryptionPassword() const {
   IncrementKeychainAccessHistogram();
   return kPassword;
diff --git a/crypto/apple/mock_keychain.h b/crypto/apple/mock_keychain.h
index 40fb4e1f75a2b7f021a8a20b01d63c7e3b8c5fa3..3c3f41e7ffcaeb74aaff872ce3b54027312ec0dd 100644
--- a/crypto/apple/mock_keychain.h
+++ b/crypto/apple/mock_keychain.h
@@ -37,13 +37,18 @@ class CRYPTO_EXPORT MockKeychain : public Keychain {
   // Keychain implementation.
   base::expected<std::vector<uint8_t>, OSStatus> FindGenericPassword(
       std::string_view service_name,
-      std::string_view account_name) const override;
+      std::string_view account_name,
+      AppleSecKeychainItemRef* item) const override;
 
   OSStatus AddGenericPassword(
       std::string_view service_name,
       std::string_view account_name,
       base::span<const uint8_t> password) const override;
 
+#if !BUILDFLAG(IS_IOS)
+  OSStatus ItemDelete(SecKeychainItemRef itemRef) const override;
+#endif  // !BUILDFLAG(IS_IOS)
+
   // Returns the password that OSCrypt uses to generate its encryption key.
   std::string GetEncryptionPassword() const;
 
