74ca47
From 710b0a6aaf1c648bc8fd33d4ab5bcc859a0ed851 Mon Sep 17 00:00:00 2001
74ca47
From: Thierry Bordaz <tbordaz@redhat.com>
74ca47
Date: Thu, 13 Apr 2017 15:21:49 +0200
74ca47
Subject: [PATCH] Ticket 49184 - Overflow in memberof
74ca47
74ca47
Bug Description:
74ca47
    The function memberof_call_foreach_dn can be used to retrieve ancestors of a
74ca47
    given entry. (ancestors are groups owning directly or indirectly a given entry).
74ca47
74ca47
    With the use of group cache in memberof, at the entrance of memberof_call_foreach_dn
74ca47
    there is an attempt to get the entry ancestors from the cache.
74ca47
74ca47
    Before doing so it needs to test if the cache is safe. In fact in case of
74ca47
    circular groups the use of the cache is disabled and lookup in the cache should not
74ca47
    happend.
74ca47
74ca47
    To know if the cache is safe it needs to access a flag (use_cache) in callback_data.
74ca47
    The callback_data structure is opaque at this level. So accessing it
74ca47
    while its structure is unknown is dangerous.
74ca47
74ca47
    The bug is that we may read an 'int' at an offset that overflow the actual structure.
74ca47
    This is just a test and should not trigger a crash.
74ca47
74ca47
Fix Description:
74ca47
    Add a flag to call memberof_call_foreach_dn so that, that indicates if
74ca47
    it is valid to use the group cache.
74ca47
74ca47
https://pagure.io/389-ds-base/issue/49184
74ca47
74ca47
Reviewed by: William Brown and Mark Reynolds (thanks to you !!)
74ca47
74ca47
Platforms tested: F23
74ca47
74ca47
Flag Day: no
74ca47
74ca47
Doc impact: no
74ca47
---
74ca47
 dirsrvtests/tests/tickets/ticket49184_test.py | 146 ++++++++++++++++++++++++++
74ca47
 ldap/servers/plugins/memberof/memberof.c      |  38 ++++---
74ca47
 2 files changed, 167 insertions(+), 17 deletions(-)
74ca47
 create mode 100644 dirsrvtests/tests/tickets/ticket49184_test.py
