Attachment 18040 Details for Bug 15424 – draft patch for this issue

[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:

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
>

Actions: View

Attachments on bug 15424: 18025 | 18026 | 18027 | 18040 | 18075 | 18076 | 18084 | 18085 | 18086 | 18087 | 18088 | 18089 | 18090 | 18091 | 18092 | 18098