From 710b0a6aaf1c648bc8fd33d4ab5bcc859a0ed851 Mon Sep 17 00:00:00 2001

From: Thierry Bordaz <tbordaz@redhat.com>

Date: Thu, 13 Apr 2017 15:21:49 +0200

Subject: [PATCH] Ticket 49184 - Overflow in memberof

Bug Description:

    The function memberof_call_foreach_dn can be used to retrieve ancestors of a

    given entry. (ancestors are groups owning directly or indirectly a given entry).

    With the use of group cache in memberof, at the entrance of memberof_call_foreach_dn

    there is an attempt to get the entry ancestors from the cache.

    Before doing so it needs to test if the cache is safe. In fact in case of

    circular groups the use of the cache is disabled and lookup in the cache should not

    happend.

    To know if the cache is safe it needs to access a flag (use_cache) in callback_data.

    The callback_data structure is opaque at this level. So accessing it

    while its structure is unknown is dangerous.

    The bug is that we may read an 'int' at an offset that overflow the actual structure.

    This is just a test and should not trigger a crash.

Fix Description:

    Add a flag to call memberof_call_foreach_dn so that, that indicates if

    it is valid to use the group cache.

https://pagure.io/389-ds-base/issue/49184

Reviewed by: William Brown and Mark Reynolds (thanks to you !!)

Platforms tested: F23

Flag Day: no

Doc impact: no

---

 dirsrvtests/tests/tickets/ticket49184_test.py | 146 ++++++++++++++++++++++++++

 ldap/servers/plugins/memberof/memberof.c      |  38 ++++---

 2 files changed, 167 insertions(+), 17 deletions(-)

 create mode 100644 dirsrvtests/tests/tickets/ticket49184_test.py

diff --git a/dirsrvtests/tests/tickets/ticket49184_test.py b/dirsrvtests/tests/tickets/ticket49184_test.py

new file mode 100644

index 0000000..20edfde

--- /dev/null

+++ b/dirsrvtests/tests/tickets/ticket49184_test.py

@@ -0,0 +1,146 @@

+import time

+import ldap

+import logging

+import pytest

+from lib389 import DirSrv, Entry, tools, tasks

+from lib389.tools import DirSrvTools

+from lib389._constants import *

+from lib389.properties import *

+from lib389.tasks import *

+from lib389.utils import *

+from lib389.topologies import topology_st as topo

+

+DEBUGGING = os.getenv("DEBUGGING", default=False)

+GROUP_DN_1 = ("cn=group1," + DEFAULT_SUFFIX)

+GROUP_DN_2 = ("cn=group2," + DEFAULT_SUFFIX)

+SUPER_GRP1 = ("cn=super_grp1,"  + DEFAULT_SUFFIX)

+SUPER_GRP2 = ("cn=super_grp2,"  + DEFAULT_SUFFIX)

+SUPER_GRP3 = ("cn=super_grp3,"  + DEFAULT_SUFFIX)

+

+if DEBUGGING:

+    logging.getLogger(__name__).setLevel(logging.DEBUG)

+else:

+    logging.getLogger(__name__).setLevel(logging.INFO)

+log = logging.getLogger(__name__)

+

+def _add_group_with_members(topo, group_dn):

+    # Create group

+    try:

+        topo.standalone.add_s(Entry((group_dn,

+                                      {'objectclass': 'top groupofnames extensibleObject'.split(),

+                                       'cn': 'group'})))

+    except ldap.LDAPError as e:

+        log.fatal('Failed to add group: error ' + e.message['desc'])

+        assert False

+

+    # Add members to the group - set timeout

+    log.info('Adding members to the group...')

+    for idx in range(1, 5):

+        try:

+            MEMBER_VAL = ("uid=member%d,%s" % (idx, DEFAULT_SUFFIX))

+            topo.standalone.modify_s(group_dn,

+                                      [(ldap.MOD_ADD,

+                                        'member',

+                                        MEMBER_VAL)])

+        except ldap.LDAPError as e:

+            log.fatal('Failed to update group: member (%s) - error: %s' %

+                      (MEMBER_VAL, e.message['desc']))

+            assert False

+

+def _check_memberof(topo, member=None, memberof=True, group_dn=None):

+    # Check that members have memberof attribute on M1

+    for idx in range(1, 5):

+        try:

+            USER_DN = ("uid=member%d,%s" % (idx, DEFAULT_SUFFIX))

+            ent = topo.standalone.getEntry(USER_DN, ldap.SCOPE_BASE, "(objectclass=*)")

+            if presence_flag:

+                assert ent.hasAttr('memberof') and ent.getValue('memberof') == group_dn

+            else:

+                assert not ent.hasAttr('memberof')

+        except ldap.LDAPError as e:

+            log.fatal('Failed to retrieve user (%s): error %s' % (USER_DN, e.message['desc']))

+            assert False

+            

