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

Michal Židek mzidek at redhat.com
Thu Aug 15 15:08:15 UTC 2013


On 08/15/2013 10:58 AM, Lukas Slebodnik wrote:
> On (14/08/13 20:32), 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.
>>
>> Michal
>
> I tested sssd with your patches and I found few issues.
>
>>From 8544f2eab5d20d082cdb3788ef07ebaa3b3157c5 Mon Sep 17 00:00:00 2001
>> From: Michal Zidek <mzidek at redhat.com>
>> Date: Mon, 12 Aug 2013 19:29:56 +0200
>> Subject: [PATCH 1/3] mmap_cache: Check data->name value in client code
>>
>> data->name value must be checked to prevent segfaults in
>> case of corrupted memory cache.
>>
>> resolves:
>> https://fedorahosted.org/sssd/ticket/2018
>> ---
>> src/sss_client/nss_mc_group.c  | 18 ++++++++++++++++++
>> src/sss_client/nss_mc_passwd.c | 19 +++++++++++++++++++
>> 2 files changed, 37 insertions(+)
>>
>> diff --git a/src/sss_client/nss_mc_group.c b/src/sss_client/nss_mc_group.c
> ... snip ..
>>
>> +        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 + name_len) > (strs_offset + data->strs_len)
>> +            || (uint8_t *)data->strs + data->strs_len > max_addr) {
> Could you use the same consitions also in responder code?
> In file src/responder/nss/nsssrv_mmap_cache.c, there is a similar code.
> #        rec = MC_SLOT_TO_PTR(mcc->data_table, slot, struct sss_mc_rec);
> #        name_ptr = *((rel_ptr_t *)rec->data);
> #        /* FIXME: This check relies on fact that offset of member strs
> #         * is the same in structures sss_mc_pwd_data and sss_mc_group_data. */
> #        if (name_ptr != offsetof(struct sss_mc_pwd_data, strs)) {
> #            DEBUG(SSSDBG_FATAL_FAILURE,
> #                  ("Corrupted fastcache. name_ptr value is %u.\n", name_ptr));
>
> And it was one of simo's objections in previous mail and FIXME will be removed.
>
>> +            ret = ENOENT;
>> +            goto done;
>> +        }
>>

Ok. I can do this, but since this will require responder code changes
and the previous patches changed the conditions in client code only --
except of the off-by-one error patch) I would prefer to have this as
a new patch (number 4).

>
>>From 9688783586aae89bf7da5923fd7a7de8b6f6694d Mon Sep 17 00:00:00 2001
>> From: Michal Zidek <mzidek at redhat.com>
>> Date: Wed, 14 Aug 2013 18:01:30 +0200
>> Subject: [PATCH 2/3] mmap_cache: Remove triple checks in client code.
>
> ACK
>
>>From 5db081330768957218d8d372c5ccf8dc75f223d7 Mon Sep 17 00:00:00 2001
>> From: Michal Zidek <mzidek at redhat.com>
>> Date: Wed, 14 Aug 2013 18:22:06 +0200
>> Subject: [PATCH 3/3] mmap_cache: Of by one error.
>>
>> Removes off by one error when using macro MC_SIZE_TO_SLOTS
>> and adds new macro MC_SLOT_WITHIN_BOUNDS.
>> ---
>> src/responder/nss/nsssrv_mmap_cache.c | 12 ++++++------
>> src/sss_client/nss_mc_group.c         |  8 ++++----
>> src/sss_client/nss_mc_passwd.c        |  8 ++++----
>> src/util/mmap_cache.h                 |  3 +++
>> 4 files changed, 17 insertions(+), 14 deletions(-)
>>
>> +++ b/src/util/mmap_cache.h
>> @@ -67,6 +67,9 @@ typedef uint32_t rel_ptr_t;
>> #define MC_SLOT_TO_PTR(base, slot, type) \
>>                                  (type *)((base) + ((slot) * MC_SLOT_SIZE))
>>
>> +#define MC_SLOT_WITHIN_BOUNDS(slot, dt_size) \
>> +    ((slot) <= ((dt_size) / MC_SLOT_SIZE))
>               ^^^^
> This is wrong. I tested edge case "slot == ((dt_size) / MC_SLOT_SIZE)"
> with gdb cheating
> and record was exactly behind the data_table, but on the other side

Ah, yes... sorry for that. This is the same thing why I wrote the
off-by-one patch. I should have noticed this. Fixed in the new patchset.

> I succesfully tested patch "Store corrupted mmap cache before reset"
> and backup file passwd_corrupted was created without any problem.
>
>> +
>> #define MC_VALID_BARRIER(val) (((val) & 0xff000000) == 0xf0000000)
>>
>> #define MC_CHECK_RECORD_LENGTH(mc_ctx, rec) \
>> --
>> 1.7.11.2
>>
>
> LS

Thank you Lukas and Simo for the review.

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: 3899 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130815/9816cb2b/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/20130815/9816cb2b/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: 6460 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130815/9816cb2b/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-mmap_cache-Use-better-checks-for-corrupted-mc-in-res.patch
Type: text/x-patch
Size: 3490 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130815/9816cb2b/attachment-0003.bin>


More information about the sssd-devel mailing list