Blob Blame Raw
From 205bce153c7db8258a8a28498cf54e7374dca588 Mon Sep 17 00:00:00 2001
From: Thierry Bordaz <tbordaz@redhat.com>
Date: Tue, 14 Apr 2015 16:24:44 +0200
Subject: [PATCH] CVE-2015-1854 389ds-base: access control bypass with modrdn

Bug Description:
	47553 fix checks the write right access only if the RDN is
	modified. This allows to rename entries even if the
	authenticated user is not allowed of that.

Fix Description:
	Roll back a wrong optimization that tested the write access
	only if RDN value was changed.

https://fedorahosted.org/389/ticket/47553

Reviewed by: ?

Platforms tested: F17 (upstream test)

Flag Day: no

Doc impact: no

(cherry picked from commit 44e5c0998bdf7dcb167e8472713ff393b776e4e3)

Conflicts:
	dirsrvtests/tickets/ticket47553_single_aci_test.py
---
 dirsrvtests/tickets/ticket47553_rdn_write_test.py  | 132 +++++++++++++++++++++
 dirsrvtests/tickets/ticket47553_single_aci_test.py |  52 ++++++--
 ldap/servers/slapd/back-ldbm/ldbm_modrdn.c         |  32 ++---
 3 files changed, 184 insertions(+), 32 deletions(-)
 create mode 100644 dirsrvtests/tickets/ticket47553_rdn_write_test.py

