headers
Lars Hecking | 18 Aug 20:25
Picon

[Fwd: mutt buffer overflow]

----- Forwarded message from Peter Valchev <pvalchev <at> sightly.net> -----

Mailing-List: contact bugtraq-help <at> securityfocus.com; run by ezmlm
Precedence: bulk
Delivered-To: mailing list bugtraq <at> securityfocus.com
Delivered-To: moderator for bugtraq <at> securityfocus.com
Date: Thu, 18 Aug 2005 02:57:33 -0600
From: Peter Valchev <pvalchev <at> sightly.net>
To: bugtraq <at> securityfocus.com, full-disclosure <at> lists.grok.org.uk
Subject: mutt buffer overflow
Mail-Followup-To: bugtraq <at> securityfocus.com,
	full-disclosure <at> lists.grok.org.uk
User-Agent: Mutt/1.4.2i

Summary/Impact:
There is a buffer overflow in mutt found thanks to ProPolice, which may
allow an attacker to execute code by sending a maliciously crafted email.
All latest versions appear affected.  Mutt is an e-mail client
that sucks less according to the headline on http://www.mutt.org/

Details:
The problem is in the mutt attachment/encoding/decoding functions,
specifically handler.c:mutt_decode_xbit() and the buffer
bufi[BUFI_SIZE].  The variable 'l' is used as a counter to reference a
position in the buffer and under certain circumstances its value can be
manipulated and becomes much larger than the size of this buffer, thus
overwriting other memory with many possible consequences.  This counter
should never exceed the size and I believe the logic in the
convert_to_state() function is supposed to reset it to 0, however
there is a flaw - I have included a possible fix but I'm not sure
it's the 100% correct fix and there seem to be no developers
willing to fix this so far.  There are other functions affected in
the same way due to copy/paste, such as mutt_decode_uuencoded() that
this patch should also fix.

There is a sample mailbox at http://sightly.net/peter/tmp/mutt-bug which
observes the problem - the last message causes data to be written to
addresses bufi[~1300] and above, when the size is 1000 (BUFI_SIZE) -
this can easily be seen by monitoring the counter from gdb or adding
printf's.  Since this and other such experiments cause the propolice
canary to get damaged (being right next to the return address), it
seems very likely for this to be exploitable, except on system such
as OpenBSD that include ProPolice by default.

Vendor response:  A bug report was submitted a week ago on August 11,
bug report #2033 and there has been no response.  The bug seems to exist
in both the latest stable and snapshot releases.  In fact a little
searching around seems it has been previously reported, but ignored
as unimportant, like seen in the Feb 26 message "Occasionally fatal bug
in handler.c?", http://blog.gmane.org/gmane.mail.mutt.devel/day=20030226

Fix:
Here is a possible fix

--- handler.c.orig	Tue Mar 26 02:49:51 2002
+++ handler.c	Wed Aug 10 16:55:02 2005
@@ -95,7 +95,7 @@ static void convert_to_state(iconv_t cd,
     return;
   }

