From 9501741466ba2c0740ffc703c5d242d6b41510e8 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet@samba.org>
Date: Tue, 29 Oct 2019 17:25:28 +1300
Subject: [PATCH 1/9] CVE-2019-14861: s4-rpc/dnsserver: Confirm sort behaviour
 in dcesrv_DnssrvEnumRecords

The sort behaviour for child records is not correct in Samba so
we add a flapping entry.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=14138

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
---
 python/samba/tests/dcerpc/dnsserver.py | 101 +++++++++++++++++++++++++
 selftest/flapping.d/dnsserver          |   2 +
 2 files changed, 103 insertions(+)
 create mode 100644 selftest/flapping.d/dnsserver

diff --git a/python/samba/tests/dcerpc/dnsserver.py b/python/samba/tests/dcerpc/dnsserver.py
index 7264a290ef2..14ce308e38f 100644
--- a/python/samba/tests/dcerpc/dnsserver.py
+++ b/python/samba/tests/dcerpc/dnsserver.py
@@ -156,6 +156,107 @@ class DnsserverTests(RpcInterfaceTestCase):
                                    None)
         super(DnsserverTests, self).tearDown()
 
+    def test_enum_is_sorted(self):
+        """
+        Confirm the zone is sorted
+        """
+
+        record_str = "192.168.50.50"
+        record_type_str = "A"
+        self.add_record(self.custom_zone, "atestrecord-1", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-2", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-3", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-4", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-0", record_type_str, record_str)
+
+        # This becomes an extra A on the zone itself by server-side magic
+        self.add_record(self.custom_zone, self.custom_zone, record_type_str, record_str)
+
+        _, result = self.conn.DnssrvEnumRecords2(dnsserver.DNS_CLIENT_VERSION_LONGHORN,
+                                                 0,
+                                                 self.server,
+                                                 self.custom_zone,
+                                                 "@",
+                                                 None,
+                                                 self.record_type_int(record_type_str),
+                                                 dnsserver.DNS_RPC_VIEW_AUTHORITY_DATA,
+                                                 None,
+                                                 None)
+
+        self.assertEqual(len(result.rec), 6)
+        self.assertEqual(result.rec[0].dnsNodeName.str, "")
+        self.assertEqual(result.rec[1].dnsNodeName.str, "atestrecord-0")
+        self.assertEqual(result.rec[2].dnsNodeName.str, "atestrecord-1")
+        self.assertEqual(result.rec[3].dnsNodeName.str, "atestrecord-2")
+        self.assertEqual(result.rec[4].dnsNodeName.str, "atestrecord-3")
+        self.assertEqual(result.rec[5].dnsNodeName.str, "atestrecord-4")
+
+    def test_enum_is_sorted_children_prefix_first(self):
+        """
+        Confirm the zone returns the selected prefix first but no more
+        as Samba is flappy for the full sort
+        """
+
+        record_str = "192.168.50.50"
+        record_type_str = "A"
+        self.add_record(self.custom_zone, "atestrecord-1.a.b", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-2.a.b", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-3.a.b", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-4.a.b", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-0.a.b", record_type_str, record_str)
+
+        # Not expected to be returned
+        self.add_record(self.custom_zone, "atestrecord-0.b.b", record_type_str, record_str)
+
+        _, result = self.conn.DnssrvEnumRecords2(dnsserver.DNS_CLIENT_VERSION_LONGHORN,
+                                                 0,
+                                                 self.server,
+                                                 self.custom_zone,
+                                                 "a.b",
+                                                 None,
+                                                 self.record_type_int(record_type_str),
+                                                 dnsserver.DNS_RPC_VIEW_AUTHORITY_DATA,
+                                                 None,
+                                                 None)
+
+        self.assertEqual(len(result.rec), 6)
+        self.assertEqual(result.rec[0].dnsNodeName.str, "")
+
+    def test_enum_is_sorted_children(self):
+        """
+        Confirm the zone is sorted
+        """
+
+        record_str = "192.168.50.50"
+        record_type_str = "A"
+        self.add_record(self.custom_zone, "atestrecord-1.a.b", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-2.a.b", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-3.a.b", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-4.a.b", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-0.a.b", record_type_str, record_str)
+
+        # Not expected to be returned
+        self.add_record(self.custom_zone, "atestrecord-0.b.b", record_type_str, record_str)
+
+        _, result = self.conn.DnssrvEnumRecords2(dnsserver.DNS_CLIENT_VERSION_LONGHORN,
+                                                 0,
+                                                 self.server,
+                                                 self.custom_zone,
+                                                 "a.b",
+                                                 None,
+                                                 self.record_type_int(record_type_str),
+                                                 dnsserver.DNS_RPC_VIEW_AUTHORITY_DATA,
+                                                 None,
+                                                 None)
+
+        self.assertEqual(len(result.rec), 6)
+        self.assertEqual(result.rec[0].dnsNodeName.str, "")
+        self.assertEqual(result.rec[1].dnsNodeName.str, "atestrecord-0")
+        self.assertEqual(result.rec[2].dnsNodeName.str, "atestrecord-1")
+        self.assertEqual(result.rec[3].dnsNodeName.str, "atestrecord-2")
+        self.assertEqual(result.rec[4].dnsNodeName.str, "atestrecord-3")
+        self.assertEqual(result.rec[5].dnsNodeName.str, "atestrecord-4")
+
     # This test fails against Samba (but passes against Windows),
     # because Samba does not return the record when we enum records.
     # Records can be given DNS_RANK_NONE when the zone they are in
