Potential remote crashes
Matthias Andree
matthias.andree at gmx.de
Sat May 10 01:27:20 CEST 2003
Timo Sirainen <tss at iki.fi> writes:
> Did a small code audit. Looks pretty good in general, but found a few
> things:
Thanks a bunch!
> static int yy_use_redo_text(buff_t *buff)
> {
> size_t used = buff->t.leng;
> size_t size = buff->size;
> size_t avail = size - used;
> byte *buf = buff->t.text+used;
> size_t count = min(yysave->size, avail-2);
>
> memcpy(buf, yysave->t.text, count );
>
> If avail is 0 or 1, count is set to (size_t)-1 or -2 and memcpy()
> crashes. I'm not sure if that can happen though, it's not so obvious
> when and how this function gets called..
I have added a band-aid workaround: a check to abort() bogofilter if
avail < 2. That's ugly, but doesn't trigger with the tests we have, and
we have a trusted minimal "count" after that. I'd rather have
self-contained functions than rely on the callers to do things
right. I'd also rather kill bogofilter than continue with a possibly
corrupted heap. We can still fix the abort() later when someone has a
message to trigger the abort(). Let's not take a chance here, there's
enough unsafe software in the world.
> size_t qp_decode(word_t *word)
> .
> while (s < e)
> {
> byte ch = *s++;
> if (ch == '=') {
> ch = *s++;
> if (ch != '\n') {
> byte cx = *s++;
> .
> *d++ = '=';
> *d++ = ch;
> *d++ = cx;
> continue;
> .
> *d = '\0';
>
> If ch == '=' is missing checks if s still is less than e, but it still
> updates d. Looks like that would allow writing \0 to 1 or 2 bytes after
> malloc()ed data.
I have cleaned up and fixed that function as well. I haven't researched
if this is exploitable, but I wouldn't rule out a vulnerability
off-hand. Looks as though it's time for another release and another
security announcement, just in case.
Timo, could you update from CVS and have another look at qp_decode() and
yy_use_redo_text()?
David, what do you think?
--
Matthias Andree
More information about the bogofilter-dev
mailing list