Thanks for the review Simo.
On 08/12/2013 11:07 PM, Simo Sorce wrote:
On Mon, 2013-08-12 at 19:56 +0200, Michal Židek wrote:
This should have been part of the patch:
[PATCH] mmap_cache: Check if slot and name_ptr are not invalid.
I missed the fact that name_ptr is called just name in the
client code and did not add check there.
Using manually corrupted cache (setting name_ptr to some high value
in hexeditor) this caused segfault in the client code. The attached
patch fixes this.
Please review and push this patch to master, 1-9 and 1-10.
I do not think this patch is correct.
Although normally data->name will point to data->strs it is a separate
pointer exactly becsaue the server code may decide to put the name
string somewhere in the strs buffer but not necessarily at the start.
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).
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.
Simo.
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.
I also removed the triple check at Lukas's request in the second
patch, since it modifies the same parts already).
New patches are attached.
Thanks
Michal
>From 94b584bfed799ed3e9949a4dbac7add614f38f56 Mon Sep 17 00:00:00 2001
From: Michal Zidek <[email protected]>
Date: Mon, 12 Aug 2013 19:29:56 +0200
Subject: [PATCH 1/2] 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 | 17 +++++++++++++++++
src/sss_client/nss_mc_passwd.c | 18 ++++++++++++++++++
2 files changed, 35 insertions(+)
diff --git a/src/sss_client/nss_mc_group.c b/src/sss_client/nss_mc_group.c
index 2d69be9..3764c9b 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,18 @@ 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 > strs_offset + data->strs_len
+ || (uint8_t *)data->strs + data->strs_len > max_addr) {
+ return ENOENT;
+ }
+
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..b29d33b 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,19 @@ 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 > strs_offset + data->strs_len
+ || (uint8_t *)data->strs + data->strs_len > max_addr) {
+ return ENOENT;
+ }
+
rec_name = (char *)data + data->name;
if (strcmp(name, rec_name) == 0) {
break;
--
1.7.11.2
>From 22b9cca2d678f9e3b7870faec2ca066211896b71 Mon Sep 17 00:00:00 2001
From: Michal Zidek <[email protected]>
Date: Tue, 13 Aug 2013 19:27:21 +0200
Subject: [PATCH 2/2] mmap_cache: Of by one error and triple check removal.
Removes off by one error when using macro MC_SIZE_TO_SLOTS and
also reduces the number of checks in client code, where the
slot number validity is checked.
---
src/responder/nss/nsssrv_mmap_cache.c | 12 ++++++------
src/sss_client/nss_mc_group.c | 34 ++++++++++++++++------------------
src/sss_client/nss_mc_passwd.c | 34 ++++++++++++++++------------------
3 files changed, 38 insertions(+), 42 deletions(-)
diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c
index cd5a643..a7e9049 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 (slot >= MC_SIZE_TO_SLOTS(mcc->dt_size)) {
return NULL;
}
while (slot != MC_INVALID_VAL) {
- if (slot > MC_SIZE_TO_SLOTS(mcc->dt_size)) {
+ if (slot >= MC_SIZE_TO_SLOTS(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 (slot >= MC_SIZE_TO_SLOTS(mcc->dt_size)) {
ret = ENOENT;
goto done;
}
while (slot != MC_INVALID_VAL) {
- if (slot > MC_SIZE_TO_SLOTS(mcc->dt_size)) {
+ if (slot >= MC_SIZE_TO_SLOTS(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 (slot >= MC_SIZE_TO_SLOTS(mcc->dt_size)) {
ret = ENOENT;
goto done;
}
while (slot != MC_INVALID_VAL) {
- if (slot > MC_SIZE_TO_SLOTS(mcc->dt_size)) {
+ if (slot >= MC_SIZE_TO_SLOTS(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 3764c9b..5491649 100644
--- a/src/sss_client/nss_mc_group.c
+++ b/src/sss_client/nss_mc_group.c
@@ -105,6 +105,7 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len,
int ret;
size_t strs_offset;
uint8_t *max_addr;
+ size_t max_slots;
ret = sss_nss_mc_get_ctx("group", &gr_mc_ctx);
if (ret) {
@@ -117,16 +118,14 @@ 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;
- }
+ max_slots = MC_SIZE_TO_SLOTS(gr_mc_ctx.dt_size);
+ /* The iteration should stop at positive match or if
+ * slot == MC_INVALID_VAL (which is bigger than max_slots).
+ * If the iter. ends with other slot value, the mmap cache
+ * is probbably corrupted. */
+ while (slot < max_slots) {
ret = sss_nss_mc_get_record(&gr_mc_ctx, slot, &rec);
if (ret) {
goto done;
@@ -159,7 +158,7 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len,
slot = rec->next;
}
- if (slot == MC_INVALID_VAL) {
+ if (slot >= max_slots) {
ret = ENOENT;
goto done;
}
@@ -182,6 +181,7 @@ errno_t sss_nss_mc_getgrgid(gid_t gid,
uint32_t slot;
int len;
int ret;
+ size_t max_slots;
ret = sss_nss_mc_get_ctx("group", &gr_mc_ctx);
if (ret) {
@@ -196,16 +196,14 @@ 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;
- }
+ max_slots = MC_SIZE_TO_SLOTS(gr_mc_ctx.dt_size);
+ /* The iteration should stop at positive match or if
+ * slot == MC_INVALID_VAL (which is bigger than max_slots).
+ * If the iter. ends with other slot value, the mmap cache
+ * is probbably corrupted. */
+ while (slot < max_slots) {
ret = sss_nss_mc_get_record(&gr_mc_ctx, slot, &rec);
if (ret) {
goto done;
@@ -226,7 +224,7 @@ errno_t sss_nss_mc_getgrgid(gid_t gid,
slot = rec->next;
}
- if (slot == MC_INVALID_VAL) {
+ if (slot >= max_slots) {
ret = ENOENT;
goto done;
}
diff --git a/src/sss_client/nss_mc_passwd.c b/src/sss_client/nss_mc_passwd.c
index b29d33b..e750a24 100644
--- a/src/sss_client/nss_mc_passwd.c
+++ b/src/sss_client/nss_mc_passwd.c
@@ -106,6 +106,7 @@ errno_t sss_nss_mc_getpwnam(const char *name, size_t name_len,
int ret;
size_t strs_offset;
uint8_t *max_addr;
+ size_t max_slots;
ret = sss_nss_mc_get_ctx("passwd", &pw_mc_ctx);
if (ret) {
@@ -118,16 +119,14 @@ 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;
- }
+ max_slots = MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size);
+ /* The iteration should stop at positive match or if
+ * slot == MC_INVALID_VAL (which is bigger than max_slots).
+ * If the iter. ends with other slot value, the mmap cache
+ * is probbably corrupted. */
+ while (slot < max_slots) {
ret = sss_nss_mc_get_record(&pw_mc_ctx, slot, &rec);
if (ret) {
goto done;
@@ -161,7 +160,7 @@ errno_t sss_nss_mc_getpwnam(const char *name, size_t name_len,
slot = rec->next;
}
- if (slot == MC_INVALID_VAL) {
+ if (slot >= max_slots) {
ret = ENOENT;
goto done;
}
@@ -184,6 +183,7 @@ errno_t sss_nss_mc_getpwuid(uid_t uid,
uint32_t slot;
int len;
int ret;
+ size_t max_slots;
ret = sss_nss_mc_get_ctx("passwd", &pw_mc_ctx);
if (ret) {
@@ -198,16 +198,14 @@ 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;
- }
+ max_slots = MC_SIZE_TO_SLOTS(pw_mc_ctx.dt_size);
+ /* The iteration should stop at positive match or if
+ * slot == MC_INVALID_VAL (which is bigger than max_slots).
+ * If the iter. ends with other slot value, the mmap cache
+ * is probbably corrupted. */
+ while (slot < max_slots) {
ret = sss_nss_mc_get_record(&pw_mc_ctx, slot, &rec);
if (ret) {
goto done;
@@ -228,7 +226,7 @@ errno_t sss_nss_mc_getpwuid(uid_t uid,
slot = rec->next;
}
- if (slot == MC_INVALID_VAL) {
+ if (slot >= max_slots) {
ret = ENOENT;
goto done;
}
--
1.7.11.2
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