+def _check_memberof(topo, member=None, memberof=True, group_dn=None):

+    ent = topo.standalone.getEntry(member, ldap.SCOPE_BASE, "(objectclass=*)")

+    if memberof:

+        assert group_dn

+        assert ent.hasAttr('memberof') and group_dn in ent.getValues('memberof')

+    else:

+        if ent.hasAttr('memberof'):

+            assert group_dn not in ent.getValues('memberof')

+

+            

+def test_ticket49184(topo):

+    """Write your testcase here...

+

+    Also, if you need any testcase initialization,

+    please, write additional fixture for that(include finalizer).

+    """

+    

+    topo.standalone.plugins.enable(name=PLUGIN_MEMBER_OF)

+    topo.standalone.restart(timeout=10)

+

+    #

+    #  create some users and a group

+    #

+    log.info('create users and group...')

+    for idx in range(1, 5):

+        try:

+            USER_DN = ("uid=member%d,%s" % (idx, DEFAULT_SUFFIX))

+            topo.standalone.add_s(Entry((USER_DN,

+                                          {'objectclass': 'top extensibleObject'.split(),

+                                           'uid': 'member%d' % (idx)})))

+        except ldap.LDAPError as e:

+            log.fatal('Failed to add user (%s): error %s' % (USER_DN, e.message['desc']))

+            assert False

+

+    # add all users in GROUP_DN_1 and checks each users is memberof GROUP_DN_1

+    _add_group_with_members(topo, GROUP_DN_1)

+    for idx in range(1, 5):

+        USER_DN = ("uid=member%d,%s" % (idx, DEFAULT_SUFFIX))

+        _check_memberof(topo, member=USER_DN, memberof=True, group_dn=GROUP_DN_1 )

+        

+    # add all users in GROUP_DN_2 and checks each users is memberof GROUP_DN_2

+    _add_group_with_members(topo, GROUP_DN_2)

+    for idx in range(1, 5):

+        USER_DN = ("uid=member%d,%s" % (idx, DEFAULT_SUFFIX))

+        _check_memberof(topo, member=USER_DN, memberof=True, group_dn=GROUP_DN_2 )

+    

+    # add the level 2, 3 and 4 group

+    for super_grp in (SUPER_GRP1, SUPER_GRP2, SUPER_GRP3):

+        topo.standalone.add_s(Entry((super_grp,

+                                          {'objectclass': 'top groupofnames extensibleObject'.split(),

+                                           'cn': 'super_grp'})))

+    topo.standalone.modify_s(SUPER_GRP1,

+                                      [(ldap.MOD_ADD,

+                                        'member',

+                                        GROUP_DN_1),

+                                        (ldap.MOD_ADD,

+                                        'member',

+                                        GROUP_DN_2)])

+    topo.standalone.modify_s(SUPER_GRP2,

+                                      [(ldap.MOD_ADD,

+                                        'member',

+                                        GROUP_DN_1),

+                                        (ldap.MOD_ADD,

+                                        'member',

+                                        GROUP_DN_2)])

+    return

+    topo.standalone.delete_s(GROUP_DN_2)

+    for idx in range(1, 5):

+        USER_DN = ("uid=member%d,%s" % (idx, DEFAULT_SUFFIX))

+        _check_memberof(topo, member=USER_DN, memberof=True, group_dn=GROUP_DN_1 )

+        _check_memberof(topo, member=USER_DN, memberof=False, group_dn=GROUP_DN_2 )

+    

+    if DEBUGGING:

+        # Add debugging steps(if any)...

+        pass

+

+

+if __name__ == '__main__':

+    # Run isolated

+    # -s for DEBUG mode

+    CURRENT_FILE = os.path.realpath(__file__)

+    pytest.main("-s %s" % CURRENT_FILE)

+

diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c

index 81ef092..5cd2c01 100644

--- a/ldap/servers/plugins/memberof/memberof.c

+++ b/ldap/servers/plugins/memberof/memberof.c

@@ -159,7 +159,7 @@ static int memberof_qsort_compare(const void *a, const void *b);

 static void memberof_load_array(Slapi_Value **array, Slapi_Attr *attr);

 static int memberof_del_dn_from_groups(Slapi_PBlock *pb, MemberOfConfig *config, Slapi_DN *sdn);

 static int memberof_call_foreach_dn(Slapi_PBlock *pb, Slapi_DN *sdn, MemberOfConfig *config,

-	char **types, plugin_search_entry_callback callback,  void *callback_data, int *cached);

+	char **types, plugin_search_entry_callback callback,  void *callback_data, int *cached, PRBool use_grp_cache);

 static int memberof_is_direct_member(MemberOfConfig *config, Slapi_Value *groupdn,

 	Slapi_Value *memberdn);

 static int memberof_is_grouping_attr(char *type, MemberOfConfig *config);

