buffer overrun in fnmatch.c

classic Classic list List threaded Threaded
6 messages Options
enh
Reply | Threaded
Open this post in threaded view
|

buffer overrun in fnmatch.c

enh
automated fuzzing caught this:

#include <fnmatch.h>
#include <string.h>
int main() {
  char *str = strdup("*[\\$:*[:lower:]");
  fnmatch(str, str, 0x27);
}

==14566==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x60200000f000 at pc 0x0000004d32d9 bp 0x7ffc96df9710 sp
0x7ffc96df9708
READ of size 1 at 0x60200000f000 thread T0
    #0 0x4d32d8 in fnmatch_ch
bionic/libc/upstream-openbsd/lib/libc/gen/fnmatch.c:202:18
    #1 0x4cf780 in my_fnmatch
bionic/libc/upstream-openbsd/lib/libc/gen/fnmatch.c:422:25
    #2 0x4d3b4a in main fnmatch-test.c:5:3

0x60200000f000 is located 0 bytes to the right of 16-byte region
[0x60200000eff0,0x60200000f000)
allocated by thread T0 here:
    #0 0x493ac3 in strdup
llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:556:3
    #1 0x4d3b3a in main fnmatch-test.c:4:15


"[[:lower:]" seems to be an even more minimal test case.

this patch seems to fix the bug:

diff --git a/libc/upstream-openbsd/lib/libc/gen/fnmatch.c
b/libc/upstream-openbsd/lib/libc/gen/fnmatch.c
index e83dc43..c419dc9 100644
--- a/libc/upstream-openbsd/lib/libc/gen/fnmatch.c
+++ b/libc/upstream-openbsd/lib/libc/gen/fnmatch.c
@@ -198,7 +198,7 @@ leadingclosebrace:
              * "x-]" is not allowed unless escaped ("x-\]")
              * XXX: Fix for locale/MBCS character width
              */
