{"thread":{"id":"61317","subject":"[PATCH] imap-send: increase command size limit","startedAt":"2024-04-14T16:48:01Z","lastAt":"2024-04-17T00:25:02Z","messageCount":8,"participants":["René Scharfe","brian m. carlson","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"492924","messageId":"7026075c-db4e-4d43-bbd1-d2edb52da9b7@web.de","threadId":"61317","inReplyTo":null,"subject":"[PATCH] imap-send: increase command size limit","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-14T16:47:52Z","receivedAt":"2024-04-14T16:48:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"nfvasprintf() has a 8KB limit, but it's not relevant, as its result is\ncombined with other strings and added to a 1KB buffer by its caller.\nThat 1KB limit is not mentioned in RFC 9051, which specifies IMAP.\n\nWhile 1KB is plenty for user names, passwords and mailbox names,\nthere's no point in limiting our commands like that.  Call xstrvfmt()\ninstead of open-coding it and use strbuf to format the command to\nsend, as we need its length.  Fail hard if it exceeds INT_MAX, because\nsocket_write() can't take more than that.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nThis time I compiled with NO_CURL=1 and NO_APPLE_COMMON_CRYPTO=1 and\nverified with a silly printf that the changed code was actually used\nand wrote the present message to an IMAP folder whose name is 1006\ncharacters log, which required a 1026 bytes long APPEND command.  Yay,\nfreedom!\n\nRan into a build issue, will send a separate patch for that.\n\n imap-send.c | 35 ++++++++++++-----------------------\n 1 file changed, 12 insertions(+), 23 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 4caa8668e6..0afd088d8a 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -68,20 +68,6 @@ static void imap_warn(const char *, ...);\n\n static char *next_arg(char **);\n\n-static int nfvasprintf(char **strp, const char *fmt, va_list ap)\n-{\n-\tint len;\n-\tchar tmp[8192];\n-\n-\tlen = vsnprintf(tmp, sizeof(tmp), fmt, ap);\n-\tif (len < 0)\n-\t\tdie(\"Fatal: Out of memory\");\n-\tif (len >= sizeof(tmp))\n-\t\tdie(\"imap command overflow!\");\n-\t*strp = xmemdupz(tmp, len);\n-\treturn len;\n-}\n-\n struct imap_server_conf {\n \tconst char *name;\n \tconst char *tunnel;\n@@ -503,11 +489,11 @@ static struct imap_cmd *issue_imap_cmd(struct imap_store *ctx,\n {\n \tstruct imap *imap = ctx->imap;\n \tstruct imap_cmd *cmd;\n-\tint n, bufl;\n-\tchar buf[1024];\n+\tint n;\n+\tstruct strbuf buf = STRBUF_INIT;\n\n \tcmd = xmalloc(sizeof(struct imap_cmd));\n-\tnfvasprintf(&cmd->cmd, fmt, ap);\n+\tcmd->cmd = xstrvfmt(fmt, ap);\n \tcmd->tag = ++imap->nexttag;\n\n \tif (cb)\n@@ -519,27 +505,30 @@ 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 = xsnprintf(buf, sizeof(buf), \"%d %s\\r\\n\", cmd->tag, cmd->cmd);\n+\t\tstrbuf_addf(&buf, \"%d %s\\r\\n\", cmd->tag, cmd->cmd);\n \telse\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+\t\tstrbuf_addf(&buf, \"%d %s{%d%s}\\r\\n\", cmd->tag, cmd->cmd,\n+\t\t\t    cmd->cb.dlen, CAP(LITERALPLUS) ? \"+\" : \"\");\n+\tif (buf.len > INT_MAX)\n+\t\tdie(\"imap command overflow!\");\n\n \tif (0 < verbosity) {\n \t\tif (imap->num_in_progress)\n \t\t\tprintf(\"(%d in progress) \", imap->num_in_progress);\n \t\tif (!starts_with(cmd->cmd, \"LOGIN\"))\n-\t\t\tprintf(\">>> %s\", buf);\n+\t\t\tprintf(\">>> %s\", buf.buf);\n \t\telse\n \t\t\tprintf(\">>> %d LOGIN <user> <pass>\\n\", cmd->tag);\n \t}\n-\tif (socket_write(&imap->buf.sock, buf, bufl) != bufl) {\n+\tif (socket_write(&imap->buf.sock, buf.buf, buf.len) != buf.len) {\n \t\tfree(cmd->cmd);\n \t\tfree(cmd);\n \t\tif (cb)\n \t\t\tfree(cb->data);\n+\t\tstrbuf_release(&buf);\n \t\treturn NULL;\n \t}\n+\tstrbuf_release(&buf);\n \tif (cmd->cb.data) {\n \t\tif (CAP(LITERALPLUS)) {\n \t\t\tn = socket_write(&imap->buf.sock, cmd->cb.data, cmd->cb.dlen);\n--\n2.44.0\n\n"},{"id":"492925","messageId":"ZhwV6CmcC8zeSJ-7@tapette.crustytoothpaste.net","threadId":"61317","inReplyTo":"7026075c-db4e-4d43-bbd1-d2edb52da9b7@web.de","subject":"Re: [PATCH] imap-send: increase command size limit","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-04-14T17:44:08Z","receivedAt":"2024-04-14T17:44:16Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-04-14 at 16:47:52, René Scharfe wrote:\n> nfvasprintf() has a 8KB limit, but it's not relevant, as its result is\n> combined with other strings and added to a 1KB buffer by its caller.\n> That 1KB limit is not mentioned in RFC 9051, which specifies IMAP.\n> \n> While 1KB is plenty for user names, passwords and mailbox names,\n> there's no point in limiting our commands like that.  Call xstrvfmt()\n> instead of open-coding it and use strbuf to format the command to\n> send, as we need its length.  Fail hard if it exceeds INT_MAX, because\n> socket_write() can't take more than that.\n> \n> Suggested-by: Jeff King <peff@peff.net>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> This time I compiled with NO_CURL=1 and NO_APPLE_COMMON_CRYPTO=1 and\n> verified with a silly printf that the changed code was actually used\n> and wrote the present message to an IMAP folder whose name is 1006\n> characters log, which required a 1026 bytes long APPEND command.  Yay,\n> freedom!\n\nI'm curious, is there a particular problem that you (or someone else)\nran into that caused you to make this change?  I agree it seems prudent\nin general, but if there's a particular real-world broken case that this\nhits (e.g., mailbox names in a given language), I think the commit\nmessage would be a great place to mention this real-world impact, which\nwould lend support to your argument that this is a valuable change to\nmake.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"492958","messageId":"xmqqil0impy3.fsf@gitster.g","threadId":"61317","inReplyTo":"ZhwV6CmcC8zeSJ-7@tapette.crustytoothpaste.net","subject":"Re: [PATCH] imap-send: increase command size limit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-15T18:38:28Z","receivedAt":"2024-04-15T18:38:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n>>  ...\n>> Suggested-by: Jeff King <peff@peff.net>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>> This time I compiled with NO_CURL=1 and NO_APPLE_COMMON_CRYPTO=1 and\n>> verified with a silly printf that the changed code was actually used\n>> and wrote the present message to an IMAP folder whose name is 1006\n>> characters log, which required a 1026 bytes long APPEND command.  Yay,\n>> freedom!\n>\n> I'm curious, is there a particular problem that you (or someone else)\n> ran into that caused you to make this change?  I agree it seems prudent\n> in general, but if there's a particular real-world broken case that this\n> hits (e.g., mailbox names in a given language), I think the commit\n> message would be a great place to mention this real-world impact, which\n> would lend support to your argument that this is a valuable change to\n> make.\n\nI personally am not curious about real-world problem in this case,\nbut I won't stop you asking ;-)\n\nI view this more about code simplification.  We no longer need a\ncustom nfvasprintf() helper nobody else cares about, leaving the\nresulting code easier to read.\n\nWill queue but will wait for a day or two to see if René wants to\nadd clarification to the proposed commit log message before merging\nit to 'next'.\n\nThanks, both.\n"},{"id":"492960","messageId":"20240415184552.GA1709228@coredump.intra.peff.net","threadId":"61317","inReplyTo":"xmqqil0impy3.fsf@gitster.g","subject":"Re: [PATCH] imap-send: increase command size limit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-15T18:45:52Z","receivedAt":"2024-04-15T18:52:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 15, 2024 at 11:38:28AM -0700, Junio C Hamano wrote:\n\n> > I'm curious, is there a particular problem that you (or someone else)\n> > ran into that caused you to make this change?  I agree it seems prudent\n> > in general, but if there's a particular real-world broken case that this\n> > hits (e.g., mailbox names in a given language), I think the commit\n> > message would be a great place to mention this real-world impact, which\n> > would lend support to your argument that this is a valuable change to\n> > make.\n> \n> I personally am not curious about real-world problem in this case,\n> but I won't stop you asking ;-)\n> \n> I view this more about code simplification.  We no longer need a\n> custom nfvasprintf() helper nobody else cares about, leaving the\n> resulting code easier to read.\n> \n> Will queue but will wait for a day or two to see if René wants to\n> add clarification to the proposed commit log message before merging\n> it to 'next'.\n\nYeah, as the suggested-by person, I can affirm that this is not\nsomething I saw in real life. I agree the primary motivation is the code\nsimplification, and dropping a pointless limit is the bonus. I'd\nprobably have written it in that style, but I'm OK either way.\n\n-Peff\n"},{"id":"492961","messageId":"20240415185530.GB1709228@coredump.intra.peff.net","threadId":"61317","inReplyTo":"7026075c-db4e-4d43-bbd1-d2edb52da9b7@web.de","subject":"Re: [PATCH] imap-send: increase command size limit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-15T18:55:30Z","receivedAt":"2024-04-15T18:55:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 14, 2024 at 06:47:52PM +0200, René Scharfe wrote:\n\n> While 1KB is plenty for user names, passwords and mailbox names,\n> there's no point in limiting our commands like that.  Call xstrvfmt()\n> instead of open-coding it and use strbuf to format the command to\n> send, as we need its length.  Fail hard if it exceeds INT_MAX, because\n> socket_write() can't take more than that.\n\nHmm. I applaud your attention to detail, but this INT_MAX thing is ugly. ;)\nShouldn't socket_write() just use size_t / ssize_t?\n\nIn particular, this made me wonder what we would do for larger items.\nLike, say, the actual message to be uploaded. And indeed, we use a\nstrbuf to read in the messages and pass the whole buffer for each to\nsocket_write(). So we'd possibly quietly truncate such a message.\n\nFixing it is a little more complicated than switching to size_t, because\nthe underlying SSL_write() uses an int. So we'd probably need some\nlooping, similar to xwrite().\n\nIn practice I doubt this is ever an issue. 2GB emails are not likely to\nbe usable in general. And I kind of doubt that this is a reasonable\nvector for attacks, since the inputs to imap-send would generally come\nfrom the user themselves (and certainly truncating the attack message is\nprobably not that interesting, though I imagine one could convince\nwrite_in_full() to do an out-of-bounds read as a size_t becomes a\nnegative int which becomes a large size_t again).\n\nSo I am happy enough with this (especially given my general opinions of\nimap-send in the first place).\n\n-Peff\n"},{"id":"492995","messageId":"5d35dd4a-3569-4292-a736-45cfd0442f24@web.de","threadId":"61317","inReplyTo":"20240415184552.GA1709228@coredump.intra.peff.net","subject":"Re: [PATCH] imap-send: increase command size limit","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-16T15:31:54Z","receivedAt":"2024-04-16T15:37:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.04.24 um 20:45 schrieb Jeff King:\n> On Mon, Apr 15, 2024 at 11:38:28AM -0700, Junio C Hamano wrote:\n>\n>>> I'm curious, is there a particular problem that you (or someone else)\n>>> ran into that caused you to make this change?  I agree it seems prudent\n>>> in general, but if there's a particular real-world broken case that this\n>>> hits (e.g., mailbox names in a given language), I think the commit\n>>> message would be a great place to mention this real-world impact, which\n>>> would lend support to your argument that this is a valuable change to\n>>> make.\n>>\n>> I personally am not curious about real-world problem in this case,\n>> but I won't stop you asking ;-)\n>>\n>> I view this more about code simplification.  We no longer need a\n>> custom nfvasprintf() helper nobody else cares about, leaving the\n>> resulting code easier to read.\n>>\n>> Will queue but will wait for a day or two to see if René wants to\n>> add clarification to the proposed commit log message before merging\n>> it to 'next'.\n>\n> Yeah, as the suggested-by person, I can affirm that this is not\n> something I saw in real life. I agree the primary motivation is the code\n> simplification, and dropping a pointless limit is the bonus. I'd\n> probably have written it in that style, but I'm OK either way.\n\nRight, it's just intended as a code cleanup.  I mention the limits only\nbecause lifting them is a change of behavior.  Though you can now use\npasswords and folder names longer than 1000 characters, I don't expect\nthat to catch on.\n\nRené\n"},{"id":"492998","messageId":"85047e64-044f-4bf8-8de9-33b082255f3e@web.de","threadId":"61317","inReplyTo":"20240415185530.GB1709228@coredump.intra.peff.net","subject":"Re: [PATCH] imap-send: increase command size limit","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-16T16:08:05Z","receivedAt":"2024-04-16T16:08:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.04.24 um 20:55 schrieb Jeff King:\n> On Sun, Apr 14, 2024 at 06:47:52PM +0200, René Scharfe wrote:\n>\n>> While 1KB is plenty for user names, passwords and mailbox names,\n>> there's no point in limiting our commands like that.  Call xstrvfmt()\n>> instead of open-coding it and use strbuf to format the command to\n>> send, as we need its length.  Fail hard if it exceeds INT_MAX, because\n>> socket_write() can't take more than that.\n>\n> Hmm. I applaud your attention to detail, but this INT_MAX thing is ugly. ;)\n\nIt is.  I thought about using cast_size_t_to_int() instead, but decided\nto preserve the original error message.\n\n> Shouldn't socket_write() just use size_t / ssize_t?\n\nProbably size_t.\n\n> In particular, this made me wonder what we would do for larger items.\n> Like, say, the actual message to be uploaded. And indeed, we use a\n> strbuf to read in the messages and pass the whole buffer for each to\n> socket_write(). So we'd possibly quietly truncate such a message.\n\nHmm, perhaps we should at least sprinkle in some more overflow checks?\n\n> Fixing it is a little more complicated than switching to size_t, because\n> the underlying SSL_write() uses an int. So we'd probably need some\n> looping, similar to xwrite().\n\nOr SSL_write_ex(), which takes and returns size_t.  It was added in\nOpenSSL 1.1.1, which reached its end of life half a year ago.\n\nhttps://www.openssl.org/docs/man1.1.1/man3/SSL_write.html\nhttps://www.openssl.org/blog/blog/2023/03/28/1.1.1-EOL/\n\n> In practice I doubt this is ever an issue. 2GB emails are not likely to\n> be usable in general.\n\nTough.  Who likes to get multi-GB patches in their inbox?  Heard of\npeople exchanging CD images by email decades ago, though, so I\nwouldn't rule this out totally.  Perhaps that's the last puzzle piece\nto convert game studios to perform email reviews of asset-heavy\nbinary diffs? ;-)\n\n> And I kind of doubt that this is a reasonable\n> vector for attacks, since the inputs to imap-send would generally come\n> from the user themselves (and certainly truncating the attack message is\n> probably not that interesting, though I imagine one could convince\n> write_in_full() to do an out-of-bounds read as a size_t becomes a\n> negative int which becomes a large size_t again).\n\nRight.  If you can get a user to upload more than 2GB of hostile data to\ntheir mail server then you don't need to bother crafting an integer\noverflow exploit.\n\nStill, checking for overflow instead of truncating silently seems like a\ngood idea even for half-dead low-impact code.  #leftoverbits\n\nRené\n"},{"id":"493043","messageId":"20240417002454.GA2305596@coredump.intra.peff.net","threadId":"61317","inReplyTo":"85047e64-044f-4bf8-8de9-33b082255f3e@web.de","subject":"Re: [PATCH] imap-send: increase command size limit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-17T00:24:54Z","receivedAt":"2024-04-17T00:25:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 16, 2024 at 06:08:05PM +0200, René Scharfe wrote:\n\n> > Shouldn't socket_write() just use size_t / ssize_t?\n> \n> Probably size_t.\n\nYes, but you need ssize_t to handle the negative return values.\n\n> > In particular, this made me wonder what we would do for larger items.\n> > Like, say, the actual message to be uploaded. And indeed, we use a\n> > strbuf to read in the messages and pass the whole buffer for each to\n> > socket_write(). So we'd possibly quietly truncate such a message.\n> \n> Hmm, perhaps we should at least sprinkle in some more overflow checks?\n\nPerhaps, but...\n\n> > Fixing it is a little more complicated than switching to size_t, because\n> > the underlying SSL_write() uses an int. So we'd probably need some\n> > looping, similar to xwrite().\n> \n> Or SSL_write_ex(), which takes and returns size_t.  It was added in\n> OpenSSL 1.1.1, which reached its end of life half a year ago.\n> \n> https://www.openssl.org/docs/man1.1.1/man3/SSL_write.html\n> https://www.openssl.org/blog/blog/2023/03/28/1.1.1-EOL/\n\nYou'd think that when I ran \"man SSL_write\" while writing the other\nemail I would have noticed that SSL_write_ex() is mentioned right there\nin the synopsis. But somehow I didn't.\n\nI don't think we document a required version for ssl. That version is\n\"only\" 5.5 years old, but I think it would be OK here (especially given\nthat imap-send is an optional component with a build-time knob).\n\nIn which case I think fixing socket_write() would fix this problem, and\nthen building your other patch on top, it doesn't need to worry about\nINT_MAX at all.\n\nLooking at the conversion, I think there's a slight gotcha with\nretaining the \"0\" return value from SSL_write_ex() to pass to\nsocket_perror() in the error path. Which makes me wonder about that\nerror path at all; it closes descriptors but doesn't handle SSL at all.\nShould it be using socket_shutdown()? And should that function set\nsock->ssl to NULL and the descriptors to -1? The rabbit hole of\nimap-send is infinite.\n\n> > In practice I doubt this is ever an issue. 2GB emails are not likely to\n> > be usable in general.\n> \n> Tough.  Who likes to get multi-GB patches in their inbox?  Heard of\n> people exchanging CD images by email decades ago, though, so I\n> wouldn't rule this out totally.  Perhaps that's the last puzzle piece\n> to convert game studios to perform email reviews of asset-heavy\n> binary diffs? ;-)\n\nOK, I laughed out loud at this. Perhaps a sign of too much Git.\n\n-Peff\n"}]}