diff --git a/selftest/flapping.d/dnsserver b/selftest/flapping.d/dnsserver
new file mode 100644
index 00000000000..9b33e8522a3
--- /dev/null
+++ b/selftest/flapping.d/dnsserver
@@ -0,0 +1,2 @@
+# This is not stable in samba due to a bug
+^samba.tests.dcerpc.dnsserver.samba.tests.dcerpc.dnsserver.DnsserverTests.test_enum_is_sorted_children
\ No newline at end of file
-- 
2.17.1


From 51fa9a6a805e4221120847ee9dcab6796021175a Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet@samba.org>
Date: Mon, 21 Oct 2019 12:12:10 +1300
Subject: [PATCH 2/9] CVE-2019-14861: s4-rpc_server: Remove special case for @
 in dns_build_tree()

BUG: https://bugzilla.samba.org/show_bug.cgi?id=14138

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
---
 source4/rpc_server/dnsserver/dnsdata.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/source4/rpc_server/dnsserver/dnsdata.c b/source4/rpc_server/dnsserver/dnsdata.c
index 59e29f029a6..f991f4042e3 100644
--- a/source4/rpc_server/dnsserver/dnsdata.c
+++ b/source4/rpc_server/dnsserver/dnsdata.c
@@ -795,10 +795,11 @@ struct dns_tree *dns_build_tree(TALLOC_CTX *mem_ctx, const char *name, struct ld
 	for (i=0; i<res->count; i++) {
 		ptr = ldb_msg_find_attr_as_string(res->msgs[i], "name", NULL);
 
-		if (strcmp(ptr, "@") == 0) {
-			base->data = res->msgs[i];
-			continue;
-		} else if (strcasecmp(ptr, name) == 0) {
+		/*
+		 * This might be the sub-domain in the zone being
+		 * requested, or @ for the root of the zone
+		 */
+		if (strcasecmp(ptr, name) == 0) {
 			base->data = res->msgs[i];
 			continue;
 		}
-- 
2.17.1


From 16405fecc403517574915a49de5f4abcaa964e21 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet@samba.org>
Date: Tue, 29 Oct 2019 14:15:36 +1300
Subject: [PATCH 3/9] CVE-2019-14861: s4-rpc/dnsserver: Avoid crash in
 ldb_qsort() via dcesrv_DnssrvEnumRecords)

dns_name_compare() had logic to put @ and the top record in the tree being
enumerated first, but if a domain had both then this would break the
older qsort() implementation in ldb_qsort() and cause a read of memory
before the base pointer.

By removing this special case (not required as the base pointer
is already seperatly located, no matter were it is in the
returned records) the crash is avoided.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=14138

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
---
 .../rpc_server/dnsserver/dcerpc_dnsserver.c   | 21 ++++++++++++-------
 source4/rpc_server/dnsserver/dnsdata.c        | 19 ++---------------
 source4/rpc_server/dnsserver/dnsserver.h      |  4 ++--
 3 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c
index 353754f9261..c0bf3425dae 100644
--- a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c
+++ b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c
@@ -1733,6 +1733,7 @@ static WERROR dnsserver_enumerate_records(struct dnsserver_state *dsstate,
 	struct DNS_RPC_RECORDS_ARRAY *recs;
 	char **add_names = NULL;
 	char *rname;
+	const char *preference_name = NULL;
 	int add_count = 0;
 	int i, ret, len;
 	WERROR status;
@@ -1749,6 +1750,7 @@ static WERROR dnsserver_enumerate_records(struct dnsserver_state *dsstate,
 		ret = ldb_search(dsstate->samdb, tmp_ctx, &res, z->zone_dn,
 				 LDB_SCOPE_ONELEVEL, attrs,
 				 "(&(objectClass=dnsNode)(!(dNSTombstoned=TRUE)))");
+		preference_name = "@";
 	} else {
 		char *encoded_name
 			= ldb_binary_encode_string(tmp_ctx, name);
@@ -1756,6 +1758,7 @@ static WERROR dnsserver_enumerate_records(struct dnsserver_state *dsstate,
 				 LDB_SCOPE_ONELEVEL, attrs,
 				 "(&(objectClass=dnsNode)(|(name=%s)(name=*.%s))(!(dNSTombstoned=TRUE)))",
 				 encoded_name, encoded_name);
+		preference_name = name;
 	}
 	if (ret != LDB_SUCCESS) {
 		talloc_free(tmp_ctx);
@@ -1769,16 +1772,18 @@ static WERROR dnsserver_enumerate_records(struct dnsserver_state *dsstate,
 	recs = talloc_zero(mem_ctx, struct DNS_RPC_RECORDS_ARRAY);
 	W_ERROR_HAVE_NO_MEMORY_AND_FREE(recs, tmp_ctx);
 
-	/* Sort the names, so that the first record is the parent record */
-	ldb_qsort(res->msgs, res->count, sizeof(struct ldb_message *), name,
-			(ldb_qsort_cmp_fn_t)dns_name_compare);
+	/*
+	 * Sort the names, so that the records are in order by the child
+	 * component below "name".
+	 *
+	 * A full tree sort is not required, so we pass in "name" so
+	 * we know which level to sort, as only direct children are
+	 * eventually returned
+	 */
+	LDB_TYPESAFE_QSORT(res->msgs, res->count, name, dns_name_compare);
 
 	/* Build a tree of name components from dns name */
-	if (strcasecmp(name, z->name) == 0) {
-		tree = dns_build_tree(tmp_ctx, "@", res);
-	} else {
-		tree = dns_build_tree(tmp_ctx, name, res);
-	}
+	tree = dns_build_tree(tmp_ctx, preference_name, res);
 	W_ERROR_HAVE_NO_MEMORY_AND_FREE(tree, tmp_ctx);
 
 	/* Find the parent record in the tree */
diff --git a/source4/rpc_server/dnsserver/dnsdata.c b/source4/rpc_server/dnsserver/dnsdata.c
index f991f4042e3..acdb87752f8 100644
--- a/source4/rpc_server/dnsserver/dnsdata.c
+++ b/source4/rpc_server/dnsserver/dnsdata.c
@@ -1052,8 +1052,8 @@ WERROR dns_fill_records_array(TALLOC_CTX *mem_ctx,
 }
 
 
-int dns_name_compare(const struct ldb_message **m1, const struct ldb_message **m2,
-				char *search_name)
+int dns_name_compare(struct ldb_message * const *m1, struct ldb_message * const *m2,
+		     const char *search_name)
 {
 	const char *name1, *name2;
 	const char *ptr1, *ptr2;
@@ -1064,21 +1064,6 @@ int dns_name_compare(const struct ldb_message **m1, const struct ldb_message **m
 		return 0;
 	}
 
-	/* '@' record and the search_name record gets preference */
-	if (name1[0] == '@') {
-		return -1;
-	}
-	if (search_name && strcasecmp(name1, search_name) == 0) {
-		return -1;
-	}
-
-	if (name2[0] == '@') {
-		return 1;
-	}
-	if (search_name && strcasecmp(name2, search_name) == 0) {
-		return 1;
-	}
-
 	/* Compare the last components of names.
 	 * If search_name is not NULL, compare the second last components of names */
 	ptr1 = strrchr(name1, '.');
diff --git a/source4/rpc_server/dnsserver/dnsserver.h b/source4/rpc_server/dnsserver/dnsserver.h
index 93f1d72f2ef..690ab87ed10 100644
--- a/source4/rpc_server/dnsserver/dnsserver.h
+++ b/source4/rpc_server/dnsserver/dnsserver.h
@@ -188,8 +188,8 @@ struct DNS_ADDR_ARRAY *dns_addr_array_copy(TALLOC_CTX *mem_ctx, struct DNS_ADDR_
 int dns_split_name_components(TALLOC_CTX *mem_ctx, const char *name, char ***components);
 char *dns_split_node_name(TALLOC_CTX *mem_ctx, const char *node_name, const char *zone_name);
 
-int dns_name_compare(const struct ldb_message **m1, const struct ldb_message **m2,
-			char *search_name);
+int dns_name_compare(struct ldb_message * const *m1, struct ldb_message * const *m2,
+		     const char *search_name);
 bool dns_record_match(struct dnsp_DnssrvRpcRecord *rec1, struct dnsp_DnssrvRpcRecord *rec2);
 
 void dnsp_to_dns_copy(TALLOC_CTX *mem_ctx, struct dnsp_DnssrvRpcRecord *dnsp,
-- 
2.17.1


From 90073f0abc495c4b5bd05322b71667c534ee9dd8 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet@samba.org>
Date: Wed, 30 Oct 2019 11:50:57 +1300
Subject: [PATCH 4/9] CVE-2019-14861: Test to demonstrate the bug

This test does not fail every time, but when it does it casues a segfault which
takes out the rpc_server master process, as this hosts the dnsserver pipe.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=14138

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
---
 python/samba/tests/dcerpc/dnsserver.py | 47 ++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/python/samba/tests/dcerpc/dnsserver.py b/python/samba/tests/dcerpc/dnsserver.py
index 14ce308e38f..a9b8a4ace91 100644
--- a/python/samba/tests/dcerpc/dnsserver.py
+++ b/python/samba/tests/dcerpc/dnsserver.py
@@ -191,6 +191,53 @@ class DnsserverTests(RpcInterfaceTestCase):
         self.assertEqual(result.rec[4].dnsNodeName.str, "atestrecord-3")
         self.assertEqual(result.rec[5].dnsNodeName.str, "atestrecord-4")
 
+    def test_enum_is_sorted_with_zone_dup(self):
+        """
+        Confirm the zone is sorted
+        """
+
+        record_str = "192.168.50.50"
+        record_type_str = "A"
+        self.add_record(self.custom_zone, "atestrecord-1", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-2", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-3", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-4", record_type_str, record_str)
+        self.add_record(self.custom_zone, "atestrecord-0", record_type_str, record_str)
+
+        # This triggers a bug in old Samba
+        self.add_record(self.custom_zone, self.custom_zone + "1", record_type_str, record_str)
+
+        dn, record = self.get_record_from_db(self.custom_zone, self.custom_zone + "1")
+
+        new_dn = ldb.Dn(self.samdb, str(dn))
+        new_dn.set_component(0, "dc", self.custom_zone)
+        self.samdb.rename(dn, new_dn)
+
+        _, result = self.conn.DnssrvEnumRecords2(dnsserver.DNS_CLIENT_VERSION_LONGHORN,
+                                                 0,
+                                                 self.server,
+                                                 self.custom_zone,
+                                                 "@",
+                                                 None,
+                                                 self.record_type_int(record_type_str),
+                                                 dnsserver.DNS_RPC_VIEW_AUTHORITY_DATA,
+                                                 None,
+                                                 None)
+
+        self.assertEqual(len(result.rec), 7)
+        self.assertEqual(result.rec[0].dnsNodeName.str, "")
+        self.assertEqual(result.rec[1].dnsNodeName.str, "atestrecord-0")
+        self.assertEqual(result.rec[2].dnsNodeName.str, "atestrecord-1")
+        self.assertEqual(result.rec[3].dnsNodeName.str, "atestrecord-2")
+        self.assertEqual(result.rec[4].dnsNodeName.str, "atestrecord-3")
+        self.assertEqual(result.rec[5].dnsNodeName.str, "atestrecord-4")
+
+        # Windows doesn't reload the zone fast enough, but doesn't
+        # have the bug anyway, it will sort last on both names (where
+        # it should)
+        if result.rec[6].dnsNodeName.str != (self.custom_zone + "1"):
+            self.assertEqual(result.rec[6].dnsNodeName.str, self.custom_zone)
+
     def test_enum_is_sorted_children_prefix_first(self):
         """
         Confirm the zone returns the selected prefix first but no more
-- 
2.17.1


From 38db53fa5e930e6bc739f5ac8b7160048b6dd7d6 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet@samba.org>
Date: Fri, 1 Nov 2019 06:53:56 +1300
Subject: [PATCH 5/9] s4-torture: Reduce flapping in
 SambaToolDrsTests.test_samba_tool_replicate_local

This test often flaps in Samba 4.9 (where more tests and DCs run in the environment)
with obj_1 being 3.  This is quite OK, we just need to see some changes get
replicated, not 0 changes.

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
(cherry picked from commit 4ae0f9ce0f5ada99cf1d236377e5a1234c879ae3)
---
 source4/torture/drs/python/samba_tool_drs.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/source4/torture/drs/python/samba_tool_drs.py b/source4/torture/drs/python/samba_tool_drs.py
index 29c6016471c..74fa433ce59 100644
--- a/source4/torture/drs/python/samba_tool_drs.py
+++ b/source4/torture/drs/python/samba_tool_drs.py
@@ -209,6 +209,7 @@ class SambaToolDrsTests(drs_base.DrsBaseTestCase):
         self._disable_inbound_repl(self.dnsname_dc1)
         self._disable_inbound_repl(self.dnsname_dc2)
 
+        self._net_drs_replicate(DC=self.dnsname_dc2, fromDC=self.dnsname_dc1)
         self._net_drs_replicate(DC=self.dnsname_dc1, fromDC=self.dnsname_dc2)
 
         # add an object with link on dc1
@@ -231,7 +232,7 @@ class SambaToolDrsTests(drs_base.DrsBaseTestCase):
 
         (obj_1, link_1) = get_num_obj_links(out)
 
-        self.assertEqual(obj_1, 2)
+        self.assertGreaterEqual(obj_1, 2)
         self.assertEqual(link_1, 1)
 
         # pull that change with --local into local db from dc2: shouldn't send link or object
-- 
2.17.1


From 80ea4bde850048474d23f13fa5bf1149b7cc6859 Mon Sep 17 00:00:00 2001
From: Isaac Boukris <iboukris@gmail.com>
Date: Sun, 27 Oct 2019 14:02:00 +0200
Subject: [PATCH 6/9] samba-tool: add user-sensitive command to set
 not-delegated flag

Signed-off-by: Isaac Boukris <iboukris@gmail.com>
---
 python/samba/netcmd/user.py | 59 +++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/python/samba/netcmd/user.py b/python/samba/netcmd/user.py
index 437866c0a42..f2019af1b60 100644
--- a/python/samba/netcmd/user.py
+++ b/python/samba/netcmd/user.py
@@ -2647,6 +2647,64 @@ class cmd_user_move(Command):
         self.outf.write('Moved user "%s" into "%s"\n' %
                         (username, full_new_parent_dn))
 
+
+class cmd_user_sensitive(Command):
+    """Set/unset or show UF_NOT_DELEGATED for an account."""
+
+    synopsis = "%prog <accountname> [(show|on|off)] [options]"
+
+    takes_optiongroups = {
+        "sambaopts": options.SambaOptions,
+        "credopts": options.CredentialsOptions,
+        "versionopts": options.VersionOptions,
+    }
+
+    takes_options = [
+        Option("-H", "--URL", help="LDB URL for database or target server", type=str,
+               metavar="URL", dest="H"),
+    ]
+
+    takes_args = ["accountname", "cmd"]
+
+    def run(self, accountname, cmd, H=None, credopts=None, sambaopts=None,
+            versionopts=None):
+
+        if cmd not in ("show", "on", "off"):
+            raise CommandError("invalid argument: '%s' (choose from 'show', 'on', 'off')" % cmd)
+
+        lp = sambaopts.get_loadparm()
+        creds = credopts.get_credentials(lp, fallback_machine=True)
+        sam = SamDB(url=H, session_info=system_session(),
+                    credentials=creds, lp=lp)
+
+        search_filter = "sAMAccountName=%s" % ldb.binary_encode(accountname)
+        flag = dsdb.UF_NOT_DELEGATED;
+
+        if cmd == "show":
+            res = sam.search(scope=ldb.SCOPE_SUBTREE, expression=search_filter,
+                             attrs=["userAccountControl"])
+            if len(res) == 0:
+                raise Exception("Unable to find account where '%s'" % search_filter)
+
+            uac = int(res[0].get("userAccountControl")[0])
+
+            self.outf.write("Account-DN: %s\n" % str(res[0].dn))
+            self.outf.write("UF_NOT_DELEGATED: %s\n" % bool(uac & flag))
+
+            return
+
+        if cmd == "on":
+            on = True
+        elif cmd == "off":
+            on = False
+
+        try:
+            sam.toggle_userAccountFlags(search_filter, flag, flags_str="Not-Delegated",
+                                        on=on, strict=True)
+        except Exception as err:
+            raise CommandError(err)
+
+
 class cmd_user(SuperCommand):
     """User management."""
 
@@ -2665,3 +2723,4 @@ class cmd_user(SuperCommand):
     subcommands["edit"] = cmd_user_edit()
     subcommands["show"] = cmd_user_show()
     subcommands["move"] = cmd_user_move()
+    subcommands["sensitive"] = cmd_user_sensitive()
-- 
2.17.1


From 5249cad8b435d162584f010f492568d6f4526662 Mon Sep 17 00:00:00 2001
From: Isaac Boukris <iboukris@gmail.com>
Date: Wed, 30 Oct 2019 15:59:16 +0100
Subject: [PATCH 7/9] CVE-2019-14870: heimdal: add S4U test for
 delegation_not_allowed

Signed-off-by: Isaac Boukris <iboukris@gmail.com>
---
 selftest/knownfail.d/heimdal_not_delegated |  1 +
 source4/selftest/tests.py                  |  1 +
 testprogs/blackbox/test_s4u_heimdal.sh     | 73 ++++++++++++++++++++++
 3 files changed, 75 insertions(+)
 create mode 100644 selftest/knownfail.d/heimdal_not_delegated
 create mode 100755 testprogs/blackbox/test_s4u_heimdal.sh

diff --git a/selftest/knownfail.d/heimdal_not_delegated b/selftest/knownfail.d/heimdal_not_delegated
new file mode 100644
index 00000000000..bfc382a3fc2
--- /dev/null
+++ b/selftest/knownfail.d/heimdal_not_delegated
@@ -0,0 +1 @@
+^samba4.blackbox.krb5.s4u
diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py
index 9c3c77f1c56..2ec0bee923b 100755
--- a/source4/selftest/tests.py
+++ b/source4/selftest/tests.py
@@ -425,6 +425,7 @@ if have_heimdal_support:
     plantestsuite("samba4.blackbox.kinit_trust(fl2003dc:local)", "fl2003dc:local", [os.path.join(bbdir, "test_kinit_trusts_heimdal.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$REALM', '$DOMAIN', '$TRUST_SERVER', '$TRUST_USERNAME', '$TRUST_PASSWORD', '$TRUST_REALM', '$TRUST_DOMAIN', '$PREFIX', "external", "arcfour-hmac-md5"])
     plantestsuite("samba4.blackbox.export.keytab(ad_dc_ntvfs:local)", "ad_dc_ntvfs:local", [os.path.join(bbdir, "test_export_keytab_heimdal.sh"), '$SERVER', '$USERNAME', '$REALM', '$DOMAIN', "$PREFIX", smbclient4])
     plantestsuite("samba4.blackbox.kpasswd(ad_dc_ntvfs:local)", "ad_dc_ntvfs:local", [os.path.join(bbdir, "test_kpasswd_heimdal.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$REALM', '$DOMAIN', "$PREFIX/ad_dc_ntvfs"])
+    plantestsuite("samba4.blackbox.krb5.s4u", "fl2008r2dc:local", [os.path.join(bbdir, "test_s4u_heimdal.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$REALM', '$DOMAIN', '$PREFIX', configuration])
 else:
     plantestsuite("samba4.blackbox.kinit(ad_dc_ntvfs:local)", "ad_dc_ntvfs:local", [os.path.join(bbdir, "test_kinit_mit.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$REALM', '$DOMAIN', '$PREFIX', smbclient4, configuration])
     plantestsuite("samba4.blackbox.kinit(fl2000dc:local)", "fl2000dc:local", [os.path.join(bbdir, "test_kinit_mit.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$REALM', '$DOMAIN', '$PREFIX', smbclient4, configuration])
diff --git a/testprogs/blackbox/test_s4u_heimdal.sh b/testprogs/blackbox/test_s4u_heimdal.sh
new file mode 100755
index 00000000000..0e12c7ec096
--- /dev/null
+++ b/testprogs/blackbox/test_s4u_heimdal.sh
@@ -0,0 +1,73 @@
+#!/bin/sh
+
+if [ $# -lt 5 ]; then
+cat <<EOF
+Usage: test_kinit.sh SERVER USERNAME PASSWORD REALM DOMAIN PREFIX
+EOF
+exit 1;
+fi
+
+SERVER=$1
+USERNAME=$2
+PASSWORD=$3
+REALM=$4
+DOMAIN=$5
+PREFIX=$6
+shift 6
+failed=0
+
+
+samba_tool="$VALGRIND $PYTHON $BINDIR/samba-tool"
+
+samba4kinit=kinit
+if test -x $BINDIR/samba4kinit; then
+	samba4kinit=$BINDIR/samba4kinit
+fi
+
+samba4kgetcred=kgetcred
+if test -x $BINDIR/samba4kgetcred; then
+	samba4kgetcred=$BINDIR/samba4kgetcred
+fi
+
+. `dirname $0`/subunit.sh
+. `dirname $0`/common_test_fns.inc
+
+ocache="$PREFIX/tmpoutcache"
+KRB5CCNAME_PATH="$PREFIX/tmpccache"
+KRB5CCNAME="FILE:$KRB5CCNAME_PATH"
+export KRB5CCNAME
+rm -rf $KRB5CCNAME_PATH
+
+princ=test_impersonate_princ
+impersonator=test_impersonator
+target="CIFS/$SERVER.$REALM"
+
+
+testit "add impersonator principal" $samba_tool user add $impersonator $PASSWORD || failed=`expr $failed + 1`
+testit "become a service" $samba_tool spn add "HOST/$impersonator" $impersonator || failed=`expr $failed + 1`
+
+testit "set TrustedToAuthForDelegation" $samba_tool delegation for-any-protocol $impersonator on || failed=`expr $failed + 1`
+testit "add msDS-AllowedToDelegateTo" $samba_tool delegation add-service $impersonator $target || failed=`expr $failed + 1`
+
+testit "add a new principal" $samba_tool user add $princ --random-password || failed=`expr $failed + 1`
+testit "set not-delegated flag" $samba_tool user sensitive $princ on || failed=`expr $failed + 1`
+
+
+echo $PASSWORD > $PREFIX/tmppassfile
+testit "kinit with password" $samba4kinit -f --password-file=$PREFIX/tmppassfile $impersonator || failed=`expr $failed + 1`
+
+testit "test S4U2Self with normal user" $samba4kgetcred --out-cache=$ocache --forwardable --impersonate=${USERNAME} $impersonator || failed=`expr $failed + 1`
+testit "test S4U2Proxy with normal user" $samba4kgetcred --out-cache=$ocache --delegation-credential-cache=${ocache} $target || failed=`expr $failed + 1`
+
+testit "test S4U2Self with sensitive user" $samba4kgetcred --out-cache=$ocache --forwardable --impersonate=$princ $impersonator || failed=`expr $failed + 1`
+testit_expect_failure "test S4U2Proxy with sensitive user" $samba4kgetcred --out-cache=$ocache --delegation-credential-cache=${ocache} $target || failed=`expr $failed + 1`
+
+rm -f $ocache
+testit "unset not-delegated flag" $samba_tool user sensitive $princ off || failed=`expr $failed + 1`
+
+testit "test S4U2Self after unsetting ND flag" $samba4kgetcred --out-cache=$ocache --forwardable --impersonate=$princ $impersonator || failed=`expr $failed + 1`
+testit "test S4U2Proxy after unsetting ND flag" $samba4kgetcred --out-cache=$ocache --delegation-credential-cache=${ocache} $target || failed=`expr $failed + 1`
+
+
+rm -f $ocache $PREFIX/tmpccache tmppassfile
+exit $failed
-- 
2.17.1


From d0d4954b9b4643678b6f465959dd69de0faafd07 Mon Sep 17 00:00:00 2001
From: Isaac Boukris <iboukris@gmail.com>
Date: Mon, 28 Oct 2019 02:54:09 +0200
Subject: [PATCH 8/9] CVE-2019-14870: heimdal: enforce delegation_not_allowed
 in S4U2Self

Signed-off-by: Isaac Boukris <iboukris@gmail.com>
---
 selftest/knownfail.d/heimdal_not_delegated |  1 -
 source4/heimdal/kdc/krb5tgs.c              | 58 ++++++++++++++--------
 2 files changed, 36 insertions(+), 23 deletions(-)
 delete mode 100644 selftest/knownfail.d/heimdal_not_delegated

diff --git a/selftest/knownfail.d/heimdal_not_delegated b/selftest/knownfail.d/heimdal_not_delegated
deleted file mode 100644
index bfc382a3fc2..00000000000
--- a/selftest/knownfail.d/heimdal_not_delegated
+++ /dev/null
@@ -1 +0,0 @@
-^samba4.blackbox.krb5.s4u
diff --git a/source4/heimdal/kdc/krb5tgs.c b/source4/heimdal/kdc/krb5tgs.c
index ff7d93138c0..ee3ac3d8f53 100644
--- a/source4/heimdal/kdc/krb5tgs.c
+++ b/source4/heimdal/kdc/krb5tgs.c
@@ -1975,30 +1975,42 @@ server_lookup:
 	    if (ret)
 		goto out;
 
+	    ret = _kdc_db_fetch(context, config, tp, HDB_F_GET_CLIENT | flags,
+				NULL, &s4u2self_impersonated_clientdb,
+				&s4u2self_impersonated_client);
+	    if (ret) {
+		const char *msg;
+
+		/*
+		 * If the client belongs to the same realm as our krbtgt, it
+		 * should exist in the local database.
+		 *
+		 */
+
+		if (ret == HDB_ERR_NOENTRY)
+		    ret = KRB5KDC_ERR_C_PRINCIPAL_UNKNOWN;
+		msg = krb5_get_error_message(context, ret);
+		kdc_log(context, config, 1,
+			"S2U4Self principal to impersonate %s not found in database: %s",
+			tpn, msg);
+		krb5_free_error_message(context, msg);
+		goto out;
+	    }
+
+	    /* Ignore pw_end attributes (as Windows does),
+	     * since S4U2Self is not password authentication. */
+	    free(s4u2self_impersonated_client->entry.pw_end);
+	    s4u2self_impersonated_client->entry.pw_end = NULL;
+
+	    ret = kdc_check_flags(context, config, s4u2self_impersonated_client, tpn,
+				  NULL, NULL, FALSE);
+	    if (ret)
+		goto out;
+
 	    /* If we were about to put a PAC into the ticket, we better fix it to be the right PAC */
 	    if(rspac.data) {
 		krb5_pac p = NULL;
 		krb5_data_free(&rspac);
-		ret = _kdc_db_fetch(context, config, tp, HDB_F_GET_CLIENT | flags,
-				    NULL, &s4u2self_impersonated_clientdb, &s4u2self_impersonated_client);
-		if (ret) {
-		    const char *msg;
-
-		    /*
-		     * If the client belongs to the same realm as our krbtgt, it
-		     * should exist in the local database.
-		     *
-		     */
-
-		    if (ret == HDB_ERR_NOENTRY)
-			ret = KRB5KDC_ERR_C_PRINCIPAL_UNKNOWN;
-		    msg = krb5_get_error_message(context, ret);
-		    kdc_log(context, config, 1,
-			    "S2U4Self principal to impersonate %s not found in database: %s",
-			    tpn, msg);
-		    krb5_free_error_message(context, msg);
-		    goto out;
-		}
 		ret = _kdc_pac_generate(context, s4u2self_impersonated_client, NULL, &p);
 		if (ret) {
 		    kdc_log(context, config, 0, "PAC generation failed for -- %s",
@@ -2034,10 +2046,12 @@ server_lookup:
 
 	    /*
 	     * If the service isn't trusted for authentication to
-	     * delegation, remove the forward flag.
+	     * delegation or if the impersonate client is disallowed
+	     * forwardable, remove the forwardable flag.
 	     */
 
-	    if (client->entry.flags.trusted_for_delegation) {
+	    if (client->entry.flags.trusted_for_delegation &&
+		s4u2self_impersonated_client->entry.flags.forwardable) {
 		str = "[forwardable]";
 	    } else {
 		b->kdc_options.forwardable = 0;
-- 
2.17.1


From 277ab21fcf31bf60458410994e188d9c236963a3 Mon Sep 17 00:00:00 2001
From: Isaac Boukris <iboukris@gmail.com>
Date: Thu, 21 Nov 2019 11:12:48 +0100
Subject: [PATCH 9/9] CVE-2019-14870: mit-kdc: enforce delegation_not_allowed
 flag

BUG: https://bugzilla.samba.org/show_bug.cgi?id=14187

Signed-off-by: Isaac Boukris <iboukris@samba.org>
---
 source4/kdc/mit_samba.c  |  5 +++++
 source4/kdc/sdb_to_kdb.c | 17 ++++++-----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/source4/kdc/mit_samba.c b/source4/kdc/mit_samba.c
index eacca0903ec..06e680b60e2 100644
--- a/source4/kdc/mit_samba.c
+++ b/source4/kdc/mit_samba.c
@@ -304,6 +304,11 @@ fetch_referral_principal:
 
 	sdb_free_entry(&sentry);
 
+	if ((kflags & KRB5_KDB_FLAG_CLIENT_REFERRALS_ONLY) == 0) {
+		kentry->attributes &= ~KRB5_KDB_DISALLOW_FORWARDABLE;
+		kentry->attributes &= ~KRB5_KDB_DISALLOW_PROXIABLE;
+	}
+
 done:
 	krb5_free_principal(ctx->context, referral_principal);
 	referral_principal = NULL;
diff --git a/source4/kdc/sdb_to_kdb.c b/source4/kdc/sdb_to_kdb.c
index 74d882738f8..b7253ade122 100644
--- a/source4/kdc/sdb_to_kdb.c
+++ b/source4/kdc/sdb_to_kdb.c
@@ -36,18 +36,13 @@ static int SDBFlags_to_kflags(const struct SDBFlags *s,
 	if (s->initial) {
 		*k |= KRB5_KDB_DISALLOW_TGT_BASED;
 	}
-	/*
-	 * Do not set any disallow rules for forwardable, proxiable,
-	 * renewable, postdate and server.
-	 *
-	 * The KDC will take care setting the flags based on the incoming
-	 * ticket.
-	 */
-	if (s->forwardable) {
-		;
+	/* The forwardable and proxiable flags are set according to client and
+	 * server attributes. */
+	if (!s->forwardable) {
+		*k |= KRB5_KDB_DISALLOW_FORWARDABLE;
 	}
-	if (s->proxiable) {
-		;
+	if (!s->proxiable) {
+		*k |= KRB5_KDB_DISALLOW_PROXIABLE;
 	}
 	if (s->renewable) {
 		;
-- 
2.17.1