@@ -659,7 +659,7 @@ memberof_del_dn_from_groups(Slapi_PBlock *pb, MemberOfConfig *config, Slapi_DN *

 		slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_del_dn_from_groups: Ancestors of %s\n", slapi_sdn_get_dn(sdn));

 		rc = memberof_call_foreach_dn(pb, sdn, config, groupattrs,

-		                              memberof_del_dn_type_callback, &data, &cached);

+		                              memberof_del_dn_type_callback, &data, &cached, PR_FALSE);

 	}

 	return rc;

@@ -776,8 +776,8 @@ add_ancestors_cbdata(memberof_cached_value *ancestors, void *callback_data)

  * could want type to be either "member" or "memberOf" depending on the case.

  */

 int

-memberof_call_foreach_dn(Slapi_PBlock *pb, Slapi_DN *sdn,

-	MemberOfConfig *config, char **types, plugin_search_entry_callback callback, void *callback_data, int *cached)

+memberof_call_foreach_dn(Slapi_PBlock *pb __attribute__((unused)), Slapi_DN *sdn,

+	MemberOfConfig *config, char **types, plugin_search_entry_callback callback, void *callback_data, int *cached, PRBool use_grp_cache)

 {

 	Slapi_PBlock *search_pb = NULL;

 	Slapi_DN *base_sdn = NULL;

@@ -792,9 +792,6 @@ memberof_call_foreach_dn(Slapi_PBlock *pb, Slapi_DN *sdn,

 	int free_it = 0;

 	int rc = 0;

 	int i = 0;

-	memberof_cached_value *ht_grp = NULL;

-	memberof_get_groups_data *data = (memberof_get_groups_data*) callback_data;

-	const char *ndn = slapi_sdn_get_ndn(sdn);

 	*cached = 0;

@@ -802,17 +799,24 @@ memberof_call_foreach_dn(Slapi_PBlock *pb, Slapi_DN *sdn,

 		return (rc);

 	}

-	/* Here we will retrieve the ancestor of sdn.

-	 * The key access is the normalized sdn

-	 * This is done through recursive internal searches of parents

-	 * If the ancestors of sdn are already cached, just use

-	 * this value

+	/* This flags indicates memberof_call_foreach_dn is called to retrieve ancestors (groups).

+	 * To improve performance, it can use a cache. (it will not in case of circular groups)

+	 * When this flag is true it means no circular group are detected (so far) so we can use the cache

 	 */

-	if (data && data->use_cache) {

+	if (use_grp_cache) {

+		/* Here we will retrieve the ancestor of sdn.

+		 * The key access is the normalized sdn

+		 * This is done through recursive internal searches of parents

+		 * If the ancestors of sdn are already cached, just use

+		 * this value

+		 */

+		memberof_cached_value *ht_grp = NULL;

+		const char *ndn = slapi_sdn_get_ndn(sdn);

+		

 		ht_grp = ancestors_cache_lookup((const void *) ndn);

 		if (ht_grp) {

 #if MEMBEROF_CACHE_DEBUG

-		slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_call_foreach_dn: Ancestors of %s already cached (%x)\n", ndn, ht_grp);

+			slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_call_foreach_dn: Ancestors of %s already cached (%x)\n", ndn, ht_grp);

 #endif

 			add_ancestors_cbdata(ht_grp, callback_data);

 			*cached = 1;

@@ -1106,7 +1110,7 @@ memberof_replace_dn_from_groups(Slapi_PBlock *pb, MemberOfConfig *config,

 		slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_replace_dn_from_groups: Ancestors of %s\n", slapi_sdn_get_dn(post_sdn));

 		if((ret = memberof_call_foreach_dn(pb, pre_sdn, config, groupattrs,

 		                                   memberof_replace_dn_type_callback,

-		                                   &data, &cached)))

+		                                   &data, &cached, PR_FALSE)))

 		{

 			break;

 		}

@@ -2383,7 +2387,7 @@ memberof_get_groups_r(MemberOfConfig *config, Slapi_DN *member_sdn,

 	slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_get_groups_r: Ancestors of %s\n", slapi_sdn_get_dn(member_sdn));

 #endif

 	rc = memberof_call_foreach_dn(NULL, member_sdn, config, config->groupattrs,

-		memberof_get_groups_callback, &member_data, &cached);

+		memberof_get_groups_callback, &member_data, &cached, member_data.use_cache);

 	merge_ancestors(&member_ndn_val, &member_data, data);

 	if (!cached && member_data.use_cache)

@@ -2578,7 +2582,7 @@ memberof_test_membership(Slapi_PBlock *pb, MemberOfConfig *config,

 	int cached = 0;

 	return memberof_call_foreach_dn(pb, group_sdn, config, attrs,

-		memberof_test_membership_callback, config, &cached);

+		memberof_test_membership_callback, config, &cached, PR_FALSE);

 }

 /*

-- 

2.9.3