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

Michal Židek mzidek at redhat.com
Tue Aug 13 17:45:56 UTC 2013


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-mmap_cache-Check-data-name-value-in-client-code.patch
Type: text/x-patch
Size: 3822 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130813/d9f132ec/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-mmap_cache-Of-by-one-error-and-triple-check-removal.patch
Type: text/x-patch
Size: 8670 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130813/d9f132ec/attachment-0001.bin>


More information about the sssd-devel mailing list