[PATCH] Some minor clean-ups to confidential attribute fix

Douglas Bagnall douglas.bagnall at catalyst.net.nz
Fri Sep 21 02:52:08 UTC 2018


On 21/09/18 12:43, Tim Beale via samba-technical wrote:
> For the changes for bug #13434 (i.e. commit fc45da529dc6794d270), I made
> a best-guess at some undocumented Microsoft logic. Microsoft has since
> clarified this logic, so we can now update the code to reflect this.
> I've also made some minor tweaks to the test code.
> 
> CI link: https://gitlab.com/catalyst-samba/samba/pipelines/30680063
> 
> Review appreciated.
> 
> Thanks.

RB+, more review needed!

In the attached version I changed the first one to go on top of my
recent formatting patches, and I fiddlingly suggest making the known
attribute name list static. But either way is OK.

Douglas
-------------- next part --------------
From e2dde423b28dbade088fd6ee5b28ff3750cb4295 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 1 Aug 2018 17:30:37 +1200
Subject: [PATCH 1/3] tests: Minor code cleanups to confidential_attr test

+ fix a couple of flake8 warnings
+ add some extra code comments (particularly around the cases where the
  child class overrides a particular method, to avoid confusion when
  browsing the code).
+ assert_not_in_result() was duplicated (it's only needed for the deny
  ACL tests)
+ skip redundant if in dirsync's assert_search_result() (it always has
  to use the base-DN - we never pass it this as an args).

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
---
 .../dsdb/tests/python/confidential_attr.py    | 37 ++++++++++++-------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py
index 97b5fc50297..f1d6a95fef8 100755
--- a/source4/dsdb/tests/python/confidential_attr.py
+++ b/source4/dsdb/tests/python/confidential_attr.py
@@ -36,7 +36,6 @@ from samba import gensec, sd_utils
 from samba.samdb import SamDB
 from samba.credentials import Credentials, DONT_USE_KERBEROS
 import samba.tests
-from samba.tests import delete_force
 import samba.dsdb
 
 parser = optparse.OptionParser("confidential_attr.py [options] <host>")
@@ -81,6 +80,7 @@ creds.set_gensec_features(creds.get_gensec_features() | gensec.FEATURE_SEAL)
 DC_MODE_RETURN_NONE = 0
 DC_MODE_RETURN_ALL = 1
 
+
 #
 # Tests start here
 #
@@ -130,7 +130,7 @@ class ConfidentialAttrCommon(samba.tests.TestCase):
 
         self.all_users = [self.user, self.conf_user]
 
-        # add some other users that also have confidential attributes, so we can
+        # add some other users that also have confidential attributes, so we
         # check we don't disclose their details, particularly in '!' searches
         for i in range(1, 3):
             username = "{0}other-user{1}".format(self.user_prefix, i)
@@ -230,11 +230,6 @@ class ConfidentialAttrCommon(samba.tests.TestCase):
         ldb_target = SamDB(url=ldaphost, credentials=creds_tmp, lp=lp)
         return ldb_target
 
-    def assert_not_in_result(self, res, exclude_dn):
-        for msg in res:
-            self.assertNotEqual(msg.dn, exclude_dn,
-                                "Search revealed object {0}".format(exclude_dn))
-
     def assert_search_result(self, expected_num, expr, samdb):
 
         # try asking for different attributes back: None/all, the confidential
@@ -376,8 +371,9 @@ class ConfidentialAttrCommon(samba.tests.TestCase):
         for search in self.get_negative_match_all_searches():
             expected_results[search] = total_objects
 
-        # if the search is matching on an inverse subset (everything except the
-        # object under test), the
+        # if we're matching on everything except the one object under test
+        # (i.e. the inverse subset), we'll still see all objects if
+        # has_rights_to == 0. Or we'll see all bar one if has_rights_to == 1.
         inverse_searches = self.get_inverse_match_searches()
         inverse_searches += ["(!({0}=*))".format(self.conf_attr)]
 
@@ -407,7 +403,9 @@ class ConfidentialAttrCommon(samba.tests.TestCase):
 
     # Returns the expected negative (i.e. '!') search behaviour. This varies
     # depending on what type of DC we're talking to (i.e. Windows or Samba)
