Skip to content

Commit

Permalink
dir: Use the right keyring to verify P2P pulls
Browse files Browse the repository at this point in the history
With Flatpak you should only have to trust each remote to provide good
updates for the apps provided by it. However the P2P support in OSTree
considers each remote to be equally trustworthy, which opens a possible
attack vector. For example if I have a flathub remote configured and
apps installed from it and I also have a remote "sketchy-remote"
configured which I have one app installed from, I expect the Flathub
apps to update from Flathub (or to update from LAN/USB sources with
Flathub GPG signatures) and not from the sketchy-remote.

The way this attack would play out is that the sketchy-remote would
deploy the same collection ID as the victim remote (in this case
org.flathub.Stable) in order to serve updates for it. So this commit
mitigates the issue by using the new "ref-keyring-map" option
added to libostree[1], which means that pulls of updates to Flathub apps
will always be verified using the Flathub GPG keyring, even if they're
coming from another source like another configured remote or a LAN/USB
source signed with the malicious remote's keyring. In the latter case
the pull from the malicious source will fail, and flatpak should then do
a successful pull from a legitimate source.

We use the "ref-keyring-map" option in both
flatpak_dir_do_resolve_p2p_refs() and repo_pull() because if we only use
it in the latter place the ref could be resolved to the malicious commit
(which would be checked with the malicious keyring), and then in
repo_pull() we would try unsuccessfully to pull the malicious commit
from the legitimate remote.

Since pulls into the system installation already use the correct
remote's keyring (see the use of ostree_repo_verify_commit_for_remote()
in flatpak_dir_pull_untrusted_local()) this mitigation is only needed
for per-user installations (or other scenarios that circumvent the
system helper). It's also only needed since the commit "dir: Fix an edge
case of resolving collection-refs" because before that commit this
attack vector wasn't exploitable.

Unfortunately this implementation is not perfect, because there's not
always a one-to-one mapping between configured remotes and GPG keyrings.
On Endless OS some remotes have keyrings in /usr/share/keyrings/ rather
than /var/lib/flatpak/repo/remote_name.trustedkeys.gpg as do remotes
added by Flatpak. However presumably you would only add a keyring to a
global directory if you trust it to the same extent as the others.

A subsequent commit will add a unit test for this.

Fixes flatpak#1447

[1] ostreedev/ostree#1810
  • Loading branch information
mwleeds committed Feb 26, 2019
1 parent ac71b7c commit 477172d
Showing 1 changed file with 27 additions and 2 deletions.
29 changes: 27 additions & 2 deletions common/flatpak-dir.c
Original file line number Diff line number Diff line change
Expand Up @@ -3200,7 +3200,7 @@ flatpak_dir_do_resolve_p2p_refs (FlatpakDir *self,
g_auto(OstreeRepoFinderResultv) results = NULL;
g_autoptr(GVariant) find_options = NULL;
g_auto(GVariantBuilder) find_builder = FLATPAK_VARIANT_BUILDER_INITIALIZER;
GVariantBuilder pull_builder;
GVariantBuilder pull_builder, ref_keyring_map_builder;
g_autoptr(GVariant) pull_options = NULL;
g_autoptr(GAsyncResult) pull_result = NULL;
g_autoptr(GMainContextPopDefault) main_context = NULL;
Expand Down Expand Up @@ -3304,6 +3304,24 @@ flatpak_dir_do_resolve_p2p_refs (FlatpakDir *self,
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)));

/* Ensure the results are signed with the GPG keys associated with the correct remote */
g_variant_builder_init (&ref_keyring_map_builder, G_VARIANT_TYPE ("a(sss)"));
for (i = 0; datas[i] != NULL; i++)
{
FlatpakDirResolveData *data = datas[i];
FlatpakDirResolve *resolve = data->resolve;

/* It's okay if some of the refs added here were removed above; the map
* can be a superset of the refs being pulled */
g_variant_builder_add (&ref_keyring_map_builder, "(sss)",
data->collection_ref.collection_id,
data->collection_ref.ref_name,
resolve->remote);
}
g_variant_builder_add (&pull_builder, "{s@v}", "ref-keyring-map",
g_variant_new_variant (g_variant_builder_end (&ref_keyring_map_builder)));

pull_options = g_variant_ref_sink (g_variant_builder_end (&pull_builder));

transaction = flatpak_repo_transaction_start (child_repo, cancellable, error);
Expand Down Expand Up @@ -3979,13 +3997,20 @@ repo_pull (OstreeRepo *self,

if (results_to_fetch != NULL)
{
GVariantBuilder pull_builder;
GVariantBuilder pull_builder, ref_keyring_map_builder;
g_autoptr(GVariant) pull_options = NULL;

/* Pull options */
g_variant_builder_init (&pull_builder, G_VARIANT_TYPE ("a{sv}"));
get_common_pull_options (&pull_builder, ref_to_fetch, dirs_to_pull, current_checksum,
force_disable_deltas, flags, progress);

/* Ensure the results are signed with the GPG keys associated with the correct remote */
g_variant_builder_init (&ref_keyring_map_builder, G_VARIANT_TYPE ("a(sss)"));
g_variant_builder_add (&ref_keyring_map_builder, "(sss)", collection_id, ref_to_fetch, remote_name);
g_variant_builder_add (&pull_builder, "{s@v}", "ref-keyring-map",
g_variant_new_variant (g_variant_builder_end (&ref_keyring_map_builder)));

pull_options = g_variant_ref_sink (g_variant_builder_end (&pull_builder));

ostree_repo_pull_from_remotes_async (self, results_to_fetch,
Expand Down

0 comments on commit 477172d

Please sign in to comment.