mimms: patch to fix many buffer overflows vulnerability

Debian Bug report logs - #374577
mimms: patch to fix many buffer overflows vulnerability

Toggle useless messages

Message #5 received at submit@bugs.debian.org (full text, mbox, reply):

From: Anon Sricharoenchai <anon_hui@yahoo.com>

To: Debian Bug Tracking System <submit@bugs.debian.org>, Ubuntu Bug Tracking System <ubuntu-users@lists.ubuntu.com>, tha_d@users.sourceforge.net

Cc: anon_hui@yahoo.com

Subject: mimms: patch to fix many buffer overflows vulnerability

Date: Mon, 19 Jun 2006 21:09:17 -0700 (PDT)

[Message part 1 (text/plain, inline)]

Package: mimms
Version: 0.0.9-1
Severity: grave
Justification: user security hole
Tags: security patch

According to the patch attached in this report, it has many possible buffer
overflows.
For example,
- memcpy(buf, data, length) without bounding the limit of "length",
  while "length" depend on the input data incoming from the internet.
- read(s, data, BUF_SIZE) in main(), where BUF_SIZE is much greater than
  sizeof(data) which is only 1024 chars allocated in main(), while
  BUF_SIZE is defined as 1024*128.


-- System Information:
Debian Release: testing/unstable
Architecture: i386 (i686)
Kernel: Linux 2.6.10-5-386
Locale: LANG=C, LC_CTYPE=thai

Versions of packages mimms depends on:
ii  libc6                   2.3.5-1ubuntu12  GNU C Library: Shared libraries an
ii  libgcc1                 1:3.4.2-2ubuntu1 GCC support library
ii  libpopt0                1.7-4            lib for parsing cmdline parameters
ii  libstdc++5              1:3.3.4-9ubuntu5 The GNU Standard C++ Library v3
ii  libuuid1                1.35-6ubuntu1    Universally unique id library

-- no debconf information


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

[mimms-0.0.9.buffer_overflow.diff (text/x-patch, inline)]

--- mimms-0.0.9.orig/mimms.cpp	2004-10-22 21:38:33.000000000 +0700
+++ mimms-0.0.9/mimms.cpp	2006-05-13 21:27:57.168095728 +0700
@@ -66,6 +66,9 @@
 int stream_ids[20];
 int output_fh;
 
