OSCrypt for POSIX uses libsecret to store a randomised encryption key.

components/os_crypt/os_crypt_linux.cc

-// Copyright 2014 The Chromium Authors. All rights reserved.
+// Copyright 2016 The Chromium Authors. All rights reserved.

[vabr (Chromium), 2016/05/13 15:10:18]

Looking at the code in this file, I see two main issues:

(1) The use of static variables with non-trivial destructor. Ideally we should
not use them at all. Alternatively, have a look at LazyInstance [1] for how to
create objects as static variables. If you use that, it might make sense to
restructure the code so that you end up with one LazyInstance grouping all of
the static data.

(2) The mix of test utilities and production code, in particular the mock key
storage. Ideally, all testing support would be in a separate file, part of a
separate build target marked as testonly=true in GN. The production code should
have a single place where to inject an alternative KeyStorage instance, which
would be done in tests (injecting the test version), and the production code
should be left unaware of the test stuff.

I know that the current code does not satisfy the above, but if we can, let's
repair as much as possible during the transition.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/lazy_instance...

[cfroussios, 2016/05/13 17:09:12]

(1) The source of the problem is that OSCrypt is a static class that ended up
needing state. At the very least, it needs a reference to the selected
KeyStorage, or else we will have to check for availability every time.

 // 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.h"
 
 #include <stddef.h>
 
+#include <functional>
 #include <memory>
 
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
+#include "components/os_crypt/key_storage_linux.h"
 #include "crypto/encryptor.h"
 #include "crypto/symmetric_key.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 = 1;
 
 // Size of initialization vector for AES 128-bit.
 const size_t kIVBlockSizeAES128 = 16;
 
+// Used for array indexing
+enum Version {
+  V10 = 0,
+  V11 = 1,
+};
+
 // Prefix for cypher text returned by obfuscation version.  We prefix the
 // cyphertext with this string so that future data migration can detect
 // this and migrate to full encryption without data loss.
-const char kObfuscationPrefix[] = "v10";
+const char* kObfuscationPrefix[] = {
+    "v10", "v11",
+};
+
+// Use a mocked key storage and don't try to contact a real service.
+bool use_mock = false;

[vabr (Chromium), 2016/05/13 15:10:19]

nit: Please prefix all global variables with g_.

https://google.github.io/styleguide/cppguide.html#Variable_Names

[cfroussios, 2016/05/13 17:09:12]

Done.

+
+std::unique_ptr<KeyStorageMock> mock;

[vabr (Chromium), 2016/05/13 15:10:19]

Only plain old data can be a static variable. unique_ptr has a non-trivial
destructor and is banned for static variables.

https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

[cfroussios, 2016/05/13 17:09:12]

Acknowledged.

+
+// Lazy acquisition of a KeyStorage. Will be null if no service is found.
+// Will return a mock if |use_mock| is true.
+KeyStorage* GetKeyStorage() {
+  if (use_mock) {
+    return mock.get();
+  }
+  static std::unique_ptr<KeyStorage> key_storage(KeyStorage::FindService());

[vabr (Chromium), 2016/05/13 15:10:18]

Also here, cannot use unique_ptr, because it has a non-trivial destructor.

[cfroussios, 2016/05/13 17:09:12]

Acknowledged.

+  return key_storage.get();
+}
+
+// Returns a cached instance of peanuts
+std::string* GetPasswordV10() {
+  static std::unique_ptr<std::string> password(new std::string("peanuts"));

[vabr (Chromium), 2016/05/13 15:10:19]

Also here.

[cfroussios, 2016/05/13 17:09:12]

Acknowledged.

+  return password.get();
+}
+
+// Caches and returns the password from the KeyStorage or null if there is no
+// service.
+// Caching is disabled when mocking.
+std::string* GetPasswordV11() {
+  if (use_mock)
+    return new std::string(GetKeyStorage()->GetKey());
+
+  static std::unique_ptr<std::string> password(
+      GetKeyStorage() ? new std::string(GetKeyStorage()->GetKey()) : nullptr);
+  return password.get();
+}
+
+std::function<std::string*()> get_password[] = {

[vabr (Chromium), 2016/05/13 15:10:19]

std::function is not yet allowed.
http://chromium-cpp.appspot.com/

[cfroussios, 2016/05/13 17:09:12]

Acknowledged.

+    &GetPasswordV10, &GetPasswordV11,
+};
 
 // Generates a newly allocated SymmetricKey object based a hard-coded password.
 // Ownership of the key is passed to the caller.  Returns NULL key if a key
 // generation error occurs.
-crypto::SymmetricKey* GetEncryptionKey() {
+std::unique_ptr<crypto::SymmetricKey> GetEncryptionKey(Version version) {
   // We currently "obfuscate" by encrypting and decrypting with hard-coded
   // password.  We need to improve this password situation by moving a secure
   // password into a system-level key store.
   // http://crbug.com/25404 and http://crbug.com/49115
-  std::string password = "peanuts";
   std::string salt(kSalt);
 
+  std::string* password = get_password[version]();
+  if (password == nullptr)
+    return nullptr;
+
   // Create an encryption key from our password and salt.
   std::unique_ptr<crypto::SymmetricKey> encryption_key(
-      crypto::SymmetricKey::DeriveKeyFromPassword(crypto::SymmetricKey::AES,
-                                                  password,
-                                                  salt,
-                                                  kEncryptionIterations,
-                                                  kDerivedKeySizeInBits));
+      crypto::SymmetricKey::DeriveKeyFromPassword(
+          crypto::SymmetricKey::AES, *password, salt, kEncryptionIterations,
+          kDerivedKeySizeInBits));
   DCHECK(encryption_key.get());
 
-  return encryption_key.release();
+  return encryption_key;
 }
 
 }  // namespace
 