-            if (((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
+            if (**pattern && ((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
             {
                 startch = *pattern;
                 *pattern += (escape && ((*pattern)[2] == '\\')) ? 3 : 2;
@@ -235,6 +235,8 @@ leadingclosebrace:
  tolower((unsigned char)**pattern)))
                 result = 0;

+            if (!**pattern) break;
+
             ++*pattern;
         }



--
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.

Reply | Threaded
Open this post in threaded view
|

Re: buffer overrun in fnmatch.c

Todd C. Miller
The problem is that classmatch() can change pattern so we need to
check to see if it was consumed afterwards.

 - todd

Index: lib/libc/gen/fnmatch.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/fnmatch.c,v
retrieving revision 1.18
diff -u -p -u -r1.18 fnmatch.c
--- lib/libc/gen/fnmatch.c 11 Dec 2014 16:25:34 -0000 1.18
+++ lib/libc/gen/fnmatch.c 31 Jul 2015 18:55:18 -0000
@@ -192,6 +192,7 @@ static int fnmatch_ch(const char **patte
                 result = 0;
                 continue;
             }
+            if (!**pattern) break;
 
 leadingclosebrace:
             /* Look at only well-formed range patterns;

Reply | Threaded
Open this post in threaded view
|

Re: buffer overrun in fnmatch.c

Stefan Sperling-5
In reply to this post by enh
On Fri, Jul 31, 2015 at 11:18:15AM -0700, enh wrote:

> automated fuzzing caught this:
>
> #include <fnmatch.h>
> #include <string.h>
> int main() {
>   char *str = strdup("*[\\$:*[:lower:]");
>   fnmatch(str, str, 0x27);
> }
>
> ==14566==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x60200000f000 at pc 0x0000004d32d9 bp 0x7ffc96df9710 sp
> 0x7ffc96df9708
> READ of size 1 at 0x60200000f000 thread T0
>     #0 0x4d32d8 in fnmatch_ch
> bionic/libc/upstream-openbsd/lib/libc/gen/fnmatch.c:202:18
>     #1 0x4cf780 in my_fnmatch
> bionic/libc/upstream-openbsd/lib/libc/gen/fnmatch.c:422:25
>     #2 0x4d3b4a in main fnmatch-test.c:5:3
>
> 0x60200000f000 is located 0 bytes to the right of 16-byte region
> [0x60200000eff0,0x60200000f000)
> allocated by thread T0 here:
>     #0 0x493ac3 in strdup
> llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:556:3
>     #1 0x4d3b3a in main fnmatch-test.c:4:15
>
>
> "[[:lower:]" seems to be an even more minimal test case.
>
> this patch seems to fix the bug:
>
> diff --git a/libc/upstream-openbsd/lib/libc/gen/fnmatch.c
> b/libc/upstream-openbsd/lib/libc/gen/fnmatch.c
> index e83dc43..c419dc9 100644
> --- a/libc/upstream-openbsd/lib/libc/gen/fnmatch.c
> +++ b/libc/upstream-openbsd/lib/libc/gen/fnmatch.c
> @@ -198,7 +198,7 @@ leadingclosebrace:
>               * "x-]" is not allowed unless escaped ("x-\]")
>               * XXX: Fix for locale/MBCS character width
>               */
> -            if (((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
> +            if (**pattern && ((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
>              {
>                  startch = *pattern;
>                  *pattern += (escape && ((*pattern)[2] == '\\')) ? 3 : 2;
> @@ -235,6 +235,8 @@ leadingclosebrace:
>   tolower((unsigned char)**pattern)))
>                  result = 0;
>
> +            if (!**pattern) break;
> +
>              ++*pattern;
>          }
>
>
>
> --
> Elliott Hughes - http://who/enh - http://jessies.org/~enh/
> Android native code/tools questions? Mail me/drop by/add me as a reviewer.

Thanks Elliott!

I can confirm with gdb that fnmatch reads past the pattern string passed
in by the caller in your test case:

  Breakpoint 2, fnmatch_ch (pattern=0x7f7ffffd2340, string=0x7f7ffffd2338, flags=Variable "flags" is not available.
 
  ) at /usr/src/lib/libc/gen/fnmatch.c:201
  201                 if (((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
  (gdb) p (*pattern)[0]
  $1 = 0 '\0'
  (gdb) p (const char *)&(*pattern)[-10]
  $2 = 0xb2012951860 "[[:lower:]"
  (gdb) up
  #1  0x00000b2091056dc9 in fnmatch (pattern=0xb2012951860 "[[:lower:]",
      string=0xb2012951860 "[[:lower:]", flags=Variable "flags" is not available.
  )
      at /usr/src/lib/libc/gen/fnmatch.c:443
  443                     if (!fnmatch_ch(&pattern, &string, flags))

I'd suggest we commit this patch which adds a small formatting
tweak to yours:

Index: gen/fnmatch.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/fnmatch.c,v
retrieving revision 1.18
diff -u -p -r1.18 fnmatch.c
--- gen/fnmatch.c 11 Dec 2014 16:25:34 -0000 1.18
+++ gen/fnmatch.c 31 Jul 2015 18:53:36 -0000
@@ -198,7 +198,7 @@ leadingclosebrace:
              * "x-]" is not allowed unless escaped ("x-\]")
              * XXX: Fix for locale/MBCS character width
              */
-            if (((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
+            if (**pattern && ((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
             {
                 startch = *pattern;
                 *pattern += (escape && ((*pattern)[2] == '\\')) ? 3 : 2;
@@ -234,6 +234,9 @@ leadingclosebrace:
                             && (tolower((unsigned char)**string) ==
  tolower((unsigned char)**pattern)))
                 result = 0;
+
+    if (!**pattern)
+ break;
 
             ++*pattern;
         }

Reply | Threaded
Open this post in threaded view
|

Re: buffer overrun in fnmatch.c

Stefan Sperling-5
In reply to this post by Todd C. Miller
On Fri, Jul 31, 2015 at 12:58:47PM -0600, Todd C. Miller wrote:

> The problem is that classmatch() can change pattern so we need to
> check to see if it was consumed afterwards.
>
>  - todd
>
> Index: lib/libc/gen/fnmatch.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/gen/fnmatch.c,v
> retrieving revision 1.18
> diff -u -p -u -r1.18 fnmatch.c
> --- lib/libc/gen/fnmatch.c 11 Dec 2014 16:25:34 -0000 1.18
> +++ lib/libc/gen/fnmatch.c 31 Jul 2015 18:55:18 -0000
> @@ -192,6 +192,7 @@ static int fnmatch_ch(const char **patte
>                  result = 0;
>                  continue;
>              }
> +            if (!**pattern) break;
>  
>  leadingclosebrace:
>              /* Look at only well-formed range patterns;

That's much cleaner and I can confirm it fixes the issue.
Can we move the break on the next line? Apart from that, ok with me.


(Yes, I'm too lazy to find a better way to drive gdb than below...)

(gdb) br fnmatch.c:202
No source file named fnmatch.c.
Make breakpoint pending on future shared library load? (y or [n]) y

Breakpoint 1 (fnmatch.c:202) pending.
(gdb) run
Starting program: /tmp/test
Breakpoint 2 at 0x168232e6085c: file /usr/src/lib/libc/gen/fnmatch.c, line 202.
Pending breakpoint "fnmatch.c:202" resolved

Breakpoint 2, fnmatch_ch (pattern=0x7f7ffffe69b0, string=0x7f7ffffe69a8, flags=Variable "flags" is not available.

) at /usr/src/lib/libc/gen/fnmatch.c:202
202                 if (((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
(gdb) p **pattern
$1 = 58 ':'
(gdb) c
Continuing.

Breakpoint 2, fnmatch_ch (pattern=0x7f7ffffe69b0, string=0x7f7ffffe69a8, flags=Variable "flags" is not available.

) at /usr/src/lib/libc/gen/fnmatch.c:202
202                 if (((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
(gdb) p **pattern
$2 = 108 'l'
(gdb) c
Continuing.

Breakpoint 2, fnmatch_ch (pattern=0x7f7ffffe69b0, string=0x7f7ffffe69a8, flags=Variable "flags" is not available.

) at /usr/src/lib/libc/gen/fnmatch.c:202
202                 if (((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
(gdb) p **pattern
$3 = 111 'o'
(gdb) c
Continuing.

Breakpoint 2, fnmatch_ch (pattern=0x7f7ffffe69b0, string=0x7f7ffffe69a8, flags=Variable "flags" is not available.

) at /usr/src/lib/libc/gen/fnmatch.c:202
202                 if (((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
(gdb) p **pattern
$4 = 119 'w'
(gdb) c
Continuing.

Breakpoint 2, fnmatch_ch (pattern=0x7f7ffffe69b0, string=0x7f7ffffe69a8, flags=Variable "flags" is not available.

) at /usr/src/lib/libc/gen/fnmatch.c:202
202                 if (((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
(gdb) p **pattern
$5 = 101 'e'
(gdb) c
Continuing.

Breakpoint 2, fnmatch_ch (pattern=0x7f7ffffe69b0, string=0x7f7ffffe69a8, flags=Variable "flags" is not available.

) at /usr/src/lib/libc/gen/fnmatch.c:202
202                 if (((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
(gdb) p **pattern
$6 = 114 'r'
(gdb) c
Continuing.

Breakpoint 2, fnmatch_ch (pattern=0x7f7ffffe69b0, string=0x7f7ffffe69a8, flags=Variable "flags" is not available.

) at /usr/src/lib/libc/gen/fnmatch.c:202
202                 if (((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
(gdb) p **pattern
$7 = 58 ':'
(gdb) c
Continuing.

Program exited with code 01.
(gdb)

Reply | Threaded
Open this post in threaded view
|

Re: buffer overrun in fnmatch.c

Todd C. Miller
On Fri, 31 Jul 2015 21:16:51 +0200, Stefan Sperling wrote:

> That's much cleaner and I can confirm it fixes the issue.
> Can we move the break on the next line? Apart from that, ok with me.

Sure.  I also verified the fix with valgrind.

 - todd

Index: lib/libc/gen/fnmatch.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/fnmatch.c,v
retrieving revision 1.18
diff -u -p -u -r1.18 fnmatch.c
--- lib/libc/gen/fnmatch.c 11 Dec 2014 16:25:34 -0000 1.18
+++ lib/libc/gen/fnmatch.c 31 Jul 2015 20:38:17 -0000
@@ -192,6 +192,8 @@ static int fnmatch_ch(const char **patte
                 result = 0;
                 continue;
             }
+            if (!**pattern)
+                break;
 
 leadingclosebrace:
             /* Look at only well-formed range patterns;

Reply | Threaded
Open this post in threaded view
|

Re: buffer overrun in fnmatch.c

Masao Uebayashi-3
In reply to this post by enh
On Fri, Jul 31, 2015 at 11:18:15AM -0700, enh wrote:
> automated fuzzing caught this:
>
> #include <fnmatch.h>
> #include <string.h>
> int main() {
>   char *str = strdup("*[\\$:*[:lower:]");
>   fnmatch(str, str, 0x27);
> }

This is the output of Valgrind as of today:

==7819== Memcheck, a memory error detector
==7819== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==7819== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==7819== Command: ./fnmatch
==7819==
==7819== Invalid read of size 1
==7819==    at 0x54438F2: fnmatch_ch (fnmatch.c:201)
==7819==    by 0x5443FCB: fnmatch (fnmatch.c:417)
==7819==    by 0x108C4D: main (fnmatch.c:5)
==7819==  Address 0x58f8050 is 0 bytes after a block of size 16 alloc'd
==7819==    at 0x501B224: malloc (in /usr/local/lib/valgrind/vgpreload_memcheck-amd64-openbsd.so)
==7819==    by 0x54A7D28: strdup (strdup.c:45)
==7819==    by 0x108C37: main (fnmatch.c:4)
==7819==
==7819== Invalid read of size 1
==7819==    at 0x54439A0: fnmatch_ch (fnmatch.c:238)
==7819==    by 0x5443FCB: fnmatch (fnmatch.c:417)
==7819==    by 0x108C4D: main (fnmatch.c:5)
==7819==  Address 0x58f8050 is 0 bytes after a block of size 16 alloc'd
==7819==    at 0x501B224: malloc (in /usr/local/lib/valgrind/vgpreload_memcheck-amd64-openbsd.so)
==7819==    by 0x54A7D28: strdup (strdup.c:45)
==7819==    by 0x108C37: main (fnmatch.c:4)
==7819==
==7819== Invalid read of size 1
==7819==    at 0x54438F2: fnmatch_ch (fnmatch.c:201)
==7819==    by 0x5443E68: fnmatch (fnmatch.c:443)
==7819==    by 0x108C4D: main (fnmatch.c:5)
==7819==  Address 0x58f8050 is 0 bytes after a block of size 16 alloc'd
==7819==    at 0x501B224: malloc (in /usr/local/lib/valgrind/vgpreload_memcheck-amd64-openbsd.so)
==7819==    by 0x54A7D28: strdup (strdup.c:45)
==7819==    by 0x108C37: main (fnmatch.c:4)
==7819==
==7819== Invalid read of size 1
==7819==    at 0x54439A0: fnmatch_ch (fnmatch.c:238)
==7819==    by 0x5443E68: fnmatch (fnmatch.c:443)
==7819==    by 0x108C4D: main (fnmatch.c:5)
==7819==  Address 0x58f8050 is 0 bytes after a block of size 16 alloc'd
==7819==    at 0x501B224: malloc (in /usr/local/lib/valgrind/vgpreload_memcheck-amd64-openbsd.so)
==7819==    by 0x54A7D28: strdup (strdup.c:45)
==7819==    by 0x108C37: main (fnmatch.c:4)
==7819==
==7819==
==7819== FILE DESCRIPTORS: 3 open at exit.
==7819== Open file descriptor 2:
==7819==    <inherited from parent>
==7819==
==7819== Open file descriptor 1:
==7819==    <inherited from parent>
==7819==
==7819== Open file descriptor 0:
==7819==    <inherited from parent>
==7819==
==7819==
==7819== HEAP SUMMARY:
==7819==     in use at exit: 16 bytes in 1 blocks
==7819==   total heap usage: 1 allocs, 0 frees, 16 bytes allocated
==7819==
==7819== LEAK SUMMARY:
==7819==    definitely lost: 16 bytes in 1 blocks
==7819==    indirectly lost: 0 bytes in 0 blocks
==7819==      possibly lost: 0 bytes in 0 blocks
==7819==    still reachable: 0 bytes in 0 blocks
==7819==         suppressed: 0 bytes in 0 blocks
==7819== Rerun with --leak-check=full to see details of leaked memory
==7819==
==7819== For counts of detected and suppressed errors, rerun with: -v
==7819== ERROR SUMMARY: 6 errors from 4 contexts (suppressed: 0 from 0)