[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 <[email protected]>
Reviewed-by: Christos Froussios <[email protected]>
Reviewed-by: Dominic Battré <[email protected]>
Reviewed-by: Robert Sesek <[email protected]>
Reviewed-by: Vasilii Sukhanov <[email protected]>
Cr-Commit-Position: refs/heads/master@{#589898}

chrome/browser/chrome_browser_main_mac.mm

@@ -32,6 +32,7 @@
 #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"
@@ -160,6 +161,11 @@ void EnsureMetadataNeverIndexFile(const base::FilePath& user_data_dir) {
 }
 
 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,6 +290,7 @@
 #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)
@@ -486,6 +487,7 @@ 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,13 +32,17 @@ 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",
@@ -73,6 +77,10 @@ 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" ]
   }
@@ -163,11 +171,18 @@ 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

@@ -0,0 +1,40 @@
+// 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

@@ -0,0 +1,23 @@
+// 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

@@ -0,0 +1,37 @@
+// 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/key_creation_util_mac.cc → components/os_crypt/encryption_key_creation_util_mac.cc

@@ -2,25 +2,37 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/os_crypt/key_creation_util_mac.h"
+#include "components/os_crypt/encryption_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 {
 
-KeyCreationUtilMac::KeyCreationUtilMac(
+EncryptionKeyCreationUtilMac::EncryptionKeyCreationUtilMac(
     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)) {}
 
-KeyCreationUtilMac::~KeyCreationUtilMac() = default;
+EncryptionKeyCreationUtilMac::~EncryptionKeyCreationUtilMac() = default;
 
-void KeyCreationUtilMac::OnKeyWasStored() {
+bool EncryptionKeyCreationUtilMac::KeyAlreadyCreated() {
+  return key_already_created_;
+}
+
+bool EncryptionKeyCreationUtilMac::ShouldPreventOverwriting() {
+  return base::FeatureList::IsEnabled(
+      os_crypt::features::kPreventEncryptionKeyOverwrites);
+}
+
+void EncryptionKeyCreationUtilMac::OnKeyWasStored() {
+  DCHECK(ShouldPreventOverwriting());
   if (key_already_created_)
     return;
   key_already_created_ = true;

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

@@ -2,10 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef COMPONENTS_OS_CRYPT_KEY_CREATION_UTIL_MAC_H_
-#define COMPONENTS_OS_CRYPT_KEY_CREATION_UTIL_MAC_H_
+#ifndef COMPONENTS_OS_CRYPT_ENCRYPTION_KEY_CREATION_UTIL_MAC_H_
+#define COMPONENTS_OS_CRYPT_ENCRYPTION_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;
 
@@ -17,31 +19,37 @@ 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 KeyCreationUtilMac {
+class COMPONENT_EXPORT(OS_CRYPT) EncryptionKeyCreationUtilMac
+    : public EncryptionKeyCreationUtil {
  public:
   // This class has to be initialized on the main UI thread since it uses
   // the local state.
-  KeyCreationUtilMac(
+  EncryptionKeyCreationUtilMac(
       PrefService* local_state,
       scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner);
-  ~KeyCreationUtilMac();
+  ~EncryptionKeyCreationUtilMac() override;
 
   // This method doesn't need to be called on the main thread.
-  bool key_already_created() { return key_already_created_; }
+  bool KeyAlreadyCreated() override;
+
+  // This method doesn't need to be called on the main thread.
+  bool ShouldPreventOverwriting() override;
 
   // 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();
+  void OnKeyWasStored() override;
 
  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_KEY_CREATION_UTIL_MAC_H_
+#endif  // COMPONENTS_OS_CRYPT_ENCRYPTION_KEY_CREATION_UTIL_MAC_H_

components/os_crypt/keychain_password_mac.h

@@ -14,11 +14,18 @@ namespace crypto {
 class AppleKeychain;
 }
 
+namespace os_crypt {
+class EncryptionKeyCreationUtil;
+}
+
+using os_crypt::EncryptionKeyCreationUtil;
+
 class COMPONENT_EXPORT(OS_CRYPT) KeychainPassword {
  public:
-  explicit KeychainPassword(const crypto::AppleKeychain& keychain)
-      : keychain_(keychain) {
-  }
+  KeychainPassword(
+      const crypto::AppleKeychain& keychain,
+      std::unique_ptr<EncryptionKeyCreationUtil> key_creation_util);
+  ~KeychainPassword();
 
   // Get the OSCrypt password for this system.  If no password exists
   // in the Keychain then one is generated, stored in the Mac keychain, and
@@ -34,6 +41,7 @@ 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,6 +9,7 @@
 #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;
@@ -53,7 +54,18 @@
 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(
@@ -64,11 +76,25 @@
     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;
-  } else if (error == errSecItemNotFound) {
-    return AddRandomPasswordToKeychain(keychain_, service_name, account_name);
-  } else {
-    OSSTATUS_DLOG(ERROR, error) << "Keychain lookup failed";
-    return std::string();
   }
+
+  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;
+  }
+
+  OSSTATUS_DLOG(ERROR, error) << "Keychain lookup failed";
+  return std::string();
 }