74ca47
74ca47
diff --git a/dirsrvtests/tests/tickets/ticket49184_test.py b/dirsrvtests/tests/tickets/ticket49184_test.py
74ca47
new file mode 100644
74ca47
index 0000000..20edfde
74ca47
--- /dev/null
74ca47
+++ b/dirsrvtests/tests/tickets/ticket49184_test.py
74ca47
@@ -0,0 +1,146 @@
74ca47
+import time
74ca47
+import ldap
74ca47
+import logging
74ca47
+import pytest
74ca47
+from lib389 import DirSrv, Entry, tools, tasks
74ca47
+from lib389.tools import DirSrvTools
74ca47
+from lib389._constants import *
74ca47
+from lib389.properties import *
74ca47
+from lib389.tasks import *
74ca47
+from lib389.utils import *
74ca47
+from lib389.topologies import topology_st as topo
74ca47
+
74ca47
+DEBUGGING = os.getenv("DEBUGGING", default=False)
74ca47
+GROUP_DN_1 = ("cn=group1," + DEFAULT_SUFFIX)
74ca47
+GROUP_DN_2 = ("cn=group2," + DEFAULT_SUFFIX)
74ca47
+SUPER_GRP1 = ("cn=super_grp1,"  + DEFAULT_SUFFIX)
74ca47
+SUPER_GRP2 = ("cn=super_grp2,"  + DEFAULT_SUFFIX)
74ca47
+SUPER_GRP3 = ("cn=super_grp3,"  + DEFAULT_SUFFIX)
74ca47
+
74ca47
+if DEBUGGING:
74ca47
+    logging.getLogger(__name__).setLevel(logging.DEBUG)
74ca47
+else:
74ca47
+    logging.getLogger(__name__).setLevel(logging.INFO)
74ca47
+log = logging.getLogger(__name__)
74ca47
+
74ca47
+def _add_group_with_members(topo, group_dn):
74ca47
+    # Create group
74ca47
+    try:
74ca47
+        topo.standalone.add_s(Entry((group_dn,
74ca47
+                                      {'objectclass': 'top groupofnames extensibleObject'.split(),
74ca47
+                                       'cn': 'group'})))
74ca47
+    except ldap.LDAPError as e:
74ca47
+        log.fatal('Failed to add group: error ' + e.message['desc'])
74ca47
+        assert False
74ca47
+
74ca47
+    # Add members to the group - set timeout
74ca47
+    log.info('Adding members to the group...')
74ca47
+    for idx in range(1, 5):
74ca47
+        try:
74ca47
+            MEMBER_VAL = ("uid=member%d,%s" % (idx, DEFAULT_SUFFIX))
74ca47
+            topo.standalone.modify_s(group_dn,
74ca47
+                                      [(ldap.MOD_ADD,
74ca47
+                                        'member',
74ca47
+                                        MEMBER_VAL)])
74ca47
+        except ldap.LDAPError as e:
74ca47
+            log.fatal('Failed to update group: member (%s) - error: %s' %
74ca47
+                      (MEMBER_VAL, e.message['desc']))
74ca47
+            assert False
74ca47
+
74ca47
+def _check_memberof(topo, member=None, memberof=True, group_dn=None):
74ca47
+    # Check that members have memberof attribute on M1
74ca47
+    for idx in range(1, 5):
74ca47
+        try:
74ca47
+            USER_DN = ("uid=member%d,%s" % (idx, DEFAULT_SUFFIX))
74ca47
+            ent = topo.standalone.getEntry(USER_DN, ldap.SCOPE_BASE, "(objectclass=*)")
74ca47
+            if presence_flag:
74ca47
+                assert ent.hasAttr('memberof') and ent.getValue('memberof') == group_dn
74ca47
+            else:
74ca47
+                assert not ent.hasAttr('memberof')
74ca47
+        except ldap.LDAPError as e:
74ca47
+            log.fatal('Failed to retrieve user (%s): error %s' % (USER_DN, e.message['desc']))
74ca47
+            assert False
74ca47
+            
74ca47
+def _check_memberof(topo, member=None, memberof=True, group_dn=None):
74ca47
+    ent = topo.standalone.getEntry(member, ldap.SCOPE_BASE, "(objectclass=*)")
74ca47
+    if memberof:
74ca47
+        assert group_dn
74ca47
+        assert ent.hasAttr('memberof') and group_dn in ent.getValues('memberof')
74ca47
+    else:
74ca47
+        if ent.hasAttr('memberof'):
74ca47
+            assert group_dn not in ent.getValues('memberof')
74ca47
+
74ca47
+            
74ca47
+def test_ticket49184(topo):
74ca47
+    """Write your testcase here...
74ca47
+
74ca47
+    Also, if you need any testcase initialization,
74ca47
+    please, write additional fixture for that(include finalizer).
74ca47
+    """
74ca47
+    
74ca47
+    topo.standalone.plugins.enable(name=PLUGIN_MEMBER_OF)
74ca47
+    topo.standalone.restart(timeout=10)
74ca47
+
74ca47
+    #
74ca47
+    #  create some users and a group
74ca47
+    #
74ca47
+    log.info('create users and group...')
74ca47
+    for idx in range(1, 5):
74ca47
+        try:
74ca47
+            USER_DN = ("uid=member%d,%s" % (idx, DEFAULT_SUFFIX))
74ca47
+            topo.standalone.add_s(Entry((USER_DN,
74ca47
+                                          {'objectclass': 'top extensibleObject'.split(),
74ca47
+                                           'uid': 'member%d' % (idx)})))
74ca47
+        except ldap.LDAPError as e:
74ca47
+            log.fatal('Failed to add user (%s): error %s' % (USER_DN, e.message['desc']))
74ca47
+            assert False
74ca47
+
74ca47
+    # add all users in GROUP_DN_1 and checks each users is memberof GROUP_DN_1
74ca47
+    _add_group_with_members(topo, GROUP_DN_1)
74ca47
+    for idx in range(1, 5):
74ca47
+        USER_DN = ("uid=member%d,%s" % (idx, DEFAULT_SUFFIX))
74ca47
+        _check_memberof(topo, member=USER_DN, memberof=True, group_dn=GROUP_DN_1 )
74ca47
+        
74ca47
+    # add all users in GROUP_DN_2 and checks each users is memberof GROUP_DN_2
74ca47
+    _add_group_with_members(topo, GROUP_DN_2)
74ca47
+    for idx in range(1, 5):
74ca47
+        USER_DN = ("uid=member%d,%s" % (idx, DEFAULT_SUFFIX))
74ca47
+        _check_memberof(topo, member=USER_DN, memberof=True, group_dn=GROUP_DN_2 )
74ca47
+    
74ca47
+    # add the level 2, 3 and 4 group
74ca47
+    for super_grp in (SUPER_GRP1, SUPER_GRP2, SUPER_GRP3):
74ca47
+        topo.standalone.add_s(Entry((super_grp,
74ca47
+                                          {'objectclass': 'top groupofnames extensibleObject'.split(),
74ca47
+                                           'cn': 'super_grp'})))
74ca47
+    topo.standalone.modify_s(SUPER_GRP1,
74ca47
+                                      [(ldap.MOD_ADD,
74ca47
+                                        'member',
74ca47
+                                        GROUP_DN_1),
74ca47
+                                        (ldap.MOD_ADD,
74ca47
+                                        'member',
74ca47
+                                        GROUP_DN_2)])
74ca47
+    topo.standalone.modify_s(SUPER_GRP2,
74ca47
+                                      [(ldap.MOD_ADD,
74ca47
+                                        'member',
74ca47
+                                        GROUP_DN_1),
74ca47
+                                        (ldap.MOD_ADD,
74ca47
+                                        'member',
74ca47
+                                        GROUP_DN_2)])
74ca47
+    return
74ca47
+    topo.standalone.delete_s(GROUP_DN_2)
74ca47
+    for idx in range(1, 5):
74ca47
+        USER_DN = ("uid=member%d,%s" % (idx, DEFAULT_SUFFIX))
74ca47
+        _check_memberof(topo, member=USER_DN, memberof=True, group_dn=GROUP_DN_1 )
74ca47
+        _check_memberof(topo, member=USER_DN, memberof=False, group_dn=GROUP_DN_2 )
74ca47
+    
74ca47
+    if DEBUGGING:
74ca47
+        # Add debugging steps(if any)...
74ca47
+        pass
74ca47
+
74ca47
+
74ca47
+if __name__ == '__main__':
74ca47
+    # Run isolated
74ca47
+    # -s for DEBUG mode
74ca47
+    CURRENT_FILE = os.path.realpath(__file__)
74ca47
+    pytest.main("-s %s" % CURRENT_FILE)
74ca47
+
74ca47
diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c
74ca47
index 81ef092..5cd2c01 100644
74ca47
--- a/ldap/servers/plugins/memberof/memberof.c
74ca47
+++ b/ldap/servers/plugins/memberof/memberof.c
74ca47
@@ -159,7 +159,7 @@ static int memberof_qsort_compare(const void *a, const void *b);
74ca47
 static void memberof_load_array(Slapi_Value **array, Slapi_Attr *attr);
