Revert "[OSCrypt] Add feature for preventing key overwrites in Keychain on Mac"

This reverts commit 3ca917bd.

Reason for revert: The CL caused crashes on Chrome canary on Mac: crbug.com/882787

Original change's description:
> [OSCrypt] Add feature for preventing key overwrites in Keychain on Mac
>
> This Cl includes:
> - Add a feature flag which is disabled by default.
> - Include key creation utility to prevent key overwrites in KeychainPassword::GetPassword().
> - Add tests for the above changes.
> - Register the local state early from the main thread in os_crypt.
>
> Bug: 791541
> Change-Id: I2a664cd285fe73b32a15b2949977b940d95e7bbe
> Reviewed-on: https://chromium-review.googlesource.com/1188318
> Commit-Queue: Tonko Sabolčec <tsabolcec@google.com>
> Reviewed-by: Christos Froussios <cfroussios@chromium.org>
> Reviewed-by: Dominic Battré <battre@chromium.org>
> Reviewed-by: Robert Sesek <rsesek@chromium.org>
> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#589898}

TBR=battre@chromium.org,vasilii@chromium.org,rsesek@chromium.org,cfroussios@chromium.org,tsabolcec@google.com

Change-Id: I1be5c8294985e81115a8856f4105fbff8a3a28a9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 791541
Reviewed-on: https://chromium-review.googlesource.com/1219086Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Tonko Sabolčec <tsabolcec@google.com>
Cr-Commit-Position: refs/heads/master@{#590270}

chrome/browser/chrome_browser_main_mac.mm

@@ -32,7 +32,6 @@
 #include "chrome/common/chrome_switches.h"
 #include "components/crash/content/app/crashpad.h"
 #include "components/metrics/metrics_service.h"
-#include "components/os_crypt/os_crypt.h"
 #include "content/public/common/main_function_params.h"
 #include "content/public/common/result_codes.h"
 #include "ui/base/l10n/l10n_util_mac.h"
@@ -161,11 +160,6 @@ void ChromeBrowserMainPartsMac::PostMainMessageLoopStart() {
 }
 
 void ChromeBrowserMainPartsMac::PreProfileInit() {
-  // Initialize the OSCrypt.
-  PrefService* local_state = g_browser_process->local_state();
-  DCHECK(local_state);
-  OSCrypt::Init(local_state);
-
   MacStartupProfiler::GetInstance()->Profile(
       MacStartupProfiler::PRE_PROFILE_INIT);
   ChromeBrowserMainPartsPosix::PreProfileInit();

chrome/browser/prefs/browser_prefs.cc

@@ -290,7 +290,6 @@
 #if defined(OS_MACOSX)
 #include "chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.h"
 #include "chrome/browser/ui/cocoa/confirm_quit.h"
-#include "components/os_crypt/os_crypt.h"
 #endif
 
 #if defined(OS_WIN)
@@ -487,7 +486,6 @@ void RegisterLocalState(PrefRegistrySimple* registry) {
 
 #if defined(OS_MACOSX)
   confirm_quit::RegisterLocalState(registry);
-  OSCrypt::RegisterLocalPrefs(registry);
   QuitWithAppsController::RegisterPrefs(registry);
 #endif
 

components/os_crypt/BUILD.gn

@@ -32,17 +32,13 @@ if (use_gnome_keyring) {
 
 component("os_crypt") {
   sources = [
-    "encryption_key_creation_util_ios.cc",
-    "encryption_key_creation_util_ios.h",
-    "encryption_key_creation_util_mac.cc",
-    "encryption_key_creation_util_mac.h",
     "ie7_password_win.cc",
     "ie7_password_win.h",
+    "key_creation_util_mac.cc",
+    "key_creation_util_mac.h",
     "keychain_password_mac.h",
     "keychain_password_mac.mm",
     "os_crypt.h",
-    "os_crypt_features_mac.cc",
-    "os_crypt_features_mac.h",
     "os_crypt_mac.mm",
     "os_crypt_pref_names_mac.cc",
     "os_crypt_pref_names_mac.h",
@@ -77,10 +73,6 @@ component("os_crypt") {
     set_sources_assignment_filter(sources_assignment_filter)
   }
 
-  if (is_ios || is_mac) {
-    sources += [ "encryption_key_creation_util.h" ]
-  }
-
   if (is_win) {
     libs = [ "crypt32.lib" ]
   }
@@ -171,18 +163,11 @@ source_set("unit_tests") {
     ":test_support",
     "//base",
     "//base/test:test_support",
-    "//components/prefs:test_support",
     "//crypto",
     "//testing/gmock",
     "//testing/gtest",
   ]
 
-  if (is_ios) {
-    set_sources_assignment_filter([])
-    sources += [ "keychain_password_mac_unittest.mm" ]
-    set_sources_assignment_filter(sources_assignment_filter)
-  }
-
   if (is_desktop_linux && !is_chromecast) {
     sources += [
       "key_storage_linux_unittest.cc",

components/os_crypt/encryption_key_creation_util.h

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef COMPONENTS_OS_CRYPT_ENCRYPTION_KEY_CREATION_UTIL_H_
-#define COMPONENTS_OS_CRYPT_ENCRYPTION_KEY_CREATION_UTIL_H_
-
-#include "base/component_export.h"
-
-namespace os_crypt {
-
-// An interface for the utility that prevents overwriting the encryption key on
-// Mac.
-// This class is used on Mac and iOS, but does nothing on iOS as the feature
-// for preventing key overwrites is available only on Mac. The object for
-// the Mac class has to be created on the main thread.
-class EncryptionKeyCreationUtil {
- public:
-  virtual ~EncryptionKeyCreationUtil() = default;
-
-  // Returns true iff the key should already exists on Mac and false on iOS.
-  // This method doesn't need to be called from the main thread.
-  virtual bool KeyAlreadyCreated() = 0;
-
-  // Returns true iff the feature for preventing key overwrites is enabled on
-  // Mac and false on iOS. This method doesn't need to be called from the main
-  // thread.
-  virtual bool ShouldPreventOverwriting() = 0;
-
-  // This asynchronously updates the preference on the main thread that the key
-  // was created. This method is called when key is added to the Keychain, or
-  // the first time the key is successfully retrieved from the Keychain and the
-  // preference hasn't been set yet. This method doesn't need to be called on
-  // the main thread.
-  virtual void OnKeyWasStored() = 0;
-};
-
-}  // namespace os_crypt
-
-#endif  // COMPONENTS_OS_CRYPT_ENCRYPTION_KEY_CREATION_UTIL_H_

components/os_crypt/encryption_key_creation_util_ios.cc

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "components/os_crypt/encryption_key_creation_util_ios.h"
-
-namespace os_crypt {
-
-EncryptionKeyCreationUtilIOS::EncryptionKeyCreationUtilIOS() = default;
-
-EncryptionKeyCreationUtilIOS::~EncryptionKeyCreationUtilIOS() = default;
-
-bool EncryptionKeyCreationUtilIOS::KeyAlreadyCreated() {
-  return false;
-}
-
-bool EncryptionKeyCreationUtilIOS::ShouldPreventOverwriting() {
-  return false;
-}
-
-void EncryptionKeyCreationUtilIOS::OnKeyWasStored() {}
-
-}  // namespace os_crypt

components/os_crypt/encryption_key_creation_util_ios.h

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef COMPONENTS_OS_CRYPT_ENCRYPTION_KEY_CREATION_UTIL_IOS_H_
-#define COMPONENTS_OS_CRYPT_ENCRYPTION_KEY_CREATION_UTIL_IOS_H_
-
-#include "base/component_export.h"
-#include "base/macros.h"
-#include "components/os_crypt/encryption_key_creation_util.h"
-
-namespace os_crypt {
-
-// A key creation utility for iOS which does nothing as there is no feature
-// for preventing key overwrites for iOS.
-class COMPONENT_EXPORT(OS_CRYPT) EncryptionKeyCreationUtilIOS
-    : public EncryptionKeyCreationUtil {
- public:
-  EncryptionKeyCreationUtilIOS();
-  ~EncryptionKeyCreationUtilIOS() override;
-
-  // Returns false.
-  bool KeyAlreadyCreated() override;
-
-  // Returns false.
-  bool ShouldPreventOverwriting() override;
-
-  // Does nothing.
-  void OnKeyWasStored() override;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(EncryptionKeyCreationUtilIOS);
-};
-
-}  // namespace os_crypt
-
-#endif  // COMPONENTS_OS_CRYPT_ENCRYPTION_KEY_CREATION_UTIL_IOS_H_

components/os_crypt/encryption_key_creation_util_mac.cc → components/os_crypt/key_creation_util_mac.cc

@@ -2,37 +2,25 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/os_crypt/encryption_key_creation_util_mac.h"
+#include "components/os_crypt/key_creation_util_mac.h"
 
 #include "base/bind.h"
-#include "base/feature_list.h"
 #include "base/single_thread_task_runner.h"
-#include "components/os_crypt/os_crypt_features_mac.h"
 #include "components/os_crypt/os_crypt_pref_names_mac.h"
 #include "components/prefs/pref_service.h"
 
 namespace os_crypt {
 
-EncryptionKeyCreationUtilMac::EncryptionKeyCreationUtilMac(
+KeyCreationUtilMac::KeyCreationUtilMac(
     PrefService* local_state,
     scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner)
     : local_state_(local_state),
       main_thread_task_runner_(main_thread_task_runner),
       key_already_created_(local_state_->GetBoolean(prefs::kKeyCreated)) {}
 
-EncryptionKeyCreationUtilMac::~EncryptionKeyCreationUtilMac() = default;
+KeyCreationUtilMac::~KeyCreationUtilMac() = default;
 
-bool EncryptionKeyCreationUtilMac::KeyAlreadyCreated() {
-  return key_already_created_;
-}
-
-bool EncryptionKeyCreationUtilMac::ShouldPreventOverwriting() {
-  return base::FeatureList::IsEnabled(
-      os_crypt::features::kPreventEncryptionKeyOverwrites);
-}
-
-void EncryptionKeyCreationUtilMac::OnKeyWasStored() {
-  DCHECK(ShouldPreventOverwriting());
+void KeyCreationUtilMac::OnKeyWasStored() {
   if (key_already_created_)
     return;
   key_already_created_ = true;

components/os_crypt/encryption_key_creation_util_mac.h → components/os_crypt/key_creation_util_mac.h

@@ -2,12 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef COMPONENTS_OS_CRYPT_ENCRYPTION_KEY_CREATION_UTIL_MAC_H_
-#define COMPONENTS_OS_CRYPT_ENCRYPTION_KEY_CREATION_UTIL_MAC_H_
+#ifndef COMPONENTS_OS_CRYPT_KEY_CREATION_UTIL_MAC_H_
+#define COMPONENTS_OS_CRYPT_KEY_CREATION_UTIL_MAC_H_
 
-#include "base/component_export.h"
 #include "base/memory/scoped_refptr.h"
-#include "components/os_crypt/encryption_key_creation_util.h"
 
 class PrefService;
 
@@ -19,37 +17,31 @@ namespace os_crypt {
 
 // A utility class which provides a method to check whether the encryption key
 // should be available in the Keychain (meaning it was created in the past).
-class COMPONENT_EXPORT(OS_CRYPT) EncryptionKeyCreationUtilMac
-    : public EncryptionKeyCreationUtil {
+class KeyCreationUtilMac {
  public:
   // This class has to be initialized on the main UI thread since it uses
   // the local state.
-  EncryptionKeyCreationUtilMac(
+  KeyCreationUtilMac(
       PrefService* local_state,
       scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner);
-  ~EncryptionKeyCreationUtilMac() override;
+  ~KeyCreationUtilMac();
 
   // This method doesn't need to be called on the main thread.
-  bool KeyAlreadyCreated() override;
-
-  // This method doesn't need to be called on the main thread.
-  bool ShouldPreventOverwriting() override;
+  bool key_already_created() { return key_already_created_; }
 
   // This asynchronously updates the preference on the main thread that the key
   // was created. This method is called when key is added to the Keychain, or
   // the first time the key is successfully retrieved from the Keychain and the
   // preference hasn't been set yet. This method doesn't need to be called on
   // the main thread.
-  void OnKeyWasStored() override;
+  void OnKeyWasStored();
 
  private:
   PrefService* local_state_;
   scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;
   volatile bool key_already_created_;
-
-  DISALLOW_COPY_AND_ASSIGN(EncryptionKeyCreationUtilMac);
 };
 
 }  // namespace os_crypt
 
-#endif  // COMPONENTS_OS_CRYPT_ENCRYPTION_KEY_CREATION_UTIL_MAC_H_
+#endif  // COMPONENTS_OS_CRYPT_KEY_CREATION_UTIL_MAC_H_

components/os_crypt/keychain_password_mac.h

@@ -14,18 +14,11 @@ namespace crypto {
 class AppleKeychain;
 }
 
-namespace os_crypt {
-class EncryptionKeyCreationUtil;
-}
-
-using os_crypt::EncryptionKeyCreationUtil;
-
 class COMPONENT_EXPORT(OS_CRYPT) KeychainPassword {
  public:
-  KeychainPassword(
-      const crypto::AppleKeychain& keychain,
-      std::unique_ptr<EncryptionKeyCreationUtil> key_creation_util);
-  ~KeychainPassword();
+  explicit KeychainPassword(const crypto::AppleKeychain& keychain)
+      : keychain_(keychain) {
+  }
 
   // Get the OSCrypt password for this system.  If no password exists
   // in the Keychain then one is generated, stored in the Mac keychain, and
@@ -41,7 +34,6 @@ class COMPONENT_EXPORT(OS_CRYPT) KeychainPassword {
 
  private:
   const crypto::AppleKeychain& keychain_;
-  std::unique_ptr<EncryptionKeyCreationUtil> key_creation_util_;
 
   DISALLOW_COPY_AND_ASSIGN(KeychainPassword);
 };

components/os_crypt/keychain_password_mac.mm

@@ -9,7 +9,6 @@
 #include "base/base64.h"
 #include "base/mac/mac_logging.h"
 #include "base/rand_util.h"
-#include "components/os_crypt/encryption_key_creation_util.h"
 #include "crypto/apple_keychain.h"
 
 using crypto::AppleKeychain;
@@ -54,18 +53,7 @@ const char KeychainPassword::service_name[] = "Chromium Safe Storage";
 const char KeychainPassword::account_name[] = "Chromium";
 #endif
 
-KeychainPassword::KeychainPassword(
-    const AppleKeychain& keychain,
-    std::unique_ptr<EncryptionKeyCreationUtil> key_creation_util)
-    : keychain_(keychain), key_creation_util_(std::move(key_creation_util)) {}
-
-KeychainPassword::~KeychainPassword() = default;
-
 std::string KeychainPassword::GetPassword() const {
-  DCHECK(key_creation_util_);
-  bool prevent_overwriting_enabled =
-      key_creation_util_->ShouldPreventOverwriting();
-
   UInt32 password_length = 0;
   void* password_data = NULL;
   OSStatus error = keychain_.FindGenericPassword(
@@ -76,25 +64,11 @@ std::string KeychainPassword::GetPassword() const {
     std::string password =
         std::string(static_cast<char*>(password_data), password_length);
     keychain_.ItemFreeContent(password_data);
-    if (prevent_overwriting_enabled) {
-      key_creation_util_->OnKeyWasStored();
-    }
-    return password;
-  }
-
-  if (error == errSecItemNotFound) {
-    if (prevent_overwriting_enabled &&
-        key_creation_util_->KeyAlreadyCreated()) {
-      return std::string();
-    }
-    std::string password =
-        AddRandomPasswordToKeychain(keychain_, service_name, account_name);
-    if (prevent_overwriting_enabled && !password.empty()) {
-      key_creation_util_->OnKeyWasStored();
-    }
     return password;
+  } else if (error == errSecItemNotFound) {
+    return AddRandomPasswordToKeychain(keychain_, service_name, account_name);
+  } else {
+    OSSTATUS_DLOG(ERROR, error) << "Keychain lookup failed";
+    return std::string();
   }
-
-  OSSTATUS_DLOG(ERROR, error) << "Keychain lookup failed";
-  return std::string();
 }

components/os_crypt/os_crypt.h

@@ -19,11 +19,6 @@
 class KeyStorageLinux;
 #endif  // defined(OS_LINUX) && !defined(OS_CHROMEOS)
 
-#if defined(OS_MACOSX) && !defined(OS_IOS)
-class PrefRegistrySimple;
-class PrefService;
-#endif
-
 namespace os_crypt {
 struct Config;
 }
@@ -73,17 +68,6 @@ class OSCrypt {
       const std::string& ciphertext,
       std::string* plaintext);
 
-#if defined(OS_MACOSX) && !defined(OS_IOS)
-  // Registers preferences used by OSCrypt.
-  static COMPONENT_EXPORT(OS_CRYPT) void RegisterLocalPrefs(
-      PrefRegistrySimple* registry);
-
-  // Initialises OSCrypt.
-  // This method should be called on the main UI thread before any calls to
-  // encryption or decryption.
-  static COMPONENT_EXPORT(OS_CRYPT) void Init(PrefService* local_state);
-#endif
-
 #if defined(OS_MACOSX)
   // For unit testing purposes we instruct the Encryptor to use a mock Keychain
   // on the Mac. The default is to use the real Keychain. Use OSCryptMocker,

components/os_crypt/os_crypt_features_mac.cc

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "components/os_crypt/os_crypt_features_mac.h"
-
-namespace os_crypt {
-namespace features {
-
-// Prevents overwriting the encryption key in the Keychain on Mac if the key is
-// unavailable, but there is a preference set that the key was created in the
-// past.
-const base::Feature kPreventEncryptionKeyOverwrites = {
-    "PreventEncryptionKeyOverwrites", base::FEATURE_DISABLED_BY_DEFAULT};
-
-}  // namespace features
-}  // namespace os_crypt

components/os_crypt/os_crypt_features_mac.h

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef COMPONENTS_OS_CRYPT_OS_CRYPT_FEATURES_MAC_H_
-#define COMPONENTS_OS_CRYPT_OS_CRYPT_FEATURES_MAC_H_
-
-#include "base/component_export.h"
-#include "base/feature_list.h"
-
-namespace os_crypt {
-namespace features {
-
-COMPONENT_EXPORT(OS_CRYPT)
-extern const base::Feature kPreventEncryptionKeyOverwrites;
-
-}  // namespace features
-}  // namespace os_crypt
-
-#endif  // COMPONENTS_OS_CRYPT_OS_CRYPT_FEATURES_MAC_H_

components/os_crypt/os_crypt_mac.mm

@@ -13,7 +13,6 @@
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/lock.h"
-#include "build/build_config.h"
 #include "components/os_crypt/keychain_password_mac.h"
 #include "components/os_crypt/os_crypt_switches.h"
 #include "crypto/apple_keychain.h"
@@ -21,22 +20,8 @@
 #include "crypto/mock_apple_keychain.h"
 #include "crypto/symmetric_key.h"
 
-#if defined(OS_IOS)
-#include "components/os_crypt/encryption_key_creation_util_ios.h"
-#else
-#include "base/threading/thread_task_runner_handle.h"
-#include "components/os_crypt/encryption_key_creation_util_mac.h"
-#include "components/os_crypt/os_crypt_pref_names_mac.h"
-#include "components/prefs/pref_registry_simple.h"
-#include "components/prefs/pref_service.h"
-#endif
-
 using crypto::AppleKeychain;
 
-namespace os_crypt {
-class EncryptionKeyCreationUtil;
-}
-
 namespace {
 
 // Salt for Symmetric key derivation.
@@ -60,11 +45,6 @@ bool use_locked_mock_keychain = false;
 // this and migrate to different encryption without data loss.
 const char kEncryptionVersionPrefix[] = "v10";
 
-// A utility which prevents overwriting the encryption key. This is temporary
-// pointer that is non-NULL between initialization and getting the encryption
-// key for the first time.
-os_crypt::EncryptionKeyCreationUtil* g_key_creation_util = nullptr;
-
 // This lock is used to make the GetEncrytionKey and
 // OSCrypt::GetRawEncryptionKey methods thread-safe.
 base::LazyInstance<base::Lock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
@@ -96,17 +76,9 @@ crypto::SymmetricKey* GetEncryptionKey() {
     crypto::MockAppleKeychain keychain;
     password = keychain.GetEncryptionPassword();
   } else {
-#if defined(OS_IOS)
-    DCHECK(!g_key_creation_util);
-    g_key_creation_util = new os_crypt::EncryptionKeyCreationUtilIOS();
-#endif
-    DCHECK(g_key_creation_util);
     AppleKeychain keychain;
-    KeychainPassword encryptor_password(
-        keychain,
-        std::unique_ptr<EncryptionKeyCreationUtil>(g_key_creation_util));
+    KeychainPassword encryptor_password(keychain);
     password = encryptor_password.GetPassword();
-    g_key_creation_util = nullptr;
   }
 
   // Subsequent code must guarantee that the correct key is cached before
@@ -233,18 +205,6 @@ bool OSCrypt::IsEncryptionAvailable() {
   return GetEncryptionKey() != nullptr;
 }
 
-#if !defined(OS_IOS)
-void OSCrypt::RegisterLocalPrefs(PrefRegistrySimple* registry) {
-  registry->RegisterBooleanPref(os_crypt::prefs::kKeyCreated, false);
-}
-
-void OSCrypt::Init(PrefService* local_state) {
-  base::AutoLock auto_lock(g_lock.Get());
-  g_key_creation_util = new os_crypt::EncryptionKeyCreationUtilMac(
-      local_state, base::ThreadTaskRunnerHandle::Get());
-}
-#endif
-
 void OSCrypt::UseMockKeychainForTesting(bool use_mock) {
   use_mock_keychain = use_mock;
   if (!use_mock_keychain)

components/os_crypt/os_crypt_pref_names_mac.h

@@ -5,7 +5,7 @@
 #ifndef COMPONENTS_OS_CRYPT_OS_CRYPT_PREF_NAMES_MAC_H_
 #define COMPONENTS_OS_CRYPT_OS_CRYPT_PREF_NAMES_MAC_H_
 
-#include "base/component_export.h"
+#include "build/build_config.h"
 
 namespace os_crypt {
 namespace prefs {
@@ -20,7 +20,7 @@ namespace prefs {
 // encryption key is generated or successfully retrieved. If this flag is set to
 // true and Chrome couldn't get the encryption key from the Keychain, encryption
 // should be temporarily unavailable instead of generating a new key.
-COMPONENT_EXPORT(OS_CRYPT) extern const char kKeyCreated[];
+extern const char kKeyCreated[];
 
 }  // namespace prefs
 }  // namespace os_crypt