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.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;
