The Samba-Bugzilla – Attachment 18040 Details for
Bug 15424
CVE-2023-4154 [SECURITY] dirsync allows SYSTEM access with only "GUID_DRS_GET_CHANGES" right, not "GUID_DRS_GET_ALL_CHANGES"
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
draft patch for this issue
dirsync-sec-issue-v0.patch (text/plain), 16.80 KB, created by
Andrew Bartlett
on 2023-08-10 22:20:40 UTC
(
hide
)
Description:
draft patch for this issue
Filename:
MIME Type:
Creator:
Andrew Bartlett
Created:
2023-08-10 22:20:40 UTC
Size:
16.80 KB
patch
obsolete
>From 9bb0c71f1b9b0ed1a47fbe8b9dd15dbfdf53ac40 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Mon, 7 Aug 2023 11:55:55 +1200 >Subject: [PATCH 1/7] CVE-2023-4154 dsdb/tests: Do not run SimpleDirsyncTests > twice > >To re-use setup code, the super-class must have no test_*() methods >otherwise these will be run as well as the class-local tests. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >--- > source4/dsdb/tests/python/dirsync.py | 3 --- > 1 file changed, 3 deletions(-) > >diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py >index ca0947e2d21..8ac5333f945 100755 >--- a/source4/dsdb/tests/python/dirsync.py >+++ b/source4/dsdb/tests/python/dirsync.py >@@ -585,9 +585,6 @@ class SimpleDirsyncTests(DirsyncBaseTests): > expression="(&(objectClass=organizationalUnit)(!(isDeleted=*)))", > controls=controls) > >- >-class ExtendedDirsyncTests(SimpleDirsyncTests): >- > def test_dirsync_linkedattributes_range(self): > self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) > res = self.ldb_admin.search(self.base_dn, >-- >2.25.1 > > >From 73ea11f1443a94ec34595c5f06642b900921b462 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Mon, 7 Aug 2023 13:15:40 +1200 >Subject: [PATCH 2/7] CVE-2023-4154 dsdb/tests: Use self.addCleanup() and > delete_force() > >Thie helps ensure this test is reliable even in spite of errors while >running. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >--- > source4/dsdb/tests/python/confidential_attr.py | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > >diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py >index 8ca56bd1023..3997848f8f9 100755 >--- a/source4/dsdb/tests/python/confidential_attr.py >+++ b/source4/dsdb/tests/python/confidential_attr.py >@@ -98,7 +98,9 @@ class ConfidentialAttrCommon(samba.tests.TestCase): > > userou = "OU=conf-attr-test" > self.ou = "{0},{1}".format(userou, self.base_dn) >+ samba.tests.delete_force(self.ldb_admin, self.ou, controls=['tree_delete:1']) > self.ldb_admin.create_ou(self.ou) >+ self.addCleanup(samba.tests.delete_force, self.ldb_admin, self.ou, controls=['tree_delete:1']) > > # use a common username prefix, so we can use sAMAccountName=CATC-* as > # a search filter to only return the users we're interested in >@@ -139,10 +141,6 @@ class ConfidentialAttrCommon(samba.tests.TestCase): > "{0} searchFlags already {1}".format(self.conf_attr, > search_flags)) > >- def tearDown(self): >- super(ConfidentialAttrCommon, self).tearDown() >- self.ldb_admin.delete(self.ou, ["tree_delete:1"]) >- > def add_attr(self, dn, attr, value): > m = Message() > m.dn = Dn(self.ldb_admin, dn) >-- >2.25.1 > > >From ec01914e7fdd2370810f0ce2dddf654a2cf90d85 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Mon, 7 Aug 2023 14:44:28 +1200 >Subject: [PATCH 3/7] CVE-2023-4154 dsdb/tests: Force the test attribute to be > not-confidential at the start > >Rather than fail, if the last run failed to reset things, just force >the DC into the required state. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >--- > source4/dsdb/tests/python/confidential_attr.py | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > >diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py >index 3997848f8f9..ee7f554a008 100755 >--- a/source4/dsdb/tests/python/confidential_attr.py >+++ b/source4/dsdb/tests/python/confidential_attr.py >@@ -136,10 +136,12 @@ class ConfidentialAttrCommon(samba.tests.TestCase): > > # sanity-check the flag is not already set (this'll cause problems if > # previous test run didn't clean up properly) >- search_flags = self.get_attr_search_flags(self.attr_dn) >- self.assertEqual(0, int(search_flags) & SEARCH_FLAG_CONFIDENTIAL, >- "{0} searchFlags already {1}".format(self.conf_attr, >- search_flags)) >+ search_flags = int(self.get_attr_search_flags(self.attr_dn)) >+ if search_flags & SEARCH_FLAG_CONFIDENTIAL: >+ self.set_attr_search_flags(self.attr_dn, str(search_flags &~ SEARCH_FLAG_CONFIDENTIAL)) >+ search_flags = int(self.get_attr_search_flags(self.attr_dn)) >+ self.assertEqual(0, search_flags & SEARCH_FLAG_CONFIDENTIAL, >+ f"{self.conf_attr} searchFlags did not reset to omit SEARCH_FLAG_CONFIDENTIAL ({search_flags})") > > def add_attr(self, dn, attr, value): > m = Message() >-- >2.25.1 > > >From 4dc5f8a40dd905f697d6f12632ff251b4d52d3f9 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Mon, 7 Aug 2023 11:56:56 +1200 >Subject: [PATCH 4/7] CVE-2023-4154 dsdb/tests: Check that secret attributes > are not visible with DirSync ever. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >--- > source4/dsdb/tests/python/dirsync.py | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > >diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py >index 8ac5333f945..d07dc707e7d 100755 >--- a/source4/dsdb/tests/python/dirsync.py >+++ b/source4/dsdb/tests/python/dirsync.py >@@ -746,6 +746,19 @@ class SimpleDirsyncTests(DirsyncBaseTests): > self.assertEqual(guid2, guid) > self.assertEqual(str(res[0].dn), "") > >+ def test_dirsync_unicodePwd(self): >+ self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) >+ res = self.ldb_admin.search(self.base_dn, >+ attrs=["unicodePwd", "supplementalCredentials", "cn"], >+ expression="(samAccountName=krbtgt)", >+ controls=["dirsync:1:0:0"]) >+ >+ self.assertTrue(len(res) == 1) >+ # This form ensures this is a case insensitive comparison >+ self.assertTrue(res[0].get("cn")) >+ self.assertTrue(res[0].get("unicodePwd") is None) >+ self.assertTrue(res[0].get("supplementalCredentials") is None) >+ > > if not getattr(opts, "listtests", False): > lp = sambaopts.get_loadparm() >-- >2.25.1 > > >From 9d2604929fed912bc54cdd6ff0c25dd998006830 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Tue, 8 Aug 2023 11:18:46 +1200 >Subject: [PATCH 5/7] CVE-2023-4154 dsdb/tests: Speed up DirSync test by only > checking positive matches once > >When we (expect to) get back a result, do not waste time against a potentially >slow server confirming we also get back results for all the other attribute >combinations. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >--- > source4/dsdb/tests/python/confidential_attr.py | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > >diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py >index ee7f554a008..678a5a82948 100755 >--- a/source4/dsdb/tests/python/confidential_attr.py >+++ b/source4/dsdb/tests/python/confidential_attr.py >@@ -742,7 +742,13 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): > # want to weed out results from any previous test runs > search = "(&{0}{1})".format(expr, self.extra_filter) > >- for attr in self.attr_filters: >+ # If we expect to return multiple results, only check the first >+ if expected_num > 0: >+ attr_filters = [self.attr_filters[0]] >+ else: >+ attr_filters = self.attr_filters >+ >+ for attr in attr_filters: > res = samdb.search(base_dn, expression=search, scope=SCOPE_SUBTREE, > attrs=attr, controls=self.dirsync) > self.assertEqual(len(res), expected_num, >-- >2.25.1 > > >From c53f4c21a0f7ecb4d773edac7d7b090d5739ed20 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Tue, 8 Aug 2023 14:30:19 +1200 >Subject: [PATCH 6/7] CVE-2023-4154 dsdb/tests: Add test for > SEARCH_FLAG_RODC_ATTRIBUTE behaviour > >SEARCH_FLAG_RODC_ATTRIBUTE should be like SEARCH_FLAG_CONFIDENTIAL, >but for DirSync and DRS replication. Accounts with >GUID_DRS_GET_CHANGES rights should not be able to read this >attribute. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >--- > .../dsdb/tests/python/confidential_attr.py | 45 ++++++++++++++++--- > 1 file changed, 40 insertions(+), 5 deletions(-) > >diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py >index 678a5a82948..df342fafc3d 100755 >--- a/source4/dsdb/tests/python/confidential_attr.py >+++ b/source4/dsdb/tests/python/confidential_attr.py >@@ -30,13 +30,15 @@ import time > from samba.tests.subunitrun import SubunitOptions, TestProgram > import samba.getopt as options > from ldb import SCOPE_BASE, SCOPE_SUBTREE >-from samba.dsdb import SEARCH_FLAG_CONFIDENTIAL, SEARCH_FLAG_PRESERVEONDELETE >+from samba.dsdb import SEARCH_FLAG_CONFIDENTIAL, SEARCH_FLAG_RODC_ATTRIBUTE, SEARCH_FLAG_PRESERVEONDELETE > from ldb import Message, MessageElement, Dn > from ldb import FLAG_MOD_REPLACE, FLAG_MOD_ADD > 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 >+from samba.dcerpc import security >+ > import samba.tests > import samba.dsdb > >@@ -137,11 +139,11 @@ class ConfidentialAttrCommon(samba.tests.TestCase): > # sanity-check the flag is not already set (this'll cause problems if > # previous test run didn't clean up properly) > search_flags = int(self.get_attr_search_flags(self.attr_dn)) >- if search_flags & SEARCH_FLAG_CONFIDENTIAL: >- self.set_attr_search_flags(self.attr_dn, str(search_flags &~ SEARCH_FLAG_CONFIDENTIAL)) >+ if search_flags & SEARCH_FLAG_CONFIDENTIAL|SEARCH_FLAG_RODC_ATTRIBUTE: >+ self.set_attr_search_flags(self.attr_dn, str(search_flags &~ (SEARCH_FLAG_CONFIDENTIAL|SEARCH_FLAG_RODC_ATTRIBUTE))) > search_flags = int(self.get_attr_search_flags(self.attr_dn)) >- self.assertEqual(0, search_flags & SEARCH_FLAG_CONFIDENTIAL, >- f"{self.conf_attr} searchFlags did not reset to omit SEARCH_FLAG_CONFIDENTIAL ({search_flags})") >+ self.assertEqual(0, search_flags & (SEARCH_FLAG_CONFIDENTIAL|SEARCH_FLAG_RODC_ATTRIBUTE), >+ f"{self.conf_attr} searchFlags did not reset to omit SEARCH_FLAG_CONFIDENTIAL and SEARCH_FLAG_RODC_ATTRIBUTE ({search_flags})") > > def add_attr(self, dn, attr, value): > m = Message() >@@ -1098,5 +1100,38 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): > user_matching, user_non_matching = time_searches(self.ldb_user) > assertRangesOverlap(user_matching, user_non_matching) > >+# Check that using the dirsync controls doesn't reveal confidential >+# "RODC filtered attribute" values to users with only >+# GUID_DRS_GET_CHANGES. The tests is so similar to the Confidential >+# attribute test we base it on that. >+class RodcFilteredAttrDirsync(ConfidentialAttrTestDirsync): >+ >+ def setUp(self): >+ super().setUp() >+ self.dirsync = ["dirsync:1:0:1000"] >+ >+ user_sid = self.sd_utils.get_object_sid(self.get_user_dn(self.user)) >+ mod = "(OA;;CR;%s;;%s)" % (security.GUID_DRS_GET_CHANGES, >+ str(user_sid)) >+ self.sd_utils.dacl_add_ace(self.base_dn, mod) >+ >+ self.ldb_user = self.get_ldb_connection(self.user, self.user_pass) >+ >+ self.addCleanup(self.sd_utils.dacl_del_ace, self.base_dn, mod) >+ >+ def make_attr_confidential(self): >+ """Marks the attribute under test as being confidential AND RODC >+ filtered (which should mean it is not visible with only >+ GUID_DRS_GET_CHANGES) >+ """ >+ >+ # work out the original 'searchFlags' value before we overwrite it >+ old_value = self.get_attr_search_flags(self.attr_dn) >+ >+ self.set_attr_search_flags(self.attr_dn, str(SEARCH_FLAG_RODC_ATTRIBUTE|SEARCH_FLAG_CONFIDENTIAL)) >+ >+ # reset the value after the test completes >+ self.addCleanup(self.set_attr_search_flags, self.attr_dn, old_value) >+ > > TestProgram(module=__name__, opts=subunitopts) >-- >2.25.1 > > >From 6cd82b81e5e042f3a55f082f4ef91a02a785507c Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Tue, 8 Aug 2023 17:58:27 +1200 >Subject: [PATCH 7/7] CVE-2023-4154: Unimplement the original DirSync behaviour > without LDAP_DIRSYNC_OBJECT_SECURITY > >This makes LDAP_DIRSYNC_OBJECT_SECURITY the only behaviour provided by >Samba. > >Having a second access control system withing the LDAP stack is unsafe >and this layer is incomplete. > >The current system gives all accounts that have been given the >GUID_DRS_GET_CHANGES extended right SYSTEM access. Currently in Samba >this equates to full access to passwords as well as "RODC Filtered >attributes" (often used with confidential attributes). > >Rather than attempting to correctly filter for secrets (passwords) and >these filtered attributes, as well as preventing search expressions for >both, we leave this complexity to the acl_read module which has this >facility already well tested. > >The implication is that callers will only see and filter by attribute >in DirSync that they could without DirSync. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >--- > source4/dsdb/samdb/ldb_modules/dirsync.c | 67 +----------------------- > 1 file changed, 1 insertion(+), 66 deletions(-) > >diff --git a/source4/dsdb/samdb/ldb_modules/dirsync.c b/source4/dsdb/samdb/ldb_modules/dirsync.c >index 9261758ec7b..b48d3faad04 100644 >--- a/source4/dsdb/samdb/ldb_modules/dirsync.c >+++ b/source4/dsdb/samdb/ldb_modules/dirsync.c >@@ -998,7 +998,6 @@ static int dirsync_search_callback(struct ldb_request *req, struct ldb_reply *ar > static int dirsync_ldb_search(struct ldb_module *module, struct ldb_request *req) > { > struct ldb_control *control; >- struct ldb_result *acl_res; > struct ldb_dirsync_control *dirsync_ctl; > struct ldb_control *extended = NULL; > struct ldb_request *down_req; >@@ -1052,72 +1051,8 @@ static int dirsync_ldb_search(struct ldb_module *module, struct ldb_request *req > } > > if (ldb_dn_compare(dsc->nc_root, req->op.search.base) != 0) { >- if (dirsync_ctl->flags & LDAP_DIRSYNC_OBJECT_SECURITY) { >- return ldb_error(ldb, LDB_ERR_UNWILLING_TO_PERFORM, >+ return ldb_error(ldb, LDB_ERR_UNWILLING_TO_PERFORM, > "DN is not one of the naming context"); >- } >- else { >- return ldb_error(ldb, LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS, >- "dN is not one of the naming context"); >- } >- } >- >- if (!(dirsync_ctl->flags & LDAP_DIRSYNC_OBJECT_SECURITY)) { >- struct dom_sid *sid; >- struct security_descriptor *sd = NULL; >- const char *acl_attrs[] = { "nTSecurityDescriptor", "objectSid", "objectClass", NULL }; >- const struct dsdb_schema *schema = NULL; >- const struct dsdb_class *objectclass = NULL; >- /* >- * If we don't have the flag and if we have the "replicate directory change" granted >- * then we upgrade ourself to system to not be blocked by the acl >- */ >- /* FIXME we won't check the replicate directory change filtered attribute set >- * it should be done so that if attr is not empty then we check that the user >- * has also this right >- */ >- >- /* >- * First change to system to get the SD of the root of current NC >- * if we don't the acl_read will forbid us the right to read it ... >- */ >- ret = dsdb_module_search_dn(module, dsc, &acl_res, >- req->op.search.base, >- acl_attrs, >- DSDB_FLAG_NEXT_MODULE|DSDB_FLAG_AS_SYSTEM, req); >- >- if (ret != LDB_SUCCESS) { >- return ret; >- } >- >- sid = samdb_result_dom_sid(dsc, acl_res->msgs[0], "objectSid"); >- /* sid can be null ... */ >- ret = dsdb_get_sd_from_ldb_message(ldb_module_get_ctx(module), acl_res, acl_res->msgs[0], &sd); >- >- if (ret != LDB_SUCCESS) { >- return ret; >- } >- schema = dsdb_get_schema(ldb, req); >- if (!schema) { >- return LDB_ERR_OPERATIONS_ERROR; >- } >- objectclass = dsdb_get_structural_oc_from_msg(schema, acl_res->msgs[0]); >- ret = acl_check_extended_right(dsc, module, req, objectclass, >- sd, acl_user_token(module), >- GUID_DRS_GET_CHANGES, SEC_ADS_CONTROL_ACCESS, sid); >- >- if (ret == LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS) { >- return ret; >- } >- dsc->assystem = true; >- ret = ldb_request_add_control(req, LDB_CONTROL_AS_SYSTEM_OID, false, NULL); >- >- if (ret != LDB_SUCCESS) { >- return ret; >- } >- talloc_free(acl_res); >- } else if (ret != LDB_SUCCESS) { >- return ret; > } > > dsc->functional_level = dsdb_functional_level(ldb); >-- >2.25.1 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Actions:
View
Attachments on
bug 15424
:
18025
|
18026
|
18027
|
18040
|
18075
|
18076
|
18084
|
18085
|
18086
|
18087
|
18088
|
18089
|
18090
|
18091
|
18092
|
18098