AutoFill credit cards should be encrypted on the Mac

These changes add encryption support on Mac for the Encryptor class.  AES 128 bit is used for the encryption, and the auto-generated password is stored now in the Mac Keychain.  This implies the Encryptor class on Mac can now block for user input, and can fail if access is denied.

BUG=42038, 49131
TEST=EncryptorTest.CypherTextDiffers, EncryptorTest.DecryptError, EncryptorPasswordTest.*

Review URL: http://codereview.chromium.org/2943014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@52590 0039d316-1c4b-4281-b951-d872f2087c98

13 files changed, 571 insertions, 31 deletions

diff --git a/chrome/browser/autofill/personal_data_manager_unittest.cc b/chrome/browser/autofill/personal_data_manager_unittest.cc
index 513b087..b60ef41 100644
--- a/chrome/browser/autofill/personal_data_manager_unittest.cc
+++ b/chrome/browser/autofill/personal_data_manager_unittest.cc
@@ -12,6 +12,7 @@
 #include "chrome/browser/autofill/personal_data_manager.h"
 #include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/pref_service.h"
+#include "chrome/browser/password_manager/encryptor.h"
 #include "chrome/common/notification_details.h"
 #include "chrome/common/notification_observer_mock.h"
 #include "chrome/common/notification_registrar.h"
@@ -46,6 +47,10 @@ class PersonalDataManagerTest : public testing::Test {
   }
 
   virtual void SetUp() {
+#if defined(OS_MACOSX)
+    Encryptor::UseMockKeychain(true);
+#endif
+
     db_thread_.Start();
 
     profile_.reset(new TestingProfile);
diff --git a/chrome/browser/keychain_mac.cc b/chrome/browser/keychain_mac.cc
index 90d95a7..5640e6b 100644
--- a/chrome/browser/keychain_mac.cc
+++ b/chrome/browser/keychain_mac.cc
@@ -61,6 +61,47 @@ OSStatus MacKeychain::AddInternetPassword(
                                         itemRef);
 }
 
