URL: https://github.com/SSSD/sssd/pull/5661 Author: sumit-bose Title: #5661: pam: change default for pam_response_filter Action: opened
PR body: """ So far pam_response_filter didn't had any default. It turned out that it would be useful to filter the environment variable KRB5CCANME by default for sudo. The reason is the e.g. in contrast to su the calling user is authenticated and hence only the Kerberos credentials of the calling user are available. But this causes a couple of inconsistencies. E.g. depending on the credential cache type the target user might not have access to the credential cache and even if the credential cache can be accessed it will contain credentials which different privileges than the target user. As a result it seems better to not make KRB5CCANME in the environment of the target user and let him pick the matching default credential cache. Resolves: https://github.com/SSSD/sssd/issues/5660 """ To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5661/head:pr5661 git checkout pr5661
From 23c449230553c9b179858fe9de76f5f6d2d90d3d Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Fri, 28 May 2021 15:59:17 +0200 Subject: [PATCH 1/4] utils: add mod_defaults_list This patch adds a new utility function to handle options with values prefixed by '+' or '-' to modify default lists. Unit tests are included. Resolves: https://github.com/SSSD/sssd/issues/5660 --- src/tests/cmocka/test_string_utils.c | 101 ++++++++++++++++++++++++++ src/tests/cmocka/test_utils.c | 3 + src/tests/cmocka/test_utils.h | 1 + src/util/string_utils.c | 104 +++++++++++++++++++++++++++ src/util/util.h | 3 + 5 files changed, 212 insertions(+) diff --git a/src/tests/cmocka/test_string_utils.c b/src/tests/cmocka/test_string_utils.c index 57e6f2617b..f11fc21a6a 100644 --- a/src/tests/cmocka/test_string_utils.c +++ b/src/tests/cmocka/test_string_utils.c @@ -269,3 +269,104 @@ void test_concatenate_string_array(void **state) assert_true(check_leaks_pop(mem_ctx) == true); talloc_free(mem_ctx); } + +void test_mod_defaults_list(void **state) +{ + int ret; + char **list; + size_t c; + size_t t; + const char *default_list[] = {"abc", "def", "ghi", "jkl", NULL}; + + const char *mod_list_error1[] = {"abc", "+def", NULL}; + const char *mod_list_error2[] = {"-abc", "def", NULL}; + + const char *mod_list1[] = {"-abc", NULL}; + const char *res_list1[] = {"def", "ghi", "jkl", NULL}; + + const char *mod_list2[] = {"-def", NULL}; + const char *res_list2[] = {"abc", "ghi", "jkl", NULL}; + + const char *mod_list3[] = {"-jkl", NULL}; + const char *res_list3[] = {"abc", "def", "ghi", NULL}; + + const char *mod_list4[] = {"+abc", NULL}; + const char *res_list4[] = {"abc", "abc", "def", "ghi", "jkl", NULL}; + + const char *mod_list5[] = {"+xyz", NULL}; + const char *res_list5[] = {"xyz", "abc", "def", "ghi", "jkl", NULL}; + + const char *mod_list6[] = {"+xyz", "-xyz", NULL}; + const char *res_list6[] = {"abc", "def", "ghi", "jkl", NULL}; + + const char *mod_list7[] = {"-xyz", NULL}; + const char *res_list7[] = {"abc", "def", "ghi", "jkl", NULL}; + + struct test_data { + const char **mod_list; + const char **res_list; + } test_data[] = { + {mod_list1, res_list1 }, + {mod_list2, res_list2 }, + {mod_list3, res_list3 }, + {mod_list4, res_list4 }, + {mod_list5, res_list5 }, + {mod_list6, res_list6 }, + {mod_list7, res_list7 }, + {NULL, NULL} + }; + + ret = mod_defaults_list(NULL, NULL, NULL, NULL); + assert_int_equal(ret, EOK); + + list = NULL; + ret = mod_defaults_list(NULL, NULL, NULL, &list); + assert_int_equal(ret, EOK); + assert_non_null(list); + assert_null(list[0]); + talloc_free(list); + + list = NULL; + ret = mod_defaults_list(NULL, default_list, NULL, &list); + assert_int_equal(ret, EOK); + assert_non_null(list); + for (c = 0; default_list[c] != NULL; c++) { + assert_string_equal(list[c], default_list[c]); + } + assert_null(list[c]); + talloc_free(list); + + list = NULL; + ret = mod_defaults_list(NULL, default_list, discard_const(mod_list_error1), + &list); + assert_int_equal(ret, EINVAL); + assert_null(list); + + list = NULL; + ret = mod_defaults_list(NULL, default_list, discard_const(mod_list_error2), + &list); + assert_int_equal(ret, EINVAL); + assert_null(list); + + list = NULL; + ret = mod_defaults_list(NULL, NULL, discard_const(mod_list4), &list); + assert_int_equal(ret, EOK); + assert_non_null(list); + assert_string_equal(list[0], "abc"); + assert_null(list[1]); + talloc_free(list); + + for (t = 0; test_data[t].res_list != NULL; t++) { + list = NULL; + ret = mod_defaults_list(NULL, default_list, + discard_const(test_data[t].mod_list), &list); + assert_int_equal(ret, EOK); + assert_non_null(list); + for (c = 0; test_data[t].res_list[c] != NULL; c++) { + assert_non_null(list[c]); + assert_string_equal(list[c], test_data[t].res_list[c]); + } + assert_null(list[c]); + talloc_free(list); + } +} diff --git a/src/tests/cmocka/test_utils.c b/src/tests/cmocka/test_utils.c index 120a2276de..536cee59f4 100644 --- a/src/tests/cmocka/test_utils.c +++ b/src/tests/cmocka/test_utils.c @@ -2173,6 +2173,9 @@ int main(int argc, const char *argv[]) cmocka_unit_test_setup_teardown(test_sss_filter_sanitize_dn, setup_leak_tests, teardown_leak_tests), + cmocka_unit_test_setup_teardown(test_mod_defaults_list, + setup_leak_tests, + teardown_leak_tests), }; /* Set debug level to invalid value so we can decide if -d 0 was used. */ diff --git a/src/tests/cmocka/test_utils.h b/src/tests/cmocka/test_utils.h index 458bcb7505..7c21de8092 100644 --- a/src/tests/cmocka/test_utils.h +++ b/src/tests/cmocka/test_utils.h @@ -32,6 +32,7 @@ void test_reverse_replace_whitespaces(void **state); void test_guid_blob_to_string_buf(void **state); void test_get_last_x_chars(void **state); void test_concatenate_string_array(void **state); +void test_mod_defaults_list(void **state); /* from src/tests/cmocka/test_sss_ptr_hash.c */ void test_sss_ptr_hash_with_free_cb(void **state); diff --git a/src/util/string_utils.c b/src/util/string_utils.c index f54395a59a..f25bf03297 100644 --- a/src/util/string_utils.c +++ b/src/util/string_utils.c @@ -146,3 +146,107 @@ char **concatenate_string_array(TALLOC_CTX *mem_ctx, return string_array; } + +errno_t mod_defaults_list(TALLOC_CTX *mem_ctx, const char **defaults_list, + char **mod_list, char ***_list) +{ + TALLOC_CTX *tmp_ctx; + errno_t ret; + size_t mod_list_size; + const char **add_list; + const char **remove_list; + size_t c; + size_t ai = 0; + size_t ri = 0; + size_t j = 0; + char **list; + size_t expected_list_size = 0; + size_t defaults_list_size = 0; + + for (defaults_list_size = 0; + defaults_list != NULL &&defaults_list[defaults_list_size] != NULL; + defaults_list_size++); + + for (mod_list_size = 0; + mod_list != NULL && mod_list[mod_list_size] != NULL; + mod_list_size++); + + tmp_ctx = talloc_new(mem_ctx); + if (tmp_ctx == NULL) { + return ENOMEM; + } + + add_list = talloc_zero_array(tmp_ctx, const char *, mod_list_size + 1); + remove_list = talloc_zero_array(tmp_ctx, const char *, mod_list_size + 1); + + if (add_list == NULL || remove_list == NULL) { + ret = ENOMEM; + goto done; + } + + for (c = 0; mod_list != NULL && mod_list[c] != NULL; c++) { + switch (mod_list[c][0]) { + case '+': + add_list[ai] = mod_list[c] + 1; + ++ai; + break; + case '-': + remove_list[ri] = mod_list[c] + 1; + ++ri; + break; + default: + DEBUG(SSSDBG_OP_FAILURE, + "The option "CONFDB_PAM_P11_ALLOWED_SERVICES" must start" + "with either '+' (for adding service) or '-' (for " + "removing service) got '%s'\n", mod_list[c]); + ret = EINVAL; + goto done; + } + } + + expected_list_size = defaults_list_size + ai + 1; + + list = talloc_zero_array(tmp_ctx, char *, expected_list_size); + if (list == NULL) { + ret = ENOMEM; + goto done; + } + + for (c = 0; add_list[c] != NULL; ++c) { + if (string_in_list(add_list[c], discard_const(remove_list), false)) { + continue; + } + + list[j] = talloc_strdup(list, add_list[c]); + if (list[j] == NULL) { + ret = ENOMEM; + goto done; + } + j++; + } + + for (c = 0; defaults_list != NULL && defaults_list[c] != NULL; ++c) { + if (string_in_list(defaults_list[c], + discard_const(remove_list), false)) { + continue; + } + + list[j] = talloc_strdup(list, defaults_list[c]); + if (list[j] == NULL) { + ret = ENOMEM; + goto done; + } + j++; + } + + if (_list != NULL) { + *_list = talloc_steal(mem_ctx, list); + } + + ret = EOK; + +done: + talloc_zfree(tmp_ctx); + + return ret; +} diff --git a/src/util/util.h b/src/util/util.h index 7a1e684aa0..7553585119 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -701,6 +701,9 @@ char **concatenate_string_array(TALLOC_CTX *mem_ctx, char **arr1, size_t len1, char **arr2, size_t len2); +errno_t mod_defaults_list(TALLOC_CTX *mem_ctx, const char **defaults_list, + char **mod_list, char ***_list); + /* from become_user.c */ errno_t become_user(uid_t uid, gid_t gid); struct sss_creds; From 326142033a16f7173d86c42de6c7daa03c2c4c45 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Fri, 28 May 2021 16:22:00 +0200 Subject: [PATCH 2/4] pam: replace first argument of filter_responses() The first argument of filter_responses() is replaced with a more generic context to allow more flexible use in future. Resolves: https://github.com/SSSD/sssd/issues/5660 --- src/responder/pam/pamsrv.h | 2 +- src/responder/pam/pamsrv_cmd.c | 8 ++++---- src/tests/cmocka/test_pam_srv.c | 31 ++++++++++++++++--------------- 3 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/responder/pam/pamsrv.h b/src/responder/pam/pamsrv.h index 383c7bee6c..1964f6a9bd 100644 --- a/src/responder/pam/pamsrv.h +++ b/src/responder/pam/pamsrv.h @@ -139,7 +139,7 @@ pam_set_last_online_auth_with_curr_token(struct sss_domain_info *domain, const char *username, uint64_t value); -errno_t filter_responses(struct confdb_ctx *cdb, +errno_t filter_responses(struct pam_ctx *pctx, struct response_data *resp_list, struct pam_data *pd); diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c index 2817db1d82..73d6117254 100644 --- a/src/responder/pam/pamsrv_cmd.c +++ b/src/responder/pam/pamsrv_cmd.c @@ -593,7 +593,7 @@ static errno_t filter_responses_env(struct response_data *resp, return EOK; } -errno_t filter_responses(struct confdb_ctx *cdb, +errno_t filter_responses(struct pam_ctx *pctx, struct response_data *resp_list, struct pam_data *pd) { @@ -604,7 +604,7 @@ errno_t filter_responses(struct confdb_ctx *cdb, int pam_verbosity = DEFAULT_PAM_VERBOSITY; char **pam_filter_opts = NULL; - ret = confdb_get_int(cdb, CONFDB_PAM_CONF_ENTRY, + ret = confdb_get_int(pctx->rctx->cdb, CONFDB_PAM_CONF_ENTRY, CONFDB_PAM_VERBOSITY, DEFAULT_PAM_VERBOSITY, &pam_verbosity); if (ret != EOK) { @@ -613,7 +613,7 @@ errno_t filter_responses(struct confdb_ctx *cdb, pam_verbosity = DEFAULT_PAM_VERBOSITY; } - ret = confdb_get_string_as_list(cdb, pd, CONFDB_PAM_CONF_ENTRY, + ret = confdb_get_string_as_list(pctx->rctx->cdb, pd, CONFDB_PAM_CONF_ENTRY, CONFDB_PAM_RESPONSE_FILTER, &pam_filter_opts); if (ret != EOK) { @@ -1036,7 +1036,7 @@ static void pam_reply(struct pam_auth_req *preq) inform_user(pd, pam_account_locked_message); } - ret = filter_responses(pctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pctx, pd->resp_list, pd); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "filter_responses failed, not fatal.\n"); } diff --git a/src/tests/cmocka/test_pam_srv.c b/src/tests/cmocka/test_pam_srv.c index 1e747bba2f..f9f57b9753 100644 --- a/src/tests/cmocka/test_pam_srv.c +++ b/src/tests/cmocka/test_pam_srv.c @@ -3191,9 +3191,10 @@ void test_filter_response(void **state) /* pd->resp_list points to the SSS_PAM_USER_INFO and pd->resp_list->next * to the SSS_PAM_ENV_ITEM message. */ + pam_test_ctx->pctx->rctx = pam_test_ctx->rctx; /* Test CONFDB_PAM_VERBOSITY option */ - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_true(pd->resp_list->do_not_send_to_client); assert_false(pd->resp_list->next->do_not_send_to_client); @@ -3204,7 +3205,7 @@ void test_filter_response(void **state) ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_false(pd->resp_list->do_not_send_to_client); assert_false(pd->resp_list->next->do_not_send_to_client); @@ -3213,7 +3214,7 @@ void test_filter_response(void **state) ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_true(pd->resp_list->do_not_send_to_client); assert_false(pd->resp_list->next->do_not_send_to_client); @@ -3223,7 +3224,7 @@ void test_filter_response(void **state) ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_true(pd->resp_list->do_not_send_to_client); assert_false(pd->resp_list->next->do_not_send_to_client); @@ -3233,7 +3234,7 @@ void test_filter_response(void **state) ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_true(pd->resp_list->do_not_send_to_client); assert_true(pd->resp_list->next->do_not_send_to_client); @@ -3242,7 +3243,7 @@ void test_filter_response(void **state) ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_true(pd->resp_list->do_not_send_to_client); assert_true(pd->resp_list->next->do_not_send_to_client); @@ -3251,7 +3252,7 @@ void test_filter_response(void **state) ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_true(pd->resp_list->do_not_send_to_client); assert_true(pd->resp_list->next->do_not_send_to_client); @@ -3260,7 +3261,7 @@ void test_filter_response(void **state) ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_true(pd->resp_list->do_not_send_to_client); assert_false(pd->resp_list->next->do_not_send_to_client); @@ -3269,7 +3270,7 @@ void test_filter_response(void **state) ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_true(pd->resp_list->do_not_send_to_client); assert_false(pd->resp_list->next->do_not_send_to_client); @@ -3278,7 +3279,7 @@ void test_filter_response(void **state) ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_true(pd->resp_list->do_not_send_to_client); assert_false(pd->resp_list->next->do_not_send_to_client); @@ -3288,7 +3289,7 @@ void test_filter_response(void **state) ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_true(pd->resp_list->do_not_send_to_client); assert_false(pd->resp_list->next->do_not_send_to_client); @@ -3297,7 +3298,7 @@ void test_filter_response(void **state) ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_true(pd->resp_list->do_not_send_to_client); assert_true(pd->resp_list->next->do_not_send_to_client); @@ -3306,7 +3307,7 @@ void test_filter_response(void **state) ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_true(pd->resp_list->do_not_send_to_client); assert_true(pd->resp_list->next->do_not_send_to_client); @@ -3315,7 +3316,7 @@ void test_filter_response(void **state) ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_true(pd->resp_list->do_not_send_to_client); assert_true(pd->resp_list->next->do_not_send_to_client); @@ -3327,7 +3328,7 @@ void test_filter_response(void **state) ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); - ret = filter_responses(pam_test_ctx->rctx->cdb, pd->resp_list, pd); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); assert_int_equal(ret, EOK); assert_true(pd->resp_list->do_not_send_to_client); assert_true(pd->resp_list->next->do_not_send_to_client); From 3199dffedfa72466e12007055083bb027b3ed385 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Fri, 28 May 2021 17:08:14 +0200 Subject: [PATCH 3/4] pam: parse pam_response_filter values only once To avoid parsing the configuration options for each PAM request the code is modified to parse them only once. If the configuration is changed it is already expected that SSSD is restarted which mean that with this change no functionality is lost. Tests had to be updated to make sure new values are read. Resolves: https://github.com/SSSD/sssd/issues/5660 --- src/responder/pam/pamsrv.h | 3 +++ src/responder/pam/pamsrv_cmd.c | 33 +++++++++++++++++++++++---------- src/tests/cmocka/test_pam_srv.c | 12 ++++++++++++ 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/src/responder/pam/pamsrv.h b/src/responder/pam/pamsrv.h index 1964f6a9bd..136b1392fb 100644 --- a/src/responder/pam/pamsrv.h +++ b/src/responder/pam/pamsrv.h @@ -58,6 +58,9 @@ struct pam_ctx { struct sss_certmap_ctx *sss_certmap_ctx; char **smartcard_services; + /* parsed list of pam_response_filter option */ + char **pam_filter_opts; + char **prompting_config_sections; int num_prompting_config_sections; diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c index 73d6117254..9849194f8c 100644 --- a/src/responder/pam/pamsrv_cmd.c +++ b/src/responder/pam/pamsrv_cmd.c @@ -602,7 +602,6 @@ errno_t filter_responses(struct pam_ctx *pctx, uint32_t user_info_type; int64_t expire_date = 0; int pam_verbosity = DEFAULT_PAM_VERBOSITY; - char **pam_filter_opts = NULL; ret = confdb_get_int(pctx->rctx->cdb, CONFDB_PAM_CONF_ENTRY, CONFDB_PAM_VERBOSITY, DEFAULT_PAM_VERBOSITY, @@ -613,13 +612,28 @@ errno_t filter_responses(struct pam_ctx *pctx, pam_verbosity = DEFAULT_PAM_VERBOSITY; } - ret = confdb_get_string_as_list(pctx->rctx->cdb, pd, CONFDB_PAM_CONF_ENTRY, - CONFDB_PAM_RESPONSE_FILTER, - &pam_filter_opts); - if (ret != EOK) { - DEBUG(SSSDBG_CONF_SETTINGS, "[%s] not available, not fatal.\n", - CONFDB_PAM_RESPONSE_FILTER); - pam_filter_opts = NULL; + if (pctx->pam_filter_opts == NULL) { + ret = confdb_get_string_as_list(pctx->rctx->cdb, pctx, + CONFDB_PAM_CONF_ENTRY, + CONFDB_PAM_RESPONSE_FILTER, + &pctx->pam_filter_opts); + if (ret != EOK) { + if (ret == ENOENT) { + DEBUG(SSSDBG_CONF_SETTINGS, "[%s] not available, not fatal.\n", + CONFDB_PAM_RESPONSE_FILTER); + pctx->pam_filter_opts = talloc_zero_array(pctx, char *, 1); + if (pctx->pam_filter_opts == NULL) { + DEBUG(SSSDBG_OP_FAILURE, + "Failed to allocate memory for empty [%s], not fatal.\n", + CONFDB_PAM_RESPONSE_FILTER); + } + } else { + DEBUG(SSSDBG_OP_FAILURE, + "Failed to read values of [%s], not fatal.\n", + CONFDB_PAM_RESPONSE_FILTER); + pctx->pam_filter_opts = NULL; + } + } } resp = resp_list; @@ -666,7 +680,7 @@ errno_t filter_responses(struct pam_ctx *pctx, } } else if (resp->type == SSS_PAM_ENV_ITEM) { resp->do_not_send_to_client = false; - ret = filter_responses_env(resp, pd, pam_filter_opts); + ret = filter_responses_env(resp, pd, pctx->pam_filter_opts); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "filter_responses_env failed.\n"); goto done; @@ -680,7 +694,6 @@ errno_t filter_responses(struct pam_ctx *pctx, ret = EOK; done: - talloc_free(pam_filter_opts); return ret; } diff --git a/src/tests/cmocka/test_pam_srv.c b/src/tests/cmocka/test_pam_srv.c index f9f57b9753..c0f6280fa3 100644 --- a/src/tests/cmocka/test_pam_srv.c +++ b/src/tests/cmocka/test_pam_srv.c @@ -3220,6 +3220,7 @@ void test_filter_response(void **state) assert_false(pd->resp_list->next->do_not_send_to_client); /* Test CONFDB_PAM_RESPONSE_FILTER option */ + talloc_zfree(pam_test_ctx->pctx->pam_filter_opts); pam_params[1].value = "NoSuchOption"; ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); @@ -3229,6 +3230,7 @@ void test_filter_response(void **state) assert_true(pd->resp_list->do_not_send_to_client); assert_false(pd->resp_list->next->do_not_send_to_client); + talloc_zfree(pam_test_ctx->pctx->pam_filter_opts); pam_params[1].value = "ENV"; /* filter all environment variables */ /* for all services */ ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); @@ -3239,6 +3241,7 @@ void test_filter_response(void **state) assert_true(pd->resp_list->do_not_send_to_client); assert_true(pd->resp_list->next->do_not_send_to_client); + talloc_zfree(pam_test_ctx->pctx->pam_filter_opts); pam_params[1].value = "ENV:"; /* filter all environment variables */ ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); @@ -3248,6 +3251,7 @@ void test_filter_response(void **state) assert_true(pd->resp_list->do_not_send_to_client); assert_true(pd->resp_list->next->do_not_send_to_client); + talloc_zfree(pam_test_ctx->pctx->pam_filter_opts); pam_params[1].value = "ENV::"; /* filter all environment variables */ ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); @@ -3257,6 +3261,7 @@ void test_filter_response(void **state) assert_true(pd->resp_list->do_not_send_to_client); assert_true(pd->resp_list->next->do_not_send_to_client); + talloc_zfree(pam_test_ctx->pctx->pam_filter_opts); pam_params[1].value = "ENV:abc:"; /* variable name does not match */ ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); @@ -3266,6 +3271,7 @@ void test_filter_response(void **state) assert_true(pd->resp_list->do_not_send_to_client); assert_false(pd->resp_list->next->do_not_send_to_client); + talloc_zfree(pam_test_ctx->pctx->pam_filter_opts); pam_params[1].value = "ENV:abc:MyService"; /* variable name does not match */ ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); @@ -3275,6 +3281,7 @@ void test_filter_response(void **state) assert_true(pd->resp_list->do_not_send_to_client); assert_false(pd->resp_list->next->do_not_send_to_client); + talloc_zfree(pam_test_ctx->pctx->pam_filter_opts); pam_params[1].value = "ENV::abc"; /* service name does not match */ ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); @@ -3285,6 +3292,7 @@ void test_filter_response(void **state) assert_false(pd->resp_list->next->do_not_send_to_client); /* service name does not match */ + talloc_zfree(pam_test_ctx->pctx->pam_filter_opts); pam_params[1].value = "ENV:MyEnv:abc"; ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); @@ -3294,6 +3302,7 @@ void test_filter_response(void **state) assert_true(pd->resp_list->do_not_send_to_client); assert_false(pd->resp_list->next->do_not_send_to_client); + talloc_zfree(pam_test_ctx->pctx->pam_filter_opts); pam_params[1].value = "ENV:MyEnv"; /* match */ ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); @@ -3303,6 +3312,7 @@ void test_filter_response(void **state) assert_true(pd->resp_list->do_not_send_to_client); assert_true(pd->resp_list->next->do_not_send_to_client); + talloc_zfree(pam_test_ctx->pctx->pam_filter_opts); pam_params[1].value = "ENV:MyEnv:"; /* match */ ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); @@ -3312,6 +3322,7 @@ void test_filter_response(void **state) assert_true(pd->resp_list->do_not_send_to_client); assert_true(pd->resp_list->next->do_not_send_to_client); + talloc_zfree(pam_test_ctx->pctx->pam_filter_opts); pam_params[1].value = "ENV:MyEnv:MyService"; /* match */ ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); assert_int_equal(ret, EOK); @@ -3322,6 +3333,7 @@ void test_filter_response(void **state) assert_true(pd->resp_list->next->do_not_send_to_client); /* multiple rules with a match */ + talloc_zfree(pam_test_ctx->pctx->pam_filter_opts); pam_params[1].value = "ENV:abc:def, " "ENV:MyEnv:MyService, " "ENV:stu:xyz"; From 14f973fd77a4bc194cdc7da5c93e154e61e92e33 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Fri, 28 May 2021 19:32:46 +0200 Subject: [PATCH 4/4] pam: change default for pam_response_filter So far pam_response_filter didn't had any default. It turned out that it would be useful to filter the environment variable KRB5CCANME by default for sudo. The reason is the e.g. in contrast to su the calling user is authenticated and hence only the Kerberos credentials of the calling user are available. But this causes a couple of inconsistencies. E.g. depending on the credential cache type the target user might not have access to the credential cache and even if the credential cache can be accessed it will contain credentials which different privileges than the target user. As a result it seems better to not make KRB5CCANME in the environment of the target user and let him pick the matching default credential cache. Resolves: https://github.com/SSSD/sssd/issues/5660 --- src/man/sssd.conf.5.xml | 16 +++++++- src/responder/pam/pamsrv_cmd.c | 55 ++++++++++++++++++------- src/tests/cmocka/test_pam_srv.c | 72 +++++++++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 16 deletions(-) diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index 09081f0abb..d2e43dd490 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -1405,10 +1405,22 @@ fallback_homedir = /home/%u </variablelist> </para> <para> - Default: not set + The list of strings can either be the list of + filters which would set this list of filters and + overwrite the defaults. Or each element of the list + can be prefixed by a '+' or '-' character which + would add the filter to the existing default or + remove it from the defaults, respectively. Please + note that either all list elements must have a '+' + or '-' prefix or none. It is considered as an error + to mix both styles. </para> <para> - Example: ENV:KRB5CCNAME:sudo-i + Default: ENV:KRB5CCNAME:sudo, ENV:KRB5CCNAME:sudo-i + </para> + <para> + Example: -ENV:KRB5CCNAME:sudo-i will remove the + filter from the default list </para> </listitem> </varlistentry> diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c index 9849194f8c..d904c0dabb 100644 --- a/src/responder/pam/pamsrv_cmd.c +++ b/src/responder/pam/pamsrv_cmd.c @@ -602,6 +602,11 @@ errno_t filter_responses(struct pam_ctx *pctx, uint32_t user_info_type; int64_t expire_date = 0; int pam_verbosity = DEFAULT_PAM_VERBOSITY; + char **new_opts; + size_t c; + const char *default_pam_response_filter[] = { "ENV:KRB5CCNAME:sudo", + "ENV:KRB5CCNAME:sudo-i", + NULL }; ret = confdb_get_int(pctx->rctx->cdb, CONFDB_PAM_CONF_ENTRY, CONFDB_PAM_VERBOSITY, DEFAULT_PAM_VERBOSITY, @@ -617,21 +622,43 @@ errno_t filter_responses(struct pam_ctx *pctx, CONFDB_PAM_CONF_ENTRY, CONFDB_PAM_RESPONSE_FILTER, &pctx->pam_filter_opts); - if (ret != EOK) { - if (ret == ENOENT) { - DEBUG(SSSDBG_CONF_SETTINGS, "[%s] not available, not fatal.\n", - CONFDB_PAM_RESPONSE_FILTER); - pctx->pam_filter_opts = talloc_zero_array(pctx, char *, 1); - if (pctx->pam_filter_opts == NULL) { - DEBUG(SSSDBG_OP_FAILURE, - "Failed to allocate memory for empty [%s], not fatal.\n", - CONFDB_PAM_RESPONSE_FILTER); + if (ret != EOK && ret != ENOENT) { + DEBUG(SSSDBG_OP_FAILURE, + "Failed to read values of [%s], not fatal.\n", + CONFDB_PAM_RESPONSE_FILTER); + pctx->pam_filter_opts = NULL; + } else { + if (pctx->pam_filter_opts == NULL + || *pctx->pam_filter_opts[0] == '+' + || *pctx->pam_filter_opts[0] == '-') { + ret = mod_defaults_list(pctx, default_pam_response_filter, + pctx->pam_filter_opts, &new_opts); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "Failed to modify [%s] defaults.\n", + CONFDB_PAM_RESPONSE_FILTER); + return ret; } - } else { - DEBUG(SSSDBG_OP_FAILURE, - "Failed to read values of [%s], not fatal.\n", - CONFDB_PAM_RESPONSE_FILTER); - pctx->pam_filter_opts = NULL; + talloc_free(pctx->pam_filter_opts); + pctx->pam_filter_opts = new_opts; + } + } + + if (pctx->pam_filter_opts == NULL) { + DEBUG(SSSDBG_CONF_SETTINGS, "No PAM response filter set.\n"); + } else { + /* Make sure there are no '+' or '-' prefixes anymore */ + for (c = 0; pctx->pam_filter_opts[c] != NULL; c++) { + if (*pctx->pam_filter_opts[0] == '+' + || *pctx->pam_filter_opts[0] == '-') { + DEBUG(SSSDBG_CRIT_FAILURE, + "Unsupport mix of prefixed and not prefixed " + "values of [%s].\n", CONFDB_PAM_RESPONSE_FILTER); + return EINVAL; + } + DEBUG(SSSDBG_CONF_SETTINGS, + "PAM response filter: [%s].\n", + pctx->pam_filter_opts[c]); } } } diff --git a/src/tests/cmocka/test_pam_srv.c b/src/tests/cmocka/test_pam_srv.c index c0f6280fa3..e00b585c13 100644 --- a/src/tests/cmocka/test_pam_srv.c +++ b/src/tests/cmocka/test_pam_srv.c @@ -3348,6 +3348,76 @@ void test_filter_response(void **state) talloc_free(pd); } +#define ENV_1 "MyEnv=abcdef" +#define ENV_2 "KRB5CCNAME=abc" +void test_filter_response_defaults(void **state) +{ + int ret; + struct pam_data *pd; + uint8_t offline_auth_data[(sizeof(uint32_t) + sizeof(int64_t))]; + uint32_t info_type; + + struct sss_test_conf_param pam_params[] = { + { CONFDB_PAM_VERBOSITY, "1" }, + { CONFDB_PAM_RESPONSE_FILTER, NULL }, + { NULL, NULL }, /* Sentinel */ + }; + + ret = add_pam_params(pam_params, pam_test_ctx->rctx->cdb); + assert_int_equal(ret, EOK); + + pd = talloc_zero(pam_test_ctx, struct pam_data); + assert_non_null(pd); + + /* Currently only KRB5CCNAME is filtered for sudo and sudo-i, so all other + * environment variables and all other services should not be affected */ + pd->service = discard_const("MyService"); + + ret = pam_add_response(pd, SSS_PAM_ENV_ITEM, + strlen(ENV_1) + 1, (const uint8_t *) ENV_1); + assert_int_equal(ret, EOK); + + ret = pam_add_response(pd, SSS_PAM_ENV_ITEM, + strlen(ENV_2) + 1, (const uint8_t *) ENV_2); + assert_int_equal(ret, EOK); + + info_type = SSS_PAM_USER_INFO_OFFLINE_AUTH; + memset(offline_auth_data, 0, sizeof(offline_auth_data)); + memcpy(offline_auth_data, &info_type, sizeof(uint32_t)); + ret = pam_add_response(pd, SSS_PAM_USER_INFO, + sizeof(offline_auth_data), offline_auth_data); + assert_int_equal(ret, EOK); + + /* pd->resp_list points to the SSS_PAM_USER_INFO and pd->resp_list->next + * to the SSS_PAM_ENV_ITEM message with KRB5CCNAME and + * pd->resp_list->next->next to the SSS_PAM_ENV_ITEM message with MyEnv. */ + + pam_test_ctx->pctx->rctx = pam_test_ctx->rctx; + + + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); + assert_int_equal(ret, EOK); + assert_true(pd->resp_list->do_not_send_to_client); + assert_false(pd->resp_list->next->do_not_send_to_client); + assert_false(pd->resp_list->next->next->do_not_send_to_client); + + pd->service = discard_const("sudo"); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); + assert_int_equal(ret, EOK); + assert_true(pd->resp_list->do_not_send_to_client); + assert_true(pd->resp_list->next->do_not_send_to_client); + assert_false(pd->resp_list->next->next->do_not_send_to_client); + + pd->service = discard_const("sudo-i"); + ret = filter_responses(pam_test_ctx->pctx, pd->resp_list, pd); + assert_int_equal(ret, EOK); + assert_true(pd->resp_list->do_not_send_to_client); + assert_true(pd->resp_list->next->do_not_send_to_client); + assert_false(pd->resp_list->next->next->do_not_send_to_client); + + talloc_free(pd); +} + static int pam_test_setup_appsvc_posix_dom(void **state) { int ret; @@ -3653,6 +3723,8 @@ int main(int argc, const char *argv[]) cmocka_unit_test_setup_teardown(test_filter_response, pam_test_setup, pam_test_teardown), + cmocka_unit_test_setup_teardown(test_filter_response_defaults, + pam_test_setup, pam_test_teardown), cmocka_unit_test_setup_teardown(test_appsvc_posix_dom, pam_test_setup_appsvc_posix_dom, pam_test_teardown),
_______________________________________________ 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 Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