-    # and what access rights the user has
+    # and what access rights the user has.
+    # Note we only handle has_rights_to="all", 1 (the test object), or 0 (i.e.
+    # we don't have rights to any objects)
     def negative_search_expected_results(self, has_rights_to, dc_mode,
                                          total_objects=None):
 
@@ -467,8 +465,10 @@ class ConfidentialAttrCommon(samba.tests.TestCase):
 
     def assert_attr_visible_to_admin(self):
         # sanity-check the admin user can always see the confidential attribute
-        self.assert_conf_attr_searches(has_rights_to="all", samdb=self.ldb_admin)
-        self.assert_negative_searches(has_rights_to="all", samdb=self.ldb_admin)
+        self.assert_conf_attr_searches(has_rights_to="all",
+                                       samdb=self.ldb_admin)
+        self.assert_negative_searches(has_rights_to="all",
+                                      samdb=self.ldb_admin)
         self.assert_attr_visible(expect_attr=True, samdb=self.ldb_admin)
 
 
@@ -666,6 +666,9 @@ class ConfidentialAttrTestDenyAcl(ConfidentialAttrCommon):
             self.assert_search_result(expected_num, search, samdb,
                                       excl_testobj)
 
+    # override method specifically for deny ACL test cases. Instead of being
+    # granted access to either no objects or only one, we are being denied
+    # access to only one object (but can still access the rest).
     def negative_searches_return_none(self, has_rights_to=0):
         expected_results = {}
 
@@ -681,6 +684,7 @@ class ConfidentialAttrTestDenyAcl(ConfidentialAttrCommon):
         expected_results[search] = self.total_objects - self.objects_with_attr
         return expected_results
 
+    # override method specifically for deny ACL test cases
     def negative_searches_return_all(self, has_rights_to=0,
                                      total_objects=None):
         expected_results = {}
@@ -699,6 +703,7 @@ class ConfidentialAttrTestDenyAcl(ConfidentialAttrCommon):
         expected_results[search] = self.total_objects - has_rights_to
         return expected_results
 
+    # override method specifically for deny ACL test cases
     def assert_negative_searches(self, has_rights_to=0,
                                  dc_mode=DC_MODE_RETURN_NONE, samdb=None):
         """Asserts user without rights cannot see objects in '!' searches"""
@@ -809,11 +814,11 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon):
         # attribute filter to avoid complicating our assertions further
         self.attr_filters += [[self.conf_attr, "name"]]
 
+    # override method specifically for dirsync, i.e. add dirsync controls
     def assert_search_result(self, expected_num, expr, samdb, base_dn=None):
 
         # Note dirsync must always search on the partition base DN
-        if base_dn is None:
-            base_dn = self.base_dn
+        base_dn = self.base_dn
 
         # we need an extra filter for dirsync because:
         # - we search on the base DN, so otherwise the '!' searches return
@@ -829,6 +834,7 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon):
                             "%u results, not %u for search %s, attr %s" %
                             (len(res), expected_num, search, str(attr)))
 
+    # override method specifically for dirsync, i.e. add dirsync controls
     def assert_attr_returned(self, expect_attr, samdb, attrs,
                              no_result_ok=False):
 
@@ -846,6 +852,8 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon):
                 attr_returned = True
         self.assertEqual(expect_attr, attr_returned)
 
+    # override method specifically for dirsync (it has slightly different
+    # behaviour to normal when requesting specific attributes)
     def assert_attr_visible(self, expect_attr, samdb=None):
         if samdb is None:
             samdb = self.ldb_user
@@ -875,6 +883,7 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon):
         self.assert_attr_returned(expect_attr=False, samdb=samdb,
                                   attrs=['name'])
 
+    # override method specifically for dirsync (total object count differs)
     def assert_negative_searches(self, has_rights_to=0,
                                  dc_mode=DC_MODE_RETURN_NONE, samdb=None):
         """Asserts user without rights cannot see objects in '!' searches"""
-- 
2.17.1


