From 0cd0887dc253527f51ed9b2eabe6229e9eb64705 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn@redhat.com>
Date: Thu, 30 Jul 2015 10:50:47 +0200
Subject: [PATCH 02/21] sss_client: Update integrity check of records in mmap
cache
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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
Reviewed-by: Michal Židek <mzidek@redhat.com>
---
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 e0fdb97..aacf59d 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 10e43e2..0da7ad0 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.5.0