Attachment 17032 Details for Bug 14694 – patch from master backported to 4.15, 4.14 and 4.13

[patch] patch from master backported to 4.15, 4.14 and 4.13

CVE-2021-3670-ldap_spin-v4-15.patch (text/plain), 12.69 KB, created by Andrew Bartlett on 2021-11-30 06:39:48 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrew Bartlett

Created: 2021-11-30 06:39:48 UTC

Size: 12.69 KB

patch

obsolete

>From e5a5f7c0016b13b82a755baa29d7160fbdb554e4 Mon Sep 17 00:00:00 2001
>From: Joseph Sutton <josephsutton@catalyst.net.nz>
>Date: Thu, 26 Aug 2021 21:18:26 +1200
>Subject: [PATCH 1/4] CVE-2021-3670 tests/krb5/test_ldap.py: Add test for LDAP
> timeouts
>
>We allow a timeout of 2x over to avoid this being a flapping test.
>Samba is not very accurate on the timeout, which is not otherwise an
>issue but makes this test fail sometimes.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14694
>
>Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>(cherry picked from commit dcfcafdbf756e12d9077ad7920eea25478c29f81)
>---
> selftest/knownfail.d/ldap-timeout       |  1 +
> source4/dsdb/tests/python/large_ldap.py | 63 +++++++++++++++++++++++++
> 2 files changed, 64 insertions(+)
> create mode 100644 selftest/knownfail.d/ldap-timeout
>
>diff --git a/selftest/knownfail.d/ldap-timeout b/selftest/knownfail.d/ldap-timeout
>new file mode 100644
>index 00000000000..378ca1f4821
>--- /dev/null
>+++ b/selftest/knownfail.d/ldap-timeout
>@@ -0,0 +1 @@
>+samba4.ldap.large_ldap\..*\.python\(.*\).__main__.LargeLDAPTest.test_timeout\(.*\)
>\ No newline at end of file
>diff --git a/source4/dsdb/tests/python/large_ldap.py b/source4/dsdb/tests/python/large_ldap.py
>index 0bf73f988d8..f1fc13939e5 100644
>--- a/source4/dsdb/tests/python/large_ldap.py
>+++ b/source4/dsdb/tests/python/large_ldap.py
>@@ -23,6 +23,7 @@ import optparse
> import sys
> import os
> import random
>+import time
> 
> sys.path.insert(0, "bin/python")
> import samba
>@@ -244,6 +245,68 @@ class LargeLDAPTest(samba.tests.TestCase):
>         # Assert we don't get all the entries but still the error
>         self.assertGreater(count, count_jpeg)
> 
>+    def test_timeout(self):
>+        policy_dn = ldb.Dn(self.ldb,
>+                           'CN=Default Query Policy,CN=Query-Policies,'
>+                           'CN=Directory Service,CN=Windows NT,CN=Services,'
>+                           f'{self.ldb.get_config_basedn().get_linearized()}')
>+
>+        # Get the current value of lDAPAdminLimits.
>+        res = self.ldb.search(base=policy_dn,
>+                              scope=ldb.SCOPE_BASE,
>+                              attrs=['lDAPAdminLimits'])
>+        msg = res[0]
>+        admin_limits = msg['lDAPAdminLimits']
>+
>+        # Ensure we restore the previous value of the attribute.
>+        admin_limits.set_flags(ldb.FLAG_MOD_REPLACE)
>+        self.addCleanup(self.ldb.modify, msg)
>+
>+        # Temporarily lower the value of MaxQueryDuration so we can test
>+        # timeout behaviour.
>+        timeout = 5
>+        query_duration = f'MaxQueryDuration={timeout}'.encode()
>+
>+        admin_limits = [limit for limit in admin_limits
>+                        if not limit.lower().startswith(b'maxqueryduration=')]
>+        admin_limits.append(query_duration)
>+
>+        # Set the new attribute value.
>+        msg = ldb.Message(policy_dn)
>+        msg['lDAPAdminLimits'] = ldb.MessageElement(admin_limits,
>+                                                    ldb.FLAG_MOD_REPLACE,
>+                                                    'lDAPAdminLimits')
>+        self.ldb.modify(msg)
>+
>+        # Use a new connection so that the limits are reloaded.
>+        samdb = SamDB(url, credentials=creds,
>+                      session_info=system_session(lp),
>+                      lp=lp)
>+
>+        # Create a large search expression that will take a long time to
>+        # evaluate.
>+        expression = '(anr=l)' * 10000
>+        expression = f'(|{expression})'
>+
>+        # Perform the LDAP search.
>+        prev = time.time()
>+        with self.assertRaises(ldb.LdbError) as err:
>+            samdb.search(base=self.ou_dn,
>+                         scope=ldb.SCOPE_SUBTREE,
>+                         expression=expression,
>+                         attrs=['objectGUID'])
>+        now = time.time()
>+        duration = now - prev
>+
>+        # Ensure that we timed out.
>+        enum, _ = err.exception.args
>+        self.assertEqual(ldb.ERR_TIME_LIMIT_EXCEEDED, enum)
>+
>+        # Ensure that the time spent searching is within the limit we
>+        # set.  We allow a margin of 100% over as the Samba timeout
>+        # handling is not very accurate (and does not need to be)
>+        self.assertLess(timeout - 1, duration)
>+        self.assertLess(duration, timeout * 2)
> 
> 
> if "://" not in url:
>-- 
>2.25.1
>
>
>From 66b0147287d0ee34b13e529b27e81fb5d7ccddbc Mon Sep 17 00:00:00 2001
>From: Joseph Sutton <josephsutton@catalyst.net.nz>
>Date: Thu, 26 Aug 2021 13:53:23 +1200
>Subject: [PATCH 2/4] CVE-2021-3670 ldap_server: Set timeout on requests based
> on MaxQueryDuration
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14694
>
>Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>(cherry picked from commit 86fe9d48883f87c928bf31ccbd275db420386803)
>---
> source4/ldap_server/ldap_backend.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
>diff --git a/source4/ldap_server/ldap_backend.c b/source4/ldap_server/ldap_backend.c
>index e19f82cf5df..c7405f66643 100644
>--- a/source4/ldap_server/ldap_backend.c
>+++ b/source4/ldap_server/ldap_backend.c
>@@ -869,7 +869,17 @@ static NTSTATUS ldapsrv_SearchRequest(struct ldapsrv_call *call)
> 		}
> 	}
> 
>-	ldb_set_timeout(samdb, lreq, req->timelimit);
>+	{
>+		time_t timeout = call->conn->limits.search_timeout;
>+
>+		if (timeout == 0
>+		    || (req->timelimit != 0
>+			&& req->timelimit < timeout))
>+		{
>+			timeout = req->timelimit;
>+		}
>+		ldb_set_timeout(samdb, lreq, timeout);
>+	}
> 
> 	if (!call->conn->is_privileged) {
> 		ldb_req_mark_untrusted(lreq);
>-- 
>2.25.1
>
>
>From 3ea0d2a26d719b8b94776a1e713f9097a8970431 Mon Sep 17 00:00:00 2001
>From: Joseph Sutton <josephsutton@catalyst.net.nz>
>Date: Tue, 28 Sep 2021 17:20:43 +1300
>Subject: [PATCH 3/4] CVE-2021-3670 ldap_server: Ensure value of
> MaxQueryDuration is greater than zero
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14694
>
>Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>(cherry picked from commit e1ab0c43629686d1d2c0b0b2bcdc90057a792049)
>---
> source4/ldap_server/ldap_server.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/source4/ldap_server/ldap_server.c b/source4/ldap_server/ldap_server.c
>index ce4fd4f41d6..fbea5859756 100644
>--- a/source4/ldap_server/ldap_server.c
>+++ b/source4/ldap_server/ldap_server.c
>@@ -255,7 +255,9 @@ static int ldapsrv_load_limits(struct ldapsrv_connection *conn)
> 			continue;
> 		}
> 		if (strcasecmp("MaxQueryDuration", policy_name) == 0) {
>-			conn->limits.search_timeout = policy_value;
>+			if (policy_value > 0) {
>+				conn->limits.search_timeout = policy_value;
>+			}
> 			continue;
> 		}
> 	}
>-- 
>2.25.1
>
>
>From cf00a68a3d6ac43a5bf682f15b824ee362f4f75f Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Mon, 27 Sep 2021 16:47:46 +1300
>Subject: [PATCH 4/4] CVE-2021-3670 ldb: Confirm the request has not yet timed
> out in ldb filter processing
>
>The LDB filter processing is where the time is spent in the LDB stack
>but the timeout event will not get run while this is ongoing, so we
>must confirm we have not yet timed out manually.
>
>RN: Ensure that the LDB request has not timed out during filter processing
>as the LDAP server MaxQueryDuration is otherwise not honoured.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14694
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>(cherry picked from commit 1d5b155619bc532c46932965b215bd73a920e56f)
>---
> lib/ldb/ldb_key_value/ldb_kv.c        |  2 ++
> lib/ldb/ldb_key_value/ldb_kv.h        | 10 +++++++
> lib/ldb/ldb_key_value/ldb_kv_index.c  | 41 +++++++++++++++++++++++++++
> lib/ldb/ldb_key_value/ldb_kv_search.c | 33 ++++++++++++++++++++-
> selftest/knownfail.d/ldap-timeout     |  1 -
> 5 files changed, 85 insertions(+), 2 deletions(-)
> delete mode 100644 selftest/knownfail.d/ldap-timeout
>
>diff --git a/lib/ldb/ldb_key_value/ldb_kv.c b/lib/ldb/ldb_key_value/ldb_kv.c
>index ed0f760b5a2..aea6f0c1be0 100644
>--- a/lib/ldb/ldb_key_value/ldb_kv.c
>+++ b/lib/ldb/ldb_key_value/ldb_kv.c
>@@ -2078,6 +2078,8 @@ static int ldb_kv_handle_request(struct ldb_module *module,
> 		}
> 	}
> 
>+	ac->timeout_timeval = tv;
>+
> 	/* set a spy so that we do not try to use the request context
> 	 * if it is freed before ltdb_callback fires */
> 	ac->spy = talloc(req, struct ldb_kv_req_spy);
>diff --git a/lib/ldb/ldb_key_value/ldb_kv.h b/lib/ldb/ldb_key_value/ldb_kv.h
>index f9dffae2dcf..ac474b04b4c 100644
>--- a/lib/ldb/ldb_key_value/ldb_kv.h
>+++ b/lib/ldb/ldb_key_value/ldb_kv.h
>@@ -152,6 +152,16 @@ struct ldb_kv_context {
> 	struct ldb_module *module;
> 	struct ldb_request *req;
> 
>+	/*
>+	 * Required as we might not get to the event loop before the
>+	 * timeout, so we need some old-style cooperative multitasking
>+	 * here.
>+	 */
>+	struct timeval timeout_timeval;
>+
>+	/* Used to throttle calls to gettimeofday() */
>+	size_t timeout_counter;
>+
> 	bool request_terminated;
> 	struct ldb_kv_req_spy *spy;
> 
>diff --git a/lib/ldb/ldb_key_value/ldb_kv_index.c b/lib/ldb/ldb_key_value/ldb_kv_index.c
>index 1cc042aa84f..d70e5f619ef 100644
>--- a/lib/ldb/ldb_key_value/ldb_kv_index.c
>+++ b/lib/ldb/ldb_key_value/ldb_kv_index.c
>@@ -2352,6 +2352,47 @@ static int ldb_kv_index_filter(struct ldb_kv_private *ldb_kv,
> 	for (i = 0; i < num_keys; i++) {
> 		int ret;
> 		bool matched;
>+
>+		/*
>+		 * Check the time every 64 records, to reduce calls to
>+		 * gettimeofday().  This is a compromise, not all
>+		 * calls to ldb_match_message() will take the same
>+		 * time, most will run quickly but by luck it might be
>+		 * possible to have 64 records that are slow, doing a
>+		 * recursive search via LDAP_MATCHING_RULE_IN_CHAIN.
>+		 *
>+		 * Thankfully this is after index processing so only
>+		 * on the subset that matches some index (but still
>+		 * possibly a big one like objectclass=user)
>+		 */
>+		if (i % 64 == 0) {
>+			struct timeval now = tevent_timeval_current();
>+			int timeval_cmp = tevent_timeval_compare(&ac->timeout_timeval,
>+								 &now);
>+
>+			/*
>+			 * The search has taken too long.  This is the
>+			 * most likely place for our time to expire,
>+			 * as we are checking the records after the
>+			 * index set intersection.  This is now the
>+			 * slow process of checking if the records
>+			 * actually match.
>+			 *
>+			 * The tevent based timeout is not likely to
>+			 * be hit, sadly, as we don't run an event
>+			 * loop.
>+			 *
>+			 * While we are indexed and most of the work
>+			 * should have been done already, the
>+			 * ldb_match_* calls can be quite expensive if
>+			 * the caller uses LDAP_MATCHING_RULE_IN_CHAIN
>+			 */
>+			if (timeval_cmp <= 0) {
>+				talloc_free(keys);
>+				return LDB_ERR_TIME_LIMIT_EXCEEDED;
>+			}
>+		}
>+
> 		msg = ldb_msg_new(ac);
> 		if (!msg) {
> 			talloc_free(keys);
>diff --git a/lib/ldb/ldb_key_value/ldb_kv_search.c b/lib/ldb/ldb_key_value/ldb_kv_search.c
>index a0e1762bc90..46031b99c16 100644
>--- a/lib/ldb/ldb_key_value/ldb_kv_search.c
>+++ b/lib/ldb/ldb_key_value/ldb_kv_search.c
>@@ -314,7 +314,8 @@ static int search_func(_UNUSED_ struct ldb_kv_private *ldb_kv,
> 	struct ldb_context *ldb;
> 	struct ldb_kv_context *ac;
> 	struct ldb_message *msg, *filtered_msg;
>-	int ret;
>+	struct timeval now;
>+	int ret, timeval_cmp;
> 	bool matched;
> 
> 	ac = talloc_get_type(state, struct ldb_kv_context);
>@@ -341,6 +342,36 @@ static int search_func(_UNUSED_ struct ldb_kv_private *ldb_kv,
> 		return 0;
> 	}
> 
>+	/*
>+	 * Check the time every 64 records, to reduce calls to
>+	 * gettimeofday().  This is a compromise, not all calls to
>+	 * ldb_match_message() will take the same time, most will fail
>+	 * quickly but by luck it might be possible to have 64 records
>+	 * that are slow, doing a recursive search via
>+	 * LDAP_MATCHING_RULE_IN_CHAIN.
>+	 */
>+	if (ac->timeout_counter++ % 64 == 0) {
>+		now = tevent_timeval_current();
>+		timeval_cmp = tevent_timeval_compare(&ac->timeout_timeval,
>+						     &now);
>+
>+		/*
>+		 * The search has taken too long.  This is the most
>+		 * likely place for our time to expire, as we are in
>+		 * an un-indexed search and we return the data from
>+		 * within this loop.  The tevent based timeout is not
>+		 * likely to be hit, sadly.
>+		 *
>+		 * ldb_match_msg_error() can be quite expensive if a
>+		 * LDAP_MATCHING_RULE_IN_CHAIN extended match was
>+		 * specified.
>+		 */
>+		if (timeval_cmp <= 0) {
>+			ac->error = LDB_ERR_TIME_LIMIT_EXCEEDED;
>+			return -1;
>+		}
>+	}
>+
> 	msg = ldb_msg_new(ac);
> 	if (!msg) {
> 		ac->error = LDB_ERR_OPERATIONS_ERROR;
>diff --git a/selftest/knownfail.d/ldap-timeout b/selftest/knownfail.d/ldap-timeout
>deleted file mode 100644
>index 378ca1f4821..00000000000
>--- a/selftest/knownfail.d/ldap-timeout
>+++ /dev/null
>@@ -1 +0,0 @@
>-samba4.ldap.large_ldap\..*\.python\(.*\).__main__.LargeLDAPTest.test_timeout\(.*\)
>\ No newline at end of file
>-- 
>2.25.1
>

Flags:

dbagnall: review+

Actions: View

Attachments on bug 14694: 17032 | 17034 | 17038 | 17430