>From 111d10375bb678ce564114dbd6b77946aeb6d5f5 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik Date: Thu, 30 Jul 2015 10:50:47 +0200 Subject: [PATCH 3/7] 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. --- src/sss_client/nss_mc_group.c | 19 ++++++++++--------- src/sss_client/nss_mc_initgr.c | 26 +++++++++++++------------- src/sss_client/nss_mc_passwd.c | 20 ++++++++++---------- 3 files changed, 33 insertions(+), 32 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_initgr.c b/src/sss_client/nss_mc_initgr.c index 153617ea9c6489b7439b9676904b42b042f6697c..74143d9fb3c674c3116d7f4cf0b4c03d993743a2 100644 --- a/src/sss_client/nss_mc_initgr.c +++ b/src/sss_client/nss_mc_initgr.c @@ -94,15 +94,15 @@ errno_t sss_nss_mc_initgroups_dyn(const char *name, size_t name_len, uint32_t slot; int ret; const size_t data_offset = offsetof(struct sss_mc_initgr_data, gids); - uint8_t *max_addr; + size_t data_size; ret = sss_nss_mc_get_ctx("initgroups", &initgr_mc_ctx); if (ret) { return ret; } - /* Get max address of data table. */ - max_addr = initgr_mc_ctx.data_table + initgr_mc_ctx.dt_size; + /* Get max size of data table. */ + data_size = initgr_mc_ctx.dt_size; /* hashes are calculated including the NULL terminator */ hash = sss_nss_mc_hash(&initgr_mc_ctx, name, name_len + 1); @@ -111,7 +111,7 @@ errno_t sss_nss_mc_initgroups_dyn(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, initgr_mc_ctx.dt_size)) { + while (MC_SLOT_WITHIN_BOUNDS(slot, data_size)) { /* free record from previous iteration */ free(rec); rec = NULL; @@ -132,14 +132,14 @@ errno_t sss_nss_mc_initgroups_dyn(const char *name, size_t name_len, rec_name = (char *)data + data->name; /* Integrity check * - name_len cannot be longer than all strings or data - * - data->name cannot point outside strings - * - all data must be within data_table - * - name must be within data_table */ - if (name_len > data->data_len - || name_len > data->strs_len - || (data->strs + name_len) > (data_offset + data->data_len) - || (uint8_t *)data->gids + data->data_len > max_addr - || (uint8_t *)rec_name + name_len > max_addr) { + * - all data must be within copy of record + * - size of record must be lower that data table size + * - data->strs cannot point outside strings */ + if (name_len > data->strs_len + || data->strs_len > data->data_len + || data->data_len > rec->len + || rec->len > data_size + || (data->strs + name_len) > (data_offset + data->data_len)) { ret = ENOENT; goto done; } @@ -151,7 +151,7 @@ errno_t sss_nss_mc_initgroups_dyn(const char *name, size_t name_len, slot = sss_nss_mc_next_slot_with_hash(rec, hash); } - if (!MC_SLOT_WITHIN_BOUNDS(slot, initgr_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