sssd/0064-mmap_cache-make-checks-independent-of-input-size.patch

From b70b4099b049b6a2bd85e773dbd81974dee24e05 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose@redhat.com>
Date: Fri, 17 Nov 2017 10:51:44 +0100
Subject: [PATCH 64/79] mmap_cache: make checks independent of input size
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently the consistency checks for the mmap_cache payload data on the
client and the responder side include the length of the input string of
the current request. Since there might be hash collisions which other
much longer or much shorter names those checks might fail although there
is no data corruption.

This patch removes the checks using the length of the input and adds a
check if the name found in the payload is zero-terminated inside of the
payload data.

Resolves https://pagure.io/SSSD/sssd/issue/3571

Reviewed-by: Michal Židek <mzidek@redhat.com>
Reviewed-by: Lukáš Slebodník <lslebodn@redhat.com>
---
 src/responder/nss/nsssrv_mmap_cache.c | 34 ++++++++++++++++++++++++----------
 src/sss_client/nss_mc_group.c         | 12 ++++++------
 src/sss_client/nss_mc_initgr.c        | 12 +++++++-----
 src/sss_client/nss_mc_passwd.c        | 12 ++++++------
 4 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c
index a87ad646f9b741db3eb18680678697032fc420ba..ad5adbce15e50c065d4d16e626be97fd23d06643 100644
--- a/src/responder/nss/nsssrv_mmap_cache.c
+++ b/src/responder/nss/nsssrv_mmap_cache.c
@@ -547,18 +547,32 @@ static struct sss_mc_rec *sss_mc_find_record(struct sss_mc_ctx *mcc,
             return NULL;
         }
 
+        if (key->len > strs_len) {
+            /* The string cannot be in current record */
+            slot = sss_mc_next_slot_with_hash(rec, hash);
+            continue;
+        }
+
         safealign_memcpy(&name_ptr, rec->data, sizeof(rel_ptr_t), NULL);
-        if (key->len > strs_len
-            || (name_ptr + key->len) > (strs_offset + strs_len)
-            || (uint8_t *)rec->data + strs_offset + strs_len > max_addr) {
-            DEBUG(SSSDBG_FATAL_FAILURE,
-                  "Corrupted fastcache. name_ptr value is %u.\n", name_ptr);
-            sss_mc_save_corrupted(mcc);
-            sss_mmap_cache_reset(mcc);
-            return NULL;
-        }
-
         t_key = (char *)rec->data + name_ptr;
+        /* name_ptr must point to some data in the strs/gids area of the data
+         * payload. Since it is a pointer relative to rec->data it must larger
+         * equal strs_offset and must be smaller then strs_offset + strs_len.
+         * Additionally the area must not end outside of the data table and
+         * t_key must be a zero-terminates string. */
+        if (name_ptr < strs_offset
+                || name_ptr >= strs_offset + strs_len
+                || (uint8_t *)rec->data > max_addr
+                || strs_offset > max_addr - (uint8_t *)rec->data
+                || strs_len > max_addr - (uint8_t *)rec->data - strs_offset) {
+            DEBUG(SSSDBG_FATAL_FAILURE,
+                  "Corrupted fastcache entry at slot %u. "
+                  "name_ptr value is %u.\n", slot, name_ptr);
+            sss_mc_save_corrupted(mcc);
+            sss_mmap_cache_reset(mcc);
+            return NULL;
+        }
+
         if (strcmp(key->str, t_key) == 0) {
             break;
         }
diff --git a/src/sss_client/nss_mc_group.c b/src/sss_client/nss_mc_group.c
index ce88d42fdaf4f19e78fc43e187bc28651cdc3c4e..ba582fe55cf3abf90d8e016c82a0bee48608ce78 100644
--- a/src/sss_client/nss_mc_group.c
+++ b/src/sss_client/nss_mc_group.c
@@ -148,20 +148,20 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len,
         }
 
         data = (struct sss_mc_grp_data *)rec->data;
+        rec_name = (char *)data + data->name;
         /* Integrity check
-         * - name_len cannot be longer than all strings
          * - data->name cannot point outside strings
          * - 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)
+         * - record must not end outside data table
+         * - rec_name is a zero-terminated string */
+        if (data->name < strs_offset
+            || data->name >= strs_offset + data->strs_len
             || data->strs_len > rec->len
-            || rec->len > data_size) {
+            || (uint8_t *) rec + rec->len > gr_mc_ctx.data_table + data_size ) {
             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_initgr.c b/src/sss_client/nss_mc_initgr.c
index a77088d849ad3601cb3edb55fc5ea4ae4c52fe38..606f1c7ee2526a15378831d4512e943bac214d0e 100644
--- a/src/sss_client/nss_mc_initgr.c
+++ b/src/sss_client/nss_mc_initgr.c
@@ -131,15 +131,17 @@ errno_t sss_nss_mc_initgroups_dyn(const char *name, size_t name_len,
         data = (struct sss_mc_initgr_data *)rec->data;
         rec_name = (char *)data + data->name;
         /* Integrity check
-         * - name_len cannot be longer than all strings or data
+         * - data->name cannot point outside all strings or data
          * - all data must be within copy of record
          * - size of record must be lower that data table size
-         * - data->strs cannot point outside strings */
-        if (name_len > data->strs_len
+         * - data->strs cannot point outside strings
+         * - rec_name is a zero-terminated string */
+        if (data->name < data_offset
+            || data->name >= data_offset + data->data_len
             || data->strs_len > data->data_len
             || data->data_len > rec->len
-            || rec->len > data_size
-            || (data->strs + name_len) > (data_offset + data->data_len)) {
+            || (uint8_t *) rec + rec->len
+                                       > initgr_mc_ctx.data_table + 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 0da7ad0aeece7d38ca34bb3fde64adc898eaf0ae..0bc1237446d3691c8c83aa0fc0cf692d4b336f9e 100644
--- a/src/sss_client/nss_mc_passwd.c
+++ b/src/sss_client/nss_mc_passwd.c
@@ -141,20 +141,20 @@ errno_t sss_nss_mc_getpwnam(const char *name, size_t name_len,
         }
 
         data = (struct sss_mc_pwd_data *)rec->data;
+        rec_name = (char *)data + data->name;
         /* Integrity check
-         * - name_len cannot be longer than all strings
          * - data->name cannot point outside strings
          * - 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)
+         * - record must not end outside data table
+         * - rec_name is a zero-terminated string */
+        if (data->name < strs_offset
+            || data->name >= strs_offset + data->strs_len
             || data->strs_len > rec->len
-            || rec->len > data_size) {
+            || (uint8_t *) rec + rec->len > pw_mc_ctx.data_table + data_size ) {
             ret = ENOENT;
             goto done;
         }
 
-        rec_name = (char *)data + data->name;
         if (strcmp(name, rec_name) == 0) {
             break;
         }
-- 
2.15.1