gpg-interface: prefer check_signature() for GPG verification

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 <hji@dyntopia.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 736f666f644c7cf78b816888ba226693464fc331..172dfbd852a9d50aeb93b42183a77e88979c6597 100644 (file)
--- 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 165274d74a84a8f30b21a049ebcbccf8aaecc6fb..2d538bcd6e30d1314f76d89764284f8735cd187f 100644 (file)
--- 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 796571e9e90fa50bc6f368bdeefacc8e86a9c520..f4e9b4f3715a0b0d4be3ef526388bf6b87b56fda 100644 (file)
--- 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 5425ae931f5e3c36970c68cf447f14056b5deeb3..f1ac5a68d92018bd5de9fc4fc74ba59085cbce87 100644 (file)
--- 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 */
        }