From 1cdb49e70e35ad69e76be10f93233cdf504375df Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Thu, 16 Dec 2021 16:13:08 -0500 Subject: [PATCH 1/4] CVE-2021-4091 (BZ#2030367) double-free of the virtual attribute context in persistent search description: A search is processed by a worker using a private pblock. If the search is persistent, the worker spawn a thread and kind of duplicate its private pblock so that the spawn thread continue to process the persistent search. Then worker ends the initial search, reinit (free) its private pblock, and returns monitoring the wait_queue. When the persistent search completes, it frees the duplicated pblock. The problem is that private pblock and duplicated pblock are referring to a same structure (pb_vattr_context). That lead to a double free Fix: When cloning the pblock (slapi_pblock_clone) make sure to transfert the references inside the original (private) pblock to the target (cloned) one That includes pb_vattr_context pointer. Reviewed by: Mark Reynolds, James Chapman, Pierre Rogier (Thanks !) --- ldap/servers/slapd/connection.c | 8 +++++--- ldap/servers/slapd/pblock.c | 14 ++++++++++++-- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/ldap/servers/slapd/connection.c b/ldap/servers/slapd/connection.c index e0c1a52d2..fc7ed9c4a 100644 --- a/ldap/servers/slapd/connection.c +++ b/ldap/servers/slapd/connection.c @@ -1823,9 +1823,11 @@ connection_threadmain() pthread_mutex_unlock(&(conn->c_mutex)); } /* ps_add makes a shallow copy of the pb - so we - * can't free it or init it here - just set operation to NULL. - * ps_send_results will call connection_remove_operation_ext to free it - */ + * can't free it or init it here - just set operation to NULL. + * ps_send_results will call connection_remove_operation_ext to free it + * The connection_thread private pblock ('pb') has be cloned and should only + * be reinit (slapi_pblock_init) + */ slapi_pblock_set(pb, SLAPI_OPERATION, NULL); slapi_pblock_init(pb); } else { diff --git a/ldap/servers/slapd/pblock.c b/ldap/servers/slapd/pblock.c index a64986aeb..c78d1250f 100644 --- a/ldap/servers/slapd/pblock.c +++ b/ldap/servers/slapd/pblock.c @@ -292,6 +292,12 @@ _pblock_assert_pb_deprecated(Slapi_PBlock *pblock) } } +/* It clones the pblock + * the content of the source pblock is transfered + * to the target pblock (returned) + * The source pblock should not be used for any operation + * it needs to be reinit (slapi_pblock_init) + */ Slapi_PBlock * slapi_pblock_clone(Slapi_PBlock *pb) { @@ -312,28 +318,32 @@ slapi_pblock_clone(Slapi_PBlock *pb) if (pb->pb_task != NULL) { _pblock_assert_pb_task(new_pb); *(new_pb->pb_task) = *(pb->pb_task); + memset(pb->pb_task, 0, sizeof(slapi_pblock_task)); } if (pb->pb_mr != NULL) { _pblock_assert_pb_mr(new_pb); *(new_pb->pb_mr) = *(pb->pb_mr); + memset(pb->pb_mr, 0, sizeof(slapi_pblock_matching_rule)); } if (pb->pb_misc != NULL) { _pblock_assert_pb_misc(new_pb); *(new_pb->pb_misc) = *(pb->pb_misc); + memset(pb->pb_misc, 0, sizeof(slapi_pblock_misc)); } if (pb->pb_intop != NULL) { _pblock_assert_pb_intop(new_pb); *(new_pb->pb_intop) = *(pb->pb_intop); - /* set pwdpolicy to NULL so this clone allocates its own policy */ - new_pb->pb_intop->pwdpolicy = NULL; + memset(pb->pb_intop, 0, sizeof(slapi_pblock_intop)); } if (pb->pb_intplugin != NULL) { _pblock_assert_pb_intplugin(new_pb); *(new_pb->pb_intplugin) = *(pb->pb_intplugin); + memset(pb->pb_intplugin, 0,sizeof(slapi_pblock_intplugin)); } if (pb->pb_deprecated != NULL) { _pblock_assert_pb_deprecated(new_pb); *(new_pb->pb_deprecated) = *(pb->pb_deprecated); + memset(pb->pb_deprecated, 0, sizeof(slapi_pblock_deprecated)); } #ifdef PBLOCK_ANALYTICS new_pb->analytics = NULL; -- 2.31.1