{"thread":{"id":"61253","subject":"[PATCH] imap-send: use xsnprintf to format command","startedAt":"2024-04-02T14:56:29Z","lastAt":"2024-04-03T20:32:22Z","messageCount":4,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"492060","messageId":"f9ad9f41-5b9b-474e-9818-f91fc937daae@web.de","threadId":"61253","inReplyTo":null,"subject":"[PATCH] imap-send: use xsnprintf to format command","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-02T14:51:05Z","receivedAt":"2024-04-02T14:56:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"nfsnprintf() wraps vsnprintf(3) and reports attempts to use too small a\nbuffer using BUG(), just like xsnprintf().\n\nIt has an extra check that makes sure the buffer size (converted to int)\nis positive.  vsnprintf(3) is supposed to handle a buffer size of zero\nor bigger than INT_MAX just fine, so this extra comparison doesn't make\nus any safer.  If a platform has a broken implementation, we'd need to\nwork around it in our compat code.\n\nCall xsnprintf() instead to reduce code duplication and make the caller\nslightly more readable by using this more common helper.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nUploaded this message using the patched command, but that's all the\ntesting I did.\n\n imap-send.c | 24 ++++--------------------\n 1 file changed, 4 insertions(+), 20 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex f2e1947e63..4caa8668e6 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -68,9 +68,6 @@ static void imap_warn(const char *, ...);\n\n static char *next_arg(char **);\n\n-__attribute__((format (printf, 3, 4)))\n-static int nfsnprintf(char *buf, int blen, const char *fmt, ...);\n-\n static int nfvasprintf(char **strp, const char *fmt, va_list ap)\n {\n \tint len;\n@@ -500,19 +497,6 @@ static char *next_arg(char **s)\n \treturn ret;\n }\n\n-__attribute__((format (printf, 3, 4)))\n-static int nfsnprintf(char *buf, int blen, const char *fmt, ...)\n-{\n-\tint ret;\n-\tva_list va;\n-\n-\tva_start(va, fmt);\n-\tif (blen <= 0 || (unsigned)(ret = vsnprintf(buf, blen, fmt, va)) >= (unsigned)blen)\n-\t\tBUG(\"buffer too small. Please report a bug.\");\n-\tva_end(va);\n-\treturn ret;\n-}\n-\n static struct imap_cmd *issue_imap_cmd(struct imap_store *ctx,\n \t\t\t\t       struct imap_cmd_cb *cb,\n \t\t\t\t       const char *fmt, va_list ap)\n@@ -535,11 +519,11 @@ static struct imap_cmd *issue_imap_cmd(struct imap_store *ctx,\n \t\tget_cmd_result(ctx, NULL);\n\n \tif (!cmd->cb.data)\n-\t\tbufl = nfsnprintf(buf, sizeof(buf), \"%d %s\\r\\n\", cmd->tag, cmd->cmd);\n+\t\tbufl = xsnprintf(buf, sizeof(buf), \"%d %s\\r\\n\", cmd->tag, cmd->cmd);\n \telse\n-\t\tbufl = nfsnprintf(buf, sizeof(buf), \"%d %s{%d%s}\\r\\n\",\n-\t\t\t\t  cmd->tag, cmd->cmd, cmd->cb.dlen,\n-\t\t\t\t  CAP(LITERALPLUS) ? \"+\" : \"\");\n+\t\tbufl = xsnprintf(buf, sizeof(buf), \"%d %s{%d%s}\\r\\n\",\n+\t\t\t\t cmd->tag, cmd->cmd, cmd->cb.dlen,\n+\t\t\t\t CAP(LITERALPLUS) ? \"+\" : \"\");\n\n \tif (0 < verbosity) {\n \t\tif (imap->num_in_progress)\n--\n2.44.0\n"},{"id":"492126","messageId":"20240403004751.GA892394@coredump.intra.peff.net","threadId":"61253","inReplyTo":"f9ad9f41-5b9b-474e-9818-f91fc937daae@web.de","subject":"Re: [PATCH] imap-send: use xsnprintf to format command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-03T00:47:51Z","receivedAt":"2024-04-03T00:47:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 02, 2024 at 04:51:05PM +0200, René Scharfe wrote:\n\n> nfsnprintf() wraps vsnprintf(3) and reports attempts to use too small a\n> buffer using BUG(), just like xsnprintf().\n> \n> It has an extra check that makes sure the buffer size (converted to int)\n> is positive.  vsnprintf(3) is supposed to handle a buffer size of zero\n> or bigger than INT_MAX just fine, so this extra comparison doesn't make\n> us any safer.  If a platform has a broken implementation, we'd need to\n> work around it in our compat code.\n> \n> Call xsnprintf() instead to reduce code duplication and make the caller\n> slightly more readable by using this more common helper.\n\nI think this is an improvement, and since the original also called BUG()\nwe are not making anything worse. But I think the original was actually\nin error to do so, since it depends on user data. E.g., setting\nimap.user to the output of $(perl -e 'print \"a\" x 1024\"') results in:\n\n  BUG: imap-send.c:511: buffer too small. Please report a bug.\n  error: git-imap-send died of signal 6\n\nNow obviously that's a dumb username, but this is the buffer used for\nall IMAP commands. So conceivably long folder names, etc, do the same\nthing. It's probably not a big deal in practice, but conceptually this\nprobably ought to be xstrfmt() and not xsnprintf().\n\nLikewise imap-send's nfvasprintf() is basically xstrfmt(), except it\ntakes a va_list. So it would have to be replaced by strbuf_vaddf().\n\nI wouldn't be surprised if there are other opportunities for string\ncleanup, but I generally hoped that if we waited long enough imap-send\nwould just go away. ;) Either because we could get rid of the tool\nentirely (though from a recent-ish search, there did not seem to be a\nlot of good other tools) or because we'd just drop the old code and rely\non curl to do the heavy lifting.\n\nAll that said, I think your patch makes the world slightly better, so\nI'm not opposed in the meantime.\n\n-Peff\n"},{"id":"492145","messageId":"970c27ec-c377-4034-853a-0251733ec50c@web.de","threadId":"61253","inReplyTo":"20240403004751.GA892394@coredump.intra.peff.net","subject":"Re: [PATCH] imap-send: use xsnprintf to format command","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-03T09:25:42Z","receivedAt":"2024-04-03T09:31:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.04.24 um 02:47 schrieb Jeff King:\n> On Tue, Apr 02, 2024 at 04:51:05PM +0200, René Scharfe wrote:\n>\n>> nfsnprintf() wraps vsnprintf(3) and reports attempts to use too small a\n>> buffer using BUG(), just like xsnprintf().\n>>\n>> It has an extra check that makes sure the buffer size (converted to int)\n>> is positive.  vsnprintf(3) is supposed to handle a buffer size of zero\n>> or bigger than INT_MAX just fine, so this extra comparison doesn't make\n>> us any safer.  If a platform has a broken implementation, we'd need to\n>> work around it in our compat code.\n>>\n>> Call xsnprintf() instead to reduce code duplication and make the caller\n>> slightly more readable by using this more common helper.\n>\n> I think this is an improvement, and since the original also called BUG()\n> we are not making anything worse. But I think the original was actually\n> in error to do so, since it depends on user data. E.g., setting\n> imap.user to the output of $(perl -e 'print \"a\" x 1024\"') results in:\n>\n>   BUG: imap-send.c:511: buffer too small. Please report a bug.\n>   error: git-imap-send died of signal 6\n>\n> Now obviously that's a dumb username, but this is the buffer used for\n> all IMAP commands. So conceivably long folder names, etc, do the same\n> thing. It's probably not a big deal in practice, but conceptually this\n> probably ought to be xstrfmt() and not xsnprintf().\n\nFrom reading the code I assumed the static buffer is there to stay\nwithin some IMAP limit.  RFC 9051 mentions the distinction between\nsynchronizing and non-synchronizing literals.  The latter have a maximum\nlength of 4096 bytes.\n\nBut those are transferred after the command, so have no relevance for\nthe command buffer size.  I see no other limits, and I don't see us\nrespecting that non-synchronizing literals limit, either.  I guess that\nmeans messages longer than 4096 bytes could be rejected by a conforming\nIMAP server?  Hmm.\n\n> Likewise imap-send's nfvasprintf() is basically xstrfmt(), except it\n> takes a va_list. So it would have to be replaced by strbuf_vaddf().\n\nLooking closer I notice that the result of the single nfvasprintf() call\nis fed into the 1024 bytes buffer.  So we could replace it with\nstrbuf_vaddf() or xstrvfmt() and still stay within that strange limit,\nas it's enforced later.\n\nIts own 8192 buffer shields us from huge allocations e.g. due to long\nusernames or paths, but we probably don't need this protection as such\nand \"attack\" would originate and be felt only locally.\n\n> I wouldn't be surprised if there are other opportunities for string\n> cleanup, but I generally hoped that if we waited long enough imap-send\n> would just go away. ;) Either because we could get rid of the tool\n> entirely (though from a recent-ish search, there did not seem to be a\n> lot of good other tools) or because we'd just drop the old code and rely\n> on curl to do the heavy lifting.\n\nOh, my build uses curl, so my earlier test run was even worth less than\nI thought.\n\nRené\n"},{"id":"492187","messageId":"20240403203221.GB1949464@coredump.intra.peff.net","threadId":"61253","inReplyTo":"970c27ec-c377-4034-853a-0251733ec50c@web.de","subject":"Re: [PATCH] imap-send: use xsnprintf to format command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-03T20:32:21Z","receivedAt":"2024-04-03T20:32:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 03, 2024 at 11:25:42AM +0200, René Scharfe wrote:\n\n> From reading the code I assumed the static buffer is there to stay\n> within some IMAP limit.  RFC 9051 mentions the distinction between\n> synchronizing and non-synchronizing literals.  The latter have a maximum\n> length of 4096 bytes.\n> \n> But those are transferred after the command, so have no relevance for\n> the command buffer size.  I see no other limits, and I don't see us\n> respecting that non-synchronizing literals limit, either.  I guess that\n> means messages longer than 4096 bytes could be rejected by a conforming\n> IMAP server?  Hmm.\n\nIt can also just be a quoted string, which likewise has no limit defined\nin that section. That's what we send for LOGIN (you have to go back to\nthe imap_exec() command which uses a format string). It also looks like\nthat would barf completely on a username or password that contains a\ndouble-quote. Yup:\n\n  $ git format-patch -1 --stdout |\n    git -c imap.user='my\"user\"' -c imap.pass=foo imap-send --no-curl\n  [...]\n  IMAP command 'LOGIN <user> <pass>' returned response (BAD) - Invalid characters in atom\n  IMAP error: LOGIN failed\n\n> > Likewise imap-send's nfvasprintf() is basically xstrfmt(), except it\n> > takes a va_list. So it would have to be replaced by strbuf_vaddf().\n> \n> Looking closer I notice that the result of the single nfvasprintf() call\n> is fed into the 1024 bytes buffer.  So we could replace it with\n> strbuf_vaddf() or xstrvfmt() and still stay within that strange limit,\n> as it's enforced later.\n\nOh, I forgot we had xstrvfmt(). That would obviously be the right match.\n\n> Its own 8192 buffer shields us from huge allocations e.g. due to long\n> usernames or paths, but we probably don't need this protection as such\n> and \"attack\" would originate and be felt only locally.\n\nYeah, I think all of the data here is user controlled. Even if you\ndidn't trust the patch itself, this is all username, mailbox name, etc.\n\n> > I wouldn't be surprised if there are other opportunities for string\n> > cleanup, but I generally hoped that if we waited long enough imap-send\n> > would just go away. ;) Either because we could get rid of the tool\n> > entirely (though from a recent-ish search, there did not seem to be a\n> > lot of good other tools) or because we'd just drop the old code and rely\n> > on curl to do the heavy lifting.\n> \n> Oh, my build uses curl, so my earlier test run was even worth less than\n> I thought.\n\nHeh.\n\n-Peff\n"}]}