From 705e280eafb13b1b55fc0b91001e4721ce79fbdf Mon Sep 17 00:00:00 2001
From: Thomas Woerner <twoerner@redhat.com>
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 <twoerner@redhat.com>
Reviewed-By: Christian Heimes <cheimes@redhat.com>
---
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 <twoerner@redhat.com>
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 <twoerner@redhat.com>
Reviewed-By: Christian Heimes <cheimes@redhat.com>
---
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 <cheimes@redhat.com>
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 <cheimes@redhat.com>
Reviewed-By: Alexander Bokovoy <abokovoy@redhat.com>
---
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 <cheimes@redhat.com>
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 <cheimes@redhat.com>
Reviewed-By: Alexander Bokovoy <abokovoy@redhat.com>
---
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 <cheimes@redhat.com>
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 <cheimes@redhat.com>
Reviewed-By: Alexander Bokovoy <abokovoy@redhat.com>
---
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 <cheimes@redhat.com>
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 <cheimes@redhat.com>
Reviewed-By: Alexander Bokovoy <abokovoy@redhat.com>
---
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 <cheimes@redhat.com>
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 <cheimes@redhat.com>
Reviewed-By: Alexander Bokovoy <abokovoy@redhat.com>
---
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 <abokovoy@redhat.com>
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 <cheimes@redhat.com>
(cherry picked from commit 82af034023b03ae64f005c8160b9e961e7b9fd55)
Reviewed-By: Christian Heimes <cheimes@redhat.com>
---
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 <cheimes@redhat.com>
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 <cheimes@redhat.com>
Reviewed-By: Alexander Bokovoy <abokovoy@redhat.com>
---
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',