+// There's currently no use of put_32() that num_bytes depend on input and
+// cause buffer overflow.
+// Use put_32() with care, it can cause buffer overflow.
 static void put_32 (command_t *cmd, uint32_t value) {
   for (int i=0; i<4; i++) {
     cmd->buf[cmd->num_bytes++] = (value >> (8*i)) & 0xff;
@@ -81,6 +84,8 @@
   return value;
 }
 
+#define min(a, b) (a) < (b) ? (a) : (b)
+#define max(a, b) (a) > (b) ? (a) : (b)
 static void send_command (int s, int command, uint32_t switches, 
 			  uint32_t extra, int length,
 			  char *data) {
@@ -107,8 +112,24 @@
   put_32 (&cmd, switches);
   put_32 (&cmd, extra);
 
-  memcpy (&cmd.buf[48], data, length);
-  if (length & 7)
+  // buffer overflow, when the length depend on input, such as, url length
+  //memcpy (&cmd.buf[48], data, length);
+  // Use min() to limit the upperbound length.
+  //memcpy (&cmd.buf[48], data, min(length, sizeof cmd.buf - 48));
+  // But the negative length can also cause buffer overflow, in case size_t is
+  // unsigned and length is casted to size_t.
+  // length can be negative in the line,
+  //   send_command (s, 0x33, num_stream_ids, 
+  //		0xFFFF | stream_ids[0] << 16, 
+  //		(num_stream_ids-1)*6+2 , data);
+  // in main() below, where num_stream_ids is zero.
+  // The solution is to cast length to size_t before min() compare.
+  memcpy (&cmd.buf[48], data,
+	  min((size_t)length, sizeof cmd.buf - 48*(sizeof cmd.buf[0])));
+  if (length & 7 &&
+    // to avoid buffer overflow
+    (48 + length)*(sizeof cmd.buf[0]) + 8 - (length & 7) <= sizeof cmd.buf
+  )
     memset(&cmd.buf[48 + length], 0, 8 - (length & 7));
 
   if (send (s, cmd.buf, len8*8+48, 0) != (len8*8+48)) {
@@ -152,18 +173,37 @@
 
 }
 
-static void string_utf16(char *dest, const char *src, int len) {
+// At this time, dest_size has unit in byte, which specify the length in bytes
+// of the buffer pointed to by dest.
+static void string_utf16(char *dest, size_t dest_size, const char *src, int len) {
   int i;
 
   memset (dest, 0, 1000);
 
-  for (i=0; i<len; i++) {
+  //for (i=0; i<len && /* avoid buffer overflow */ i*2+1 < dest_size; i++)
+  for (i=0;
+       i<len &&
+       /* avoid buffer overflow */
+       // For generic stuff.
+       // This can be buffer overflow at ref_note{string_utf16#1} if
+       // sizeof dest[0] > 1
+       //(i*2+1)*(sizeof dest[0]) < dest_size;
+       // So, advance 1 step
+       (i*2+2)*(sizeof dest[0]) <= dest_size;
+       i++) {
     dest[i*2] = src[i];
-    dest[i*2+1] = 0;
+    dest[i*2+1] = 0; // ref_note{string_utf16#1}
   }
+  //if (i*2+1 >= dest_size) return; // avoid buffer overflow
+  // For generic stuff.
+  // This can be buffer overflow at ref_note{string_utf16#2} if
+  // sizeof dest[0] > 1
+  //if ((i*2+1)*(sizeof dest[0]) >= dest_size) return; // avoid buffer overflow
+  // So, advance 1 step
+  if ((i*2+2)*(sizeof dest[0]) > dest_size) return; // avoid buffer overflow
 
   dest[i*2] = 0;
-  dest[i*2+1] = 0;
+  dest[i*2+1] = 0; // ref_note{string_utf16#2}
 }
 
 static void print_answer (char *data, int len) {
@@ -201,7 +241,10 @@
   while (command == 0x1b) {
     int len;
 
-    len = recv (s, data, BUF_SIZE, 0) ;
+    //len = recv (s, data, BUF_SIZE, 0) ;
+    // For generic stuff
+    //len = recv (s, data, BUF_SIZE*(sizeof data[0]), 0) ;
+    len = recv (s, data, sizeof data, 0) ;
     if (!len) {
       dprintf ("\nalert! eof\n");
       return;
@@ -214,14 +257,41 @@
   }
 }
 
-static int get_data (int s, char *buf, size_t count) {
+// This will continue to recv() data, even though buf_size is reached.
+// Both buf_size and count have unit in byte.
+// To allow buf_size <= 0 to not put any data in buf[]?
+static int get_data (int s, char *buf, size_t/*int*/ buf_size, size_t count) {
 
   ssize_t  len;
   size_t total = 0;
 
+  // buf_size = min(count, buf_size);
+  if (buf_size > count) buf_size = count;
+
   while (total < count) {
 
-    len = recv (s, &buf[total], count-total, 0);
+    char data[1024];
+    //len = recv (s, &buf[total], count-total, 0); // can cause buffer overflow
+    if (total*(sizeof buf[0]) < buf_size)
+      // buf_size is already set to min(count, buf_size), no need to use min()
+      // here.
+      //len = recv (s, &buf[total], min(count-total, buf_size-total), 0);
+      // For the sake of generic programming habit, use total*(sizeof buf[0])
+      // to cover the case that sizeof buf[0] != 1
+      // :)
+      // At this time, this generic consideration just focus on avoiding buffer
+      // overflow only.
+      // It doesn't care if the other program logic is generic or not.
+      // The other program logic, such as, &buf[total], it can rather be
+      // &buf[total/(sizeof buf[0])], to be more generic and logically correct.
+      // But whether it is logically correct or not, this doesn't affect the
+      // buffer overflow, so it is ignored.
+      // This is to make the program secure in generic way, not to make the
+      // program work correctly in generic way :)
+      len = recv (s, &buf[total], buf_size-total*(sizeof buf[0]), 0);
+    else
+      // cast to size_t before min() compare
+      len = recv (s, data, min(size_t(count-total), sizeof data), 0);
 
     if (len>0) {
        dprintf ("[%d/%d]", total, count);
@@ -243,7 +313,18 @@
 
 }
 
-static int get_header (int s, uint8_t *header) {
+// header_size should has the unit, which specify the number of elements of
+// that data type pointed to by header?
+// For example,
+//    uint8_t header[80];
+// should has header_size of (sizeof header)/(sizeof header[0]) rather than
+// sizeof header?  And the get_header() should aware that header_size is the
+// multiple of sizeof header rather than multiple of byte unit.
+// (When thinking in a highly generic and portable way :) )
+//
+// But at this time, header_size has unit in byte, which specify the length in
+// bytes of the buffer pointed to by header.
+static int get_header (int s, uint8_t *header, size_t header_size) {
 
   unsigned char  pre_header[8];
   int            i, header_len;
@@ -252,7 +333,7 @@
 
   while (1) {
 
-    if (!get_data (s, (char *)pre_header, 8)) {
+    if (!get_data (s, (char *)pre_header, sizeof pre_header, 8)) {
       fprintf (stderr,_("pre-header read failed\n"));
       return 0;
     }
@@ -270,7 +351,17 @@
       dprintf (_("asf header packet detected, len=%d\n"),
 	      packet_len);
 
-      if (!get_data (s, (char *)&header[header_len], packet_len)) {
+      if (!get_data (s, (char *)&header[header_len],
+		     // casting negative to size_t may cause buffer overflow
+		     //header_size - header_len,
+		     // So, select non-negative value
+		     //max(header_size - header_len, 0),
+		     // This is equivalent to using max() above.
+		     //header_size > header_len ? header_size - header_len : 0,
+		     // For generic stuff.
+		     header_size > header_len*(sizeof header[0]) ?
+		       header_size - header_len*(sizeof header[0]) : 0,
+		     packet_len)) {
 	fprintf (stderr,_("header data read failed\n"));
 	return 0;
       }
@@ -292,7 +383,7 @@
       int packet_len, command;
       char data[BUF_SIZE];
 
-      if (!get_data (s, (char *)&packet_len, 4)) {
+      if (!get_data (s, (char *)&packet_len, sizeof packet_len, 4)) {
 	fprintf (stderr,_("packet_len read failed\n"));
 	return 0;
       }
@@ -302,7 +393,7 @@
       dprintf (_("command packet detected, len=%d\n"),
 	      packet_len);
       
-      if (!get_data (s, data, packet_len)) {
+      if (!get_data (s, data, sizeof data, packet_len)) {
 	fprintf (stderr,_("command data read failed\n"));
 	return 0;
       }
@@ -373,8 +464,10 @@
 
       dprintf (_("stream object, stream id: %d\n"), stream_id);
 
-      stream_ids[num_stream_ids] = stream_id;
-      num_stream_ids++;
+      if (num_stream_ids < sizeof stream_ids / sizeof stream_ids[0]) {
+        stream_ids[num_stream_ids] = stream_id;
+        num_stream_ids++;
+      }
       
 
       /*
@@ -402,7 +495,7 @@
   int            i;
   char           data[BUF_SIZE];
 
-  if (!get_data (s, (char *)pre_header, 8)) {
+  if (!get_data (s, (char *)pre_header, sizeof pre_header, 8)) {
     fprintf (stderr,_("pre-header read failed\n"));
     return 0;
   }
@@ -420,7 +513,7 @@
     dprintf (_("asf media packet detected, len=%d\n"),
 	    packet_len);
 
-    if (!get_data (s, data, packet_len)) {
+    if (!get_data (s, data, sizeof data, packet_len)) {
       fprintf (stderr,_("media data read failed\n"));
       return 0;
     }
@@ -431,7 +524,7 @@
 
     int packet_len, command;
 
-    if (!get_data (s, (char *)&packet_len, 4)) {
+    if (!get_data (s, (char *)&packet_len, sizeof packet_len, 4)) {
       dprintf (_("packet_len read failed\n"));
       return 0;
     }
@@ -441,7 +534,7 @@
     dprintf (_("command packet detected, len=%d\n"),
 	    packet_len);
 
-    if (!get_data (s, data, packet_len)) {
+    if (!get_data (s, data, sizeof data, packet_len)) {
       fprintf (stderr,_("command data read failed\n"));
       return 0;
     }
@@ -669,32 +762,36 @@
   char uuid_string[37];
   uuid_unparse(client_uuid, uuid_string);
   
-  sprintf (str,
+  snprintf (str, sizeof str,
 	   "\034\003NSPlayer/9.0.0.2800; "
 	   "{%s}; "
 	   "Host: %s", uuid_string, host);
-  string_utf16 (data, str, strlen(str)+2);
+           // long host[] can cause buffer overflow if use sprintf()
+  string_utf16 (data, sizeof data, str, strlen(str)+2);
 
   send_command (s, 1, 0, 0x0004000b, strlen(str) * 2+8, data);
 
-  len = read (s, data, BUF_SIZE) ;
+  len = read (s, data, sizeof data) ;
+  //len = read (s, data, BUF_SIZE) ; // buffer overflow
   if (len)
     print_answer (data, len);
   
   /* cmd2 */
 
-  string_utf16 (&data[8], "\002\000\\\\192.168.0.129\\TCP\\1037\0000", 
+  string_utf16 (&data[8], sizeof data - 8*(sizeof data[0]),
+		"\002\000\\\\192.168.0.129\\TCP\\1037\0000", 
 		28);
   memset (data, 0, 8);
   send_command (s, 2, 0, 0, 28*2+8, data);
 
-  len = read (s, data, BUF_SIZE) ;
+  len = read (s, data, sizeof data) ;
+  //len = read (s, data, BUF_SIZE) ; // buffer overflow
   if (len)
     print_answer (data, len);
 
   /* 0x5 */
 
-  string_utf16 (&data[8], path, strlen(path));
+  string_utf16 (&data[8], sizeof data - 8*(sizeof data[0]), path, strlen(path));
   memset (data, 0, 8);
   send_command (s, 5, 0, 0, strlen(path)*2+12, data);
 
@@ -710,13 +807,17 @@
   num_stream_ids = 0;
   /* get_headers(s, asf_header);  */
 
-  asf_header_len = get_header (s, asf_header);
+  asf_header_len = get_header (s, asf_header, sizeof asf_header);
   packet_length = interp_header (asf_header, asf_header_len);
 
   /* 0x33 */
 
   memset (data, 0, 40);
 
+  // num_stream_ids is limited to size of stream_ids array (in interp_header())
+  // which is 20.
+  // (20-1)*6 == 114 < sizeof data == 1024
+  // No buffer overflow here :)
   for (i=1; i<num_stream_ids; i++) {
     data [ (i-1) * 6 + 2 ] = 0xFF;
     data [ (i-1) * 6 + 3 ] = 0xFF;

Message #12 received at 374577-quiet@bugs.debian.org (full text, mbox, reply):

From: "Wesley J. Landaker" <wjl@icecavern.net>

To: control@bugs.debian.org, 374577-quiet@bugs.debian.org

Subject: this is not a bug in mimms 2.0.0

Date: Tue, 20 Jun 2006 17:48:02 -0600

[Message part 1 (text/plain, inline)]

close #374577 2.0.0-1
thanks

This bug applies to mimms 0.0.9, but not 2.0.0. Marking fixed in Debian 
revision 2.0.0-1.

-- 
Wesley J. Landaker <wjl@icecavern.net> <xmpp:wjl@icecavern.net>
OpenPGP FP: 4135 2A3B 4726 ACC5 9094  0097 F0A9 8A4C 4CD6 E3D2

[Message part 2 (application/pgp-signature, inline)]

Message #17 received at 374577@bugs.debian.org (full text, mbox, reply):

From: Martin Schulze <joey@infodrom.org>

To: Anon Sricharoenchai <anon_hui@yahoo.com>

Cc: Debian Bug Tracking System <374577@bugs.debian.org>, tha_d@users.sourceforge.net

Subject: Re: mimms: patch to fix many buffer overflows vulnerability

Date: Thu, 22 Jun 2006 16:57:32 +0200

Anon Sricharoenchai wrote:
> Package: mimms
> Version: 0.0.9-1
> Severity: grave
> Justification: user security hole
> Tags: security patch
> 
> According to the patch attached in this report, it has many possible buffer
> overflows.
> For example,
> - memcpy(buf, data, length) without bounding the limit of "length",
>   while "length" depend on the input data incoming from the internet.
> - read(s, data, BUF_SIZE) in main(), where BUF_SIZE is much greater than
>   sizeof(data) which is only 1024 chars allocated in main(), while
>   BUF_SIZE is defined as 1024*128.

Woha!  Good work Anon!  I'm impressed.  I've assigned CVE-2006-2200 to
these issues.

One question remains, though:

> +  // buf_size = min(count, buf_size);
> +  if (buf_size > count) buf_size = count;

Is there any reason not to write mim() here?

Regards,

	Joey

-- 
Given enough thrust pigs will fly, but it's not necessarily a good idea.

Please always Cc to me when replying to me on the lists.

Message #22 received at 374577@bugs.debian.org (full text, mbox, reply):

From: Anon Sricharoenchai <anon_hui@yahoo.com>

To: Martin Schulze <joey@infodrom.org>

Cc: Debian Bug Tracking System <374577@bugs.debian.org>, anon_hui@yahoo.com

Subject: Re: mimms: patch to fix many buffer overflows vulnerability

Date: Mon, 26 Jun 2006 16:29:12 +0700 (ICT)

[Message part 1 (text/plain, inline)]

Martin Schulze <joey@infodrom.org> wrote:One question remains, though:

> +  // buf_size = min(count, buf_size);
> +  if (buf_size > count) buf_size = count;

Is there any reason not to write mim() here?


It's a bit faster than buf_size = min(), since there's no need to reassign "buf_size" again, if it's less than "count" :)


 __________________________________________________
คุณใช้ Yahoo! รึเปล่า
คุณเบื่อหน่ายอีเมลขยะใช่ไหม  Yahoo! เมล มีการป้องกันอีเมลขยะที่ดีที่สุด 
http://th.mail.yahoo.com 

[Message part 2 (text/html, inline)]

Message #27 received at 374577@bugs.debian.org (full text, mbox, reply):

From: Loïc Minier <lool@dooz.org>

To: "Wesley J. Landaker" <wjl@icecavern.net>

Cc: Martin Schulze <joey@infodrom.org>, team@security.debian.org

Subject: Re: mimms security bug in sarge

Date: Wed, 5 Jul 2006 19:12:22 +0200

        Hi,

 For the record, I forwarded the fix to Martin Pitt which reviewed the
 patch and spotted what looks like a mistake "start > end" instead of
 "start < end" which I've fixed and uploaded to unstable.  This is both
 in mms.c and mmsh.c

 Wesley, you might be interested in fixing this in other sources
 affected by the problem.

   Bye,
-- 
Loïc Minier <lool@dooz.org>

Message #32 received at 374577@bugs.debian.org (full text, mbox, reply):

From: Loïc Minier <lool@dooz.org>

To: "Wesley J. Landaker" <wjl@icecavern.net>

Cc: Martin Schulze <joey@infodrom.org>, team@security.debian.org, 374577@bugs.debian.org

Subject: Re: mimms security bug in sarge

Date: Tue, 11 Jul 2006 13:56:25 +0200

[Message part 1 (text/plain, inline)]

        Hi,

 The saga continues after the review of Matthias Hopf which suggested
 additional fixes.  See his message in attachment and the patches I made
 for Debian.

   Bye,
-- 
Loïc Minier <lool@dooz.org>

[Message part 2 (message/rfc822, inline)]

From: Matthias Hopf <mat@mshopf.de>

To: Xine DEV ML <xine-devel@lists.sourceforge.net>

Cc: vendor-sec@lst.de, Loic Minier <lool@dooz.org>

Subject: Security: libmms patches

Date: Tue, 11 Jul 2006 11:54:47 +0200

[Message part 3 (text/plain, inline)]

Hi all,

unfortunately this only made it through after xine-lib 1.1.2 release:

There has been a vulnerability report about libmms on [vendor-sec].
(CVE-2006-2200)

Please note that the original patch from the Debian maintainer is
partially incorrect (it should read memset(dest,0,2*len)), but the memset
isn't really necessary and could be nuked anyway. The use of memset in
the patch certainly doesn't do any harm, though, and it fixes the
potential overflow.

Luckily, xine uses libmms in a way that these vulnerabilities cannot be
exploited (buffers are large enough), and the xine module even seems to
rely on the side effects of the memset of the 'broken' library.  Note
that the library sources are included (not an externally linked
library).

While analyzing the source I found a couple of potential heap overflows,
though, which I'm pretty sure that they can be exploited with some
effort. They are fixed in CVS. I also attached the according patch. But
I'm pretty sure that I overlooked some additional ones.

This source is a wormhole.
Sorry, Thibaut, but then you maybe coded the glue layer only :-]

Matthias

-- 
Matthias Hopf <mhopf@suse.de>       __        __   __
Maxfeldstr. 5 / 90409 Nuernberg    (_   | |  (_   |__         mat@mshopf.de
Phone +49-911-74053-715            __)  |_|  __)  |__  labs   www.mshopf.de

[xine-lib-mms-fixes.diff (text/x-patch, attachment)]

[Message part 5 (message/rfc822, inline)]

From: Loïc Minier <lool+sf@via.ecp.fr>

To: Xine DEV ML <xine-devel@lists.sourceforge.net>, vendor-sec@lst.de

Subject: Re: Security: libmms patches

Date: Tue, 11 Jul 2006 13:52:12 +0200

[Message part 6 (text/plain, inline)]

        Hi,

On Tue, Jul 11, 2006, Matthias Hopf wrote:
> Please note that the original patch from the Debian maintainer is
> partially incorrect (it should read memset(dest,0,2*len)), but the memset
> isn't really necessary and could be nuked anyway. The use of memset in
> the patch certainly doesn't do any harm, though, and it fixes the
> potential overflow.
[...]
> While analyzing the source I found a couple of potential heap overflows,
> though, which I'm pretty sure that they can be exploited with some
> effort. They are fixed in CVS. I also attached the according patch. But
> I'm pretty sure that I overlooked some additional ones.

 Thanks for your review and for the additional fixes, I've adapted the
 patch and added the memset() fixes you mention for the libmms source
 package in Debian, I attach the incremental patch, "libmms_0.2-7.diff".
   I also attach the cumulative patch of all successive patches I have
 applied for CVE-2006-2200 to the libmms tree,
 "libmms_0.2-7-cumulative.diff".

   Bye,
-- 
Loïc Minier <lool@dooz.org>

[libmms_0.2-7.diff (text/plain, attachment)]

[libmms_0.2-7-cumulative.diff (text/plain, attachment)]

Send a report that this bug log contains spam.