From: Fabian Stelzer Date: Tue, 11 Jan 2022 08:41:15 GMT Subject: Re: [PATCH] fmt-merge-msg: prevent use-after-free with signed tags Message-ID: <20220111084115.esuyxeopdpaq7g7y@fs> In-Reply-To: <6e08b73d602853b3de71257117e85e32b96b5c19.1641849502.git.me@ttaylorr.com> 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 >Signed-off-by: Taylor Blau >--- > 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