dir: Check commit signatures before resolving a ref

Currently flatpak_dir_pull() has a phase where it tries to resolve a ref
to a commit before doing the pull, which is good because it means we're
pulling the same commit even if we do multiple subpath pulls, and it
allows us to get set up for accurate progress reporting. On the P2P code
path, this resolution is accomplished with an
ostree_repo_find_remotes_async() call, and then checking the results
from that. Normally that works fine, but in case a remote tries to
maliciously serve an update to refs which didn't originate from it (by
setting the same collection ID as the victim remote) things break. The
find_remotes_async() will use the malicious remote's keyring for
verification and return that commit as the most recent. This causes
errors later during the pull phase.

For example, if we're trying to update example-ref from good-remote,
and good-remote is offering commit v1 and malicious-remote is offering
commit v2, we resolve example-ref to commit v2. Then pulling that commit
from malicious-remote using good-remote's keyring fails, and pulling
commit v2 from good-remote fails because it doesn't exist there.

So this commit changes flatpak_dir_pull() so that it pulls commit
metadata before deciding on a commit. Since the pull code uses the
"ref-keyring-map" option, the bad signatures will be found and the
latest good commit will be returned. This requires a few changes:
1) Move the ostree_repo_prepare_transaction() call up to before the new
pull, which also means using "goto out;" in a few more places.
2) Use OSTREE_REPO_PULL_FLAGS_MIRROR for the pull and
ostree_repo_resolve_collection_ref() after the pull, conditional on
checks for ostree v2019.2. That is more correct but we need the patch in this
PR[1] for it to work.
3) Change repo_pull() so that it will accept results_to_fetch != NULL &&
rev_to_fetch == NULL. This means making a g_autofree version of
rev_to_fetch and resolving it after the pull if necessary.

This is all working toward the goal of getting the unit test in the
following commit, test-p2p-security.sh, to succeed.

[1] ostreedev/ostree#1821

common/flatpak-dir.c

