| Message ID | 7026075c-db4e-4d43-bbd1-d2edb52da9b7@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | imap-send: increase command size limit | expand |
On 2024-04-14 at 16:47:52, René Scharfe wrote: > nfvasprintf() has a 8KB limit, but it's not relevant, as its result is > combined with other strings and added to a 1KB buffer by its caller. > That 1KB limit is not mentioned in RFC 9051, which specifies IMAP. > > 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. > > Suggested-by: Jeff King <peff@peff.net> > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > This time I compiled with NO_CURL=1 and NO_APPLE_COMMON_CRYPTO=1 and > verified with a silly printf that the changed code was actually used > and wrote the present message to an IMAP folder whose name is 1006 > characters log, which required a 1026 bytes long APPEND command. Yay, > freedom! I'm curious, is there a particular problem that you (or someone else) ran into that caused you to make this change? I agree it seems prudent in general, but if there's a particular real-world broken case that this hits (e.g., mailbox names in a given language), I think the commit message would be a great place to mention this real-world impact, which would lend support to your argument that this is a valuable change to make.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: >> ... >> Suggested-by: Jeff King <peff@peff.net> >> Signed-off-by: René Scharfe <l.s.r@web.de> >> --- >> This time I compiled with NO_CURL=1 and NO_APPLE_COMMON_CRYPTO=1 and >> verified with a silly printf that the changed code was actually used >> and wrote the present message to an IMAP folder whose name is 1006 >> characters log, which required a 1026 bytes long APPEND command. Yay, >> freedom! > > I'm curious, is there a particular problem that you (or someone else) > ran into that caused you to make this change? I agree it seems prudent > in general, but if there's a particular real-world broken case that this > hits (e.g., mailbox names in a given language), I think the commit > message would be a great place to mention this real-world impact, which > would lend support to your argument that this is a valuable change to > make. I personally am not curious about real-world problem in this case, but I won't stop you asking ;-) I view this more about code simplification. We no longer need a custom nfvasprintf() helper nobody else cares about, leaving the resulting code easier to read. Will queue but will wait for a day or two to see if René wants to add clarification to the proposed commit log message before merging it to 'next'. Thanks, both.
On Mon, Apr 15, 2024 at 11:38:28AM -0700, Junio C Hamano wrote: > > I'm curious, is there a particular problem that you (or someone else) > > ran into that caused you to make this change? I agree it seems prudent > > in general, but if there's a particular real-world broken case that this > > hits (e.g., mailbox names in a given language), I think the commit > > message would be a great place to mention this real-world impact, which > > would lend support to your argument that this is a valuable change to > > make. > > I personally am not curious about real-world problem in this case, > but I won't stop you asking ;-) > > I view this more about code simplification. We no longer need a > custom nfvasprintf() helper nobody else cares about, leaving the > resulting code easier to read. > > Will queue but will wait for a day or two to see if René wants to > add clarification to the proposed commit log message before merging > it to 'next'. Yeah, as the suggested-by person, I can affirm that this is not something I saw in real life. I agree the primary motivation is the code simplification, and dropping a pointless limit is the bonus. I'd probably have written it in that style, but I'm OK either way. -Peff
On Sun, Apr 14, 2024 at 06:47:52PM +0200, René Scharfe wrote: > 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
Am 15.04.24 um 20:45 schrieb Jeff King: > On Mon, Apr 15, 2024 at 11:38:28AM -0700, Junio C Hamano wrote: > >>> I'm curious, is there a particular problem that you (or someone else) >>> ran into that caused you to make this change? I agree it seems prudent >>> in general, but if there's a particular real-world broken case that this >>> hits (e.g., mailbox names in a given language), I think the commit >>> message would be a great place to mention this real-world impact, which >>> would lend support to your argument that this is a valuable change to >>> make. >> >> I personally am not curious about real-world problem in this case, >> but I won't stop you asking ;-) >> >> I view this more about code simplification. We no longer need a >> custom nfvasprintf() helper nobody else cares about, leaving the >> resulting code easier to read. >> >> Will queue but will wait for a day or two to see if René wants to >> add clarification to the proposed commit log message before merging >> it to 'next'. > > Yeah, as the suggested-by person, I can affirm that this is not > something I saw in real life. I agree the primary motivation is the code > simplification, and dropping a pointless limit is the bonus. I'd > probably have written it in that style, but I'm OK either way. Right, it's just intended as a code cleanup. I mention the limits only because lifting them is a change of behavior. Though you can now use passwords and folder names longer than 1000 characters, I don't expect that to catch on. René
Am 15.04.24 um 20:55 schrieb Jeff King: > On Sun, Apr 14, 2024 at 06:47:52PM +0200, René Scharfe wrote: > >> 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. ;) It is. I thought about using cast_size_t_to_int() instead, but decided to preserve the original error message. > Shouldn't socket_write() just use size_t / ssize_t? Probably size_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. Hmm, perhaps we should at least sprinkle in some more overflow checks? > 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(). Or SSL_write_ex(), which takes and returns size_t. It was added in OpenSSL 1.1.1, which reached its end of life half a year ago. https://www.openssl.org/docs/man1.1.1/man3/SSL_write.html https://www.openssl.org/blog/blog/2023/03/28/1.1.1-EOL/ > In practice I doubt this is ever an issue. 2GB emails are not likely to > be usable in general. Tough. Who likes to get multi-GB patches in their inbox? Heard of people exchanging CD images by email decades ago, though, so I wouldn't rule this out totally. Perhaps that's the last puzzle piece to convert game studios to perform email reviews of asset-heavy binary diffs? ;-) > 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). Right. If you can get a user to upload more than 2GB of hostile data to their mail server then you don't need to bother crafting an integer overflow exploit. Still, checking for overflow instead of truncating silently seems like a good idea even for half-dead low-impact code. #leftoverbits René
On Tue, Apr 16, 2024 at 06:08:05PM +0200, René Scharfe wrote: > > Shouldn't socket_write() just use size_t / ssize_t? > > Probably size_t. Yes, but you need ssize_t to handle the negative return values. > > 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. > > Hmm, perhaps we should at least sprinkle in some more overflow checks? Perhaps, but... > > 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(). > > Or SSL_write_ex(), which takes and returns size_t. It was added in > OpenSSL 1.1.1, which reached its end of life half a year ago. > > https://www.openssl.org/docs/man1.1.1/man3/SSL_write.html > https://www.openssl.org/blog/blog/2023/03/28/1.1.1-EOL/ You'd think that when I ran "man SSL_write" while writing the other email I would have noticed that SSL_write_ex() is mentioned right there in the synopsis. But somehow I didn't. I don't think we document a required version for ssl. That version is "only" 5.5 years old, but I think it would be OK here (especially given that imap-send is an optional component with a build-time knob). In which case I think fixing socket_write() would fix this problem, and then building your other patch on top, it doesn't need to worry about INT_MAX at all. Looking at the conversion, I think there's a slight gotcha with retaining the "0" return value from SSL_write_ex() to pass to socket_perror() in the error path. Which makes me wonder about that error path at all; it closes descriptors but doesn't handle SSL at all. Should it be using socket_shutdown()? And should that function set sock->ssl to NULL and the descriptors to -1? The rabbit hole of imap-send is infinite. > > In practice I doubt this is ever an issue. 2GB emails are not likely to > > be usable in general. > > Tough. Who likes to get multi-GB patches in their inbox? Heard of > people exchanging CD images by email decades ago, though, so I > wouldn't rule this out totally. Perhaps that's the last puzzle piece > to convert game studios to perform email reviews of asset-heavy > binary diffs? ;-) OK, I laughed out loud at this. Perhaps a sign of too much Git. -Peff
diff --git a/imap-send.c b/imap-send.c index 4caa8668e6..0afd088d8a 100644 --- a/imap-send.c +++ b/imap-send.c @@ -68,20 +68,6 @@ static void imap_warn(const char *, ...); static char *next_arg(char **); -static int nfvasprintf(char **strp, const char *fmt, va_list ap) -{ - int len; - char tmp[8192]; - - len = vsnprintf(tmp, sizeof(tmp), fmt, ap); - if (len < 0) - die("Fatal: Out of memory"); - if (len >= sizeof(tmp)) - die("imap command overflow!"); - *strp = xmemdupz(tmp, len); - return len; -} - struct imap_server_conf { const char *name; const char *tunnel; @@ -503,11 +489,11 @@ static struct imap_cmd *issue_imap_cmd(struct imap_store *ctx, { struct imap *imap = ctx->imap; struct imap_cmd *cmd; - int n, bufl; - char buf[1024]; + int n; + struct strbuf buf = STRBUF_INIT; cmd = xmalloc(sizeof(struct imap_cmd)); - nfvasprintf(&cmd->cmd, fmt, ap); + cmd->cmd = xstrvfmt(fmt, ap); cmd->tag = ++imap->nexttag; if (cb) @@ -519,27 +505,30 @@ static struct imap_cmd *issue_imap_cmd(struct imap_store *ctx, get_cmd_result(ctx, NULL); if (!cmd->cb.data) - bufl = xsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd); + strbuf_addf(&buf, "%d %s\r\n", cmd->tag, cmd->cmd); else - bufl = xsnprintf(buf, sizeof(buf), "%d %s{%d%s}\r\n", - cmd->tag, cmd->cmd, cmd->cb.dlen, - CAP(LITERALPLUS) ? "+" : ""); + strbuf_addf(&buf, "%d %s{%d%s}\r\n", cmd->tag, cmd->cmd, + cmd->cb.dlen, CAP(LITERALPLUS) ? "+" : ""); + if (buf.len > INT_MAX) + die("imap command overflow!"); if (0 < verbosity) { if (imap->num_in_progress) printf("(%d in progress) ", imap->num_in_progress); if (!starts_with(cmd->cmd, "LOGIN")) - printf(">>> %s", buf); + printf(">>> %s", buf.buf); else printf(">>> %d LOGIN <user> <pass>\n", cmd->tag); } - if (socket_write(&imap->buf.sock, buf, bufl) != bufl) { + if (socket_write(&imap->buf.sock, buf.buf, buf.len) != buf.len) { free(cmd->cmd); free(cmd); if (cb) free(cb->data); + strbuf_release(&buf); return NULL; } + strbuf_release(&buf); if (cmd->cb.data) { if (CAP(LITERALPLUS)) { n = socket_write(&imap->buf.sock, cmd->cb.data, cmd->cb.dlen);
nfvasprintf() has a 8KB limit, but it's not relevant, as its result is combined with other strings and added to a 1KB buffer by its caller. That 1KB limit is not mentioned in RFC 9051, which specifies IMAP. 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. Suggested-by: Jeff King <peff@peff.net> Signed-off-by: René Scharfe <l.s.r@web.de> --- This time I compiled with NO_CURL=1 and NO_APPLE_COMMON_CRYPTO=1 and verified with a silly printf that the changed code was actually used and wrote the present message to an IMAP folder whose name is 1006 characters log, which required a 1026 bytes long APPEND command. Yay, freedom! Ran into a build issue, will send a separate patch for that. imap-send.c | 35 ++++++++++++----------------------- 1 file changed, 12 insertions(+), 23 deletions(-) -- 2.44.0