Always unlock all libsecret items in Password Manager and OSCrypt

components/os_crypt/key_storage_libsecret.cc

Index: components/os_crypt/key_storage_libsecret.cc
diff --git a/components/os_crypt/key_storage_libsecret.cc b/components/os_crypt/key_storage_libsecret.cc
index 1a330dcb2cb393fd1ce58a650c8138565111b1df..d4efbccbf9855ac5d71806a89f0b201ae0b0edfb 100644
--- a/components/os_crypt/key_storage_libsecret.cc
+++ b/components/os_crypt/key_storage_libsecret.cc
@@ -33,6 +33,18 @@ const SecretSchema kKeystoreSchemaV2 = {
         {nullptr, SECRET_SCHEMA_ATTRIBUTE_STRING},
     }};
 
+// From a search result, extract a SecretValue, asserting that there was only
+// one result.
+SecretValue* ToSingleSecret(GList* secret_items) {
+  GList* first = g_list_first(secret_items);
+  if (g_list_next(first) != nullptr) {
+    LOG(ERROR) << "OSCrypt found more than one encryption keys.";

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

Seems like you use VLOG everywhere?

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

Done.

+  }
+  SecretItem* secret_item = static_cast<SecretItem*>(first->data);
+  g_list_free(secret_items);

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

Doesn't it destroy SecretItem* too?

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

There is no method to free a SecretItem.
https://developer.gnome.org/libsecret/unstable/SecretItem.html
(delete means deleting it from the keyring)

I'm not 100% sure what they do, so let's assume it does.

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

The concern was that secret_item_get_secret(secret_item) reads from the
uninitialized memory.

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

Yes, I forgot to complete my point that if doesn't need deleting, then it might
be a pointer to a static container.

Anyway, this is not the assumption anymore.

+  return LibsecretLoader::secret_item_get_secret(secret_item);
+}
+
 }  // namespace
 
 std::string KeyStorageLibsecret::AddRandomPasswordInLibsecret() {
@@ -55,8 +67,16 @@ 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) {
+    g_error_free(error);
+    return std::string();
+  }
+  SecretValue* password_libsecret = ToSingleSecret(search_results);
   if (error) {

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

if (error) again?

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

Done.

     VLOG(1) << "Libsecret lookup failed: " << error->message;
     g_error_free(error);
@@ -83,26 +103,39 @@ 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 || !search_results) {

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

Why the identical logic above checks just |error|. Can it be that both error and
search_results aren't empty?

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

I intended to flatten the nulls in ToSingleSecret() and forgot to finish it.

Done.

+    g_error_free(error);

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

is it ok to free 0?

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

They check that it's not null.
https://github.com/GNOME/glib/blob/master/glib/gerror.c#L487

But reading deeper I now see that they may print an error if it is null.

+    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 || !success) {
+    g_error_free(error);
     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)
+    g_error_free(error);
 
   VLOG(1) << "OSCrypt migrated from deprecated password.";