{"thread":{"id":"49764","subject":"[PATCH 0/3] fix pull/merge --verify-signature on an unborn branch","startedAt":"2018-11-06T07:49:13Z","lastAt":"2018-11-06T07:52:16Z","messageCount":4,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"362560","messageId":"20181106074910.GA31978@sigill.intra.peff.net","threadId":"49764","inReplyTo":null,"subject":"[PATCH 0/3] fix pull/merge --verify-signature on an unborn branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-06T07:49:10Z","receivedAt":"2018-11-06T07:49:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This bug was reported to the private security list, but I don't think\nit's easily exploitable, since merging or pulling into an unborn branch\nis pretty uncommon.\n\nThe root of the issue in both commands is just that we handle unborn\nbranches in a special code path that never learned about\n--verify-signatures.\n\n  [1/3]: merge: extract verify_merge_signature() helper\n  [2/3]: merge: handle --verify-signatures for unborn branch\n  [3/3]: pull: handle --verify-signatures for unborn branch\n\n builtin/merge.c                    | 30 +++++-------------------------\n builtin/pull.c                     | 11 +++++++++++\n commit.c                           | 26 ++++++++++++++++++++++++++\n commit.h                           |  7 +++++++\n t/t5573-pull-verify-signatures.sh  |  7 +++++++\n t/t7612-merge-verify-signatures.sh |  7 +++++++\n 6 files changed, 63 insertions(+), 25 deletions(-)\n\n-Peff\n"},{"id":"362561","messageId":"20181106075017.GA22021@sigill.intra.peff.net","threadId":"49764","inReplyTo":"20181106074910.GA31978@sigill.intra.peff.net","subject":"[PATCH 1/3] merge: extract verify_merge_signature() helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-06T07:50:17Z","receivedAt":"2018-11-06T07:50:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The logic to implement \"merge --verify-signatures\" is inline in\ncmd_merge(), but this site misses some cases. Let's extract the logic\ninto a function so we can call it from more places.\n\nWe'll move it to commit.[ch], since one of the callers (git-pull) is\noutside our source file. This function isn't all that general (after\nall, its main function is to exit the program) but it's not worth trying\nto fix that. The heavy lifting is done by check_commit_signature(), and\nour purpose here is just sharing the die() logic. We'll mark it with a\ncomment to make that clear.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/merge.c | 26 +-------------------------\n commit.c        | 26 ++++++++++++++++++++++++++\n commit.h        |  7 +++++++\n 3 files changed, 34 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 4aa6071598..c677f9375e 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1357,31 +1357,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \n \tif (verify_signatures) {\n \t\tfor (p = remoteheads; p; p = p->next) {\n-\t\t\tstruct commit *commit = p->item;\n-\t\t\tchar hex[GIT_MAX_HEXSZ + 1];\n-\t\t\tstruct signature_check signature_check;\n-\t\t\tmemset(&signature_check, 0, sizeof(signature_check));\n-\n-\t\t\tcheck_commit_signature(commit, &signature_check);\n-\n-\t\t\tfind_unique_abbrev_r(hex, &commit->object.oid, DEFAULT_ABBREV);\n-\t\t\tswitch (signature_check.result) {\n-\t\t\tcase 'G':\n-\t\t\t\tbreak;\n-\t\t\tcase 'U':\n-\t\t\t\tdie(_(\"Commit %s has an untrusted GPG signature, \"\n-\t\t\t\t      \"allegedly by %s.\"), hex, signature_check.signer);\n-\t\t\tcase 'B':\n-\t\t\t\tdie(_(\"Commit %s has a bad GPG signature \"\n-\t\t\t\t      \"allegedly by %s.\"), hex, signature_check.signer);\n-\t\t\tdefault: /* 'N' */\n-\t\t\t\tdie(_(\"Commit %s does not have a GPG signature.\"), hex);\n-\t\t\t}\n-\t\t\tif (verbosity >= 0 && signature_check.result == 'G')\n-\t\t\t\tprintf(_(\"Commit %s has a good GPG signature by %s\\n\"),\n-\t\t\t\t       hex, signature_check.signer);\n-\n-\t\t\tsignature_check_clear(&signature_check);\n+\t\t\tverify_merge_signature(p->item, verbosity);\n \t\t}\n \t}\n \ndiff --git a/commit.c b/commit.c\nindex d566d7e45c..e1428aed6d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1100,7 +1100,33 @@ int check_commit_signature(const struct commit *commit, struct signature_check *\n \treturn ret;\n }\n \n+void verify_merge_signature(struct commit *commit, int verbosity)\n+{\n+\tchar hex[GIT_MAX_HEXSZ + 1];\n+\tstruct signature_check signature_check;\n+\tmemset(&signature_check, 0, sizeof(signature_check));\n+\n+\tcheck_commit_signature(commit, &signature_check);\n+\n+\tfind_unique_abbrev_r(hex, &commit->object.oid, DEFAULT_ABBREV);\n+\tswitch (signature_check.result) {\n+\tcase 'G':\n+\t\tbreak;\n+\tcase 'U':\n+\t\tdie(_(\"Commit %s has an untrusted GPG signature, \"\n+\t\t      \"allegedly by %s.\"), hex, signature_check.signer);\n+\tcase 'B':\n+\t\tdie(_(\"Commit %s has a bad GPG signature \"\n+\t\t      \"allegedly by %s.\"), hex, signature_check.signer);\n+\tdefault: /* 'N' */\n+\t\tdie(_(\"Commit %s does not have a GPG signature.\"), hex);\n+\t}\n+\tif (verbosity >= 0 && signature_check.result == 'G')\n+\t\tprintf(_(\"Commit %s has a good GPG signature by %s\\n\"),\n+\t\t       hex, signature_check.signer);\n \n+\tsignature_check_clear(&signature_check);\n+}\n \n void append_merge_tag_headers(struct commit_list *parents,\n \t\t\t      struct commit_extra_header ***tail)\ndiff --git a/commit.h b/commit.h\nindex 6c4428c593..a98cca635a 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -331,6 +331,13 @@ extern int remove_signature(struct strbuf *buf);\n  */\n extern int check_commit_signature(const struct commit *commit, struct signature_check *sigc);\n \n+/*\n+ * Verify a single commit with check_commit_signature() and die() if it is not\n+ * a good signature. This isn't really suitable for general use, but is a\n+ * helper to implement consistent logic for pull/merge --verify-signatures.\n+ */\n+void verify_merge_signature(struct commit *commit, int verbose);\n+\n int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused);\n int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused);\n \n-- \n2.19.1.1533.g982fead9fb\n\n"},{"id":"362562","messageId":"20181106075115.GB22021@sigill.intra.peff.net","threadId":"49764","inReplyTo":"20181106074910.GA31978@sigill.intra.peff.net","subject":"[PATCH 2/3] merge: handle --verify-signatures for unborn branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-06T07:51:15Z","receivedAt":"2018-11-06T07:51:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When git-merge sees that we are on an unborn branch (i.e., there is no\nHEAD), it follows a totally separate code path than the usual merge\nlogic. This code path does not know about verify_signatures, and so we\nfail to notice bad or missing signatures.\n\nThis has been broken since --verify-signatures was added in efed002249\n(merge/pull: verify GPG signatures of commits being merged, 2013-03-31).\nIn an ideal world, we'd unify the flow for this case with the regular\nmerge logic, which would fix this bug and avoid introducing similar\nones. But because the unborn case is so different, it would be a burden\non the rest of the function to continually handle the missing HEAD. So\nlet's just port the verification check to this special case.\n\nReported-by: Felix Eckhofer <felix@eckhofer.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/merge.c                    | 4 ++++\n t/t7612-merge-verify-signatures.sh | 7 +++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex c677f9375e..be156b122d 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1336,6 +1336,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"%s - not something we can merge\"), argv[0]);\n \t\tif (remoteheads->next)\n \t\t\tdie(_(\"Can merge only exactly one commit into empty head\"));\n+\n+\t\tif (verify_signatures)\n+\t\t\tverify_merge_signature(remoteheads->item, verbosity);\n+\n \t\tremote_head_oid = &remoteheads->item->object.oid;\n \t\tread_empty(remote_head_oid, 0);\n \t\tupdate_ref(\"initial pull\", \"HEAD\", remote_head_oid, NULL, 0,\ndiff --git a/t/t7612-merge-verify-signatures.sh b/t/t7612-merge-verify-signatures.sh\nindex e2b1df817a..d99218a725 100755\n--- a/t/t7612-merge-verify-signatures.sh\n+++ b/t/t7612-merge-verify-signatures.sh\n@@ -103,4 +103,11 @@ test_expect_success GPG 'merge commit with bad signature with merge.verifySignat\n \tgit merge --no-verify-signatures $(cat forged.commit)\n '\n \n+test_expect_success GPG 'merge unsigned commit into unborn branch' '\n+\ttest_when_finished \"git checkout initial\" &&\n+\tgit checkout --orphan unborn &&\n+\ttest_must_fail git merge --verify-signatures side-unsigned 2>mergeerror &&\n+\ttest_i18ngrep \"does not have a GPG signature\" mergeerror\n+'\n+\n test_done\n-- \n2.19.1.1533.g982fead9fb\n\n"},{"id":"362563","messageId":"20181106075212.GC22021@sigill.intra.peff.net","threadId":"49764","inReplyTo":"20181106074910.GA31978@sigill.intra.peff.net","subject":"[PATCH 3/3] pull: handle --verify-signatures for unborn branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-06T07:52:13Z","receivedAt":"2018-11-06T07:52:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We usually just forward the --verify-signatures option along to\ngit-merge, and trust it to do the right thing. However, when we are on\nan unborn branch (i.e., there is no HEAD yet), we handle this case\nourselves without even calling git-merge. And in this code path, we do\nnot respect the verification option at all.\n\nIt may be more maintainable in the long run to call git-merge for the\nunborn case. That would fix this bug, as well as prevent similar ones in\nthe future. But unfortunately it's not easy to do. As t5520.3\ndemonstrates, there are some special cases that git-merge does not\nhandle, like \"git pull .. master:master\" (by the time git-merge is\ninvoked, we've overwritten the unborn HEAD).\n\nSo for now let's just teach git-pull to handle this feature.\n\nReported-by: Felix Eckhofer <felix@eckhofer.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pull.c                    | 11 +++++++++++\n t/t5573-pull-verify-signatures.sh |  7 +++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 798ecf7faf..91616cf7d6 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -557,6 +557,17 @@ static int run_fetch(const char *repo, const char **refspecs)\n static int pull_into_void(const struct object_id *merge_head,\n \t\tconst struct object_id *curr_head)\n {\n+\tif (opt_verify_signatures) {\n+\t\tstruct commit *commit;\n+\n+\t\tcommit = lookup_commit(the_repository, merge_head);\n+\t\tif (!commit)\n+\t\t\tdie(_(\"unable to access commit %s\"),\n+\t\t\t    oid_to_hex(merge_head));\n+\n+\t\tverify_merge_signature(commit, opt_verbosity);\n+\t}\n+\n \t/*\n \t * Two-way merge: we treat the index as based on an empty tree,\n \t * and try to fast-forward to HEAD. This ensures we will not lose\ndiff --git a/t/t5573-pull-verify-signatures.sh b/t/t5573-pull-verify-signatures.sh\nindex 747775c147..3e9876e197 100755\n--- a/t/t5573-pull-verify-signatures.sh\n+++ b/t/t5573-pull-verify-signatures.sh\n@@ -78,4 +78,11 @@ test_expect_success GPG 'pull commit with bad signature with --no-verify-signatu\n \tgit pull --ff-only --no-verify-signatures bad 2>pullerror\n '\n \n+test_expect_success GPG 'pull unsigned commit into unborn branch' '\n+\tgit init empty-repo &&\n+\ttest_must_fail \\\n+\t\tgit -C empty-repo pull --verify-signatures ..  2>pullerror &&\n+\ttest_i18ngrep \"does not have a GPG signature\" pullerror\n+'\n+\n test_done\n-- \n2.19.1.1533.g982fead9fb\n"}]}