From: Dmitry V. Levin Date: Wed, 10 Sep 2014 00:07:32 +0000 (+0000) Subject: getdents, getdents64: fix potential out-of-bounds read issues X-Git-Tag: v4.10~394 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=68d64241ac8e8c49f950fef506099f093fc6fa18;p=strace getdents, getdents64: fix potential out-of-bounds read issues * file.c (sys_getdents): Check for invalid d_reclen. Avoid reading from uninitialized memory. (sys_getdents64): Likewise. * tests/getdents.awk: New file. * tests/getdents.test: New test. * tests/Makefile.am (TESTS): Add it. (EXTRA_DIST): Add getdents.awk. --- diff --git a/file.c b/file.c index 986f4465..00444299 100644 --- a/file.c +++ b/file.c @@ -2060,7 +2060,7 @@ sys_readdir(struct tcb *tcp) int sys_getdents(struct tcb *tcp) { - int i, len, dents = 0; + unsigned int i, len, dents = 0; char *buf; if (entering(tcp)) { @@ -2072,38 +2072,55 @@ sys_getdents(struct tcb *tcp) tprintf("%#lx, %lu", tcp->u_arg[1], tcp->u_arg[2]); return 0; } - len = tcp->u_rval; - /* Beware of insanely large or negative values in tcp->u_rval */ + + /* Beware of insanely large or too small values in tcp->u_rval */ if (tcp->u_rval > 1024*1024) len = 1024*1024; - if (tcp->u_rval < 0) + else if (tcp->u_rval < (int) sizeof(struct kernel_dirent)) len = 0; - buf = len ? malloc(len) : NULL; - if (len && !buf) - die_out_of_memory(); - if (umoven(tcp, tcp->u_arg[1], len, buf) < 0) { - tprintf("%#lx, %lu", tcp->u_arg[1], tcp->u_arg[2]); - free(buf); - return 0; + else + len = tcp->u_rval; + + if (len) { + buf = malloc(len); + if (!buf) + die_out_of_memory(); + if (umoven(tcp, tcp->u_arg[1], len, buf) < 0) { + tprintf("%#lx, %lu", tcp->u_arg[1], tcp->u_arg[2]); + free(buf); + return 0; + } + } else { + buf = NULL; } + if (!abbrev(tcp)) tprints("{"); - for (i = 0; i < len;) { + for (i = 0; len && i <= len - sizeof(struct kernel_dirent); ) { struct kernel_dirent *d = (struct kernel_dirent *) &buf[i]; + if (!abbrev(tcp)) { + int oob = d->d_reclen < sizeof(struct kernel_dirent) || + i + d->d_reclen - 1 >= len; + int d_name_len = oob ? len - i : d->d_reclen; + d_name_len -= offsetof(struct kernel_dirent, d_name) + 1; + tprintf("%s{d_ino=%lu, d_off=%lu, ", i ? " " : "", d->d_ino, d->d_off); - tprintf("d_reclen=%u, d_name=\"%s\", d_type=", - d->d_reclen, d->d_name); - printxval(direnttypes, buf[i + d->d_reclen - 1], "DT_???"); + tprintf("d_reclen=%u, d_name=\"%.*s\", d_type=", + d->d_reclen, d_name_len, d->d_name); + if (oob) + tprints("?"); + else + printxval(direnttypes, buf[i + d->d_reclen - 1], "DT_???"); tprints("}"); } - if (!d->d_reclen) { - tprints("/* d_reclen == 0, problem here */"); + dents++; + if (d->d_reclen < sizeof(struct kernel_dirent)) { + tprints("/* d_reclen < sizeof(struct kernel_dirent) */"); break; } i += d->d_reclen; - dents++; } if (!abbrev(tcp)) tprints("}"); @@ -2117,7 +2134,10 @@ sys_getdents(struct tcb *tcp) int sys_getdents64(struct tcb *tcp) { - int i, len, dents = 0; + /* the minimum size of a valid dirent64 structure */ + const unsigned int d_name_offset = offsetof(struct dirent64, d_name); + + unsigned int i, len, dents = 0; char *buf; if (entering(tcp)) { @@ -2130,38 +2150,52 @@ sys_getdents64(struct tcb *tcp) return 0; } - len = tcp->u_rval; - /* Beware of insanely large or negative tcp->u_rval */ + /* Beware of insanely large or too small tcp->u_rval */ if (tcp->u_rval > 1024*1024) len = 1024*1024; - if (tcp->u_rval < 0) + else if (tcp->u_rval < (int) d_name_offset) len = 0; - buf = len ? malloc(len) : NULL; - if (len && !buf) - die_out_of_memory(); - - if (umoven(tcp, tcp->u_arg[1], len, buf) < 0) { - tprintf("%#lx, %lu", tcp->u_arg[1], tcp->u_arg[2]); - free(buf); - return 0; + else + len = tcp->u_rval; + + if (len) { + buf = malloc(len); + if (!buf) + die_out_of_memory(); + if (umoven(tcp, tcp->u_arg[1], len, buf) < 0) { + tprintf("%#lx, %lu", tcp->u_arg[1], tcp->u_arg[2]); + free(buf); + return 0; + } + } else { + buf = NULL; } + if (!abbrev(tcp)) tprints("{"); - for (i = 0; i < len;) { + for (i = 0; len && i <= len - d_name_offset; ) { struct dirent64 *d = (struct dirent64 *) &buf[i]; if (!abbrev(tcp)) { - tprintf("%s{d_ino=%" PRIu64 ", d_off=%" PRId64 ", ", + int d_name_len; + if (d->d_reclen >= d_name_offset + && i + d->d_reclen <= len) { + d_name_len = d->d_reclen - d_name_offset; + } else { + d_name_len = len - i - d_name_offset; + } + + tprintf("%s{d_ino=%" PRIu64 ", d_off=%" PRId64 + ", d_reclen=%u, d_type=", i ? " " : "", d->d_ino, - d->d_off); - tprints("d_type="); + d->d_off, + d->d_reclen); printxval(direnttypes, d->d_type, "DT_???"); - tprints(", "); - tprintf("d_reclen=%u, d_name=\"%s\"}", - d->d_reclen, d->d_name); + tprintf(", d_name=\"%.*s\"}", + d_name_len, d->d_name); } - if (!d->d_reclen) { - tprints("/* d_reclen == 0, problem here */"); + if (d->d_reclen < d_name_offset) { + tprints("/* d_reclen < offsetof(struct dirent64, d_name) */"); break; } i += d->d_reclen; diff --git a/tests/Makefile.am b/tests/Makefile.am index 922452ab..843fa1d4 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -18,6 +18,7 @@ TESTS = \ ptrace_setoptions.test \ strace-f.test \ qual_syscall.test \ + getdents.test \ scm_rights-fd.test \ sigaction.test \ stat.test \ @@ -34,6 +35,6 @@ net-fd.log: net.log TEST_LOG_COMPILER = $(srcdir)/run.sh -EXTRA_DIST = init.sh run.sh sigaction.awk $(TESTS) +EXTRA_DIST = init.sh run.sh getdents.awk sigaction.awk $(TESTS) CLEANFILES = $(TESTS:=.tmp) diff --git a/tests/getdents.awk b/tests/getdents.awk new file mode 100644 index 00000000..9e7b9682 --- /dev/null +++ b/tests/getdents.awk @@ -0,0 +1,45 @@ +BEGIN { + lines = 3 + fail = 0 + + i = "[0-9]+" + len = "[1-9]" i + + d_ino = "d_ino=" i + d_off = "d_off=" i + d_reclen = "d_reclen=" len + d_name1 = "d_name=\"\\.\"" + d_name2 = "d_name=\"\\.\\.\"" + d_type = "d_type=DT_DIR" + + d1_1 = "{" d_ino ", " d_off ", " d_reclen ", " d_name1 ", " d_type "}" + d1_2 = "{" d_ino ", " d_off ", " d_reclen ", " d_name2 ", " d_type "}" + d2_1 = "{" d_ino ", " d_off ", " d_reclen ", " d_type ", " d_name1 "}" + d2_2 = "{" d_ino ", " d_off ", " d_reclen ", " d_type ", " d_name2 "}" + + getdents = "^getdents\\(" i ", {(" d1_1 " " d1_2 "|" d1_2 " " d1_1 ")}, " len "\\) += " len "$" + getdents64 = "^getdents64\\(" i ", {(" d2_1 " " d2_2 "|" d2_2 " " d2_1 ")}, " len "\\) += " len "$" +} + +NR == 1 {if (match($0, getdents) || match($0, getdents64)) next} + +NR == 2 && /^getdents(64)?\([0-9]+, \{\}, [1-9][0-9]+\) += 0$/ {next} + +NR == lines && /^\+\+\+ exited with 0 \+\+\+$/ {next} + +{ + print "Line " NR " does not match: " $0 + fail=1 +} + +END { + if (NR != lines) { + print "Expected " lines " lines, found " NR " line(s)." + print "" + exit 1 + } + if (fail) { + print "" + exit 1 + } +} diff --git a/tests/getdents.test b/tests/getdents.test new file mode 100755 index 00000000..7fb81b83 --- /dev/null +++ b/tests/getdents.test @@ -0,0 +1,28 @@ +#!/bin/sh + +# Check that getdents/getdents64 syscalls are traced properly. + +. "${srcdir=.}/init.sh" + +check_prog grep +check_prog ls +check_prog mkdir +check_prog rmdir + +mkdir emptydir || + framework_skip_ 'failed to create an empty directory' + +ls emptydir || + { rmdir emptydir; framework_skip_ 'failed to list an empty directory'; } + +args='-vegetdents,getdents64' +$STRACE $args -o $LOG ls emptydir +rc=$? +rmdir emptydir +[ $rc -eq 0 ] || + { cat $LOG; fail_ "strace $args failed"; } + +awk -f "$srcdir"/getdents.awk $LOG || + { cat $LOG; fail_ "strace $args failed to trace getdents/getdents64 properly"; } + +exit 0