Fix race condition on OSCrypt linux

components/os_crypt/os_crypt_linux.cc

 // Copyright 2016 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.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <iterator>
@@ skipping 54 matching lines @@
 // Lazy acquisition and caching of a KeyStorage. Will be null if no service is
 // found.
 KeyStorageLinux* GetKeyStorage() {
   if (!g_cache.Get().is_key_storage_cached) {
     g_cache.Get().is_key_storage_cached = true;
     g_cache.Get().key_storage_cache = KeyStorageLinux::CreateService();
   }
   return g_cache.Get().key_storage_cache.get();
 }
 
+// Pointer to a function that creates and returns the |KeyStorage| instance to
+// be used. The function maintains ownership of the pointer.
+KeyStorageLinux* (*g_key_storage_provider)() = &GetKeyStorage;
+
 // Returns a cached string of "peanuts". Is thread-safe.
 std::string* GetPasswordV10() {
   base::AutoLock auto_lock(g_cache.Get().lock);
   if (!g_cache.Get().password_v10_cache.get()) {
     g_cache.Get().password_v10_cache.reset(new std::string("peanuts"));
   }
   return g_cache.Get().password_v10_cache.get();
 }
 
 // Caches and returns the password from the KeyStorage or null if there is no
 // service. Is thread-safe.
 std::string* GetPasswordV11() {
   base::AutoLock auto_lock(g_cache.Get().lock);
   if (!g_cache.Get().is_password_v11_cached) {
     g_cache.Get().password_v11_cache.reset(
-        GetKeyStorage() ? new std::string(GetKeyStorage()->GetKey()) : nullptr);
+        g_key_storage_provider()
+            ? new std::string(g_key_storage_provider()->GetKey())
+            : nullptr);
     g_cache.Get().is_password_v11_cached = true;
   }
   return g_cache.Get().password_v11_cache.get();
 }
 
-// Pointer to a function that creates and returns the |KeyStorage| instance to
-// be used. The function maintains ownership of the pointer.
-KeyStorageLinux* (*g_key_storage_provider)() = &GetKeyStorage;
-
 // Pointers to functions that return a password for deriving the encryption key.
 // One function for each supported password version (see Version enum).
 std::string* (*g_get_password[])() = {
     &GetPasswordV10, &GetPasswordV11,
 };
 
 // Generates a newly allocated SymmetricKey object based on a password.
 // Ownership of the key is passed to the caller. Returns null key if a key
 // generation error occurs.
 std::unique_ptr<crypto::SymmetricKey> GetEncryptionKey(Version version) {
@@ skipping 33 matching lines @@
 }
 
 // static
 bool OSCrypt::EncryptString(const std::string& plaintext,
                             std::string* ciphertext) {
   if (plaintext.empty()) {
     ciphertext->clear();
     return true;
   }
 
   // If a |KeyStorage| is available, use a password backed by the |KeyStorage|.

[Lei Zhang, 2016/09/28 15:35:06]

The comment is a bit out of date.

[cfroussios, 2016/09/28 16:08:17]

Done.

   // Otherwise use the hardcoded password.
-  Version version = g_key_storage_provider() ? Version::V11 : Version::V10;
+  Version version =
+      GetEncryptionKey(Version::V11) ? Version::V11 : Version::V10;
 
   std::unique_ptr<crypto::SymmetricKey> encryption_key(
       GetEncryptionKey(version));

[Lei Zhang, 2016/09/28 15:35:06]

Aren't we calling GetEncryptionKey() twice in a row now?

[Lei Zhang, 2016/09/28 15:43:21]

So I was thinking the code below does the same but avoids the second
GetEncryptionKey() call if the first succeeds. WDYT?

Version version = Version::V11;
std::unique_ptr<crypto::SymmetricKey> encryption_key =
    GetEncryptionKey(version);
if (!encryption_key) {
  version = Version::V10;
  encryption_key = GetEncryptionKey(version);
}
if (!encryption_key)
  return false;

[cfroussios, 2016/09/28 16:08:16]

I had a fix ready for this, but I was thinking that I wanted |encryption_key| to
continue and explicitly depend on |version|. I'm going to go with your code.

   if (!encryption_key)
     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;
@@ skipping 92 matching lines @@
     // |KeyStorage| instance can be created. Enable V11 by returning the mock.
     if (get_key_storage_mock)
       g_key_storage_provider = get_key_storage_mock;
   } else {
     // Restore real implementation
     std::copy(std::begin(get_password_save), std::end(get_password_save),
               std::begin(g_get_password));
     g_key_storage_provider = &GetKeyStorage;
   }
 }