backport of:
From 2770f3556cc3002045790efc8edd04d3eda5e596 Mon Sep 17 00:00:00 2001
From: William Brown <william@blackhats.net.au>
Date: Thu, 26 May 2022 14:19:05 +1000
Subject: [PATCH] Issue 5170 - BUG - incorrect behaviour of filter test
Bug Description: In the filter test during access only
checks, OR conditions were not correctly evaluated. They
would have their access checked, but it was not confirmed
if this was the element that the entry matched. This mean
that queries could incorrectly reduce entries.
Fix Description: Remove the access check only mode from being
using in filter tests since it is broken, and requires the full
filter test to be evaluated along with it to work in complex cases.
fixes: https://github.com/389ds/389-ds-base/issues/5170
Author: William Brown <william@blackhats.net.au>
Review by: @mreynolds389
---
.../filter/filter_test_aci_with_optimiser.py | 91 +++++++++++++++++++
ldap/servers/slapd/back-ldbm/ldbm_search.c | 12 ++-
ldap/servers/slapd/filterentry.c | 16 +++-
3 files changed, 114 insertions(+), 5 deletions(-)
create mode 100644 dirsrvtests/tests/suites/filter/filter_test_aci_with_optimiser.py
diff --git a/dirsrvtests/tests/suites/filter/filter_test_aci_with_optimiser.py b/dirsrvtests/tests/suites/filter/filter_test_aci_with_optimiser.py
new file mode 100644
index 0000000000..fdf044a2fe
--- /dev/null
+++ b/dirsrvtests/tests/suites/filter/filter_test_aci_with_optimiser.py
@@ -0,0 +1,91 @@
+
+
+import ldap
+import logging
+import pytest
+import os
+from lib389._constants import *
+from lib389.topologies import topology_st as topo
+from lib389.idm.domain import Domain
+from lib389.idm.organizationalunit import OrganizationalUnits
+from lib389.idm.account import Anonymous
+
+log = logging.getLogger(__name__)
+
+
+def test_filter_access(topo):
+ """Search that compound filters are correctly processed by access control
+
+ :id: ad6a3ffc-2620-4e76-909b-926f94c1a920
+ :setup: Standalone Instance
+ :steps:
+ 1. Add anonymous aci
+ 2. Add ou
+ 2. Test good filters
+ 4. Test bad filters
+ :expectedresults:
+ 1. Success
+ 2. Success
+ 3. The good filters return the OU entry
+ 4. The bad filters do not return the OU entry
+ """
+
+ # Add aci
+ ACI_TEXT = ('(targetattr="objectclass || cn")(version 3.0; acl "Anonymous read access"; allow' +
+ '(read, search, compare) userdn = "ldap:///anyone";)')
+ domain = Domain(topo.standalone, DEFAULT_SUFFIX)
+ domain.replace('aci', ACI_TEXT)
+
+ # To remove noise, delete EVERYTHING else.
+
+ ous = OrganizationalUnits(topo.standalone, DEFAULT_SUFFIX)
+ existing_ous = ous.list()
+ for eou in existing_ous:
+ eou.delete(recursive=True)
+
+ # Create restricted entry
+ OU_PROPS = {
+ 'ou': 'restricted',
+ 'description': 'secret data'
+ }
+ ou = ous.create(properties=OU_PROPS)
+ OU_DN = ou.dn
+
+ # Do anonymous search using different filters
+ GOOD_FILTERS = [
+ "(|(objectClass=top)(&(objectClass=organizationalunit)(description=secret data)))",
+ "(|(&(objectClass=organizationalunit)(description=secret data))(objectClass=top))",
+ "(|(objectClass=organizationalunit)(description=secret data)(sn=*))",
+ "(|(description=secret data)(objectClass=organizationalunit)(sn=*))",
+ "(|(sn=*)(description=secret data)(objectClass=organizationalunit))",
+ "(objectClass=top)",
+ ]
+ BAD_FILTERS = [
+ "(|(objectClass=person)(&(objectClass=organizationalunit)(description=secret data)))",
+ "(&(objectClass=top)(objectClass=organizationalunit)(description=secret data))",
+ "(|(&(description=*)(objectClass=top))(objectClass=person))",
+ "(description=secret data)",
+ "(description=*)",
+ "(ou=*)",
+ ]
+ conn = Anonymous(topo.standalone).bind()
+
+ # These searches should return the OU
+ for search_filter in GOOD_FILTERS:
+ entries = conn.search_s(OU_DN, ldap.SCOPE_SUBTREE, search_filter)
+ log.debug(f"Testing good filter: {search_filter} result: {len(entries)}")
+ assert len(entries) == 1
+
+ # These searches should not return the OU
+ for search_filter in BAD_FILTERS:
+ entries = conn.search_s(OU_DN, ldap.SCOPE_SUBTREE, search_filter)
+ log.debug(f"Testing bad filter: {search_filter} result: {len(entries)}")
+ assert len(entries) == 0
+
+
+if __name__ == '__main__':
+ # Run isolated
+ # -s for DEBUG mode
+ CURRENT_FILE = os.path.realpath(__file__)
+ pytest.main(["-s", CURRENT_FILE])
+
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_search.c b/ldap/servers/slapd/back-ldbm/ldbm_search.c
index 9412172f14..3b032597b7 100644
--- a/ldap/servers/slapd/back-ldbm/ldbm_search.c
+++ b/ldap/servers/slapd/back-ldbm/ldbm_search.c
@@ -1658,9 +1658,9 @@ ldbm_back_next_search_entry(Slapi_PBlock *pb)
if (0 != (sr->sr_flags & SR_FLAG_CAN_SKIP_FILTER_TEST)) {
/* Since we do access control checking in the filter test (?Why?) we need to check access now */
slapi_log_err(SLAPI_LOG_FILTER, "ldbm_back_next_search_entry_ext",
- "Bypassing filter test\n");
+ "Bypassing filter test for %s\n", slapi_entry_get_dn_const(e->ep_entry));
if (ACL_CHECK_FLAG) {
- filter_test = slapi_vattr_filter_test_ext(pb, e->ep_entry, filter, ACL_CHECK_FLAG, 1 /* Only perform access checking, thank you */);
+ filter_test = slapi_vattr_filter_test(pb, e->ep_entry, filter, ACL_CHECK_FLAG);
} else {
filter_test = 0;
}
@@ -1679,9 +1679,18 @@ ldbm_back_next_search_entry(Slapi_PBlock *pb)
}
} else {
/* Old-style case---we need to do a filter test */
+ slapi_log_err(SLAPI_LOG_FILTER, "ldbm_back_next_search_entry",
+ "Applying filter test to %s\n", slapi_entry_get_dn_const(e->ep_entry));
filter_test = slapi_vattr_filter_test(pb, e->ep_entry, filter, ACL_CHECK_FLAG);
+ slapi_log_err(SLAPI_LOG_FILTER, "ldbm_back_next_search_entry",
+ "Applying filter test intermediate value %d \n", filter_test);
+ if (filter_test == 0) {
+ filter_test = slapi_vattr_filter_test(pb, e->ep_entry, filter, 0);
+ }
}
}
+ slapi_log_err(SLAPI_LOG_FILTER, "ldbm_back_next_search_entry",
+ "filter test value %d %s \n", filter_test, slapi_entry_get_dn_const(e->ep_entry));
if ((filter_test == 0) || (sr->sr_virtuallistview && (filter_test != -1)))
/* ugaston - if filter failed due to subentries or tombstones (filter_test=-1),
* just forget about it, since we don't want to return anything at all. */
diff --git a/ldap/servers/slapd/filterentry.c b/ldap/servers/slapd/filterentry.c
index af14be544b..c5b42516f0 100644
--- a/ldap/servers/slapd/filterentry.c
+++ b/ldap/servers/slapd/filterentry.c
@@ -764,6 +764,12 @@ slapi_vattr_filter_test_ext(
int rc = 0; /* a no op request succeeds */
int access_check_done = 0;
+ if (only_check_access != 0) {
+ slapi_log_err(SLAPI_LOG_ERR, "slapi_vattr_filter_test_ext",
+ "⚠️ DANGER ⚠️ - only_check_access mode is BROKEN!!! YOU MUST CHECK ACCESS WITH FILTER MATCHING");
+ }
+ PR_ASSERT(only_check_access == 0);
+
/* Fix for ticket 48275
* If we want to handle or components which can contain nonmatching components without access propoerly
* always filter verification and access check have to be done together for each component
@@ -940,6 +946,9 @@ test_filter_access(Slapi_PBlock *pb,
rc = plugin_call_acl_plugin(pb, e, attrs, attr_val,
SLAPI_ACL_SEARCH, ACLPLUGIN_ACCESS_DEFAULT, NULL);
+ slapi_log_err(SLAPI_LOG_FILTER, "slapi_vattr_filter_test_ext_internal",
+ "acl result for %s %s = %d\n", slapi_entry_get_dn_const(e), attr_type, rc);
+
return (rc);
}
@@ -1013,14 +1022,17 @@ vattr_test_filter_list_or(
continue;
}
}
- if (only_check_access)
- continue;
/* now check if filter matches */
+ /*
+ * We can NOT skip this because we need to know if the item we matched on
+ * is the item with access denied.
+ */
undefined = 0;
rc = slapi_vattr_filter_test_ext_internal(pb, e, f, 0, 0, access_check_done);
if (rc == 0) {
undefined = 0;
nomatch = 0;
+ /* We matched, and have access. we can now return */
break;
} else if (rc > 0) {
undefined = rc;
