[AFL] r2 write outside allocated buffer in dalvik parsing flagname() #1839

Closed
ekse opened this Issue Dec 15, 2014 · 2 comments

Comments

Projects
None yet
2 participants
@ekse
Contributor

ekse commented Dec 15, 2014

flagname() is copying the string Ljava_lang_Object; and is copying outside the bounds of the allocated buffer. The allocated buffer size is 16 bytes but more bytes are written.

If you look at the backtrace, the class parameter in the call to flagname is in the middle of the string (class=0x584fab9 "ct;"), I am not entirely sure if this is the value at the start of the function or at the crash time, I will have to check that later.

The code in flagname copies 1 byte at a time in a loop, it would probably be safer to use a bounded copy function.

Crash file : https://www.dropbox.com/sh/vyfrkckh11a36qj/AADaRBxdINtT1ie6etMpaPX4a/1/id_000007%2Csig_06%2Csrc_000000%2Cop_flip4%2Cpos_140?dl=1
Base file : https://www.dropbox.com/sh/vyfrkckh11a36qj/AADmbWbWRN07xSerpKY8M5Dta/Hello.dex?dl=1

Valgrind output

==18969== Invalid write of size 1
==18969==    at 0x42E34D2: dex_loadcode.isra.0.part.1 (bin_dex.c:81)
==18969==    by 0x42E4D79: entries (bin_dex.c:603)
==18969==    by 0x429811C: r_bin_object_new (bin.c:375)
==18969==    by 0x429BC16: r_bin_load_io_at_offset_as_sz (bin.c:976)
==18969==    by 0x429C81B: r_bin_load_io_at_offset_as (bin.c:597)
==18969==    by 0x429D32A: r_bin_load_io (bin.c:496)
==18969==    by 0x40F72A5: r_core_bin_load (file.c:338)
==18969==    by 0x804C0EA: main (radare2.c:546)
==18969==  Address 0x584fb60 is 0 bytes after a block of size 16 alloc'd
==18969==    at 0x402A17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==18969==    by 0x42E33C5: dex_loadcode.isra.0.part.1 (bin_dex.c:57)
==18969==    by 0x42E4D79: entries (bin_dex.c:603)
==18969==    by 0x429811C: r_bin_object_new (bin.c:375)
==18969==    by 0x429BC16: r_bin_load_io_at_offset_as_sz (bin.c:976)
==18969==    by 0x429C81B: r_bin_load_io_at_offset_as (bin.c:597)
==18969==    by 0x429D32A: r_bin_load_io (bin.c:496)
==18969==    by 0x40F72A5: r_core_bin_load (file.c:338)
==18969==    by 0x804C0EA: main (radare2.c:546)

Backtrace

#0  0x042e34d2 in flagname (method=0x584fb18 "<init>", class=0x584fab9 "ct;") at /home/ml2/tools/afl-0.89b/projects/radare2/libr/..//libr/bin/p/bin_dex.c:81
#1  dex_loadcode (bin=bin@entry=0x584f1a0, arch=<optimized out>) at /home/ml2/tools/afl-0.89b/projects/radare2/libr/..//libr/bin/p/bin_dex.c:400
#2  0x042e4d7a in dex_loadcode (arch=0x58448d8, bin=0x584f1a0) at /home/ml2/tools/afl-0.89b/projects/radare2/libr/..//libr/bin/p/bin_dex.c:603
#3  entries (arch=0x58448d8) at /home/ml2/tools/afl-0.89b/projects/radare2/libr/..//libr/bin/p/bin_dex.c:593
#4  0x0429811d in r_bin_object_set_items (o=0x584b968, binfile=0x58448d8) at bin.c:375
#5  r_bin_object_new (binfile=binfile@entry=0x58448d8, plugin=plugin@entry=0x57abac8, baseaddr=0x0, loadaddr=0x0, offset=0x0, sz=0x3e8) at bin.c:870
#6  0x0429bc17 in r_bin_file_new_from_bytes (xtrname=0x0, offset=0x0, pluginname=0x0, fd=<optimized out>, loadaddr=0x0, baseaddr=0x0, rawstr=<optimized out>,
    file_sz=<optimized out>, sz=<optimized out>, bytes=0x58444c0 "dex\n035", file=<optimized out>, bin=0x57a8290) at bin.c:976