+OSStatus MacKeychain::FindGenericPassword(CFTypeRef keychainOrArray,
+                                          UInt32 serviceNameLength,
+                                          const char *serviceName,
+                                          UInt32 accountNameLength,
+                                          const char *accountName,
+                                          UInt32 *passwordLength,
+                                          void **passwordData,
+                                          SecKeychainItemRef *itemRef) const {
+  return SecKeychainFindGenericPassword(keychainOrArray,
+                                        serviceNameLength,
+                                        serviceName,
+                                        accountNameLength,
+                                        accountName,
+                                        passwordLength,
+                                        passwordData,
+                                        itemRef);
+}
+
+OSStatus MacKeychain::ItemFreeContent(SecKeychainAttributeList *attrList,
+                                      void *data) const {
+  return SecKeychainItemFreeContent(attrList, data);
+}
+
+OSStatus MacKeychain::AddGenericPassword(SecKeychainRef keychain,
+                                         UInt32 serviceNameLength,
+                                         const char *serviceName,
+                                         UInt32 accountNameLength,
+                                         const char *accountName,
+                                         UInt32 passwordLength,
+                                         const void *passwordData,
+                                         SecKeychainItemRef *itemRef) const {
+  return SecKeychainAddGenericPassword(keychain,
+                                       serviceNameLength,
+                                       serviceName,
+                                       accountNameLength,
+                                       accountName,
+                                       passwordLength,
+                                       passwordData,
+                                       itemRef);
+}
+
 void MacKeychain::Free(CFTypeRef ref) const {
   if (ref) {
     CFRelease(ref);
diff --git a/chrome/browser/keychain_mac.h b/chrome/browser/keychain_mac.h
index 68d7b30..1029343 100644
--- a/chrome/browser/keychain_mac.h
+++ b/chrome/browser/keychain_mac.h
@@ -58,6 +58,27 @@ class MacKeychain {
                                        const void *passwordData,
                                        SecKeychainItemRef *itemRef) const;
 
+  virtual OSStatus FindGenericPassword(CFTypeRef keychainOrArray,
+                                       UInt32 serviceNameLength,
+                                       const char *serviceName,
+                                       UInt32 accountNameLength,
+                                       const char *accountName,
+                                       UInt32 *passwordLength,
+                                       void **passwordData,
+                                       SecKeychainItemRef *itemRef) const;
+
+  virtual OSStatus ItemFreeContent(SecKeychainAttributeList *attrList,
+                                   void *data) const;
+
+  virtual OSStatus AddGenericPassword(SecKeychainRef keychain,
+                                      UInt32 serviceNameLength,
+                                      const char *serviceName,
+                                      UInt32 accountNameLength,
+                                      const char *accountName,
+                                      UInt32 passwordLength,
+                                      const void *passwordData,
+                                      SecKeychainItemRef *itemRef) const;
+
   // Calls CFRelease on the given ref, after checking that |ref| is non-NULL.
   virtual void Free(CFTypeRef ref) const;
 
diff --git a/chrome/browser/keychain_mock_mac.cc b/chrome/browser/keychain_mock_mac.cc
index 879ffdc..3114863 100644
--- a/chrome/browser/keychain_mock_mac.cc
+++ b/chrome/browser/keychain_mock_mac.cc
@@ -8,7 +8,9 @@
 
 MockKeychain::MockKeychain(unsigned int item_capacity)
     : item_capacity_(item_capacity), item_count_(0), search_copy_count_(0),
-      keychain_item_copy_count_(0), attribute_data_copy_count_(0) {
+      keychain_item_copy_count_(0), attribute_data_copy_count_(0),
+      find_generic_result_(noErr), called_add_generic_(false),
+      password_data_count_(0) {
   UInt32 tags[] = { kSecAccountItemAttr,
                     kSecServerItemAttr,
                     kSecPortItemAttr,
@@ -334,6 +336,54 @@ OSStatus MockKeychain::SearchCopyNext(SecKeychainSearchRef searchRef,
   return noErr;
 }
 
+OSStatus MockKeychain::FindGenericPassword(CFTypeRef keychainOrArray,
+                                           UInt32 serviceNameLength,
+                                           const char *serviceName,
+                                           UInt32 accountNameLength,
+                                           const char *accountName,
+                                           UInt32 *passwordLength,
+                                           void **passwordData,
+                                           SecKeychainItemRef *itemRef) const {
+  // When simulating |noErr| we return canned |passwordData| and
+  // |passwordLenght|.  Otherwise, just return given code.
+  if (find_generic_result_ == noErr) {
+    static char password[] = "my_password";
+
+    DCHECK(passwordData);
+    *passwordData = static_cast<void*>(password);
+    DCHECK(passwordLength);
+    *passwordLength = strlen(password);
+    password_data_count_++;
+  }
+
+  return find_generic_result_;
+}
+
+OSStatus MockKeychain::ItemFreeContent(SecKeychainAttributeList *attrList,
+                                       void *data) const {
+  // No-op.
+  password_data_count_--;
+  return noErr;
+}
+
+OSStatus MockKeychain::AddGenericPassword(SecKeychainRef keychain,
+                                          UInt32 serviceNameLength,
+                                          const char *serviceName,
+                                          UInt32 accountNameLength,
+                                          const char *accountName,
+                                          UInt32 passwordLength,
+                                          const void *passwordData,
+                                          SecKeychainItemRef *itemRef) const {
+  called_add_generic_ = true;
+
+  DCHECK(passwordLength > 0);
+  DCHECK(passwordData);
+  add_generic_password_ =
+      std::string(const_cast<char*>(static_cast<const char*>(passwordData)),
+                  passwordLength);
+  return noErr;
+}
+
 void MockKeychain::Free(CFTypeRef ref) const {
   if (!ref) {
     return;
diff --git a/chrome/browser/keychain_mock_mac.h b/chrome/browser/keychain_mock_mac.h
index ba7a48eb..666395e 100644
--- a/chrome/browser/keychain_mock_mac.h
+++ b/chrome/browser/keychain_mock_mac.h
@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_KEYCHAIN_MOCK_MAC_H_
 
 #include <set>
+#include <string>
 #include <vector>
 
 #include "chrome/browser/keychain_mac.h"
@@ -56,6 +57,24 @@ class MockKeychain : public MacKeychain {
                                        UInt32 passwordLength,
                                        const void *passwordData,
                                        SecKeychainItemRef *itemRef) const;
+  virtual OSStatus FindGenericPassword(CFTypeRef keychainOrArray,
+                                       UInt32 serviceNameLength,
+                                       const char *serviceName,
+                                       UInt32 accountNameLength,
+                                       const char *accountName,
+                                       UInt32 *passwordLength,
+                                       void **passwordData,
+                                       SecKeychainItemRef *itemRef) const;
+  virtual OSStatus ItemFreeContent(SecKeychainAttributeList *attrList,
+                                   void *data) const;
+  virtual OSStatus AddGenericPassword(SecKeychainRef keychain,
+                                      UInt32 serviceNameLength,
+                                      const char *serviceName,
+                                      UInt32 accountNameLength,
+                                      const char *accountName,
+                                      UInt32 passwordLength,
+                                      const void *passwordData,
+                                      SecKeychainItemRef *itemRef) const;
   virtual void Free(CFTypeRef ref) const;
 
   // Return the counts of objects returned by Create/Copy functions but never
@@ -83,6 +102,25 @@ class MockKeychain : public MacKeychain {
   // Adds a keychain item with the given info to the test set.
   void AddTestItem(const KeychainTestData& item_data);
 
+  // |FindGenericPassword()| can return different results depending on user
+  // interaction with the system Keychain.  For mocking purposes we allow the
+  // user of this class to specify the result code of the
+  // |FindGenericPassword()| call so we can simulate the result of different
+  // user interactions.
+  void set_find_generic_result(OSStatus result) {
+    find_generic_result_ = result;
+  }
+
+  // Returns the true if |AddGenericPassword()| was called.
+  bool called_add_generic() const { return called_add_generic_; }
+
+  // Returns the value of the password set when |AddGenericPassword()| was
+  // called.
+  std::string add_generic_password() const { return add_generic_password_; }
+
+  // Returns the number of allocations - deallocations for password data.
+  int password_data_count() const { return password_data_count_; }
+
  private:
   // Sets the data and length of |tag| in the item-th test item.
   void SetTestDataBytes(int item, UInt32 tag, const void* data, size_t length);
@@ -136,6 +174,19 @@ class MockKeychain : public MacKeychain {
 
   // Tracks which items (by index) were added with AddInternetPassword.
   mutable std::set<unsigned int> added_via_api_;
+
+  // Result code for the |FindGenericPassword()| method.
+  OSStatus find_generic_result_;
+
+  // Records whether |AddGenericPassword()| gets called.
+  mutable bool called_add_generic_;
+
+  // Tracks the allocations and frees of password data in |FindGenericPassword|
+  // and |ItemFreeContent|.
+  mutable unsigned int password_data_count_;
+
+  // Records the password being set when |AddGenericPassword()| gets called.
+  mutable std::string add_generic_password_;
 };
 
 #endif  // CHROME_BROWSER_KEYCHAIN_MOCK_MAC_H_
diff --git a/chrome/browser/password_manager/encryptor.h b/chrome/browser/password_manager/encryptor.h
index 8170892..480477c 100644
--- a/chrome/browser/password_manager/encryptor.h
+++ b/chrome/browser/password_manager/encryptor.h
@@ -1,7 +1,6 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
-// A class for encrypting/decrypting strings
 
 #ifndef CHROME_BROWSER_PASSWORD_MANAGER_ENCRYPTOR_H__
 #define CHROME_BROWSER_PASSWORD_MANAGER_ENCRYPTOR_H__
@@ -11,6 +10,9 @@
 #include "base/values.h"
 #include "base/string16.h"
 
+// The Encryptor class gives access to simple encryption and decryption of
+// strings.  Note that on Mac, access to the system Keychain is required and
+// these calls can block the current thread to collect user input.
 class Encryptor {
  public:
   // Encrypt a string16. The output (second argument) is
@@ -37,6 +39,12 @@ class Encryptor {
   static bool DecryptString(const std::string& ciphertext,
                             std::string* plaintext);
 
+#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.
+  static void UseMockKeychain(bool use_mock);
+#endif
+
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(Encryptor);
 };
diff --git a/chrome/browser/password_manager/encryptor_mac.mm b/chrome/browser/password_manager/encryptor_mac.mm
index 44ed207..0e7ce2f 100644
--- a/chrome/browser/password_manager/encryptor_mac.mm
+++ b/chrome/browser/password_manager/encryptor_mac.mm
@@ -1,11 +1,71 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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 "chrome/browser/password_manager/encryptor.h"
 
+#include <CommonCrypto/CommonCryptor.h>  // for kCCBlockSizeAES128
+
+#include "base/crypto/encryptor.h"
+#include "base/crypto/symmetric_key.h"
 #include "base/logging.h"
+#include "base/scoped_ptr.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/password_manager/encryptor_password_mac.h"
+#include "chrome/browser/keychain_mac.h"
+
+namespace {
+
+// Salt for Symmetric key derivation.
+const char kSalt[] = "saltysalt";
+
+// Key size required for 128 bit AES.
+const size_t kDerivedKeySizeInBits = 128;
+
+// Constant for Symmetic key derivation.
+const size_t kEncryptionIterations = 1003;
+
+// TODO(dhollowa): Refactor to allow dependency injection of Keychain.
+static bool use_mock_keychain = false;
+
+// Prefix for cypher text returned by current encryption version.  We prefix
+// the cypher text with this string so that future data migration can detect
+// this and migrate to different encryption without data loss.
+const char kEncryptionVersionPrefix[] = "v10";
+
+// Generates a newly allocated SymmetricKey object based on the password found
+// in the Keychain.  The generated key is for AES encryption.  Ownership of the
+// key is passed to the caller.  Returns NULL key in the case password access
+// is denied or key generation error occurs.
+base::SymmetricKey* GetEncryptionKey() {
+
+  std::string password;
+  if (use_mock_keychain) {
+    password = "mock_password";
+  } else {
+    MacKeychain keychain;
+    EncryptorPassword encryptor_password(keychain);
+    password = encryptor_password.GetEncryptorPassword();
+  }
+
+  if (password.empty())
+    return NULL;
+
+  std::string salt(kSalt);
+
+  // Create an encryption key from our password and salt.
+  scoped_ptr<base::SymmetricKey> encryption_key(
+      base::SymmetricKey::DeriveKeyFromPassword(base::SymmetricKey::AES,
+                                                password,
+                                                salt,
+                                                kEncryptionIterations,
+                                                kDerivedKeySizeInBits));
+  DCHECK(encryption_key.get());
+
+  return encryption_key.release();
+}
+
+}  // namespace
 
 bool Encryptor::EncryptString16(const string16& plaintext,
                                 std::string* ciphertext) {
@@ -24,19 +84,65 @@ bool Encryptor::DecryptString16(const std::string& ciphertext,
 
 bool Encryptor::EncryptString(const std::string& plaintext,
                               std::string* ciphertext) {
-  // This doesn't actually encrypt, we need to work on the Encryptor API.
-  // http://code.google.com/p/chromium/issues/detail?id=25404
+  if (plaintext.empty()) {
+    *ciphertext = std::string();
+    return true;
+  }
+
+  scoped_ptr<base::SymmetricKey> encryption_key(GetEncryptionKey());
+  if (!encryption_key.get())
+    return false;
+
+  std::string iv(kCCBlockSizeAES128, ' ');
+  base::Encryptor encryptor;
+  if (!encryptor.Init(encryption_key.get(), base::Encryptor::CBC, iv))
+    return false;
+
+  if (!encryptor.Encrypt(plaintext, ciphertext))
+    return false;
 
-  // this does a copy
-  ciphertext->assign(plaintext.data(), plaintext.length());
+  // Prefix the cypher text with version information.
+  ciphertext->insert(0, kEncryptionVersionPrefix);
   return true;
 }
 
 bool Encryptor::DecryptString(const std::string& ciphertext,
                               std::string* plaintext) {
-  // This doesn't actually decrypt, we need to work on the Encryptor API.
-  // http://code.google.com/p/chromium/issues/detail?id=25404
+  if (ciphertext.empty()) {
+    *plaintext = std::string();
+    return true;
+  }
+
+  // Check that the incoming cyphertext was indeed encrypted with the expected
+  // version.  If the prefix is not found then we'll assume we're dealing with
+  // old data saved as clear text and we'll return it directly.
+  // Credit card numbers are current legacy data, so false match with prefix
+  // won't happen.
+  if (ciphertext.find(kEncryptionVersionPrefix) != 0) {
+    *plaintext = ciphertext;
+    return true;
+  }
+
+  // Strip off the versioning prefix before decrypting.
+  std::string raw_ciphertext =
+      ciphertext.substr(strlen(kEncryptionVersionPrefix));
+
+  scoped_ptr<base::SymmetricKey> encryption_key(GetEncryptionKey());
+  if (!encryption_key.get())
+    return false;
+
+  std::string iv(kCCBlockSizeAES128, ' ');
+  base::Encryptor encryptor;
+  if (!encryptor.Init(encryption_key.get(), base::Encryptor::CBC, iv))
+    return false;
+
+  if (!encryptor.Decrypt(raw_ciphertext, plaintext))
+    return false;
 
-  plaintext->assign(ciphertext.data(), ciphertext.length());
   return true;
 }
+
+void Encryptor::UseMockKeychain(bool use_mock) {
+  use_mock_keychain = use_mock;
+}
+
diff --git a/chrome/browser/password_manager/encryptor_password_mac.h b/chrome/browser/password_manager/encryptor_password_mac.h
new file mode 100644
index 0000000..73f8702
--- /dev/null
+++ b/chrome/browser/password_manager/encryptor_password_mac.h
@@ -0,0 +1,33 @@
+// Copyright (c) 2010 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 CHROME_BROWSER_PASSWORD_MANAGER_ENCRYPTOR_PASSWORD_H__
+#define CHROME_BROWSER_PASSWORD_MANAGER_ENCRYPTOR_PASSWORD_H__
+
+#include <string>
+
+#include "base/basictypes.h"
+
+class MacKeychain;
+
+class EncryptorPassword {
+ public:
+  explicit EncryptorPassword(const MacKeychain& keychain)
+      : keychain_(keychain) {
+  }
+
+  // Get the Encryptor password for this system.  If no password exists
+  // in the Keychain then one is generated, stored in the Mac keychain, and
+  // returned.
+  // If one exists then it is fetched from the Keychain and returned.
+  // If the user disallows access to the Keychain (or an error occurs) then an
+  // empty string is returned.
+  std::string GetEncryptorPassword() const;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(EncryptorPassword);
+  const MacKeychain& keychain_;
+};
+
+#endif  // CHROME_BROWSER_PASSWORD_MANAGER_ENCRYPTOR_PASSWORD_H__
diff --git a/chrome/browser/password_manager/encryptor_password_mac.mm b/chrome/browser/password_manager/encryptor_password_mac.mm
new file mode 100644
index 0000000..a92fdcd
--- /dev/null
+++ b/chrome/browser/password_manager/encryptor_password_mac.mm
@@ -0,0 +1,76 @@
+// Copyright (c) 2010 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 "chrome/browser/password_manager/encryptor_password_mac.h"
+
+#import <Security/Security.h>
+
+#include "app/l10n_util.h"
+#include "chrome/browser/keychain_mac.h"
+#include "chrome/browser/sync/util/crypto_helpers.h"
+#include "grit/generated_resources.h"
+
+namespace {
+
+// Generates a random password and adds it to the Keychain.  The added password
+// is returned from the function.  If an error occurs, an empty password is
+// returned.
+std::string AddRandomPasswordToKeychain(const MacKeychain& keychain,
+                                        const std::string& service_name,
+                                        const std::string& account_name) {
+  std::string password = Generate128BitRandomHexString();
+  void* password_data =
+      const_cast<void*>(static_cast<const void*>(password.data()));
+
+  OSStatus error = keychain.AddGenericPassword(NULL,
+                                               service_name.size(),
+                                               service_name.data(),
+                                               account_name.size(),
+                                               account_name.data(),
+                                               password.size(),
+                                               password_data,
+                                               NULL);
+
+  if (error != noErr) {
+    DLOG(ERROR) << "Keychain add failed with error " << error;
+    return std::string();
+  }
+
+  return password;
+}
+
+}  // namespace
+
+std::string EncryptorPassword::GetEncryptorPassword() const {
+  // These two strings ARE indeed user facing.  But they are used to access
+  // the encryption keyword.  So as to not lose encrypted data when system
+  // locale changes we DO NOT LOCALIZE.
+  const std::string service_name = "Chrome Safe Storage";
+  const std::string account_name = "Chrome";
+
+  UInt32 password_length = 0;
+  void* password_data = NULL;
+  OSStatus error = keychain_.FindGenericPassword(NULL,
+                                                 service_name.size(),
+                                                 service_name.data(),
+                                                 account_name.size(),
+                                                 account_name.data(),
+                                                 &password_length,
+                                                 &password_data,
+                                                 NULL);
+
+  if (error == noErr) {
+    std::string password =
+        std::string(static_cast<char*>(password_data), password_length);
+    keychain_.ItemFreeContent(NULL, password_data);
+    return password;
+  } else if (error == errSecItemNotFound) {
+    return AddRandomPasswordToKeychain(keychain_, service_name, account_name);
+  } else {
+    DLOG(ERROR) << "Keychain lookup failed with error " << error;
+    return std::string();
+  }
+}
+
+
diff --git a/chrome/browser/password_manager/encryptor_password_mac_unittest.cc b/chrome/browser/password_manager/encryptor_password_mac_unittest.cc
new file mode 100644
index 0000000..b17af55
--- /dev/null
+++ b/chrome/browser/password_manager/encryptor_password_mac_unittest.cc
@@ -0,0 +1,77 @@
+// Copyright (c) 2010 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 "chrome/browser/keychain_mock_mac.h"
+#include "chrome/browser/password_manager/encryptor_password_mac.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+// Test that if we have an existing password in the Keychain and we are
+// authorized by the user to read it then we get it back correctly.
+TEST(EncryptorPasswordTest, FindPasswordSuccess) {
+  MockKeychain keychain(0);
+  keychain.set_find_generic_result(noErr);
+  EncryptorPassword password(keychain);
+  EXPECT_FALSE(password.GetEncryptorPassword().empty());
+  EXPECT_FALSE(keychain.called_add_generic());
+  EXPECT_EQ(0, keychain.password_data_count());
+}
+
+// Test that if we do not have an existing password in the Keychain then it
+// gets added successfully and returned.
+TEST(EncryptorPasswordTest, FindPasswordNotFound) {
+  MockKeychain keychain(0);
+  keychain.set_find_generic_result(errSecItemNotFound);
+  EncryptorPassword password(keychain);
+  EXPECT_FALSE(password.GetEncryptorPassword().empty());
+  EXPECT_TRUE(keychain.called_add_generic());
+  EXPECT_EQ(0, keychain.password_data_count());
+}
+
+// Test that if get denied access by the user then we return an empty password.
+// And we should not try to add one.
+TEST(EncryptorPasswordTest, FindPasswordNotAuthorized) {
+  MockKeychain keychain(0);
+  keychain.set_find_generic_result(errSecAuthFailed);
+  EncryptorPassword password(keychain);
+  EXPECT_TRUE(password.GetEncryptorPassword().empty());
+  EXPECT_FALSE(keychain.called_add_generic());
+  EXPECT_EQ(0, keychain.password_data_count());
+}
+
+// Test that if some random other error happens then we return an empty
+// password, and we should not try to add one.
+TEST(EncryptorPasswordTest, FindPasswordOtherError) {
+  MockKeychain keychain(0);
+  keychain.set_find_generic_result(errSecNotAvailable);
+  EncryptorPassword password(keychain);
+  EXPECT_TRUE(password.GetEncryptorPassword().empty());
+  EXPECT_FALSE(keychain.called_add_generic());
+  EXPECT_EQ(0, keychain.password_data_count());
+}
+
+// Test that subsequent additions to the keychain give different passwords.
+TEST(EncryptorPasswordTest, PasswordsDiffer) {
+  MockKeychain keychain1(0);
+  keychain1.set_find_generic_result(errSecItemNotFound);
+  EncryptorPassword encryptor_password1(keychain1);
+  std::string password1 = encryptor_password1.GetEncryptorPassword();
+  EXPECT_FALSE(password1.empty());
+  EXPECT_TRUE(keychain1.called_add_generic());
+  EXPECT_EQ(0, keychain1.password_data_count());
+
+  MockKeychain keychain2(0);
+  keychain2.set_find_generic_result(errSecItemNotFound);
+  EncryptorPassword encryptor_password2(keychain2);
+  std::string password2 = encryptor_password2.GetEncryptorPassword();
+  EXPECT_FALSE(password2.empty());
+  EXPECT_TRUE(keychain2.called_add_generic());
+  EXPECT_EQ(0, keychain2.password_data_count());
+
+  // And finally check that the passwords are different.
+  EXPECT_NE(password1, password2);
+}
+
+}  // namespace
diff --git a/chrome/browser/password_manager/encryptor_unittest.cc b/chrome/browser/password_manager/encryptor_unittest.cc
index 29e518d8..8ca176f 100644
--- a/chrome/browser/password_manager/encryptor_unittest.cc
+++ b/chrome/browser/password_manager/encryptor_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -12,25 +12,46 @@
 
 namespace {
 
-TEST(EncryptorTest, String16EncryptionDecryption) {
+class EncryptorTest : public testing::Test {
+ public:
+  EncryptorTest() {}
+
+  virtual void SetUp() {
+#if defined(OS_MACOSX)
+    Encryptor::UseMockKeychain(true);
+#endif
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(EncryptorTest);
+};
+
+TEST_F(EncryptorTest, String16EncryptionDecryption) {
   string16 plaintext;
   string16 result;
   std::string utf8_plaintext;
   std::string utf8_result;
   std::string ciphertext;
 
-  // test borderline cases (empty strings)
+  // Test borderline cases (empty strings).
   EXPECT_TRUE(Encryptor::EncryptString16(plaintext, &ciphertext));
   EXPECT_TRUE(Encryptor::DecryptString16(ciphertext, &result));
   EXPECT_EQ(plaintext, result);
 
-  // test a simple string
+  // Test a simple string.
   plaintext = ASCIIToUTF16("hello");
   EXPECT_TRUE(Encryptor::EncryptString16(plaintext, &ciphertext));
   EXPECT_TRUE(Encryptor::DecryptString16(ciphertext, &result));
   EXPECT_EQ(plaintext, result);
 
-  // test unicode
+  // Test a 16-byte aligned string.  This previously hit a boundary error in
+  // base::Encryptor::Crypt() on Mac.
+  plaintext = ASCIIToUTF16("1234567890123456");
+  EXPECT_TRUE(Encryptor::EncryptString16(plaintext, &ciphertext));
+  EXPECT_TRUE(Encryptor::DecryptString16(ciphertext, &result));
+  EXPECT_EQ(plaintext, result);
+
+  // Test Unicode.
   char16 wchars[] = { 0xdbeb, 0xdf1b, 0x4e03, 0x6708, 0x8849,
                       0x661f, 0x671f, 0x56db, 0x597c, 0x4e03,
                       0x6708, 0x56db, 0x6708, 0xe407, 0xdbaf,
@@ -53,27 +74,74 @@ TEST(EncryptorTest, String16EncryptionDecryption) {
   EXPECT_EQ(utf8_plaintext, UTF16ToUTF8(result));
 }
 
-TEST(EncryptorTest, EncryptionDecryption) {
+TEST_F(EncryptorTest, EncryptionDecryption) {
   std::string plaintext;
   std::string result;
   std::string ciphertext;
 
-  // test borderline cases (empty strings)
-  EXPECT_TRUE(Encryptor::EncryptString(plaintext, &ciphertext));
-  EXPECT_TRUE(Encryptor::DecryptString(ciphertext, &result));
+  // Test borderline cases (empty strings).
+  ASSERT_TRUE(Encryptor::EncryptString(plaintext, &ciphertext));
+  ASSERT_TRUE(Encryptor::DecryptString(ciphertext, &result));
   EXPECT_EQ(plaintext, result);
 
-  // test a simple string
+  // Test a simple string.
   plaintext = "hello";
-  EXPECT_TRUE(Encryptor::EncryptString(plaintext, &ciphertext));
-  EXPECT_TRUE(Encryptor::DecryptString(ciphertext, &result));
+  ASSERT_TRUE(Encryptor::EncryptString(plaintext, &ciphertext));
+  ASSERT_TRUE(Encryptor::DecryptString(ciphertext, &result));
   EXPECT_EQ(plaintext, result);
 
   // Make sure it null terminates.
   plaintext.assign("hello", 3);
-  EXPECT_TRUE(Encryptor::EncryptString(plaintext, &ciphertext));
-  EXPECT_TRUE(Encryptor::DecryptString(ciphertext, &result));
+  ASSERT_TRUE(Encryptor::EncryptString(plaintext, &ciphertext));
+  ASSERT_TRUE(Encryptor::DecryptString(ciphertext, &result));
   EXPECT_EQ(plaintext, "hel");
 }
 
+// Encryption currently only enabled on Mac and Windows.
+// TODO(dhollowa): http://crbug.com/49115 Implement secure store on Linux.
+#if defined(OS_WIN) || defined(OS_MACOSX)
+TEST_F(EncryptorTest, CypherTextDiffers) {
+  std::string plaintext;
+  std::string result;
+  std::string ciphertext;
+
+  // Test borderline cases (empty strings).
+  ASSERT_TRUE(Encryptor::EncryptString(plaintext, &ciphertext));
+  ASSERT_TRUE(Encryptor::DecryptString(ciphertext, &result));
+  // |cyphertext| is empty on the Mac, different on Windows.
+  EXPECT_TRUE(ciphertext.empty() || plaintext != ciphertext);
+  EXPECT_EQ(plaintext, result);
+
+  // Test a simple string.
+  plaintext = "hello";
+  ASSERT_TRUE(Encryptor::EncryptString(plaintext, &ciphertext));
+  ASSERT_TRUE(Encryptor::DecryptString(ciphertext, &result));
+  EXPECT_NE(plaintext, ciphertext);
+  EXPECT_EQ(plaintext, result);
+
+  // Make sure it null terminates.
+  plaintext.assign("hello", 3);
+  ASSERT_TRUE(Encryptor::EncryptString(plaintext, &ciphertext));
+  ASSERT_TRUE(Encryptor::DecryptString(ciphertext, &result));
+  EXPECT_NE(plaintext, ciphertext);
+  EXPECT_EQ(result, "hel");
+}
+
+TEST_F(EncryptorTest, DecryptError) {
+  std::string plaintext;
+  std::string result;
+  std::string ciphertext;
+
+  // Test a simple string, messing with ciphertext prior to decrypting.
+  plaintext = "hello";
+  ASSERT_TRUE(Encryptor::EncryptString(plaintext, &ciphertext));
+  EXPECT_NE(plaintext, ciphertext);
+  ASSERT_LT(4UL, ciphertext.size());
+  ciphertext[3] = ciphertext[0] + 1;
+  EXPECT_FALSE(Encryptor::DecryptString(ciphertext, &result));
+  EXPECT_NE(plaintext, result);
+  EXPECT_TRUE(result.empty());
+}
+#endif
+
 }  // namespace
diff --git a/chrome/browser/webdata/web_database.cc b/chrome/browser/webdata/web_database.cc
index 5bc985f..48a3d40 100644
--- a/chrome/browser/webdata/web_database.cc
+++ b/chrome/browser/webdata/web_database.cc
@@ -24,12 +24,12 @@
 #include "gfx/codec/png_codec.h"
 #include "webkit/glue/password_form.h"
 
-// Encryptor is the *wrong* way of doing things; we need to turn it into a
-// bottleneck to use the platform methods (e.g. Keychain on the Mac, Gnome
-// Keyring / KWallet on Linux). That's going to take a massive change in its
-// API... see:
-//   http://code.google.com/p/chromium/issues/detail?id=25404 (Linux)
-// but the (possibly-now-unused) Mac encryptor stub code needs to die too.
+// Encryptor is now in place for Windows and Mac.  The Linux implementation
+// currently obfuscates only.  Mac Encryptor implementation can block the
+// active thread while presenting UI to the user.  See |encryptor_mac.mm| for
+// details.
+// For details on the Linux work see:
+//   http://crbug.com/25404
 #include "chrome/browser/password_manager/encryptor.h"
 
 using webkit_glue::FormField;
diff --git a/chrome/browser/webdata/web_database_unittest.cc b/chrome/browser/webdata/web_database_unittest.cc
index 6464f39..ce13dbf 100644
--- a/chrome/browser/webdata/web_database_unittest.cc
+++ b/chrome/browser/webdata/web_database_unittest.cc
@@ -20,6 +20,7 @@
 #include "chrome/browser/autofill/autofill_type.h"
 #include "chrome/browser/autofill/credit_card.h"
 #include "chrome/browser/search_engines/template_url.h"
+#include "chrome/browser/password_manager/encryptor.h"
 #include "chrome/browser/webdata/autofill_change.h"
 #include "chrome/browser/webdata/autofill_entry.h"
 #include "chrome/browser/webdata/web_database.h"
@@ -94,6 +95,9 @@ class WebDatabaseTest : public testing::Test {
   typedef std::set<AutofillEntry, bool (*)(const AutofillEntry&,
     const AutofillEntry&)>::iterator AutofillEntrySetIterator;
   virtual void SetUp() {
+#if defined(OS_MACOSX)
+    Encryptor::UseMockKeychain(true);
+#endif
     PathService::Get(chrome::DIR_TEST_DATA, &file_);
     const std::string test_db = "TestWebDatabase" +
         Int64ToString(base::Time::Now().ToInternalValue()) +