[SSSD] [sssd PR#424][+Changes requested] TOOLS: Add a new sssctl command access-report

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/424
Title: #424: TOOLS: Add a new sssctl command access-report

Label: +Changes requested
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#424][comment] TOOLS: Add a new sssctl command access-report

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/424
Title: #424: TOOLS: Add a new sssctl command access-report

jhrozek commented:
"""
Self-nack, coverity found some issues:
```
Error: UNINIT (CWE-457):
sssd-1.16.1/src/providers/ipa/ipa_access.c:714: var_decl: Declaring variable 
"ret" without initializer.
sssd-1.16.1/src/providers/ipa/ipa_access.c:727: uninit_use_in_call: Using 
uninitialized value "ret" when calling "_tevent_req_error".
#  725|   subreq = ipa_fetch_hbac_send(state, params->ev, params->be_ctx, 
access_ctx);
#  726|   if (subreq == NULL) {
#  727|-> tevent_req_error(req, ret);
#  728|   tevent_req_post(req, params->ev);
#  729|   return req;

Error: UNINIT (CWE-457):
sssd-1.16.1/src/tools/sssctl/sssctl_access_report.c:277: var_decl: Declaring 
variable "ret" without initializer.
sssd-1.16.1/src/tools/sssctl/sssctl_access_report.c:315: uninit_use: Using 
uninitialized value "ret".
#  313|   done:
#  314|   talloc_free(tmp_ctx);
#  315|-> return ret;
#  316|   }
#  317|   

Error: COMPILER_WARNING:
sssd-1.16.1/src/tools/sssctl/sssctl_access_report.c: scope_hint: In function 
'sssctl_ipa_access_report'
sssd-1.16.1/src/tools/sssctl/sssctl_access_report.c:340:8: warning: 'ret' may 
be used uninitialized in this function [-Wmaybe-uninitialized]
# if (ret != EOK) {
#^
#  338|   /* Run the pam account phase to make sure the rules are fetched 
by SSSD */
#  339|   ret = refresh_hbac_rules(tool_ctx, domain);
#  340|-> if (ret != EOK) {
#  341|   ERROR("Unable to refresh HBAC rules, using cached content\n");
#  342|   /* Non-fatal */
```
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/424#issuecomment-345361656
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/455
Title: #455: mmap_cache: make checks independent of input size

jhrozek commented:
"""
Since @mzidek-rh was already looking into the issue earlier, I hope Michal can 
help with the review as well.
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/455#issuecomment-345282431
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#455][opened] mmap_cache: make checks independent of input size

[sumit-bose]

   URL: https://github.com/SSSD/sssd/pull/455
Author: sumit-bose
 Title: #455: mmap_cache: make checks independent of input size
Action: opened

PR body:
"""
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
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/455/head:pr455
git checkout pr455
From 67918c340961d3f2da61ca50d948149ab4d3ef83 Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Fri, 17 Nov 2017 10:51:44 +0100
Subject: [PATCH] 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..5c8748483 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(_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 realtive 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..dd925a4e3 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;