[SSSD] [PATCH] mmap_cache: Check data->name value in client code

Simo Sorce simo at redhat.com
Wed Aug 14 20:54:21 UTC 2013


On Wed, 2013-08-14 at 20:32 +0200, Michal Židek wrote:
> On 08/14/2013 08:13 PM, Simo Sorce wrote:
> > On Wed, 2013-08-14 at 19:41 +0200, Michal Židek wrote:
> >>
> >> +        strs_offset = offsetof(struct sss_mc_grp_data, strs);
> >>           data = (struct sss_mc_grp_data *)rec->data;
> >> +        /* Integrity check
> >> +         * - name_len cannot be longer than all strings
> >> +         * - data->name cannot point outside strings
> >> +         * - all strings must be within data_table */
> >> +        if (name_len > data->strs_len
> >> +            || data->name > strs_offset + data->strs_len
> >> +            || (uint8_t *)data->strs + data->strs_len > max_addr) {
> >> +            ret = ENOENT;
> >> +            goto done;
> >> +        }
> >> +
> >
> > Sorry Michal, I noticed this before but forgot to comment on it.
> >
> > I think the second check should be:
> >   || (data->name + name_len) > (strs_offset + data->strs_len)
> >
> > Otherwise you could run out of bounds in the strcmp.
> 
> 
> You are right.
> 
> >
> > Also given this same complex check is done in 2 places maybe it can make
> > sense to have a common utility function or a macro to do the check.
> >
> > The other patches look good.
> >
> > Simo.
> >
> 
> Thanks for the review.
> 
> New patches are attached.

LGTM,
if Lukas also agree all is in order feel free to push.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list