Re: [PATCH v1 0/2] gpg-interface: prefer check_signature() for GPG verification

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2609 bytes --]

Hi Junio,

On Thu, 5 Mar 2020, Junio C Hamano wrote:

> Hans Jerry Illikainen <hji@dyntopia.com> writes:
>
> > This patch refactors the use of verify_signed_buffer() for GPG
> > verification to use check_signature() instead.
> >
> > Previously, both check_signature() and verify_signed_buffer() were used
> > to verify signatures in various parts of Git.  However,
> > verify_signed_buffer() does not parse the GPG status message.  Instead,
> > it relies entirely on the exit code from GPG coupled with the existence
> > of a GOODSIG string in the output buffer.  Unfortunately, the mere
> > prescience of GOODSIG does not necessarily imply a valid signature, as
> > shown by Michał Górny [1].
> >
> > verify_signed_buffer() should be reserved for internal use by
> > check_signature() since check_signature() parses and verifies the status
> > message.  This is accomplished in this patch.
> >
> > Changes since v0:
> > * Added regression tests for log-tree and fmt-merge-msg.
> > * Fixed a bug in log-tree.c that caused "No signature" to be shown
> >   erroneously.
> > * Fixed a similar bug in fmt-merge-msg.c.
> > * Always invoke signature_check_clear() after check_signature().  The
> >   check function may touch the signature_check structure on failure.
>
> Thanks.  Will queue.  Let's cook it slower and aim for the next
> cycle.

Good call about cooking this slower: it fails both on Windows and on macOS
(see
https://dev.azure.com/gitgitgadget/git/_build/results?buildId=32672&view=ms.vss-test-web.build-test-results-tab&runId=101636&resultId=101463&paneView=debug
for details):

	expecting success of 6200.2 'GPG':
		git tag -s -m signed-tag-msg signed-good-tag left

	++ git tag -s -m signed-tag-msg signed-good-tag left
	error: gpg failed to sign the data
	error: unable to sign the tag
	error: last command exited with $?=128


Not very helpful log, I must say.

Ciao,
Dscho

>
> > [1] https://dev.gentoo.org/~mgorny/articles/attack-on-git-signature-verification.html
> >
> > Hans Jerry Illikainen (2):
> >   t: increase test coverage of signature verification output
> >   gpg-interface: prefer check_signature() for GPG verification
> >
> >  builtin/fmt-merge-msg.c  |  11 ++--
> >  gpg-interface.c          |  97 +++++++++++++++++------------------
> >  gpg-interface.h          |   9 ----
> >  log-tree.c               |  34 ++++++-------
> >  t/t4202-log.sh           | 106 +++++++++++++++++++++++++++++++++++++++
> >  t/t6200-fmt-merge-msg.sh |  23 +++++++++
> >  6 files changed, 202 insertions(+), 78 deletions(-)
>
>