[PATCH v1] Add bounds checking to sys_getdents, sys_getdents64

[Zubin Mithra]

Hi Mike,

On Fri, Aug 1, 2014 at 4:25 PM, Mike Frysinger <vapier at gentoo.org> wrote:

> On Thu 03 Jul 2014 17:45:41 zubin.mithra at gmail.com wrote:
> > From: Zubin Mithra <zubin.mithra at gmail.com>
> >
> > * 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 | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/file.c b/file.c
> > index a92a7dc..d739df6 100644
> > --- a/file.c
> > +++ b/file.c
> > @@ -2041,7 +2041,8 @@ sys_readdir(struct tcb *tcp)
> >  int
> >  sys_getdents(struct tcb *tcp)
> >  {
> > -     int i, len, dents = 0;
> > +     unsigned int i;
> > +     int len, dents = 0;
> >       char *buf;
> >
> >       if (entering(tcp)) {
> > @@ -2076,6 +2077,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;
> > +                     }
>
> you shouldn't compare signed & unsigned values.  i'm not sure this code
> needs
> to have "i" converted to unsigned considering the top of it makes sure to
> clamp the value of len to [0, 1024*1024].
>
> also, should it be "...}" to indicate that there's something, but we're
> ignoring it ?  maybe not since we already silently clamp the result ...


Thank you for the review, Mike ! I've made the two changes and sent over a
patch for the same.



Thanks!
Zubin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20140804/b608f657/attachment.html>