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

Michal Židek mzidek at redhat.com
Wed Aug 14 17:41:27 UTC 2013


On 08/14/2013 06:43 PM, Michal Židek wrote:
> On 08/14/2013 04:51 PM, Simo Sorce wrote:
>> On Tue, 2013-08-13 at 19:42 +0200, Michal Židek wrote:
>>> Thanks for the review Simo.
>>>
>>> On 08/12/2013 11:07 PM, Simo Sorce wrote:
>>
>>>> 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).
>>
>> Sure.
>>
>>>> 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.
>>
>> Thanks.
>>
>>
>>> 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.
>>
>> Let's look at MC_SIZE_TO_SLOTS() definition:
>>
>> We always add (MC_SLOT_SIZE -1) to the requested size.
>>
>> This means if you ask 1 byte you get 1 slot, if you ask for MC_SLOT_SIZE
>> + 1 you get 2 slots.
>>
>> Ie you get the right number of slots required for the size when you call
>> that macro.
>>
>> So yeah good catch!
>>
>> However I think I'd like to see this fixed in a different way, by using
>> a macro as we use this check elsewhere.
>>
>> Something like:
>> #define MC_SLOT_WITHIN_BOUNDS(slot, size) \
>>     (slot <= (size / MC_SLOT_SIZE))
>>
>> and change the check to:
>> if (!MC_SLOT_WITHIN_BOUNDS(slot, mcc->dt_size)) { ...
>>
>> This would be more error proof if someone should add code in future to
>> check bounds.
>>
>>> I also removed the triple check at Lukas's request in the second
>>> patch, since it modifies the same parts already).
>>
>> I would prefer this to be done in a separate patch please.
>>
>> Simo.
>>
>
> Ok. I split the second patch into two. So now the second patch
> is about the triple check and the third one about the off-by-one error.
> The first patch is unchanged.
>
> Thanks for the review
>
> Michal
>
>

Sorry, the previous patchset included coverity issue that Lukas
mentioned. Sorry for the noise.

New patches attached.

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: 3869 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130814/699052e9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-mmap_cache-Remove-triple-checks-in-client-code.patch
Type: text/x-patch
Size: 5272 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130814/699052e9/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-mmap_cache-Of-by-one-error.patch
Type: text/x-patch
Size: 6461 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130814/699052e9/attachment-0002.bin>


More information about the sssd-devel mailing list