From f88ed7d6f1fe39d1fbf3ecb4d530f832221b8fd1 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Fri, 14 Sep 2018 13:27:56 +1200
Subject: [PATCH 2/3] acl_read: Rework Samba code to reflect Windows logic

This patch should not alter functionality. It is just updating the Samba
code to better match the Windows specification docs.

When fixing Samba BUG #13434, the Microsoft behaviour wasn't clearly
documented, so we made a best guess based on observed behaviour.
The problem was an exception was made to allow "objectClass=*" searches
to return objects, even if you didn't have Read Property rights for the
object's objectClass attribute. However, the logic behind what
attributes were and weren't covered by this exception wasn't clear.

I made a guess that it was attributes belonging to the Public Info
property-set that also have the systemOnly flag set.

Microsoft have confirmed the object visibility behaviour. It turns out
that an optimization is made for the 4 attributes that are always
present for every object (i.e. objectClass, distinguishedName,
name, objectGUID). They're updating their Docs to reflect this.

Now that we know the Windows logic, we can update the Samba code.
This simplifies the code somewhat.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
---
 source4/dsdb/samdb/ldb_modules/acl_read.c | 73 ++++++++---------------
 1 file changed, 24 insertions(+), 49 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/acl_read.c b/source4/dsdb/samdb/ldb_modules/acl_read.c
index 280845a47a5..6ab4780a196 100644
--- a/source4/dsdb/samdb/ldb_modules/acl_read.c
+++ b/source4/dsdb/samdb/ldb_modules/acl_read.c
@@ -38,9 +38,6 @@
 #include "param/param.h"
 #include "dsdb/samdb/ldb_modules/util.h"
 
