From patchwork Thu Dec 14 21:47:46 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13493720 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 81A382C6B3 for ; Thu, 14 Dec 2023 21:47:48 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 8803 invoked by uid 109); 14 Dec 2023 21:47:47 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 14 Dec 2023 21:47:47 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 11676 invoked by uid 111); 14 Dec 2023 21:47:46 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 14 Dec 2023 16:47:46 -0500 Authentication-Results: peff.net; auth=none Date: Thu, 14 Dec 2023 16:47:46 -0500 From: Jeff King To: Patrick Steinhardt Cc: git@vger.kernel.org, Taylor Blau , Carlos =?utf-8?b?QW5kcsOpcyBSYW3DrXJleiBDYXRhw7Fv?= Subject: [PATCH 1/2] t5100: make rfc822 comment test more careful Message-ID: <20231214214746.GA2798346@coredump.intra.peff.net> References: <20231214214444.GB2297853@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231214214444.GB2297853@coredump.intra.peff.net> When processing "From" headers in an email, mailinfo "unquotes" quoted strings and rfc822 parenthesized comments. For quoted strings, we actually remove the double-quotes, so: From: "A U Thor" become: Author: A U Thor Email: someone@example.com But for comments, we leave the outer parentheses in place, so: From: A U (this is a comment) Thor becomes: Author: A U (this is a comment) Thor Email: someone@example.com So what is the comment "unquoting" actually doing? In our code, being in a comment section has exactly two effects: 1. We'll unquote backslash-escaped characters inside a comment section. 2. We _won't_ unquote double-quoted strings inside a comment section. Our test for comments in t5100 checks this: From: "A U Thor" (this is \(really\) a comment (honestly)) So it is covering (1), but not (2). Let's add in a quoted string to cover this. Moreover, because the comment appears at the end of the From header, there's nothing to confirm that we correctly found the end of the comment section (and not just the end-of-string). Let's instead move it to the beginning of the header, which means we can confirm that the existing quoted string is detected (which will only happen if we know we've left the comment block). As expected, the test continues to pass, but this will give us more confidence as we refactor the code in the next patch. Signed-off-by: Jeff King --- I did this as a separate patch so it is easy to see that the existing code already passes the test (i.e., our refactor is a true noop in terms of behavior, and not fixing or breaking anything). t/t5100/comment.expect | 2 +- t/t5100/comment.in | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t5100/comment.expect b/t/t5100/comment.expect index 7228177984..bd71956a47 100644 --- a/t/t5100/comment.expect +++ b/t/t5100/comment.expect @@ -1,4 +1,4 @@ -Author: A U Thor (this is (really) a comment (honestly)) +Author: (this is (really) a "comment" (honestly)) A U Thor Email: somebody@example.com Subject: testing comments Date: Sun, 25 May 2008 00:38:18 -0700 diff --git a/t/t5100/comment.in b/t/t5100/comment.in index c53a192dfe..0b7e903b06 100644 --- a/t/t5100/comment.in +++ b/t/t5100/comment.in @@ -1,5 +1,5 @@ From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001 -From: "A U Thor" (this is \(really\) a comment (honestly)) +From: (this is \(really\) a "comment" (honestly)) "A U Thor" Date: Sun, 25 May 2008 00:38:18 -0700 Subject: [PATCH] testing comments From patchwork Thu Dec 14 21:48:59 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13493721 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A2F04671E9 for ; Thu, 14 Dec 2023 21:49:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 8815 invoked by uid 109); 14 Dec 2023 21:49:00 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 14 Dec 2023 21:49:00 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 11686 invoked by uid 111); 14 Dec 2023 21:48:59 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 14 Dec 2023 16:48:59 -0500 Authentication-Results: peff.net; auth=none Date: Thu, 14 Dec 2023 16:48:59 -0500 From: Jeff King To: Patrick Steinhardt Cc: git@vger.kernel.org, Taylor Blau , Carlos =?utf-8?b?QW5kcsOpcyBSYW3DrXJleiBDYXRhw7Fv?= Subject: [PATCH 2/2] mailinfo: avoid recursion when unquoting From headers Message-ID: <20231214214859.GB2798346@coredump.intra.peff.net> References: <20231214214444.GB2297853@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline 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 --- 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; } }