{"thread":{"id":"56244","subject":"[PATCH] apply: keep buffer/size pair in sync when parsing binary hunks","startedAt":"2021-08-10T01:01:55Z","lastAt":"2021-08-12T20:56:22Z","messageCount":4,"participants":["Jeff King","Carlo Arenas","Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"432348","messageId":"YRHQAFDXs5xvyDND@coredump.intra.peff.net","threadId":"56244","inReplyTo":null,"subject":"[PATCH] apply: keep buffer/size pair in sync when parsing binary hunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-10T01:01:52Z","receivedAt":"2021-08-10T01:01:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We parse through binary hunks by looping through the buffer with code\nlike:\n\n llen = linelen(buffer, size);\n\n ...do something with the line...\n\n buffer += llen;\n size -= llen;\n\nHowever, before we enter the loop, there is one call that increments\n\"buffer\" but forgets to decrement \"size\". As a result, our \"size\" is off\nby the length of that line, and subsequent calls to linelen() may look\npast the end of the buffer for a newline.\n\nThe fix is easy: we just need to decrement size as we do elsewhere.\n\nThis bug goes all the way back to 0660626caf (binary diff: further\nupdates., 2006-05-05). Presumably nobody noticed because it only\ntriggers if the patch is corrupted, and even then we are often \"saved\"\nby luck. We use a strbuf to store the incoming patch, so we overallocate\nthere, plus we add a 16-byte run of NULs as slop for memory comparisons.\nSo if this happened accidentally, the common case is that we'd just read\na few uninitialized bytes from the end of the strbuf before producing\nthe expected \"this patch is corrupted\" error complaint.\n\nHowever, it is possible to carefully construct a case which reads off\nthe end of the buffer. The included test does so. It will pass both\nbefore and after this patch when run normally, but using a tool like\nASan shows that we get an out-of-bounds read before this patch, but not\nafter.\n\nReported-by: Xingman Chen \nSigned-off-by: Jeff King \n---\nThis was reported to the security list, but I think the impact is small\nenough to just take it as a normal fix.\n\nI hate the test, as it is brittle and subject to bitrot if we do things\nlike change the default strbuf hint size. If that does happen, it will\nstill pass; it just won't do anything interesting. But I would like some\nrecord of how to reproduce, and I prefer this to putting something\nhard-to-run in the commit message.\n\n apply.c | 1 +\n t/t4103-apply-binary.sh | 23 +++++++++++++++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/apply.c b/apply.c\nindex 44bc31d6eb..4ed4b27169 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1917,6 +1917,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,\n \n \tstate->linenr++;\n \tbuffer += llen;\n+\tsize -= llen;\n \twhile (1) {\n \t\tint byte_length, max_byte_length, newsize;\n \t\tllen = linelen(buffer, size);\ndiff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh\nindex fad6d3f542..d370ecfe0d 100755\n--- a/t/t4103-apply-binary.sh\n+++ b/t/t4103-apply-binary.sh\n@@ -158,4 +158,27 @@ test_expect_success 'apply binary -p0 diff' '\n \ttest -z \"$(git diff --name-status binary -- file3)\"\n '\n \n+test_expect_success 'reject truncated binary diff' '\n+\tdo_reset &&\n+\n+\t# this length is calculated to get us very close to\n+\t# the 8192-byte strbuf we will use to read in the patch.\n+\ttest-tool genrandom foo 6205 >file1 &&\n+\tgit diff --binary >patch &&\n+\n+\t# truncate the patch at the second \"literal\" line,\n+\t# but exclude the trailing newline. We must use perl\n+\t# for this, since tools like \"sed\" cannot reliably\n+\t# produce output without the trailing newline.\n+\tperl -pe \"\n+\t\tif (/^literal/ && \\$count++ >= 1) {\n+\t\t\tchomp;\n+\t\t\tprint;\n+\t\t\texit 0;\n+\t\t}\n+\t\" patch.trunc &&\n+\n+\tdo_reset &&\n+\ttest_must_fail git apply patch.trunc\n+'\n test_done\n-- \n2.33.0.rc1.475.g023efe0ae4\n"},{"id":"432350","messageId":"CAPUEsphWN_-BWyfF9mnPhL56RSnmPZfmvh_QwhjoAb3xin8V-w@mail.gmail.com","threadId":"56244","inReplyTo":"YRHQAFDXs5xvyDND@coredump.intra.peff.net","subject":"Re: [PATCH] apply: keep buffer/size pair in sync when parsing binary hunks","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-08-10T05:26:51Z","receivedAt":"2021-08-10T05:27:08Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Aug 9, 2021 at 9:24 PM Jeff King wrote:\n> diff --git a/apply.c b/apply.c\n> index 44bc31d6eb..4ed4b27169 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -1917,6 +1917,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,\n>\n> state->linenr++;\n> buffer += llen;\n> + size -= llen;\n> while (1) {\n\nIronically, I was looking at this code because of your previous\npatch[1] that you suggested was ugly\nand because I was going to suggest moving from a for to a while loop\nto avoid the overly long line.\n\nIt is interesting to note though, that having a for (and obviously\nremoving the last 2 lines from the loop) with a comma separated\nincrement instead would\nhave made this issue IMHO more obvious, and also why I decided against\nthat; would it be a good idea to fix that as well?\n\n for (; ; buffer += llen, size -= llen) {\n\nCarlo\n\n[1] https://lore.kernel.org/git/YRGwjgAIyLPb7g50@coredump.intra.peff.net/\n"},{"id":"432402","messageId":"YRLLPPANZhhTNUTT@coredump.intra.peff.net","threadId":"56244","inReplyTo":"CAPUEsphWN_-BWyfF9mnPhL56RSnmPZfmvh_QwhjoAb3xin8V-w@mail.gmail.com","subject":"Re: [PATCH] apply: keep buffer/size pair in sync when parsing binary hunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-10T18:53:48Z","receivedAt":"2021-08-10T18:53:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 09, 2021 at 10:26:51PM -0700, Carlo Arenas wrote:\n\n> On Mon, Aug 9, 2021 at 9:24 PM Jeff King wrote:\n> > diff --git a/apply.c b/apply.c\n> > index 44bc31d6eb..4ed4b27169 100644\n> > --- a/apply.c\n> > +++ b/apply.c\n> > @@ -1917,6 +1917,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,\n> >\n> > state->linenr++;\n> > buffer += llen;\n> > + size -= llen;\n> > while (1) {\n> \n> Ironically, I was looking at this code because of your previous\n> patch[1] that you suggested was ugly\n> and because I was going to suggest moving from a for to a while loop\n> to avoid the overly long line.\n> \n> It is interesting to note though, that having a for (and obviously\n> removing the last 2 lines from the loop) with a comma separated\n> increment instead would\n> have made this issue IMHO more obvious, and also why I decided against\n> that; would it be a good idea to fix that as well?\n> \n> for (; ; buffer += llen, size -= llen) {\n\nThat wouldn't have fixed this issue, since it was actually before the\nstart of the loop that where the problem was. It might have made it more\nobvious if the two parts were next to each other, but I'm not so sure.\nThere are lots of reasons why the stuff before a loop may not be\nsymmetric with the loop increment.\n\nWe could also initialize it like this:\n\n for (buffer += llen, size -= llen; ; buffer += llen, size -= llen)\n\nbut IMHO that is harder to read. The increment at the start is not part\nof the loop initialization. It is really a \"finishing\" of the earlier\nparsing that occurred (in fact, if I were writing this from scratch, I'd\nprobably have put extra whitespace before the start of the loop).\n\nI did consider moving that other case to a while-loop, but then you have\nto manually catch all of the loop continuations (which is verbose and\nerror-prone). So I dunno. There is no free lunch. ;)\n\n-Peff\n"},{"id":"432605","messageId":"61158ae83406a_a3522086a@natae.notmuch","threadId":"56244","inReplyTo":"CAPUEsphWN_-BWyfF9mnPhL56RSnmPZfmvh_QwhjoAb3xin8V-w@mail.gmail.com","subject":"Re: [PATCH] apply: keep buffer/size pair in sync when parsing binary hunks","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-08-12T20:56:08Z","receivedAt":"2021-08-12T20:56:22Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Carlo Arenas wrote:\n> On Mon, Aug 9, 2021 at 9:24 PM Jeff King wrote:\n> > diff --git a/apply.c b/apply.c\n> > index 44bc31d6eb..4ed4b27169 100644\n> > --- a/apply.c\n> > +++ b/apply.c\n> > @@ -1917,6 +1917,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,\n> >\n> > state->linenr++;\n> > buffer += llen;\n> > + size -= llen;\n> > while (1) {\n> \n> Ironically, I was looking at this code because of your previous\n> patch[1] that you suggested was ugly\n> and because I was going to suggest moving from a for to a while loop\n> to avoid the overly long line.\n> \n> It is interesting to note though, that having a for (and obviously\n> removing the last 2 lines from the loop) with a comma separated\n> increment instead would\n> have made this issue IMHO more obvious, and also why I decided against\n> that; would it be a good idea to fix that as well?\n\nWhat's the point in updating size when it can be calculated? Just add a\npointer to the end of the buffer, and then size is the difference\nbetween the buffer which is moving, and the end pointer which is fixed:\n\n-- 8< --\n\ndiff --git a/apply.c b/apply.c\nindex 44bc31d6eb..cfb5a00356 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1891,15 +1891,15 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,\n \t * to 1-26 bytes, and 'a'-'z' corresponds to 27-52 bytes.\n \t */\n \tint llen, used;\n-\tunsigned long size = *sz_p;\n \tchar *buffer = *buf_p;\n+\tchar *end = *buf_p + *sz_p;\n \tint patch_method;\n \tunsigned long origlen;\n \tchar *data = NULL;\n \tint hunk_size = 0;\n \tstruct fragment *frag;\n \n-\tllen = linelen(buffer, size);\n+\tllen = linelen(buffer, end - buffer);\n \tused = llen;\n \n \t*status_p = 0;\n@@ -1919,13 +1919,12 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,\n \tbuffer += llen;\n \twhile (1) {\n \t\tint byte_length, max_byte_length, newsize;\n-\t\tllen = linelen(buffer, size);\n+\t\tllen = linelen(buffer, end - buffer);\n \t\tused += llen;\n \t\tstate->linenr++;\n \t\tif (llen == 1) {\n \t\t\t/* consume the blank line */\n \t\t\tbuffer++;\n-\t\t\tsize--;\n \t\t\tbreak;\n \t\t}\n \t\t/*\n@@ -1955,7 +1954,6 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,\n \t\t\tgoto corrupt;\n \t\thunk_size = newsize;\n \t\tbuffer += llen;\n-\t\tsize -= llen;\n \t}\n \n \tCALLOC_ARRAY(frag, 1);\n@@ -1966,7 +1964,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,\n \tfree(data);\n \tfrag->size = origlen;\n \t*buf_p = buffer;\n-\t*sz_p = size;\n+\t*sz_p = end - buffer;\n \t*used_p = used;\n \tfrag->binary_patch_method = patch_method;\n \treturn frag;\n\n-- \nFelipe Contreras\n"}]}