From 205bce153c7db8258a8a28498cf54e7374dca588 Mon Sep 17 00:00:00 2001 From: Thierry Bordaz 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