From f1e3df316992d32b17ee27385e4c644ef7b66cc1 Mon Sep 17 00:00:00 2001 From: Hans Jerry Illikainen Date: Wed, 4 Mar 2020 11:48:03 +0000 Subject: t: increase test coverage of signature verification output There weren't any tests for unsuccessful signature verification of signed merge tags shown in 'git log'. There also weren't any tests for the GPG output from 'git fmt-merge-msg'. This was noticed while investigating a buggy refactor that slipped through the test suite; see commit 72b006f4bfd30b7c5037c163efaf279ab65bea9c. This commit adds signature verification tests to the 'log' and 'fmt-merge-msg' builtins. Thanks to Linus Torvalds for reporting and finding the (now reverted) commit that introduced the regression. Note that the "log --show-signature for merged tag with GPG failure" test case is really hacky. It relies on an implementation detail of verify_signed_buffer() -- namely, it assumes that the signature is written to a temporary file whose path is under TMPDIR. The rationale for that test case is to check whether the code path that yields the "No signature" message is reachable on failure. The functionality in log-tree.c that may show this message does some pre-parsing of a possible signature that prevents the GPG interface from being invoked if a signature is actually missing. And I haven't been able to construct a signature that both 1. satisfies that pre-processing, and 2. causes GPG to fail without any sort of output on stderr along the lines of "this is a bogus/corrupt/... signature" (the "No signature" message should only be shown if GPG produce no output). Signed-off-by: Hans Jerry Illikainen [jc: fixed missing test title noticed by Dscho] Signed-off-by: Junio C Hamano diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 4694b6d..28145fe 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -1607,6 +1607,66 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' ' grep "^| | gpg: Good signature" actual ' +test_expect_success GPG 'log --graph --show-signature for merged tag with missing key' ' + test_when_finished "git reset --hard && git checkout master" && + git checkout -b plain-nokey master && + echo aaa >bar && + git add bar && + git commit -m bar_commit && + git checkout -b tagged-nokey master && + echo bbb >baz && + git add baz && + git commit -m baz_commit && + git tag -s -m signed_tag_msg signed_tag_nokey && + git checkout plain-nokey && + git merge --no-ff -m msg signed_tag_nokey && + GNUPGHOME=. git log --graph --show-signature -n1 plain-nokey >actual && + grep "^|\\\ merged tag" actual && + grep "^| | gpg: Signature made" actual && + grep "^| | gpg: Can'"'"'t check signature: \(public key not found\|No public key\)" actual +' + +test_expect_success GPG 'log --graph --show-signature for merged tag with bad signature' ' + test_when_finished "git reset --hard && git checkout master" && + git checkout -b plain-bad master && + echo aaa >bar && + git add bar && + git commit -m bar_commit && + git checkout -b tagged-bad master && + echo bbb >baz && + git add baz && + git commit -m baz_commit && + git tag -s -m signed_tag_msg signed_tag_bad && + git cat-file tag signed_tag_bad >raw && + sed -e "s/signed_tag_msg/forged/" raw >forged && + git hash-object -w -t tag forged >forged.tag && + git checkout plain-bad && + git merge --no-ff -m msg "$(cat forged.tag)" && + git log --graph --show-signature -n1 plain-bad >actual && + grep "^|\\\ merged tag" actual && + grep "^| | gpg: Signature made" actual && + grep "^| | gpg: BAD signature from" actual +' + +test_expect_success GPG 'log --show-signature for merged tag with GPG failure' ' + test_when_finished "git reset --hard && git checkout master" && + git checkout -b plain-fail master && + echo aaa >bar && + git add bar && + git commit -m bar_commit && + git checkout -b tagged-fail master && + echo bbb >baz && + git add baz && + git commit -m baz_commit && + git tag -s -m signed_tag_msg signed_tag_fail && + git checkout plain-fail && + git merge --no-ff -m msg signed_tag_fail && + TMPDIR="$(pwd)/bogus" git log --show-signature -n1 plain-fail >actual && + grep "^merged tag" actual && + grep "^No signature" actual && + ! grep "^gpg: Signature made" actual +' + test_expect_success GPGSM 'log --graph --show-signature for merged tag x509' ' test_when_finished "git reset --hard && git checkout master" && test_config gpg.format x509 && @@ -1628,6 +1688,51 @@ test_expect_success GPGSM 'log --graph --show-signature for merged tag x509' ' grep "^| | gpgsm: Good signature" actual ' +test_expect_success GPGSM 'log --graph --show-signature for merged tag x509 missing key' ' + test_when_finished "git reset --hard && git checkout master" && + test_config gpg.format x509 && + test_config user.signingkey $GIT_COMMITTER_EMAIL && + git checkout -b plain-x509-nokey master && + echo aaa >bar && + git add bar && + git commit -m bar_commit && + git checkout -b tagged-x509-nokey master && + echo bbb >baz && + git add baz && + git commit -m baz_commit && + git tag -s -m signed_tag_msg signed_tag_x509_nokey && + git checkout plain-x509-nokey && + git merge --no-ff -m msg signed_tag_x509_nokey && + GNUPGHOME=. git log --graph --show-signature -n1 plain-x509-nokey >actual && + grep "^|\\\ merged tag" actual && + grep "^| | gpgsm: certificate not found" actual +' + +test_expect_success GPGSM 'log --graph --show-signature for merged tag x509 bad signature' ' + test_when_finished "git reset --hard && git checkout master" && + test_config gpg.format x509 && + test_config user.signingkey $GIT_COMMITTER_EMAIL && + git checkout -b plain-x509-bad master && + echo aaa >bar && + git add bar && + git commit -m bar_commit && + git checkout -b tagged-x509-bad master && + echo bbb >baz && + git add baz && + git commit -m baz_commit && + git tag -s -m signed_tag_msg signed_tag_x509_bad && + git cat-file tag signed_tag_x509_bad >raw && + sed -e "s/signed_tag_msg/forged/" raw >forged && + git hash-object -w -t tag forged >forged.tag && + git checkout plain-x509-bad && + git merge --no-ff -m msg "$(cat forged.tag)" && + git log --graph --show-signature -n1 plain-x509-bad >actual && + grep "^|\\\ merged tag" actual && + grep "^| | gpgsm: Signature made" actual && + grep "^| | gpgsm: invalid signature" actual +' + + test_expect_success GPG '--no-show-signature overrides --show-signature' ' git log -1 --show-signature --no-show-signature signed >actual && ! grep "^gpg:" actual diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh index 8a72b4c..b15582a 100755 --- a/t/t6200-fmt-merge-msg.sh +++ b/t/t6200-fmt-merge-msg.sh @@ -6,6 +6,7 @@ test_description='fmt-merge-msg test' . ./test-lib.sh +. "$TEST_DIRECTORY/lib-gpg.sh" test_expect_success setup ' echo one >one && @@ -73,6 +74,10 @@ test_expect_success setup ' apos="'\''" ' +test_expect_success GPG 'set up a signed tag' ' + git tag -s -m signed-tag-msg signed-good-tag left +' + test_expect_success 'message for merging local branch' ' echo "Merge branch ${apos}left${apos}" >expected && @@ -83,6 +88,24 @@ test_expect_success 'message for merging local branch' ' test_cmp expected actual ' +test_expect_success GPG 'message for merging local tag signed by good key' ' + git checkout master && + git fetch . signed-good-tag && + git fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 && + grep "^Merge tag ${apos}signed-good-tag${apos}" actual && + grep "^# gpg: Signature made" actual && + grep "^# gpg: Good signature from" actual +' + +test_expect_success GPG 'message for merging local tag signed by unknown key' ' + git checkout master && + git fetch . signed-good-tag && + GNUPGHOME=. git fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 && + grep "^Merge tag ${apos}signed-good-tag${apos}" actual && + grep "^# gpg: Signature made" actual && + grep "^# gpg: Can${apos}t check signature: \(public key not found\|No public key\)" actual +' + test_expect_success 'message for merging external branch' ' echo "Merge branch ${apos}left${apos} of $(pwd)" >expected && -- cgit v0.10.2-6-g49f6 From 67948981983b336eab2fa7e6a0e125d529391dfc Mon Sep 17 00:00:00 2001 From: Hans Jerry Illikainen Date: Wed, 4 Mar 2020 11:48:04 +0000 Subject: gpg-interface: prefer check_signature() for GPG verification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit refactors the use of verify_signed_buffer() outside of gpg-interface.c to use check_signature() instead. It also turns verify_signed_buffer() into a file-local function since it's now only invoked internally by check_signature(). There were previously two globally scoped functions used in different parts of Git to perform GPG signature verification: verify_signed_buffer() and check_signature(). Now only check_signature() is used. The verify_signed_buffer() function doesn't guard against duplicate signatures as described by Michał Górny [1]. Instead it only ensures a non-erroneous exit code from GPG and the presence of at least one GOODSIG status field. This stands in contrast with check_signature() that returns an error if more than one signature is encountered. The lower degree of verification makes the use of verify_signed_buffer() problematic if callers don't parse and validate the various parts of the GPG status message themselves. And processing these messages seems like a task that should be reserved to gpg-interface.c with the function check_signature(). Furthermore, the use of verify_signed_buffer() makes it difficult to introduce new functionality that relies on the content of the GPG status lines. Now all operations that does signature verification share a single entry point to gpg-interface.c. This makes it easier to propagate changed or additional functionality in GPG signature verification to all parts of Git, without having odd edge-cases that don't perform the same degree of verification. [1] https://dev.gentoo.org/~mgorny/articles/attack-on-git-signature-verification.html Signed-off-by: Hans Jerry Illikainen Signed-off-by: Junio C Hamano diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 736f666..172dfbd 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -494,6 +494,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out) enum object_type type; unsigned long size, len; char *buf = read_object_file(oid, &type, &size); + struct signature_check sigc = { 0 }; struct strbuf sig = STRBUF_INIT; if (!buf || type != OBJ_TAG) @@ -502,10 +503,12 @@ static void fmt_merge_msg_sigs(struct strbuf *out) if (size == len) ; /* merely annotated */ - else if (verify_signed_buffer(buf, len, buf + len, size - len, &sig, NULL)) { - if (!sig.len) - strbuf_addstr(&sig, "gpg verification failed.\n"); - } + else if (check_signature(buf, len, buf + len, size - len, &sigc) && + !sigc.gpg_output) + strbuf_addstr(&sig, "gpg verification failed.\n"); + else + strbuf_addstr(&sig, sigc.gpg_output); + signature_check_clear(&sigc); if (!tag_number++) { fmt_tag_signature(&tagbuf, &sig, buf, len); diff --git a/gpg-interface.c b/gpg-interface.c index 165274d..2d538bc 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -256,6 +256,55 @@ error: FREE_AND_NULL(sigc->key); } +static int verify_signed_buffer(const char *payload, size_t payload_size, + const char *signature, size_t signature_size, + struct strbuf *gpg_output, + struct strbuf *gpg_status) +{ + struct child_process gpg = CHILD_PROCESS_INIT; + struct gpg_format *fmt; + struct tempfile *temp; + int ret; + struct strbuf buf = STRBUF_INIT; + + temp = mks_tempfile_t(".git_vtag_tmpXXXXXX"); + if (!temp) + return error_errno(_("could not create temporary file")); + if (write_in_full(temp->fd, signature, signature_size) < 0 || + close_tempfile_gently(temp) < 0) { + error_errno(_("failed writing detached signature to '%s'"), + temp->filename.buf); + delete_tempfile(&temp); + return -1; + } + + fmt = get_format_by_sig(signature); + if (!fmt) + BUG("bad signature '%s'", signature); + + argv_array_push(&gpg.args, fmt->program); + argv_array_pushv(&gpg.args, fmt->verify_args); + argv_array_pushl(&gpg.args, + "--status-fd=1", + "--verify", temp->filename.buf, "-", + NULL); + + if (!gpg_status) + gpg_status = &buf; + + sigchain_push(SIGPIPE, SIG_IGN); + ret = pipe_command(&gpg, payload, payload_size, + gpg_status, 0, gpg_output, 0); + sigchain_pop(SIGPIPE); + + delete_tempfile(&temp); + + ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG "); + strbuf_release(&buf); /* no matter it was used or not */ + + return ret; +} + int check_signature(const char *payload, size_t plen, const char *signature, size_t slen, struct signature_check *sigc) { @@ -418,51 +467,3 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig return 0; } - -int verify_signed_buffer(const char *payload, size_t payload_size, - const char *signature, size_t signature_size, - struct strbuf *gpg_output, struct strbuf *gpg_status) -{ - struct child_process gpg = CHILD_PROCESS_INIT; - struct gpg_format *fmt; - struct tempfile *temp; - int ret; - struct strbuf buf = STRBUF_INIT; - - temp = mks_tempfile_t(".git_vtag_tmpXXXXXX"); - if (!temp) - return error_errno(_("could not create temporary file")); - if (write_in_full(temp->fd, signature, signature_size) < 0 || - close_tempfile_gently(temp) < 0) { - error_errno(_("failed writing detached signature to '%s'"), - temp->filename.buf); - delete_tempfile(&temp); - return -1; - } - - fmt = get_format_by_sig(signature); - if (!fmt) - BUG("bad signature '%s'", signature); - - argv_array_push(&gpg.args, fmt->program); - argv_array_pushv(&gpg.args, fmt->verify_args); - argv_array_pushl(&gpg.args, - "--status-fd=1", - "--verify", temp->filename.buf, "-", - NULL); - - if (!gpg_status) - gpg_status = &buf; - - sigchain_push(SIGPIPE, SIG_IGN); - ret = pipe_command(&gpg, payload, payload_size, - gpg_status, 0, gpg_output, 0); - sigchain_pop(SIGPIPE); - - delete_tempfile(&temp); - - ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG "); - strbuf_release(&buf); /* no matter it was used or not */ - - return ret; -} diff --git a/gpg-interface.h b/gpg-interface.h index 796571e..f4e9b4f 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -54,15 +54,6 @@ size_t parse_signature(const char *buf, size_t size); int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key); -/* - * Run "gpg" to see if the payload matches the detached signature. - * gpg_output, when set, receives the diagnostic output from GPG. - * gpg_status, when set, receives the status output from GPG. - */ -int verify_signed_buffer(const char *payload, size_t payload_size, - const char *signature, size_t signature_size, - struct strbuf *gpg_output, struct strbuf *gpg_status); - int git_gpg_config(const char *, const char *, void *); void set_signing_key(const char *); const char *get_signing_key(void); diff --git a/log-tree.c b/log-tree.c index 5425ae9..f1ac5a6 100644 --- a/log-tree.c +++ b/log-tree.c @@ -449,22 +449,21 @@ static void show_signature(struct rev_info *opt, struct commit *commit) { struct strbuf payload = STRBUF_INIT; struct strbuf signature = STRBUF_INIT; - struct strbuf gpg_output = STRBUF_INIT; + struct signature_check sigc = { 0 }; int status; if (parse_signed_commit(commit, &payload, &signature) <= 0) goto out; - status = verify_signed_buffer(payload.buf, payload.len, - signature.buf, signature.len, - &gpg_output, NULL); - if (status && !gpg_output.len) - strbuf_addstr(&gpg_output, "No signature\n"); - - show_sig_lines(opt, status, gpg_output.buf); + status = check_signature(payload.buf, payload.len, signature.buf, + signature.len, &sigc); + if (status && !sigc.gpg_output) + show_sig_lines(opt, status, "No signature\n"); + else + show_sig_lines(opt, status, sigc.gpg_output); + signature_check_clear(&sigc); out: - strbuf_release(&gpg_output); strbuf_release(&payload); strbuf_release(&signature); } @@ -497,8 +496,9 @@ static int show_one_mergetag(struct commit *commit, struct object_id oid; struct tag *tag; struct strbuf verify_message; + struct signature_check sigc = { 0 }; int status, nth; - size_t payload_size, gpg_message_offset; + size_t payload_size; hash_object_file(the_hash_algo, extra->value, extra->len, type_name(OBJ_TAG), &oid); @@ -520,19 +520,19 @@ static int show_one_mergetag(struct commit *commit, else strbuf_addf(&verify_message, "parent #%d, tagged '%s'\n", nth + 1, tag->tag); - gpg_message_offset = verify_message.len; payload_size = parse_signature(extra->value, extra->len); status = -1; if (extra->len > payload_size) { /* could have a good signature */ - if (!verify_signed_buffer(extra->value, payload_size, - extra->value + payload_size, - extra->len - payload_size, - &verify_message, NULL)) - status = 0; /* good */ - else if (verify_message.len <= gpg_message_offset) + status = check_signature(extra->value, payload_size, + extra->value + payload_size, + extra->len - payload_size, &sigc); + if (sigc.gpg_output) + strbuf_addstr(&verify_message, sigc.gpg_output); + else strbuf_addstr(&verify_message, "No signature\n"); + signature_check_clear(&sigc); /* otherwise we couldn't verify, which is shown as bad */ } -- cgit v0.10.2-6-g49f6