Unlock all libsecret items in Password Manager and OSCrypt

The various libsecret methods have different contracts w.r.t. unlocking
keyring items. Of the currently used methods, only secret_service_store
will trigger unlocking as necessary. As a result, operations fail in
various ways until someone performs the first store operation.

With the CL, the libsecret native backend of Password Manager is set to explicitly
request unlocking of items on every call. OSCrypt avoids using
secret_service_lookup, which apparently ignores locked items.

BUG=657828,631171

Review-Url: https://codereview.chromium.org/2441653002
Cr-Commit-Position: refs/heads/master@{#427067}

chrome/browser/password_manager/native_backend_libsecret.cc

@@ -318,7 +318,8 @@ bool NativeBackendLibsecret::AddUpdateLoginSearch(
   GError* error = nullptr;
   GList* found = LibsecretLoader::secret_service_search_sync(
       nullptr,  // default secret service
-      &kLibsecretSchema, attrs.Get(), SECRET_SEARCH_ALL,
+      &kLibsecretSchema, attrs.Get(),
+      static_cast<SecretSearchFlags>(SECRET_SEARCH_ALL | SECRET_SEARCH_UNLOCK),
       nullptr,  // no cancellable ojbect
       &error);
   if (error) {
@@ -421,7 +422,8 @@ bool NativeBackendLibsecret::GetLoginsList(
   GError* error = nullptr;
   GList* found = LibsecretLoader::secret_service_search_sync(
       nullptr,  // default secret service
-      &kLibsecretSchema, attrs.Get(), SECRET_SEARCH_ALL,
+      &kLibsecretSchema, attrs.Get(),
+      static_cast<SecretSearchFlags>(SECRET_SEARCH_ALL | SECRET_SEARCH_UNLOCK),
       nullptr,  // no cancellable ojbect
       &error);
   if (error) {

chrome/browser/password_manager/native_backend_libsecret_unittest.cc

@@ -129,6 +129,7 @@ GList* mock_secret_service_search_sync(SecretService* service,
                                        SecretSearchFlags flags,
                                        GCancellable* cancellable,
                                        GError** error) {
+  EXPECT_TRUE(flags & SECRET_SEARCH_UNLOCK);
   GList* result = nullptr;
   for (MockSecretItem* item : *global_mock_libsecret_items) {
     if (Matches(item, attributes))

components/os_crypt/key_storage_libsecret.cc

@@ -33,6 +33,22 @@ const SecretSchema kKeystoreSchemaV2 = {
         {nullptr, SECRET_SCHEMA_ATTRIBUTE_STRING},
     }};
 
+// From a search result, extracts a SecretValue, asserting that there was at
+// most one result. Returns nullptr if there are no results.
+SecretValue* ToSingleSecret(GList* secret_items) {
+  GList* first = g_list_first(secret_items);
+  if (first == nullptr)
+    return nullptr;
+  if (g_list_next(first) != nullptr) {
+    VLOG(1) << "OSCrypt found more than one encryption keys.";
+  }
+  SecretItem* secret_item = static_cast<SecretItem*>(first->data);
+  SecretValue* secret_value =
+      LibsecretLoader::secret_item_get_secret(secret_item);
+  g_list_free(secret_items);
+  return secret_value;
+}
+
 }  // namespace
 
 std::string KeyStorageLibsecret::AddRandomPasswordInLibsecret() {
@@ -42,8 +58,13 @@ std::string KeyStorageLibsecret::AddRandomPasswordInLibsecret() {
   bool success = LibsecretLoader::secret_password_store_sync(
       &kKeystoreSchemaV2, nullptr, KeyStorageLinux::kKey, password.c_str(),
       nullptr, &error, "application", kApplicationName, nullptr);
-  if (error || !success) {
+  if (error) {
     VLOG(1) << "Libsecret lookup failed: " << error->message;
+    g_error_free(error);
+    return std::string();
+  }
+  if (!success) {
+    VLOG(1) << "Libsecret lookup failed.";
     return std::string();
   }
 
@@ -55,13 +76,17 @@ std::string KeyStorageLibsecret::GetKey() {
   GError* error = nullptr;
   LibsecretAttributesBuilder attrs;
   attrs.Append("application", kApplicationName);
-  SecretValue* password_libsecret = LibsecretLoader::secret_service_lookup_sync(
-      nullptr, &kKeystoreSchemaV2, attrs.Get(), nullptr, &error);
+  GList* search_results = LibsecretLoader::secret_service_search_sync(
+      nullptr /* default secret service */, &kKeystoreSchemaV2, attrs.Get(),
+      static_cast<SecretSearchFlags>(SECRET_SEARCH_UNLOCK |
+                                     SECRET_SEARCH_LOAD_SECRETS),
+      nullptr, &error);
   if (error) {
     VLOG(1) << "Libsecret lookup failed: " << error->message;
     g_error_free(error);
     return std::string();
   }
+  SecretValue* password_libsecret = ToSingleSecret(search_results);
   if (!password_libsecret) {
     std::string password = Migrate();
     if (!password.empty())
@@ -83,26 +108,48 @@ std::string KeyStorageLibsecret::Migrate() {
   LibsecretAttributesBuilder attrs;
 
   // Detect old entry.
-  SecretValue* password_libsecret = LibsecretLoader::secret_service_lookup_sync(
-      nullptr, &kKeystoreSchemaV1, attrs.Get(), nullptr, &error);
-  if (error || !password_libsecret)
+  GList* search_results = LibsecretLoader::secret_service_search_sync(
+      nullptr /* default secret service */, &kKeystoreSchemaV1, attrs.Get(),
+      static_cast<SecretSearchFlags>(SECRET_SEARCH_UNLOCK |
+                                     SECRET_SEARCH_LOAD_SECRETS),
+      nullptr, &error);
+  if (error) {
+    g_error_free(error);
+    g_list_free(search_results);
+    return std::string();
+  }
+  SecretValue* password_libsecret = ToSingleSecret(search_results);
+  if (!password_libsecret)
     return std::string();
 
   VLOG(1) << "OSCrypt detected a deprecated password in Libsecret.";
   std::string password(
       LibsecretLoader::secret_value_get_text(password_libsecret));
+  LibsecretLoader::secret_value_unref(password_libsecret);
 
   // Create new entry.
   bool success = LibsecretLoader::secret_password_store_sync(
       &kKeystoreSchemaV2, nullptr, KeyStorageLinux::kKey, password.c_str(),
       nullptr, &error, "application", kApplicationName, nullptr);
-  if (error || !success)
+  if (error) {
+    VLOG(1) << "Failed to store migrated password. " << error->message;
+    g_error_free(error);
+    return std::string();
+  }
+  if (!success) {
+    VLOG(1) << "Failed to store migrated password.";
     return std::string();
+  }
 
   // Delete old entry.
   // Even if deletion failed, we have to use the password that we created.
   success = LibsecretLoader::secret_password_clear_sync(
       &kKeystoreSchemaV1, nullptr, &error, nullptr);
+  if (error) {
+    VLOG(1) << "OSCrypt failed to delete deprecated password. "
+            << error->message;
+    g_error_free(error);
+  }
 
   VLOG(1) << "OSCrypt migrated from deprecated password.";
 

components/os_crypt/key_storage_libsecret_unittest.cc

@@ -16,6 +16,8 @@ namespace {
 // cast to the correct signature. We can reduce SecretValue to an std::string,
 // because we don't use anything else from it.
 using MockSecretValue = std::string;
+// Likewise, we only need a SecretValue from SecretItem.
+using MockSecretItem = MockSecretValue;
 
 const SecretSchema kKeystoreSchemaV1 = {
     "chrome_libsecret_os_crypt_password",
@@ -61,13 +63,6 @@ class MockLibsecretLoader : public LibsecretLoader {
                                                   GError** error,
                                                   ...);
 
-  static MockSecretValue* mock_secret_service_lookup_sync(
-      SecretService* service,
-      const SecretSchema* schema,
-      GHashTable* attributes,
-      GCancellable* cancellable,
-      GError** error);
-
   static void mock_secret_value_unref(gpointer value);
 
   static GList* mock_secret_service_search_sync(SecretService* service,
@@ -82,6 +77,8 @@ class MockLibsecretLoader : public LibsecretLoader {
                                                   GError** error,
                                                   ...);
 
+  static MockSecretValue* mock_secret_item_get_secret(MockSecretItem* item);
+
   // MockLibsecretLoader owns these objects.
   static MockSecretValue* stored_password_mock_ptr_;
   static MockSecretValue* deprecated_password_mock_ptr_;
@@ -111,38 +108,33 @@ gboolean MockLibsecretLoader::mock_secret_password_store_sync(
 }
 
 // static
-MockSecretValue* MockLibsecretLoader::mock_secret_service_lookup_sync(
+void MockLibsecretLoader::mock_secret_value_unref(gpointer value) {}
+
+// static
+GList* MockLibsecretLoader::mock_secret_service_search_sync(
     SecretService* service,
     const SecretSchema* schema,
     GHashTable* attributes,
+    SecretSearchFlags flags,
     GCancellable* cancellable,
     GError** error) {
   bool is_known_schema = strcmp(schema->name, kKeystoreSchemaV2.name) == 0 ||
                          strcmp(schema->name, kKeystoreSchemaV1.name) == 0;
   EXPECT_TRUE(is_known_schema);
 
+  EXPECT_TRUE(flags & SECRET_SEARCH_UNLOCK);
+  EXPECT_TRUE(flags & SECRET_SEARCH_LOAD_SECRETS);
+
+  MockSecretItem* item = nullptr;
   if (strcmp(schema->name, kKeystoreSchemaV2.name) == 0)
-    return stored_password_mock_ptr_;
+    item = stored_password_mock_ptr_;
   else if (strcmp(schema->name, kKeystoreSchemaV1.name) == 0)
-    return deprecated_password_mock_ptr_;
-
-  NOTREACHED();
-  return nullptr;
-}
+    item = deprecated_password_mock_ptr_;
 
-// static
-void MockLibsecretLoader::mock_secret_value_unref(gpointer value) {}
-
-// static
-GList* MockLibsecretLoader::mock_secret_service_search_sync(
-    SecretService* service,
-    const SecretSchema* schema,
-    GHashTable* attributes,
-    SecretSearchFlags flags,
-    GCancellable* cancellable,
-    GError** error) {
-  *error = nullptr;
-  return nullptr;
+  GList* result = nullptr;
+  result = g_list_append(result, item);
+  g_clear_error(error);
+  return result;
 }
 
 // static
@@ -151,29 +143,34 @@ gboolean MockLibsecretLoader::mock_secret_password_clear_sync(
     GCancellable* cancellable,
     GError** error,
     ...) {
+  // We would only delete entries in the deprecated schema.
   EXPECT_STREQ(kKeystoreSchemaV1.name, schema->name);
   delete deprecated_password_mock_ptr_;
   deprecated_password_mock_ptr_ = nullptr;
   return true;
 }
 
+// static
+MockSecretValue* MockLibsecretLoader::mock_secret_item_get_secret(
+    MockSecretItem* item) {
+  return item;
+}
+
 // static
 bool MockLibsecretLoader::ResetForOSCrypt() {
-  // 4 methods used by KeyStorageLibsecret
+  // Methods used by KeyStorageLibsecret
   secret_password_store_sync =
       &MockLibsecretLoader::mock_secret_password_store_sync;
   secret_value_get_text = (decltype(&::secret_value_get_text)) &
                           MockLibsecretLoader::mock_secret_value_get_text;
   secret_value_unref = &MockLibsecretLoader::mock_secret_value_unref;
-  secret_service_lookup_sync =
-      (decltype(&::secret_service_lookup_sync)) &
-      MockLibsecretLoader::mock_secret_service_lookup_sync;
+  secret_service_search_sync =
+      &MockLibsecretLoader::mock_secret_service_search_sync;
+  secret_item_get_secret =
+      (decltype(&::secret_item_get_secret))mock_secret_item_get_secret;
   // Used by Migrate()
   secret_password_clear_sync =
       &MockLibsecretLoader::mock_secret_password_clear_sync;
-  // 1 method used by LibsecretLoader::EnsureLibsecretLoaded()
-  secret_service_search_sync =
-      &MockLibsecretLoader::mock_secret_service_search_sync;
 
   delete stored_password_mock_ptr_;
   stored_password_mock_ptr_ = nullptr;

components/os_crypt/libsecret_util_linux.cc

@@ -26,8 +26,6 @@ decltype(
 decltype(&::secret_item_load_secret_sync)
     LibsecretLoader::secret_item_load_secret_sync;
 decltype(&::secret_value_unref) LibsecretLoader::secret_value_unref;
-decltype(
-    &::secret_service_lookup_sync) LibsecretLoader::secret_service_lookup_sync;
 
 bool LibsecretLoader::libsecret_loaded_ = false;
 
@@ -42,8 +40,6 @@ const LibsecretLoader::FunctionInfo LibsecretLoader::kFunctions[] = {
      reinterpret_cast<void**>(&secret_password_clear_sync)},
     {"secret_password_store_sync",
      reinterpret_cast<void**>(&secret_password_store_sync)},
-    {"secret_service_lookup_sync",
-     reinterpret_cast<void**>(&secret_service_lookup_sync)},
     {"secret_service_search_sync",
      reinterpret_cast<void**>(&secret_service_search_sync)},
     {"secret_value_get_text", reinterpret_cast<void**>(&secret_value_get_text)},
@@ -102,11 +98,12 @@ bool LibsecretLoader::LibsecretIsAvailable() {
        {nullptr, SECRET_SCHEMA_ATTRIBUTE_STRING}}};
 
   GError* error = nullptr;
-  GList* found =
-      secret_service_search_sync(nullptr,  // default secret service
-                                 &kDummySchema, attrs.Get(), SECRET_SEARCH_ALL,
-                                 nullptr,  // no cancellable ojbect
-                                 &error);
+  GList* found = secret_service_search_sync(
+      nullptr,  // default secret service
+      &kDummySchema, attrs.Get(),
+      static_cast<SecretSearchFlags>(SECRET_SEARCH_ALL | SECRET_SEARCH_UNLOCK),
+      nullptr,  // no cancellable ojbect
+      &error);
   bool success = (error == nullptr);
   if (error)
     g_error_free(error);

components/os_crypt/libsecret_util_linux.h

@@ -20,7 +20,6 @@ class LibsecretLoader {
   static decltype(&::secret_item_load_secret_sync) secret_item_load_secret_sync;
   static decltype(&::secret_password_clear_sync) secret_password_clear_sync;
   static decltype(&::secret_password_store_sync) secret_password_store_sync;
-  static decltype(&::secret_service_lookup_sync) secret_service_lookup_sync;
   static decltype(&::secret_service_search_sync) secret_service_search_sync;
   static decltype(&::secret_value_get_text) secret_value_get_text;
   static decltype(&::secret_value_unref) secret_value_unref;