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

[Simo Sorce]

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.

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

[Michal Židek]

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

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

[Michal Židek]

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

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

[Simo Sorce]

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.

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.

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

[Michal Židek]

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

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

[Simo Sorce]

On Wed, 2013-08-14 at 20:32 +0200, 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.

LGTM,
if Lukas also agree all is in order feel free to push.

Simo.

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

[Lukas Slebodnik]

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 
> 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 
> 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 
> 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.
> #define MC_VALID_BARRIER(val) (((val) & 0xff000000) == 0xf0000000)
> #define MC_CHECK_RECORD_LENGTH(mc_ctx, rec) \
>
> 1.7.11.2

LS

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

[Simo Sorce]

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 
> > 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 
> > 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 
> > 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.

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

[Michal Židek]

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 
> > 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 
> > 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 
> > 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

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

[Simo Sorce]

On Thu, 2013-08-15 at 17:08 +0200, Michal Židek wrote:
> +static size_t sss_mc_get_strs_offset(struct sss_mc_ctx *mcc)
> +{
> if (mcc->type == SSS_MC_PASSWD) {
>    return offsetof(struct sss_mc_pwd_data, strs);
> }
> return offsetof(struct sss_mc_grp_data, strs);
>
> +}

Can you use a switch()/case: here?

I know it looks a lot more boilerplate but if we start adding other maps
we'll not risk returning a completely bogus pointer by mistake.

Simo.

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

[Michal Židek]

On 08/15/2013 05:31 PM, Simo Sorce wrote:
> On Thu, 2013-08-15 at 17:08 +0200, Michal Židek wrote:
> > +static size_t sss_mc_get_strs_offset(struct sss_mc_ctx *mcc)
> > +{
> > if (mcc->type == SSS_MC_PASSWD) {
> >    return offsetof(struct sss_mc_pwd_data, strs);
> > }
> > return offsetof(struct sss_mc_grp_data, strs);
> >
> > +}
>
> Can you use a switch()/case: here?
>
> I know it looks a lot more boilerplate but if we start adding other maps
> we'll not risk returning a completely bogus pointer by mistake.
>
> Simo.

Ok. New patches are attached.

Thanks
Michal

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

[Simo Sorce]

On Thu, 2013-08-15 at 18:38 +0200, Michal Židek wrote:
> case SSS_MC_GROUP:
>    *_offset = offsetof(struct sss_mc_pwd_data, strs);
>    ret = EOK;
>    break;

This should use 'struct sss_mc_grp_data'.

Simo.

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

[Michal Židek]

On 08/15/2013 07:16 PM, Simo Sorce wrote:
> On Thu, 2013-08-15 at 18:38 +0200, Michal Židek wrote:
> > case SSS_MC_GROUP:
> >    *_offset = offsetof(struct sss_mc_pwd_data, strs);
> >    ret = EOK;
> >    break;
>
> This should use 'struct sss_mc_grp_data'.
>
> Simo.

Ah, sure, but I checked the last patchset I sent (without the
ret variable) and it is fixed there.

Michal

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

[Michal Židek]

On 08/15/2013 06:38 PM, Michal Židek wrote:
> On 08/15/2013 05:31 PM, Simo Sorce wrote:
> > On Thu, 2013-08-15 at 17:08 +0200, Michal Židek wrote:
> > > +static size_t sss_mc_get_strs_offset(struct sss_mc_ctx *mcc)
> > > +{
> > > if (mcc->type == SSS_MC_PASSWD) {
> > >    return offsetof(struct sss_mc_pwd_data, strs);
> > > }
> > > return offsetof(struct sss_mc_grp_data, strs);
> > >
> > > +}
> >
> > Can you use a switch()/case: here?
> >
> > I know it looks a lot more boilerplate but if we start adding other maps
> > we'll not risk returning a completely bogus pointer by mistake.
> >
> > Simo.
>
> Ok. New patches are attached.
>
> Thanks
> Michal

Actually, I think it looks better without the ret variable. So again,
new patches are attached.

Michal

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

[Michal Židek]

On 08/15/2013 07:18 PM, Michal Židek wrote:
> On 08/15/2013 06:38 PM, Michal Židek wrote:
> > On 08/15/2013 05:31 PM, Simo Sorce wrote:
> > > On Thu, 2013-08-15 at 17:08 +0200, Michal Židek wrote:
> > > > +static size_t sss_mc_get_strs_offset(struct sss_mc_ctx *mcc)
> > > > +{
> > > > if (mcc->type == SSS_MC_PASSWD) {
> > > >    return offsetof(struct sss_mc_pwd_data, strs);
> > > > }
> > > > return offsetof(struct sss_mc_grp_data, strs);
> > > >
> > > > +}
> > >
> > > Can you use a switch()/case: here?
> > >
> > > I know it looks a lot more boilerplate but if we start adding other maps
> > > we'll not risk returning a completely bogus pointer by mistake.
> > >
> > > Simo.
> >
> > Ok. New patches are attached.
> >
> > Thanks
> > Michal
>
> Actually, I think it looks better without the ret variable. So again,
> new patches are attached.
>
> Michal

Simo acked this on IRC.

Thanks
Michal

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

[Jakub Hrozek]

On Mon, Aug 19, 2013 at 03:27:04PM +0200, Michal Židek wrote:
> On 08/15/2013 07:18 PM, Michal Židek wrote:
> > On 08/15/2013 06:38 PM, Michal Židek wrote:
> > > On 08/15/2013 05:31 PM, Simo Sorce wrote:
> > > > On Thu, 2013-08-15 at 17:08 +0200, Michal Židek wrote:
> > > > > +static size_t sss_mc_get_strs_offset(struct sss_mc_ctx *mcc)
> > > > > +{
> > > > > if (mcc->type == SSS_MC_PASSWD) {
> > > > >    return offsetof(struct sss_mc_pwd_data, strs);
> > > > > }
> > > > > return offsetof(struct sss_mc_grp_data, strs);
> > > > >
> > > > > +}
> > > >
> > > > Can you use a switch()/case: here?
> > > >
> > > > I know it looks a lot more boilerplate but if we start adding other maps
> > > > we'll not risk returning a completely bogus pointer by mistake.
> > > >
> > > > Simo.
> > >
> > > Ok. New patches are attached.
> > >
> > > Thanks
> > > Michal
> >
> > Actually, I think it looks better without the ret variable. So again,
> > new patches are attached.
> >
> > Michal
>
> Simo acked this on IRC.
>
> Thanks
> Michal

Pushed to master, sssd-1-10 and sssd-1-9