Skip to content

Commit

Permalink
dir: Fix an edge case of resolving collection-refs
Browse files Browse the repository at this point in the history
In flatpak_dir_do_resolve_p2p_refs() after pulling a ref we use
ostree_repo_resolve_rev() and pass a refspec with the remote from which
the ref originated. This has a couple side effects, one good and one
bad:
1) The good side effect is that the attack I speculated about in this
comment[1] is not exploitable. Because if the ref in question is pulled
from any remote other than its origin (or a LAN/USB source using another
remote's keyring) it will not be found by ostree_repo_resolve_rev() and
the malicious commit will not be used.
2) The bad side effect is that there are some legitimate reasons a ref
could be pulled from another remote (say, a configured mirror), and in
those cases the pulled ref will not be found. So if I have remote A and
remote B both configured with the same collection ID, a ref installed
from one could be pulled from the other. See this issue[2] for a
concrete example.

The solution is to use OSTREE_REPO_PULL_FLAGS_MIRROR for the pull and
use ostree_repo_resolve_collection_ref() to resolve the ref. This is
done in the caller as well for consistency
(flatpak_dir_resolve_p2p_refs()). This fixes the bad side effect
described above and brings us a step closer to fixing issue flatpak#1832. This
also means the attack from flatpak#1447 is exploitable, but that is addressed
in a subsequent commit.

This change is conditional on a version check for OSTree 2019.2 because
we need this bug fix[3].

Also, add a helper function flatpak_repo_resolve_rev() which falls back
to using ostree_repo_resolve_rev() when
ostree_repo_resolve_collection_ref() fails, so we start to move toward
using /refs/mirrors/ but maintain backwards compatibility for
/refs/remotes/. A subsequent commit will make wider use of
flatpak_repo_resolve_rev() across the codebase; for now just use it for
the case described above.

[1] flatpak#1447 (comment)
[2] flatpak#1832
[3] ostreedev/ostree#1821
  • Loading branch information
mwleeds committed Feb 26, 2019
1 parent 31c0071 commit f087e17
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 6 deletions.
19 changes: 13 additions & 6 deletions common/flatpak-dir.c
Original file line number Diff line number Diff line change
Expand Up @@ -3294,9 +3294,14 @@ flatpak_dir_do_resolve_p2p_refs (FlatpakDir *self,
}
}

OstreeRepoPullFlags flags = OSTREE_REPO_PULL_FLAGS_COMMIT_ONLY;
#if OSTREE_CHECK_VERSION (2019, 2)
flags |= OSTREE_REPO_PULL_FLAGS_MIRROR;
#endif