74ca47
 static int memberof_del_dn_from_groups(Slapi_PBlock *pb, MemberOfConfig *config, Slapi_DN *sdn);
74ca47
 static int memberof_call_foreach_dn(Slapi_PBlock *pb, Slapi_DN *sdn, MemberOfConfig *config,
74ca47
-	char **types, plugin_search_entry_callback callback,  void *callback_data, int *cached);
74ca47
+	char **types, plugin_search_entry_callback callback,  void *callback_data, int *cached, PRBool use_grp_cache);
74ca47
 static int memberof_is_direct_member(MemberOfConfig *config, Slapi_Value *groupdn,
74ca47
 	Slapi_Value *memberdn);
74ca47
 static int memberof_is_grouping_attr(char *type, MemberOfConfig *config);
74ca47
@@ -659,7 +659,7 @@ memberof_del_dn_from_groups(Slapi_PBlock *pb, MemberOfConfig *config, Slapi_DN *
74ca47
 
74ca47
 		slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_del_dn_from_groups: Ancestors of %s\n", slapi_sdn_get_dn(sdn));
74ca47
 		rc = memberof_call_foreach_dn(pb, sdn, config, groupattrs,
74ca47
-		                              memberof_del_dn_type_callback, &data, &cached);
74ca47
+		                              memberof_del_dn_type_callback, &data, &cached, PR_FALSE);
74ca47
 	}
74ca47
 
74ca47
 	return rc;
74ca47
@@ -776,8 +776,8 @@ add_ancestors_cbdata(memberof_cached_value *ancestors, void *callback_data)
74ca47
  * could want type to be either "member" or "memberOf" depending on the case.
74ca47
  */
74ca47
 int