-  if (cd == (iconv_t)(-1))
+  if (cd == (iconv_t)(-1) || *l >= BUFI_SIZE)
   {
     state_prefix_put (bufi, *l, s);
     *l = 0;

--
Peter Valchev <pvalchev <at> sightly.net>

----- End forwarded message -----
Permalink | Reply |
headers
Thomas Roessler | 18 Aug 22:32

Re: [Fwd: mutt buffer overflow]

On 2005-08-18 19:25:54 +0100, Lars Hecking forwarded from bugtraq:


*



> overwriting other memory with many possible consequences.  This
> counter should never exceed the size and I believe the logic in
> the convert_to_state() function is supposed to reset it to 0,
> however there is a flaw - I have included a possible fix but I'm
> not sure it's the 100% correct fix and there seem to be no
> developers willing to fix this so far.  There are other functions
> affected in the same way due to copy/paste, such as
> mutt_decode_uuencoded() that this patch should also fix.


To begin with, neither of the proof of concept examples [1], [2]
posted to bugtraq leads to a buffer overflow here (latest CVS built
on Fedora Core 4).

1. ftp://ftp.00f.net/misc/mutt-crash-poc.mbox
2. http://sightly.net/peter/tmp/mutt-bug

So Frank's observation that this might be tied to a particular iconv
implementation seems to be valid.  Still, the question is if mutt is
making assumptions that it shouldn't make, or if that iconv
implementation is broken.

The assumption that is made in mutt_decode_xbit is that
mutt_convert_to_state actually decreases the l counter by at least
one -- if l is sufficiently large.

This maps to a similar assumption that mutt_iconv doesn't return
without at least consuming one byte.

mutt_convert_to_state invokes mutt_iconv with inrepls set to 0, and
outrepl being "?", which immediately leaves out a lot of the
complexity in that function.

Reading through it, the one way in which that function does not
consume any input is when the input buffer begins with an incomplete
multibyte sequence.  Our assumption now becomes that any string of
sufficient length either begins with a valid (hence consumed, at
least partially) or invalid (hence skipped) multibyte sequence, but
not with an incomplete one.

I'd be curious to hear what the attached program prints on a
platform where the iso-2022-jp example [1] leads to a crash.

Regards,
-- 
Thomas Roessler · Personal soap box at <http://log.does-not-exist.org/>.

#include <stdio.h>
#include <string.h>
#include <iconv.h>
#include <errno.h>

int main ()
{
  iconv_t cd = iconv_open ("ISO_8859-15", "ISO-2022-JP");

  char _ib[] = "\033AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
  char _ob[1000];

  size_t ibl = sizeof (_ib) - 1;
  size_t obl = sizeof (_ob);

  char *ib = _ib;
  char *ob = _ob;

  int rv = iconv (cd, &ib, &ibl, &ob, &obl);

  printf ("rv = %d, errno = %d (%s)\n",
	  rv, errno, errno == EILSEQ ? "EILSEQ" : errno == EINVAL ? "EINVAL" : "?");

  printf ("'%s'\n", _ob);

  return 0;
}
Permalink | Reply |
headers
TAKAHASHI Tamotsu | 20 Aug 15:19
Picon

Re: [Fwd: mutt buffer overflow]

* Thu Aug 18 2005 Thomas Roessler <roessler <at> does-not-exist.org>


*



> The assumption that is made in mutt_decode_xbit is that
> mutt_convert_to_state actually decreases the l counter by at least
> one -- if l is sufficiently large.
> 
> This maps to a similar assumption that mutt_iconv doesn't return
> without at least consuming one byte.
> 
> mutt_convert_to_state invokes mutt_iconv with inrepls set to 0, and
> outrepl being "?", which immediately leaves out a lot of the
> complexity in that function.
> 
> Reading through it, the one way in which that function does not
> consume any input is when the input buffer begins with an incomplete
> multibyte sequence.  Our assumption now becomes that any string of
> sufficient length either begins with a valid (hence consumed, at
> least partially) or invalid (hence skipped) multibyte sequence, but
> not with an incomplete one.


So what should we do?
Which should be fixed - mutt_iconv, mutt_convert_to_state, or
(mutt_decode_uuencoded and) mutt_decode_xbit?
We can't use inrepl/outrepl tricks in other functions than
mutt_iconv. But mutt_iconv would affect many other places of mutt.

If the input buffer is longer than MB_LEN_MAX, at least one byte
must be consumed anyway. And BUFI_SIZE == 1000 > MB_LEN_MAX.
So, mutt_convert_to_state in mutt_decode_xbit must consume at
least one byte.

Attachment: An untested patch for mutt_convert_to_state.
-- 
tamo

--- handler.c.BAK	Sat Aug 20 21:49:43 2005
+++ handler.c	Sat Aug 20 22:02:59 2005
@@ -80,6 +80,7 @@ void mutt_convert_to_state(iconv_t cd, c
   ICONV_CONST char *ib;
   char *ob;
   size_t ibl, obl;
+  int must_decrease = 0;

   if (!bufi)
   {
@@ -100,6 +101,9 @@ void mutt_convert_to_state(iconv_t cd, c
     return;
   }

+  if (*l >= MB_LEN_MAX)
+    must_decrease = 1;
+
   ib = bufi, ibl = *l;
   for (;;)
   {
@@ -108,6 +112,14 @@ void mutt_convert_to_state(iconv_t cd, c
     if (ob == bufo)
       break;
     state_prefix_put (bufo, ob - bufo, s);
+  }
+
+  if (must_decrease && ibl >= *l)
+  {
+    iconv (cd, 0, 0, 0, 0); /* ??? */
+    state_prefix_put ("?", 1, s);
+    ibl = *l - 1;
+    ib = bufi + 1;
   }
   memmove (bufi, ib, ibl);
   *l = ibl;
Permalink | Reply |
headers
Vincent Lefevre | 18 Aug 23:32

Re: [Fwd: mutt buffer overflow]

*



On 2005-08-18 22:32:27 +0200, Thomas Roessler wrote:
> So Frank's observation that this might be tied to a particular iconv
> implementation seems to be valid.  Still, the question is if mutt is
> making assumptions that it shouldn't make, or if that iconv
> implementation is broken.


Do you mean that Mutt is making assumptions that are not clearly
documented? I'd say that in doubt, Mutt shouldn't make any assumption,
in particular when there is a risk of buffer overflow.

--

-- 
Vincent Lefèvre <vincent <at> vinc17.org> - Web: <http://www.vinc17.org/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.org/blog/>
Work: CR INRIA - computer arithmetic / SPACES project at LORIA
Permalink | Reply |

Gmane