[PATCH] imap-send: use xsnprintf to format command

[PATCH] imap-send: use xsnprintf to format command

[René Scharfe]

nfsnprintf() wraps vsnprintf(3) and reports attempts to use too small a
buffer using BUG(), just like xsnprintf().

It has an extra check that makes sure the buffer size (converted to int)
is positive.  vsnprintf(3) is supposed to handle a buffer size of zero
or bigger than INT_MAX just fine, so this extra comparison doesn't make
us any safer.  If a platform has a broken implementation, we'd need to
work around it in our compat code.

Call xsnprintf() instead to reduce code duplication and make the caller
slightly more readable by using this more common helper.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
Uploaded this message using the patched command, but that's all the
testing I did.

 imap-send.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index f2e1947e63..4caa8668e6 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -68,9 +68,6 @@ static void imap_warn(const char *, ...);

 static char *next_arg(char **);

-__attribute__((format (printf, 3, 4)))
-static int nfsnprintf(char *buf, int blen, const char *fmt, ...);
-
 static int nfvasprintf(char **strp, const char *fmt, va_list ap)
 {
 	int len;
@@ -500,19 +497,6 @@ static char *next_arg(char **s)
 	return ret;
 }

-__attribute__((format (printf, 3, 4)))
-static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
-{
-	int ret;
-	va_list va;
-
-	va_start(va, fmt);
-	if (blen <= 0 || (unsigned)(ret = vsnprintf(buf, blen, fmt, va)) >= (unsigned)blen)
-		BUG("buffer too small. Please report a bug.");
-	va_end(va);
-	return ret;
-}
-
 static struct imap_cmd *issue_imap_cmd(struct imap_store *ctx,
 				       struct imap_cmd_cb *cb,
 				       const char *fmt, va_list ap)
@@ -535,11 +519,11 @@ static struct imap_cmd *issue_imap_cmd(struct imap_store *ctx,
 		get_cmd_result(ctx, NULL);

 	if (!cmd->cb.data)
-		bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd);
+		bufl = xsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd);
 	else
-		bufl = nfsnprintf(buf, sizeof(buf), "%d %s{%d%s}\r\n",
-				  cmd->tag, cmd->cmd, cmd->cb.dlen,
-				  CAP(LITERALPLUS) ? "+" : "");
+		bufl = xsnprintf(buf, sizeof(buf), "%d %s{%d%s}\r\n",
+				 cmd->tag, cmd->cmd, cmd->cb.dlen,
+				 CAP(LITERALPLUS) ? "+" : "");

 	if (0 < verbosity) {
 		if (imap->num_in_progress)
--
2.44.0

Re: [PATCH] imap-send: use xsnprintf to format command

[Jeff King]

On Tue, Apr 02, 2024 at 04:51:05PM +0200, René Scharfe wrote:

> nfsnprintf() wraps vsnprintf(3) and reports attempts to use too small a
> buffer using BUG(), just like xsnprintf().
> 
> It has an extra check that makes sure the buffer size (converted to int)
> is positive.  vsnprintf(3) is supposed to handle a buffer size of zero
> or bigger than INT_MAX just fine, so this extra comparison doesn't make
> us any safer.  If a platform has a broken implementation, we'd need to
> work around it in our compat code.
> 
> Call xsnprintf() instead to reduce code duplication and make the caller
> slightly more readable by using this more common helper.

I think this is an improvement, and since the original also called BUG()
we are not making anything worse. But I think the original was actually
in error to do so, since it depends on user data. E.g., setting
imap.user to the output of $(perl -e 'print "a" x 1024"') results in:

  BUG: imap-send.c:511: buffer too small. Please report a bug.
  error: git-imap-send died of signal 6

Now obviously that's a dumb username, but this is the buffer used for
all IMAP commands. So conceivably long folder names, etc, do the same
thing. It's probably not a big deal in practice, but conceptually this
probably ought to be xstrfmt() and not xsnprintf().

Likewise imap-send's nfvasprintf() is basically xstrfmt(), except it
takes a va_list. So it would have to be replaced by strbuf_vaddf().

I wouldn't be surprised if there are other opportunities for string
cleanup, but I generally hoped that if we waited long enough imap-send
would just go away. ;) Either because we could get rid of the tool
entirely (though from a recent-ish search, there did not seem to be a
lot of good other tools) or because we'd just drop the old code and rely
on curl to do the heavy lifting.

All that said, I think your patch makes the world slightly better, so
I'm not opposed in the meantime.

-Peff

Re: [PATCH] imap-send: use xsnprintf to format command

[René Scharfe]