diff --git a/dirsrvtests/tickets/ticket47553_rdn_write_test.py b/dirsrvtests/tickets/ticket47553_rdn_write_test.py
new file mode 100644
index 0000000..f15d9b3
--- /dev/null
+++ b/dirsrvtests/tickets/ticket47553_rdn_write_test.py
@@ -0,0 +1,132 @@
+import os
+import sys
+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 ldap.controls.simple import GetEffectiveRightsControl
+
+logging.getLogger(__name__).setLevel(logging.DEBUG)
+log = logging.getLogger(__name__)
+
+installation1_prefix = None
+
+SRC_ENTRY_CN = "tuser"
+EXT_RDN = "01"
+DST_ENTRY_CN = SRC_ENTRY_CN + EXT_RDN
+
+SRC_ENTRY_DN = "cn=%s,%s" % (SRC_ENTRY_CN, SUFFIX)
+DST_ENTRY_DN = "cn=%s,%s" % (DST_ENTRY_CN, SUFFIX)
+
+class TopologyStandalone(object):
+    def __init__(self, standalone):
+        standalone.open()
+        self.standalone = standalone
+
+
+#@pytest.fixture(scope="module")
+def topology(request):
+    global installation1_prefix
+
+    # Creating standalone instance ...
+    standalone = DirSrv(verbose=False)
+    if installation1_prefix:
+        args_instance[SER_DEPLOYED_DIR] = installation1_prefix
+    args_instance[SER_HOST] = HOST_STANDALONE
+    args_instance[SER_PORT] = PORT_STANDALONE
+    args_instance[SER_SERVERID_PROP] = SERVERID_STANDALONE
+    args_instance[SER_CREATION_SUFFIX] = DEFAULT_SUFFIX
+    args_standalone = args_instance.copy()
+    standalone.allocate(args_standalone)
+    instance_standalone = standalone.exists()
+    if instance_standalone:
+        standalone.delete()
+    standalone.create()
+    standalone.open()
+
+    # Clear out the tmp dir
+    standalone.clearTmpDir(__file__)
+
+    return TopologyStandalone(standalone)
+
+def test_ticket47553_rdn_write_init(topology):
+    topology.standalone.log.info("\n\n######################### Add entry tuser ######################\n")
+    topology.standalone.add_s(Entry((SRC_ENTRY_DN, {
+                                                'objectclass': "top person".split(),
+                                                'sn': SRC_ENTRY_CN,
+                                                'cn': SRC_ENTRY_CN})))
+
+def test_ticket47553_rdn_write_get_ger(topology):
+    ANONYMOUS_DN = ""
+    topology.standalone.log.info("\n\n######################### GER rights for anonymous ######################\n")
+    request_ctrl = GetEffectiveRightsControl(criticality=True, authzId="dn:" + ANONYMOUS_DN)
+    msg_id = topology.standalone.search_ext(SUFFIX, ldap.SCOPE_SUBTREE, "objectclass=*", serverctrls=[request_ctrl])
+    rtype, rdata, rmsgid, response_ctrl = topology.standalone.result3(msg_id)
+    value = ''
+    for dn, attrs in rdata:
+        topology.standalone.log.info("dn: %s" % dn)
+        for value in attrs['entryLevelRights']:
+            topology.standalone.log.info("###############  entryLevelRights: %r" % value)
+            assert 'n' not in value
+
+def test_ticket47553_rdn_write_modrdn_anonymous(topology):
+    ANONYMOUS_DN = ""
+    topology.standalone.close()
+    topology.standalone.binddn = ANONYMOUS_DN
+    topology.standalone.open()
+    msg_id = topology.standalone.search_ext("", ldap.SCOPE_BASE, "objectclass=*")
+    rtype, rdata, rmsgid, response_ctrl = topology.standalone.result3(msg_id)
+    value = ''
+    for dn, attrs in rdata:
+        topology.standalone.log.info("dn: %s" % dn)
+        for attr in attrs:
+            topology.standalone.log.info("###############  %r: %r" % (attr, attrs[attr]))
+
+
+    try:
+        topology.standalone.rename_s(SRC_ENTRY_DN, "cn=%s" % DST_ENTRY_CN, delold=True)
+    except Exception as e:
+        topology.standalone.log.info("Exception (expected): %s" % type(e).__name__)
+        isinstance(e, ldap.INSUFFICIENT_ACCESS)
+
+    try:
+        topology.standalone.getEntry(DST_ENTRY_DN, ldap.SCOPE_BASE, "objectclass=*")
+        assert False
+    except Exception as e:
+        topology.standalone.log.info("The entry was not renamed (expected)")
+        isinstance(e, ldap.NO_SUCH_OBJECT)
+
+def test_ticket47553_rdn_write(topology):
+    '''
+    Write your testcase here...
+    '''
+
+    log.info('Test complete')
+
+
+def test_ticket47553_rdn_write_final(topology):
+    topology.standalone.delete()
+    log.info('Testcase PASSED')
+
+
+def run_isolated():
+    global installation1_prefix
+    installation1_prefix = '/home/tbordaz/install_master'
+
+    topo = topology(True)
+    test_ticket47553_rdn_write_init(topo)
+    test_ticket47553_rdn_write_get_ger(topo)
+    test_ticket47553_rdn_write(topo)
+    test_ticket47553_rdn_write_modrdn_anonymous(topo)
+    test_ticket47553_rdn_write_final(topo)
+
+
+if __name__ == '__main__':
+    run_isolated()
+
diff --git a/dirsrvtests/tickets/ticket47553_single_aci_test.py b/dirsrvtests/tickets/ticket47553_single_aci_test.py
index 4be2470..0c8d7e9 100644
--- a/dirsrvtests/tickets/ticket47553_single_aci_test.py
+++ b/dirsrvtests/tickets/ticket47553_single_aci_test.py
@@ -276,7 +276,27 @@ def _moddn_aci_deny_tree(topology, mod_type=None, target_from=STAGING_DN, target
     #topology.master1.modify_s(SUFFIX, mod)
     topology.master1.log.info("Add a DENY aci under %s " % PROD_EXCEPT_DN)
     topology.master1.modify_s(PROD_EXCEPT_DN, mod)
-    
+
+def _write_aci_staging(topology, mod_type=None):
+    assert mod_type is not None
+
+    ACI_TARGET = "(targetattr= \"cn\")(target=\"ldap:///cn=*,%s\")" % STAGING_DN
+    ACI_ALLOW        = "(version 3.0; acl \"write staging entries\"; allow (write)"
+    ACI_SUBJECT      = " userdn = \"ldap:///%s\";)" % BIND_DN
+    ACI_BODY         = ACI_TARGET + ACI_ALLOW + ACI_SUBJECT
+    mod = [(mod_type, 'aci', ACI_BODY)]
+    topology.master1.modify_s(SUFFIX, mod)
+
+def _write_aci_production(topology, mod_type=None):
+    assert mod_type is not None
+
+    ACI_TARGET = "(targetattr= \"cn\")(target=\"ldap:///cn=*,%s\")" % PRODUCTION_DN
+    ACI_ALLOW        = "(version 3.0; acl \"write production entries\"; allow (write)"
+    ACI_SUBJECT      = " userdn = \"ldap:///%s\";)" % BIND_DN
+    ACI_BODY         = ACI_TARGET + ACI_ALLOW + ACI_SUBJECT
+    mod = [(mod_type, 'aci', ACI_BODY)]
+    topology.master1.modify_s(SUFFIX, mod)
+
 def _moddn_aci_staging_to_production(topology, mod_type=None, target_from=STAGING_DN, target_to=PRODUCTION_DN):
     assert mod_type != None
 
@@ -293,6 +313,8 @@ def _moddn_aci_staging_to_production(topology, mod_type=None, target_from=STAGIN
     ACI_BODY         = ACI_TARGET_FROM + ACI_TARGET_TO + ACI_ALLOW + ACI_SUBJECT
     mod = [(mod_type, 'aci', ACI_BODY)]
     topology.master1.modify_s(SUFFIX, mod)
+    
+    _write_aci_staging(topology, mod_type=mod_type)
 
 def _moddn_aci_from_production_to_staging(topology, mod_type=None):
     assert mod_type != None
@@ -303,6 +325,8 @@ def _moddn_aci_from_production_to_staging(topology, mod_type=None):
     ACI_BODY         = ACI_TARGET + ACI_ALLOW + ACI_SUBJECT
     mod = [(mod_type, 'aci', ACI_BODY)]
     topology.master1.modify_s(SUFFIX, mod)
+    
+    _write_aci_production(topology, mod_type=mod_type)
 
 
 def test_ticket47553_init(topology):
@@ -347,12 +371,9 @@ def test_ticket47553_init(topology):
                                             'description': "production except DIT"})))
     
     # enable acl error logging
