From e5a5f7c0016b13b82a755baa29d7160fbdb554e4 Mon Sep 17 00:00:00 2001 From: Joseph Sutton 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 Reviewed-by: Douglas Bagnall (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 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 Reviewed-by: Douglas Bagnall (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 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 Reviewed-by: Douglas Bagnall (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 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 Reviewed-by: Douglas Bagnall (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