Re: [PATCH] fmt-merge-msg: prevent use-after-free with signed tags

[Fabian Stelzer <fs@gigacodes.de>]

On 10.01.2022 16:19, Taylor Blau wrote:
>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.

Sorry for breaking any workflows :/
Thanks Taylor for the quick fix and the additional test conditions.

fmt_merge_msg_sigs() could probably use some additional refactoring to avoid 
these multiple pointers to the same (detached) buffer. But thats for another 
time.

Thanks

>
>Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
>Signed-off-by: Taylor Blau <me@ttaylorr.com>
>---
> fmt-merge-msg.c          | 2 +-
> t/t6200-fmt-merge-msg.sh | 8 ++++++++
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
>diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
>index e5c0aff2bf..baca57d5b6 100644
>--- 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 7544245f90..5a221f8ef1 100755
>--- 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
> '
>
>--
>2.34.1.455.gd6eb6fd089