-    #mod = [(ldap.MOD_REPLACE, 'nsslapd-errorlog-level', '128')]
-    #topology.master1.modify_s(DN_CONFIG, mod)
-    #topology.master2.modify_s(DN_CONFIG, mod)
-    
-
-    
+    mod = [(ldap.MOD_REPLACE, 'nsslapd-errorlog-level', str(128+262144))]
+    topology.master1.modify_s(DN_CONFIG, mod)
+    topology.master2.modify_s(DN_CONFIG, mod)
     
     
     # add dummy entries in the staging DIT
@@ -883,6 +904,7 @@ def test_ticket47553_moddn_staging_prod_9(topology):
     _bind_manager(topology)
     mod = [(ldap.MOD_ADD, 'aci', ACI_BODY)]
     topology.master1.modify_s(PRODUCTION_DN, mod)
+    _write_aci_staging(topology, mod_type=ldap.MOD_ADD)
     _bind_normal(topology)
     
     topology.master1.log.info("Try to MODDN %s -> %s,%s" % (old_dn, new_rdn, new_superior))
@@ -891,6 +913,7 @@ def test_ticket47553_moddn_staging_prod_9(topology):
     _bind_manager(topology)
     mod = [(ldap.MOD_DELETE, 'aci', ACI_BODY)]
     topology.master1.modify_s(PRODUCTION_DN, mod)
+    _write_aci_staging(topology, mod_type=ldap.MOD_DELETE)
     _bind_normal(topology)
     
     
@@ -934,6 +957,7 @@ def test_ticket47553_moddn_staging_prod_9(topology):
     _bind_manager(topology)
     mod = [(ldap.MOD_ADD, 'aci', ACI_BODY)]
     topology.master1.modify_s(PRODUCTION_DN, mod)
