Re: [PATCH] imap-send: increase command size limit

From: Jeff King <peff@peff.net>
Date: Apr 15, 2024, 18:55 UTC
Message-ID: <20240415185530.GB1709228@coredump.intra.peff.net>
In-Reply-To: <7026075c-db4e-4d43-bbd1-d2edb52da9b7@web.de>

On Sun, Apr 14, 2024 at 06:47:52PM +0200, René Scharfe wrote:

Show 5 quoted lines

> While 1KB is plenty for user names, passwords and mailbox names,
> there's no point in limiting our commands like that.  Call xstrvfmt()
> instead of open-coding it and use strbuf to format the command to
> send, as we need its length.  Fail hard if it exceeds INT_MAX, because
> socket_write() can't take more than that.

Hmm. I applaud your attention to detail, but this INT_MAX thing is ugly. ;) Shouldn't socket_write() just use size_t / ssize_t?

In particular, this made me wonder what we would do for larger items. Like, say, the actual message to be uploaded. And indeed, we use a strbuf to read in the messages and pass the whole buffer for each to socket_write(). So we'd possibly quietly truncate such a message.

Fixing it is a little more complicated than switching to size_t, because the underlying SSL_write() uses an int. So we'd probably need some looping, similar to xwrite().

In practice I doubt this is ever an issue. 2GB emails are not likely to be usable in general. And I kind of doubt that this is a reasonable vector for attacks, since the inputs to imap-send would generally come from the user themselves (and certainly truncating the attack message is probably not that interesting, though I imagine one could convince write_in_full() to do an out-of-bounds read as a size_t becomes a negative int which becomes a large size_t again).

So I am happy enough with this (especially given my general opinions of imap-send in the first place).

-Peff