From 1280a85a0f6770580896d885bba903b2061be289 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Thu, 28 Apr 2022 20:34:36 +1200 Subject: [PATCH 1/3] CVE-2023-0225 CVE-2020-25720 s4/dsdb/util: Add functions for dsHeuristics 28, 29 These are the newly-added AttributeAuthorizationOnLDAPAdd and BlockOwnerImplicitRights. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14810 BUG: https://bugzilla.samba.org/show_bug.cgi?id=15276 Signed-off-by: Joseph Sutton Reviewed-by: Andrew Bartlett (cherry picked from commit 0af5706b559e89c77123ed174b41fd3d01705aa5) [abartlet@samba.org This patch is needed for a clean backport of CVE-2023-0225 as these constants are used in the acl_modify test even when this behaviour is not itself used.] --- libds/common/flags.h | 2 ++ source4/dsdb/samdb/ldb_modules/util.c | 40 +++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/libds/common/flags.h b/libds/common/flags.h index 75e04b0c488..bee1016b294 100644 --- a/libds/common/flags.h +++ b/libds/common/flags.h @@ -258,6 +258,8 @@ #define DS_HR_KVNOEMUW2K 0x00000011 #define DS_HR_TWENTIETH_CHAR 0x00000014 +#define DS_HR_ATTR_AUTHZ_ON_LDAP_ADD 0x0000001C +#define DS_HR_BLOCK_OWNER_IMPLICIT_RIGHTS 0x0000001D #define DS_HR_THIRTIETH_CHAR 0x0000001E #define DS_HR_FOURTIETH_CHAR 0x00000028 #define DS_HR_FIFTIETH_CHAR 0x00000032 diff --git a/source4/dsdb/samdb/ldb_modules/util.c b/source4/dsdb/samdb/ldb_modules/util.c index 9e00aedd09e..c2949f0734d 100644 --- a/source4/dsdb/samdb/ldb_modules/util.c +++ b/source4/dsdb/samdb/ldb_modules/util.c @@ -1433,6 +1433,46 @@ bool dsdb_do_list_object(struct ldb_module *module, return result; } +bool dsdb_attribute_authz_on_ldap_add(struct ldb_module *module, + TALLOC_CTX *mem_ctx, + struct ldb_request *parent) +{ + TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx); + bool result = false; + const struct ldb_val *hr_val = dsdb_module_find_dsheuristics(module, + tmp_ctx, + parent); + if (hr_val != NULL && hr_val->length >= DS_HR_ATTR_AUTHZ_ON_LDAP_ADD) { + uint8_t val = hr_val->data[DS_HR_ATTR_AUTHZ_ON_LDAP_ADD - 1]; + if (val != '0' && val != '2') { + result = true; + } + } + + talloc_free(tmp_ctx); + return result; +} + +bool dsdb_block_owner_implicit_rights(struct ldb_module *module, + TALLOC_CTX *mem_ctx, + struct ldb_request *parent) +{ + TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx); + bool result = false; + const struct ldb_val *hr_val = dsdb_module_find_dsheuristics(module, + tmp_ctx, + parent); + if (hr_val != NULL && hr_val->length >= DS_HR_BLOCK_OWNER_IMPLICIT_RIGHTS) { + uint8_t val = hr_val->data[DS_HR_BLOCK_OWNER_IMPLICIT_RIGHTS - 1]; + if (val != '0' && val != '2') { + result = true; + } + } + + talloc_free(tmp_ctx); + return result; +} + /* show the chain of requests, useful for debugging async requests */ -- 2.25.1 From abe09849c89b66f91e96a7b75eee07c0dfceb995 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 4 Jan 2023 21:37:49 +1300 Subject: [PATCH 2/3] 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 [abartlet@samba.org The self.set_heuristic(samba.dsdb.DS_HR_ATTR_AUTHZ_ON_LDAP_ADD, b'11') in the test setUp() is unused in this test but is included as a clean backport, so the fact that the server does not implement this is unimportant] --- 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 c6bf668aa9c..9ef9600d886 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -1417,6 +1417,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 b30b44e882866e3b726eaf8eca0f9e3eb76c282a Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Mon, 9 Jan 2023 11:22:34 +1300 Subject: [PATCH 3/3] 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 4098ae2d671..b602520ca2b 100644 --- a/source4/dsdb/samdb/ldb_modules/acl.c +++ b/source4/dsdb/samdb/ldb_modules/acl.c @@ -900,11 +900,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); @@ -1050,6 +1045,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