| Message ID | 20231212221243.GA1656116@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d1bd3a8c3424e818f4117a39fe418909e24cea5f |
| Headers | show |
| Series | mailinfo: fix out-of-bounds memory reads in unquote_quoted_pair() | expand |
On Tue, Dec 12, 2023 at 05:12:43PM -0500, Jeff King wrote: > When processing a header like a "From" line, mailinfo uses > unquote_quoted_pair() to handle double-quotes and rfc822 parenthesized > comments. It takes a NUL-terminated string on input, and loops over the > "in" pointer until it sees the NUL. When it finds the start of an > interesting block, it delegates to helper functions which also increment > "in", and return the updated pointer. > > But there's a bug here: the helpers find the NUL with a post-increment > in the loop condition, like: > > while ((c = *in++) != 0) > > So when they do see a NUL (rather than the correct termination of the > quote or comment section), they return "in" as one _past_ the NUL > terminator. And thus the outer loop in unquote_quoted_pair() does not > realize we hit the NUL, and keeps reading past the end of the buffer. > > We should instead make sure to return "in" positioned at the NUL, so > that the caller knows to stop their loop, too. A hacky way to do this is > to return "in - 1" after leaving the inner loop. But a slightly cleaner > solution is to avoid incrementing "in" until we are sure it contained a > non-NUL byte (i.e., doing it inside the loop body). > > The two tests here show off the problem. Since we check the output, > they'll _usually_ report a failure in a normal build, but it depends on > what garbage bytes are found after the heap buffer. Building with > SANITIZE=address reliably notices the problem. The outcome (both the > exit code and the exact bytes) are just what Git happens to produce for > these cases today, and shouldn't be taken as an endorsement. It might be > reasonable to abort on an unterminated string, for example. The priority > for this patch is fixing the out-of-bounds memory access. Makes sense. > diff --git a/mailinfo.c b/mailinfo.c > index a07d2da16d..737b9e5e13 100644 > --- a/mailinfo.c > +++ b/mailinfo.c > @@ -58,12 +58,12 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line) > > static const char *unquote_comment(struct strbuf *outbuf, const char *in) > { > - int c; > int take_next_literally = 0; > > strbuf_addch(outbuf, '('); > > - while ((c = *in++) != 0) { > + while (*in) { > + int c = *in++; > if (take_next_literally == 1) { > take_next_literally = 0; > } else { > @@ -88,10 +88,10 @@ static const char *unquote_comment(struct strbuf *outbuf, const char *in) > > static const char *unquote_quoted_string(struct strbuf *outbuf, const char *in) > { > - int c; > int take_next_literally = 0; > > - while ((c = *in++) != 0) { > + while (*in) { > + int c = *in++; > if (take_next_literally == 1) { > take_next_literally = 0; > } else { I was wondering whether we want to convert `unquote_quoted_pair()` in the same way. It's not subject to the same issue because it doesn't return `in` to its callers. But it would lower the cognitive overhead a bit by keeping the coding style consistent. But please feel free to ignore this remark. Another thing I was wondering about is the recursive nature of these functions, and whether it can lead to similar issues like we recently had when parsing revisions with stack exhaustion. I _think_ this should not be much of a problem in this case though, as we're talking about mail headers here. While the length of header values isn't restricted per se (the line length is restricted to 1000, but with Comment Folding Whitespace values can span multiple lines), but mail provdiers will sure clamp down on mails with a "From:" header that is megabytes in size. So taken together, this looks good to me. Patrick
On Wed, Dec 13, 2023 at 08:07:21AM +0100, Patrick Steinhardt wrote: > > static const char *unquote_quoted_string(struct strbuf *outbuf, const char *in) > > { > > - int c; > > int take_next_literally = 0; > > > > - while ((c = *in++) != 0) { > > + while (*in) { > > + int c = *in++; > > if (take_next_literally == 1) { > > take_next_literally = 0; > > } else { > > I was wondering whether we want to convert `unquote_quoted_pair()` in > the same way. It's not subject to the same issue because it doesn't > return `in` to its callers. But it would lower the cognitive overhead a > bit by keeping the coding style consistent. But please feel free to > ignore this remark. I dunno. I don't think the consistency matters that much between functions, and on its own, the "while ((c = *in++))" pattern is not harmful. It's used elsewhere in the same file for other functions that are also fine (for decoding q/b headers). > Another thing I was wondering about is the recursive nature of these > functions, and whether it can lead to similar issues like we recently > had when parsing revisions with stack exhaustion. I _think_ this should > not be much of a problem in this case though, as we're talking about > mail headers here. While the length of header values isn't restricted > per se (the line length is restricted to 1000, but with Comment Folding > Whitespace values can span multiple lines), but mail provdiers will sure > clamp down on mails with a "From:" header that is megabytes in size. It's just unquote_comment() that is recursive, but yeah, there is nothing to stop it from recursing forever on "((((((...". The stack requirements are pretty small, though. I needed between 2^17 and 2^18 bytes to segfault on my machine using: perl -e 'print "From: ", "(" x 2**18;' | git mailinfo /dev/null /dev/null Absurdly big for an email, but maybe within the realm of possibility? I think it might be possible to drop the recursion and just use a depth counter, like this: diff --git a/mailinfo.c b/mailinfo.c index 737b9e5e13..db236f9f9f 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -59,6 +59,7 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line) static const char *unquote_comment(struct strbuf *outbuf, const char *in) { int take_next_literally = 0; + int depth = 1; strbuf_addch(outbuf, '('); @@ -72,11 +73,14 @@ static const char *unquote_comment(struct strbuf *outbuf, const char *in) take_next_literally = 1; continue; case '(': - in = unquote_comment(outbuf, in); + strbuf_addch(outbuf, '('); + depth++; continue; case ')': strbuf_addch(outbuf, ')'); - return in; + if (!--depth) + return in; + continue; } } I doubt it's a big deal either way, but if it's that easy to do it might be worth it. -Peff
Patrick Steinhardt <ps@pks.im> writes: >> static const char *unquote_quoted_string(struct strbuf *outbuf, const char *in) >> { >> - int c; >> int take_next_literally = 0; >> >> - while ((c = *in++) != 0) { >> + while (*in) { >> + int c = *in++; >> if (take_next_literally == 1) { >> take_next_literally = 0; >> } else { > > I was wondering whether we want to convert `unquote_quoted_pair()` in > the same way. It's not subject to the same issue because it doesn't > return `in` to its callers. But it would lower the cognitive overhead a > bit by keeping the coding style consistent. But please feel free to > ignore this remark. Yeah, I was wondering about the value of establishing a pattern that can be followed safely and with clarity, too. I also briefly wondered how bad if we picked the "compensate for the increment done by the last iteration before leaving the loop by returning (in-1)" idiom (which Peff called "a hacky way") to be that universal pattern, but this bug was a clear enough evidence that it does not work very well in developers' minds. I actually had trouble with the proposed update, and wondered if - while ((c = *in++) != 0) { + while ((c = *in)) { + in++; is easier to follow, but then was hit by the possibility that the same "we have incremented 'in' a bit too early" may exist if such a loop wants to use 'in' in its body. Wouldn't it mean that - while ((c = *in++) != 0) { + for (; c = *in; in++) { would be even a better rewrite? Enough bikeshedding... > Another thing I was wondering about is the recursive nature of these > functions, and whether it can lead to similar issues like we recently > had when parsing revisions with stack exhaustion. I _think_ this should > not be much of a problem in this case though, as we're talking about > mail headers here. While the length of header values isn't restricted > per se (the line length is restricted to 1000, but with Comment Folding > Whitespace values can span multiple lines), but mail provdiers will sure > clamp down on mails with a "From:" header that is megabytes in size. Good thinking, and I think Peff's iterative rewrite would be a good way to deal with it if it ever becomes an issue. > So taken together, this looks good to me. Thanks, both, for writing and reviewing.
On Wed, Dec 13, 2023 at 03:20:27AM -0500, Jeff King wrote: > On Wed, Dec 13, 2023 at 08:07:21AM +0100, Patrick Steinhardt wrote: [snip] > > Another thing I was wondering about is the recursive nature of these > > functions, and whether it can lead to similar issues like we recently > > had when parsing revisions with stack exhaustion. I _think_ this should > > not be much of a problem in this case though, as we're talking about > > mail headers here. While the length of header values isn't restricted > > per se (the line length is restricted to 1000, but with Comment Folding > > Whitespace values can span multiple lines), but mail provdiers will sure > > clamp down on mails with a "From:" header that is megabytes in size. > > It's just unquote_comment() that is recursive, but yeah, there is > nothing to stop it from recursing forever on "((((((...". The stack > requirements are pretty small, though. I needed between 2^17 and 2^18 > bytes to segfault on my machine using: > > perl -e 'print "From: ", "(" x 2**18;' | > git mailinfo /dev/null /dev/null > > Absurdly big for an email, but maybe within the realm of possibility? I > think it might be possible to drop the recursion and just use a depth > counter, like this: It's definitely not as large as I'd have expected it to be, we're only talking about kilobytes of data. Feels like it might be in the realm of possibility to get transferred by a mail provider. > diff --git a/mailinfo.c b/mailinfo.c > index 737b9e5e13..db236f9f9f 100644 > --- a/mailinfo.c > +++ b/mailinfo.c > @@ -59,6 +59,7 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line) > static const char *unquote_comment(struct strbuf *outbuf, const char *in) > { > int take_next_literally = 0; > + int depth = 1; > > strbuf_addch(outbuf, '('); > > @@ -72,11 +73,14 @@ static const char *unquote_comment(struct strbuf *outbuf, const char *in) > take_next_literally = 1; > continue; > case '(': > - in = unquote_comment(outbuf, in); > + strbuf_addch(outbuf, '('); > + depth++; > continue; > case ')': > strbuf_addch(outbuf, ')'); > - return in; > + if (!--depth) > + return in; > + continue; > } > } > > I doubt it's a big deal either way, but if it's that easy to do it might > be worth it. Isn't this only protecting against unbalanced braces? That might be a sensible check to do regardless, but does it protect against recursion blowing the stack if you just happen to have many opening braces? Might be I'm missing something. Patrick
On Wed, Dec 13, 2023 at 06:54:03AM -0800, Junio C Hamano wrote: > I actually had trouble with the proposed update, and wondered if > > - while ((c = *in++) != 0) { > + while ((c = *in)) { > + in++; > > is easier to follow, but then was hit by the possibility that the > same "we have incremented 'in' a bit too early" may exist if such > a loop wants to use 'in' in its body. Wouldn't it mean that > > - while ((c = *in++) != 0) { > + for (; c = *in; in++) { > > would be even a better rewrite? No, the "for" loop wouldn't work, because the loop body actually depends on "in" having already been incremented. If we find the end of the comment or quoted string, we return "in", and the caller is expecting it to have moved past the closing quote. So that would have to become "return in+1". IOW, the issue is that the normal end-of-quote parsing and hitting end-of-string are fundamentally different. So we either need to differentiate the returns (either with "+1" on one, or "-1" on the other). Or we need to choose to increment "in" based on which we found (which is what my patch does). -Peff
diff --git a/mailinfo.c b/mailinfo.c index a07d2da16d..737b9e5e13 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -58,12 +58,12 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line) static const char *unquote_comment(struct strbuf *outbuf, const char *in) { - int c; int take_next_literally = 0; strbuf_addch(outbuf, '('); - while ((c = *in++) != 0) { + while (*in) { + int c = *in++; if (take_next_literally == 1) { take_next_literally = 0; } else { @@ -88,10 +88,10 @@ static const char *unquote_comment(struct strbuf *outbuf, const char *in) static const char *unquote_quoted_string(struct strbuf *outbuf, const char *in) { - int c; int take_next_literally = 0; - while ((c = *in++) != 0) { + while (*in) { + int c = *in++; if (take_next_literally == 1) { take_next_literally = 0; } else { diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh index db11cababd..654d8cf3ee 100755 --- a/t/t5100-mailinfo.sh +++ b/t/t5100-mailinfo.sh @@ -268,4 +268,26 @@ test_expect_success 'mailinfo warn CR in base64 encoded email' ' test_must_be_empty quoted-cr/0002.err ' +test_expect_success 'from line with unterminated quoted string' ' + echo "From: bob \"unterminated string smith <bob@example.com>" >in && + git mailinfo /dev/null /dev/null <in >actual && + cat >expect <<-\EOF && + Author: bob unterminated string smith + Email: bob@example.com + + EOF + test_cmp expect actual +' + +test_expect_success 'from line with unterminated comment' ' + echo "From: bob (unterminated comment smith <bob@example.com>" >in && + git mailinfo /dev/null /dev/null <in >actual && + cat >expect <<-\EOF && + Author: bob (unterminated comment smith + Email: bob@example.com + + EOF + test_cmp expect actual +' + test_done
When processing a header like a "From" line, mailinfo uses unquote_quoted_pair() to handle double-quotes and rfc822 parenthesized comments. It takes a NUL-terminated string on input, and loops over the "in" pointer until it sees the NUL. When it finds the start of an interesting block, it delegates to helper functions which also increment "in", and return the updated pointer. But there's a bug here: the helpers find the NUL with a post-increment in the loop condition, like: while ((c = *in++) != 0) So when they do see a NUL (rather than the correct termination of the quote or comment section), they return "in" as one _past_ the NUL terminator. And thus the outer loop in unquote_quoted_pair() does not realize we hit the NUL, and keeps reading past the end of the buffer. We should instead make sure to return "in" positioned at the NUL, so that the caller knows to stop their loop, too. A hacky way to do this is to return "in - 1" after leaving the inner loop. But a slightly cleaner solution is to avoid incrementing "in" until we are sure it contained a non-NUL byte (i.e., doing it inside the loop body). The two tests here show off the problem. Since we check the output, they'll _usually_ report a failure in a normal build, but it depends on what garbage bytes are found after the heap buffer. Building with SANITIZE=address reliably notices the problem. The outcome (both the exit code and the exact bytes) are just what Git happens to produce for these cases today, and shouldn't be taken as an endorsement. It might be reasonable to abort on an unterminated string, for example. The priority for this patch is fixing the out-of-bounds memory access. Reported-by: Carlos Andrés Ramírez Cataño <antaigroupltda@gmail.com> Signed-off-by: Jeff King <peff@peff.net> --- This was reported to the security list, but because it's just an out-of-bounds read, we won't do a coordinated disclosure. mailinfo.c | 8 ++++---- t/t5100-mailinfo.sh | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-)