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(a)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(a)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(a)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