From e1d4db4a2db077f9f4bdcc794b4a43ccbbcaf002 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Feb 2021 17:05:14 +1300 Subject: [PATCH 1/3] CVE-2020-27840: pytests:segfault: add ldb.Dn validate test ldb.Dn.validate wraps ldb_dn_explode. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14595 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett --- python/samba/tests/segfault.py | 6 ++++++ selftest/knownfail.d/python-segfaults | 1 + 2 files changed, 7 insertions(+) diff --git a/python/samba/tests/segfault.py b/python/samba/tests/segfault.py index 07e2d46d56a..70bd5b180e3 100644 --- a/python/samba/tests/segfault.py +++ b/python/samba/tests/segfault.py @@ -174,3 +174,9 @@ class SegfaultTests(samba.tests.TestCase): def test_dcerpc_idl_inline_arrays(self): """Inline arrays were incorrectly handled.""" dnsserver.DNS_RPC_SERVER_INFO_DOTNET().pExtensions + + @segfault_detector + def test_ldb_dn_explode_crash(self): + for i in range(106, 550, 5): + dn = ldb.Dn(ldb.Ldb(), "a=b%s,c= " % (' ' * i)) + dn.validate() diff --git a/selftest/knownfail.d/python-segfaults b/selftest/knownfail.d/python-segfaults index 1be0566dcb1..524f7dd013b 100644 --- a/selftest/knownfail.d/python-segfaults +++ b/selftest/knownfail.d/python-segfaults @@ -1 +1,2 @@ samba.tests.segfault.samba.tests.segfault.SegfaultTests.test_net_replicate_init__3 +samba.tests.segfault.samba.tests.segfault.SegfaultTests.test_ldb_dn_explode_crash -- 2.25.1 From da1cb77df08a181efa38856a91fa2507c497246c Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 11 Dec 2020 16:32:25 +1300 Subject: [PATCH 2/3] CVE-2020-27840 ldb_dn: avoid head corruption in ldb_dn_explode A DN string with lots of trailing space can cause ldb_dn_explode() to put a zero byte in the wrong place in the heap. When a DN string has a value represented with trailing spaces, like this "CN=foo ,DC=bar" the whitespace is supposed to be ignored. We keep track of this in the `t` pointer, which is NULL when we are not walking through trailing spaces, and points to the first space when we are. We are walking with the `p` pointer, writing the value to `d`, and keeping the length in `l`. "CN=foo ,DC= " ==> "foo " ^ ^ ^ t p d --l--- The value is finished when we encounter a comma or the end of the string. If `t` is not NULL at that point, we assume there are trailing spaces and wind `d and `l` back by the correct amount. Then we switch to expecting an attribute name (e.g. "CN"), until we get to an "=", which puts us back into looking for a value. Unfortunately, we forget to immediately tell `t` that we'd finished the last value, we can end up like this: "CN=foo ,DC= " ==> "" ^ ^ ^ t p d l=0 where `p` is pointing to a new value that contains only spaces, while `t` is still referring to the old value. `p` notices the value ends, and we subtract `p - t` from `d`: "CN=foo ,DC= " ==> ? "" ^ ^ ^ t p d l ~= SIZE_MAX - 8 At that point `d` wants to terminate its string with a '\0', but instead it terminates someone else's byte. This does not crash if the number of trailing spaces is small, as `d` will point into a previous value (a copy of "foo" in this example). Corrupting that value will ultimately not matter, as we will soon try to allocate a buffer `l` long, which will be greater than the available memory and the whole operation will fail properly. However, with more spaces, `d` will point into memory before the beginning of the allocated buffer, with the exact offset depending on the length of the earlier attributes and the number of spaces. What about a longer DN with more attributes? For example, "CN=foo ,DC= ,DC=example,DC=com" -- since `d` has moved out of bounds, won't we continue to use it and write more DN values into mystery memory? Fortunately not, because the aforementioned allocation of `l` bytes must happen first, and `l` is now huge. The allocation happens in a talloc_memdup(), which is by default restricted to allocating 256MB. So this allows a person who controls a string parsed by ldb_dn_explode to corrupt heap memory by placing a single zero byte at a chosen offset before the allocated buffer. An LDAP bind request can send a string DN as a username. This DN is necessarily parsed before the password is checked, so an attacker does not need proper credentials. The attacker can easily cause a denial of service and we cannot rule out more subtle attacks. The immediate solution is to reset `t` to NULL when a comma is encountered, indicating that we are no longer looking at trailing whitespace. Found with the help of Honggfuzz. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14595 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett --- lib/ldb/common/ldb_dn.c | 1 + selftest/knownfail.d/python-segfaults | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldb/common/ldb_dn.c b/lib/ldb/common/ldb_dn.c index 001fcad621f..cce5ad5b2ff 100644 --- a/lib/ldb/common/ldb_dn.c +++ b/lib/ldb/common/ldb_dn.c @@ -570,6 +570,7 @@ static bool ldb_dn_explode(struct ldb_dn *dn) /* trim back */ d -= (p - t); l -= (p - t); + t = NULL; } in_attr = true; diff --git a/selftest/knownfail.d/python-segfaults b/selftest/knownfail.d/python-segfaults index 524f7dd013b..1be0566dcb1 100644 --- a/selftest/knownfail.d/python-segfaults +++ b/selftest/knownfail.d/python-segfaults @@ -1,2 +1 @@ samba.tests.segfault.samba.tests.segfault.SegfaultTests.test_net_replicate_init__3 -samba.tests.segfault.samba.tests.segfault.SegfaultTests.test_ldb_dn_explode_crash -- 2.25.1 From 082121c78cbd229e4e9022a4fccb2483699183cc Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Feb 2021 16:28:43 +1300 Subject: [PATCH 3/3] CVE-2020-27840: pytests: move Dn.validate test to ldb We had the test in the Samba Python segfault suite because a) the signal catching infrastructure was there, and b) the ldb tests lack Samba's knownfail mechanism, which allowed us to assert the failure. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14595 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett --- lib/ldb/tests/python/crash.py | 45 ++++++++++++++++++++++++++++++++++ lib/ldb/wscript | 1 + python/samba/tests/segfault.py | 6 ----- 3 files changed, 46 insertions(+), 6 deletions(-) create mode 100644 lib/ldb/tests/python/crash.py diff --git a/lib/ldb/tests/python/crash.py b/lib/ldb/tests/python/crash.py new file mode 100644 index 00000000000..32839814552 --- /dev/null +++ b/lib/ldb/tests/python/crash.py @@ -0,0 +1,45 @@ +#!/usr/bin/env python3 +# +# Tests for crashing functions + +import os +from unittest import TestCase +import os +import sys +import traceback + +import ldb + + +def segfault_detector(f): + def wrapper(*args, **kwargs): + pid = os.fork() + if pid == 0: + # child, crashing? + try: + f(*args, **kwargs) + except Exception as e: + traceback.print_exc() + sys.stderr.flush() + sys.stdout.flush() + os._exit(0) + + # parent, waiting + pid2, status = os.waitpid(pid, 0) + if os.WIFSIGNALED(status): + signal = os.WTERMSIG(status) + raise AssertionError("Failed with signal %d" % signal) + + return wrapper + + +class LdbDnCrashTests(TestCase): + @segfault_detector + def test_ldb_dn_explode_crash(self): + for i in range(106, 150): + dn = ldb.Dn(ldb.Ldb(), "a=b%s,c= " % (' ' * i)) + dn.validate() + +if __name__ == '__main__': + import unittest + unittest.TestProgram() diff --git a/lib/ldb/wscript b/lib/ldb/wscript index f374f64aeab..32a1a2e0ec0 100644 --- a/lib/ldb/wscript +++ b/lib/ldb/wscript @@ -614,6 +614,7 @@ def test(ctx): os.mkdir(tmp_dir) pyret = samba_utils.RUN_PYTHON_TESTS( ['tests/python/api.py', + 'tests/python/crash.py', 'tests/python/index.py', 'tests/python/repack.py'], extra_env={'SELFTEST_PREFIX': test_prefix}) diff --git a/python/samba/tests/segfault.py b/python/samba/tests/segfault.py index 70bd5b180e3..07e2d46d56a 100644 --- a/python/samba/tests/segfault.py +++ b/python/samba/tests/segfault.py @@ -174,9 +174,3 @@ class SegfaultTests(samba.tests.TestCase): def test_dcerpc_idl_inline_arrays(self): """Inline arrays were incorrectly handled.""" dnsserver.DNS_RPC_SERVER_INFO_DOTNET().pExtensions - - @segfault_detector - def test_ldb_dn_explode_crash(self): - for i in range(106, 550, 5): - dn = ldb.Dn(ldb.Ldb(), "a=b%s,c= " % (' ' * i)) - dn.validate() -- 2.25.1