Am 03.04.24 um 02:47 schrieb Jeff King:
> On Tue, Apr 02, 2024 at 04:51:05PM +0200, René Scharfe wrote:
>
>> nfsnprintf() wraps vsnprintf(3) and reports attempts to use too small a
>> buffer using BUG(), just like xsnprintf().
>>
>> It has an extra check that makes sure the buffer size (converted to int)
>> is positive.  vsnprintf(3) is supposed to handle a buffer size of zero
>> or bigger than INT_MAX just fine, so this extra comparison doesn't make
>> us any safer.  If a platform has a broken implementation, we'd need to
>> work around it in our compat code.
>>
>> Call xsnprintf() instead to reduce code duplication and make the caller
>> slightly more readable by using this more common helper.
>
> I think this is an improvement, and since the original also called BUG()
> we are not making anything worse. But I think the original was actually
> in error to do so, since it depends on user data. E.g., setting
> imap.user to the output of $(perl -e 'print "a" x 1024"') results in:
>
>   BUG: imap-send.c:511: buffer too small. Please report a bug.
>   error: git-imap-send died of signal 6
>
> Now obviously that's a dumb username, but this is the buffer used for
> all IMAP commands. So conceivably long folder names, etc, do the same
> thing. It's probably not a big deal in practice, but conceptually this
> probably ought to be xstrfmt() and not xsnprintf().

From reading the code I assumed the static buffer is there to stay
within some IMAP limit.  RFC 9051 mentions the distinction between
synchronizing and non-synchronizing literals.  The latter have a maximum
length of 4096 bytes.

But those are transferred after the command, so have no relevance for
the command buffer size.  I see no other limits, and I don't see us
respecting that non-synchronizing literals limit, either.  I guess that
means messages longer than 4096 bytes could be rejected by a conforming
IMAP server?  Hmm.

> Likewise imap-send's nfvasprintf() is basically xstrfmt(), except it
> takes a va_list. So it would have to be replaced by strbuf_vaddf().

Looking closer I notice that the result of the single nfvasprintf() call
is fed into the 1024 bytes buffer.  So we could replace it with
strbuf_vaddf() or xstrvfmt() and still stay within that strange limit,
as it's enforced later.

Its own 8192 buffer shields us from huge allocations e.g. due to long
usernames or paths, but we probably don't need this protection as such
and "attack" would originate and be felt only locally.

> I wouldn't be surprised if there are other opportunities for string
> cleanup, but I generally hoped that if we waited long enough imap-send
> would just go away. ;) Either because we could get rid of the tool
> entirely (though from a recent-ish search, there did not seem to be a
> lot of good other tools) or because we'd just drop the old code and rely
> on curl to do the heavy lifting.

Oh, my build uses curl, so my earlier test run was even worth less than
I thought.

René

Re: [PATCH] imap-send: use xsnprintf to format command

[Jeff King]

On Wed, Apr 03, 2024 at 11:25:42AM +0200, René Scharfe wrote:

> From reading the code I assumed the static buffer is there to stay
> within some IMAP limit.  RFC 9051 mentions the distinction between
> synchronizing and non-synchronizing literals.  The latter have a maximum
> length of 4096 bytes.
> 
> But those are transferred after the command, so have no relevance for
> the command buffer size.  I see no other limits, and I don't see us
> respecting that non-synchronizing literals limit, either.  I guess that
> means messages longer than 4096 bytes could be rejected by a conforming
> IMAP server?  Hmm.

It can also just be a quoted string, which likewise has no limit defined
in that section. That's what we send for LOGIN (you have to go back to
the imap_exec() command which uses a format string). It also looks like
that would barf completely on a username or password that contains a
double-quote. Yup:

  $ git format-patch -1 --stdout |
    git -c imap.user='my"user"' -c imap.pass=foo imap-send --no-curl
  [...]
  IMAP command 'LOGIN <user> <pass>' returned response (BAD) - Invalid characters in atom
  IMAP error: LOGIN failed

> > Likewise imap-send's nfvasprintf() is basically xstrfmt(), except it
> > takes a va_list. So it would have to be replaced by strbuf_vaddf().
> 
> Looking closer I notice that the result of the single nfvasprintf() call
> is fed into the 1024 bytes buffer.  So we could replace it with
> strbuf_vaddf() or xstrvfmt() and still stay within that strange limit,
> as it's enforced later.

Oh, I forgot we had xstrvfmt(). That would obviously be the right match.

> Its own 8192 buffer shields us from huge allocations e.g. due to long
> usernames or paths, but we probably don't need this protection as such
> and "attack" would originate and be felt only locally.

Yeah, I think all of the data here is user controlled. Even if you
didn't trust the patch itself, this is all username, mailbox name, etc.

> > I wouldn't be surprised if there are other opportunities for string
> > cleanup, but I generally hoped that if we waited long enough imap-send
> > would just go away. ;) Either because we could get rid of the tool
> > entirely (though from a recent-ish search, there did not seem to be a
> > lot of good other tools) or because we'd just drop the old code and rely
> > on curl to do the heavy lifting.
> 
> Oh, my build uses curl, so my earlier test run was even worth less than
> I thought.

Heh.

-Peff