lib/repo-refs: Resolve collection-refs in-memory and in parent repos #1821

mwleeds · 2019-02-14T21:19:25Z

Currently the behavior of ostree_repo_resolve_rev() is that it tries to
resolve a ref to a commit by checking the refs/ directories, but also by
checking for in-memory ref-checksum pairs which are part of an
in-progress transaction and also by checking the parent repo if one
exists. Currently ostree_repo_resolve_collection_ref() only checks the
refs/ directories, so this commit makes its behavior analagous since it
is the analagous API which supports collection-refs.

The impetus for this was that currently Flatpak uses
ostree_repo_resolve_rev() to load a commit after doing a P2P pull in
flatpak_dir_do_resolve_p2p_refs(), but that assumes the ref came from
the same remote that originally provided it, which might not be the case
if more than one remote has the same collection ID configured. And
changing Flatpak to use ostree_repo_resolve_collection_ref() doesn't
work without this patch.

Currently the behavior of ostree_repo_resolve_rev() is that it tries to resolve a ref to a commit by checking the refs/ directories, but also by checking for in-memory ref-checksum pairs which are part of an in-progress transaction and also by checking the parent repo if one exists. Currently ostree_repo_resolve_collection_ref() only checks the refs/ directories, so this commit makes its behavior analagous since it is the analagous API which supports collection-refs. The impetus for this was that currently Flatpak uses ostree_repo_resolve_rev() to load a commit after doing a P2P pull in flatpak_dir_do_resolve_p2p_refs(), but that assumes the ref came from the same remote that originally provided it, which might not be the case if more than one remote has the same collection ID configured. And changing Flatpak to use ostree_repo_resolve_collection_ref() doesn't work without this patch.

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]. [1] flatpak#1447 (comment) [2] flatpak#1832 [3] ostreedev/ostree#1821

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

pwithnall · 2019-02-18T13:41:54Z

The test failure is that vmsync thing again, which is unrelated.

pwithnall · 2019-02-18T13:42:00Z

@rh-atomic-bot, r+ 36933d9

rh-atomic-bot · 2019-02-18T13:42:11Z

⚡ Test exempted: pull fully rebased and already tested.

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 repo_resolve_rev() which falls back to using ostree_repo_resolve_rev() when ostree_repo_resolve_collection_ref() fails, and use it pretty much everywhere Flatpak resolves a ref. This way we start to move toward using /refs/mirrors/ but maintain backwards compatibility for /refs/remotes/. [1] flatpak#1447 (comment) [2] flatpak#1832 [3] ostreedev/ostree#1821

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

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 repo_resolve_rev() which falls back to using ostree_repo_resolve_rev() when ostree_repo_resolve_collection_ref() fails, and use it pretty much everywhere Flatpak resolves a ref. This way we start to move toward using /refs/mirrors/ but maintain backwards compatibility for /refs/remotes/. [1] flatpak#1447 (comment) [2] flatpak#1832 [3] ostreedev/ostree#1821

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

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