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é
