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

[Michal Židek]

This should have been part of the patch:
[PATCH] mmap_cache: Check if slot and name_ptr are not invalid.

I missed the fact that name_ptr is called just name in the
client code and did not add check there.

Using manually corrupted cache (setting name_ptr to some high value
in hexeditor) this caused segfault in the client code. The attached
patch fixes this.

Please review and push this patch to master, 1-9 and 1-10.

Thanks
Michal

[Simo Sorce]

On Mon, 2013-08-12 at 19:56 +0200, Michal Židek wrote:
> This should have been part of the patch:
> [PATCH] mmap_cache: Check if slot and name_ptr are not invalid.
>
> I missed the fact that name_ptr is called just name in the
> client code and did not add check there.
>
> Using manually corrupted cache (setting name_ptr to some high value
> in hexeditor) this caused segfault in the client code. The attached
> patch fixes this.
>
> Please review and push this patch to master, 1-9 and 1-10.

I do not think this patch is correct.
Although normally data->name will point to data->strs it is a separate
pointer exactly becsaue the server code may decide to put the name
string somewhere in the strs buffer but not necessarily at the start.

What you need to check is somehing like:
if (data->name > offsetof(struct sss_mc_pwd_data, strs) +
data->strs_len) { return ENOENT; }

... except you should probably not trust strs_len entirely at this point
if you are trying to catch malformed data and you should also check that
data + strs_len is within the mmaped memory region.

Also at this point it may make sense to do a strlen(name) upfront and
check that strs_len > name and return immediately if not.

Simo.

[Michal Židek]

Thanks for the review Simo.

On 08/12/2013 11:07 PM, Simo Sorce wrote:
> On Mon, 2013-08-12 at 19:56 +0200, Michal Židek wrote:
> > This should have been part of the patch:
> > [PATCH] mmap_cache: Check if slot and name_ptr are not invalid.
> >
> > I missed the fact that name_ptr is called just name in the
> > client code and did not add check there.
> >
> > Using manually corrupted cache (setting name_ptr to some high value
> > in hexeditor) this caused segfault in the client code. The attached
> > patch fixes this.
> >
> > Please review and push this patch to master, 1-9 and 1-10.
>
> I do not think this patch is correct.
> Although normally data->name will point to data->strs it is a separate
> pointer exactly becsaue the server code may decide to put the name
> string somewhere in the strs buffer but not necessarily at the start.
>
> What you need to check is somehing like:
> if (data->name > offsetof(struct sss_mc_pwd_data, strs) +
> data->strs_len) { return ENOENT; }
>
> ... except you should probably not trust strs_len entirely at this point
> if you are trying to catch malformed data and you should also check that
> data + strs_len is within the mmaped memory region.

Ok. The new check tests if data + strs_len is in the data_table
(if it is somewhere else in the mmaped region it is already corrupted).
> Also at this point it may make sense to do a strlen(name) upfront and
> check that strs_len > name and return immediately if not.

I added this one check too... I think it is not bad to have another
line of defense.
> Simo.

Btw. I think we have off-by-one error in cases where we use pattern:
if (slot > MC_SIZE_TO_SLOTS(data_table_size) {
      return something (ENOENT/NULL);
}

If the slots are numbered from 0 and MC_SIZE_TO_SLOTS returns
number of slots needed to store some amount of data, there should
be '>=' no '>'. Please check my thinking. If I am correct then the
second patch should fix it.

I also removed the triple check at Lukas's request in the second
patch, since it modifies the same parts already).

New patches are attached.

Thanks
Michal

[Lukas Slebodnik]

On (13/08/13 19:45), Michal Židek wrote:
> Thanks for the review Simo.
>
> On 08/12/2013 11:07 PM, Simo Sorce wrote:
> > On Mon, 2013-08-12 at 19:56 +0200, Michal Židek wrote:
> > > This should have been part of the patch:
> > > [PATCH] mmap_cache: Check if slot and name_ptr are not invalid.
> > >
> > > I missed the fact that name_ptr is called just name in the
> > > client code and did not add check there.
> > >
> > > Using manually corrupted cache (setting name_ptr to some high value
> > > in hexeditor) this caused segfault in the client code. The attached
> > > patch fixes this.
> > >
> > > Please review and push this patch to master, 1-9 and 1-10.
> >
> > I do not think this patch is correct.
> > Although normally data->name will point to data->strs it is a separate
> > pointer exactly becsaue the server code may decide to put the name
> > string somewhere in the strs buffer but not necessarily at the start.
> >
> > What you need to check is somehing like:
> > if (data->name > offsetof(struct sss_mc_pwd_data, strs) +
> > data->strs_len) { return ENOENT; }
> >
> > ... except you should probably not trust strs_len entirely at this point
> > if you are trying to catch malformed data and you should also check that
> > data + strs_len is within the mmaped memory region.
>
> Ok. The new check tests if data + strs_len is in the data_table
> (if it is somewhere else in the mmaped region it is already corrupted).
> > Also at this point it may make sense to do a strlen(name) upfront and
> > check that strs_len > name and return immediately if not.
>
> I added this one check too... I think it is not bad to have another
> line of defense.
> > Simo.
>
> Btw. I think we have off-by-one error in cases where we use pattern:
> if (slot > MC_SIZE_TO_SLOTS(data_table_size) {
>     return something (ENOENT/NULL);
> }
>
> If the slots are numbered from 0 and MC_SIZE_TO_SLOTS returns
> number of slots needed to store some amount of data, there should
> be '>=' no '>'. Please check my thinking. If I am correct then the
> second patch should fix it.

Nice catch.
> I also removed the triple check at Lukas's request in the second
> patch, since it modifies the same parts already).

Thank you
> New patches are attached.
>
> Thanks
> Michal
>    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) {
>        return ENOENT;

replace return ENOENT with "ret = ENOENT; goto done", because it
introduce coverity warning:
sssd-1.10.93/src/sss_client/nss_mc_passwd.c:190: leaked_storage: Variable "rec"
going out of scope leaks the storage it points to.

And the same in src/sss_client/nss_mc_group.c
>    }
>   rec_name = (char *)data + data->name;
>   if (strcmp(name, rec_name) == 0) {
>       break;

LS

[Lukas Slebodnik]

On (12/08/13 19:56), Michal Židek wrote:
> This should have been part of the patch:
> [PATCH] mmap_cache: Check if slot and name_ptr are not invalid.
>
> I missed the fact that name_ptr is called just name in the
> client code and did not add check there.
>
> Using manually corrupted cache (setting name_ptr to some high value
> in hexeditor) this caused segfault in the client code. The attached
> patch fixes this.
>
> Please review and push this patch to master, 1-9 and 1-10.
>
> Thanks
> Michal

If you decided to make changes in client code you shold aslo update another
issue.

There is a pattern in functions (sss_nss_mc_getpwnam, ...)
> if (slot > MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)) {
>        return ENOENT;
>    }
>
> while (slot != MC_INVALID_VAL) {

This condition is subset of next condition.
>    if (slot > MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)) {
>        /* This probably means that the memory cache was corrupted */
>        return ENOENT;
>    }
>    // some work

In both tests (slot != MC_INVALID_VAL) (slot > MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)
result is the same (return ENOENT).
This like a combination of blacklist and then whitelist.

Three tests from quoted example can be reduced to a one test.

LS