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 a4615587fd7929e8fb49e65dbda30a40b8599cfd..f7ed102d8b37294f992c3edf25156ce4f3e20c69 100644 (file)
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -495,6 +495,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)
@@ -503,10 +504,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)) {
+                       strbuf_addstr(&sig, sigc.gpg_output);
+                       signature_check_clear(&sigc);
+               } else
+                       strbuf_addstr(&sig, "gpg verification failed.\n");
 
                if (!tag_number++) {
                        fmt_tag_signature(&tagbuf, &sig, buf, len);

diff --git a/gpg-interface.c b/gpg-interface.c
index 131e7d529e9bdb480c88a01a33a3dd8339d219cd..5134ce27806866c41d5eda19cf01110cbf7849f2 100644 (file)
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -207,6 +207,55 @@ found_duplicate_status:
        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)
 {
@@ -351,51 +400,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 3e624ec289ab5f46b686f9eb2489c997bca23232..93cc3aff5c93ce8fa19aadea909960f9267edbdc 100644 (file)
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -46,15 +46,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 1e56df62a79c0d221fa55ba351b6b16f8027d12b..aa6b038adb1f2bf58ceba1e4a367a7e3bb6553e0 100644 (file)
--- a/log-tree.c
+++ b/log-tree.c
@@ -448,22 +448,22 @@ 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.result == 'N')
+               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);
 }
@@ -496,6 +496,7 @@ 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;
 
@@ -524,12 +525,13 @@ static int show_one_mergetag(struct commit *commit,
        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))
+               if (!check_signature(extra->value, payload_size,
+                                    extra->value + payload_size,
+                                    extra->len - payload_size, &sigc)) {
+                       strbuf_addstr(&verify_message, sigc.gpg_output);
+                       signature_check_clear(&sigc);
                        status = 0; /* good */
-               else if (verify_message.len <= gpg_message_offset)
+               } else if (verify_message.len <= gpg_message_offset)
                        strbuf_addstr(&verify_message, "No signature\n");
                /* otherwise we couldn't verify, which is shown as bad */
        }