[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