From 48eac4c26a0cce03bec2fdca52292d70025123bc Mon Sep 17 00:00:00 2001 From: Matthew Leeds Date: Tue, 12 Nov 2019 15:02:32 -0800 Subject: [PATCH] Revert "dir: Check commit signatures before resolving a ref" This reverts commit 915ad583a7bf70e03bf58bf14b9d3bdb7ef33277. This commit turned out to have unintended side effects. Specifically, with it we do a pull with OSTREE_REPO_PULL_FLAGS_MIRROR, and then flatpak_dir_setup_extra_data() does a non-mirror pull in the same transaction, so the ref being pulled ends up being written to disk under both refs/remotes/ and refs/mirrors/ in ostree_repo_commit_transaction(). This is a problem because only the remote ref is deleted during an uninstall, so the disk space is leaked, and we don't have the infrastructure in place to keep both refs up to date as they're updated. It would be nice to consistently use OSTREE_REPO_PULL_FLAGS_MIRROR for all pulls but that turns out to be a deep rabbit hole to go down; see the discussion in https://github.com/flatpak/flatpak/pull/3220 So revert the commit instead (with a few exceptions: keep a still-relevant FIXME comment, keep an assertion in the "out:" section, and keep a debug statement printing out the resolved rev). Note that this means that since we're no longer checking commit signatures during ref resolution, in theory remote B could try to set the same collection ID as remote A and serve a malicious update for something from remote A, but the signature would be found to be invalid during the pull phase due to our use of "ref-keyring-map" so the transaction would fail. All the other uses of OSTREE_REPO_PULL_FLAGS_MIRROR across the codebase should be kept I think: - flatpak create-usb uses it when pulling into the repo on the USB which works perfectly well with refs/mirrors/ (and the USB is mirroring the collection-refs!) - it's used when pulling into a temporary "child" repo in a few places and there it makes sense since the child repo is mirroring the refs so they can be pulled into the main repo. In fact, in the case of flatpak_dir_do_resolve_p2p_refs(), we need MIRROR since otherwise ostree_repo_resolve_collection_ref() gives us the commit on-disk rather than the just-pulled one that's in memory. (cherry picked from commit 13366524d82af14ad35fa4618ccf202ece1adacc) --- common/flatpak-dir.c | 133 +++++++++++++------------------------------ 1 file changed, 41 insertions(+), 92 deletions(-) diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c index 2f1c379d..029a66cc 100644 --- a/common/flatpak-dir.c +++ b/common/flatpak-dir.c @@ -4395,8 +4395,8 @@ repo_pull (OstreeRepo *self, const char *remote_name, const char **dirs_to_pull, const char *ref_to_fetch, - const char *rev_to_fetch, /* (nullable) */ - const OstreeRepoFinderResult * const *results_to_fetch, /* (nullable) */ + const char *rev_to_fetch, + const OstreeRepoFinderResult * const *results_to_fetch, FlatpakPullFlags flatpak_flags, OstreeRepoPullFlags flags, OstreeAsyncProgress *progress, @@ -4405,20 +4405,20 @@ repo_pull (OstreeRepo *self, { gboolean force_disable_deltas = (flatpak_flags & FLATPAK_PULL_FLAGS_NO_STATIC_DELTAS) != 0; 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; @@ -4437,15 +4437,11 @@ 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) @@ -4462,6 +4458,9 @@ 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; @@ -4473,10 +4472,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 (owned_rev_to_fetch != NULL) + if (rev_to_fetch != NULL) { g_variant_builder_add (&find_builder, "{s@v}", "override-commit-ids", - g_variant_new_variant (g_variant_new_strv ((const char * const *)&owned_rev_to_fetch, 1))); + g_variant_new_variant (g_variant_new_strv (&rev_to_fetch, 1))); } find_options = g_variant_ref_sink (g_variant_builder_end (&find_builder)); @@ -4538,7 +4537,6 @@ 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) @@ -4557,13 +4555,10 @@ 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))); - 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))); - } + 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))); options = g_variant_ref_sink (g_variant_builder_end (&builder)); @@ -4572,21 +4567,13 @@ repo_pull (OstreeRepo *self, return translate_ostree_repo_pull_errors (error); } - if (owned_rev_to_fetch == NULL) - { - if (!flatpak_repo_resolve_rev (self, pulled_using_p2p ? collection_id : NULL, - remote_name, ref_to_fetch, FALSE, &owned_rev_to_fetch, - cancellable, 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, owned_rev_to_fetch, &new_commit, NULL, error)) + if (!ostree_repo_load_commit (self, rev_to_fetch, &new_commit, NULL, error)) return FALSE; old_timestamp = ostree_commit_get_timestamp (old_commit); @@ -5160,10 +5147,6 @@ flatpak_dir_pull (FlatpakDir *self, /* If @opt_results is set, @opt_rev must be. */ g_return_val_if_fail (opt_results == NULL || opt_rev != NULL, FALSE); - /* @repo should either be NULL or a child repo. If the repo is not a child it - * doesn't make sense to use it in the find_remotes_async() call below. */ - g_return_val_if_fail (repo == NULL || ostree_repo_equal (ostree_repo_get_parent (repo), self->repo), FALSE); - if (!flatpak_dir_ensure_repo (self, cancellable, error)) return FALSE; @@ -5191,15 +5174,6 @@ 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) */ @@ -5219,6 +5193,7 @@ 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 @@ -5251,7 +5226,7 @@ flatpak_dir_pull (FlatpakDir *self, context = flatpak_main_context_new_default (); - ostree_repo_find_remotes_async (repo, (const OstreeCollectionRef * const *) collection_refs_to_fetch, + ostree_repo_find_remotes_async (self->repo, (const OstreeCollectionRef * const *) collection_refs_to_fetch, find_options, NULL /* default finders */, progress, cancellable, async_result_cb, &find_result); @@ -5259,74 +5234,45 @@ flatpak_dir_pull (FlatpakDir *self, while (find_result == NULL) g_main_context_iteration (context, TRUE); - allocated_results = ostree_repo_find_remotes_finish (repo, find_result, error); + allocated_results = ostree_repo_find_remotes_finish (self->repo, find_result, error); results = (const OstreeRepoFinderResult * const *) allocated_results; if (results == NULL) - goto out; + return FALSE; - /* Do a version check to ensure we have these: - * https://github.com/ostreedev/ostree/pull/1821 - * https://github.com/ostreedev/ostree/pull/1825 */ -#if OSTREE_CHECK_VERSION (2019, 2) - /* 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. This also means the flatpak_dir_setup_extra_data() call - * below won't need to fetch the metadata. For an explanation of the - * attack we're protecting against see - * https://github.com/flatpak/flatpak/issues/1447#issuecomment-445347590 */ - if (results[0] != NULL) - { - OstreeRepoPullFlags metadata_pull_flags = flags | OSTREE_REPO_PULL_FLAGS_COMMIT_ONLY; - /*TODO: Use OSTREE_REPO_PULL_FLAGS_MIRROR for all collection-ref pulls */ - metadata_pull_flags |= OSTREE_REPO_PULL_FLAGS_MIRROR; - 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 (!flatpak_repo_resolve_rev (repo, collection_ref.collection_id, - state->remote_name, ref, TRUE, &rev, - cancellable, error)) - goto out; - } -#else - gsize i; 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)); -#endif if (rev == NULL) - { - g_set_error (error, FLATPAK_ERROR, FLATPAK_ERROR_REF_NOT_FOUND, _("No such ref (%s, %s) in remote %s or elsewhere"), - collection_ref.collection_id, collection_ref.ref_name, state->remote_name); - goto out; - } + return flatpak_fail_error (error, FLATPAK_ERROR_REF_NOT_FOUND, _("No such ref (%s, %s) in remote %s or elsewhere"), + collection_ref.collection_id, collection_ref.ref_name, state->remote_name); } else { - if (!flatpak_remote_state_lookup_ref (state, ref, &rev, NULL, error)) - goto out; - if (rev == NULL) - { - flatpak_fail_error (error, FLATPAK_ERROR_REF_NOT_FOUND, - _("Couldn't find latest checksum for ref %s in remote %s"), - ref, state->remote_name); - goto out; - } + flatpak_remote_state_lookup_ref (state, ref, &rev, NULL, error); + if (rev == NULL && error != NULL && *error == NULL) + flatpak_fail_error (error, FLATPAK_ERROR_REF_NOT_FOUND, _("Couldn't find latest checksum for ref %s in remote %s"), + ref, state->remote_name); results = NULL; } + + if (rev == NULL) + { + g_assert (error == NULL || *error != NULL); + return FALSE; + } } - g_assert (rev != NULL); g_debug ("%s: Using commit %s for pull of ref %s from remote %s", G_STRFUNC, rev, ref, state->remote_name); + 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); @@ -5338,6 +5284,9 @@ 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,