On Thu, 2013-08-15 at 10:58 +0200, 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 <[email protected]>
> >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;
> >+ }
> >
>
> >From 9688783586aae89bf7da5923fd7a7de8b6f6694d Mon Sep 17 00:00:00 2001
> >From: Michal Zidek <[email protected]>
> >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 <[email protected]>
> >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
> I succesfully tested patch "Store corrupted mmap cache before reset"
> and backup file passwd_corrupted was created without any problem.
My fault, I should have not put an equal sign there, thanks for the
careful checking.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
