[PATCH v2] Add bounds checking to sys_getdents, sys_getdents64
Dmitry V. Levin
ldv at altlinux.org
Thu Sep 11 01:29:04 UTC 2014
On Mon, Aug 04, 2014 at 07:42:57AM +0530, Zubin Mithra wrote:
> * file.c (sys_getdents): Add d_reclen check.
> (sys_getdents64): Add d_reclen check.
>
> Signed-off-by: Zubin Mithra <zubin.mithra at gmail.com>
> ---
> file.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/file.c b/file.c
> index a92a7dc..0934ce1 100644
> --- a/file.c
> +++ b/file.c
> @@ -2076,6 +2076,10 @@ sys_getdents(struct tcb *tcp)
> i ? " " : "", d->d_ino, d->d_off);
> tprintf("d_reclen=%u, d_name=\"%s\", d_type=",
> d->d_reclen, d->d_name);
> + if (i + d->d_reclen >= len) {
> + tprints("...}");
> + break;
> + }
> printxval(direnttypes, buf[i + d->d_reclen - 1], "DT_???");
> tprints("}");
> }
I was talking about this d_reclen check back in April, but this is not
the only out-of-bounds read issue with getdents.
> @@ -2138,8 +2142,13 @@ sys_getdents64(struct tcb *tcp)
> tprints("d_type=");
> printxval(direnttypes, d->d_type, "DT_???");
> tprints(", ");
> - tprintf("d_reclen=%u, d_name=\"%s\"}",
> + tprintf("d_reclen=%u, d_name=\"%s\"",
> d->d_reclen, d->d_name);
> + if (i + d->d_reclen >= len) {
> + tprints("...}");
> + break;
> + }
> + tprints("}");
> }
> if (!d->d_reclen) {
> tprints("/* d_reclen == 0, problem here */");
getdents64 doesn't need this check, but there are other issues
similar to getdents.
Here is a fix of potential out-of-bounds read issues in
getdents/getdents64 I was thinking of:
diff --git a/file.c b/file.c
index 986f446..0044429 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 922452a..8f22d6d 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -20,6 +20,7 @@ TESTS = \
qual_syscall.test \
scm_rights-fd.test \
sigaction.test \
+ getdents.test \
stat.test \
net.test \
net-fd.test \
diff --git a/tests/getdents.test b/tests/getdents.test
new file mode 100755
index 0000000..187f04e
--- /dev/null
+++ b/tests/getdents.test
@@ -0,0 +1,32 @@
+#!/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'
+dirent='\{d_ino=[0-9]+, d_off=[0-9]+, d_reclen=[1-9][0-9]*, d_name="\.\.?", d_type=DT_DIR\}'
+dirent64='\{d_ino=[0-9]+, d_off=[0-9]+, d_reclen=[1-9][0-9]*, d_type=DT_DIR, d_name="\.\.?"\}'
+dirent_or_dirent64="($dirent $dirent|$dirent64 $dirent64)"
+getdents1="getdents(64)?\([0-9]+, \{$dirent_or_dirent64\}, [1-9][0-9]*\) += [1-9][0-9]*"
+getdents2='getdents(64)?\([0-9]+, \{\}, [1-9][0-9]*\) += 0'
+
+$STRACE $args -o $LOG ls emptydir &&
+LC_ALL=C grep -E -x "$getdents1" $LOG > /dev/null &&
+LC_ALL=C grep -E -x "$getdents2" $LOG > /dev/null ||
+ { rmdir emptydir; cat $LOG; fail_ "strace $args failed to trace getdents/getdents64 properly"; }
+
+rmdir emptydir
+
+exit 0
--
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20140911/f24dc9d3/attachment.bin>
More information about the Strace-devel
mailing list