+// static
 bool OSCrypt::EncryptString16(const base::string16& plaintext,
                               std::string* ciphertext) {
   return EncryptString(base::UTF16ToUTF8(plaintext), ciphertext);
 }
 
+// static
 bool OSCrypt::DecryptString16(const std::string& ciphertext,
                               base::string16* plaintext) {
   std::string utf8;
   if (!DecryptString(ciphertext, &utf8))
     return false;
 
   *plaintext = base::UTF8ToUTF16(utf8);
   return true;
 }
 
+// static
 bool OSCrypt::EncryptString(const std::string& plaintext,
                             std::string* ciphertext) {
   // This currently "obfuscates" by encrypting with hard-coded password.
   // We need to improve this password situation by moving a secure password
   // into a system-level key store.
   // http://crbug.com/25404 and http://crbug.com/49115
 
   if (plaintext.empty()) {
     *ciphertext = std::string();
     return true;
   }
 
-  std::unique_ptr<crypto::SymmetricKey> encryption_key(GetEncryptionKey());
+  // If a KeyStorage is available, use a password backed by the KeyStorage.
+  // Otherwise use the hardcode password.
+  Version version = GetKeyStorage() ? Version::V11 : Version::V10;
+
+  std::unique_ptr<crypto::SymmetricKey> encryption_key(
+      GetEncryptionKey(version));
   if (!encryption_key.get())
     return false;
 
   std::string iv(kIVBlockSizeAES128, ' ');
   crypto::Encryptor encryptor;
   if (!encryptor.Init(encryption_key.get(), crypto::Encryptor::CBC, iv))
     return false;
 
   if (!encryptor.Encrypt(plaintext, ciphertext))
     return false;
 
+  // TODO(cfrousios) update with encryption type used

[vabr (Chromium), 2016/05/13 15:10:18]

nit: Unless this is something planned for the next CL or two, please capture the
TODO in a bug or a bug comment and refer to it in the TODO: TODO(crbug.com/XXX).

[cfroussios, 2016/05/13 17:09:12]

Done.

   // Prefix the cypher text with version information.
-  ciphertext->insert(0, kObfuscationPrefix);
+  ciphertext->insert(0, kObfuscationPrefix[version]);
   return true;
 }
 
+// static
 bool OSCrypt::DecryptString(const std::string& ciphertext,
                             std::string* plaintext) {
   // This currently "obfuscates" by encrypting with hard-coded password.
   // We need to improve this password situation by moving a secure password
   // into a system-level key store.
   // http://crbug.com/25404 and http://crbug.com/49115
 
   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(kObfuscationPrefix) != 0) {
+  // Check that the incoming cyphertext was encrypted and with what version.
+  // Credit card numbers are current legacy unencrypted data, so false match
+  // with prefix won't happen.
+  Version version;
+  if (ciphertext.find(kObfuscationPrefix[Version::V10]) == 0) {

[vabr (Chromium), 2016/05/13 15:10:19]

Could you use StartsWith from base/strings/string_util.h instead? That would be
more efficient than a general find().

Also below.

[cfroussios, 2016/05/13 17:09:12]

I'm not sure it is more efficient. find runs on the whole string only for legacy
data. StartsWith, on the other hand, has an option for case sensitivity, making
it more general in a different way.

+    version = Version::V10;
+  } else if (ciphertext.find(kObfuscationPrefix[Version::V11]) == 0) {
+    version = Version::V11;
+  } else {
+    // 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.
     *plaintext = ciphertext;
     return true;
   }
 
   // Strip off the versioning prefix before decrypting.
-  std::string raw_ciphertext = ciphertext.substr(strlen(kObfuscationPrefix));
+  std::string raw_ciphertext =
+      ciphertext.substr(strlen(kObfuscationPrefix[version]));
 
-  std::unique_ptr<crypto::SymmetricKey> encryption_key(GetEncryptionKey());
+  std::unique_ptr<crypto::SymmetricKey> encryption_key(
+      GetEncryptionKey(version));
   if (!encryption_key.get())
     return false;
 
   std::string iv(kIVBlockSizeAES128, ' ');
   crypto::Encryptor encryptor;
   if (!encryptor.Init(encryption_key.get(), crypto::Encryptor::CBC, iv))
     return false;
 
   if (!encryptor.Decrypt(raw_ciphertext, plaintext))
     return false;
 
   return true;
 }
+
+// static
+KeyStorageMock* OSCrypt::UseMockKeyStorage(bool use) {
+  use_mock = use;
+  if (!mock.get())
+    mock.reset(new KeyStorageMock());
+  return mock.get();
+}