URL: https://github.com/SSSD/sssd/pull/456
Author: fidencio
 Title: #456: Fix the covscan warning that PR #455 triggered
Action: opened

PR body:
"""
This patch must be applied atop of PR #455!

@sumit-bose, if you prefer, feel free to add this patch to PR #455 and close 
this one.
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/456/head:pr456
git checkout pr456
From 9e9b65c644422e19ae4ac52296d114ac834af5c1 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@redhat.com>
Date: Fri, 17 Nov 2017 10:51:44 +0100
Subject: [PATCH 1/2] mmap_cache: make checks independent of input size

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
---
 src/responder/nss/nsssrv_mmap_cache.c | 18 +++++++++++++-----
 src/sss_client/nss_mc_group.c         | 14 ++++++++------
 src/sss_client/nss_mc_initgr.c        | 14 +++++++++-----
 src/sss_client/nss_mc_passwd.c        | 14 ++++++++------
 4 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c
index a87ad646f..3441c79c9 100644
--- a/src/responder/nss/nsssrv_mmap_cache.c
+++ b/src/responder/nss/nsssrv_mmap_cache.c
@@ -548,17 +548,25 @@ static struct sss_mc_rec *sss_mc_find_record(struct sss_mc_ctx *mcc,
         }
 
         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) {
+        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 + strs_offset + strs_len > max_addr
+                || memchr(t_key, '\0',
+                          (strs_offset + strs_len) - name_ptr) == NULL ) {
             DEBUG(SSSDBG_FATAL_FAILURE,
-                  "Corrupted fastcache. name_ptr value is %u.\n", name_ptr);
+                  "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;
         }
 
-        t_key = (char *)rec->data + name_ptr;
         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 ce88d42fd..4b1601a17 100644
--- a/src/sss_client/nss_mc_group.c
+++ b/src/sss_client/nss_mc_group.c
@@ -148,20 +148,22 @@ 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
+            || memchr(rec_name, '\0',
+                      (strs_offset + data->strs_len) - data->name) == NULL) {
             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 a77088d84..d8c01f52e 100644
--- a/src/sss_client/nss_mc_initgr.c
+++ b/src/sss_client/nss_mc_initgr.c
@@ -131,15 +131,19 @@ 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
+            || memchr(rec_name, '\0',
+                      (data_offset + data->data_len) - data->name) == NULL) {
             ret = ENOENT;
             goto done;
         }
diff --git a/src/sss_client/nss_mc_passwd.c b/src/sss_client/nss_mc_passwd.c
index 0da7ad0ae..868427f03 100644
--- a/src/sss_client/nss_mc_passwd.c
+++ b/src/sss_client/nss_mc_passwd.c
@@ -141,20 +141,22 @@ 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
+            || memchr(rec_name, '\0',
+                      (strs_offset + data->strs_len) - data->name) == NULL ) {
             ret = ENOENT;
             goto done;
         }
 
-        rec_name = (char *)data + data->name;
         if (strcmp(name, rec_name) == 0) {
             break;
         }

From 5a846435514bfe994583fb1092afeff76c5a88aa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com>
Date: Tue, 21 Nov 2017 16:12:24 +0100
Subject: [PATCH 2/2] NSS: Fix covscan warning
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

 Error: NULL_RETURNS (CWE-476): [#def1]
 sssd-1.16.1/src/responder/nss/nss_protocol.c:162: returned_null: "memchr" returns null (checked 7 out of 8 times).
 sssd-1.16.1/src/responder/nss/nsssrv_mmap_cache.c:557: example_checked: Example 1: "memchr(t_key, 0, strs_offset + strs_len - name_ptr)" has its value checked in "memchr(t_key, 0, strs_offset + strs_len - name_ptr) == NULL".
 sssd-1.16.1/src/sss_client/idmap/sss_nss_idmap.c:171: example_assign: Example 2: Assigning: "p" = return value from "memchr(p, 0, buf_len - (p - buf))".
 sssd-1.16.1/src/sss_client/idmap/sss_nss_idmap.c:172: example_checked: Example 2 (cont.): "p" has its value checked in "p == NULL".
 sssd-1.16.1/src/sss_client/nss_mc_group.c:157: example_checked: Example 3: "memchr(rec_name, 0, 16UL + data->strs_len - data->name)" has its value checked in "memchr(rec_name, 0, 16UL + data->strs_len - data->name) == NULL".
 sssd-1.16.1/src/sss_client/nss_mc_initgr.c:139: example_checked: Example 4: "memchr(rec_name, 0, 24UL + data->data_len - data->name)" has its value checked in "memchr(rec_name, 0, 24UL + data->data_len - data->name) == NULL".
 sssd-1.16.1/src/sss_client/nss_mc_passwd.c:150: example_checked: Example 5: "memchr(rec_name, 0, 16UL + data->strs_len - data->name)" has its value checked in "memchr(rec_name, 0, 16UL + data->strs_len - data->name) == NULL".
 sssd-1.16.1/src/responder/nss/nss_protocol.c:162: var_assigned: Assigning: "p" = null return value from "memchr".
 sssd-1.16.1/src/responder/nss/nss_protocol.c:176: dereference: Incrementing a pointer which might be null: "p".
 #  174|       }
 #  175|
 #  176|->     p++;
 #  177|       if ((p - body) + sizeof(uint32_t) != blen) {
 #  178|           DEBUG(SSSDBG_CRIT_FAILURE, "Body has unexpected size!\n");

Signed-off-by: Fabiano FidĂȘncio <fiden...@redhat.com>
---
 src/responder/nss/nss_protocol.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/responder/nss/nss_protocol.c b/src/responder/nss/nss_protocol.c
index 265538649..13f6d1541 100644
--- a/src/responder/nss/nss_protocol.c
+++ b/src/responder/nss/nss_protocol.c
@@ -160,6 +160,13 @@ nss_protocol_parse_name_ex(struct cli_ctx *cli_ctx, const char **_rawname,
     }
 
     p = memchr(body, '\0', blen);
+    /* Although body for sure is null terminated, let's add this check here
+     * so static analyzers are happier. */
+    if (p == NULL) {
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              "memchr() returned NULL, body is not null terminated!\n");
+        return EINVAL;
+    }
 
     /* If the body isn't valid UTF-8, fail */
     if (!sss_utf8_check(body, (p - body))) {
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

Reply via email to