Always unlock all libsecret items in Password Manager and OSCrypt

components/os_crypt/libsecret_util_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/libsecret_util_linux.h"
 
 #include <dlfcn.h>
 
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 
 //
 // LibsecretLoader
 //
 
 decltype(
     &::secret_password_store_sync) LibsecretLoader::secret_password_store_sync;
 decltype(
     &::secret_service_search_sync) LibsecretLoader::secret_service_search_sync;
 decltype(
     &::secret_password_clear_sync) LibsecretLoader::secret_password_clear_sync;
 decltype(&::secret_item_get_secret) LibsecretLoader::secret_item_get_secret;
 decltype(&::secret_value_get_text) LibsecretLoader::secret_value_get_text;
 decltype(
     &::secret_item_get_attributes) LibsecretLoader::secret_item_get_attributes;
 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;
 
 const LibsecretLoader::FunctionInfo LibsecretLoader::kFunctions[] = {
     {"secret_item_get_secret",
      reinterpret_cast<void**>(&secret_item_get_secret)},
     {"secret_item_get_attributes",
      reinterpret_cast<void**>(&secret_item_get_attributes)},
     {"secret_item_load_secret_sync",
      reinterpret_cast<void**>(&secret_item_load_secret_sync)},
     {"secret_password_clear_sync",
      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)},
     {"secret_value_unref", reinterpret_cast<void**>(&secret_value_unref)},
 };
 
 // static
 bool LibsecretLoader::EnsureLibsecretLoaded() {
   return LoadLibsecret() && LibsecretIsAvailable();
 }
@@ skipping 38 matching lines @@
   // is meant to return an empty result.
   LibsecretAttributesBuilder attrs;
   attrs.Append("application", "chrome-string_to_get_empty_result");
   const SecretSchema kDummySchema = {
       "_chrome_dummy_schema",
       SECRET_SCHEMA_DONT_MATCH_NAME,
       {{"application", SECRET_SCHEMA_ATTRIBUTE_STRING},
        {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),

[vasilii, 2016/10/21 14:10:07]

Will it unlock if nothing is found?
What actually happens when the Keyring is locked? Can you read metadata at
least?

[cfroussios, 2016/10/21 17:56:36]

Search will always return the SecretItems, which individually may or may not be
locked. Metadata isn't protected.

In this particular call unlocking is redundant, because we're expecting no
results. I added this so that I can make it a rule that we always unlock the
stuff that we use. Tests now expect this flag without caring what we're
fetching.

Is there a case where we only need the metadata? I didn't see it.

[vasilii, 2016/10/24 10:41:14]

This method will never unlock anything, we put SECRET_SEARCH_UNLOCK only for
consistency. Is it correct?

[cfroussios, 2016/10/24 11:25:55]

Yes, it's only for consistency. I don't expect any effect here.

[vasilii, 2016/10/24 12:39:35]

Acknowledged.

+      nullptr,  // no cancellable ojbect
+      &error);
   bool success = (error == nullptr);
   if (error)
     g_error_free(error);
   if (found)
     g_list_free(found);
 
   return success;
 }
 
 //
@@ skipping 18 matching lines @@
   name_values_.push_back(value);
   gpointer value_str =
       static_cast<gpointer>(const_cast<char*>(name_values_.back().c_str()));
   g_hash_table_insert(attrs_, name_str, value_str);
 }
 
 void LibsecretAttributesBuilder::Append(const std::string& name,
                                         int64_t value) {
   Append(name, base::Int64ToString(value));
 }