From d000349089eb15b3476ec302f4279f118336290e Mon Sep 17 00:00:00 2001
From: Mark Reynolds <mreynolds@redhat.com>
Date: Thu, 16 Dec 2021 16:13:08 -0500
Subject: [PATCH 1/2] 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