Original email from Andrej Gessel <andrej.gessel@janztec.com>: Hello, if I run "ldbsearch '(distinguishedName=abc)' I get following output: =============================================================== INTERNAL ERROR: Signal 11 in pid 5500 (4.8.0) Please read the Trouble-Shooting section of the Samba HOWTO =============================================================== smb_panic(): calling panic action [/bin/sleep 999999999] ^Csmb_panic(): action returned status 0 PANIC: internal error [1] 5500 abort (core dumped) ldbsearch distinguishedName=abc Samba version 4.8.0 and master. Attached are 2 Patches: 1) I tried to test it, but it doesn't really work. I think someone can write better test for it, as me. 2) Patch to fix this issue. Andrej
Created attachment 14113 [details] Patch for master (no test) Added just the patch to fix the issue. We probably need a proper test. Seems to have been introduced in 4.8 (GUID indexing). DNs would have normally been parsed by this point in previous versions. We probably need to also check similar code paths that might also trigger failures in this way.
I've been talking with Jermey, and this needs a CVE. Can someone get one for us?
Yeah, looks like an authenticated user DOS on the LDAP server, so CVE-worthy I'm afraid.
Ping on getting a CVE number for this?
Created attachment 14206 [details] patch also sent with the original report
Valgrind output: ==25090== Invalid read of size 1 ==25090== at 0x4C2E0E2: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==25090== by 0x87F5F34: ltdb_index_dn_attr (ldb_index.c:1394) ==25090== by 0x87F6243: ltdb_index_dn_base_dn (ldb_index.c:1470) ==25090== by 0x87F53FF: ltdb_index_dn_leaf (ldb_index.c:980) ==25090== by 0x87F630A: ltdb_index_dn (ldb_index.c:1499) ==25090== by 0x87F5CE8: ltdb_index_dn_and (ldb_index.c:1303) ==25090== by 0x87F62B3: ltdb_index_dn (ldb_index.c:1487) ==25090== by 0x87F6A1F: ltdb_search_indexed (ldb_index.c:1756) ==25090== by 0x87F3740: ltdb_search (ldb_search.c:808) ==25090== by 0x87F192C: ltdb_callback (ldb_tdb.c:1919) ==25090== by 0x40A7C33: tevent_common_loop_timer_delay (tevent_timed.c:369) ==25090== by 0x40A9F3D: epoll_event_loop_once (tevent_epoll.c:915) ==25090== Address 0x0 is not stack'd, malloc'd or (recently) free'd ==25090== =============================================================== INTERNAL ERROR: Signal 11 in pid 25090 (4.9.0pre1-DEVELOPERBUILD) Please read the Trouble-Shooting section of the Samba HOWTO ===============================================================
Can we please (again) get a CVE for this issue allocated please.
Created attachment 14207 [details] patch for master Here is a more comprehensive patch for master in addition to that provided by the reporter.
Created attachment 14208 [details] backported patch for 4.8
(In reply to Andrew Bartlett from comment #8) I will get a CVE for you, please give me some time.
(In reply to Huzaifa Sidhpurwala from comment #11) Please use CVE-2018-1140
*** Bug 13466 has been marked as a duplicate of this bug. ***
Note that #13466 makes this a DOS that does not require authentication.
Could someone provide an advisory and review the patch, please? I would like to address this asap as a patch is available. Thanks!
Created attachment 14267 [details] patch for master I've updated the patch to include Kai's test and done some work on the DNS server to validate the inputs more strongly.
Created attachment 14270 [details] Inital CVE text I'm also working on further robustness patches for the DNS server, given the level of untrusted input here.
Created attachment 14271 [details] patch for master
Created attachment 14272 [details] backported patch for 4.8
Created attachment 14273 [details] patch for 4.7 (security for SCOPE_ONELEVEL searchs on LDAP, hardening for DNS)
The remaining question is if ldb_dn_valdiate() should also casefold the DN. This might avoid similar issues elsewhere, as it is largely assumed that such a DN is very well formed.
Can I please have a peer review of this approach, as a one-line fix has become a monolith here. In particular, should some of this be split back out into a 'make things safer later' patch set, or is this all required in the security release? Should we have a different bug and/or CVE for the ldb_dn_add_child_fmt() use? For that issue in the DNS server (and marked against that bug), the patches try to avoid DNS lookup of eg foo,dc=bar.samba.example.com I can't see any real harm in those, as such an object shouldn't be able to exist, but they are of course invalid and should be prevented. Also, we need to check a corrupt base DN combined with a SCOPE_ONE search on an @IDXONE database. This is where the bug came in, and may be present in older versions.
Created attachment 14274 [details] additional API test patch for master
Created attachment 14275 [details] Updated CVE text I've decided to assume that there is no check on SCOPE_ONELEVEL and so we have the same issue just in a different guise there. I can't prove it yet but I can't prove otherwise yet either.
Created attachment 14276 [details] reviewed version of patch 14271 I reviewed the main patch and squashed in the following minor changes: 1. Fix a nearby non-check: --- a/source4/dns_server/dnsserver_common.c +++ b/source4/dns_server/dnsserver_common.c @@ -959,7 +959,11 @@ WERROR dns_common_name2dn(struct ldb_context *samdb, if (host_part_len == 0) { dn = ldb_dn_copy(mem_ctx, z->dn); - ldb_dn_add_child_fmt(dn, "DC=@"); + ok = ldb_dn_add_child_fmt(dn, "DC=@"); + if (! ok) { + TALLOC_FREE(dn); + return WERR_NOT_ENOUGH_MEMORY; + } *_dn = dn; return WERR_OK; } 2. ldb_dn_add_child_val() can do its work without the talloc/format overhead, and also in there because the value of ret is set once and always known, I prefer explicit return values. --- a/lib/ldb/common/ldb_dn.c +++ b/lib/ldb/common/ldb_dn.c @@ -1616,12 +1616,14 @@ bool ldb_dn_add_child_val(struct ldb_dn *dn, { bool ret; int ldb_ret; + struct ldb_dn *child = NULL; if ( !dn || dn->invalid) { return false; } - ret = ldb_dn_add_child_fmt(dn, "X=Y"); + child = ldb_dn_new(dn, dn->ldb, "X=Y"); + ret = ldb_dn_add_child(dn, child); if (ret == false) { - return ret; + return false; } ldb_ret = ldb_dn_set_component(dn, @@ -1637,7 +1637,7 @@ bool ldb_dn_add_child_val(struct ldb_dn *dn, return false; } - return ret; + return true; } 3. One that ISN'T just fiddling (function returns bool, errcode would reverse polarity): --- a/source4/dns_server/dlz_bind9.c +++ b/source4/dns_server/dlz_bind9.c @@ -1262,7 +1262,7 @@ static bool b9_has_soa(struct dlz_bind9_data *state, struct ldb_dn *dn, const ch "DC", zone_name_val)) { talloc_free(tmp_ctx); - return ISC_R_NOMEMORY; + return false; } werr = dns_common_lookup(state->samdb, tmp_ctx, dn, 4. This constant ldb_val can come out of the loop: --- a/source4/dns_server/dlz_bind9.c +++ b/source4/dns_server/dlz_bind9.c @@ -1052,11 +1052,10 @@ _PUBLIC_ isc_result_t dlz_allnodes(const char *zone, void *dbdata, struct ldb_dn *dn; struct ldb_result *res; TALLOC_CTX *tmp_ctx = talloc_new(state); + struct ldb_val zone_name_val = data_blob_string_const(zone); for (i=0; zone_prefixes[i]; i++) { const char *casefold; - struct ldb_val zone_name_val - = data_blob_string_const(zone); dn = ldb_dn_copy(tmp_ctx, ldb_get_default_basedn(state->samdb)); if (dn == NULL) { 5. Error string formatting: --- a/lib/ldb/ldb_tdb/ldb_index.c +++ b/lib/ldb/ldb_tdb/ldb_index.c @@ -1610,7 +1610,7 @@ static int ltdb_index_dn_attr(struct ldb_module *module, const char *dn_str = ldb_dn_get_linearized(dn); ldb_asprintf_errstring(ldb_module_get_ctx(module), __location__ - ": Failed to get casefold DN" + ": Failed to get casefold DN " "from: %s", dn_str); return LDB_ERR_OPERATIONS_ERROR; What I haven't entirely worked out yet: * are there other cases lurking somewhere? * and here around lib/ldb/ldb_tdb/ldb_search.c:790 + } else if (ldb_dn_validate(req->op.search.base) == false) { + + /* We don't want invalid base DNs here */ + ldb_asprintf_errstring(ldb, + "Invalid Base DN: %s", + ldb_dn_get_linearized(req->op.search.base)); + ret = LDB_ERR_INVALID_DN_SYNTAX; + } else { /* If we are not checking the base DN life is easy */ ret = LDB_SUCCESS; } do none of the other "else if" branches need the validation? Also it would be good to see more tests that pin this down (and I know Andrew is working on this).
Created attachment 14278 [details] Updated CVE text Pulling back to 4.8 only for the security release. DNS input validation issue separated (will be a non-security bug, hardening improvement only) and just the actual LDb segfault marked as CVE. LDAP one-level searches do not fault on older Samba versions due to protection in the acl_read module. Direct LDB one-level searches would still be an issue if UTF8 functions were loaded (only Samba is known to do so), but almost nobody uses LDB and we release the 1.4.1 LDB release anyway.
Created attachment 14279 [details] reduce the 4.8 patchset to a smaller security subset These are the patches we actually need (apart from the LDB release).
Created attachment 14282 [details] CVE-2018-1140 patch for master We believe this subset of the original patchset is sufficient for the security issue.
Created attachment 14283 [details] Additional hardening patches for master These patches apply after the security ones I think they need another ldb release
Created attachment 14289 [details] patch for master The hardening patches have been sent back to bug 13466. Samba 4.7 is no longer considered impacted
Created attachment 14290 [details] patch for Samba 4.8 Backported patches for 4.8 (only)
Created attachment 14295 [details] patch for master
Created attachment 14296 [details] patch for master
Created attachment 14297 [details] patch for Samba 4.8
Created attachment 14316 [details] CVE-2018-1140 patch for master This patch differs from previous one in Reviewed-bys only: 1/7: reviewed by Andrew 6/7: reviewed by me A similar change is coming up in the 4.8 patch
Created attachment 14317 [details] CVE-2018-1140 patch for 4.8
Thanks Douglas. I'm doing a private CI run and will approve if these pass, to make sure there are no unintended consequences of these fixes.
Comment on attachment 14316 [details] CVE-2018-1140 patch for master The master patch passed a full autobuild in the Catalyst Cloud.
I think the patches are ready. 4.8 also passed a full autobuild in the Catalyst Cloud.
Andrew, Why was this upgraded to critical?
The DoS on the DNS server takes down the full DC, and there is no auto restart. Otherwise, pay no attention, it just didn't seem like 'normal' was the right rating.
For the DNS attack this is a CVSS 7.5 https://www.first.org/cvss/calculator/3.0#CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H
Neither patch for master nor for 4.8 applies on current v4-9-stable branch.
Re-assigning to Andrew. Could someone please provide a patch for 4.9?, please?
Passing on to Gary who will get the updated patch. 4.9 looks easy (change the version number) master got lots of files renamed since this work was done.
Created attachment 14418 [details] CVE-2018-1140 patch for master (only)
Created attachment 14419 [details] CVE-2018-1140 patch for 4.9 (only) An autobuild is running on the Catalyst Cloud for the new master and 4.9 patches
Samba 4.8.4, 4.9.7 an 4.6.16 have been released in order to address these defects.
(In reply to Karolin Seeger from comment #48) This was 4.8 only...
(In reply to Karolin Seeger from comment #49) Pushed to autobuild-master and autobuild-v4-9-test.
Opening up bug. I can't see anything needing redaction here.
Pushed to all relevant branches. Closing out bug report. Thanks!