From: Junio C Hamano Date: Mon, 10 Jan 2022 21:38:36 GMT Subject: Re: [PATCH] fmt-merge-msg: prevent use-after-free with signed tags Message-ID: In-Reply-To: <6e08b73d602853b3de71257117e85e32b96b5c19.1641849502.git.me@ttaylorr.com> Taylor Blau 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 > Signed-off-by: Taylor Blau > --- Will fast-track. Thanks.