#7  r_bin_load_io_at_offset_as_sz (bin=bin@entry=0x57a8290, desc=desc@entry=0x5844198, baseaddr=0x0, loadaddr=0x0, xtr_idx=xtr_idx@entry=0x0, offset=0x0, name=name@entry=0x0,
    sz=0x8000000) at bin.c:582
#8  0x0429c81c in r_bin_load_io_at_offset_as (bin=0x57a8290, desc=0x5844198, baseaddr=0x0, loadaddr=0x0, xtr_idx=0x0, offset=0x0, name=0x0) at bin.c:597
#9  0x0429d32b in r_bin_load_io (bin=0x57a8290, desc=0x5844198, baseaddr=0x0, loadaddr=0x0, xtr_idx=0x0) at bin.c:496
#10 0x040f72a6 in r_core_file_do_load_for_io_plugin (loadaddr=0x0, baseaddr=0x0, r=0x8053300 <r>) at file.c:338
#11 r_core_bin_load (r=0x8053300 <r>, filenameuri=0x58441f0 "id:000007,sig:06,src:000000,op:flip4,pos:140", baddr=0x0) at file.c:470
#12 0x0804c0eb in main (argc=0x2, argv=0xbe86a634, envp=0xbe86a640) at radare2.c:546
#13 0x0555ba83 in __libc_start_main (main=0x804a120 <main>, argc=0x2, argv=0xbe86a634, init=0x8050920 <__libc_csu_init>, fini=0x8050990 <__libc_csu_fini>,
    rtld_fini=0x400f180 <_dl_fini>, stack_end=0xbe86a62c) at libc-start.c:287
#14 0x0804feb5 in _start ()

