[389-ds-base] branch 389-ds-base-1.3.8 updated: Ticket 50020 - during MODRDN referential integrity can fail erronously while updating large groups

[git repository hosting]

This is an automated email from the git hooks/post-receive script.

tbordaz pushed a commit to branch 389-ds-base-1.3.8
in repository 389-ds-base.

The following commit(s) were added to refs/heads/389-ds-base-1.3.8 by this push:
     new 3800388  Ticket 50020 - during MODRDN referential integrity can fail erronously while updating large groups
3800388 is described below

commit 38003882f03a4e2473ef2fef524cf153e691de0b
Author: Thierry Bordaz 
AuthorDate: Fri Nov 9 17:07:11 2018 +0100

Ticket 50020 - during MODRDN referential integrity can fail erronously while updating large groups

Bug Description:
    	During a MODRDN of a group member, referential integrity will update the groups containing this member.
    	Under specific conditions, the MODRDN can fail (err=1).

on MODRDN Referential integrity checks if the original DN of the target MODRDN entry is
    	member of a given group. If it is then it updates the group.
    	The returned code of the group update is using the variable 'rc'.
    	It does a normalized DN comparison to compare original DN with members DN, to determine if
    	a group needs to be updated.
    	If the group does not need to be updated, 'rc' is not set.
    	The bug is that it uses 'rc' to normalize the DN and if the group is not updated
    	the returned code reflects the normalization returned code rather that the group update.

The bug is hit in specific conditions

One of the evaluated group contains more than 128 members
    	    the last member (last value) of the group is not the moved entry
    	    the last member (last value) of the group is a DN value that contains escaped chars

Fix Description:
    	Use a local variable to check the result of the DN normalization

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

Reviewed by: Simon Pichugin, Mark Reynolds (thanks)

Platforms tested: F27

Flag Day: no
---
 dirsrvtests/tests/suites/plugins/referint_test.py | 103 ++++++++++++++++++++++
 ldap/servers/plugins/referint/referint.c          |  18 ++--
 2 files changed, 113 insertions(+), 8 deletions(-)

