git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 2/2] mailinfo: avoid recursion when unquoting From headers

From
Jeff King <peff@peff.net>
Date
Dec 14, 2023, 21:48 UTC
Message-ID
<20231214214859.GB2798346@coredump.intra.peff.net>
In-Reply-To
<20231214214444.GB2297853@coredump.intra.peff.net>

Our unquote_comment() function is recursive; when it sees a comment within a comment, like:

  (this is an (embedded) comment)

it recurses to handle the inner comment. This is fine for practical use, but it does mean that you can easily run out of stack space with a malicious header. For example:

  perl -e 'print "From: ", "(" x 2**18;' |
  git mailinfo /dev/null /dev/null

segfaults on my system. And since mailinfo is likely to be fed untrusted input from the Internet (if not by human users, who might recognize a garbage header, but certainly there are automated systems that apply patches from a list) it may be possible for an attacker to trigger the problem.

That said, I don't think there's an interesting security vulnerability here. All an attacker can do is make it impossible to parse their email and apply their patch, and there are lots of ways to generate bogus emails. So it's more of an annoyance than anything.

But it's pretty easy to fix it. The recursion is not helping us preserve any particular state from each level. The only flag in our parsing is take_next_literally, and we can never recurse when it is set (since the start of a new comment implies it was not backslash-escaped). So it is really only useful for finding the end of the matched pair of parentheses. We can do that easily with a simple depth counter.

Signed-off-by: Jeff King <peff@peff.net>
---
 mailinfo.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
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;
 			}
 		}
 
-- 
2.43.0.363.g1d22a8f302
Previous: Jeff KingNext: Patrick Steinhardt
Message 7 of 10 in “mailinfo: fix out-of-bounds memory reads in unquote_quoted_pair()”
  1. mailinfo: fix out-of-bounds memory reads in unquote_quoted_pair()Jeff King, Dec 12, 2023
  2. Patrick SteinhardtDec 13, 2023
  3. Jeff KingDec 13, 2023
  4. Patrick SteinhardtDec 14, 2023
  5. 0/2 avoiding recursion in mailinfoJeff King, Dec 14, 2023
  6. 1/2 t5100: make rfc822 comment test more carefulJeff King, Dec 14, 2023
  7. 2/2 mailinfo: avoid recursion when unquoting From headersJeff King, Dec 14, 2023
  8. Patrick SteinhardtDec 15, 2023
  9. Junio C HamanoDec 13, 2023
  10. Jeff KingDec 14, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.