From dcf75750dff23e848cde2ae63a0778b123de6dd7 Mon Sep 17 00:00:00 2001

From: William Brown <firstyear@redhat.com>

Date: Thu, 2 Nov 2017 13:32:41 +1000

Subject: [PATCH] Ticket 49436 - double free in COS in some conditions

Bug Description:  virtualattrs and COS have some serious memory

ownership issues. What was happening is that COS with multiple

attributes using the same sp_handle would cause a structure

to be registered twice. During shutdown we would then trigger

a double free in the process.

Fix Description:  Change the behaviour of sp_handles to use a

handle *per* attribute we register to guarantee the assocation

between them.

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

Author: wibrown

Review by: mreynolds, vashirov (Thanks!)

(cherry pick from commit ee4428a3f5d2d8e37a7107c7dce9d622fc17d41c)

---

 dirsrvtests/tests/suites/cos/indirect_cos_test.py | 43 +++++++----------------

 ldap/servers/plugins/cos/cos_cache.c              | 32 +++++++++--------

 ldap/servers/plugins/roles/roles_cache.c          |  8 ++---

 ldap/servers/slapd/vattr.c                        | 28 +++++++++------

 4 files changed, 51 insertions(+), 60 deletions(-)

diff --git a/dirsrvtests/tests/suites/cos/indirect_cos_test.py b/dirsrvtests/tests/suites/cos/indirect_cos_test.py

index 1aac6b8ed..452edcdf8 100644

--- a/dirsrvtests/tests/suites/cos/indirect_cos_test.py

+++ b/dirsrvtests/tests/suites/cos/indirect_cos_test.py

@@ -7,6 +7,7 @@ import subprocess

 from lib389 import Entry

 from lib389.idm.user import UserAccounts

+from lib389.idm.domain import Domain

 from lib389.topologies import topology_st as topo

 from lib389._constants import (DEFAULT_SUFFIX, DN_DM, PASSWORD, HOST_STANDALONE,

                                SERVERID_STANDALONE, PORT_STANDALONE)

@@ -48,14 +49,8 @@ def check_user(inst):

 def setup_subtree_policy(topo):

     """Set up subtree password policy

     """

-    try:

-        topo.standalone.modify_s("cn=config", [(ldap.MOD_REPLACE,

-                                                'nsslapd-pwpolicy-local',

-                                                'on')])

-    except ldap.LDAPError as e:

-        log.error('Failed to set fine-grained policy: error {}'.format(

-            e.message['desc']))

-        raise e

+

+    topo.standalone.config.set('nsslapd-pwpolicy-local', 'on')

     log.info('Create password policy for subtree {}'.format(OU_PEOPLE))

     try:

@@ -68,15 +63,9 @@ def setup_subtree_policy(topo):

             OU_PEOPLE, e.message['desc']))

         raise e

-    log.info('Add pwdpolicysubentry attribute to {}'.format(OU_PEOPLE))

-    try:

-        topo.standalone.modify_s(DEFAULT_SUFFIX, [(ldap.MOD_REPLACE,

-                                                   'pwdpolicysubentry',

-                                                   PW_POLICY_CONT_PEOPLE2)])

-    except ldap.LDAPError as e:

-        log.error('Failed to pwdpolicysubentry pw policy '

-                  'policy for {}: error {}'.format(OU_PEOPLE, e.message['desc']))

-        raise e

+    domain = Domain(topo.standalone, DEFAULT_SUFFIX)

+    domain.replace('pwdpolicysubentry', PW_POLICY_CONT_PEOPLE2)

+

     time.sleep(1)

