From e115f70d717ab8232172b358739ba7a0e1102caa Mon Sep 17 00:00:00 2001

From: Lukas Slebodnik <lslebodn@redhat.com>

Date: Thu, 30 Jul 2015 10:50:47 +0200

Subject: [PATCH 41/47] sss_client: Update integrity check of records in mmap

 cache

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

The function sss_nss_mc_get_record return copy of record from memory

cache in last argument. Because we should not access data directly

to avoid problems with consistency of record.

The function sss_nss_mc_get_record also check whether length of record

is within data area (with macro MC_CHECK_RECORD_LENGTH)

However we also tried to do the same check in functions sss_nss_mc_get{gr, pw}*

Pointer to end of strings in record was compared to pointer to the end

of data table. But these two pointers are not within the same allocated area

and does not make sense to compare them. Sometimes record can be allocated

before mmaped area and sometime after. Sometimes it will return cached data

and other time will fall back to responder.

Resolves:

https://fedorahosted.org/sssd/ticket/2743

Reviewed-by: Michal Židek <mzidek@redhat.com>

(cherry picked from commit ba847347cade817ee927397d82c952b51b0dcb2b)

---

 src/sss_client/nss_mc_group.c  | 19 ++++++++++---------

 src/sss_client/nss_mc_initgr.c | 26 +++++++++++++-------------

 src/sss_client/nss_mc_passwd.c | 20 ++++++++++----------

 3 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/src/sss_client/nss_mc_group.c b/src/sss_client/nss_mc_group.c

index e0fdb97f628ac19741409be29566e4af5a391f74..aacf59d9fd8b81ea895f4660de08f3e44f0ce645 100644

--- a/src/sss_client/nss_mc_group.c

+++ b/src/sss_client/nss_mc_group.c

@@ -112,16 +112,16 @@ 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;

+    const size_t strs_offset = offsetof(struct sss_mc_grp_data, strs);

+    size_t data_size;

     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;

+    /* Get max size of data table. */

+    data_size = gr_mc_ctx.dt_size;

     /* hashes are calculated including the NULL terminator */

     hash = sss_nss_mc_hash(&gr_mc_ctx, name, name_len + 1);

@@ -130,7 +130,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 (MC_SLOT_WITHIN_BOUNDS(slot, gr_mc_ctx.dt_size)) {

+    while (MC_SLOT_WITHIN_BOUNDS(slot, data_size)) {

         /* free record from previous iteration */

         free(rec);

         rec = NULL;

@@ -147,15 +147,16 @@ 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 */

+         * - 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)

-            || (uint8_t *)data->strs + data->strs_len > max_addr) {

+            || data->strs_len > rec->len

+            || rec->len > data_size) {

             ret = ENOENT;

             goto done;

         }

@@ -168,7 +169,7 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len,

         slot = sss_nss_mc_next_slot_with_hash(rec, hash);

     }

-    if (!MC_SLOT_WITHIN_BOUNDS(slot, gr_mc_ctx.dt_size)) {

+    if (!MC_SLOT_WITHIN_BOUNDS(slot, data_size)) {

         ret = ENOENT;

         goto done;

     }

diff --git a/src/sss_client/nss_mc_initgr.c b/src/sss_client/nss_mc_initgr.c

index 153617ea9c6489b7439b9676904b42b042f6697c..74143d9fb3c674c3116d7f4cf0b4c03d993743a2 100644

--- a/src/sss_client/nss_mc_initgr.c

+++ b/src/sss_client/nss_mc_initgr.c

@@ -94,15 +94,15 @@ errno_t sss_nss_mc_initgroups_dyn(const char *name, size_t name_len,

     uint32_t slot;

     int ret;

     const size_t data_offset = offsetof(struct sss_mc_initgr_data, gids);

-    uint8_t *max_addr;

+    size_t data_size;

     ret = sss_nss_mc_get_ctx("initgroups", &initgr_mc_ctx);

     if (ret) {

         return ret;

     }

-    /* Get max address of data table. */

-    max_addr = initgr_mc_ctx.data_table + initgr_mc_ctx.dt_size;

+    /* Get max size of data table. */

+    data_size = initgr_mc_ctx.dt_size;

     /* hashes are calculated including the NULL terminator */

     hash = sss_nss_mc_hash(&initgr_mc_ctx, name, name_len + 1);

@@ -111,7 +111,7 @@ errno_t sss_nss_mc_initgroups_dyn(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 (MC_SLOT_WITHIN_BOUNDS(slot, initgr_mc_ctx.dt_size)) {

+    while (MC_SLOT_WITHIN_BOUNDS(slot, data_size)) {

         /* free record from previous iteration */

         free(rec);

         rec = NULL;

@@ -132,14 +132,14 @@ errno_t sss_nss_mc_initgroups_dyn(const char *name, size_t name_len,

         rec_name = (char *)data + data->name;

         /* Integrity check

          * - name_len cannot be longer than all strings or data

-         * - data->name cannot point outside strings

-         * - all data must be within data_table

-         * - name must be within data_table */

-        if (name_len > data->data_len

-            || name_len > data->strs_len

-            || (data->strs + name_len) > (data_offset + data->data_len)

-            || (uint8_t *)data->gids + data->data_len > max_addr

-            || (uint8_t *)rec_name + name_len > max_addr) {

+         * - 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_len > data->data_len

+            || data->data_len > rec->len

+            || rec->len > data_size

+            || (data->strs + name_len) > (data_offset + data->data_len)) {

             ret = ENOENT;

             goto done;

         }

@@ -151,7 +151,7 @@ errno_t sss_nss_mc_initgroups_dyn(const char *name, size_t name_len,

         slot = sss_nss_mc_next_slot_with_hash(rec, hash);

     }

-    if (!MC_SLOT_WITHIN_BOUNDS(slot, initgr_mc_ctx.dt_size)) {

+    if (!MC_SLOT_WITHIN_BOUNDS(slot, 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 10e43e2af43c5e7f1738e281b3ed260d89f3a004..0da7ad0aeece7d38ca34bb3fde64adc898eaf0ae 100644

--- a/src/sss_client/nss_mc_passwd.c

+++ b/src/sss_client/nss_mc_passwd.c

@@ -105,16 +105,16 @@ 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;

+    const size_t strs_offset = offsetof(struct sss_mc_pwd_data, strs);

+    size_t data_size;

     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;

+    /* Get max size of data table. */

+    data_size = pw_mc_ctx.dt_size;

     /* hashes are calculated including the NULL terminator */

     hash = sss_nss_mc_hash(&pw_mc_ctx, name, name_len + 1);

@@ -123,7 +123,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 (MC_SLOT_WITHIN_BOUNDS(slot, pw_mc_ctx.dt_size)) {

+    while (MC_SLOT_WITHIN_BOUNDS(slot, data_size)) {

         /* free record from previous iteration */

         free(rec);

         rec = NULL;

@@ -140,16 +140,16 @@ 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 */

+         * - 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)

-            || (uint8_t *)data->strs + data->strs_len > max_addr) {

+            || data->strs_len > rec->len

+            || rec->len > data_size) {

             ret = ENOENT;

             goto done;

         }

@@ -162,7 +162,7 @@ errno_t sss_nss_mc_getpwnam(const char *name, size_t name_len,

         slot = sss_nss_mc_next_slot_with_hash(rec, hash);

     }

-    if (!MC_SLOT_WITHIN_BOUNDS(slot, pw_mc_ctx.dt_size)) {

+    if (!MC_SLOT_WITHIN_BOUNDS(slot, data_size)) {

         ret = ENOENT;

         goto done;

     }

-- 

2.4.3