From 705e280eafb13b1b55fc0b91001e4721ce79fbdf Mon Sep 17 00:00:00 2001 From: Thomas Woerner Date: Mon, 22 Oct 2018 13:57:11 +0200 Subject: [PATCH] Fix ressource leak in client/config.c get_config_entry The leak happens due to using strndup to create a temporary string without freeing it afterwards. See: https://pagure.io/freeipa/issue/7738 Signed-off-by: Thomas Woerner Reviewed-By: Christian Heimes --- client/config.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/client/config.c b/client/config.c index ecc126ff47..a09564b702 100644 --- a/client/config.c +++ b/client/config.c @@ -123,17 +123,18 @@ get_config_entry(char * in_data, const char *section, const char *key) line++; p = strchr(line, ']'); if (p) { - tmp = strndup(line, p - line); if (in_section) { /* We exited the matching section without a match */ free(data); return NULL; } + tmp = strndup(line, p - line); if (strcmp(section, tmp) == 0) { free(tmp); in_section = 1; continue; } + free(tmp); } } /* [ */ From ebb14ed6f57c5504dc2f44339274b108483efd16 Mon Sep 17 00:00:00 2001 From: Thomas Woerner Date: Mon, 22 Oct 2018 15:18:23 +0200 Subject: [PATCH] Fix ressource leak in daemons/ipa-slapi-plugins/ipa-cldap/ipa_cldap_netlogon.c ipa_cldap_netlogon The leak happens due to using strndup in a for loop to create a temporary string without freeing it in all cases. See: https://pagure.io/freeipa/issue/7738 Signed-off-by: Thomas Woerner Reviewed-By: Christian Heimes --- daemons/ipa-slapi-plugins/ipa-cldap/ipa_cldap_netlogon.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/daemons/ipa-slapi-plugins/ipa-cldap/ipa_cldap_netlogon.c b/daemons/ipa-slapi-plugins/ipa-cldap/ipa_cldap_netlogon.c index 5863f667ea..460f96cd59 100644 --- a/daemons/ipa-slapi-plugins/ipa-cldap/ipa_cldap_netlogon.c +++ b/daemons/ipa-slapi-plugins/ipa-cldap/ipa_cldap_netlogon.c @@ -260,6 +260,10 @@ int ipa_cldap_netlogon(struct ipa_cldap_ctx *ctx, if (req->kvps.pairs[i].value.bv_val[len-1] == '.') { len--; } + if (domain != NULL) { + free(domain); + domain = NULL; + } domain = strndup(req->kvps.pairs[i].value.bv_val, len); if (!domain) { ret = ENOMEM; From 305150416429b85d46ad4162bac492db303cf9cf Mon Sep 17 00:00:00 2001 From: Christian Heimes Date: Wed, 24 Oct 2018 10:12:39 +0200 Subject: [PATCH] Fix ipadb_multires resource handling * ipadb_get_pwd_policy() initializes struct ipadb_multires *res to NULL. * ipadb_multires_free() supports NULL as no-op. * ipadb_multibase_search() consistently frees and NULLs struct ipadb_multires **res on error. See: https://pagure.io/freeipa/issue/7738 Signed-off-by: Christian Heimes Reviewed-By: Alexander Bokovoy --- daemons/ipa-kdb/ipa_kdb_common.c | 13 +++++++++---- daemons/ipa-kdb/ipa_kdb_pwdpolicy.c | 2 +- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/daemons/ipa-kdb/ipa_kdb_common.c b/daemons/ipa-kdb/ipa_kdb_common.c index 5995efe6b1..e2592cea3f 100644 --- a/daemons/ipa-kdb/ipa_kdb_common.c +++ b/daemons/ipa-kdb/ipa_kdb_common.c @@ -634,10 +634,12 @@ krb5_error_code ipadb_multires_init(LDAP *lcontext, struct ipadb_multires **r) void ipadb_multires_free(struct ipadb_multires *r) { - for (int i = 0; i < r->count; i++) { - ldap_msgfree(r->res[i]); + if (r != NULL) { + for (int i = 0; i < r->count; i++) { + ldap_msgfree(r->res[i]); + } + free(r); } - free(r); } LDAPMessage *ipadb_multires_next_entry(struct ipadb_multires *r) @@ -670,8 +672,11 @@ krb5_error_code ipadb_multibase_search(struct ipadb_context *ipactx, if (ret != 0) return ret; ret = ipadb_check_connection(ipactx); - if (ret != 0) + if (ret != 0) { + ipadb_multires_free(*res); + *res = NULL; return ipadb_simple_ldap_to_kerr(ret); + } for (int b = 0; basedns[b]; b++) { LDAPMessage *r; diff --git a/daemons/ipa-kdb/ipa_kdb_pwdpolicy.c b/daemons/ipa-kdb/ipa_kdb_pwdpolicy.c index 1ec584612b..10f128700b 100644 --- a/daemons/ipa-kdb/ipa_kdb_pwdpolicy.c +++ b/daemons/ipa-kdb/ipa_kdb_pwdpolicy.c @@ -141,7 +141,7 @@ krb5_error_code ipadb_get_pwd_policy(krb5_context kcontext, char *name, char *esc_name = NULL; char *src_filter = NULL; krb5_error_code kerr; - struct ipadb_multires *res; + struct ipadb_multires *res = NULL; LDAPMessage *lentry; osa_policy_ent_t pentry = NULL; uint32_t result; From 4ca3120b9a09ad48866446af29b38ca7c005b0d0 Mon Sep 17 00:00:00 2001 From: Christian Heimes Date: Wed, 24 Oct 2018 10:19:14 +0200 Subject: [PATCH] Don't abuse strncpy() length limitation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On two occasions C code abused strncpy()'s length limitation to copy a string of known length without the trailing NULL byte. Recent GCC is raising the compiler warning: warning: ‘strncpy’ output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation] Use memcpy() instead if strncpy() to copy data of known size. See: https://pagure.io/freeipa/issue/7738 Signed-off-by: Christian Heimes Reviewed-By: Alexander Bokovoy --- daemons/ipa-kdb/ipa_kdb.c | 2 +- daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/daemons/ipa-kdb/ipa_kdb.c b/daemons/ipa-kdb/ipa_kdb.c index 00c732624b..20967316ed 100644 --- a/daemons/ipa-kdb/ipa_kdb.c +++ b/daemons/ipa-kdb/ipa_kdb.c @@ -110,7 +110,7 @@ static char *ipadb_realm_to_ldapi_uri(char *realm) /* copy path and escape '/' to '%2f' */ for (q = LDAPIDIR; *q; q++) { if (*q == '/') { - strncpy(p, "%2f", 3); + memcpy(p, "%2f", 3); p += 3; } else { *p = *q; diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c index db7183bf2b..61b46904ab 100644 --- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c +++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c @@ -1003,7 +1003,7 @@ int ipapwd_set_extradata(const char *dn, xdata[5] = (unixtime & 0xff000000) >> 24; /* append the principal name */ - strncpy(&xdata[6], principal, p_len); + memcpy(&xdata[6], principal, p_len); xdata[xd_len -1] = 0; From a06fb8d0f7b7c6aba942186b93d87823398f5337 Mon Sep 17 00:00:00 2001 From: Christian Heimes Date: Thu, 1 Nov 2018 11:41:29 +0100 Subject: [PATCH] has_krbprincipalkey: avoid double free Set keys to NULL after free rder to avoid potential double free. See: https://pagure.io/freeipa/issue/7738 Signed-off-by: Christian Heimes Reviewed-By: Alexander Bokovoy --- daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c index 209d596255..3c3c7e8845 100644 --- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c +++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c @@ -176,7 +176,11 @@ static bool has_krbprincipalkey(Slapi_Entry *entry) { if (rc || (num_keys <= 0)) { /* this one is not valid, ignore it */ - if (keys) ipa_krb5_free_key_data(keys, num_keys); + if (keys) { + ipa_krb5_free_key_data(keys, num_keys); + keys = NULL; + num_keys = 0; + } } else { /* It exists at least this one that is valid, no need to continue */ if (keys) ipa_krb5_free_key_data(keys, num_keys); From 2884ab69babfd7d40f951ba814234ce4763b0cd8 Mon Sep 17 00:00:00 2001 From: Christian Heimes Date: Thu, 1 Nov 2018 11:41:41 +0100 Subject: [PATCH] ipadb_mspac_get_trusted_domains: NULL ptr deref Fix potential NULL pointer deref in ipadb_mspac_get_trusted_domains(). In theory, dn could be empty and rdn NULL. The man page for ldap_str2dn() does not guarantee that it returns a non-empty result. See: https://pagure.io/freeipa/issue/7738 Signed-off-by: Christian Heimes Reviewed-By: Alexander Bokovoy --- daemons/ipa-kdb/ipa_kdb_mspac.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c index 11e036986a..329a5c1158 100644 --- a/daemons/ipa-kdb/ipa_kdb_mspac.c +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c @@ -2586,6 +2586,12 @@ krb5_error_code ipadb_mspac_get_trusted_domains(struct ipadb_context *ipactx) } /* We should have a single AVA in the domain RDN */ + if (rdn == NULL) { + ldap_dnfree(dn); + ret = EINVAL; + goto done; + } + t[n].parent_name = strndup(rdn[0]->la_value.bv_val, rdn[0]->la_value.bv_len); ldap_dnfree(dn); From 28b89df5ed8a9a060227433e8eeebf7eea844bb9 Mon Sep 17 00:00:00 2001 From: Christian Heimes Date: Thu, 1 Nov 2018 11:41:47 +0100 Subject: [PATCH] ipapwd_pre_mod: NULL ptr deref In ipapwd_pre_mod, check userpw for NULL before dereferencing its first element. See: https://pagure.io/freeipa/issue/7738 Signed-off-by: Christian Heimes Reviewed-By: Alexander Bokovoy --- daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c index 3c3c7e8845..9aef2f7d7d 100644 --- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c +++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c @@ -766,7 +766,7 @@ static int ipapwd_pre_mod(Slapi_PBlock *pb) /* Check this is a clear text password, or refuse operation (only if we need * to comput other hashes */ if (! unhashedpw && (gen_krb_keys || is_smb || is_ipant)) { - if ('{' == userpw[0]) { + if ((userpw != NULL) && ('{' == userpw[0])) { if (0 == strncasecmp(userpw, "{CLEAR}", strlen("{CLEAR}"))) { unhashedpw = slapi_ch_strdup(&userpw[strlen("{CLEAR}")]); if (NULL == unhashedpw) { From 5abe3d9feff3c0e66a43fa3799611521f83ee893 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy Date: Wed, 7 Nov 2018 11:57:53 +0200 Subject: [PATCH] ipaserver.install.adtrust: fix CID 323644 Fix Coverity finding CID 323644: logically dead code path The code to determine whether NetBIOS name was already set or need to be set after deriving it from a domain or asking a user for an interactive input, was refactored at some point to avoid retrieving the whole LDAP entry. Instead, it was provided with the actual NetBIOS name retrieved. As result, a part of the code got neglected and was never executed. Fix this code and provide a test that tries to test predefined, interactively provided and automatically derived NetBIOS name depending on how the installer is being run. We mock up the actual execution so that no access to LDAP or Samba is needed. Backport to ipa-4-7 takes into account Python 2.7 differences: - uses mock instead of unittest.mock if the latter is not available - derives ApiMockup from object Fixes: https://pagure.io/freeipa/issue/7753 Reviewed-By: Christian Heimes (cherry picked from commit 82af034023b03ae64f005c8160b9e961e7b9fd55) Reviewed-By: Christian Heimes --- ipaserver/install/adtrust.py | 3 +- .../test_ipaserver/test_adtrust_mockup.py | 58 +++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) create mode 100644 ipatests/test_ipaserver/test_adtrust_mockup.py diff --git a/ipaserver/install/adtrust.py b/ipaserver/install/adtrust.py index e9ae3fa3ed..75194eed8f 100644 --- a/ipaserver/install/adtrust.py +++ b/ipaserver/install/adtrust.py @@ -95,7 +95,6 @@ def set_and_check_netbios_name(netbios_name, unattended, api): cur_netbios_name = None gen_netbios_name = None reset_netbios_name = False - entry = None if api.Backend.ldap2.isconnected(): cur_netbios_name = retrieve_netbios_name(api) @@ -133,7 +132,7 @@ def set_and_check_netbios_name(netbios_name, unattended, api): gen_netbios_name = adtrustinstance.make_netbios_name( api.env.domain) - if entry is not None: + if gen_netbios_name is not None: # Fix existing trust configuration print("Trust is configured but no NetBIOS domain name found, " "setting it now.") diff --git a/ipatests/test_ipaserver/test_adtrust_mockup.py b/ipatests/test_ipaserver/test_adtrust_mockup.py new file mode 100644 index 0000000000..614a06f8c8 --- /dev/null +++ b/ipatests/test_ipaserver/test_adtrust_mockup.py @@ -0,0 +1,58 @@ +# Copyright (C) 2018 FreeIPA Project Contributors - see LICENSE file + +from __future__ import print_function +import ipaserver.install.adtrust as adtr +from ipaserver.install.adtrust import set_and_check_netbios_name +from collections import namedtuple +from unittest import TestCase +try: + from unittest import mock +except ImportError: + import mock +from io import StringIO + + +class ApiMockup(object): + Backend = namedtuple('Backend', 'ldap2') + Calls = namedtuple('Callbacks', 'retrieve_netbios_name') + env = namedtuple('Environment', 'domain') + + +class TestNetbiosName(TestCase): + @classmethod + def setUpClass(cls): + api = ApiMockup() + ldap2 = namedtuple('LDAP', 'isconnected') + ldap2.isconnected = mock.MagicMock(return_value=True) + api.Backend.ldap2 = ldap2 + api.Calls.retrieve_netbios_name = adtr.retrieve_netbios_name + adtr.retrieve_netbios_name = mock.MagicMock(return_value=None) + cls.api = api + + @classmethod + def tearDownClass(cls): + adtr.retrieve_netbios_name = cls.api.Calls.retrieve_netbios_name + + def test_NetbiosName(self): + """ + Test set_and_check_netbios_name() using permutation of two inputs: + - predefined and not defined NetBIOS name + - unattended and interactive run + As result, the function has to return expected NetBIOS name in + all cases. For interactive run we override input to force what + we expect. + """ + self.api.env.domain = 'example.com' + expected_nname = 'EXAMPLE' + # NetBIOS name, unattended, should set the name? + tests = ((expected_nname, True, False), + (None, True, True), + (None, False, True), + (expected_nname, False, False)) + with mock.patch('sys.stdin', new_callable=StringIO) as stdin: + stdin.write(expected_nname + '\r') + for test in tests: + nname, setname = set_and_check_netbios_name( + test[0], test[1], self.api) + assert expected_nname == nname + assert setname == test[2] From 48a6048be2a3c6cf496a67a2732b8aaee91af620 Mon Sep 17 00:00:00 2001 From: Christian Heimes Date: Thu, 8 Nov 2018 10:42:43 +0100 Subject: [PATCH] Copy-paste error in permssions plugin, CID 323649 Address a bug in the code block for attributeLevelRights for old clients. The backward compatibility code for deprecated options was not triggered, because the new name was checked against wrong dict. Coverity Scan issue 323649, Copy-paste error The copied code will not have its intended effect. In postprocess_result: A copied piece of code is inconsistent with the original (CWE-398) See: Fixes: https://pagure.io/freeipa/issue/7753 Signed-off-by: Christian Heimes Reviewed-By: Alexander Bokovoy --- ipaserver/plugins/permission.py | 2 +- ipatests/test_xmlrpc/test_old_permission_plugin.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ipaserver/plugins/permission.py b/ipaserver/plugins/permission.py index 2127d8234e..8ffe01bd88 100644 --- a/ipaserver/plugins/permission.py +++ b/ipaserver/plugins/permission.py @@ -486,7 +486,7 @@ def postprocess_result(self, entry, options): if old_client: for old_name, new_name in _DEPRECATED_OPTION_ALIASES.items(): - if new_name in entry: + if new_name in rights: rights[old_name] = rights[new_name] del rights[new_name] diff --git a/ipatests/test_xmlrpc/test_old_permission_plugin.py b/ipatests/test_xmlrpc/test_old_permission_plugin.py index 6d1117b6b3..600e449421 100644 --- a/ipatests/test_xmlrpc/test_old_permission_plugin.py +++ b/ipatests/test_xmlrpc/test_old_permission_plugin.py @@ -73,8 +73,8 @@ 'ipapermbindruletype': u'rscwo', 'ipapermdefaultattr': u'rscwo', 'ipapermexcludedattr': u'rscwo', - 'ipapermlocation': u'rscwo', - 'ipapermright': u'rscwo', + 'subtree': u'rscwo', # old + 'permissions': u'rscwo', # old 'ipapermtarget': u'rscwo', 'ipapermtargetfilter': u'rscwo', 'ipapermtargetto': u'rscwo',