| Message ID | 6e08b73d602853b3de71257117e85e32b96b5c19.1641849502.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fmt-merge-msg: prevent use-after-free with signed tags | expand |
Taylor Blau <me@ttaylorr.com> writes: > 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. Very clearly described. > 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> > --- Will fast-track. Thanks.
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
On Tue, Jan 11, 2022 at 09:41:15AM +0100, Fabian Stelzer wrote: > 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. I thought similarly when trying to looking at the original bisection. But now that we're in the release candidate phase, I figure that any less-than-minimal fix was liable to cause more harm than good. It is worth looking at in the future, though. Thanks, Taylor
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 '
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> --- fmt-merge-msg.c | 2 +- t/t6200-fmt-merge-msg.sh | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) -- 2.34.1.455.gd6eb6fd089