-/* The attributeSecurityGuid for the Public Information Property-Set */
-#define PUBLIC_INFO_PROPERTY_SET "e48d0154-bcf8-11d1-8702-00c04fb96050"
-
 struct aclread_context {
 	struct ldb_module *module;
 	struct ldb_request *req;
@@ -282,16 +279,13 @@ static int check_attr_access_rights(TALLOC_CTX *mem_ctx, const char *attr_name,
 				    struct aclread_context *ac,
 				    struct security_descriptor *sd,
 				    const struct dsdb_class *objectclass,
-				    struct dom_sid *sid, struct ldb_dn *dn,
-				    bool *is_public_info)
+				    struct dom_sid *sid, struct ldb_dn *dn)
 {
 	int ret;
 	const struct dsdb_attribute *attr = NULL;
 	uint32_t access_mask;
 	struct ldb_context *ldb = ldb_module_get_ctx(ac->module);
 
-	*is_public_info = false;
-
 	attr = dsdb_attribute_by_lDAPDisplayName(ac->schema, attr_name);
 	if (!attr) {
 		ldb_debug_set(ldb,
@@ -302,35 +296,6 @@ static int check_attr_access_rights(TALLOC_CTX *mem_ctx, const char *attr_name,
 		return LDB_SUCCESS;
 	}
 
-	/*
-	 * If we have no Read Property (RP) rights for a child object, it should
-	 * still appear as a visible object in 'objectClass=*' searches,
-	 * as long as we have List Contents (LC) rights for it.
-	 * This is needed for the acl.py tests (e.g. test_search1()).
-	 * I couldn't find the Windows behaviour documented in the specs, so
-	 * this is a guess, but it seems to only apply to attributes in the
-	 * Public Information Property Set that have the systemOnly flag set to
-	 * TRUE. (This makes sense in a way, as it's not disclosive to find out
-	 * that a child object has a 'objectClass' or 'name' attribute, as every
-	 * object has these attributes).
-	 */
-	if (attr->systemOnly) {
-		struct GUID public_info_guid;
-		NTSTATUS status;
-
-		status = GUID_from_string(PUBLIC_INFO_PROPERTY_SET,
-					  &public_info_guid);
-		if (!NT_STATUS_IS_OK(status)) {
-			ldb_set_errstring(ldb, "Public Info GUID parse error");
-			return LDB_ERR_OPERATIONS_ERROR;
-		}
-
-		if (GUID_compare(&attr->attributeSecurityGUID,
-				 &public_info_guid) == 0) {
-			*is_public_info = true;
-		}
-	}
-
 	access_mask = get_attr_access_mask(attr, ac->sd_flags);
 
 	/* the access-mask should be non-zero. Skip attribute otherwise */
@@ -399,8 +364,14 @@ static int parse_tree_check_attr_access(struct ldb_parse_tree *tree,
 {
 	struct parse_tree_aclread_ctx *ctx = NULL;
 	const char *attr_name = NULL;
-	bool is_public_info = false;
 	int ret;
+	static const char * const attrs_always_present[] = {
+		"objectClass",
+		"distinguishedName",
+		"name",
+		"objectGUID",
+		NULL
+	};
 
 	ctx = (struct parse_tree_aclread_ctx *)private_context;
 
@@ -418,9 +389,24 @@ static int parse_tree_check_attr_access(struct ldb_parse_tree *tree,
 		return LDB_SUCCESS;
 	}
 
+	/*
+	 * If the search filter is checking for an attribute's presence, and the
+	 * attribute is always present, we can skip access rights checks. Every
+	 * object has these attributes, and so there's no security reason to
+	 * hide their presence.
+	 * Note: the acl.py tests (e.g. test_search1()) rely on this exception.
+	 * I.e. even if we lack Read Property (RP) rights for a child object, it
+	 * should still appear as a visible object in 'objectClass=*' searches,
+	 * so long as we have List Contents (LC) rights for the object.
+	 */
+	if (tree->operation == LDB_OP_PRESENT &&
+	    is_attr_in_list(attrs_always_present, attr_name)) {
+		return LDB_SUCCESS;
+	}
+
 	ret = check_attr_access_rights(ctx->mem_ctx, attr_name, ctx->ac,
 				       ctx->sd, ctx->objectclass, ctx->sid,
-				       ctx->dn, &is_public_info);
+				       ctx->dn);
 
 	/*
 	 * if the user does not have the rights to view this attribute, then we
@@ -428,17 +414,6 @@ static int parse_tree_check_attr_access(struct ldb_parse_tree *tree,
 	 * object doesn't exist (for this particular user, at least)
 	 */
 	if (ret == LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS) {
-
-		/*
-		 * We make an exception for attribute=* searches involving the
-		 * Public Information property-set. This allows searches like
-		 * objectClass=* to return visible objects, even if the user
-		 * doesn't have Read Property rights on the attribute
-		 */
-		if (tree->operation == LDB_OP_PRESENT && is_public_info) {
-			return LDB_SUCCESS;
-		}
-
 		ctx->suppress_result = true;
 		return LDB_SUCCESS;
 	}
-- 
2.17.1


From 8ca74656ffee013171e04d96553f2174a42dc338 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Fri, 14 Sep 2018 14:06:30 +1200
Subject: [PATCH 3/3] tests: test ldap search requesting non-existent attribute

As an after-thought to commit 563e454e8c55e94a950, we thought it
might be a good idea to add a test case that requests an non-existent
attribute in the attribute-filter as well the search-filter.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
---
 source4/dsdb/tests/python/ldap.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/source4/dsdb/tests/python/ldap.py b/source4/dsdb/tests/python/ldap.py
index affc3b85a3b..37ccfb4edc7 100755
--- a/source4/dsdb/tests/python/ldap.py
+++ b/source4/dsdb/tests/python/ldap.py
@@ -637,6 +637,14 @@ class BasicTests(samba.tests.TestCase):
         self.assertTrue(len(res) == 1,
                         "Search including unknown attribute failed")
 
+        # likewise, if we specifically request an unknown attribute
+        res = ldb.search(base=self.base_dn,
+                         expression="(cn=ldaptestgroup)",
+                         scope=SCOPE_SUBTREE,
+                         attrs=["thisdoesnotexist"])
+        self.assertTrue(len(res) == 1,
+                        "Search requesting unknown attribute failed")
+
         delete_force(self.ldb, "cn=ldaptestgroup,cn=users," + self.base_dn)
 
         # attributes not in objectclasses and mandatory attributes missing test
-- 
2.17.1



More information about the samba-technical mailing list