[SSSD] [PATCH] mmap_cache: Check data->name value in client code
Lukas Slebodnik
lslebodn at redhat.com
Wed Aug 14 14:11:50 UTC 2013
On (13/08/13 19:45), Michal Židek wrote:
>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.
Nice catch.
>
>I also removed the triple check at Lukas's request in the second
>patch, since it modifies the same parts already).
Thank you
>
>New patches are attached.
>
>Thanks
>Michal
>
>+ 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;
replace return ENOENT with "ret = ENOENT; goto done", because it
introduce coverity warning:
sssd-1.10.93/src/sss_client/nss_mc_passwd.c:190: leaked_storage: Variable "rec"
going out of scope leaks the storage it points to.
And the same in src/sss_client/nss_mc_group.c
>+ }
>+
> rec_name = (char *)data + data->name;
> if (strcmp(name, rec_name) == 0) {
> break;
LS
-------------- next part --------------
>From 92a0bea3ee48fd5925ca23f4de8794693abb9954 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Wed, 14 Aug 2013 14:46:03 +0200
Subject: [PATCH] temp
---
src/sss_client/nss_mc_group.c | 3 ++-
src/sss_client/nss_mc_passwd.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/sss_client/nss_mc_group.c b/src/sss_client/nss_mc_group.c
index 549164972db4a73c04e8c7cb9e0264348bdd9568..2d56c3e38945a6c3644e7bda317f3ef801a1be55 100644
--- a/src/sss_client/nss_mc_group.c
+++ b/src/sss_client/nss_mc_group.c
@@ -147,7 +147,8 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len,
if (name_len > data->strs_len
|| data->name > strs_offset + data->strs_len
|| (uint8_t *)data->strs + data->strs_len > max_addr) {
- return ENOENT;
+ ret = ENOENT;
+ goto done;
}
rec_name = (char *)data + data->name;
diff --git a/src/sss_client/nss_mc_passwd.c b/src/sss_client/nss_mc_passwd.c
index e750a24369201f1930de1007404670a4b9654f4e..b522b916756804d4577adffa9dd0ecf06030c06a 100644
--- a/src/sss_client/nss_mc_passwd.c
+++ b/src/sss_client/nss_mc_passwd.c
@@ -149,7 +149,8 @@ errno_t sss_nss_mc_getpwnam(const char *name, size_t name_len,
if (name_len > data->strs_len
|| data->name > strs_offset + data->strs_len
|| (uint8_t *)data->strs + data->strs_len > max_addr) {
- return ENOENT;
+ ret = ENOENT;
+ goto done;
}
rec_name = (char *)data + data->name;
--
1.8.3.1
More information about the sssd-devel
mailing list