fmt-merge-msg: prevent use-after-free with signed tags

When merging a signed tag, fmt_merge_msg_sigs() is responsible for
populating the body of the merge message with the names of the signed
tags, their signatures, and the validity of those signatures.

In 02769437e1 (ssh signing: use sigc struct to pass payload,
2021-12-09), check_signature() was taught to pass the object payload via
the sigc struct instead of passing the payload buffer separately.

In effect, 02769437e1 causes buf, and sigc.payload to point at the same
region in memory. This causes a problem for fmt_tag_signature(), which
wants to read from this location, since it is freed beforehand by
signature_check_clear() (which frees it via sigc's `payload` member).

That makes the subsequent use in fmt_tag_signature() a use-after-free.

As a result, merge messages did not contain the body of any signed tags.
Luckily, they tend not to contain garbage, either, since the result of
strstr()-ing the object buffer in fmt_tag_signature() is guarded:

    const char *tag_body = strstr(buf, "\n\n");
    if (tag_body) {
      tag_body += 2;
      strbuf_add(tagbuf, tag_body, buf + len - tag_body);
    }

Unfortunately, the tests in t6200 did not catch this at the time because
they do not search for the body of signed tags in fmt-merge-msg's
output.

Resolve this by waiting to call signature_check_clear() until after its
contents can be safely discarded. Harden ourselves against any future
regressions in this area by making sure we can find signed tag messages
in the output of fmt-merge-msg, too.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index e4f7810be208495e8883f3ca4abad3f21f320003..422bd0c055ab8f068a07fbadcef8e7f2e3b15d4c 100644 (file)
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -541,7 +541,6 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
                        else
                                strbuf_addstr(&sig, sigc.output);
                }
-               signature_check_clear(&sigc);
 
                if (!tag_number++) {
                        fmt_tag_signature(&tagbuf, &sig, buf, len);
@@ -565,6 +564,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
                }
                strbuf_release(&payload);
                strbuf_release(&sig);
+               signature_check_clear(&sigc);
        next:
                free(origbuf);
        }

diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 6e10a539ce69e81a36396b2b7b7210d20b03083a..d3e847f5e265493bcb76b86ee84a6990c539bfe6 100755 (executable)
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -126,6 +126,7 @@ test_expect_success GPG 'message for merging local tag signed by good key' '
        git fetch . signed-good-tag &&
        git fmt-merge-msg <.git/FETCH_HEAD >actual &&
        grep "^Merge tag ${apos}signed-good-tag${apos}" actual &&
+       grep "^signed-tag-msg" actual &&
        grep "^# gpg: Signature made" actual &&
        grep "^# gpg: Good signature from" actual
 '
@@ -135,6 +136,7 @@ test_expect_success GPG 'message for merging local tag signed by unknown key' '
        git fetch . signed-good-tag &&
        GNUPGHOME=. git fmt-merge-msg <.git/FETCH_HEAD >actual &&
        grep "^Merge tag ${apos}signed-good-tag${apos}" actual &&
+       grep "^signed-tag-msg" actual &&
        grep "^# gpg: Signature made" actual &&
        grep -E "^# gpg: Can${apos}t check signature: (public key not found|No public key)" actual
 '
@@ -145,6 +147,7 @@ test_expect_success GPGSSH 'message for merging local tag signed by good ssh key
        git fetch . signed-good-ssh-tag &&
        git fmt-merge-msg <.git/FETCH_HEAD >actual &&
        grep "^Merge tag ${apos}signed-good-ssh-tag${apos}" actual &&
+       grep "^signed-ssh-tag-msg" actual &&
        grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual &&
        ! grep "${GPGSSH_BAD_SIGNATURE}" actual
 '
@@ -155,6 +158,7 @@ test_expect_success GPGSSH 'message for merging local tag signed by unknown ssh
        git fetch . signed-untrusted-ssh-tag &&
        git fmt-merge-msg <.git/FETCH_HEAD >actual &&
        grep "^Merge tag ${apos}signed-untrusted-ssh-tag${apos}" actual &&
+       grep "^signed-ssh-tag-msg-untrusted" actual &&
        grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual &&
        ! grep "${GPGSSH_BAD_SIGNATURE}" actual &&
        grep "${GPGSSH_KEY_NOT_TRUSTED}" actual
@@ -166,6 +170,7 @@ test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag sign
        git fetch . expired-signed &&
        git fmt-merge-msg <.git/FETCH_HEAD >actual &&
        grep "^Merge tag ${apos}expired-signed${apos}" actual &&
+       grep "^expired-signed" actual &&
        ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
 '
 
@@ -175,6 +180,7 @@ test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag sign
        git fetch . notyetvalid-signed &&
        git fmt-merge-msg <.git/FETCH_HEAD >actual &&
        grep "^Merge tag ${apos}notyetvalid-signed${apos}" actual &&
+       grep "^notyetvalid-signed" actual &&
        ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
 '
 
@@ -184,6 +190,7 @@ test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag sign
        git fetch . timeboxedvalid-signed &&
        git fmt-merge-msg <.git/FETCH_HEAD >actual &&
        grep "^Merge tag ${apos}timeboxedvalid-signed${apos}" actual &&
+       grep "^timeboxedvalid-signed" actual &&
        grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual &&
        ! grep "${GPGSSH_BAD_SIGNATURE}" actual
 '
@@ -194,6 +201,7 @@ test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag sign
        git fetch . timeboxedinvalid-signed &&
        git fmt-merge-msg <.git/FETCH_HEAD >actual &&
        grep "^Merge tag ${apos}timeboxedinvalid-signed${apos}" actual &&
+       grep "^timeboxedinvalid-signed" actual &&
        ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
 '