g_variant_builder_init (&pull_builder, G_VARIANT_TYPE ("a{sv}"));
g_variant_builder_add (&pull_builder, "{s@v}", "flags",
g_variant_new_variant (g_variant_new_int32 (OSTREE_REPO_PULL_FLAGS_COMMIT_ONLY)));
g_variant_new_variant (g_variant_new_int32 (flags)));
g_variant_builder_add (&pull_builder, "{s@v}", "inherit-transaction",
g_variant_new_variant (g_variant_new_boolean (TRUE)));
pull_options = g_variant_ref_sink (g_variant_builder_end (&pull_builder));
Expand All @@ -3321,13 +3326,13 @@ flatpak_dir_do_resolve_p2p_refs (FlatpakDir *self,
FlatpakDirResolveData *data = datas[i];
FlatpakDirResolve *resolve = data->resolve;
g_autoptr(GVariant) commit_data = NULL;
g_autofree char *refspec = NULL;

if (resolve->resolved_commit != NULL)
continue;

refspec = g_strdup_printf ("%s:%s", resolve->remote, resolve->ref);
if (!ostree_repo_resolve_rev (child_repo, refspec, FALSE, &resolve->resolved_commit, error))
if (!flatpak_repo_resolve_rev (child_repo, data->collection_ref.collection_id, resolve->remote,
resolve->ref, FALSE, &resolve->resolved_commit,
cancellable, error))
return FALSE;

if (!ostree_repo_load_commit (child_repo, resolve->resolved_commit, &commit_data, NULL, error))
Expand All @@ -3353,7 +3358,6 @@ flatpak_dir_resolve_p2p_refs (FlatpakDir *self,
{
FlatpakDirResolve *resolve = resolves[i];
FlatpakDirResolveData *data = g_new0 (FlatpakDirResolveData, 1);
g_autofree char *refspec = g_strdup_printf ("%s:%s", resolve->remote, resolve->ref);

g_assert (resolve->ref != NULL);
g_assert (resolve->remote != NULL);
Expand All @@ -3365,7 +3369,10 @@ flatpak_dir_resolve_p2p_refs (FlatpakDir *self,
g_assert (data->collection_ref.collection_id != NULL);

if (resolve->opt_commit == NULL)
ostree_repo_resolve_rev (self->repo, refspec, TRUE, &data->local_commit, NULL);
{
flatpak_repo_resolve_rev (self->repo, data->collection_ref.collection_id, resolve->remote,
resolve->ref, TRUE, &data->local_commit, cancellable, NULL);
}

/* The ostree p2p api doesn't let you mix pulls with specific commit IDs
* and HEAD (https://github.com/ostreedev/ostree/issues/1622) so we need
Expand Down
9 changes: 9 additions & 0 deletions common/flatpak-utils-private.h
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,15 @@ gboolean flatpak_check_required_version (const char *ref,
int flatpak_levenshtein_distance (const char *s,
const char *t);

gboolean flatpak_repo_resolve_rev (OstreeRepo *repo,
const char *collection_id, /* nullable */
const char *remote_name, /* nullable */
const char *ref_name,
gboolean allow_noent,
char **out_rev,
GCancellable *cancellable,
GError **error);

#define FLATPAK_MESSAGE_ID "c7b39b1e006b464599465e105b361485"

#endif /* __FLATPAK_UTILS_H__ */
46 changes: 46 additions & 0 deletions common/flatpak-utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -6147,6 +6147,52 @@ flatpak_disable_raw_mode (void)
tcsetattr (STDIN_FILENO, TCSAFLUSH, &raw);
}

/* Wrapper around ostree_repo_resolve_rev() and
* ostree_repo_resolve_collection_ref() that intelligently chooses the correct
* one to use */
gboolean
flatpak_repo_resolve_rev (OstreeRepo *repo,
const char *collection_id, /* nullable */
const char *remote_name, /* nullable */
const char *ref_name,
gboolean allow_noent,
char **out_rev,
GCancellable *cancellable,
GError **error)
{
if (collection_id != NULL)
{
/* 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)
OstreeCollectionRef c_r;
c_r.collection_id = (char *)collection_id;
c_r.ref_name = (char *)ref_name;
OstreeRepoResolveRevExtFlags flags = remote_name == NULL ?
OSTREE_REPO_RESOLVE_REV_EXT_LOCAL_ONLY :
OSTREE_REPO_RESOLVE_REV_EXT_NONE;
if (ostree_repo_resolve_collection_ref (repo, &c_r,
allow_noent,
flags,
out_rev,
cancellable, NULL))
return TRUE;
#endif
}

/* There may be several remotes with the same branch (if we for
* instance changed the origin) so prepend the current origin to
* make sure we get the right one */
if (remote_name != NULL)
{
g_autofree char *refspec = g_strdup_printf ("%s:%s", remote_name, ref_name);
return ostree_repo_resolve_rev (repo, refspec, allow_noent, out_rev, error);
}
else
return ostree_repo_resolve_rev_ext (repo, ref_name, allow_noent,
OSTREE_REPO_LIST_REFS_EXT_NONE, out_rev, error);
}


#if !GLIB_CHECK_VERSION (2, 56, 0)
Expand Down

0 comments on commit f087e17

Please sign in to comment.