{"thread":{"id":"60609","subject":"[PATCH] mailinfo: fix out-of-bounds memory reads in unquote_quoted_pair()","startedAt":"2023-12-12T22:12:45Z","lastAt":"2023-12-15T07:58:58Z","messageCount":10,"participants":["Jeff King","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"485594","messageId":"20231212221243.GA1656116@coredump.intra.peff.net","threadId":"60609","inReplyTo":null,"subject":"[PATCH] mailinfo: fix out-of-bounds memory reads in unquote_quoted_pair()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-12T22:12:43Z","receivedAt":"2023-12-12T22:12:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When processing a header like a \"From\" line, mailinfo uses\nunquote_quoted_pair() to handle double-quotes and rfc822 parenthesized\ncomments. It takes a NUL-terminated string on input, and loops over the\n\"in\" pointer until it sees the NUL. When it finds the start of an\ninteresting block, it delegates to helper functions which also increment\n\"in\", and return the updated pointer.\n\nBut there's a bug here: the helpers find the NUL with a post-increment\nin the loop condition, like:\n\n while ((c = *in++) != 0)\n\nSo when they do see a NUL (rather than the correct termination of the\nquote or comment section), they return \"in\" as one _past_ the NUL\nterminator. And thus the outer loop in unquote_quoted_pair() does not\nrealize we hit the NUL, and keeps reading past the end of the buffer.\n\nWe should instead make sure to return \"in\" positioned at the NUL, so\nthat the caller knows to stop their loop, too. A hacky way to do this is\nto return \"in - 1\" after leaving the inner loop. But a slightly cleaner\nsolution is to avoid incrementing \"in\" until we are sure it contained a\nnon-NUL byte (i.e., doing it inside the loop body).\n\nThe two tests here show off the problem. Since we check the output,\nthey'll _usually_ report a failure in a normal build, but it depends on\nwhat garbage bytes are found after the heap buffer. Building with\nSANITIZE=address reliably notices the problem. The outcome (both the\nexit code and the exact bytes) are just what Git happens to produce for\nthese cases today, and shouldn't be taken as an endorsement. It might be\nreasonable to abort on an unterminated string, for example. The priority\nfor this patch is fixing the out-of-bounds memory access.\n\nReported-by: Carlos Andrés Ramírez Cataño \nSigned-off-by: Jeff King \n---\nThis was reported to the security list, but because it's just an\nout-of-bounds read, we won't do a coordinated disclosure.\n\n mailinfo.c | 8 ++++----\n t/t5100-mailinfo.sh | 22 ++++++++++++++++++++++\n 2 files changed, 26 insertions(+), 4 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex a07d2da16d..737b9e5e13 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -58,12 +58,12 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)\n \n static const char *unquote_comment(struct strbuf *outbuf, const char *in)\n {\n-\tint c;\n \tint take_next_literally = 0;\n \n \tstrbuf_addch(outbuf, '(');\n \n-\twhile ((c = *in++) != 0) {\n+\twhile (*in) {\n+\t\tint c = *in++;\n \t\tif (take_next_literally == 1) {\n \t\t\ttake_next_literally = 0;\n \t\t} else {\n@@ -88,10 +88,10 @@ static const char *unquote_comment(struct strbuf *outbuf, const char *in)\n \n static const char *unquote_quoted_string(struct strbuf *outbuf, const char *in)\n {\n-\tint c;\n \tint take_next_literally = 0;\n \n-\twhile ((c = *in++) != 0) {\n+\twhile (*in) {\n+\t\tint c = *in++;\n \t\tif (take_next_literally == 1) {\n \t\t\ttake_next_literally = 0;\n \t\t} else {\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex db11cababd..654d8cf3ee 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -268,4 +268,26 @@ test_expect_success 'mailinfo warn CR in base64 encoded email' '\n \ttest_must_be_empty quoted-cr/0002.err\n '\n \n+test_expect_success 'from line with unterminated quoted string' '\n+\techo \"From: bob \\\"unterminated string smith \" >in &&\n+\tgit mailinfo /dev/null /dev/null actual &&\n+\tcat >expect <<-\\EOF &&\n+\tAuthor: bob unterminated string smith\n+\tEmail: bob@example.com\n+\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'from line with unterminated comment' '\n+\techo \"From: bob (unterminated comment smith \" >in &&\n+\tgit mailinfo /dev/null /dev/null actual &&\n+\tcat >expect <<-\\EOF &&\n+\tAuthor: bob (unterminated comment smith\n+\tEmail: bob@example.com\n+\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.43.0.310.g85bf1f633d\n"},{"id":"485604","messageId":"ZXlYIZ0Hb1kN84NU@tanuki","threadId":"60609","inReplyTo":"20231212221243.GA1656116@coredump.intra.peff.net","subject":"Re: [PATCH] mailinfo: fix out-of-bounds memory reads in unquote_quoted_pair()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-13T07:07:21Z","receivedAt":"2023-12-13T07:07:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Dec 12, 2023 at 05:12:43PM -0500, Jeff King wrote:\n> When processing a header like a \"From\" line, mailinfo uses\n> unquote_quoted_pair() to handle double-quotes and rfc822 parenthesized\n> comments. It takes a NUL-terminated string on input, and loops over the\n> \"in\" pointer until it sees the NUL. When it finds the start of an\n> interesting block, it delegates to helper functions which also increment\n> \"in\", and return the updated pointer.\n> \n> But there's a bug here: the helpers find the NUL with a post-increment\n> in the loop condition, like:\n> \n> while ((c = *in++) != 0)\n> \n> So when they do see a NUL (rather than the correct termination of the\n> quote or comment section), they return \"in\" as one _past_ the NUL\n> terminator. And thus the outer loop in unquote_quoted_pair() does not\n> realize we hit the NUL, and keeps reading past the end of the buffer.\n> \n> We should instead make sure to return \"in\" positioned at the NUL, so\n> that the caller knows to stop their loop, too. A hacky way to do this is\n> to return \"in - 1\" after leaving the inner loop. But a slightly cleaner\n> solution is to avoid incrementing \"in\" until we are sure it contained a\n> non-NUL byte (i.e., doing it inside the loop body).\n> \n> The two tests here show off the problem. Since we check the output,\n> they'll _usually_ report a failure in a normal build, but it depends on\n> what garbage bytes are found after the heap buffer. Building with\n> SANITIZE=address reliably notices the problem. The outcome (both the\n> exit code and the exact bytes) are just what Git happens to produce for\n> these cases today, and shouldn't be taken as an endorsement. It might be\n> reasonable to abort on an unterminated string, for example. The priority\n> for this patch is fixing the out-of-bounds memory access.\n\nMakes sense.\n\n> diff --git a/mailinfo.c b/mailinfo.c\n> index a07d2da16d..737b9e5e13 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -58,12 +58,12 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)\n> \n> static const char *unquote_comment(struct strbuf *outbuf, const char *in)\n> {\n> -\tint c;\n> \tint take_next_literally = 0;\n> \n> \tstrbuf_addch(outbuf, '(');\n> \n> -\twhile ((c = *in++) != 0) {\n> +\twhile (*in) {\n> +\t\tint c = *in++;\n> \t\tif (take_next_literally == 1) {\n> \t\t\ttake_next_literally = 0;\n> \t\t} else {\n> @@ -88,10 +88,10 @@ static const char *unquote_comment(struct strbuf *outbuf, const char *in)\n> \n> static const char *unquote_quoted_string(struct strbuf *outbuf, const char *in)\n> {\n> -\tint c;\n> \tint take_next_literally = 0;\n> \n> -\twhile ((c = *in++) != 0) {\n> +\twhile (*in) {\n> +\t\tint c = *in++;\n> \t\tif (take_next_literally == 1) {\n> \t\t\ttake_next_literally = 0;\n> \t\t} else {\n\nI was wondering whether we want to convert `unquote_quoted_pair()` in\nthe same way. It's not subject to the same issue because it doesn't\nreturn `in` to its callers. But it would lower the cognitive overhead a\nbit by keeping the coding style consistent. But please feel free to\nignore this remark.\n\nAnother thing I was wondering about is the recursive nature of these\nfunctions, and whether it can lead to similar issues like we recently\nhad when parsing revisions with stack exhaustion. I _think_ this should\nnot be much of a problem in this case though, as we're talking about\nmail headers here. While the length of header values isn't restricted\nper se (the line length is restricted to 1000, but with Comment Folding\nWhitespace values can span multiple lines), but mail provdiers will sure\nclamp down on mails with a \"From:\" header that is megabytes in size.\n\nSo taken together, this looks good to me.\n\nPatrick\n"},{"id":"485609","messageId":"20231213082027.GB1684525@coredump.intra.peff.net","threadId":"60609","inReplyTo":"ZXlYIZ0Hb1kN84NU@tanuki","subject":"Re: [PATCH] mailinfo: fix out-of-bounds memory reads in unquote_quoted_pair()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-13T08:20:27Z","receivedAt":"2023-12-13T08:20:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 13, 2023 at 08:07:21AM +0100, Patrick Steinhardt wrote:\n\n> > static const char *unquote_quoted_string(struct strbuf *outbuf, const char *in)\n> > {\n> > -\tint c;\n> > \tint take_next_literally = 0;\n> > \n> > -\twhile ((c = *in++) != 0) {\n> > +\twhile (*in) {\n> > +\t\tint c = *in++;\n> > \t\tif (take_next_literally == 1) {\n> > \t\t\ttake_next_literally = 0;\n> > \t\t} else {\n> \n> I was wondering whether we want to convert `unquote_quoted_pair()` in\n> the same way. It's not subject to the same issue because it doesn't\n> return `in` to its callers. But it would lower the cognitive overhead a\n> bit by keeping the coding style consistent. But please feel free to\n> ignore this remark.\n\nI dunno. I don't think the consistency matters that much between\nfunctions, and on its own, the \"while ((c = *in++))\" pattern is not\nharmful. It's used elsewhere in the same file for other functions that\nare also fine (for decoding q/b headers).\n\n> Another thing I was wondering about is the recursive nature of these\n> functions, and whether it can lead to similar issues like we recently\n> had when parsing revisions with stack exhaustion. I _think_ this should\n> not be much of a problem in this case though, as we're talking about\n> mail headers here. While the length of header values isn't restricted\n> per se (the line length is restricted to 1000, but with Comment Folding\n> Whitespace values can span multiple lines), but mail provdiers will sure\n> clamp down on mails with a \"From:\" header that is megabytes in size.\n\nIt's just unquote_comment() that is recursive, but yeah, there is\nnothing to stop it from recursing forever on \"((((((...\". The stack\nrequirements are pretty small, though. I needed between 2^17 and 2^18\nbytes to segfault on my machine using:\n\n perl -e 'print \"From: \", \"(\" x 2**18;' |\n git mailinfo /dev/null /dev/null\n\nAbsurdly big for an email, but maybe within the realm of possibility? I\nthink it might be possible to drop the recursion and just use a depth\ncounter, like this:\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 737b9e5e13..db236f9f9f 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -59,6 +59,7 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)\n static const char *unquote_comment(struct strbuf *outbuf, const char *in)\n {\n \tint take_next_literally = 0;\n+\tint depth = 1;\n \n \tstrbuf_addch(outbuf, '(');\n \n@@ -72,11 +73,14 @@ static const char *unquote_comment(struct strbuf *outbuf, const char *in)\n \t\t\t\ttake_next_literally = 1;\n \t\t\t\tcontinue;\n \t\t\tcase '(':\n-\t\t\t\tin = unquote_comment(outbuf, in);\n+\t\t\t\tstrbuf_addch(outbuf, '(');\n+\t\t\t\tdepth++;\n \t\t\t\tcontinue;\n \t\t\tcase ')':\n \t\t\t\tstrbuf_addch(outbuf, ')');\n-\t\t\t\treturn in;\n+\t\t\t\tif (!--depth)\n+\t\t\t\t\treturn in;\n+\t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n \nI doubt it's a big deal either way, but if it's that easy to do it might\nbe worth it.\n\n-Peff\n"},{"id":"485615","messageId":"xmqqy1dynofo.fsf@gitster.g","threadId":"60609","inReplyTo":"ZXlYIZ0Hb1kN84NU@tanuki","subject":"Re: [PATCH] mailinfo: fix out-of-bounds memory reads in unquote_quoted_pair()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-13T14:54:03Z","receivedAt":"2023-12-13T14:54:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> static const char *unquote_quoted_string(struct strbuf *outbuf, const char *in)\n>> {\n>> -\tint c;\n>> \tint take_next_literally = 0;\n>> \n>> -\twhile ((c = *in++) != 0) {\n>> +\twhile (*in) {\n>> +\t\tint c = *in++;\n>> \t\tif (take_next_literally == 1) {\n>> \t\t\ttake_next_literally = 0;\n>> \t\t} else {\n>\n> I was wondering whether we want to convert `unquote_quoted_pair()` in\n> the same way. It's not subject to the same issue because it doesn't\n> return `in` to its callers. But it would lower the cognitive overhead a\n> bit by keeping the coding style consistent. But please feel free to\n> ignore this remark.\n\nYeah, I was wondering about the value of establishing a pattern that\ncan be followed safely and with clarity, too. I also briefly\nwondered how bad if we picked the \"compensate for the increment done\nby the last iteration before leaving the loop by returning (in-1)\"\nidiom (which Peff called \"a hacky way\") to be that universal\npattern, but this bug was a clear enough evidence that it does not\nwork very well in developers' minds.\n\nI actually had trouble with the proposed update, and wondered if\n\n-\twhile ((c = *in++) != 0) {\n+\twhile ((c = *in)) {\n+\t\tin++;\n\nis easier to follow, but then was hit by the possibility that the\nsame \"we have incremented 'in' a bit too early\" may exist if such\na loop wants to use 'in' in its body. Wouldn't it mean that\n\n-\twhile ((c = *in++) != 0) {\n+\tfor (; c = *in; in++) {\n\nwould be even a better rewrite?\n\nEnough bikeshedding...\n\n> Another thing I was wondering about is the recursive nature of these\n> functions, and whether it can lead to similar issues like we recently\n> had when parsing revisions with stack exhaustion. I _think_ this should\n> not be much of a problem in this case though, as we're talking about\n> mail headers here. While the length of header values isn't restricted\n> per se (the line length is restricted to 1000, but with Comment Folding\n> Whitespace values can span multiple lines), but mail provdiers will sure\n> clamp down on mails with a \"From:\" header that is megabytes in size.\n\nGood thinking, and I think Peff's iterative rewrite would be a good\nway to deal with it if it ever becomes an issue.\n\n> So taken together, this looks good to me.\n\nThanks, both, for writing and reviewing.\n\n"},{"id":"485630","messageId":"ZXqxoKLFG19tMFpF@tanuki","threadId":"60609","inReplyTo":"20231213082027.GB1684525@coredump.intra.peff.net","subject":"Re: [PATCH] mailinfo: fix out-of-bounds memory reads in unquote_quoted_pair()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-14T07:41:20Z","receivedAt":"2023-12-14T07:41:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 13, 2023 at 03:20:27AM -0500, Jeff King wrote:\n> On Wed, Dec 13, 2023 at 08:07:21AM +0100, Patrick Steinhardt wrote:\n[snip]\n> > Another thing I was wondering about is the recursive nature of these\n> > functions, and whether it can lead to similar issues like we recently\n> > had when parsing revisions with stack exhaustion. I _think_ this should\n> > not be much of a problem in this case though, as we're talking about\n> > mail headers here. While the length of header values isn't restricted\n> > per se (the line length is restricted to 1000, but with Comment Folding\n> > Whitespace values can span multiple lines), but mail provdiers will sure\n> > clamp down on mails with a \"From:\" header that is megabytes in size.\n> \n> It's just unquote_comment() that is recursive, but yeah, there is\n> nothing to stop it from recursing forever on \"((((((...\". The stack\n> requirements are pretty small, though. I needed between 2^17 and 2^18\n> bytes to segfault on my machine using:\n> \n> perl -e 'print \"From: \", \"(\" x 2**18;' |\n> git mailinfo /dev/null /dev/null\n> \n> Absurdly big for an email, but maybe within the realm of possibility? I\n> think it might be possible to drop the recursion and just use a depth\n> counter, like this:\n\nIt's definitely not as large as I'd have expected it to be, we're only\ntalking about kilobytes of data. Feels like it might be in the realm of\npossibility to get transferred by a mail provider.\n\n> diff --git a/mailinfo.c b/mailinfo.c\n> index 737b9e5e13..db236f9f9f 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -59,6 +59,7 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)\n> static const char *unquote_comment(struct strbuf *outbuf, const char *in)\n> {\n> \tint take_next_literally = 0;\n> +\tint depth = 1;\n> \n> \tstrbuf_addch(outbuf, '(');\n> \n> @@ -72,11 +73,14 @@ static const char *unquote_comment(struct strbuf *outbuf, const char *in)\n> \t\t\t\ttake_next_literally = 1;\n> \t\t\t\tcontinue;\n> \t\t\tcase '(':\n> -\t\t\t\tin = unquote_comment(outbuf, in);\n> +\t\t\t\tstrbuf_addch(outbuf, '(');\n> +\t\t\t\tdepth++;\n> \t\t\t\tcontinue;\n> \t\t\tcase ')':\n> \t\t\t\tstrbuf_addch(outbuf, ')');\n> -\t\t\t\treturn in;\n> +\t\t\t\tif (!--depth)\n> +\t\t\t\t\treturn in;\n> +\t\t\t\tcontinue;\n> \t\t\t}\n> \t\t}\n> \n> I doubt it's a big deal either way, but if it's that easy to do it might\n> be worth it.\n\nIsn't this only protecting against unbalanced braces? That might be a\nsensible check to do regardless, but does it protect against recursion\nblowing the stack if you just happen to have many opening braces?\n\nMight be I'm missing something.\n\nPatrick\n"},{"id":"485665","messageId":"20231214214057.GA2297853@coredump.intra.peff.net","threadId":"60609","inReplyTo":"xmqqy1dynofo.fsf@gitster.g","subject":"Re: [PATCH] mailinfo: fix out-of-bounds memory reads in unquote_quoted_pair()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-14T21:40:57Z","receivedAt":"2023-12-14T21:40:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 13, 2023 at 06:54:03AM -0800, Junio C Hamano wrote:\n\n> I actually had trouble with the proposed update, and wondered if\n> \n> -\twhile ((c = *in++) != 0) {\n> +\twhile ((c = *in)) {\n> +\t\tin++;\n> \n> is easier to follow, but then was hit by the possibility that the\n> same \"we have incremented 'in' a bit too early\" may exist if such\n> a loop wants to use 'in' in its body. Wouldn't it mean that\n> \n> -\twhile ((c = *in++) != 0) {\n> +\tfor (; c = *in; in++) {\n> \n> would be even a better rewrite?\n\nNo, the \"for\" loop wouldn't work, because the loop body actually depends\non \"in\" having already been incremented. If we find the end of the\ncomment or quoted string, we return \"in\", and the caller is expecting it\nto have moved past the closing quote. So that would have to become\n\"return in+1\".\n\nIOW, the issue is that the normal end-of-quote parsing and hitting\nend-of-string are fundamentally different. So we either need to\ndifferentiate the returns (either with \"+1\" on one, or \"-1\" on the\nother). Or we need to choose to increment \"in\" based on which we found\n(which is what my patch does).\n\n-Peff\n"},{"id":"485666","messageId":"20231214214444.GB2297853@coredump.intra.peff.net","threadId":"60609","inReplyTo":"ZXqxoKLFG19tMFpF@tanuki","subject":"[PATCH 0/2] avoiding recursion in mailinfo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-14T21:44:44Z","receivedAt":"2023-12-14T21:44:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 14, 2023 at 08:41:20AM +0100, Patrick Steinhardt wrote:\n\n> > @@ -72,11 +73,14 @@ static const char *unquote_comment(struct strbuf *outbuf, const char *in)\n> > \t\t\t\ttake_next_literally = 1;\n> > \t\t\t\tcontinue;\n> > \t\t\tcase '(':\n> > -\t\t\t\tin = unquote_comment(outbuf, in);\n> > +\t\t\t\tstrbuf_addch(outbuf, '(');\n> > +\t\t\t\tdepth++;\n> > \t\t\t\tcontinue;\n> > \t\t\tcase ')':\n> > \t\t\t\tstrbuf_addch(outbuf, ')');\n> > -\t\t\t\treturn in;\n> > +\t\t\t\tif (!--depth)\n> > +\t\t\t\t\treturn in;\n> > +\t\t\t\tcontinue;\n> > \t\t\t}\n> > \t\t}\n> > \n> > I doubt it's a big deal either way, but if it's that easy to do it might\n> > be worth it.\n> \n> Isn't this only protecting against unbalanced braces? That might be a\n> sensible check to do regardless, but does it protect against recursion\n> blowing the stack if you just happen to have many opening braces?\n> \n> Might be I'm missing something.\n\nIt protects against recrusion blowing the stack because it removes the\nrecursive call to unquote_comment(). ;)\n\nThe \"depth\" stuff is there because without recursion, we have to know\nwhen we've hit the correct closing paren (as opposed to an embedded\none).\n\nHere it is as a patch (actually two patches). I don't think it's high\npriority, but I'd sunk enough brain cells into thinking about it that I\nwanted to capture the work. ;)\n\nI did it on top of the earlier mailinfo out-of-bounds read fix, but it\ncould be applied separately.\n\n [1/2]: t5100: make rfc822 comment test more careful\n [2/2]: mailinfo: avoid recursion when unquoting From headers\n\n mailinfo.c | 8 ++++++--\n t/t5100/comment.expect | 2 +-\n t/t5100/comment.in | 2 +-\n 3 files changed, 8 insertions(+), 4 deletions(-)\n\n-Peff\n"},{"id":"485667","messageId":"20231214214746.GA2798346@coredump.intra.peff.net","threadId":"60609","inReplyTo":"20231214214444.GB2297853@coredump.intra.peff.net","subject":"[PATCH 1/2] t5100: make rfc822 comment test more careful","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-14T21:47:46Z","receivedAt":"2023-12-14T21:47:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When processing \"From\" headers in an email, mailinfo \"unquotes\" quoted\nstrings and rfc822 parenthesized comments. For quoted strings, we\nactually remove the double-quotes, so:\n\n From: \"A U Thor\" \n\nbecome:\n\n Author: A U Thor\n Email: someone@example.com\n\nBut for comments, we leave the outer parentheses in place, so:\n\n From: A U (this is a comment) Thor \n\nbecomes:\n\n Author: A U (this is a comment) Thor\n Email: someone@example.com\n\nSo what is the comment \"unquoting\" actually doing? In our code, being in\na comment section has exactly two effects:\n\n 1. We'll unquote backslash-escaped characters inside a comment\n section.\n\n 2. We _won't_ unquote double-quoted strings inside a comment section.\n\nOur test for comments in t5100 checks this:\n\n From: \"A U Thor\" (this is \\(really\\) a comment (honestly))\n\nSo it is covering (1), but not (2). Let's add in a quoted string to\ncover this.\n\nMoreover, because the comment appears at the end of the From header,\nthere's nothing to confirm that we correctly found the end of the\ncomment section (and not just the end-of-string). Let's instead move it\nto the beginning of the header, which means we can confirm that the\nexisting quoted string is detected (which will only happen if we know\nwe've left the comment block).\n\nAs expected, the test continues to pass, but this will give us more\nconfidence as we refactor the code in the next patch.\n\nSigned-off-by: Jeff King \n---\nI did this as a separate patch so it is easy to see that the existing\ncode already passes the test (i.e., our refactor is a true noop in terms\nof behavior, and not fixing or breaking anything).\n\n t/t5100/comment.expect | 2 +-\n t/t5100/comment.in | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5100/comment.expect b/t/t5100/comment.expect\nindex 7228177984..bd71956a47 100644\n--- a/t/t5100/comment.expect\n+++ b/t/t5100/comment.expect\n@@ -1,4 +1,4 @@\n-Author: A U Thor (this is (really) a comment (honestly))\n+Author: (this is (really) a \"comment\" (honestly)) A U Thor\n Email: somebody@example.com\n Subject: testing comments\n Date: Sun, 25 May 2008 00:38:18 -0700\ndiff --git a/t/t5100/comment.in b/t/t5100/comment.in\nindex c53a192dfe..0b7e903b06 100644\n--- a/t/t5100/comment.in\n+++ b/t/t5100/comment.in\n@@ -1,5 +1,5 @@\n From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001\n-From: \"A U Thor\" (this is \\(really\\) a comment (honestly))\n+From: (this is \\(really\\) a \"comment\" (honestly)) \"A U Thor\" \n Date: Sun, 25 May 2008 00:38:18 -0700\n Subject: [PATCH] testing comments\n \n-- \n2.43.0.363.g1d22a8f302\n\n"},{"id":"485668","messageId":"20231214214859.GB2798346@coredump.intra.peff.net","threadId":"60609","inReplyTo":"20231214214444.GB2297853@coredump.intra.peff.net","subject":"[PATCH 2/2] mailinfo: avoid recursion when unquoting From headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-14T21:48:59Z","receivedAt":"2023-12-14T21:49:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our unquote_comment() function is recursive; when it sees a comment\nwithin a comment, like:\n\n (this is an (embedded) comment)\n\nit recurses to handle the inner comment. This is fine for practical use,\nbut it does mean that you can easily run out of stack space with a\nmalicious header. For example:\n\n perl -e 'print \"From: \", \"(\" x 2**18;' |\n git mailinfo /dev/null /dev/null\n\nsegfaults on my system. And since mailinfo is likely to be fed untrusted\ninput from the Internet (if not by human users, who might recognize a\ngarbage header, but certainly there are automated systems that apply\npatches from a list) it may be possible for an attacker to trigger the\nproblem.\n\nThat said, I don't think there's an interesting security vulnerability\nhere. All an attacker can do is make it impossible to parse their email\nand apply their patch, and there are lots of ways to generate bogus\nemails. So it's more of an annoyance than anything.\n\nBut it's pretty easy to fix it. The recursion is not helping us preserve\nany particular state from each level. The only flag in our parsing is\ntake_next_literally, and we can never recurse when it is set (since the\nstart of a new comment implies it was not backslash-escaped). So it is\nreally only useful for finding the end of the matched pair of\nparentheses. We can do that easily with a simple depth counter.\n\nSigned-off-by: Jeff King \n---\n mailinfo.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 737b9e5e13..db236f9f9f 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -59,6 +59,7 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)\n static const char *unquote_comment(struct strbuf *outbuf, const char *in)\n {\n \tint take_next_literally = 0;\n+\tint depth = 1;\n \n \tstrbuf_addch(outbuf, '(');\n \n@@ -72,11 +73,14 @@ static const char *unquote_comment(struct strbuf *outbuf, const char *in)\n \t\t\t\ttake_next_literally = 1;\n \t\t\t\tcontinue;\n \t\t\tcase '(':\n-\t\t\t\tin = unquote_comment(outbuf, in);\n+\t\t\t\tstrbuf_addch(outbuf, '(');\n+\t\t\t\tdepth++;\n \t\t\t\tcontinue;\n \t\t\tcase ')':\n \t\t\t\tstrbuf_addch(outbuf, ')');\n-\t\t\t\treturn in;\n+\t\t\t\tif (!--depth)\n+\t\t\t\t\treturn in;\n+\t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n \n-- \n2.43.0.363.g1d22a8f302\n"},{"id":"485711","messageId":"ZXwHPL3sWhzwamrp@tanuki","threadId":"60609","inReplyTo":"20231214214444.GB2297853@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] avoiding recursion in mailinfo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-15T07:58:52Z","receivedAt":"2023-12-15T07:58:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 14, 2023 at 04:44:44PM -0500, Jeff King wrote:\n> On Thu, Dec 14, 2023 at 08:41:20AM +0100, Patrick Steinhardt wrote:\n> \n> > > @@ -72,11 +73,14 @@ static const char *unquote_comment(struct strbuf *outbuf, const char *in)\n> > > \t\t\t\ttake_next_literally = 1;\n> > > \t\t\t\tcontinue;\n> > > \t\t\tcase '(':\n> > > -\t\t\t\tin = unquote_comment(outbuf, in);\n> > > +\t\t\t\tstrbuf_addch(outbuf, '(');\n> > > +\t\t\t\tdepth++;\n> > > \t\t\t\tcontinue;\n> > > \t\t\tcase ')':\n> > > \t\t\t\tstrbuf_addch(outbuf, ')');\n> > > -\t\t\t\treturn in;\n> > > +\t\t\t\tif (!--depth)\n> > > +\t\t\t\t\treturn in;\n> > > +\t\t\t\tcontinue;\n> > > \t\t\t}\n> > > \t\t}\n> > > \n> > > I doubt it's a big deal either way, but if it's that easy to do it might\n> > > be worth it.\n> > \n> > Isn't this only protecting against unbalanced braces? That might be a\n> > sensible check to do regardless, but does it protect against recursion\n> > blowing the stack if you just happen to have many opening braces?\n> > \n> > Might be I'm missing something.\n> \n> It protects against recrusion blowing the stack because it removes the\n> recursive call to unquote_comment(). ;)\n\nDoh. Of course it does, I completely missed the removals.\n\n> The \"depth\" stuff is there because without recursion, we have to know\n> when we've hit the correct closing paren (as opposed to an embedded\n> one).\n\nYes, makes sense now. The patches look good to me, thanks!\n\nPatrick\n"}]}