Re: [PATCH 1/5] gpg-interface: check good signature in a reliable way

Michael J Gruber [off-list ref] writes:

Currently, verify_signed_buffer() only checks the return code of gpg,
and some callers implement additional unreliable checks for "Good
signature" in the gpg output meant for the user.

Use the status output instead and parse for a line beinning with
"[GNUPG:] GOODSIG ". This is the only reliable way of checking for a
good gpg signature.

If needed we can change this easily to "[GNUPG:] VALIDSIG " if we want
to take into account the trust model.

Thanks.  I didn't look beyond "man gpg" nor bother looking at
DETAILS file in its source, which the manpage refers to.

I think GOODSIG is a good starting point.  Depending on the context
(e.g. "%G?") we may also want to consider EXPSIG (but not EXPKEYSIG
or REVKEYSIG) acceptable, while reading "log --show-signature" on
ancient part of the history, no?

Junio C Hamano venit, vidit, dixit 14.02.2013 18:22:

Michael J Gruber [off-list ref] writes:

quoted

Currently, verify_signed_buffer() only checks the return code of gpg,
and some callers implement additional unreliable checks for "Good
signature" in the gpg output meant for the user.

Use the status output instead and parse for a line beinning with
"[GNUPG:] GOODSIG ". This is the only reliable way of checking for a
good gpg signature.

If needed we can change this easily to "[GNUPG:] VALIDSIG " if we want
to take into account the trust model.

Thanks.  I didn't look beyond "man gpg" nor bother looking at
DETAILS file in its source, which the manpage refers to.

I think GOODSIG is a good starting point.  Depending on the context
(e.g. "%G?") we may also want to consider EXPSIG (but not EXPKEYSIG
or REVKEYSIG) acceptable, while reading "log --show-signature" on
ancient part of the history, no?

Yes, we could certainly return a more detailed status to the callers.
Currently, "0" is OK (GOODSIG) and everything else is a fail. We would
need to change the callers to allow more details on the "fail" as well
as the "OK" so that they can decide what is good enough, say:

-1: fail for technical reasons (no sig, can't run gpg etc.)
0: sig present bad (cryptographically) BAD
1: REVKEYSIG
2: EXPKEYSIG
3: EXPSIG
4: GOODSIG
5: VALIDSIG

I'd have to recheck whether a bitmask or ordered values make more sense.

quoted

Signed-off-by: Michael J Gruber <redacted>
---
 gpg-interface.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 4559033..c582b2e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c

@@ -96,15 +96,17 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 /*
  * 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 child_process gpg;
-	const char *args_gpg[] = {NULL, "--verify", "FILE", "-", NULL};
+	const char *args_gpg[] = {NULL, "--status-fd=1", "--verify", "FILE", "-", NULL};
 	char path[PATH_MAX];
 	int fd, ret;
+	struct strbuf buf = STRBUF_INIT;
 
 	args_gpg[0] = gpg_program;
 	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");

@@ -119,9 +121,10 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	memset(&gpg, 0, sizeof(gpg));
 	gpg.argv = args_gpg;
 	gpg.in = -1;
+	gpg.out = -1;
 	if (gpg_output)
 		gpg.err = -1;
-	args_gpg[2] = path;
+	args_gpg[3] = path;
 	if (start_command(&gpg)) {
 		unlink(path);
 		return error(_("could not run gpg."));

@@ -134,9 +137,15 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 		strbuf_read(gpg_output, gpg.err, 0);
 		close(gpg.err);
 	}
+	strbuf_read(&buf, gpg.out, 0);
+	close(gpg.out);
+
 	ret = finish_command(&gpg);
 
 	unlink_or_warn(path);
 
+	ret |= !strstr(buf.buf, "\n[GNUPG:] GOODSIG ");
+	strbuf_release(&buf);
+
 	return ret;
 }