| From dbb3e65f7e382adf5fa6a6afb3d8684aca3f201a 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 1/1] 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> |
| --- |
| common/ldb_dn.c | 1 + |
| 1 file changed, 1 insertion(+) |
| |
| diff --git a/common/ldb_dn.c b/common/ldb_dn.c |
| index 001fcad621f..cce5ad5b2ff 100644 |
| --- a/common/ldb_dn.c |
| +++ b/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; |
| -- |
| 2.25.1 |