@@ -116,12 +105,9 @@ def setup(topo, request):

     """

     log.info('Add custom schema...')

     try:

-        ATTR_1 = ("( 1.3.6.1.4.1.409.389.2.189 NAME 'x-department' " +

-                  "SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'user defined' )")

-        ATTR_2 = ("( 1.3.6.1.4.1.409.389.2.187 NAME 'x-en-ou' " +

-                  "SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'user defined' )")

-        OC = ("( xPerson-oid NAME 'xPerson' DESC '' SUP person STRUCTURAL MAY " +

-              "( x-department $ x-en-ou ) X-ORIGIN 'user defined' )")

+        ATTR_1 = (b"( 1.3.6.1.4.1.409.389.2.189 NAME 'x-department' SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'user defined' )")

+        ATTR_2 = (b"( 1.3.6.1.4.1.409.389.2.187 NAME 'x-en-ou' SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'user defined' )")

+        OC = (b"( xPerson-oid NAME 'xPerson' DESC '' SUP person STRUCTURAL MAY ( x-department $ x-en-ou ) X-ORIGIN 'user defined' )")

         topo.standalone.modify_s("cn=schema", [(ldap.MOD_ADD, 'attributeTypes', ATTR_1),

                                                (ldap.MOD_ADD, 'attributeTypes', ATTR_2),

                                                (ldap.MOD_ADD, 'objectClasses', OC)])

@@ -142,14 +128,9 @@ def setup(topo, request):

         'homeDirectory': '/home/test_user',

         'seeAlso': 'cn=cosTemplate,dc=example,dc=com'

     }

-    users.create(properties=user_properties)

-    try:

-        topo.standalone.modify_s(TEST_USER_DN, [(ldap.MOD_ADD,

-                                                 'objectclass',

-                                                 'xPerson')])

-    except ldap.LDAPError as e:

-        log.fatal('Failed to add objectclass to user')

-        raise e

+    user = users.create(properties=user_properties)

+

+    user.add('objectClass', 'xPerson')

     # Setup COS

     log.info("Setup indirect COS...")

diff --git a/ldap/servers/plugins/cos/cos_cache.c b/ldap/servers/plugins/cos/cos_cache.c

index 9ae15db15..662dace35 100644

--- a/ldap/servers/plugins/cos/cos_cache.c

+++ b/ldap/servers/plugins/cos/cos_cache.c

@@ -109,9 +109,6 @@ void *cos_get_plugin_identity(void);

 #define COSTYPE_INDIRECT 3

 #define COS_DEF_ERROR_NO_TEMPLATES -2

-/* the global plugin handle */

-static volatile vattr_sp_handle *vattr_handle = NULL;

-

 /* both variables are protected by change_lock */

 static int cos_cache_notify_flag = 0;

 static PRBool cos_cache_at_work = PR_FALSE;

@@ -323,16 +320,6 @@ cos_cache_init(void)

         views_api = 0;

     }

-    if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle,

-                                cos_cache_vattr_get,

-                                cos_cache_vattr_compare,

-                                cos_cache_vattr_types) != 0) {

-        slapi_log_err(SLAPI_LOG_ERR, COS_PLUGIN_SUBSYSTEM,

-                      "cos_cache_init - Cannot register as service provider\n");

-        ret = -1;

-        goto out;

-    }

-

     if (PR_CreateThread(PR_USER_THREAD,

                         cos_cache_wait_on_change,

                         NULL,

@@ -860,8 +847,23 @@ cos_dn_defs_cb(Slapi_Entry *e, void *callback_data)

                                           dnVals[valIndex]->bv_val);

                 }

-                slapi_vattrspi_regattr((vattr_sp_handle *)vattr_handle,

-                                       dnVals[valIndex]->bv_val, NULL, NULL);

+                /*

+                 * Each SP_handle is associated to one and only one vattr.

+                 * We could consider making this a single function rather

+                 * than the double-call.

+                 */

+

+                vattr_sp_handle *vattr_handle = NULL;

+

+                if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle,

+                                            cos_cache_vattr_get,

+                                            cos_cache_vattr_compare,

+                                            cos_cache_vattr_types) != 0) {

+                    slapi_log_err(SLAPI_LOG_ERR, COS_PLUGIN_SUBSYSTEM, "cos_cache_init - Cannot register as service provider for %s\n", dnVals[valIndex]->bv_val);

+                } else {

+                    slapi_vattrspi_regattr((vattr_sp_handle *)vattr_handle, dnVals[valIndex]->bv_val, NULL, NULL);

+                }

+

             } /* if(attrType is cosAttribute) */

             /*

diff --git a/ldap/servers/plugins/roles/roles_cache.c b/ldap/servers/plugins/roles/roles_cache.c

index 59f5a6081..1e5865af8 100644

--- a/ldap/servers/plugins/roles/roles_cache.c

+++ b/ldap/servers/plugins/roles/roles_cache.c

@@ -47,9 +47,6 @@ static char *allUserAttributes[] = {

 /* views scoping */

 static void **views_api;

-/* Service provider handler */

-static vattr_sp_handle *vattr_handle = NULL;

-

 /* List of nested roles */

 typedef struct _role_object_nested

 {

@@ -224,6 +221,10 @@ roles_cache_init()

       so that we update the corresponding cache */

     slapi_register_backend_state_change(NULL, roles_cache_trigger_update_suffix);

+    /* Service provider handler - only used once! and freed by vattr! */

+    vattr_sp_handle *vattr_handle = NULL;

+

+

     if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle,

                                 roles_sp_get_value,

                                 roles_sp_compare_value,

@@ -622,7 +623,6 @@ roles_cache_stop()

         current_role = next_role;

     }

     slapi_rwlock_unlock(global_lock);

-    slapi_ch_free((void **)&vattr_handle);

     roles_list = NULL;

     slapi_log_err(SLAPI_LOG_PLUGIN, ROLES_PLUGIN_SUBSYSTEM, "<-- roles_cache_stop\n");

diff --git a/ldap/servers/slapd/vattr.c b/ldap/servers/slapd/vattr.c

index 82deb41fe..432946c79 100644

--- a/ldap/servers/slapd/vattr.c

+++ b/ldap/servers/slapd/vattr.c

@@ -1864,7 +1864,12 @@ vattr_map_create(void)

 void

 vattr_map_entry_free(vattr_map_entry *vae)

 {

-    slapi_ch_free((void **)&(vae->sp_list));

+    vattr_sp_handle *list_entry = vae->sp_list;

+    while (list_entry != NULL) {

+        vattr_sp_handle *next_entry = list_entry->next;

+        slapi_ch_free((void **)&list_entry);

+        list_entry = next_entry;

+    }

     slapi_ch_free_string(&(vae->type_name));

     slapi_ch_free((void **)&vae;;

 }

@@ -2143,16 +2148,9 @@ slapi_vattr_schema_check_type(Slapi_Entry *e, char *type)

 vattr_map_entry *

 vattr_map_entry_new(char *type_name, vattr_sp_handle *sph, void *hint)

 {

-    vattr_map_entry *result = NULL;

-    vattr_sp_handle *sp_copy = NULL;

-

-    sp_copy = (vattr_sp_handle *)slapi_ch_calloc(1, sizeof(vattr_sp_handle));

-    sp_copy->sp = sph->sp;

-    sp_copy->hint = hint;

-

-    result = (vattr_map_entry *)slapi_ch_calloc(1, sizeof(vattr_map_entry));

+    vattr_map_entry *result = (vattr_map_entry *)slapi_ch_calloc(1, sizeof(vattr_map_entry));

     result->type_name = slapi_ch_strdup(type_name);

-    result->sp_list = sp_copy;

+    result->sp_list = sph;

     /* go get schema */

     result->objectclasses = vattr_map_entry_build_schema(type_name);

@@ -2273,6 +2271,16 @@ we'd need to hold a lock on the read path, which we don't want to do.

 So any SP which relinquishes its need to handle a type needs to continue

 to handle the calls on it, but return nothing */

 /* DBDB need to sort out memory ownership here, it's not quite right */

+/*

+ * This function was inconsistent. We would allocated and "kind of",

+ * copy the sp_handle here for the vattr_map_entry_new path. But we

+ * would "take ownership" for the existing entry and the list addition

+ * path. Instead now, EVERY sp_handle we take, we take ownership of

+ * and the CALLER must allocate a new one each time.

+ *

+ * Better idea, is that regattr should just take the fn pointers

+ * and callers never *see* the sp_handle structure at all.

+ */

 int

 vattr_map_sp_insert(char *type_to_add, vattr_sp_handle *sp, void *hint)

-- 

2.13.6