From 4b3da3a97d1cbfd17a4eef466eb3bc1fc4887a34 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet@samba.org>
Date: Tue, 8 Aug 2023 17:58:27 +1200
Subject: [PATCH 24/28] 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>

Conflict: NA
Reference: https://download.samba.org/pub/samba/patches/security/samba-4.18.8-security-2023-10-10.patch
[PATCH 24/28] CVE-2023-4154: Unimplement the original DirSync
 behaviour without LDAP_DIRSYNC_OBJECT_SECURITY
---
 selftest/knownfail.d/dirsync             |  3 +--
 source4/dsdb/samdb/ldb_modules/dirsync.c | 22 +++++++++++-----------
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/selftest/knownfail.d/dirsync b/selftest/knownfail.d/dirsync
index db098549a08..fcf4d469d6e 100644
--- a/selftest/knownfail.d/dirsync
+++ b/selftest/knownfail.d/dirsync
@@ -1,12 +1,11 @@
 ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_OBJECT_SECURITY_insist_on_empty_element\(.*\)
 ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_OBJ_SEC_insist_on_empty_element\(.*\)
-^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES\(.*\)
 ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES_OBJ_SEC_insist_on_empty_element\(.*\)
 ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES_insist_on_empty_element\(.*\)
 ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_with_GET_CHANGES_OBJECT_SECURITY_insist_on_empty_element\(.*\)
+^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_with_GET_CHANGES\(.*\)
 ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_OBJECT_SECURITY_insist_on_empty_element\(.*\)
 ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_OBJECT_SECURITY_with_GET_CHANGES_insist_on_empty_element\(.*\)
-^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES\(.*\)
 ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES_attr\(.*\)
 ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES_insist_on_empty_element\(.*\)
 ^samba4.ldap.dirsync.python\(.*\).__main__.FilteredDirsyncTests.test_dirsync_with_GET_CHANGES\(.*\)
diff --git a/source4/dsdb/samdb/ldb_modules/dirsync.c b/source4/dsdb/samdb/ldb_modules/dirsync.c
index b3c463741c8..fbb75790095 100644
--- a/source4/dsdb/samdb/ldb_modules/dirsync.c
+++ b/source4/dsdb/samdb/ldb_modules/dirsync.c
@@ -56,7 +56,6 @@ struct dirsync_context {
 	bool linkIncrVal;
 	bool localonly;
 	bool partial;
-	bool assystem;
 	int functional_level;
 	const struct GUID *our_invocation_id;
 	const struct dsdb_schema *schema;
@@ -872,10 +871,6 @@ static int dirsync_search_callback(struct ldb_request *req, struct ldb_reply *ar
 			DSDB_SEARCH_SHOW_DELETED |
 			DSDB_SEARCH_SHOW_EXTENDED_DN;
 
-		if (dsc->assystem) {
-			flags = flags | DSDB_FLAG_AS_SYSTEM;
-		}
-
 		ret = dsdb_module_search_tree(dsc->module, dsc, &res,
 					dn, LDB_SCOPE_BASE,
 					req->op.search.tree,
@@ -1102,16 +1097,21 @@ static int dirsync_ldb_search(struct ldb_module *module, struct ldb_request *req
 			return LDB_ERR_OPERATIONS_ERROR;
 		}
 		objectclass = dsdb_get_structural_oc_from_msg(schema, acl_res->msgs[0]);
+
+		/*
+		 * While we never use the answer to this for access
+		 * control (after CVE-2023-4154), we return a
+		 * different error message depending on if the user
+		 * was granted GUID_DRS_GET_CHANGES to provide a closer
+		 * emulation and keep some tests passing.
+		 *
+		 * (Samba's ACL logic is not well suited to redacting
+		 * only the secret and RODC filtered attributes).
+		 */
 		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;
 		}
-- 
2.34.1