258 lines
9.2 KiB
Diff
258 lines
9.2 KiB
Diff
From 44ed6c2263c2c969bec4229f99b37d8f2e09dde0 Mon Sep 17 00:00:00 2001
|
|
From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
|
|
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 <douglas.bagnall@catalyst.net.nz>
|
|
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
|
|
---
|
|
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 5fbc51a2cf77ebd7ca42cd7dda58d5fd0ec5127d Mon Sep 17 00:00:00 2001
|
|
From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
|
|
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 <douglas.bagnall@catalyst.net.nz>
|
|
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
|
|
---
|
|
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 83f94e3b913..047244287f5 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 90f08c437ce81f2a96ce0740a93aa00e94eb5f16 Mon Sep 17 00:00:00 2001
|
|
From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
|
|
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 <douglas.bagnall@catalyst.net.nz>
|
|
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
|
|
---
|
|
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 edc3343e827..33265da373a 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
|