diff --git a/dirsrvtests/tests/suites/plugins/referint_test.py b/dirsrvtests/tests/suites/plugins/referint_test.py
new file mode 100644
index 0000000..67a11de
--- /dev/null
+++ b/dirsrvtests/tests/suites/plugins/referint_test.py
@@ -0,0 +1,103 @@
+# --- BEGIN COPYRIGHT BLOCK ---
+# Copyright (C) 2016 Red Hat, Inc.
+# All rights reserved.
+#
+# License: GPL (version 3 or any later version).
+# See LICENSE for details.
+# --- END COPYRIGHT BLOCK ---
+#
+'''
+Created on Dec 12, 2019
+
+@author: tbordaz
+'''
+import logging
+import subprocess
+import pytest
+from lib389 import Entry
+from lib389.utils import *
+from lib389.plugins import *
+from lib389._constants import *
+from lib389.idm.user import UserAccounts, UserAccount
+from lib389.idm.group import Groups
+from lib389.topologies import topology_st as topo
+
+log = logging.getLogger(__name__)
+
+ESCAPED_RDN_BASE = "foo,oo"
+def _user_get_dn(no):
+    uid = '%s%d' % (ESCAPED_RDN_BASE, no)
+    dn = 'uid=%s,%s' % (uid, SUFFIX)
+    return (uid, dn)
+
+def add_escaped_user(server, no):
+    (uid, dn) = _user_get_dn(no)
+    log.fatal('Adding user (%s): ' % dn)
+    server.add_s(Entry((dn, {'objectclass': ['top', 'person', 'organizationalPerson', 'inetOrgPerson'],
+                             'uid': [uid],
+                             'sn' : [uid],
+                             'cn' : [uid]})))
+    return dn
+
[e-mail]50020
+def test_referential_false_failure(topo):
+    """On MODRDN referential integrity can erronously fail
+
+    :id: f77aeb80-c4c4-471b-8c1b-4733b714778b
+    :setup: Standalone Instance
+    :steps:
+        1. Configure the plugin
+        2. Create a group
+            - 1rst member the one that will be move
+            - more than 128 members
+            - last member is a DN containing escaped char
+        3. Rename the 1rst member
+    :expectedresults:
+        1. should succeed
+        2. should succeed
+        3. should succeed
+    """
+
+    inst = topo[0]
+
+    # stop the plugin, and start it
+    plugin = ReferentialIntegrityPlugin(inst)
+    plugin.disable()
+    plugin.enable()
+
+    ############################################################################
+    # Configure plugin
+    ############################################################################
+    GROUP_CONTAINER = "ou=groups,%s" % DEFAULT_SUFFIX
+    plugin.replace('referint-membership-attr', 'member')
+    plugin.replace('nsslapd-plugincontainerscope', GROUP_CONTAINER)
+
+    ############################################################################
+    # Creates a group with members having escaped DN
+    ############################################################################
+    # Add some users and a group
+    users = UserAccounts(inst, DEFAULT_SUFFIX, None)
+    user1 = users.create_test_user(uid=1001)
+    user2 = users.create_test_user(uid=1002)
+
+    groups = Groups(inst, GROUP_CONTAINER, None)
+    group = groups.create(properties={'cn': 'group'})
+    group.add('member', user2.dn)
+    group.add('member', user1.dn)
+
+    # Add more than 128 members so that referint follows the buggy path
+    for i in range(130):
+        escaped_user = add_escaped_user(inst, i)
+        group.add('member', escaped_user)
+
+    ############################################################################
+    # Check that the MODRDN succeeds
+    ###########################################################################
+    # Here we need to restart so that member values are taken in the right order
+    # the last value is the escaped one
+    inst.restart()
+
+    # Here if the bug is fixed, referential is able to update the member value
+    inst.rename_s(user1.dn, 'uid=new_test_user_1001', newsuperior=SUFFIX, delold=0)
+
+
diff --git a/ldap/servers/plugins/referint/referint.c b/ldap/servers/plugins/referint/referint.c
index f6d1c27..9e4e680 100644
--- a/ldap/servers/plugins/referint/referint.c
+++ b/ldap/servers/plugins/referint/referint.c
@@ -824,20 +824,21 @@ _update_one_per_mod(Slapi_DN *entrySDN, /* DN of the searched entry */
          */
         for (nval = slapi_attr_first_value(attr, &v); nval != -1;
              nval = slapi_attr_next_value(attr, nval, &v)) {
+            int normalize_rc;
             p = NULL;
             dnlen = 0;

/* DN syntax, which should be a string */
             sval = slapi_ch_strdup(slapi_value_get_string(v));
-            rc = slapi_dn_normalize_case_ext(sval, 0, &p, &dnlen);
-            if (rc == 0) { /* sval is passed in; not terminated */
+            normalize_rc = slapi_dn_normalize_case_ext(sval, 0, &p, &dnlen);
+            if (normalize_rc == 0) { /* sval is passed in; not terminated */
                 *(p + dnlen) = '\0';
                 sval = p;
-            } else if (rc > 0) {
+            } else if (normalize_rc > 0) {
                 slapi_ch_free_string(&sval);
                 sval = p;
             }
-            /* else: (rc < 0) Ignore the DN normalization error for now. */
+            /* else: (normalize_rc < 0) Ignore the DN normalization error for now. */

p = PL_strstr(sval, slapi_sdn_get_ndn(origDN));
             if (p == sval) {
@@ -1013,20 +1014,21 @@ _update_all_per_mod(Slapi_DN *entrySDN, /* DN of the searched entry */
         for (nval = slapi_attr_first_value(attr, &v);
              nval != -1;
              nval = slapi_attr_next_value(attr, nval, &v)) {
+            int normalize_rc;
             p = NULL;
             dnlen = 0;

/* DN syntax, which should be a string */
             sval = slapi_ch_strdup(slapi_value_get_string(v));
-            rc = slapi_dn_normalize_case_ext(sval, 0, &p, &dnlen);
-            if (rc == 0) { /* sval is passed in; not terminated */
+            normalize_rc = slapi_dn_normalize_case_ext(sval, 0, &p, &dnlen);
+            if (normalize_rc == 0) { /* sval is passed in; not terminated */
                 *(p + dnlen) = '\0';
                 sval = p;
-            } else if (rc > 0) {
+            } else if (normalize_rc > 0) {
                 slapi_ch_free_string(&sval);
                 sval = p;
             }
-            /* else: (rc < 0) Ignore the DN normalization error for now. */
+            /* else: normalize_rc < 0) Ignore the DN normalization error for now. */

p = PL_strstr(sval, slapi_sdn_get_ndn(origDN));
             if (p == sval) {