git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "René Scharfe" <l.s.r@web.de>
To: Jeff King <peff@peff.net>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH] imap-send: increase command size limit
Date: Tue, 16 Apr 2024 18:08:05 +0200	[thread overview]
Message-ID: <85047e64-044f-4bf8-8de9-33b082255f3e@web.de> (raw)
In-Reply-To: <20240415185530.GB1709228@coredump.intra.peff.net>

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é


  reply	other threads:[~2024-04-16 16:08 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-14 16:47 [PATCH] imap-send: increase command size limit René Scharfe
2024-04-14 17:44 ` brian m. carlson
2024-04-15 18:38   ` Junio C Hamano
2024-04-15 18:45     ` Jeff King
2024-04-16 15:31       ` René Scharfe
2024-04-15 18:55 ` Jeff King
2024-04-16 16:08   ` René Scharfe [this message]
2024-04-17  0:24     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=85047e64-044f-4bf8-8de9-33b082255f3e@web.de \
    --to=l.s.r@web.de \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).