static char *flagname (const char *class, const char *method) {

 53 static char *flagname (const char *class, const char *method) {
 54     char *p, *str, *s;
 55     if (!class || !method)
 56         return NULL;
 57     s = malloc (strlen (class) + strlen (method)+10);   <---- allocation here
 58     str = s;
 59     p = (char*)r_str_lchr (class, '$');
 60     if (!p) p = (char *)r_str_lchr (class, '/');
 61 //eprintf ("D=%d (%s)\n", p, p?p:".");
 62     p = (char*)r_str_rchr (class, p, '/');
 63 #if 1
 64     //if (!p) p = class; else p--;
 65 //if (p) p = r_str_lchr (p, '/');
 66 //eprintf ("P=%d\n", p);
 67 #if 0
 68     if (!p) {
 69         char *q = r_str_lchr (p-1, '/');
 70         if (q) p = q;
 71     }
 72 #endif
 73     if (p) class = p+1;
 74 #endif
 75 //eprintf ("NAME (%s)\n", class);
 76     for (str=s; *class; class++) {
 77         switch (*class) {
 78         case '$':
 79         case '/': *s++ = '_'; break;
 80         case ';': *s++ = '.'; break;
 81         default: *s++ = *class; break;  <--- write happens here
 82         }
 83     }
 84     for (*s++='.'; *method; method++) {
 85         switch (*method) {
 86         case '<': case '>':
 87         case '/': *s++ = '_'; break;
 88         case ';': *s++ = '.'; break;
 89         default: *s++ = *method; break;
 90         }
 91     }
 92     *s = 0;
 93     return str;
 94 }
 95 
@ekse

This comment has been minimized.

Show comment
Hide comment
@ekse

ekse Dec 16, 2014

Contributor

Here's a breakdown of what happens. flagname(class, method) is called to generate a flag.

flagname (class=0x8116150 "Ljava/lang/Object;", method=0x8116168 "<init>")

In the fuzzed file the offset to the string "Ljava/lang/Object;" points 2 byte before it on a NULL byte.

gdb-peda$ hexdump class /2
0x0000 0x08116210 ¦ 00 12 4c 6a 61 76 61 2f 6c 61 6e 67 2f 4f 62 6a  ¦ ..Ljava/lang/Obj
0x0010 0x08116220 ¦ 65 63 74 3b 00 12 4c 6a 61 76 61 2f 6c 61 6e 67  ¦ ect;..Ljava/lang
0x0020 0x08116230

Let's look what happens in flagname() line by line (commented out code is removed).

s = malloc (strlen (class) + strlen (method)+10); // strlen(class) returns 0
p = (char*)r_str_lchr (class, '$'); // r_str_lchr returns 0
if (!p) p = (char *)r_str_lchr (class, '/'); // r_str_lchr returns 0
p = (char*)r_str_rchr (class, p, '/'); // r_str_rchr returns the address of class
if (p) class = p+1; // p now points to class+1 which is the next string in the file '\x12Ljava/lang/String'

This is the first bug, p shouldn't point after the end of the class string and will result in a bigger copy than the size of the buffer. A workaround could be to check for this condition, eg.

 if (p && p < (class + strlen(class)) class = p+1

The second issue is that the copy is done unbounded. If the copy was done bounded to the size of the malloc'ed buffer the overflow wouldn't happen even with the first bug. Ideally the code should use a function such as strlcpy (http://www.sudo.ws/todd/papers/strlcpy.html), there might already be such a function in utils/str.c.

for (str=s; *class; class++) {
    switch (*class) {
    case '$':
    case '/': *s++ = '_'; break;
    case ';': *s++ = '.'; break;
    default: *s++ = *class; break;
    }

In any case the flagname function should be cleaned up, there's a lot of leftover code.

Contributor

ekse commented Dec 16, 2014

Here's a breakdown of what happens. flagname(class, method) is called to generate a flag.

flagname (class=0x8116150 "Ljava/lang/Object;", method=0x8116168 "<init>")

In the fuzzed file the offset to the string "Ljava/lang/Object;" points 2 byte before it on a NULL byte.

gdb-peda$ hexdump class /2
0x0000 0x08116210 ¦ 00 12 4c 6a 61 76 61 2f 6c 61 6e 67 2f 4f 62 6a  ¦ ..Ljava/lang/Obj
0x0010 0x08116220 ¦ 65 63 74 3b 00 12 4c 6a 61 76 61 2f 6c 61 6e 67  ¦ ect;..Ljava/lang
0x0020 0x08116230

Let's look what happens in flagname() line by line (commented out code is removed).

s = malloc (strlen (class) + strlen (method)+10); // strlen(class) returns 0
p = (char*)r_str_lchr (class, '$'); // r_str_lchr returns 0
if (!p) p = (char *)r_str_lchr (class, '/'); // r_str_lchr returns 0
p = (char*)r_str_rchr (class, p, '/'); // r_str_rchr returns the address of class
if (p) class = p+1; // p now points to class+1 which is the next string in the file '\x12Ljava/lang/String'

This is the first bug, p shouldn't point after the end of the class string and will result in a bigger copy than the size of the buffer. A workaround could be to check for this condition, eg.

 if (p && p < (class + strlen(class)) class = p+1

The second issue is that the copy is done unbounded. If the copy was done bounded to the size of the malloc'ed buffer the overflow wouldn't happen even with the first bug. Ideally the code should use a function such as strlcpy (http://www.sudo.ws/todd/papers/strlcpy.html), there might already be such a function in utils/str.c.

for (str=s; *class; class++) {
    switch (*class) {
    case '$':
    case '/': *s++ = '_'; break;
    case ';': *s++ = '.'; break;
    default: *s++ = *class; break;
    }

In any case the flagname function should be cleaned up, there's a lot of leftover code.

@radare radare closed this in 5f42271 Dec 16, 2014

@radare

This comment has been minimized.

Show comment
Hide comment
@radare

radare Dec 16, 2014

Owner

the simplest bugfix for this is changing if (p) to (p && *p) :)

Owner

radare commented Dec 16, 2014

the simplest bugfix for this is changing if (p) to (p && *p) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment