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

[Michal Židek]

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.

Michal