>From 02fb547a95e9d02faac76ccafdc137e85d93a52c Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik Date: Thu, 30 Jul 2015 10:50:47 +0200 Subject: [PATCH] sss_client: Update integrity check of records in mmap cache The function sss_nss_mc_get_record return copy of record from memory cache in last argument. Because we should not access data directly to avoid problems with consistency of record. The function sss_nss_mc_get_record also check whether length of record is within data area (with macro MC_CHECK_RECORD_LENGTH) However we also tried to do the same check in functions sss_nss_mc_get{gr, pw}* Pointer to end of strings in record was compared to pointer to the end of data table. But these two pointers are not within the same allocated area and does not make sense to compare them. Sometimes record can be allocated before mmaped area and sometime after. Sometimes it will return cached data and other time will fall back to responder. Resolves: https://fedorahosted.org/sssd/ticket/2743 --- src/sss_client/nss_mc_group.c | 19 ++++++++++--------- src/sss_client/nss_mc_passwd.c | 20 ++++++++++---------- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/src/sss_client/nss_mc_group.c b/src/sss_client/nss_mc_group.c index e0fdb97f628ac19741409be29566e4af5a391f74..aacf59d9fd8b81ea895f4660de08f3e44f0ce645 100644 --- a/src/sss_client/nss_mc_group.c +++ b/src/sss_client/nss_mc_group.c @@ -112,16 +112,16 @@ 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; + const size_t strs_offset = offsetof(struct sss_mc_grp_data, strs); + size_t data_size; 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; + /* Get max size of data table. */ + data_size = gr_mc_ctx.dt_size; /* hashes are calculated including the NULL terminator */ hash = sss_nss_mc_hash(&gr_mc_ctx, name, name_len + 1); @@ -130,7 +130,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 (MC_SLOT_WITHIN_BOUNDS(slot, gr_mc_ctx.dt_size)) { + while (MC_SLOT_WITHIN_BOUNDS(slot, data_size)) { /* free record from previous iteration */ free(rec); rec = NULL; @@ -147,15 +147,16 @@ 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 */ + * - all strings must be within copy of record + * - size of record must be lower that data table size */ if (name_len > data->strs_len || (data->name + name_len) > (strs_offset + data->strs_len) - || (uint8_t *)data->strs + data->strs_len > max_addr) { + || data->strs_len > rec->len + || rec->len > data_size) { ret = ENOENT; goto done; } @@ -168,7 +169,7 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len, slot = sss_nss_mc_next_slot_with_hash(rec, hash); } - if (!MC_SLOT_WITHIN_BOUNDS(slot, gr_mc_ctx.dt_size)) { + if (!MC_SLOT_WITHIN_BOUNDS(slot, data_size)) { ret = ENOENT; goto done; } diff --git a/src/sss_client/nss_mc_passwd.c b/src/sss_client/nss_mc_passwd.c index 10e43e2af43c5e7f1738e281b3ed260d89f3a004..0da7ad0aeece7d38ca34bb3fde64adc898eaf0ae 100644 --- a/src/sss_client/nss_mc_passwd.c +++ b/src/sss_client/nss_mc_passwd.c @@ -105,16 +105,16 @@ 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; + const size_t strs_offset = offsetof(struct sss_mc_pwd_data, strs); + size_t data_size; 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; + /* Get max size of data table. */ + data_size = pw_mc_ctx.dt_size; /* hashes are calculated including the NULL terminator */ hash = sss_nss_mc_hash(&pw_mc_ctx, name, name_len + 1); @@ -123,7 +123,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 (MC_SLOT_WITHIN_BOUNDS(slot, pw_mc_ctx.dt_size)) { + while (MC_SLOT_WITHIN_BOUNDS(slot, data_size)) { /* free record from previous iteration */ free(rec); rec = NULL; @@ -140,16 +140,16 @@ 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 */ + * - all strings must be within copy of record + * - size of record must be lower that data table size */ if (name_len > data->strs_len || (data->name + name_len) > (strs_offset + data->strs_len) - || (uint8_t *)data->strs + data->strs_len > max_addr) { + || data->strs_len > rec->len + || rec->len > data_size) { ret = ENOENT; goto done; } @@ -162,7 +162,7 @@ errno_t sss_nss_mc_getpwnam(const char *name, size_t name_len, slot = sss_nss_mc_next_slot_with_hash(rec, hash); } - if (!MC_SLOT_WITHIN_BOUNDS(slot, pw_mc_ctx.dt_size)) { + if (!MC_SLOT_WITHIN_BOUNDS(slot, data_size)) { ret = ENOENT; goto done; } -- 2.4.6