+    _write_aci_staging(topology, mod_type=ldap.MOD_ADD)
     _bind_normal(topology)
     
     try:
@@ -949,6 +973,7 @@ def test_ticket47553_moddn_staging_prod_9(topology):
     _bind_manager(topology)
     mod = [(ldap.MOD_DELETE, 'aci', ACI_BODY)]
     topology.master1.modify_s(PRODUCTION_DN, mod)
+    _write_aci_staging(topology, mod_type=ldap.MOD_DELETE)
     _bind_normal(topology)
     
     # Add the moddn aci that will be evaluated because of the config flag
@@ -1009,7 +1034,12 @@ def test_ticket47553_moddn_prod_staging(topology):
     old_dn  = "%s,%s" % (old_rdn, PRODUCTION_DN)
     new_rdn = old_rdn
     new_superior = STAGING_DN
-    
+
+    # add the write right because we want to check the moddn
+    _bind_manager(topology)
+    _write_aci_production(topology, mod_type=ldap.MOD_ADD)
+    _bind_normal(topology)
+
     try:
         topology.master1.log.info("Try to move back MODDN %s -> %s,%s" % (old_dn, new_rdn, new_superior))
         topology.master1.rename_s(old_dn, new_rdn, newsuperior=new_superior)
@@ -1019,7 +1049,11 @@ def test_ticket47553_moddn_prod_staging(topology):
     except Exception as e:
         topology.master1.log.info("Exception (expected): %s" % type(e).__name__)
         assert isinstance(e, ldap.INSUFFICIENT_ACCESS)
-    
+
+    _bind_manager(topology)
+    _write_aci_production(topology, mod_type=ldap.MOD_DELETE)
+    _bind_normal(topology)
+
     # successfull MOD with the both ACI
     _bind_manager(topology)
     _moddn_aci_staging_to_production(topology, mod_type=ldap.MOD_DELETE, target_from=STAGING_DN, target_to=PRODUCTION_DN)
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c b/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c
index 6a4982c..4129318 100644
--- a/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c
+++ b/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c
@@ -677,31 +677,17 @@ ldbm_back_modrdn( Slapi_PBlock *pb )
             
             /* JCMACL - Should be performed before the child check. */
             /* JCMACL - Why is the check performed against the copy, rather than the existing entry? */
+            /* This check must be performed even if the entry is renamed with its own name 
+             * No optimization here we need to check we have the write access to the target entry
+             */
+            ldap_result_code = plugin_call_acl_plugin(pb, ec->ep_entry,
+                    NULL /*attr*/, NULL /*value*/, SLAPI_ACL_WRITE,
+                    ACLPLUGIN_ACCESS_MODRDN, &errbuf);
+            if (ldap_result_code != LDAP_SUCCESS)
             {
-                Slapi_RDN *new_rdn;
-                Slapi_RDN *old_rdn;
-
-                /* Taken from the entry */
-                old_rdn = slapi_entry_get_srdn(ec->ep_entry);
-
-                /* Taken from the request */
-                new_rdn = slapi_rdn_new();
-                slapi_sdn_get_rdn(&dn_newrdn, new_rdn);
-
-                /* Only if we change the RDN value, we need the write access to the entry */
-                if (slapi_rdn_compare(old_rdn, new_rdn)) {
-                        ldap_result_code = plugin_call_acl_plugin(pb, ec->ep_entry,
-                                NULL /*attr*/, NULL /*value*/, SLAPI_ACL_WRITE,
-                                ACLPLUGIN_ACCESS_MODRDN, &errbuf);
-                }
-
-                slapi_rdn_free(&new_rdn);
+                goto error_return;
+            }
 
-                if (ldap_result_code != LDAP_SUCCESS) {
-                        goto error_return;
-                }
-            } 
-        
             /* Set the new dn to the copy of the entry */
             slapi_entry_set_sdn( ec->ep_entry, &dn_newdn );
             if (entryrdn_get_switch()) { /* subtree-rename: on */
-- 
1.9.3