From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 11 Sep 2014 05:21:53 +0400 From: "Dmitry V. Levin" To: ALT Devel discussion list Message-ID: <20140911012153.GA13087@altlinux.org> Mail-Followup-To: ALT Devel discussion list References: <1407118377-2545-1-git-send-email-zubin.mithra@gmail.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="lrZ03NoBR/3+SXJZ" Content-Disposition: inline In-Reply-To: <1407118377-2545-1-git-send-email-zubin.mithra@gmail.com> X-fingerprint: FE4C 93AB E19A 2E4C CB5D 3E4E 7CAB E6AC 9E35 361E Subject: Re: [devel] [PATCH v2] Add bounds checking to sys_getdents, sys_getdents64 X-BeenThere: devel@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: ALT Linux Team development discussions List-Id: ALT Linux Team development discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Sep 2014 01:21:53 -0000 Archived-At: List-Archive: List-Post: --lrZ03NoBR/3+SXJZ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Aug 04, 2014 at 07:42:57AM +0530, Zubin Mithra wrote: >=20 > * file.c (sys_getdents): Add d_reclen check. > (sys_getdents64): Add d_reclen check. >=20 > Signed-off-by: Zubin Mithra > --- > file.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) >=20 > 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=3D%u, d_name=3D\"%s\", d_type=3D", > d->d_reclen, d->d_name); > + if (i + d->d_reclen >=3D 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=3D"); > printxval(direnttypes, d->d_type, "DT_???"); > tprints(", "); > - tprintf("d_reclen=3D%u, d_name=3D\"%s\"}", > + tprintf("d_reclen=3D%u, d_name=3D\"%s\"", > d->d_reclen, d->d_name); > + if (i + d->d_reclen >=3D len) { > + tprints("...}"); > + break; > + } > + tprints("}"); > } > if (!d->d_reclen) { > tprints("/* d_reclen =3D=3D 0, problem here */"); getdents64 doesn't need this check, but there are other out-of-bounds read 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 =3D 0; + unsigned int i, len, dents =3D 0; char *buf; =20 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 =3D 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 =3D 1024*1024; - if (tcp->u_rval < 0) + else if (tcp->u_rval < (int) sizeof(struct kernel_dirent)) len =3D 0; - buf =3D 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 =3D tcp->u_rval; + + if (len) { + buf =3D 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 =3D NULL; } + if (!abbrev(tcp)) tprints("{"); - for (i =3D 0; i < len;) { + for (i =3D 0; len && i <=3D len - sizeof(struct kernel_dirent); ) { struct kernel_dirent *d =3D (struct kernel_dirent *) &buf[i]; + if (!abbrev(tcp)) { + int oob =3D d->d_reclen < sizeof(struct kernel_dirent) || + i + d->d_reclen - 1 >=3D len; + int d_name_len =3D oob ? len - i : d->d_reclen; + d_name_len -=3D offsetof(struct kernel_dirent, d_name) + 1; + tprintf("%s{d_ino=3D%lu, d_off=3D%lu, ", i ? " " : "", d->d_ino, d->d_off); - tprintf("d_reclen=3D%u, d_name=3D\"%s\", d_type=3D", - d->d_reclen, d->d_name); - printxval(direnttypes, buf[i + d->d_reclen - 1], "DT_???"); + tprintf("d_reclen=3D%u, d_name=3D\"%.*s\", d_type=3D", + 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 =3D=3D 0, problem here */"); + dents++; + if (d->d_reclen < sizeof(struct kernel_dirent)) { + tprints("/* d_reclen < sizeof(struct kernel_dirent) */"); break; } i +=3D 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 =3D 0; + /* the minimum size of a valid dirent64 structure */ + const unsigned int d_name_offset =3D offsetof(struct dirent64, d_name); + + unsigned int i, len, dents =3D 0; char *buf; =20 if (entering(tcp)) { @@ -2130,38 +2150,52 @@ sys_getdents64(struct tcb *tcp) return 0; } =20 - len =3D 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 =3D 1024*1024; - if (tcp->u_rval < 0) + else if (tcp->u_rval < (int) d_name_offset) len =3D 0; - buf =3D 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 =3D tcp->u_rval; + + if (len) { + buf =3D 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 =3D NULL; } + if (!abbrev(tcp)) tprints("{"); - for (i =3D 0; i < len;) { + for (i =3D 0; len && i <=3D len - d_name_offset; ) { struct dirent64 *d =3D (struct dirent64 *) &buf[i]; if (!abbrev(tcp)) { - tprintf("%s{d_ino=3D%" PRIu64 ", d_off=3D%" PRId64 ", ", + int d_name_len; + if (d->d_reclen >=3D d_name_offset + && i + d->d_reclen <=3D len) { + d_name_len =3D d->d_reclen - d_name_offset; + } else { + d_name_len =3D len - i - d_name_offset; + } + + tprintf("%s{d_ino=3D%" PRIu64 ", d_off=3D%" PRId64 + ", d_reclen=3D%u, d_type=3D", i ? " " : "", d->d_ino, - d->d_off); - tprints("d_type=3D"); + d->d_off, + d->d_reclen); printxval(direnttypes, d->d_type, "DT_???"); - tprints(", "); - tprintf("d_reclen=3D%u, d_name=3D\"%s\"}", - d->d_reclen, d->d_name); + tprintf(", d_name=3D\"%.*s\"}", + d_name_len, d->d_name); } - if (!d->d_reclen) { - tprints("/* d_reclen =3D=3D 0, problem here */"); + if (d->d_reclen < d_name_offset) { + tprints("/* d_reclen < offsetof(struct dirent64, d_name) */"); break; } i +=3D 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 =3D \ 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=3D.}/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=3D'-vegetdents,getdents64' +dirent=3D'\{d_ino=3D[0-9]+, d_off=3D[0-9]+, d_reclen=3D[1-9][0-9]*, d_name= =3D"\.\.?", d_type=3DDT_DIR\}' +dirent64=3D'\{d_ino=3D[0-9]+, d_off=3D[0-9]+, d_reclen=3D[1-9][0-9]*, d_ty= pe=3DDT_DIR, d_name=3D"\.\.?"\}' +dirent_or_dirent64=3D"($dirent $dirent|$dirent64 $dirent64)" +getdents1=3D"getdents(64)?\([0-9]+, \{$dirent_or_dirent64\}, [1-9][0-9]*\)= +=3D [1-9][0-9]*" +getdents2=3D'getdents(64)?\([0-9]+, \{\}, [1-9][0-9]*\) +=3D 0' + +$STRACE $args -o $LOG ls emptydir && +LC_ALL=3DC grep -E -x "$getdents1" $LOG > /dev/null && +LC_ALL=3DC grep -E -x "$getdents2" $LOG > /dev/null || + { rmdir emptydir; cat $LOG; fail_ "strace $args failed to trace getdents/= getdents64 properly"; } + +rmdir emptydir + +exit 0 --=20 ldv --lrZ03NoBR/3+SXJZ Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iEYEARECAAYFAlQQ+TEACgkQfKvmrJ41Nh6AeQCgmUL5YlbjOoF42DS0uKWsrsa+ mSsAn1mJF0dw2l/Pxv376tEVeEwxixpA =Mket -----END PGP SIGNATURE----- --lrZ03NoBR/3+SXJZ--