From a115802bb307bc89c33707f355b27f16cc87a29b Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 4 Jan 2023 21:37:49 +1300 Subject: [PATCH 1/2] CVE-2023-0225 pytest/acl: test deleting dNSHostName as unprivileged user BUG: https://bugzilla.samba.org/show_bug.cgi?id=15276 Signed-off-by: Douglas Bagnall Reviewed-by: Joseph Sutton Reviewed-by: Andrew Bartlett --- selftest/knownfail.d/dns-host-name-deletion | 2 + source4/dsdb/tests/python/acl_modify.py | 236 ++++++++++++++++++++ source4/selftest/tests.py | 1 + 3 files changed, 239 insertions(+) create mode 100644 selftest/knownfail.d/dns-host-name-deletion create mode 100755 source4/dsdb/tests/python/acl_modify.py diff --git a/selftest/knownfail.d/dns-host-name-deletion b/selftest/knownfail.d/dns-host-name-deletion new file mode 100644 index 00000000000..ac11619ffc3 --- /dev/null +++ b/selftest/knownfail.d/dns-host-name-deletion @@ -0,0 +1,2 @@ +^samba4.ldap.acl_modify.python$.*$.__main__.AclModifyTests.test_modify_delete_dns_host_name_ldif_unspecified$.*$ +^samba4.ldap.acl_modify.python$.*$.__main__.AclModifyTests.test_modify_delete_dns_host_name_unspecified$.*$ diff --git a/source4/dsdb/tests/python/acl_modify.py b/source4/dsdb/tests/python/acl_modify.py new file mode 100755 index 00000000000..c85748a764f --- /dev/null +++ b/source4/dsdb/tests/python/acl_modify.py @@ -0,0 +1,236 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- + + +import optparse +import sys +sys.path.insert(0, "bin/python") +import samba + +from samba.tests.subunitrun import SubunitOptions, TestProgram + +import samba.getopt as options + +from ldb import ERR_INSUFFICIENT_ACCESS_RIGHTS +from ldb import Message, MessageElement, Dn +from ldb import FLAG_MOD_REPLACE, FLAG_MOD_DELETE +from samba.dcerpc import security + +from samba.auth import system_session +from samba import gensec, sd_utils +from samba.samdb import SamDB +from samba.credentials import Credentials, DONT_USE_KERBEROS +import samba.tests +import samba.dsdb + + +parser = optparse.OptionParser("acl.py [options] ") +sambaopts = options.SambaOptions(parser) +parser.add_option_group(sambaopts) +parser.add_option_group(options.VersionOptions(parser)) + +# use command line creds if available +credopts = options.CredentialsOptions(parser) +parser.add_option_group(credopts) +subunitopts = SubunitOptions(parser) +parser.add_option_group(subunitopts) + +opts, args = parser.parse_args() + +if len(args) < 1: + parser.print_usage() + sys.exit(1) + +host = args[0] +if "://" not in host: + ldaphost = "ldap://%s" % host +else: + ldaphost = host + start = host.rindex("://") + host = host.lstrip(start + 3) + +lp = sambaopts.get_loadparm() +creds = credopts.get_credentials(lp) +creds.set_gensec_features(creds.get_gensec_features() | gensec.FEATURE_SEAL) + +# +# Tests start here +# + + +class AclTests(samba.tests.TestCase): + + def setUp(self): + super(AclTests, self).setUp() + + strict_checking = samba.tests.env_get_var_value('STRICT_CHECKING', allow_missing=True) + if strict_checking is None: + strict_checking = '1' + self.strict_checking = bool(int(strict_checking)) + + self.ldb_admin = SamDB(ldaphost, credentials=creds, session_info=system_session(lp), lp=lp) + self.base_dn = self.ldb_admin.domain_dn() + self.domain_sid = security.dom_sid(self.ldb_admin.get_domain_sid()) + self.user_pass = "samba123@" + self.configuration_dn = self.ldb_admin.get_config_basedn().get_linearized() + self.sd_utils = sd_utils.SDUtils(self.ldb_admin) + self.addCleanup(self.delete_admin_connection) + # used for anonymous login + self.creds_tmp = Credentials() + self.creds_tmp.set_username("") + self.creds_tmp.set_password("") + self.creds_tmp.set_domain(creds.get_domain()) + self.creds_tmp.set_realm(creds.get_realm()) + self.creds_tmp.set_workstation(creds.get_workstation()) + print("baseDN: %s" % self.base_dn) + + # set AttributeAuthorizationOnLDAPAdd and BlockOwnerImplicitRights + self.set_heuristic(samba.dsdb.DS_HR_ATTR_AUTHZ_ON_LDAP_ADD, b'11') + + def set_heuristic(self, index, values): + self.assertGreater(index, 0) + self.assertLess(index, 30) + self.assertIsInstance(values, bytes) + + # Get the old "dSHeuristics" if it was set + dsheuristics = self.ldb_admin.get_dsheuristics() + # Reset the "dSHeuristics" as they were before + self.addCleanup(self.ldb_admin.set_dsheuristics, dsheuristics) + # Set the "dSHeuristics" to activate the correct behaviour + default_heuristics = b"000000000100000000020000000003" + if dsheuristics is None: + dsheuristics = b"" + dsheuristics += default_heuristics[len(dsheuristics):] + dsheuristics = (dsheuristics[:index - 1] + + values + + dsheuristics[index - 1 + len(values):]) + self.ldb_admin.set_dsheuristics(dsheuristics) + + def get_user_dn(self, name): + return "CN=%s,CN=Users,%s" % (name, self.base_dn) + + def get_ldb_connection(self, target_username, target_password): + creds_tmp = Credentials() + creds_tmp.set_username(target_username) + creds_tmp.set_password(target_password) + creds_tmp.set_domain(creds.get_domain()) + creds_tmp.set_realm(creds.get_realm()) + creds_tmp.set_workstation(creds.get_workstation()) + creds_tmp.set_gensec_features(creds_tmp.get_gensec_features() + | gensec.FEATURE_SEAL) + creds_tmp.set_kerberos_state(DONT_USE_KERBEROS) # kinit is too expensive to use in a tight loop + ldb_target = SamDB(url=ldaphost, credentials=creds_tmp, lp=lp) + return ldb_target + + # Test if we have any additional groups for users than default ones + def assert_user_no_group_member(self, username): + res = self.ldb_admin.search(self.base_dn, expression="(distinguishedName=%s)" % self.get_user_dn(username)) + try: + self.assertEqual(res[0]["memberOf"][0], "") + except KeyError: + pass + else: + self.fail() + + def delete_admin_connection(self): + del self.sd_utils + del self.ldb_admin + + +class AclModifyTests(AclTests): + + def setup_computer_with_hostname(self, account_name): + ou_dn = f'OU={account_name},{self.base_dn}' + dn = f'CN={account_name},{ou_dn}' + + user, password = "mouse", "mus musculus 123!" + self.addCleanup(self.ldb_admin.deleteuser, user) + + self.ldb_admin.newuser(user, password) + self.ldb_user = self.get_ldb_connection(user, password) + + self.addCleanup(self.ldb_admin.delete, ou_dn, + controls=["tree_delete:0"]) + self.ldb_admin.create_ou(ou_dn) + + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': account_name + '$', + }) + + host_name = f'{account_name}.{self.ldb_user.domain_dns_name()}' + + m = Message(Dn(self.ldb_admin, dn)) + m['dNSHostName'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + + self.ldb_admin.modify(m) + return host_name, dn + + def test_modify_delete_dns_host_name_specified(self): + '''Test deleting dNSHostName''' + account_name = self.id().rsplit(".", 1)[1][:63] + host_name, dn = self.setup_computer_with_hostname(account_name) + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement(host_name, + FLAG_MOD_DELETE, + 'dNSHostName') + + self.assertRaisesLdbError( + ERR_INSUFFICIENT_ACCESS_RIGHTS, + "User able to delete dNSHostName (with specified name)", + self.ldb_user.modify, m) + + def test_modify_delete_dns_host_name_unspecified(self): + '''Test deleting dNSHostName''' + account_name = self.id().rsplit(".", 1)[1][:63] + host_name, dn = self.setup_computer_with_hostname(account_name) + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement([], + FLAG_MOD_DELETE, + 'dNSHostName') + + self.assertRaisesLdbError( + ERR_INSUFFICIENT_ACCESS_RIGHTS, + "User able to delete dNSHostName (without specified name)", + self.ldb_user.modify, m) + + def test_modify_delete_dns_host_name_ldif_specified(self): + '''Test deleting dNSHostName''' + account_name = self.id().rsplit(".", 1)[1][:63] + host_name, dn = self.setup_computer_with_hostname(account_name) + + ldif = f""" +dn: {dn} +changetype: modify +delete: dNSHostName +dNSHostName: {host_name} +""" + self.assertRaisesLdbError( + ERR_INSUFFICIENT_ACCESS_RIGHTS, + "User able to delete dNSHostName (with specified name)", + self.ldb_user.modify_ldif, ldif) + + def test_modify_delete_dns_host_name_ldif_unspecified(self): + '''Test deleting dNSHostName''' + account_name = self.id().rsplit(".", 1)[1][:63] + host_name, dn = self.setup_computer_with_hostname(account_name) + + ldif = f""" +dn: {dn} +changetype: modify +delete: dNSHostName +""" + self.assertRaisesLdbError( + ERR_INSUFFICIENT_ACCESS_RIGHTS, + "User able to delete dNSHostName (without specific name)", + self.ldb_user.modify_ldif, ldif) + + +ldb = SamDB(ldaphost, credentials=creds, session_info=system_session(lp), lp=lp) + +TestProgram(module=__name__, opts=subunitopts) diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index 2780d77ad07..30a228ad2ac 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -1426,6 +1426,7 @@ for env in all_fl_envs + ["schema_dc"]: plantestsuite("samba4.ldap.possibleInferiors.python(%s)" % env, env, [python, os.path.join(samba4srcdir, "dsdb/samdb/ldb_modules/tests/possibleinferiors.py"), "ldap://$SERVER", '-U"$USERNAME%$PASSWORD"', "-W$DOMAIN"]) plantestsuite_loadlist("samba4.ldap.secdesc.python(%s)" % env, env, [python, os.path.join(DSDB_PYTEST_DIR, "sec_descriptor.py"), '$SERVER', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN', '$LOADLIST', '$LISTOPT']) plantestsuite_loadlist("samba4.ldap.acl.python(%s)" % env, env, ["STRICT_CHECKING=0", python, os.path.join(DSDB_PYTEST_DIR, "acl.py"), '$SERVER', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN', '$LOADLIST', '$LISTOPT']) + plantestsuite_loadlist("samba4.ldap.acl_modify.python(%s)" % env, env, ["STRICT_CHECKING=0", python, os.path.join(DSDB_PYTEST_DIR, "acl_modify.py"), '$SERVER', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN', '$LOADLIST', '$LISTOPT']) for env in all_fl_envs + ["schema_dc", "ad_dc_no_ntlm"]: if env != "fl2000dc": -- 2.25.1 From 570ca5c8dd1944e75df1f87f8439ad8d8e190aa8 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Mon, 9 Jan 2023 11:22:34 +1300 Subject: [PATCH 2/2] CVE-2023-0225 s4-acl: Don't return early if dNSHostName element has no values This early return would mistakenly allow an unprivileged user to delete the dNSHostName attribute by making an LDAP modify request with no values. We should no longer allow this. Add or replace operations with no values and no privileges are disallowed. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15276 Signed-off-by: Joseph Sutton Reviewed-by: Andrew Bartlett --- selftest/knownfail.d/dns-host-name-deletion | 2 -- source4/dsdb/samdb/ldb_modules/acl.c | 12 +++++++----- 2 files changed, 7 insertions(+), 7 deletions(-) delete mode 100644 selftest/knownfail.d/dns-host-name-deletion diff --git a/selftest/knownfail.d/dns-host-name-deletion b/selftest/knownfail.d/dns-host-name-deletion deleted file mode 100644 index ac11619ffc3..00000000000 --- a/selftest/knownfail.d/dns-host-name-deletion +++ /dev/null @@ -1,2 +0,0 @@ -^samba4.ldap.acl_modify.python$.*$.__main__.AclModifyTests.test_modify_delete_dns_host_name_ldif_unspecified$.*$ -^samba4.ldap.acl_modify.python$.*$.__main__.AclModifyTests.test_modify_delete_dns_host_name_unspecified$.*$ diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c index 48dbd35c1cb..be674e4ff8b 100644 --- a/source4/dsdb/samdb/ldb_modules/acl.c +++ b/source4/dsdb/samdb/ldb_modules/acl.c @@ -946,11 +946,6 @@ static int acl_check_dns_host_name(TALLOC_CTX *mem_ctx, NULL }; - if (el->num_values == 0) { - return LDB_SUCCESS; - } - dnsHostName = &el->values[0]; - tmp_ctx = talloc_new(mem_ctx); if (tmp_ctx == NULL) { return ldb_oom(ldb); @@ -1101,6 +1096,13 @@ static int acl_check_dns_host_name(TALLOC_CTX *mem_ctx, --account_name_len; } + /* Check for add or replace requests with no value. */ + if (el->num_values == 0) { + talloc_free(tmp_ctx); + return ldb_operr(ldb); + } + dnsHostName = &el->values[0]; + dnsHostName_str = (const char *)dnsHostName->data; dns_host_name_len = dnsHostName->length; -- 2.25.1