Bug 13374 - (CVE-2018-1140) [SECURITY] CVE-2018-1140 ldbsearch '(distinguishedName=abc)' and DNS query with escapes crashes
(CVE-2018-1140)
[SECURITY] CVE-2018-1140 ldbsearch '(distinguishedName=abc)' and DNS query wi...
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB
4.8.0
All All
: P5 critical
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks: 13466 13509
  Show dependency treegraph
 
Reported: 2018-04-08 22:07 UTC by Garming Sam
Modified: 2018-08-24 09:28 UTC (History)
18 users (show)

See Also:


Attachments
Patch for master (no test) (1.46 KB, patch)
2018-04-08 22:12 UTC, Garming Sam
no flags Details
patch also sent with the original report (1.35 KB, patch)
2018-05-20 23:24 UTC, Andrew Bartlett
no flags Details
patch for master (16.21 KB, patch)
2018-05-21 03:47 UTC, Andrew Bartlett
no flags Details
backported patch for 4.8 (16.19 KB, patch)
2018-05-21 04:00 UTC, Andrew Bartlett
no flags Details
patch for master (25.03 KB, patch)
2018-07-02 05:09 UTC, Andrew Bartlett
no flags Details
Inital CVE text (1.87 KB, text/plain)
2018-07-03 02:52 UTC, Andrew Bartlett
no flags Details
patch for master (66.19 KB, patch)
2018-07-03 05:45 UTC, Andrew Bartlett
no flags Details
backported patch for 4.8 (66.32 KB, patch)
2018-07-03 05:46 UTC, Andrew Bartlett
no flags Details
patch for 4.7 (security for SCOPE_ONELEVEL searchs on LDAP, hardening for DNS) (57.24 KB, patch)
2018-07-03 05:47 UTC, Andrew Bartlett
no flags Details
additional API test patch for master (3.73 KB, patch)
2018-07-04 02:23 UTC, Andrew Bartlett
no flags Details
Updated CVE text (1.98 KB, text/plain)
2018-07-04 02:46 UTC, Andrew Bartlett
dbagnall: review+
Details
reviewed version of patch 14271 (66.42 KB, patch)
2018-07-04 04:43 UTC, Douglas Bagnall
dbagnall: review+
Details
Updated CVE text (1.88 KB, text/plain)
2018-07-05 04:38 UTC, Andrew Bartlett
dbagnall: review+
Details
reduce the 4.8 patchset to a smaller security subset (20.69 KB, patch)
2018-07-05 04:45 UTC, Douglas Bagnall
no flags Details
CVE-2018-1140 patch for master (20.69 KB, patch)
2018-07-05 23:16 UTC, Douglas Bagnall
no flags Details
Additional hardening patches for master (50.48 KB, patch)
2018-07-06 01:16 UTC, Douglas Bagnall
no flags Details
patch for master (44.44 KB, patch)
2018-07-06 03:39 UTC, Andrew Bartlett
no flags Details
patch for Samba 4.8 (44.37 KB, patch)
2018-07-06 03:43 UTC, Andrew Bartlett
no flags Details
patch for master (45.38 KB, patch)
2018-07-06 04:24 UTC, Andrew Bartlett
no flags Details
patch for master (43.86 KB, patch)
2018-07-06 04:59 UTC, Andrew Bartlett
no flags Details
patch for Samba 4.8 (43.79 KB, patch)
2018-07-06 04:59 UTC, Andrew Bartlett
no flags Details
CVE-2018-1140 patch for master (43.97 KB, patch)
2018-07-10 03:02 UTC, Douglas Bagnall
abartlet: review+
Details
CVE-2018-1140 patch for 4.8 (43.90 KB, patch)
2018-07-10 03:03 UTC, Douglas Bagnall
abartlet: review+
Details
CVE-2018-1140 patch for master (only) (44.14 KB, patch)
2018-08-14 02:41 UTC, Andrew Bartlett
gary: review+
Details
CVE-2018-1140 patch for 4.9 (only) (43.97 KB, patch)
2018-08-14 02:43 UTC, Andrew Bartlett
gary: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Garming Sam 2018-04-08 22:07:08 UTC

    
Comment 1 Garming Sam 2018-04-08 22:08:08 UTC
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
Comment 2 Garming Sam 2018-04-08 22:12:31 UTC
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.
Comment 3 Andrew Bartlett 2018-05-01 21:44:11 UTC
I've been talking with Jermey, and this needs a CVE.  Can someone get one for us?
Comment 4 Jeremy Allison 2018-05-01 22:03:16 UTC
Yeah, looks like an authenticated user DOS on the LDAP server, so CVE-worthy I'm afraid.
Comment 5 Garming Sam 2018-05-14 03:52:23 UTC
Ping on getting a CVE number for this?
Comment 6 Andrew Bartlett 2018-05-20 23:24:57 UTC
Created attachment 14206 [details]
patch also sent with the original report
Comment 7 Garming Sam 2018-05-21 00:08:55 UTC
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
===============================================================
Comment 8 Andrew Bartlett 2018-05-21 02:59:05 UTC
Can we please (again) get a CVE for this issue allocated please.
Comment 9 Andrew Bartlett 2018-05-21 03:47:15 UTC
Created attachment 14207 [details]
patch for master