74ca47
-memberof_call_foreach_dn(Slapi_PBlock *pb, Slapi_DN *sdn,
74ca47
-	MemberOfConfig *config, char **types, plugin_search_entry_callback callback, void *callback_data, int *cached)
74ca47
+memberof_call_foreach_dn(Slapi_PBlock *pb __attribute__((unused)), Slapi_DN *sdn,
74ca47
+	MemberOfConfig *config, char **types, plugin_search_entry_callback callback, void *callback_data, int *cached, PRBool use_grp_cache)
74ca47
 {
74ca47
 	Slapi_PBlock *search_pb = NULL;
74ca47
 	Slapi_DN *base_sdn = NULL;
74ca47
@@ -792,9 +792,6 @@ memberof_call_foreach_dn(Slapi_PBlock *pb, Slapi_DN *sdn,
74ca47
 	int free_it = 0;
74ca47
 	int rc = 0;
74ca47
 	int i = 0;
74ca47
-	memberof_cached_value *ht_grp = NULL;
74ca47
-	memberof_get_groups_data *data = (memberof_get_groups_data*) callback_data;
74ca47
-	const char *ndn = slapi_sdn_get_ndn(sdn);
74ca47
 
74ca47
 	*cached = 0;
74ca47
 
74ca47
@@ -802,17 +799,24 @@ memberof_call_foreach_dn(Slapi_PBlock *pb, Slapi_DN *sdn,
74ca47
 		return (rc);
74ca47
 	}
74ca47
 
74ca47
-	/* Here we will retrieve the ancestor of sdn.
74ca47
-	 * The key access is the normalized sdn
74ca47
-	 * This is done through recursive internal searches of parents
74ca47
-	 * If the ancestors of sdn are already cached, just use
74ca47
-	 * this value
74ca47
+	/* This flags indicates memberof_call_foreach_dn is called to retrieve ancestors (groups).
74ca47
+	 * To improve performance, it can use a cache. (it will not in case of circular groups)
74ca47
+	 * When this flag is true it means no circular group are detected (so far) so we can use the cache
74ca47
 	 */
74ca47
-	if (data && data->use_cache) {
74ca47
+	if (use_grp_cache) {
74ca47
+		/* Here we will retrieve the ancestor of sdn.
74ca47
+		 * The key access is the normalized sdn
74ca47
+		 * This is done through recursive internal searches of parents
74ca47
+		 * If the ancestors of sdn are already cached, just use
74ca47
+		 * this value
74ca47
+		 */
74ca47
+		memberof_cached_value *ht_grp = NULL;
74ca47
+		const char *ndn = slapi_sdn_get_ndn(sdn);
74ca47
+		
74ca47
 		ht_grp = ancestors_cache_lookup((const void *) ndn);
74ca47
 		if (ht_grp) {
74ca47
 #if MEMBEROF_CACHE_DEBUG
74ca47
-		slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_call_foreach_dn: Ancestors of %s already cached (%x)\n", ndn, ht_grp);
74ca47
+			slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_call_foreach_dn: Ancestors of %s already cached (%x)\n", ndn, ht_grp);
74ca47
 #endif
74ca47
 			add_ancestors_cbdata(ht_grp, callback_data);
74ca47
 			*cached = 1;
74ca47
@@ -1106,7 +1110,7 @@ memberof_replace_dn_from_groups(Slapi_PBlock *pb, MemberOfConfig *config,
74ca47
 		slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_replace_dn_from_groups: Ancestors of %s\n", slapi_sdn_get_dn(post_sdn));
74ca47
 		if((ret = memberof_call_foreach_dn(pb, pre_sdn, config, groupattrs,
74ca47
 		                                   memberof_replace_dn_type_callback,
74ca47
-		                                   &data, &cached)))
74ca47
+		                                   &data, &cached, PR_FALSE)))
74ca47
 		{
74ca47
 			break;
74ca47
 		}
74ca47
@@ -2383,7 +2387,7 @@ memberof_get_groups_r(MemberOfConfig *config, Slapi_DN *member_sdn,
74ca47
 	slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_get_groups_r: Ancestors of %s\n", slapi_sdn_get_dn(member_sdn));
74ca47
 #endif
74ca47
 	rc = memberof_call_foreach_dn(NULL, member_sdn, config, config->groupattrs,
74ca47
-		memberof_get_groups_callback, &member_data, &cached);
74ca47
+		memberof_get_groups_callback, &member_data, &cached, member_data.use_cache);
74ca47
 
74ca47
 	merge_ancestors(&member_ndn_val, &member_data, data);
74ca47
 	if (!cached && member_data.use_cache)
74ca47
@@ -2578,7 +2582,7 @@ memberof_test_membership(Slapi_PBlock *pb, MemberOfConfig *config,
74ca47
 	int cached = 0;
74ca47
 
74ca47
 	return memberof_call_foreach_dn(pb, group_sdn, config, attrs,
74ca47
-		memberof_test_membership_callback, config, &cached);
74ca47
+		memberof_test_membership_callback, config, &cached, PR_FALSE);
74ca47
 }
74ca47
 
74ca47
 /*
74ca47
-- 
74ca47
2.9.3
74ca47