URL: https://github.com/SSSD/sssd/pull/5494 Author: abbra Title: #5494: pam_sss_gss: support authentication indicators Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5494/head:pr5494 git checkout pr5494
From b48633f72f997a3674cf5ea8bcd453ff18191edf Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Fri, 5 Feb 2021 20:36:27 +0200 Subject: [PATCH 1/5] pam_sss_gss: support authentication indicators MIT Kerberos allows to associate authentication indicators with the issued ticket based on the way how the TGT was obtained. The indicators present in the TGT then copied to service tickets. There are two ways to check the authentication indicators: - when KDC issues a service ticket, a policy at KDC side can reject the ticket issuance based on a lack of certain indicator - when a server application presented with a service ticket from a client, it can verify that this ticket contains intended authentication indicators before authorizing access from the client. Add support to validate presence of a specific (set of) authentication indicator(s) in pam_sss_gss when validating a user's TGT. This concept can be used to only allow access to a PAM service when user is in possession of a ticket obtained using some of pre-authentication mechanisms that require multiple factors: smart-cards (PKINIT), 2FA tokens (otp/radius), etc. Resolves: https://github.com/SSSD/sssd/issues/5482 Signed-off-by: Alexander Bokovoy <aboko...@redhat.com> --- src/confdb/confdb.c | 13 ++ src/confdb/confdb.h | 3 + src/config/SSSDConfig/sssdoptions.py | 2 + src/config/SSSDConfigTest.py | 6 +- src/config/cfg_rules.ini | 3 + src/config/etc/sssd.api.conf | 2 + src/db/sysdb_subdomains.c | 11 ++ src/man/pam_sss_gss.8.xml | 13 ++ src/man/sssd.conf.5.xml | 64 +++++++ src/responder/pam/pamsrv.c | 21 +++ src/responder/pam/pamsrv.h | 2 + src/responder/pam/pamsrv_gssapi.c | 250 +++++++++++++++++++++++++++ 12 files changed, 388 insertions(+), 2 deletions(-) diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c index befcfff2db..cca76159bc 100644 --- a/src/confdb/confdb.c +++ b/src/confdb/confdb.c @@ -1603,6 +1603,19 @@ static int confdb_get_domain_internal(struct confdb_ctx *cdb, } } + tmp = ldb_msg_find_attr_as_string(res->msgs[0], + CONFDB_PAM_GSSAPI_INDICATORS_MAP, + NULL); + if (tmp != NULL && tmp[0] != '\0') { + ret = split_on_separator(domain, tmp, ',', true, true, + &domain->gssapi_indicators_map, NULL); + if (ret != 0) { + DEBUG(SSSDBG_FATAL_FAILURE, + "Cannot parse %s\n", CONFDB_PAM_GSSAPI_INDICATORS_MAP); + goto done; + } + } + domain->has_views = false; domain->view_name = NULL; diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index 036f9ecadf..a2be227ddd 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -146,6 +146,7 @@ #define CONFDB_PAM_INITGROUPS_SCHEME "pam_initgroups_scheme" #define CONFDB_PAM_GSSAPI_SERVICES "pam_gssapi_services" #define CONFDB_PAM_GSSAPI_CHECK_UPN "pam_gssapi_check_upn" +#define CONFDB_PAM_GSSAPI_INDICATORS_MAP "pam_gssapi_indicators_map" /* SUDO */ #define CONFDB_SUDO_CONF_ENTRY "config/sudo" @@ -437,6 +438,8 @@ struct sss_domain_info { /* List of PAM services that are allowed to authenticate with GSSAPI. */ char **gssapi_services; char *gssapi_check_upn; /* true | false | NULL */ + /* List of indicators associated with the specific PAM service */ + char **gssapi_indicators_map; }; /** diff --git a/src/config/SSSDConfig/sssdoptions.py b/src/config/SSSDConfig/sssdoptions.py index fb9a9aa43c..5d9946ba8f 100644 --- a/src/config/SSSDConfig/sssdoptions.py +++ b/src/config/SSSDConfig/sssdoptions.py @@ -106,6 +106,8 @@ def __init__(self): 'pam_initgroups_scheme' : _('When shall the PAM responder force an initgroups request'), 'pam_gssapi_services' : _('List of PAM services that are allowed to authenticate with GSSAPI.'), 'pam_gssapi_check_upn' : _('Whether to match authenticated UPN with target user'), + 'pam_gssapi_indicators_map' : _('List of pairs <PAM service>:<authentication indicator> that ' + 'must be enforced for PAM access with GSSAPI authentication'), # [sudo] 'sudo_timed': _('Whether to evaluate the time-based attributes in sudo rules'), diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py index 6a95e63dd1..04c4b35baa 100755 --- a/src/config/SSSDConfigTest.py +++ b/src/config/SSSDConfigTest.py @@ -655,7 +655,8 @@ def testListOptions(self): 'cached_auth_timeout', 'auto_private_groups', 'pam_gssapi_services', - 'pam_gssapi_check_upn'] + 'pam_gssapi_check_upn', + 'pam_gssapi_indicators_map'] self.assertTrue(type(options) == dict, "Options should be a dictionary") @@ -1036,7 +1037,8 @@ def testRemoveProvider(self): 'cached_auth_timeout', 'auto_private_groups', 'pam_gssapi_services', - 'pam_gssapi_check_upn'] + 'pam_gssapi_check_upn', + 'pam_gssapi_indicators_map'] self.assertTrue(type(options) == dict, "Options should be a dictionary") diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index 49ceb9a2ce..bf2d03b824 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -141,6 +141,7 @@ option = p11_uri option = pam_initgroups_scheme option = pam_gssapi_services option = pam_gssapi_check_upn +option = pam_gssapi_indicators_map [rule/allowed_sudo_options] validator = ini_allowed_options @@ -441,6 +442,7 @@ option = re_expression option = auto_private_groups option = pam_gssapi_services option = pam_gssapi_check_upn +option = pam_gssapi_indicators_map #Entry cache timeouts option = entry_cache_user_timeout @@ -838,6 +840,7 @@ option = use_fully_qualified_names option = auto_private_groups option = pam_gssapi_services option = pam_gssapi_check_upn +option = pam_gssapi_indicators_map [rule/sssd_checks] validator = sssd_checks diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf index d3cad73809..49ced63859 100644 --- a/src/config/etc/sssd.api.conf +++ b/src/config/etc/sssd.api.conf @@ -82,6 +82,7 @@ p11_uri = str, None, false pam_initgroups_scheme = str, None, false pam_gssapi_services = str, None, false pam_gssapi_check_upn = bool, None, false +pam_gssapi_indicators_map = str, None, false [sudo] # sudo service @@ -203,6 +204,7 @@ re_expression = str, None, false auto_private_groups = str, None, false pam_gssapi_services = str, None, false pam_gssapi_check_upn = bool, None, false +pam_gssapi_indicators_map = str, None, false #Entry cache timeouts entry_cache_user_timeout = int, None, false diff --git a/src/db/sysdb_subdomains.c b/src/db/sysdb_subdomains.c index c0a676d491..675d41c354 100644 --- a/src/db/sysdb_subdomains.c +++ b/src/db/sysdb_subdomains.c @@ -273,6 +273,17 @@ check_subdom_config_file(struct confdb_ctx *confdb, goto done; } + /* allow to set pam_gssapi_indicators_map */ + ret = confdb_get_string_as_list(confdb, subdomain, sd_conf_path, + CONFDB_PAM_GSSAPI_INDICATORS_MAP, + &subdomain->gssapi_indicators_map); + if (ret != EOK && ret != ENOENT) { + DEBUG(SSSDBG_OP_FAILURE, + "Failed to get %s option for the subdomain: %s\n", + CONFDB_PAM_GSSAPI_INDICATORS_MAP, subdomain->name); + goto done; + } + /* case_sensitive=Preserving */ ret = confdb_get_string(confdb, tmp_ctx, sd_conf_path, CONFDB_DOMAIN_CASE_SENSITIVE, NULL, diff --git a/src/man/pam_sss_gss.8.xml b/src/man/pam_sss_gss.8.xml index ce5b11bff0..a83369de28 100644 --- a/src/man/pam_sss_gss.8.xml +++ b/src/man/pam_sss_gss.8.xml @@ -70,6 +70,19 @@ <manvolnum>5</manvolnum> </citerefentry> for more details on these options. </para> + <para> + Some Kerberos deployments allow to assocate authentication + indicators with a particular pre-authentication method used to + obtain the ticket granting ticket by the user. + <command>pam_sss_gss.so</command> allows to enforce presence of + authentication indicators in the service tickets before a particular + PAM service can be accessed. + </para> + <para> + If <option>pam_gssapi_indicators_map</option> is set in the [pam] or + domain section of sssd.conf, then SSSD will perform a check of the + presence of any configured indicators in the service ticket. + </para> </refsect1> <refsect1 id='options'> diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index 3ac1642782..42659dffe6 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -1771,6 +1771,70 @@ pam_gssapi_services = sudo, sudo-i </para> </listitem> </varlistentry> + <varlistentry> + <term>pam_gssapi_indicators_map</term> + <listitem> + <para> + Comma separated list of authentication indicators required + to be present in a Kerberos ticket to access a PAM service + that is allowed to try GSSAPI authentication using + pam_sss_gss.so module. + </para> + <para> + Each element of the list can be either an authentication indicator + name or a pair <quote>service:indicator</quote>. Indicators not + prefixed with the PAM service name will be required to access any + PAM service configured to be used with + <option>pam_gssapi_services</option>. A resulting list of indicators + per PAM service is then checked against indicators in the Kerberos + ticket during authentication by pam_sss_gss.so. Any indicator from the + ticket that matches the resulting list of indicators for the PAM service + would grant access. If none of the indicators in the list match, access + will be denied. If the resulting list of indicators for the PAM service + is empty, the check will not prevent the access. + </para> + <para> + To disable GSSAPI authentication indicator check, set this option + to <quote>-</quote> (dash). To disable the check for a specific PAM + service, add <quote>service:-</quote>. + </para> + <para> + Note: This option can also be set per-domain which + overwrites the value in [pam] section. It can also + be set for trusted domain which overwrites the value + in the domain section. + </para> + <para> + Following authentication indicators are supported by IPA Kerberos deployments: + <itemizedlist> + <listitem> + <para>pkinit -- pre-authentication using X.509 certificates -- whether stored in files or on smart cards.</para> + </listitem> + <listitem> + <para>hardened -- SPAKE pre-authentication or any pre-authentication wrapped in a FAST channel.</para> + </listitem> + <listitem> + <para>radius -- pre-authentication with the help of a RADIUS server.</para> + </listitem> + <listitem> + <para>otp -- pre-authentication using integrated two-factor authentication (2FA or one-time password, OTP) in IPA.</para> + </listitem> + </itemizedlist> + </para> + <para> + Example: to require access to SUDO services only + for users which obtained their Kerberos tickets + with a X.509 certificate pre-authentication + (PKINIT), set + <programlisting> +pam_gssapi_indicators_map = sudo:pkinit, sudo-i:pkinit + </programlisting> + </para> + <para> + Default: not set (use of authentication indicators is not required) + </para> + </listitem> + </varlistentry> </variablelist> </refsect2> diff --git a/src/responder/pam/pamsrv.c b/src/responder/pam/pamsrv.c index 65370662db..f6bed9cf07 100644 --- a/src/responder/pam/pamsrv.c +++ b/src/responder/pam/pamsrv.c @@ -370,6 +370,27 @@ static int pam_process_init(TALLOC_CTX *mem_ctx, goto done; } + ret = confdb_get_string(pctx->rctx->cdb, pctx, CONFDB_PAM_CONF_ENTRY, + CONFDB_PAM_GSSAPI_INDICATORS_MAP, "-", &tmpstr); + if (ret != EOK) { + DEBUG(SSSDBG_FATAL_FAILURE, + "Failed to determine gssapi services.\n"); + goto done; + } + DEBUG(SSSDBG_TRACE_INTERNAL, "Found value [%s] for option [%s].\n", tmpstr, + CONFDB_PAM_GSSAPI_INDICATORS_MAP); + + if (tmpstr != NULL) { + ret = split_on_separator(pctx, tmpstr, ',', true, true, + &pctx->gssapi_indicators_map, NULL); + if (ret != EOK) { + DEBUG(SSSDBG_MINOR_FAILURE, + "split_on_separator() failed [%d]: [%s].\n", ret, + sss_strerror(ret)); + goto done; + } + } + /* The responder is initialized. Now tell it to the monitor. */ ret = sss_monitor_service_init(rctx, rctx->ev, SSS_BUS_PAM, SSS_PAM_SBUS_SERVICE_NAME, diff --git a/src/responder/pam/pamsrv.h b/src/responder/pam/pamsrv.h index 3553296918..383c7bee6c 100644 --- a/src/responder/pam/pamsrv.h +++ b/src/responder/pam/pamsrv.h @@ -65,6 +65,8 @@ struct pam_ctx { /* List of PAM services that are allowed to authenticate with GSSAPI. */ char **gssapi_services; + /* List of authentication indicators associated with a PAM service */ + char **gssapi_indicators_map; bool gssapi_check_upn; }; diff --git a/src/responder/pam/pamsrv_gssapi.c b/src/responder/pam/pamsrv_gssapi.c index 2d05c7888a..e4da4c4ebe 100644 --- a/src/responder/pam/pamsrv_gssapi.c +++ b/src/responder/pam/pamsrv_gssapi.c @@ -24,6 +24,7 @@ #include <gssapi/gssapi_krb5.h> #include <stdint.h> #include <stdlib.h> +#include <string.h> #include <talloc.h> #include <ldb.h> @@ -83,6 +84,117 @@ static bool pam_gssapi_should_check_upn(struct pam_ctx *pam_ctx, return pam_ctx->gssapi_check_upn; } +static int pam_gssapi_check_indicators(TALLOC_CTX *mem_ctx, + const char *pam_service, + char **gssapi_indicators_map, + char **indicators) +{ + char *authind = NULL; + size_t pam_len = strlen(pam_service); + char **map = gssapi_indicators_map; + char **result = NULL; + int res; + + authind = talloc_strdup(mem_ctx, ""); + if (authind == NULL) { + return ENOMEM; + } + + for (int i = 0; map[i]; i++) { + if (map[i][0] == '-') { + DEBUG(SSSDBG_TRACE_FUNC, + "Indicators aren't used for [%s]\n", + pam_service); + talloc_free(authind); + return EOK; + } + if (!strchr(map[i], ':')) { + authind = talloc_asprintf_append(authind, "%s ", map[i]); + if (authind == NULL) { + /* Since we allocate on pam_ctx, caller will free it */ + return ENOMEM; + } + continue; + } + + res = strncmp(map[i], pam_service, pam_len); + if (res == 0) { + if (strlen(map[i]) > pam_len) { + if (map[i][pam_len] != ':') { + /* different PAM service, skip it */ + continue; + } + + if (map[i][pam_len + 1] == '-') { + DEBUG(SSSDBG_TRACE_FUNC, + "Indicators aren't used for [%s]\n", + pam_service); + talloc_free(authind); + return EOK; + } + + authind = talloc_asprintf_append(authind, "%s ", + map[i] + (pam_len + 1)); + if (authind == NULL) { + /* Since we allocate on pam_ctx, caller will free it */ + return ENOMEM; + } + } else { + DEBUG(SSSDBG_MINOR_FAILURE, "Invalid value for %s: [%s]\n", + CONFDB_PAM_GSSAPI_INDICATORS_MAP, map[i]); + talloc_free(authind); + return EINVAL; + } + } + } + + res = ENOENT; + map = NULL; + + if (authind[0] == '\0') { + /* empty list of per-service indicators -> skip */ + goto done; + } + + /* trim a space after the final indicator + * to prevent split_on_separator() to fail */ + authind[strlen(authind) - 1] = '\0'; + + res = split_on_separator(mem_ctx, authind, ' ', true, true, + &map, NULL); + if (res != 0) { + DEBUG(SSSDBG_FATAL_FAILURE, + "Cannot parse list of indicators: [%s]\n", authind); + res = EINVAL; + goto done; + } + + res = diff_string_lists(mem_ctx, indicators, map, NULL, NULL, &result); + if (res != 0) { + DEBUG(SSSDBG_FATAL_FAILURE,"Cannot diff lists of indicators\n"); + res = EINVAL; + goto done; + } + + if (result && result[0] != NULL) { + for (int i = 0; result[i]; i++) { + DEBUG(SSSDBG_TRACE_FUNC, + "indicator [%s] is allowed for PAM service [%s]\n", + result[i], pam_service); + } + res = EOK; + goto done; + } + + res = EPERM; + +done: + talloc_free(result); + talloc_free(authind); + talloc_free(map); + return res; +} + static bool pam_gssapi_allowed(struct pam_ctx *pam_ctx, struct sss_domain_info *domain, const char *service) @@ -385,12 +497,126 @@ static char *gssapi_get_name(TALLOC_CTX *mem_ctx, gss_name_t gss_name) return exported; } +#define AUTH_INDICATORS_TAG "auth-indicators" + +static char **gssapi_get_indicators(TALLOC_CTX *mem_ctx, gss_name_t gss_name) +{ + gss_buffer_set_t attrs = GSS_C_NO_BUFFER_SET; + int is_mechname; + OM_uint32 major; + OM_uint32 minor; + gss_buffer_desc value = GSS_C_EMPTY_BUFFER; + gss_buffer_desc display_value = GSS_C_EMPTY_BUFFER; + char *exported = NULL; + char **map = NULL; + int res; + + major = gss_inquire_name(&minor, gss_name, &is_mechname, NULL, &attrs); + if (major != GSS_S_COMPLETE) { + DEBUG(SSSDBG_CRIT_FAILURE, "Unable to inquire name\n"); + return NULL; + } + + if (attrs == GSS_C_NO_BUFFER_SET) { + DEBUG(SSSDBG_TRACE_FUNC, "No krb5 attributes in the ticket\n"); + return NULL; + } + + exported = talloc_strdup(mem_ctx, ""); + if (exported == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Unable to pre-allocate indicators\n"); + goto done; + } + + for (int i = 0; i < attrs->count; i++) { + int authenticated = 0; + int complete = 0; + int more = -1; + + /* skip anything but auth-indicators */ + if (strncmp(AUTH_INDICATORS_TAG, attrs->elements[i].value, + sizeof(AUTH_INDICATORS_TAG) - 1) != 0) + continue; + + /* retrieve all indicators */ + while (more != 0) { + value.value = NULL; + display_value.value = NULL; + + major = gss_get_name_attribute(&minor, gss_name, + &attrs->elements[i], + &authenticated, + &complete, &value, + &display_value, + &more); + if (major != GSS_S_COMPLETE) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Unable to retrieve an attribute\n"); + goto done; + } + + if ((value.value != NULL) && authenticated) { + DEBUG(SSSDBG_TRACE_FUNC, + "attribute's [%.*s] value [%.*s] authenticated\n", + (int) attrs->elements[i].length, + (char*) attrs->elements[i].value, + (int) value.length, + (char*) value.value); + exported = talloc_asprintf_append(exported, "%.*s ", + (int) value.length, + (char*) value.value); + } + + if (exported == NULL) { + /* Since we allocate on mem_ctx, caller will free + * the previous version of 'exported' */ + DEBUG(SSSDBG_CRIT_FAILURE, + "Unable to collect an attribute value\n"); + goto done; + } + (void) gss_release_buffer(&minor, &value); + (void) gss_release_buffer(&minor, &display_value); + } + } + + if (exported[0] != '\0') { + /* trim a space after the final indicator + * to prevent split_on_separator() to fail */ + exported[strlen(exported) - 1] = '\0'; + } else { + /* empty list */ + goto done; + } + + res = split_on_separator(mem_ctx, exported, ' ', true, true, + &map, NULL); + if (res != 0) { + DEBUG(SSSDBG_FATAL_FAILURE, + "Cannot parse list of indicators: [%s]\n", exported); + goto done; + } else { + DEBUG(SSSDBG_TRACE_FUNC, "authentication indicators: [%s]\n", + exported); + } + +done: + (void) gss_release_buffer(&minor, &value); + (void) gss_release_buffer(&minor, &display_value); + (void) gss_release_buffer_set(&minor, &attrs); + + talloc_free(exported); + return map; +} + + struct gssapi_state { struct cli_ctx *cli_ctx; struct sss_domain_info *domain; const char *username; char *authenticated_upn; + char **auth_indicators; bool established; gss_ctx_id_t ctx; }; @@ -568,6 +794,8 @@ gssapi_handshake(struct gssapi_state *state, DEBUG(SSSDBG_TRACE_FUNC, "Security context established with [%s]\n", state->authenticated_upn); + state->auth_indicators = gssapi_get_indicators(state, client_name); + state->established = true; ret = EOK; @@ -632,6 +860,7 @@ pam_cmd_gssapi_sec_ctx(struct cli_ctx *cli_ctx) const char *domain_name; const char *username; char *target; + char **indicators_map = NULL; size_t gss_data_len; uint8_t *gss_data; errno_t ret; @@ -699,6 +928,27 @@ pam_cmd_gssapi_sec_ctx(struct cli_ctx *cli_ctx) goto done; } + /* Use map for auth-indicators from the domain, if defined and + * fallback to the [pam] section otherwise */ + indicators_map = domain->gssapi_indicators_map ? + domain->gssapi_indicators_map : + (pam_ctx->gssapi_indicators_map ? + pam_ctx->gssapi_indicators_map : NULL); + if (indicators_map != NULL) { + ret = pam_gssapi_check_indicators(state, + pam_service, + indicators_map, + state->auth_indicators); + DEBUG(SSSDBG_TRACE_FUNC, + "Check if acquired service ticket has req. indicators: %d\n", + ret); + if ((ret == EPERM) || (ret == ENOMEM) || (ret == EINVAL)) { + /* skip further checks if denied or no memory, + * ENOENT means the check is not applicable */ + goto done; + } + } + if (!pam_gssapi_should_check_upn(pam_ctx, domain)) { /* We are done. */ goto done; From c1eeed021ab3792c877688f934e7477fb9ad6024 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Tue, 9 Feb 2021 21:52:19 +0200 Subject: [PATCH 2/5] prompt config: fix covscan errors Covscan is confused by dangling pointers in arrays after freeing. Its analyzer may decide to visit already visited list elements and since they weren't NULL-ed, it may consider double-free to happen in the code. Signed-off-by: Alexander Bokovoy <aboko...@redhat.com> --- src/sss_client/pam_sss_prompt_config.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/sss_client/pam_sss_prompt_config.c b/src/sss_client/pam_sss_prompt_config.c index 35094b4068..1c67fb18fd 100644 --- a/src/sss_client/pam_sss_prompt_config.c +++ b/src/sss_client/pam_sss_prompt_config.c @@ -98,6 +98,7 @@ static void pc_free_password(struct prompt_config *pc) { if (pc != NULL && pc_get_type(pc) == PC_TYPE_PASSWORD) { free(pc->data.password.prompt); + pc->data.password.prompt = NULL; } return; } @@ -106,7 +107,9 @@ static void pc_free_2fa(struct prompt_config *pc) { if (pc != NULL && pc_get_type(pc) == PC_TYPE_2FA) { free(pc->data.two_fa.prompt_1st); + pc->data.two_fa.prompt_1st = NULL; free(pc->data.two_fa.prompt_2nd); + pc->data.two_fa.prompt_2nd = NULL; } return; } @@ -115,6 +118,7 @@ static void pc_free_2fa_single(struct prompt_config *pc) { if (pc != NULL && pc_get_type(pc) == PC_TYPE_2FA_SINGLE) { free(pc->data.two_fa_single.prompt); + pc->data.two_fa_single.prompt = NULL; } return; } @@ -123,6 +127,7 @@ static void pc_free_sc_pin(struct prompt_config *pc) { if (pc != NULL && pc_get_type(pc) == PC_TYPE_SC_PIN) { free(pc->data.sc_pin.prompt); + pc->data.sc_pin.prompt = NULL; } return; } @@ -153,6 +158,7 @@ void pc_list_free(struct prompt_config **pc_list) return; } free(pc_list[c]); + pc_list[c] = NULL; } free(pc_list); } @@ -541,6 +547,7 @@ errno_t pc_list_from_response(int size, uint8_t *buf, done: if (ret != EOK) { pc_list_free(pl); + pl = NULL; } return ret; From 62fe354fa4cf020be2bb8fdff3bbb06e362084c2 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Tue, 9 Feb 2021 22:02:46 +0200 Subject: [PATCH 3/5] pam_sss: free env_item when not needed Signed-off-by: Alexander Bokovoy <aboko...@redhat.com> --- src/sss_client/pam_sss.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c index fa5a9694b8..aa0234c3ae 100644 --- a/src/sss_client/pam_sss.c +++ b/src/sss_client/pam_sss.c @@ -996,7 +996,7 @@ static int eval_response(pam_handle_t *pamh, size_t buflen, uint8_t *buf, { int ret; size_t p=0; - char *env_item; + char *env_item = NULL; int32_t c; int32_t type; int32_t len; @@ -1020,6 +1020,7 @@ static int eval_response(pam_handle_t *pamh, size_t buflen, uint8_t *buf, while(c>0) { if (buflen < (p+2*sizeof(int32_t))) { D(("response buffer is too small")); + free(env_item); return PAM_BUF_ERR; } @@ -1031,6 +1032,7 @@ static int eval_response(pam_handle_t *pamh, size_t buflen, uint8_t *buf, if (buflen < (p + len)) { D(("response buffer is too small")); + free(env_item); return PAM_BUF_ERR; } @@ -1207,6 +1209,9 @@ static int eval_response(pam_handle_t *pamh, size_t buflen, uint8_t *buf, p += len; --c; + + free(env_item); + env_item = NULL; } return PAM_SUCCESS; @@ -2139,7 +2144,7 @@ static void eval_argv(pam_handle_t *pamh, int argc, const char **argv, static int prompt_by_config(pam_handle_t *pamh, struct pam_items *pi) { size_t c; - int ret; + int ret = PAM_SYSTEM_ERR; if (pi->pc == NULL || *pi->pc == NULL) { return PAM_SYSTEM_ERR; From b2e0e5952fd9401c45c0ea28e7dde1fbbfe2c1c6 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Wed, 10 Feb 2021 09:17:08 +0200 Subject: [PATCH 4/5] covscan: initialize ret variable before use covscan does consider 'ret' unitialized even though GET_ATTR/GET_ATTR_ARRAY macros have explicit and unconditional assignment to ret. This is confusing but causing actual failures in covscan runs. Signed-off-by: Alexander Bokovoy <aboko...@redhat.com> --- src/lib/sifp/sss_sifp_attrs.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/lib/sifp/sss_sifp_attrs.c b/src/lib/sifp/sss_sifp_attrs.c index 1004252e3b..ed24e84d2d 100644 --- a/src/lib/sifp/sss_sifp_attrs.c +++ b/src/lib/sifp/sss_sifp_attrs.c @@ -96,7 +96,7 @@ sss_sifp_find_attr_as_bool(sss_sifp_attr **attrs, const char *name, bool *_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; GET_ATTR(attrs, name, SSS_SIFP_ATTR_TYPE_BOOL, boolean, *_value, ret); return ret; } @@ -106,7 +106,7 @@ sss_sifp_find_attr_as_int16(sss_sifp_attr **attrs, const char *name, int16_t *_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; GET_ATTR(attrs, name, SSS_SIFP_ATTR_TYPE_INT16, int16, *_value, ret); return ret; } @@ -116,7 +116,7 @@ sss_sifp_find_attr_as_uint16(sss_sifp_attr **attrs, const char *name, uint16_t *_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; GET_ATTR(attrs, name, SSS_SIFP_ATTR_TYPE_UINT16, uint16, *_value, ret); return ret; } @@ -126,7 +126,7 @@ sss_sifp_find_attr_as_int32(sss_sifp_attr **attrs, const char *name, int32_t *_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; GET_ATTR(attrs, name, SSS_SIFP_ATTR_TYPE_INT32, int32, *_value, ret); return ret; } @@ -136,7 +136,7 @@ sss_sifp_find_attr_as_uint32(sss_sifp_attr **attrs, const char *name, uint32_t *_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; GET_ATTR(attrs, name, SSS_SIFP_ATTR_TYPE_UINT32, uint32, *_value, ret); return ret; } @@ -146,7 +146,7 @@ sss_sifp_find_attr_as_int64(sss_sifp_attr **attrs, const char *name, int64_t *_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; GET_ATTR(attrs, name, SSS_SIFP_ATTR_TYPE_INT64, int64, *_value, ret); return ret; } @@ -156,7 +156,7 @@ sss_sifp_find_attr_as_uint64(sss_sifp_attr **attrs, const char *name, uint64_t *_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; GET_ATTR(attrs, name, SSS_SIFP_ATTR_TYPE_UINT64, uint64, *_value, ret); return ret; } @@ -166,7 +166,7 @@ sss_sifp_find_attr_as_string(sss_sifp_attr **attrs, const char *name, const char **_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; const char *value = NULL; GET_ATTR(attrs, name, SSS_SIFP_ATTR_TYPE_STRING, str, value, ret); @@ -219,7 +219,7 @@ sss_sifp_find_attr_as_bool_array(sss_sifp_attr **attrs, unsigned int *_num_values, bool **_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; GET_ATTR_ARRAY(attrs, name, SSS_SIFP_ATTR_TYPE_BOOL, boolean, *_num_values, *_value, ret); return ret; @@ -231,7 +231,7 @@ sss_sifp_find_attr_as_int16_array(sss_sifp_attr **attrs, unsigned int *_num_values, int16_t **_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; GET_ATTR_ARRAY(attrs, name, SSS_SIFP_ATTR_TYPE_INT16, int16, *_num_values, *_value, ret); return ret; @@ -243,7 +243,7 @@ sss_sifp_find_attr_as_uint16_array(sss_sifp_attr **attrs, unsigned int *_num_values, uint16_t **_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; GET_ATTR_ARRAY(attrs, name, SSS_SIFP_ATTR_TYPE_UINT16, uint16, *_num_values, *_value, ret); return ret; @@ -255,7 +255,7 @@ sss_sifp_find_attr_as_int32_array(sss_sifp_attr **attrs, unsigned int *_num_values, int32_t **_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; GET_ATTR_ARRAY(attrs, name, SSS_SIFP_ATTR_TYPE_INT32, int32, *_num_values, *_value, ret); return ret; @@ -267,7 +267,7 @@ sss_sifp_find_attr_as_uint32_array(sss_sifp_attr **attrs, unsigned int *_num_values, uint32_t **_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; GET_ATTR_ARRAY(attrs, name, SSS_SIFP_ATTR_TYPE_UINT32, uint32, *_num_values, *_value, ret); return ret; @@ -279,7 +279,7 @@ sss_sifp_find_attr_as_int64_array(sss_sifp_attr **attrs, unsigned int *_num_values, int64_t **_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; GET_ATTR_ARRAY(attrs, name, SSS_SIFP_ATTR_TYPE_INT64, int64, *_num_values, *_value, ret); return ret; @@ -291,7 +291,7 @@ sss_sifp_find_attr_as_uint64_array(sss_sifp_attr **attrs, unsigned int *_num_values, uint64_t **_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; GET_ATTR_ARRAY(attrs, name, SSS_SIFP_ATTR_TYPE_UINT64, uint64, *_num_values, *_value, ret); return ret; @@ -303,7 +303,7 @@ sss_sifp_find_attr_as_string_array(sss_sifp_attr **attrs, unsigned int *_num_values, const char * const **_value) { - sss_sifp_error ret; + sss_sifp_error ret = SSS_SIFP_ATTR_MISSING; char **value; GET_ATTR_ARRAY(attrs, name, SSS_SIFP_ATTR_TYPE_STRING, str, From 9e924bb53bf2d2747cc19bff1e1b372085ed5bd9 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Wed, 10 Feb 2021 09:20:45 +0200 Subject: [PATCH 5/5] covscan: symlink() expects non-NULL second argument There are no checks that talloc()-ed symlink file name is not NULL. Signed-off-by: Alexander Bokovoy <aboko...@redhat.com> --- src/sbus/server/sbus_server.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/sbus/server/sbus_server.c b/src/sbus/server/sbus_server.c index 69efd739b4..4ada9a2599 100644 --- a/src/sbus/server/sbus_server.c +++ b/src/sbus/server/sbus_server.c @@ -146,6 +146,10 @@ sbus_server_symlink_create(const char *filename, { errno_t ret; + if (symlink_filename == NULL) { + return ENOMEM; + } + DEBUG(SSSDBG_TRACE_LIBS, "Symlinking the dbus path %s to a link %s\n", filename, symlink_filename); errno = 0;
_______________________________________________ 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
