From 720e3b5f834056d3fa978f84c85cdf15274d5c75 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Sat, 27 Jun 2015 12:00:49 -0400 Subject: [PATCH] pull: Error if gpg=true and summary is 404, add more tests I did a quick audit pass through the pull code. What I focused on the most is the case where `gpg-verify-summary=true`, and in particular where `gpg-verify=false` too. This should be a valid and secure configuration. The primary change here is to error out very quickly if either `summary` or `summary.sig` are 404. Previously, we'd only error out if we were processing deltas. Expand the existing test case to cover this, plus invalid summary and invalid sig. (The test case was failing with current git master too). --- src/libostree/ostree-repo-pull.c | 45 ++++++++++++++++++++++---------- tests/test-pull-summary-sigs.sh | 40 ++++++++++++++++++++++------ 2 files changed, 63 insertions(+), 22 deletions(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 96c3a188..29b72bf4 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -1318,7 +1318,10 @@ request_static_delta_superblock_sync (OtPullData *pull_data, delta = g_strconcat (from_revision ? from_revision : "", from_revision ? "-" : "", to_revision, NULL); summary_csum = g_hash_table_lookup (pull_data->summary_deltas_checksums, delta); - + /* At this point we've GPG verified the data, so in theory + * could trust that they provided the right data, but let's + * make this a hard error. + */ if (pull_data->gpg_verify_summary && !summary_csum) { g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, @@ -1818,27 +1821,41 @@ ostree_repo_pull_with_options (OstreeRepo *self, soup_uri_free (uri); } + if (!bytes_summary && pull_data->gpg_verify_summary) + { + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, + "GPG verification enabled, but no summary found (use gpg-verify-summary=false in remote config to disable)"); + goto out; + } + if (bytes_summary) { - g_autoptr(GVariant) sig_variant = NULL; - glnx_unref_object OstreeGpgVerifyResult *result = NULL; - pull_data->summary_data = g_bytes_ref (bytes_summary); - pull_data->summary = g_variant_new_from_bytes (OSTREE_SUMMARY_GVARIANT_FORMAT, bytes_summary, FALSE); - uri = suburi_new (pull_data->base_uri, "summary.sig", NULL); if (!fetch_uri_contents_membuf_sync (pull_data, uri, FALSE, TRUE, &bytes_sig, cancellable, error)) goto out; soup_uri_free (uri); + } - if (bytes_sig) - pull_data->summary_data_sig = g_bytes_ref (bytes_sig); - else - { - g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, - "GPG verification enabled, but no summary signatures found (use gpg-verify-summary=false in remote config to disable)"); - goto out; - } + if (!bytes_sig && pull_data->gpg_verify_summary) + { + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, + "GPG verification enabled, but no summary.sig found (use gpg-verify-summary=false in remote config to disable)"); + goto out; + } + + if (bytes_summary) + { + pull_data->summary_data = g_bytes_ref (bytes_summary); + pull_data->summary = g_variant_new_from_bytes (OSTREE_SUMMARY_GVARIANT_FORMAT, bytes_summary, FALSE); + } + + if (bytes_summary && bytes_sig) + { + g_autoptr(GVariant) sig_variant = NULL; + glnx_unref_object OstreeGpgVerifyResult *result = NULL; + + pull_data->summary_data_sig = g_bytes_ref (bytes_sig); sig_variant = g_variant_new_from_bytes (OSTREE_SUMMARY_SIG_GVARIANT_FORMAT, bytes_sig, FALSE); result = _ostree_repo_gpg_verify_with_metadata (self, diff --git a/tests/test-pull-summary-sigs.sh b/tests/test-pull-summary-sigs.sh index a57a9eb1..b0acf460 100644 --- a/tests/test-pull-summary-sigs.sh +++ b/tests/test-pull-summary-sigs.sh @@ -60,22 +60,46 @@ cd $prev_dir ${OSTREE} --repo=${test_tmpdir}/ostree-srv/gnomerepo summary -u ${COMMIT_SIGN} +repo_reinit () { + cd ${test_tmpdir} + rm -rf repo + mkdir repo + ${OSTREE} --repo=repo init --mode=archive-z2 + ${OSTREE} --repo=repo remote add --set=gpg-verify-summary=true origin $(cat httpd-address)/ostree/gnomerepo +} + cd ${test_tmpdir} -rm -rf repo -mkdir repo -${OSTREE} --repo=repo init --mode=archive-z2 -${OSTREE} --repo=repo remote add origin $(cat httpd-address)/ostree/gnomerepo +repo_reinit ${OSTREE} --repo=repo pull origin main echo "ok pull with signed summary" +cd ${test_tmpdir} +repo_reinit +mv ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{,.good} +echo invalid > ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig +if ${OSTREE} --repo=repo pull origin main 2>err.txt; then + assert_not_reached "Successful pull with invalid GPG sig" +fi +assert_file_has_content err.txt "no signatures found" +mv ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.good,} +echo "ok pull with invalid summary gpg signature fails" + +cd ${test_tmpdir} +repo_reinit +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary{,.good} +# Some leading garbage +(echo invalid && cat ${test_tmpdir}/ostree-srv/gnomerepo/summary) > summary.bad.tmp && mv summary.bad.tmp ${test_tmpdir}/ostree-srv/gnomerepo/summary +if ${OSTREE} --repo=repo pull origin main; then + assert_not_reached "Successful pull with invalid summary" +fi +mv ${test_tmpdir}/ostree-srv/gnomerepo/summary{.good,} +echo "ok pull with invalid summary (leading garbage) fails" + # Generate a delta ${OSTREE} --repo=${test_tmpdir}/ostree-srv/gnomerepo static-delta generate --empty main ${OSTREE} --repo=${test_tmpdir}/ostree-srv/gnomerepo summary -u ${COMMIT_SIGN} cd ${test_tmpdir} -rm -rf repo -mkdir repo -${OSTREE} --repo=repo init --mode=archive-z2 -${OSTREE} --repo=repo remote add origin $(cat httpd-address)/ostree/gnomerepo +repo_reinit ${OSTREE} --repo=repo pull origin main echo "ok pull delta with signed summary"