On 08/14/2013 06:43 PM, Michal Židek wrote:
On 08/14/2013 04:51 PM, Simo Sorce wrote:
On Tue, 2013-08-13 at 19:42 +0200, Michal Židek wrote:
Thanks for the review Simo.
On 08/12/2013 11:07 PM, Simo Sorce wrote:
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).
Sure.
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.
Thanks.
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.
Let's look at MC_SIZE_TO_SLOTS() definition:
We always add (MC_SLOT_SIZE -1) to the requested size.
This means if you ask 1 byte you get 1 slot, if you ask for MC_SLOT_SIZE
+ 1 you get 2 slots.
Ie you get the right number of slots required for the size when you call
that macro.
So yeah good catch!
However I think I'd like to see this fixed in a different way, by using
a macro as we use this check elsewhere.
Something like:
#define MC_SLOT_WITHIN_BOUNDS(slot, size) \
(slot <= (size / MC_SLOT_SIZE))
and change the check to:
if (!MC_SLOT_WITHIN_BOUNDS(slot, mcc->dt_size)) { ...
This would be more error proof if someone should add code in future to
check bounds.
I also removed the triple check at Lukas's request in the second
patch, since it modifies the same parts already).
I would prefer this to be done in a separate patch please.
Simo.
Ok. I split the second patch into two. So now the second patch
is about the triple check and the third one about the off-by-one error.
The first patch is unchanged.
Thanks for the review
Michal
Sorry, the previous patchset included coverity issue that Lukas
mentioned. Sorry for the noise.
New patches attached.
Michal
>From ee737abe0f878a554665f81bb0bd91502755a144 Mon Sep 17 00:00:00 2001
From: Michal Zidek <[email protected]>
Date: Mon, 12 Aug 2013 19:29:56 +0200
Subject: [PATCH 1/3] 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..012ea13 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 > 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..013dd4c 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 > 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 9873b2a670c52f9bed0c490b89230a6c6d9be563 Mon Sep 17 00:00:00 2001
From: Michal Zidek <[email protected]>
Date: Wed, 14 Aug 2013 18:01:30 +0200
Subject: [PATCH 2/3] 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 012ea13..5625d6c 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 013dd4c..302043e 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 af38c011d48164a393f302006c26aae6ee7e912b Mon Sep 17 00:00:00 2001
From: Michal Zidek <[email protected]>
Date: Wed, 14 Aug 2013 18:22:06 +0200
Subject: [PATCH 3/3] 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 5625d6c..00341fb 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 302043e..ef5b34b 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..ac68a86 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
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
