mirror/flatpak
1
0
Fork 0
mirror of https://github.com/flatpak/flatpak.git synced 2026-04-08 09:01:02 -04:00
Code Issues Packages Projects Releases Wiki Activity

Revert "dir: Check commit signatures before resolving a ref"

Browse Source 
This reverts commit 915ad583a7.

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 13366524d8)
This commit is contained in:
Matthew Leeds
2019-11-12 15:02:32 -08:00
committed by Alexander Larsson
parent 8490b19705
commit 48eac4c26a
1 changed files with 41 additions and 92 deletions
Download Patch File Download Diff File Expand all files Collapse all files

133
common/flatpak-dir.c
View File

@@ -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,