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
>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/4] 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 index 2d69be9..da5da04 100644 --- a/src/sss_client/nss_mc_group.c +++ b/src/sss_client/nss_mc_group.c @@ -23,6 +23,7 @@ #include <stdio.h> #include <string.h> #include <stdlib.h> +#include <stddef.h> #include <sys/mman.h> #include <time.h> #include "nss_mc.h" @@ -102,12 +103,17 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len, uint32_t hash; uint32_t slot; int ret; + size_t strs_offset; + uint8_t *max_addr; ret = sss_nss_mc_get_ctx("group", &gr_mc_ctx); if (ret) { return ret; } + /* Get max address of data table. */ + max_addr = gr_mc_ctx.data_table + gr_mc_ctx.dt_size; + /* hashes are calculated including the NULL terminator */ hash = sss_nss_mc_hash(&gr_mc_ctx, name, name_len + 1); slot = gr_mc_ctx.hash_table[hash]; @@ -133,7 +139,19 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len, continue; } + 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) { + ret = ENOENT; + goto done; + } + rec_name = (char *)data + data->name; if (strcmp(name, rec_name) == 0) { break; diff --git a/src/sss_client/nss_mc_passwd.c b/src/sss_client/nss_mc_passwd.c index fa21bd2..4b08766 100644 --- a/src/sss_client/nss_mc_passwd.c +++ b/src/sss_client/nss_mc_passwd.c @@ -23,6 +23,7 @@ #include <stdio.h> #include <string.h> #include <stdlib.h> +#include <stddef.h> #include <sys/mman.h> #include <time.h> #include "nss_mc.h" @@ -103,12 +104,17 @@ errno_t sss_nss_mc_getpwnam(const char *name, size_t name_len, uint32_t hash; uint32_t slot; int ret; + size_t strs_offset; + uint8_t *max_addr; ret = sss_nss_mc_get_ctx("passwd", &pw_mc_ctx); if (ret) { return ret; } + /* Get max address of data table. */ + max_addr = pw_mc_ctx.data_table + pw_mc_ctx.dt_size; + /* hashes are calculated including the NULL terminator */ hash = sss_nss_mc_hash(&pw_mc_ctx, name, name_len + 1); slot = pw_mc_ctx.hash_table[hash]; @@ -134,7 +140,20 @@ errno_t sss_nss_mc_getpwnam(const char *name, size_t name_len, continue; } + strs_offset = offsetof(struct sss_mc_pwd_data, strs); + data = (struct sss_mc_pwd_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) { + ret = ENOENT; + goto done; + } + rec_name = (char *)data + data->name; if (strcmp(name, rec_name) == 0) { break; -- 1.7.11.2
>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/4] mmap_cache: Remove triple checks in client code. We had pattern in client code with 3 conditions that can be replaced with one. --- src/sss_client/nss_mc_group.c | 30 ++++++++++-------------------- src/sss_client/nss_mc_passwd.c | 30 ++++++++++-------------------- 2 files changed, 20 insertions(+), 40 deletions(-) diff --git a/src/sss_client/nss_mc_group.c b/src/sss_client/nss_mc_group.c index da5da04..9fe72a6 100644 --- a/src/sss_client/nss_mc_group.c +++ b/src/sss_client/nss_mc_group.c @@ -117,16 +117,11 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len, /* hashes are calculated including the NULL terminator */ hash = sss_nss_mc_hash(&gr_mc_ctx, name, name_len + 1); slot = gr_mc_ctx.hash_table[hash]; - if (slot > MC_SIZE_TO_SLOTS(gr_mc_ctx.dt_size)) { - return ENOENT; - } - - while (slot != MC_INVALID_VAL) { - if (slot > MC_SIZE_TO_SLOTS(gr_mc_ctx.dt_size)) { - /* This probably means that the memory cache was corrupted. */ - return ENOENT; - } + /* If slot is not within the bounds of mmaped region and + * it's value is not MC_INVALID_VAL, then the cache is + * probbably corrupted. */ + while (slot < MC_SIZE_TO_SLOTS(gr_mc_ctx.dt_size)) { ret = sss_nss_mc_get_record(&gr_mc_ctx, slot, &rec); if (ret) { goto done; @@ -160,7 +155,7 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len, slot = rec->next; } - if (slot == MC_INVALID_VAL) { + if (slot >= MC_SIZE_TO_SLOTS(gr_mc_ctx.dt_size)) { ret = ENOENT; goto done; } @@ -197,16 +192,11 @@ errno_t sss_nss_mc_getgrgid(gid_t gid, /* hashes are calculated including the NULL terminator */ hash = sss_nss_mc_hash(&gr_mc_ctx, gidstr, len+1); slot = gr_mc_ctx.hash_table[hash]; - if (slot > MC_SIZE_TO_SLOTS(gr_mc_ctx.dt_size)) { - return ENOENT; - } - - while (slot != MC_INVALID_VAL) { - if (slot > MC_SIZE_TO_SLOTS(gr_mc_ctx.dt_size)) { - /* This probably means that the memory cache was corrupted. */ - return ENOENT; - } + /* If slot is not within the bounds of mmaped region and + * it's value is not MC_INVALID_VAL, then the cache is + * probbably corrupted. */ + while (slot < MC_SIZE_TO_SLOTS(gr_mc_ctx.dt_size)) { ret = sss_nss_mc_get_record(&gr_mc_ctx, slot, &rec); if (ret) { goto done; @@ -227,7 +217,7 @@ errno_t sss_nss_mc_getgrgid(gid_t gid, slot = rec->next; } - if (slot == MC_INVALID_VAL) { + if (slot >= MC_SIZE_TO_SLOTS(gr_mc_ctx.dt_size)) { ret = ENOENT; goto done; } diff --git a/src/sss_client/nss_mc_passwd.c b/src/sss_client/nss_mc_passwd.c index 4b08766..7aca4a0 100644 --- a/src/sss_client/nss_mc_passwd.c +++ b/src/sss_client/nss_mc_passwd.c @@ -118,16 +118,11 @@ errno_t sss_nss_mc_getpwnam(const char *name, size_t name_len, /* hashes are calculated including the NULL terminator */ hash = sss_nss_mc_hash(&pw_mc_ctx, name, name_len + 1); slot = pw_mc_ctx.hash_table[hash]; - if (slot > MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)) { - return ENOENT; - } - - while (slot != MC_INVALID_VAL) { - if (slot > MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)) { - /* This probably means that the memory cache was corrupted */ - return ENOENT; - } + /* If slot is not within the bounds of mmaped region and + * it's value is not MC_INVALID_VAL, then the cache is + * probbably corrupted. */ + while (slot < MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)) { ret = sss_nss_mc_get_record(&pw_mc_ctx, slot, &rec); if (ret) { goto done; @@ -162,7 +157,7 @@ errno_t sss_nss_mc_getpwnam(const char *name, size_t name_len, slot = rec->next; } - if (slot == MC_INVALID_VAL) { + if (slot >= MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)) { ret = ENOENT; goto done; } @@ -199,16 +194,11 @@ errno_t sss_nss_mc_getpwuid(uid_t uid, /* hashes are calculated including the NULL terminator */ hash = sss_nss_mc_hash(&pw_mc_ctx, uidstr, len+1); slot = pw_mc_ctx.hash_table[hash]; - if (slot > MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)) { - return ENOENT; - } - - while (slot != MC_INVALID_VAL) { - if (slot > MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)) { - /* This probably means that the memory cache was corrupted */ - return ENOENT; - } + /* If slot is not within the bounds of mmaped region and + * it's value is not MC_INVALID_VAL, then the cache is + * probbably corrupted. */ + while (slot < MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)) { ret = sss_nss_mc_get_record(&pw_mc_ctx, slot, &rec); if (ret) { goto done; @@ -229,7 +219,7 @@ errno_t sss_nss_mc_getpwuid(uid_t uid, slot = rec->next; } - if (slot == MC_INVALID_VAL) { + if (slot >= MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)) { ret = ENOENT; goto done; } -- 1.7.11.2
>From 5a3dd0209949966a2199fad3e7e380180e4f9cf6 Mon Sep 17 00:00:00 2001 From: Michal Zidek <[email protected]> Date: Wed, 14 Aug 2013 18:22:06 +0200 Subject: [PATCH 3/4] 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(-) diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c index cd5a643..a1bab0c 100644 --- a/src/responder/nss/nsssrv_mmap_cache.c +++ b/src/responder/nss/nsssrv_mmap_cache.c @@ -368,12 +368,12 @@ static struct sss_mc_rec *sss_mc_find_record(struct sss_mc_ctx *mcc, hash = sss_mc_hash(mcc, key->str, key->len); slot = mcc->hash_table[hash]; - if (slot > MC_SIZE_TO_SLOTS(mcc->dt_size)) { + if (!MC_SLOT_WITHIN_BOUNDS(slot, mcc->dt_size)) { return NULL; } while (slot != MC_INVALID_VAL) { - if (slot > MC_SIZE_TO_SLOTS(mcc->dt_size)) { + if (!MC_SLOT_WITHIN_BOUNDS(slot, mcc->dt_size)) { DEBUG(SSSDBG_FATAL_FAILURE, ("Corrupted fastcache. Slot number too big.\n")); sss_mmap_cache_reset(mcc); @@ -617,13 +617,13 @@ errno_t sss_mmap_cache_pw_invalidate_uid(struct sss_mc_ctx *mcc, uid_t uid) hash = sss_mc_hash(mcc, uidstr, strlen(uidstr) + 1); slot = mcc->hash_table[hash]; - if (slot > MC_SIZE_TO_SLOTS(mcc->dt_size)) { + if (!MC_SLOT_WITHIN_BOUNDS(slot, mcc->dt_size)) { ret = ENOENT; goto done; } while (slot != MC_INVALID_VAL) { - if (slot > MC_SIZE_TO_SLOTS(mcc->dt_size)) { + if (!MC_SLOT_WITHIN_BOUNDS(slot, mcc->dt_size)) { DEBUG(SSSDBG_FATAL_FAILURE, ("Corrupted fastcache.\n")); sss_mmap_cache_reset(mcc); ret = ENOENT; @@ -755,13 +755,13 @@ errno_t sss_mmap_cache_gr_invalidate_gid(struct sss_mc_ctx *mcc, gid_t gid) hash = sss_mc_hash(mcc, gidstr, strlen(gidstr) + 1); slot = mcc->hash_table[hash]; - if (slot > MC_SIZE_TO_SLOTS(mcc->dt_size)) { + if (!MC_SLOT_WITHIN_BOUNDS(slot, mcc->dt_size)) { ret = ENOENT; goto done; } while (slot != MC_INVALID_VAL) { - if (slot > MC_SIZE_TO_SLOTS(mcc->dt_size)) { + if (!MC_SLOT_WITHIN_BOUNDS(slot, mcc->dt_size)) { DEBUG(SSSDBG_FATAL_FAILURE, ("Corrupted fastcache.\n")); sss_mmap_cache_reset(mcc); ret = ENOENT; diff --git a/src/sss_client/nss_mc_group.c b/src/sss_client/nss_mc_group.c index 9fe72a6..4e3d9fb 100644 --- a/src/sss_client/nss_mc_group.c +++ b/src/sss_client/nss_mc_group.c @@ -121,7 +121,7 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len, /* If slot is not within the bounds of mmaped region and * it's value is not MC_INVALID_VAL, then the cache is * probbably corrupted. */ - while (slot < MC_SIZE_TO_SLOTS(gr_mc_ctx.dt_size)) { + while (MC_SLOT_WITHIN_BOUNDS(slot, gr_mc_ctx.dt_size)) { ret = sss_nss_mc_get_record(&gr_mc_ctx, slot, &rec); if (ret) { goto done; @@ -155,7 +155,7 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len, slot = rec->next; } - if (slot >= MC_SIZE_TO_SLOTS(gr_mc_ctx.dt_size)) { + if (!MC_SLOT_WITHIN_BOUNDS(slot, gr_mc_ctx.dt_size)) { ret = ENOENT; goto done; } @@ -196,7 +196,7 @@ errno_t sss_nss_mc_getgrgid(gid_t gid, /* If slot is not within the bounds of mmaped region and * it's value is not MC_INVALID_VAL, then the cache is * probbably corrupted. */ - while (slot < MC_SIZE_TO_SLOTS(gr_mc_ctx.dt_size)) { + while (MC_SLOT_WITHIN_BOUNDS(slot, gr_mc_ctx.dt_size)) { ret = sss_nss_mc_get_record(&gr_mc_ctx, slot, &rec); if (ret) { goto done; @@ -217,7 +217,7 @@ errno_t sss_nss_mc_getgrgid(gid_t gid, slot = rec->next; } - if (slot >= MC_SIZE_TO_SLOTS(gr_mc_ctx.dt_size)) { + if (!MC_SLOT_WITHIN_BOUNDS(slot, gr_mc_ctx.dt_size)) { ret = ENOENT; goto done; } diff --git a/src/sss_client/nss_mc_passwd.c b/src/sss_client/nss_mc_passwd.c index 7aca4a0..a0a8d87 100644 --- a/src/sss_client/nss_mc_passwd.c +++ b/src/sss_client/nss_mc_passwd.c @@ -122,7 +122,7 @@ errno_t sss_nss_mc_getpwnam(const char *name, size_t name_len, /* If slot is not within the bounds of mmaped region and * it's value is not MC_INVALID_VAL, then the cache is * probbably corrupted. */ - while (slot < MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)) { + while (MC_SLOT_WITHIN_BOUNDS(slot, pw_mc_ctx.dt_size)) { ret = sss_nss_mc_get_record(&pw_mc_ctx, slot, &rec); if (ret) { goto done; @@ -157,7 +157,7 @@ errno_t sss_nss_mc_getpwnam(const char *name, size_t name_len, slot = rec->next; } - if (slot >= MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)) { + if (!MC_SLOT_WITHIN_BOUNDS(slot, pw_mc_ctx.dt_size)) { ret = ENOENT; goto done; } @@ -198,7 +198,7 @@ errno_t sss_nss_mc_getpwuid(uid_t uid, /* If slot is not within the bounds of mmaped region and * it's value is not MC_INVALID_VAL, then the cache is * probbably corrupted. */ - while (slot < MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)) { + while (MC_SLOT_WITHIN_BOUNDS(slot, pw_mc_ctx.dt_size)) { ret = sss_nss_mc_get_record(&pw_mc_ctx, slot, &rec); if (ret) { goto done; @@ -219,7 +219,7 @@ errno_t sss_nss_mc_getpwuid(uid_t uid, slot = rec->next; } - if (slot >= MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size)) { + if (!MC_SLOT_WITHIN_BOUNDS(slot, pw_mc_ctx.dt_size)) { ret = ENOENT; goto done; } diff --git a/src/util/mmap_cache.h b/src/util/mmap_cache.h index 6c223df..abf8cac 100644 --- a/src/util/mmap_cache.h +++ 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)) + #define MC_VALID_BARRIER(val) (((val) & 0xff000000) == 0xf0000000) #define MC_CHECK_RECORD_LENGTH(mc_ctx, rec) \ -- 1.7.11.2
>From 7bd1408c89a7564abe8f0c8fa28aa3bd42483665 Mon Sep 17 00:00:00 2001 From: Michal Zidek <[email protected]> Date: Thu, 15 Aug 2013 16:08:17 +0200 Subject: [PATCH 4/4] mmap_cache: Use better checks for corrupted mc in responder We introduced new way to check integrity of memcache in the client code. We should use similiar checks in the responder. --- src/responder/nss/nsssrv_mmap_cache.c | 68 +++++++++++++++++++++++++++++++++-- src/util/mmap_cache.h | 2 -- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c index a1bab0c..03752e5 100644 --- a/src/responder/nss/nsssrv_mmap_cache.c +++ b/src/responder/nss/nsssrv_mmap_cache.c @@ -356,6 +356,51 @@ static errno_t sss_mc_find_free_slots(struct sss_mc_ctx *mcc, return EOK; } +static errno_t sss_mc_get_strs_offset(struct sss_mc_ctx *mcc, + size_t *_offset) +{ + errno_t ret; + + switch (mcc->type) { + case SSS_MC_PASSWD: + *_offset = offsetof(struct sss_mc_pwd_data, strs); + ret = EOK; + break; + case SSS_MC_GROUP: + *_offset = offsetof(struct sss_mc_pwd_data, strs); + ret = EOK; + break; + default: + DEBUG(SSSDBG_FATAL_FAILURE, ("Unknown memory cache type.\n")); + ret = EINVAL; + } + + return ret; +} + +static errno_t sss_mc_get_strs_len(struct sss_mc_ctx *mcc, + struct sss_mc_rec *rec, + size_t *_len) +{ + errno_t ret; + + switch (mcc->type) { + case SSS_MC_PASSWD: + *_len = ((struct sss_mc_pwd_data *)&rec->data)->strs_len; + ret = EOK; + break; + case SSS_MC_GROUP: + *_len = ((struct sss_mc_grp_data *)&rec->data)->strs_len; + ret = EOK; + break; + default: + DEBUG(SSSDBG_FATAL_FAILURE, ("Unknown memory cache type.\n")); + ret = EINVAL; + } + + return ret; +} + static struct sss_mc_rec *sss_mc_find_record(struct sss_mc_ctx *mcc, struct sized_string *key) { @@ -364,6 +409,10 @@ static struct sss_mc_rec *sss_mc_find_record(struct sss_mc_ctx *mcc, uint32_t slot; rel_ptr_t name_ptr; char *t_key; + size_t strs_offset; + size_t strs_len; + uint8_t *max_addr; + errno_t ret; hash = sss_mc_hash(mcc, key->str, key->len); @@ -372,6 +421,14 @@ static struct sss_mc_rec *sss_mc_find_record(struct sss_mc_ctx *mcc, return NULL; } + /* Get max address of data table. */ + max_addr = mcc->data_table + mcc->dt_size; + + ret = sss_mc_get_strs_offset(mcc, &strs_offset); + if (ret != EOK) { + return NULL; + } + while (slot != MC_INVALID_VAL) { if (!MC_SLOT_WITHIN_BOUNDS(slot, mcc->dt_size)) { DEBUG(SSSDBG_FATAL_FAILURE, @@ -381,10 +438,15 @@ static struct sss_mc_rec *sss_mc_find_record(struct sss_mc_ctx *mcc, } rec = MC_SLOT_TO_PTR(mcc->data_table, slot, struct sss_mc_rec); + ret = sss_mc_get_strs_len(mcc, rec, &strs_len); + if (ret != EOK) { + return NULL; + } + 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)) { + if (key->len > strs_len + || (name_ptr + key->len) > (strs_offset + strs_len) + || (uint8_t *)rec->data + strs_offset + strs_len > max_addr) { DEBUG(SSSDBG_FATAL_FAILURE, ("Corrupted fastcache. name_ptr value is %u.\n", name_ptr)); sss_mmap_cache_reset(mcc); diff --git a/src/util/mmap_cache.h b/src/util/mmap_cache.h index abf8cac..7c6693a 100644 --- a/src/util/mmap_cache.h +++ b/src/util/mmap_cache.h @@ -113,8 +113,6 @@ struct sss_mc_rec { char data[0]; }; -/* FIXME: Function sss_mc_find_record currently relies on fact that - * offset of strs is the same in both sss_mc_pwd_data and sss_mc_grp_data. */ struct sss_mc_pwd_data { rel_ptr_t name; /* ptr to name string, rel. to struct base addr */ uint32_t uid; -- 1.7.11.2
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
