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