Benjamin Drieu <benj(_at_)debian(_dot_)org>:
here is another report from a Debian user (bug #176938). It is about
a buffer overflow. It includes a possible fix.
[ Note: thinking of security issues, I tried to contact Eric directly
about this bug, but got no answer. Looking more deeply at the
problem, security risks are not that high not to forward this to
friends. ]
I've been heads-down working on a book.
Basically the problem is stuff_warning (sink.c:1478):
char buf[POPBUFSIZE];
...
vsnprintf(buf, sizeof(buf), fmt, ap);
...
snprintf(buf+strlen(buf), sizeof(buf)-strlen(buf), "\r\n");
...
stuffline(ctl, buf);
Stuffline expects buf to be terminated by \n\0, but obviously buf is not
guaranteed to have even a terminating \0, due to idiosyncracies of snprintf.
Thus random memory contents are displayed (e.g. I see a command used to
initiate an ssh tunnel for a different account, some IMAP messages; haven't
noticed any passwords yet)
The actual call which passes a string longer than 512 characters is in
do_session (driver.c, line 1124).
Note that the texts are i18nized, thus you cannot rely on them being shorter
than a given number of characters.
Ah. That's the problem. I know the size of *my* messages -- I don't know
what they'll look like when gettext() gets done with them. I've been tempted
to rip out the gettext support for a long time; this might push me over
the edge.
Suggested fix: increase stuff_warning buffer size (to avoid truncation of
fetchmail error messages), fix all calls to snprintf/vsnprintf to manually
ensure strings are terminated properly.
Most of these calls are guarded in a different way. If you look, you'll
notice that my static-array declarations have a tendency to look like
this:
char foo[FOOSIZE+1];
The idea here is that the rest of the code treats FOOSIZE as the
buffer size and never steps on the initial NUL value of the last
character. It's a compile-time way to accomplish the same thing.
Doesn't work reliably for auto buffers, though.
Maybe grep for occurrences of
strncpy/strncat as well.
I'm aware of this problem, too. If you look, you'll find that those calls are
usually immediately followed by code that zeros the last byte of the buffer.
However, I'm glad you reminded me of this issue; I grepped and found three
unguarded instances, in code I didn't write :-).
I admit that I had a higher opinion on fetchmail before I glanced at the
#ifdef-packed source code. I cannot say that I like bits like
#ifdef HAVE_SNPRINTF
snprintf(buf+strlen(buf), sizeof(buf)-strlen(buf), "\r\n");
#else
strcat(buf, "\r\n");
#endif /* HAVE_SNPRINTF */
That code has, however, passed two separate security audits. That makes
me reluctant to mess with it.
If the platform is broken and doesn't have snprintf (BTW why not use strncat
here?), why isn't there a probably slower portable C implementation included
with fetchmail, instead of leaving a buffer-overflow waiting to happen?
Because nobody has reported this as a bug or supplied a clean snprintf
implementation, before this. Bear in mind that this code dates from 1996.
Conditions were a little different then.
--
<a href="http://www.catb.org/~esr/";>Eric S. Raymond</a>