@@ -3922,21 +3922,20 @@ repo_pull (OstreeRepo                           *self,
   gboolean force_disable_deltas = (flatpak_flags & FLATPAK_PULL_FLAGS_NO_STATIC_DELTAS) != 0;
   g_autofree char *remote_and_branch = NULL;
   g_autofree char *current_checksum = NULL;
-
+  g_autofree char *owned_rev_to_fetch = g_strdup (rev_to_fetch);
   g_autoptr(GVariant) old_commit = NULL;
   g_autoptr(GVariant) new_commit = NULL;
   const char *revs_to_fetch[2];
   gboolean res = FALSE;
   g_autofree gchar *collection_id = NULL;
   g_autoptr(GError) dummy_error = NULL;
+  gboolean pulled_using_p2p = FALSE;
+  OstreeCollectionRef collection_ref;
 
   /* The ostree fetcher asserts if error is NULL */
   if (error == NULL)
     error = &dummy_error;
 
-  /* If @results_to_fetch is set, @rev_to_fetch must be. */
-  g_assert (results_to_fetch == NULL || rev_to_fetch != NULL);
-
   /* We always want this on for every type of pull */
   flags |= OSTREE_REPO_PULL_FLAGS_BAREUSERONLY_FILES;
 
@@ -3955,11 +3954,15 @@ repo_pull (OstreeRepo                           *self,
     {
       g_autoptr(GAsyncResult) find_result = NULL, pull_result = NULL;
       g_auto(OstreeRepoFinderResultv) results = NULL;
-      OstreeCollectionRef collection_ref;
       OstreeCollectionRef *collection_refs_to_fetch[2];
       guint32 update_freq = 0;
       g_autoptr(GMainContextPopDefault) context = NULL;
 
+      pulled_using_p2p = TRUE;
+
+      collection_ref.collection_id = collection_id;
+      collection_ref.ref_name = (char *) ref_to_fetch;
+
       context = flatpak_main_context_new_default ();
 
       if (results_to_fetch == NULL)
@@ -3976,9 +3979,6 @@ repo_pull (OstreeRepo                           *self,
                                      g_variant_new_variant (g_variant_new_boolean (TRUE)));
             }
 
-          collection_ref.collection_id = collection_id;
-          collection_ref.ref_name = (char *) ref_to_fetch;
-
           collection_refs_to_fetch[0] = &collection_ref;
           collection_refs_to_fetch[1] = NULL;
 
@@ -3990,10 +3990,10 @@ repo_pull (OstreeRepo                           *self,
           g_variant_builder_add (&find_builder, "{s@v}", "update-frequency",
                                  g_variant_new_variant (g_variant_new_uint32 (update_freq)));
 
-          if (rev_to_fetch != NULL)
+          if (owned_rev_to_fetch != NULL)
             {
               g_variant_builder_add (&find_builder, "{s@v}", "override-commit-ids",
-                                     g_variant_new_variant (g_variant_new_strv (&rev_to_fetch, 1)));
+                                     g_variant_new_variant (g_variant_new_strv ((const char * const *)&owned_rev_to_fetch, 1)));
             }
 
           find_options = g_variant_ref_sink (g_variant_builder_end (&find_builder));
@@ -4049,6 +4049,7 @@ repo_pull (OstreeRepo                           *self,
       if (error != NULL && *error != NULL)
         g_debug ("Failed to pull using find-remotes; falling back to normal pull: %s", (*error)->message);
       g_clear_error (error);
+      pulled_using_p2p = FALSE;
     }
 
   if (!res)
@@ -4067,10 +4068,13 @@ repo_pull (OstreeRepo                           *self,
       g_variant_builder_add (&builder, "{s@v}", "refs",
                              g_variant_new_variant (g_variant_new_strv ((const char * const *) refs_to_fetch, -1)));
 
-      revs_to_fetch[0] = rev_to_fetch;
-      revs_to_fetch[1] = NULL;
-      g_variant_builder_add (&builder, "{s@v}", "override-commit-ids",
-                             g_variant_new_variant (g_variant_new_strv ((const char * const *) revs_to_fetch, -1)));
+      if (owned_rev_to_fetch)
+        {
+          revs_to_fetch[0] = owned_rev_to_fetch;
+          revs_to_fetch[1] = NULL;
+          g_variant_builder_add (&builder, "{s@v}", "override-commit-ids",
+                                 g_variant_new_variant (g_variant_new_strv ((const char * const *) revs_to_fetch, -1)));
+        }
 
       options = g_variant_ref_sink (g_variant_builder_end (&builder));
 
@@ -4079,13 +4083,31 @@ repo_pull (OstreeRepo                           *self,
         return translate_ostree_repo_pull_errors (error);
     }
 
+  if (owned_rev_to_fetch == NULL)
+    {
+      if (pulled_using_p2p)
+        {
+#if OSTREE_CHECK_VERSION (2019, 2)
+          if (!ostree_repo_resolve_collection_ref (self, &collection_ref,
+                                                   FALSE, OSTREE_REPO_RESOLVE_REV_EXT_NONE,
+                                                   &owned_rev_to_fetch,
+                                                   cancellable, error))
+            return FALSE;
+#endif
+        }
+
+      if (owned_rev_to_fetch == NULL &&
+          !ostree_repo_resolve_rev (self, remote_and_branch, FALSE, &owned_rev_to_fetch, error))
+        return FALSE;
+    }
+
   if (old_commit &&
       (flatpak_flags & FLATPAK_PULL_FLAGS_ALLOW_DOWNGRADE) == 0)
     {
       guint64 old_timestamp;
       guint64 new_timestamp;
 
-      if (!ostree_repo_load_commit (self, rev_to_fetch, &new_commit, NULL, error))
+      if (!ostree_repo_load_commit (self, owned_rev_to_fetch, &new_commit, NULL, error))
         return FALSE;
 
       old_timestamp = ostree_commit_get_timestamp (old_commit);
@@ -4699,6 +4721,15 @@ flatpak_dir_pull (FlatpakDir                           *self,
 
   g_assert (progress != NULL);
 
+  if (repo == NULL)
+    repo = self->repo;
+
+  /* Past this we must use goto out, so we clean up console and
+     abort the transaction on error */
+
+  if (!ostree_repo_prepare_transaction (repo, NULL, cancellable, error))
+    goto out;
+
   /* We get the rev ahead of time so that we know it for looking up e.g. extra-data
      and to make sure we're atomically using a single rev if we happen to do multiple
      pulls (e.g. with subpaths) */
@@ -4718,9 +4749,11 @@ flatpak_dir_pull (FlatpakDir                           *self,
           OstreeCollectionRef *collection_refs_to_fetch[2];
           gboolean force_disable_deltas = (flatpak_flags & FLATPAK_PULL_FLAGS_NO_STATIC_DELTAS) != 0;
           guint update_freq = 0;
-          gsize i;
           g_autoptr(GMainContextPopDefault) context = NULL;
 
+          /* FIXME: It would be nice to break out a helper function from
+           * flatpak_dir_do_resolve_p2p_refs() and reuse it here */
+
           g_variant_builder_init (&find_builder, G_VARIANT_TYPE ("a{sv}"));
 
           if (force_disable_deltas)
@@ -4747,50 +4780,77 @@ flatpak_dir_pull (FlatpakDir                           *self,
 
           context = flatpak_main_context_new_default ();
 
-          ostree_repo_find_remotes_async (self->repo, (const OstreeCollectionRef * const *) collection_refs_to_fetch,
+          ostree_repo_find_remotes_async (repo, (const OstreeCollectionRef * const *) collection_refs_to_fetch,
                                           find_options,
                                           NULL /* default finders */, progress, cancellable,
                                           async_result_cb, &find_result);
 
           while (find_result == NULL)
             g_main_context_iteration (context, TRUE);
 
-          allocated_results = ostree_repo_find_remotes_finish (self->repo, find_result, error);
+          allocated_results = ostree_repo_find_remotes_finish (repo, find_result, error);
 
           results = (const OstreeRepoFinderResult * const *) allocated_results;
           if (results == NULL)
-            return FALSE;
+            goto out;
 
-          for (i = 0, rev = NULL; results[i] != NULL && rev == NULL; i++)
-            rev = g_strdup (g_hash_table_lookup (results[i]->ref_to_checksum, &collection_ref));
+          /* Go ahead and pull commit metadata now, so that if any of the
+           * signatures are bad we can fall back to another rev from another
+           * result */
+          {
+            OstreeRepoPullFlags metadata_pull_flags = flags | OSTREE_REPO_PULL_FLAGS_COMMIT_ONLY;
+            /*TODO: Use OSTREE_REPO_PULL_FLAGS_MIRROR for all collection-ref pulls */
+#if OSTREE_CHECK_VERSION (2019, 2)
+            metadata_pull_flags |= OSTREE_REPO_PULL_FLAGS_MIRROR;
+#endif
+            if (!repo_pull (repo, state->remote_name,
+                            NULL, ref, NULL, results,
+                            flatpak_flags, metadata_pull_flags,
+                            progress, cancellable, error))
+              {
+                g_prefix_error (error, _("While pulling %s from remote %s: "), ref, state->remote_name);
+                goto out;
+              }
+          }
 
+#if OSTREE_CHECK_VERSION (2019, 2)
+          if (!ostree_repo_resolve_collection_ref (repo, &collection_ref,
+                                                   TRUE, OSTREE_REPO_RESOLVE_REV_EXT_NONE,
+                                                   &rev,
+                                                   cancellable, error))
+            goto out;
+#else
+          g_autofree char *refspec = g_strdup_printf ("%s:%s", state->remote_name, ref);
+          if (!ostree_repo_resolve_rev (repo, refspec, TRUE, &rev, error))
+            goto out;
+#endif
           if (rev == NULL)
-            return flatpak_fail_error (error, FLATPAK_ERROR_INVALID_DATA, _("No such ref (%s, %s) in remote %s or elsewhere"),
-                                 collection_ref.collection_id, collection_ref.ref_name, state->remote_name);
+            {
+              g_set_error (error, FLATPAK_ERROR, FLATPAK_ERROR_INVALID_DATA, _("No such ref (%s, %s) in remote %s or elsewhere"),
+                           collection_ref.collection_id, collection_ref.ref_name, state->remote_name);
+              goto out;
+            }
         }
       else
         {
           flatpak_remote_state_lookup_ref (state, ref, &rev, NULL, error);
           if (rev == NULL && error != NULL && *error == NULL)
-            flatpak_fail_error (error, FLATPAK_ERROR_INVALID_DATA, _("Couldn't find latest checksum for ref %s in remote %s"),
-                          ref, state->remote_name);
+            {
+              g_set_error (error, FLATPAK_ERROR, FLATPAK_ERROR_INVALID_DATA, _("Couldn't find latest checksum for ref %s in remote %s"),
+                           ref, state->remote_name);
+              goto out;
+            }
 
           results = NULL;
         }
 
       if (rev == NULL)
         {
           g_assert (error == NULL || *error != NULL);
-          return FALSE;
+          goto out;
         }
     }
 
-  if (repo == NULL)
-    repo = self->repo;
-
-  /* Past this we must use goto out, so we clean up console and
-     abort the transaction on error */
-
   if (subpaths != NULL && subpaths[0] != NULL)
     {
       subdirs_arg = g_ptr_array_new_with_free_func (g_free);
@@ -4802,9 +4862,6 @@ flatpak_dir_pull (FlatpakDir                           *self,
       g_ptr_array_add (subdirs_arg, NULL);
     }
 
-  if (!ostree_repo_prepare_transaction (repo, NULL, cancellable, error))
-    goto out;
-
   /* Setup extra data information before starting to pull, so we can have precise
    * progress reports */
   if (!flatpak_dir_setup_extra_data (self, repo, state->remote_name,