| Message ID | f9ad9f41-5b9b-474e-9818-f91fc937daae@web.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | imap-send: use xsnprintf to format command | expand |
On Tue, Apr 02, 2024 at 04:51:05PM +0200, René Scharfe wrote: > nfsnprintf() wraps vsnprintf(3) and reports attempts to use too small a > buffer using BUG(), just like xsnprintf(). > > It has an extra check that makes sure the buffer size (converted to int) > is positive. vsnprintf(3) is supposed to handle a buffer size of zero > or bigger than INT_MAX just fine, so this extra comparison doesn't make > us any safer. If a platform has a broken implementation, we'd need to > work around it in our compat code. > > Call xsnprintf() instead to reduce code duplication and make the caller > slightly more readable by using this more common helper. I think this is an improvement, and since the original also called BUG() we are not making anything worse. But I think the original was actually in error to do so, since it depends on user data. E.g., setting imap.user to the output of $(perl -e 'print "a" x 1024"') results in: BUG: imap-send.c:511: buffer too small. Please report a bug. error: git-imap-send died of signal 6 Now obviously that's a dumb username, but this is the buffer used for all IMAP commands. So conceivably long folder names, etc, do the same thing. It's probably not a big deal in practice, but conceptually this probably ought to be xstrfmt() and not xsnprintf(). Likewise imap-send's nfvasprintf() is basically xstrfmt(), except it takes a va_list. So it would have to be replaced by strbuf_vaddf(). I wouldn't be surprised if there are other opportunities for string cleanup, but I generally hoped that if we waited long enough imap-send would just go away. ;) Either because we could get rid of the tool entirely (though from a recent-ish search, there did not seem to be a lot of good other tools) or because we'd just drop the old code and rely on curl to do the heavy lifting. All that said, I think your patch makes the world slightly better, so I'm not opposed in the meantime. -Peff
Am 03.04.24 um 02:47 schrieb Jeff King: > On Tue, Apr 02, 2024 at 04:51:05PM +0200, René Scharfe wrote: > >> nfsnprintf() wraps vsnprintf(3) and reports attempts to use too small a >> buffer using BUG(), just like xsnprintf(). >> >> It has an extra check that makes sure the buffer size (converted to int) >> is positive. vsnprintf(3) is supposed to handle a buffer size of zero >> or bigger than INT_MAX just fine, so this extra comparison doesn't make >> us any safer. If a platform has a broken implementation, we'd need to >> work around it in our compat code. >> >> Call xsnprintf() instead to reduce code duplication and make the caller >> slightly more readable by using this more common helper. > > I think this is an improvement, and since the original also called BUG() > we are not making anything worse. But I think the original was actually > in error to do so, since it depends on user data. E.g., setting > imap.user to the output of $(perl -e 'print "a" x 1024"') results in: > > BUG: imap-send.c:511: buffer too small. Please report a bug. > error: git-imap-send died of signal 6 > > Now obviously that's a dumb username, but this is the buffer used for > all IMAP commands. So conceivably long folder names, etc, do the same > thing. It's probably not a big deal in practice, but conceptually this > probably ought to be xstrfmt() and not xsnprintf(). From reading the code I assumed the static buffer is there to stay within some IMAP limit. RFC 9051 mentions the distinction between synchronizing and non-synchronizing literals. The latter have a maximum length of 4096 bytes. But those are transferred after the command, so have no relevance for the command buffer size. I see no other limits, and I don't see us respecting that non-synchronizing literals limit, either. I guess that means messages longer than 4096 bytes could be rejected by a conforming IMAP server? Hmm. > Likewise imap-send's nfvasprintf() is basically xstrfmt(), except it > takes a va_list. So it would have to be replaced by strbuf_vaddf(). Looking closer I notice that the result of the single nfvasprintf() call is fed into the 1024 bytes buffer. So we could replace it with strbuf_vaddf() or xstrvfmt() and still stay within that strange limit, as it's enforced later. Its own 8192 buffer shields us from huge allocations e.g. due to long usernames or paths, but we probably don't need this protection as such and "attack" would originate and be felt only locally. > I wouldn't be surprised if there are other opportunities for string > cleanup, but I generally hoped that if we waited long enough imap-send > would just go away. ;) Either because we could get rid of the tool > entirely (though from a recent-ish search, there did not seem to be a > lot of good other tools) or because we'd just drop the old code and rely > on curl to do the heavy lifting. Oh, my build uses curl, so my earlier test run was even worth less than I thought. René
On Wed, Apr 03, 2024 at 11:25:42AM +0200, René Scharfe wrote: > From reading the code I assumed the static buffer is there to stay > within some IMAP limit. RFC 9051 mentions the distinction between > synchronizing and non-synchronizing literals. The latter have a maximum > length of 4096 bytes. > > But those are transferred after the command, so have no relevance for > the command buffer size. I see no other limits, and I don't see us > respecting that non-synchronizing literals limit, either. I guess that > means messages longer than 4096 bytes could be rejected by a conforming > IMAP server? Hmm. It can also just be a quoted string, which likewise has no limit defined in that section. That's what we send for LOGIN (you have to go back to the imap_exec() command which uses a format string). It also looks like that would barf completely on a username or password that contains a double-quote. Yup: $ git format-patch -1 --stdout | git -c imap.user='my"user"' -c imap.pass=foo imap-send --no-curl [...] IMAP command 'LOGIN <user> <pass>' returned response (BAD) - Invalid characters in atom IMAP error: LOGIN failed > > Likewise imap-send's nfvasprintf() is basically xstrfmt(), except it > > takes a va_list. So it would have to be replaced by strbuf_vaddf(). > > Looking closer I notice that the result of the single nfvasprintf() call > is fed into the 1024 bytes buffer. So we could replace it with > strbuf_vaddf() or xstrvfmt() and still stay within that strange limit, > as it's enforced later. Oh, I forgot we had xstrvfmt(). That would obviously be the right match. > Its own 8192 buffer shields us from huge allocations e.g. due to long > usernames or paths, but we probably don't need this protection as such > and "attack" would originate and be felt only locally. Yeah, I think all of the data here is user controlled. Even if you didn't trust the patch itself, this is all username, mailbox name, etc. > > I wouldn't be surprised if there are other opportunities for string > > cleanup, but I generally hoped that if we waited long enough imap-send > > would just go away. ;) Either because we could get rid of the tool > > entirely (though from a recent-ish search, there did not seem to be a > > lot of good other tools) or because we'd just drop the old code and rely > > on curl to do the heavy lifting. > > Oh, my build uses curl, so my earlier test run was even worth less than > I thought. Heh. -Peff
diff --git a/imap-send.c b/imap-send.c index f2e1947e63..4caa8668e6 100644 --- a/imap-send.c +++ b/imap-send.c @@ -68,9 +68,6 @@ static void imap_warn(const char *, ...); static char *next_arg(char **); -__attribute__((format (printf, 3, 4))) -static int nfsnprintf(char *buf, int blen, const char *fmt, ...); - static int nfvasprintf(char **strp, const char *fmt, va_list ap) { int len; @@ -500,19 +497,6 @@ static char *next_arg(char **s) return ret; } -__attribute__((format (printf, 3, 4))) -static int nfsnprintf(char *buf, int blen, const char *fmt, ...) -{ - int ret; - va_list va; - - va_start(va, fmt); - if (blen <= 0 || (unsigned)(ret = vsnprintf(buf, blen, fmt, va)) >= (unsigned)blen) - BUG("buffer too small. Please report a bug."); - va_end(va); - return ret; -} - static struct imap_cmd *issue_imap_cmd(struct imap_store *ctx, struct imap_cmd_cb *cb, const char *fmt, va_list ap) @@ -535,11 +519,11 @@ static struct imap_cmd *issue_imap_cmd(struct imap_store *ctx, get_cmd_result(ctx, NULL); if (!cmd->cb.data) - bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd); + bufl = xsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd); else - bufl = nfsnprintf(buf, sizeof(buf), "%d %s{%d%s}\r\n", - cmd->tag, cmd->cmd, cmd->cb.dlen, - CAP(LITERALPLUS) ? "+" : ""); + bufl = xsnprintf(buf, sizeof(buf), "%d %s{%d%s}\r\n", + cmd->tag, cmd->cmd, cmd->cb.dlen, + CAP(LITERALPLUS) ? "+" : ""); if (0 < verbosity) { if (imap->num_in_progress)
nfsnprintf() wraps vsnprintf(3) and reports attempts to use too small a buffer using BUG(), just like xsnprintf(). It has an extra check that makes sure the buffer size (converted to int) is positive. vsnprintf(3) is supposed to handle a buffer size of zero or bigger than INT_MAX just fine, so this extra comparison doesn't make us any safer. If a platform has a broken implementation, we'd need to work around it in our compat code. Call xsnprintf() instead to reduce code duplication and make the caller slightly more readable by using this more common helper. Signed-off-by: René Scharfe <l.s.r@web.de> --- Uploaded this message using the patched command, but that's all the testing I did. imap-send.c | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) -- 2.44.0