URL: https://github.com/SSSD/sssd/pull/5319
Author: alexey-tikhonov
Title: #5319: Covscan cleanup
Action: opened
PR body:
"""
"""
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5319/head:pr5319
git checkout pr5319
From a07676cf85f4721bb5d7bb59fe5ffcd53788e6ea Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Tue, 15 Sep 2020 20:00:26 +0200
Subject: [PATCH 1/6] PAM responder: fixed compliantion warning
Fixed following warning:
```
Error: CLANG_WARNING:
sssd-2.3.2/src/responder/pam/pamsrv_cmd.c:982:9: warning: Access to field 'cache_credentials' results in a dereference of a null pointer (loaded from field 'domain')
# preq->domain->cache_credentials &&
# ^ ~~~~~~
```
---
src/responder/pam/pamsrv_cmd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index ba8a1b848a..1d02514979 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -979,6 +979,7 @@ static void pam_reply(struct pam_auth_req *preq)
/* If this was a successful login, save the lastLogin time */
if (pd->cmd == SSS_PAM_AUTHENTICATE &&
pd->pam_status == PAM_SUCCESS &&
+ preq->domain &&
preq->domain->cache_credentials &&
!pd->offline_auth &&
!pd->last_auth_saved &&
From 0e01def7b7414b8f0947126ce761e405a9b4b4e0 Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Wed, 16 Sep 2020 11:20:01 +0200
Subject: [PATCH 2/6] KCM: supress false positive cppcheck warnings
Supress a bunch of warnings like this:
```
Error: CPPCHECK_WARNING (CWE-456):
sssd-2.3.2/src/responder/kcm/kcmsrv_ccache_json.c:154: error[uninitvar]: Uninitialized variable: key_uuid
# 152| uuid_t key_uuid;
# 153|
# 154|-> ret = sec_key_get_uuid(sec_key, key_uuid);
# 155| if (ret != EOK) {
# 156| DEBUG(SSSDBG_MINOR_FAILURE, "Cannot convert key to UUID\n");
```
Those are clearly false positives as in all those places `uuid` is output arg and
isn't read in following execution flow. "cppcheck" fails to detect this because
`uuid_t` and uuid_parse()/uuid_copy() are opaquie for analyzer.
There is no sane way to initialize uuid_t in a way that would please cppcheck.
Moreover, it doesn't make sense to do so from performance point of view.
Hence suppression.
---
src/responder/kcm/kcmsrv_ccache_json.c | 4 ++++
src/responder/kcm/kcmsrv_ops.c | 24 ++++++++++++++++++++++++
2 files changed, 28 insertions(+)
diff --git a/src/responder/kcm/kcmsrv_ccache_json.c b/src/responder/kcm/kcmsrv_ccache_json.c
index 72e24c4304..3e2303fe47 100644
--- a/src/responder/kcm/kcmsrv_ccache_json.c
+++ b/src/responder/kcm/kcmsrv_ccache_json.c
@@ -151,6 +151,10 @@ bool sec_key_match_uuid(const char *sec_key,
errno_t ret;
uuid_t key_uuid;
+ /* `key_uuid` is output arg and isn't read in sec_key_get_uuid() but
+ * since libuuid is opaquie for cppcheck it generates false positive here
+ */
+ /* cppcheck-suppress uninitvar */
ret = sec_key_get_uuid(sec_key, key_uuid);
if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE, "Cannot convert key to UUID\n");
diff --git a/src/responder/kcm/kcmsrv_ops.c b/src/responder/kcm/kcmsrv_ops.c
index 1fc21453eb..5d890e9f4a 100644
--- a/src/responder/kcm/kcmsrv_ops.c
+++ b/src/responder/kcm/kcmsrv_ops.c
@@ -468,6 +468,10 @@ static void kcm_op_initialize_got_byname(struct tevent_req *subreq)
return;
}
+ /* `uuid` is output arg and isn't read in kcm_cc_get_uuid() but
+ * since libuuid is opaquie for cppcheck it generates false positive here
+ */
+ /* cppcheck-suppress uninitvar */
ret = kcm_cc_get_uuid(state->new_cc, uuid);
if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE,
@@ -528,6 +532,10 @@ static void kcm_op_initialize_fill_princ_step(struct tevent_req *req)
}
mod_ctx->client = state->princ;
+ /* `uuid` is output arg and isn't read in kcm_cc_get_uuid() but
+ * since libuuid is opaquie for cppcheck it generates false positive here
+ */
+ /* cppcheck-suppress uninitvar */
ret = kcm_cc_get_uuid(state->new_cc, uuid);
if (ret != EOK) {
tevent_req_error(req, ret);
@@ -660,6 +668,10 @@ static void kcm_op_initialize_got_default(struct tevent_req *subreq)
/* If there was a previous default ccache, switch to the initialized
* one by default
*/
+ /* `dfl_uuid` is output arg and isn't read in kcm_cc_get_uuid() but
+ * since libuuid is opaquie for cppcheck it generates false positive here
+ */
+ /* cppcheck-suppress uninitvar */
ret = kcm_cc_get_uuid(state->new_cc, dfl_uuid);
if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE,
@@ -773,6 +785,10 @@ static void kcm_op_destroy_getbyname_done(struct tevent_req *subreq)
struct kcm_op_common_state);
uuid_t uuid;
+ /* `uuid` is output arg and isn't read in kcm_ccdb_uuid_by_name_recv() but
+ * since libuuid is opaquie for cppcheck it generates false positive here
+ */
+ /* cppcheck-suppress uninitvar */
ret = kcm_ccdb_uuid_by_name_recv(subreq, state, uuid);
talloc_zfree(subreq);
if (ret != EOK) {
@@ -910,6 +926,10 @@ static void kcm_op_store_getbyname_done(struct tevent_req *subreq)
struct kcm_op_store_state);
uuid_t uuid;
+ /* `uuid` is output arg and isn't read in kcm_ccdb_uuid_by_name_recv() but
+ * since libuuid is opaquie for cppcheck it generates false positive here
+ */
+ /* cppcheck-suppress uninitvar */
ret = kcm_ccdb_uuid_by_name_recv(subreq, state, uuid);
talloc_zfree(subreq);
if (ret != EOK) {
@@ -2051,6 +2071,10 @@ static void kcm_op_set_kdc_offset_getbyname_done(struct tevent_req *subreq)
struct kcm_op_set_kdc_offset_state *state = tevent_req_data(req,
struct kcm_op_set_kdc_offset_state);
+ /* `uuid` is output arg and isn't read in kcm_ccdb_uuid_by_name_recv() but
+ * since libuuid is opaquie for cppcheck it generates false positive here
+ */
+ /* cppcheck-suppress uninitvar */
ret = kcm_ccdb_uuid_by_name_recv(subreq, state, uuid);
talloc_zfree(subreq);
if (ret != EOK) {
From 54abd9769085352490eb10713dcc53623932c426 Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Wed, 16 Sep 2020 13:50:32 +0200
Subject: [PATCH 3/6] RESOLV: makes use of sss_rand() helper
Makes use of sss_rand() helper instead of plain srand()/rand()
Reduces amount of "Error: DC.WEAK_CRYPTO (CWE-327)" warnings.
---
src/resolv/async_resolv.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/src/resolv/async_resolv.c b/src/resolv/async_resolv.c
index 00b9531d49..07f05ff17b 100644
--- a/src/resolv/async_resolv.c
+++ b/src/resolv/async_resolv.c
@@ -2178,12 +2178,6 @@ static int reply_weight_rearrange(int len,
return ENOMEM;
}
- /* This is not security relevant functionality and
- * it is undesirable to pull unnecessary dependency (util/crypto)
- * so plain srand() & rand() are used here.
- */
- srand(time(NULL) * getpid());
-
/* promote all servers with weight==0 to the top */
r = *(start);
prev = NULL;
@@ -2221,7 +2215,7 @@ static int reply_weight_rearrange(int len,
* first in the selected order which is greater than or equal to
* the random number selected.
*/
- selected = (int)((total + 1) * (rand()/(RAND_MAX + 1.0)));
+ selected = (int)((total + 1) * (sss_rand()/(RAND_MAX + 1.0)));
for (i = 0, r = *start, prev = NULL; r != NULL; r=r->next, ++i) {
if (totals[i] >= selected)
break;
From b399c1f6d8e0dda92aaccb35a2c16ebd6160a360 Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Wed, 16 Sep 2020 15:58:50 +0200
Subject: [PATCH 4/6] UTIL: fortify IS_SSSD_ERROR() check
Fixes following warning:
```
Error: NEGATIVE_RETURNS (CWE-394):
sssd-2.3.2/src/providers/ldap/sdap_async.c:1516: var_tested_neg: Variable "lret" tests negative.
sssd-2.3.2/src/providers/ldap/sdap_async.c:1525: negative_returns: "lret" is passed to a parameter that cannot be negative.
# 1523| }
# 1524| else {
# 1525|-> sss_log(SSS_LOG_ERR, "LDAP connection error, %s",
# 1526| sss_ldap_err2string(lret));
# 1527| }
```
---
src/util/util_errors.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/util_errors.h b/src/util/util_errors.h
index 41697ea522..75c46286af 100644
--- a/src/util/util_errors.h
+++ b/src/util/util_errors.h
@@ -170,7 +170,7 @@ enum sssd_errors {
#define SSSD_ERR_BASE(err) ((err) & ~ERR_MASK)
#define SSSD_ERR_IDX(err) ((err) & ERR_MASK)
#define IS_SSSD_ERROR(err) \
- ((SSSD_ERR_BASE(err) == ERR_BASE) && ((err) <= ERR_LAST))
+ (((err) > 0) && (SSSD_ERR_BASE(err) == ERR_BASE) && ((err) <= ERR_LAST))
#define ERR_OK 0
/* Backwards compat */
From ee2ad18dc160efc30717b0739ae489664368cb8d Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Wed, 16 Sep 2020 17:12:01 +0200
Subject: [PATCH 5/6] LDAP: sdap_parse_entry() optimization
It doesn't make sense to iterate over `map` if sdap_parse_range()
returned ECANCELED anyway.
Also fixes following warning:
```
Error: CLANG_WARNING:
sssd-2.3.2/src/providers/ldap/sdap.c:529:13: warning: Value stored to 'ret' is never read
# ret = EOK;
# ^ ~~~
```
---
src/providers/ldap/sdap.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c
index 010577b0db..a7f41963e1 100644
--- a/src/providers/ldap/sdap.c
+++ b/src/providers/ldap/sdap.c
@@ -501,7 +501,9 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
goto done;
}
- if (map) {
+ if (ret == ECANCELED) {
+ store = false;
+ } else if (map) {
for (i = 1; i < attrs_num; i++) {
/* check if this attr is valid with the chosen schema */
if (!map[i].name) continue;
@@ -525,11 +527,6 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
store = true;
}
- if (ret == ECANCELED) {
- ret = EOK;
- store = false;
- }
-
if (store) {
vals = ldap_get_values_len(sh->ldap, sm->msg, str);
if (!vals) {
From 86219cdfdcd4ce46d71f2efc3ac9faf7bd696b06 Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Wed, 16 Sep 2020 17:32:59 +0200
Subject: [PATCH 6/6] DP: fixes couple of covscan's complains
Fixes warnings like:
```
Error: MISSING_RESTORE (CWE-573):
sssd-2.3.2/src/providers/data_provider_fo.c:61: compare: Verifying that non-local "ctx->be_fo" is initially equal to sentinel value "NULL".
sssd-2.3.2/src/providers/data_provider_fo.c:65: modify: Modifying non-local "ctx->be_fo".
sssd-2.3.2/src/providers/data_provider_fo.c:67: end_of_path: Value of non-local "ctx->be_fo" that was verified to be "NULL" is not restored as it was along other paths.
sssd-2.3.2/src/providers/data_provider_fo.c:87: restore_example: The original value of non-local "ctx->be_fo" was restored here.
# 65| ctx->be_fo = talloc_zero(ctx, struct be_failover_ctx);
# 66| if (!ctx->be_fo) {
# 67|-> return ENOMEM;
# 68| }
# 69|
```
---
src/providers/data_provider_fo.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/providers/data_provider_fo.c b/src/providers/data_provider_fo.c
index afc6081afa..8dc09f5b23 100644
--- a/src/providers/data_provider_fo.c
+++ b/src/providers/data_provider_fo.c
@@ -63,7 +63,7 @@ int be_init_failover(struct be_ctx *ctx)
}
ctx->be_fo = talloc_zero(ctx, struct be_failover_ctx);
- if (!ctx->be_fo) {
+ if (ctx->be_fo == NULL) {
return ENOMEM;
}
@@ -884,7 +884,7 @@ errno_t be_res_init(struct be_ctx *ctx)
}
ctx->be_res = talloc_zero(ctx, struct be_resolv_ctx);
- if (!ctx->be_res) {
+ if (ctx->be_res == NULL) {
return ENOMEM;
}
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
