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>

Taylor Blau committed Jan 10, 2022 at 16:19 UTC c39fc06b999305963600358f3f5e99698440cad2
2 files changed +9 -1
fmt-merge-msg.c
+1 -1
@@ -541,7 +541,6 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
541 else
542 strbuf_addstr(&sig, sigc.output);
543 }
544 - signature_check_clear(&sigc);
544
545 if (!tag_number++) {
546 fmt_tag_signature(&tagbuf, &sig, buf, len);
@@ -565,6 +564,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
564 }
565 strbuf_release(&payload);
566 strbuf_release(&sig);
567 + signature_check_clear(&sigc);
568 next:
569 free(origbuf);
570 }
t/t6200-fmt-merge-msg.sh
+8
@@ -126,6 +126,7 @@ test_expect_success GPG 'message for merging local tag signed by good key' '
126 git fetch . signed-good-tag &&
127 git fmt-merge-msg <.git/FETCH_HEAD >actual &&
128 grep "^Merge tag ${apos}signed-good-tag${apos}" actual &&
129 + grep "^signed-tag-msg" actual &&
130 grep "^# gpg: Signature made" actual &&
131 grep "^# gpg: Good signature from" actual
132 '
@@ -135,6 +136,7 @@ test_expect_success GPG 'message for merging local tag signed by unknown key' '
136 git fetch . signed-good-tag &&
137 GNUPGHOME=. git fmt-merge-msg <.git/FETCH_HEAD >actual &&
138 grep "^Merge tag ${apos}signed-good-tag${apos}" actual &&
139 + grep "^signed-tag-msg" actual &&
140 grep "^# gpg: Signature made" actual &&
141 grep -E "^# gpg: Can${apos}t check signature: (public key not found|No public key)" actual
142 '
@@ -145,6 +147,7 @@ test_expect_success GPGSSH 'message for merging local tag signed by good ssh key
147 git fetch . signed-good-ssh-tag &&
148 git fmt-merge-msg <.git/FETCH_HEAD >actual &&
149 grep "^Merge tag ${apos}signed-good-ssh-tag${apos}" actual &&
150 + grep "^signed-ssh-tag-msg" actual &&
151 grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual &&
152 ! grep "${GPGSSH_BAD_SIGNATURE}" actual
153 '
@@ -155,6 +158,7 @@ test_expect_success GPGSSH 'message for merging local tag signed by unknown ssh
158 git fetch . signed-untrusted-ssh-tag &&
159 git fmt-merge-msg <.git/FETCH_HEAD >actual &&
160 grep "^Merge tag ${apos}signed-untrusted-ssh-tag${apos}" actual &&
161 + grep "^signed-ssh-tag-msg-untrusted" actual &&
162 grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual &&
163 ! grep "${GPGSSH_BAD_SIGNATURE}" actual &&
164 grep "${GPGSSH_KEY_NOT_TRUSTED}" actual
@@ -166,6 +170,7 @@ test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag sign
170 git fetch . expired-signed &&
171 git fmt-merge-msg <.git/FETCH_HEAD >actual &&
172 grep "^Merge tag ${apos}expired-signed${apos}" actual &&
173 + grep "^expired-signed" actual &&
174 ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
175 '
176
@@ -175,6 +180,7 @@ test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag sign
180 git fetch . notyetvalid-signed &&
181 git fmt-merge-msg <.git/FETCH_HEAD >actual &&
182 grep "^Merge tag ${apos}notyetvalid-signed${apos}" actual &&
183 + grep "^notyetvalid-signed" actual &&
184 ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
185 '
186
@@ -184,6 +190,7 @@ test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag sign
190 git fetch . timeboxedvalid-signed &&
191 git fmt-merge-msg <.git/FETCH_HEAD >actual &&
192 grep "^Merge tag ${apos}timeboxedvalid-signed${apos}" actual &&
193 + grep "^timeboxedvalid-signed" actual &&
194 grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual &&
195 ! grep "${GPGSSH_BAD_SIGNATURE}" actual
196 '
@@ -194,6 +201,7 @@ test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag sign
201 git fetch . timeboxedinvalid-signed &&
202 git fmt-merge-msg <.git/FETCH_HEAD >actual &&
203 grep "^Merge tag ${apos}timeboxedinvalid-signed${apos}" actual &&
204 + grep "^timeboxedinvalid-signed" actual &&
205 ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
206 '
207