Here is a more comprehensive patch for master in addition to that provided by the reporter.
Comment 10 Andrew Bartlett 2018-05-21 04:00:58 UTC
Created attachment 14208 [details]
backported patch for 4.8
Comment 11 Huzaifa Sidhpurwala 2018-05-21 04:04:55 UTC
(In reply to Andrew Bartlett from comment #8)

I will get a CVE for you, please give me some time.
Comment 12 Huzaifa Sidhpurwala 2018-05-21 09:05:24 UTC
(In reply to Huzaifa Sidhpurwala from comment #11)

Please use CVE-2018-1140
Comment 13 Andrew Bartlett 2018-06-25 17:35:10 UTC
*** Bug 13466 has been marked as a duplicate of this bug. ***
Comment 14 Kai Blin 2018-06-26 08:51:50 UTC
Note that #13466 makes this a DOS that does not require authentication.
Comment 15 Karolin Seeger 2018-06-28 07:18:32 UTC
Could someone provide an advisory and review the patch, please?
I would like to address this asap as a patch is available.

Thanks!
Comment 16 Andrew Bartlett 2018-07-02 05:09:32 UTC
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.
Comment 17 Andrew Bartlett 2018-07-03 02:52:27 UTC
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.
Comment 18 Andrew Bartlett 2018-07-03 05:45:58 UTC
Created attachment 14271 [details]
patch for master
Comment 19 Andrew Bartlett 2018-07-03 05:46:37 UTC
Created attachment 14272 [details]
backported patch for 4.8
Comment 20 Andrew Bartlett 2018-07-03 05:47:09 UTC
Created attachment 14273 [details]
patch for 4.7 (security for SCOPE_ONELEVEL searchs on LDAP, hardening for DNS)
Comment 21 Andrew Bartlett 2018-07-03 05:48:41 UTC
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.
Comment 22 Andrew Bartlett 2018-07-03 09:02:45 UTC
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.
Comment 23 Andrew Bartlett 2018-07-04 02:23:36 UTC
Created attachment 14274 [details]
additional API test patch for master
Comment 24 Andrew Bartlett 2018-07-04 02:46:50 UTC
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.
Comment 25 Douglas Bagnall 2018-07-04 04:43:55 UTC
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).
Comment 26 Andrew Bartlett 2018-07-05 04:38:58 UTC
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.
Comment 27 Douglas Bagnall 2018-07-05 04:45:31 UTC
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).
Comment 28 Douglas Bagnall 2018-07-05 23:16:29 UTC
Created attachment 14282 [details]
CVE-2018-1140 patch for master

We believe this subset of the original patchset is sufficient for the security issue.
Comment 29 Douglas Bagnall 2018-07-06 01:16:31 UTC
Created attachment 14283 [details]
Additional hardening patches for master

These patches apply after the security ones 

I think they need another ldb release
Comment 30 Andrew Bartlett 2018-07-06 03:39:24 UTC
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
Comment 31 Andrew Bartlett 2018-07-06 03:43:01 UTC
Created attachment 14290 [details]
patch for Samba 4.8

Backported patches for 4.8 (only)
Comment 32 Andrew Bartlett 2018-07-06 04:24:24 UTC
Created attachment 14295 [details]
patch for master
Comment 33 Andrew Bartlett 2018-07-06 04:59:00 UTC
Created attachment 14296 [details]
patch for master
Comment 34 Andrew Bartlett 2018-07-06 04:59:50 UTC
Created attachment 14297 [details]
patch for Samba 4.8
Comment 35 Douglas Bagnall 2018-07-10 03:02:38 UTC
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
Comment 36 Douglas Bagnall 2018-07-10 03:03:45 UTC
Created attachment 14317 [details]
CVE-2018-1140 patch for 4.8
Comment 37 Andrew Bartlett 2018-07-10 04:16:57 UTC
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 38 Andrew Bartlett 2018-07-10 08:31:59 UTC
Comment on attachment 14316 [details]
CVE-2018-1140 patch for master

The master patch passed a full autobuild in the Catalyst Cloud.
Comment 39 Andrew Bartlett 2018-07-10 09:38:05 UTC
I think the patches are ready.  4.8 also passed a full autobuild in the Catalyst Cloud.
Comment 40 Huzaifa Sidhpurwala 2018-07-30 08:00:43 UTC
Andrew,

Why was this upgraded to critical?
Comment 41 Andrew Bartlett 2018-07-30 08:24:30 UTC
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.
Comment 42 Andrew Bartlett 2018-08-07 02:13:52 UTC
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
Comment 43 Karolin Seeger 2018-08-10 09:05:47 UTC
Neither patch for master nor for 4.8 applies on current v4-9-stable branch.
Comment 44 Karolin Seeger 2018-08-13 09:57:42 UTC
Re-assigning to Andrew. Could someone please provide a patch for 4.9?, please?
Comment 45 Andrew Bartlett 2018-08-14 01:32:22 UTC
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.
Comment 46 Andrew Bartlett 2018-08-14 02:41:57 UTC
Created attachment 14418 [details]
CVE-2018-1140 patch for master (only)
Comment 47 Andrew Bartlett 2018-08-14 02:43:05 UTC
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
Comment 48 Karolin Seeger 2018-08-14 08:43:34 UTC
Samba 4.8.4, 4.9.7 an 4.6.16 have been released in order to address these defects.
Comment 49 Karolin Seeger 2018-08-14 08:53:26 UTC
(In reply to Karolin Seeger from comment #48)
This was 4.8 only...
Comment 50 Karolin Seeger 2018-08-14 10:05:40 UTC
(In reply to Karolin Seeger from comment #49)
Pushed to autobuild-master and autobuild-v4-9-test.
Comment 51 Andrew Bartlett 2018-08-14 22:22:23 UTC
Opening up bug.  I can't see anything needing redaction here.
Comment 52 Karolin Seeger 2018-08-24 09:28:16 UTC
Pushed to all relevant branches.
Closing out bug report.

Thanks!