Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Tim Beale timbeale at catalyst.net.nz
Tue Aug 29 02:52:18 UTC 2017


Hi,

Attached is an updated patch-set that should hopefully address your
review comments. It applies on top of the bug-fix patch I just sent.

This patch-set also contains a couple of new patches I added. I updated
the tests to send several bad GetNCChanges requests one after the other,
and it highlighted a problem where Samba could incorrectly 'accept'
these bad requests (note that although Samba didn't reject these bad
requests outright, it also didn't return any actual objects in the
response). I had to fix this problem in order to get the tests working
as expected.

Cheers,
Tim

On 14/08/17 19:20, Andrew Bartlett via samba-technical wrote:
> On Thu, 2017-08-10 at 13:04 +0200, Stefan Metzmacher wrote:
>> Hi Andrew,
>>
>> here's my version with some comments in it.
>>
>> In general please check if we need the BUG: line
>> in every commit, and move commits without towards the end of the
>> patchset. (The BUG: line should be before the Signed-off-by: line
>> followed by an empty line...)
>>
>> metze
> 
> Regarding:
> 
>> Subject: [PATCH 05/10] TODO: s4-drsuapi: Refuse to replicate an NC is that not
>>  actually an NC
>>
>> This prevents replication of an OU, you must replicate a whole NC per Windows 2012R2
>>
>> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
>>
>> TODO: shouldn't we use dsdb_search_one()?
> 
> No, I think dsdb_search_dn() is correct, as used often elsewhere.  We
> could remove the check of count > 1, we can't get that unless we have
> DB corruption, but we of course just had that...
> 
> I'm heads-down in the binary index work, but I'm grateful to Tim who
> will tidy this up for us, so you can expect to see an improved set of
> patches soon. 
> 
> Thanks,
> 
> Andrew Bartlett
> 
-------------- next part --------------
From 03ade4cf3cbd47f163255528943ae9bdbe17356d Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 14 Aug 2017 11:02:05 +1200
Subject: [PATCH 01/14] s4-drsuapi: Use sam_ctx consistently in
 dcesrv_drsuapi_DsGetNCChanges()

Trying to use bstate->sam_ctx_system by mistake can cause crashes if
non-admin users replicate. To avoid this problem we use the sam_ctx
variable, however it wasn't used consistently everywhere. Replace the
remaining references to b_state->sam_ctx to avoid potential confusion.

This change was made based on review feedback from Metze.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 source4/rpc_server/drsuapi/getncchanges.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index b98f14c..1f9a35f 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -2043,6 +2043,7 @@ WERROR dcesrv_drsuapi_DsGetNCChanges(struct dcesrv_call_state *dce_call, TALLOC_
 	DCESRV_PULL_HANDLE_WERR(h, r->in.bind_handle, DRSUAPI_BIND_HANDLE);
 	b_state = h->data;
 
+	/* sam_ctx_system is not present for non-administrator users */
 	sam_ctx = b_state->sam_ctx_system?b_state->sam_ctx_system:b_state->sam_ctx;
 
 	invocation_id = *(samdb_ntds_invocation_id(sam_ctx));
@@ -2107,7 +2108,7 @@ WERROR dcesrv_drsuapi_DsGetNCChanges(struct dcesrv_call_state *dce_call, TALLOC_
 	user_sid = &dce_call->conn->auth_state.session_info->security_token->sids[PRIMARY_USER_SID_INDEX];
 
 	/* all clients must have GUID_DRS_GET_CHANGES */
-	werr = drs_security_access_check_nc_root(b_state->sam_ctx,
+	werr = drs_security_access_check_nc_root(sam_ctx,
 						 mem_ctx,
 						 dce_call->conn->auth_state.session_info->security_token,
 						 req10->naming_context,
@@ -2149,7 +2150,7 @@ WERROR dcesrv_drsuapi_DsGetNCChanges(struct dcesrv_call_state *dce_call, TALLOC_
 		return werr;
 	}
 	if (is_gc_pas_request) {
-		werr = drs_security_access_check_nc_root(b_state->sam_ctx,
+		werr = drs_security_access_check_nc_root(sam_ctx,
 							 mem_ctx,
 							 dce_call->conn->auth_state.session_info->security_token,
 							 req10->naming_context,
@@ -2166,7 +2167,7 @@ WERROR dcesrv_drsuapi_DsGetNCChanges(struct dcesrv_call_state *dce_call, TALLOC_
 		return werr;
 	}
 	if (is_secret_request) {
-		werr = drs_security_access_check_nc_root(b_state->sam_ctx,
+		werr = drs_security_access_check_nc_root(sam_ctx,
 							 mem_ctx,
 							 dce_call->conn->auth_state.session_info->security_token,
 							 req10->naming_context,
@@ -2261,7 +2262,7 @@ allowed:
 		ncRoot->guid = getnc_state->ncRoot_guid;
 
 		/* find out if we are to replicate Schema NC */
-		ret = ldb_dn_compare_base(ldb_get_schema_basedn(b_state->sam_ctx),
+		ret = ldb_dn_compare_base(ldb_get_schema_basedn(sam_ctx),
 					  getnc_state->ncRoot_dn);
 
 		getnc_state->is_schema_nc = (0 == ret);
@@ -2532,7 +2533,7 @@ allowed:
 		struct dsdb_syntax_ctx syntax_ctx;
 		uint32_t j = 0;
 
-		dsdb_syntax_ctx_init(&syntax_ctx, b_state->sam_ctx, schema);
+		dsdb_syntax_ctx_init(&syntax_ctx, sam_ctx, schema);
 		syntax_ctx.pfm_remote = pfm_remote;
 
 		local_pas = talloc_array(b_state, uint32_t, req10->partial_attribute_set->num_attids);
@@ -2832,7 +2833,7 @@ allowed:
 		DEBUG(3,("UpdateRefs on getncchanges for %s\n",
 			 GUID_string(mem_ctx, &req10->destination_dsa_guid)));
 		ureq.naming_context = ncRoot;
-		ureq.dest_dsa_dns_name = samdb_ntds_msdcs_dns_name(b_state->sam_ctx, mem_ctx,
+		ureq.dest_dsa_dns_name = samdb_ntds_msdcs_dns_name(sam_ctx, mem_ctx,
 								   &req10->destination_dsa_guid);
 		if (!ureq.dest_dsa_dns_name) {
 			return WERR_NOT_ENOUGH_MEMORY;
-- 
2.7.4


From 782e60451b844251ad68af66d1c3c68a5e534880 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Fri, 4 Aug 2017 15:21:31 +1200
Subject: [PATCH 02/14] dsdb: Use samba.generate_random_password() in dirsync
 test

We do not like fixed passwords

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

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
---
 source4/dsdb/tests/python/dirsync.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py
index e6c5bba..168d511 100755
--- a/source4/dsdb/tests/python/dirsync.py
+++ b/source4/dsdb/tests/python/dirsync.py
@@ -79,7 +79,7 @@ class DirsyncBaseTests(samba.tests.TestCase):
         self.ldb_admin = SamDB(ldapshost, credentials=creds, session_info=system_session(lp), lp=lp)
         self.base_dn = self.ldb_admin.domain_dn()
         self.domain_sid = security.dom_sid(self.ldb_admin.get_domain_sid())
-        self.user_pass = "samba123 at AAA"
+        self.user_pass = samba.generate_random_password(12, 16)
         self.configuration_dn = self.ldb_admin.get_config_basedn().get_linearized()
         self.sd_utils = sd_utils.SDUtils(self.ldb_admin)
         #used for anonymous login
-- 
2.7.4


From 9936e33ec0123340cc134ad160143c775ec1ee6f Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Wed, 9 Aug 2017 13:56:07 +1200
Subject: [PATCH 03/14] selftest: Make dirsync test use symobolic name and OA
 not A

A is for Allow, OA is for Object Allow, which means check the GUID.

The previous ACE allowed all access, which was not the intention.

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
---
 source4/dsdb/tests/python/dirsync.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py
index 168d511..e302c42 100755
--- a/source4/dsdb/tests/python/dirsync.py
+++ b/source4/dsdb/tests/python/dirsync.py
@@ -30,7 +30,7 @@ import base64
 from ldb import LdbError, SCOPE_BASE
 from ldb import Message, MessageElement, Dn
 from ldb import FLAG_MOD_ADD, FLAG_MOD_DELETE
-from samba.dcerpc import security, misc, drsblobs
+from samba.dcerpc import security, misc, drsblobs, security
 from samba.ndr import ndr_unpack, ndr_pack
 
 from samba.auth import system_session
@@ -119,7 +119,8 @@ class SimpleDirsyncTests(DirsyncBaseTests):
         self.desc_sddl = self.sd_utils.get_sd_as_sddl(self.base_dn)
 
         user_sid = self.sd_utils.get_object_sid(self.get_user_dn(self.dirsync_user))
-        mod = "(A;;CR;1131f6aa-9c07-11d1-f79f-00c04fc2dcd2;;%s)" % str(user_sid)
+        mod = "(OA;;CR;%s;;%s)" % (security.GUID_DRS_GET_CHANGES,
+                                   str(user_sid))
         self.sd_utils.dacl_add_ace(self.base_dn, mod)
 
         # add admins to the Domain Admins group
-- 
2.7.4


From 2a300ad80d78166538b00336ce21787c6fd804a5 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Tue, 8 Aug 2017 14:34:14 +1200
Subject: [PATCH 04/14] s4-drsuapi: Refuse to replicate an NC is that not
 actually an NC

This prevents replication of an OU, you must replicate a whole NC per Windows 2012R2

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 source4/rpc_server/drsuapi/getncchanges.c  | 39 ++++++++++++++++++++++++------
 source4/torture/drs/python/getnc_unpriv.py |  7 +-----
 2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index 1f9a35f..705d068 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -2241,6 +2241,14 @@ allowed:
 	}
 
 	if (getnc_state == NULL) {
+		struct ldb_result *res = NULL;
+		const char *attrs[] = {
+			"instanceType",
+			"objectGuID",
+			NULL
+		};
+		uint32_t nc_instanceType;
+
 		getnc_state = talloc_zero(b_state, struct drsuapi_getncchanges_state);
 		if (getnc_state == NULL) {
 			return WERR_NOT_ENOUGH_MEMORY;
@@ -2251,14 +2259,21 @@ allowed:
 			return WERR_NOT_ENOUGH_MEMORY;
 		}
 
-		ret = dsdb_find_guid_by_dn(b_state->sam_ctx,
-					   getnc_state->ncRoot_dn,
-					   &getnc_state->ncRoot_guid);
+		ret = dsdb_search_dn(sam_ctx, mem_ctx, &res,
+				     getnc_state->ncRoot_dn, attrs,
+				     DSDB_SEARCH_SHOW_DELETED |
+				     DSDB_SEARCH_SHOW_RECYCLED);
 		if (ret != LDB_SUCCESS) {
-			DEBUG(0,(__location__ ": Failed to find GUID of ncRoot_dn %s\n",
-				 ldb_dn_get_linearized(getnc_state->ncRoot_dn)));
-			return WERR_DS_DRA_INTERNAL_ERROR;
-		}
+			DBG_WARNING("Failed to find ncRoot_dn %s\n",
+				    ldb_dn_get_linearized(getnc_state->ncRoot_dn));
+			return WERR_DS_CANT_FIND_EXPECTED_NC;
+		}
+		getnc_state->ncRoot_guid = samdb_result_guid(res->msgs[0],
+							     "objectGUID");
+		nc_instanceType = ldb_msg_find_attr_as_int(res->msgs[0],
+							   "instanceType",
+							   0);
+		TALLOC_FREE(res);
 		ncRoot->guid = getnc_state->ncRoot_guid;
 
 		/* find out if we are to replicate Schema NC */
@@ -2279,6 +2294,16 @@ allowed:
 		 */
 		switch (req10->extended_op) {
 		case DRSUAPI_EXOP_NONE:
+			if ((nc_instanceType & INSTANCE_TYPE_IS_NC_HEAD) == 0) {
+				const char *dn_str
+					= ldb_dn_get_linearized(getnc_state->ncRoot_dn);
+
+				DBG_NOTICE("Rejecting full replication on "
+					   "not NC %s", dn_str);
+
+				return WERR_DS_CANT_FIND_EXPECTED_NC;
+			}
+
 			break;
 		case DRSUAPI_EXOP_FSMO_RID_ALLOC:
 			werr = getncchanges_rid_alloc(b_state, mem_ctx, req10, &r->out.ctr->ctr6, &search_dn);
diff --git a/source4/torture/drs/python/getnc_unpriv.py b/source4/torture/drs/python/getnc_unpriv.py
index b1725f4..8e40da4 100644
--- a/source4/torture/drs/python/getnc_unpriv.py
+++ b/source4/torture/drs/python/getnc_unpriv.py
@@ -102,14 +102,9 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
         user with the correct GET_CHANGES rights
         """
 
-        ou1 = "OU=single_obj,%s" % self.ou
-        self.ldb_dc1.add({
-            "dn": ou1,
-            "objectclass": "organizationalUnit"
-            })
         req8 = self._exop_req8(dest_dsa=None,
                                invocation_id=self.ldb_dc1.get_invocation_id(),
-                               nc_dn_str=ou1,
+                               nc_dn_str=self.ldb_dc1.get_default_basedn(),
                                exop=drsuapi.DRSUAPI_EXOP_NONE,
                                replica_flags=drsuapi.DRSUAPI_DRS_WRIT_REP)
         (level, ctr) = self.user_drs.DsGetNCChanges(self.user_drs_handle, 8, req8)
-- 
2.7.4


From 1b013d7362f9e0cd5610912890fbc12f31cec4ce Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Tue, 8 Aug 2017 16:49:24 +1200
Subject: [PATCH 05/14] selftest: encrypt the LDAP connection in drs_base.py

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
---
 source4/torture/drs/python/drs_base.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/source4/torture/drs/python/drs_base.py b/source4/torture/drs/python/drs_base.py
index b19c51a..2803e18 100644
--- a/source4/torture/drs/python/drs_base.py
+++ b/source4/torture/drs/python/drs_base.py
@@ -32,7 +32,7 @@ from samba import dsdb
 from samba.dcerpc import drsuapi, misc, drsblobs, security
 from samba.ndr import ndr_unpack, ndr_pack
 from samba.drs_utils import drs_DsBind
-
+from samba import gensec
 from ldb import (
     SCOPE_BASE,
     Message,
@@ -49,6 +49,8 @@ class DrsBaseTestCase(SambaToolCmdTest):
 
     def setUp(self):
         super(DrsBaseTestCase, self).setUp()
+        creds = self.get_credentials()
+        creds.set_gensec_features(creds.get_gensec_features() | gensec.FEATURE_SEAL)
 
         # connect to DCs
         url_dc = samba.tests.env_get_var_value("DC1")
-- 
2.7.4


From 33467e887de62eac262599196a7637a9537d4970 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Tue, 8 Aug 2017 16:50:11 +1200
Subject: [PATCH 06/14] selftest: Move get_partial_attribute_set() to
 DrsBaseTestCase

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
---
 source4/torture/drs/python/drs_base.py   | 7 +++++++
 source4/torture/drs/python/getnc_exop.py | 6 ------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/source4/torture/drs/python/drs_base.py b/source4/torture/drs/python/drs_base.py
index 2803e18..e79f076 100644
--- a/source4/torture/drs/python/drs_base.py
+++ b/source4/torture/drs/python/drs_base.py
@@ -452,6 +452,13 @@ class DrsBaseTestCase(SambaToolCmdTest):
         (drs_handle, supported_extensions) = drs_DsBind(drs)
         return (drs, drs_handle)
 
+    def get_partial_attribute_set(self, attids=[drsuapi.DRSUAPI_ATTID_objectClass]):
+        partial_attribute_set = drsuapi.DsPartialAttributeSet()
+        partial_attribute_set.attids = attids
+        partial_attribute_set.num_attids = len(attids)
+        return partial_attribute_set
+
+
 
 class AbstractLink:
     def __init__(self, attid, flags, identifier, targetGUID,
diff --git a/source4/torture/drs/python/getnc_exop.py b/source4/torture/drs/python/getnc_exop.py
index caa7826..2b4bdc4 100644
--- a/source4/torture/drs/python/getnc_exop.py
+++ b/source4/torture/drs/python/getnc_exop.py
@@ -559,12 +559,6 @@ class DrsReplicaPrefixMapTestCase(drs_base.DrsBaseTestCase):
             if enum == ldb.ERR_NO_SUCH_OBJECT:
                 pass
 
-    def get_partial_attribute_set(self, attids=[drsuapi.DRSUAPI_ATTID_objectClass]):
-        partial_attribute_set = drsuapi.DsPartialAttributeSet()
-        partial_attribute_set.attids = attids
-        partial_attribute_set.num_attids = len(attids)
-        return partial_attribute_set
-
     def test_missing_prefix_map_dsa(self):
         partial_attribute_set = self.get_partial_attribute_set()
 
-- 
2.7.4


From 53680a7fc547c4b900f213ced452ce265abb2ad7 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Tue, 8 Aug 2017 16:52:04 +1200
Subject: [PATCH 07/14] selftest: Confirm privileged replication of an OU is
 not permitted

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 source4/torture/drs/python/getnc_exop.py | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/source4/torture/drs/python/getnc_exop.py b/source4/torture/drs/python/getnc_exop.py
index 2b4bdc4..37e5333 100644
--- a/source4/torture/drs/python/getnc_exop.py
+++ b/source4/torture/drs/python/getnc_exop.py
@@ -35,6 +35,7 @@ from drs_base import AbstractLink
 
 import samba.tests
 import random
+from samba import werror, WERRORError
 
 import ldb
 from ldb import SCOPE_BASE
@@ -193,6 +194,29 @@ class DrsReplicaSyncTestCase(drs_base.DrsBaseTestCase):
         (level, ctr) = self.drs.DsGetNCChanges(self.drs_handle, 8, req8)
         self._check_ctr6(ctr, [ou2])
 
+    def test_do_full_repl_on_ou(self):
+        """
+        Make sure that a full replication on a not-an-nc fails with
+        the right error code
+        """
+
+        non_nc_ou = "OU=not-an-NC,%s" % self.ou
+        self.ldb_dc1.add({
+            "dn": non_nc_ou,
+            "objectclass": "organizationalUnit"
+            })
+        req8 = self._exop_req8(dest_dsa=None,
+                               invocation_id=self.ldb_dc1.get_invocation_id(),
+                               nc_dn_str=non_nc_ou,
+                               exop=drsuapi.DRSUAPI_EXOP_NONE,
+                               replica_flags=drsuapi.DRSUAPI_DRS_WRIT_REP)
+
+        try:
+            (level, ctr) = self.drs.DsGetNCChanges(self.drs_handle, 8, req8)
+            self.fail("Expected DsGetNCChanges to fail with WERR_DS_CANT_FIND_EXPECTED_NC")
+        except WERRORError as (enum, estr):
+            self.assertEquals(enum, werror.WERR_DS_CANT_FIND_EXPECTED_NC)
+
     def test_link_utdv_hwm(self):
         """Test verify the DRS_GET_ANC behavior."""
 
-- 
2.7.4


From 0b08fc6798787889a53c0015cda43e36db33d96a Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 16 Aug 2017 16:57:15 +1200
Subject: [PATCH 08/14] selftest: Extend further getnc_unpriv tests to pass
 against windows 2012R2

An important change in this patch is changing the ACE type from
 A (Allow)
to
 AO (Object Allow)

as that will then respect the supplied GUID, which we also make use
the constant from the security.idl.

This reworks the tests to check replication with users with the
following rights:
- only GET_CHANGES
- only GET_ALL_CHANGES
- both GET_CHANGES and GET_ALL_CHANGES
- no rights

We basically want to test various different GetNCChanges requests
against each type of user rights, and the only difference is the
error/success value we get back. I've structured the tests this way, so
that we have 4 test_repl_xyz_userpriv() functions (to cover each of the
above user rights cases), and each test sends the same series of
GetNCChanges requests of varying validity.

Currently all these tests fail against Samba because Samba sends
different error codes to Windows.

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 selftest/knownfail.d/getnc_unpriv          |  10 ++
 source4/torture/drs/python/getnc_unpriv.py | 211 ++++++++++++++++++++++++-----
 2 files changed, 190 insertions(+), 31 deletions(-)
 create mode 100644 selftest/knownfail.d/getnc_unpriv

diff --git a/selftest/knownfail.d/getnc_unpriv b/selftest/knownfail.d/getnc_unpriv
new file mode 100644
index 0000000..40977f9
--- /dev/null
+++ b/selftest/knownfail.d/getnc_unpriv
@@ -0,0 +1,10 @@
+samba4.drs.getnc_unpriv.python\(vampire_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_no_userpriv\(vampire_dc\)
+samba4.drs.getnc_unpriv.python\(vampire_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_getchanges_userpriv\(vampire_dc\)
+samba4.drs.getnc_unpriv.python\(vampire_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_getallchanges_userpriv\(vampire_dc\)
+samba4.drs.getnc_unpriv.python\(vampire_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_both_userpriv\(vampire_dc\)
+samba4.drs.getnc_unpriv.python\(vampire_dc\)\(vampire_dc\)
+samba4.drs.getnc_unpriv.python\(promoted_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_no_userpriv\(promoted_dc\)
+samba4.drs.getnc_unpriv.python\(promoted_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_getchanges_userpriv\(promoted_dc\)
+samba4.drs.getnc_unpriv.python\(promoted_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_getallchanges_userpriv\(promoted_dc\)
+samba4.drs.getnc_unpriv.python\(promoted_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_both_userpriv\(promoted_dc\)
+samba4.drs.getnc_unpriv.python\(promoted_dc\)\(promoted_dc\)
diff --git a/source4/torture/drs/python/getnc_unpriv.py b/source4/torture/drs/python/getnc_unpriv.py
index 8e40da4..41d9611 100644
--- a/source4/torture/drs/python/getnc_unpriv.py
+++ b/source4/torture/drs/python/getnc_unpriv.py
@@ -1,7 +1,12 @@
 #!/usr/bin/env python
 # -*- coding: utf-8 -*-
 #
-# Tests replication scenarios with different user privileges
+# Tests replication scenarios with different user privileges.
+# We want to test every replication scenario we can think of against:
+# - users with only GET_CHANGES privileges
+# - users with only GET_ALL_CHANGES privileges
+# - users with both GET_CHANGES and GET_ALL_CHANGES privileges
+# - users with no privileges
 #
 # Copyright (C) Kamen Mazdrashki <kamenim at samba.org> 2011
 # Copyright (C) Andrew Bartlett <abartlet at samba.org> 2017
@@ -30,12 +35,13 @@
 
 import drs_base
 import samba.tests
+from samba import werror, WERRORError
 
 from samba import sd_utils
 import ldb
 from ldb import SCOPE_BASE
 
-from samba.dcerpc import drsuapi
+from samba.dcerpc import drsuapi, security
 from samba.credentials import DONT_USE_KERBEROS
 
 class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
@@ -55,10 +61,13 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
         (self.drs, self.drs_handle) = self._ds_bind(self.dnsname_dc1)
 
         self.sd_utils = sd_utils.SDUtils(self.ldb_dc1)
-        user_dn = "cn=%s,%s" % (self.get_changes_user, self.ou)
-        user_sid = self.sd_utils.get_object_sid(user_dn)
-        mod = "(A;;CR;1131f6aa-9c07-11d1-f79f-00c04fc2dcd2;;%s)" % str(user_sid)
-        self.sd_utils.dacl_add_ace(self.base_dn, mod)
+        self.user_dn = "cn=%s,%s" % (self.get_changes_user, self.ou)
+        user_sid = self.sd_utils.get_object_sid(self.user_dn)
+        self.acl_mod_get_changes = "(OA;;CR;%s;;%s)" % (security.GUID_DRS_GET_CHANGES,
+                                                        str(user_sid))
+        self.acl_mod_get_all_changes = "(OA;;CR;%s;;%s)" % (security.GUID_DRS_GET_ALL_CHANGES,
+                                                            str(user_sid))
+        self.desc_sddl = self.sd_utils.get_sd_as_sddl(self.base_dn)
 
         # We set DONT_USE_KERBEROS to avoid a race with getting the
         # user replicated to our selected KDC
@@ -70,6 +79,7 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
                                                               self.user_creds)
 
     def tearDown(self):
+        self.sd_utils.modify_sd_on_dn(self.base_dn, self.desc_sddl)
         try:
             self.ldb_dc1.delete(self.ou, ["tree_delete:1"])
         except ldb.LdbError as (enum, string):
@@ -77,36 +87,175 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
                 pass
         super(DrsReplicaSyncUnprivTestCase, self).tearDown()
 
-    def test_do_single_repl(self):
+    def _test_repl_exop(self, exop, repl_obj, expected_error, dest_dsa=None,
+                        partial_attribute_set=None):
         """
-        Make sure that DRSU_EXOP_REPL_OBJ works as a less-privileged
-        user with the correct GET_CHANGES rights
+        Common function to send a replication request and check the result
+        matches what's expected.
         """
-
-        ou1 = "OU=single_obj,%s" % self.ou
-        self.ldb_dc1.add({
-            "dn": ou1,
-            "objectclass": "organizationalUnit"
-            })
-        req8 = self._exop_req8(dest_dsa=None,
+        req8 = self._exop_req8(dest_dsa=dest_dsa,
                                invocation_id=self.ldb_dc1.get_invocation_id(),
-                               nc_dn_str=ou1,
-                               exop=drsuapi.DRSUAPI_EXOP_REPL_OBJ,
-                               replica_flags=drsuapi.DRSUAPI_DRS_WRIT_REP)
-        (level, ctr) = self.user_drs.DsGetNCChanges(self.user_drs_handle, 8, req8)
-        self._check_ctr6(ctr, [ou1])
+                               nc_dn_str=repl_obj,
+                               exop=exop,
+                               replica_flags=drsuapi.DRSUAPI_DRS_WRIT_REP,
+                               partial_attribute_set=partial_attribute_set)
+
+        if expected_error is None:
+            # user is OK, request should be accepted without throwing an error
+            (level, ctr) = self.user_drs.DsGetNCChanges(self.user_drs_handle,
+                                                        8, req8)
+        else:
+            # check the request is rejected (with the error we're expecting)
+            try:
+                (level, ctr) = self.user_drs.DsGetNCChanges(self.user_drs_handle,
+                                                            8, req8)
+                self.fail("Should have failed with user denied access")
+            except WERRORError as (enum, estr):
+                self.assertEquals(enum, expected_error,
+                                  "Got unexpected error: %s" % estr)
 
-    def test_do_full_repl(self):
+    def _test_repl_single_obj(self, repl_obj, expected_error,
+                              partial_attribute_set=None):
         """
-        Make sure that full replication works as a less-privileged
-        user with the correct GET_CHANGES rights
+        Checks that replication on a single object either succeeds or fails as
+        expected (based on the user's access rights)
         """
+        self._test_repl_exop(exop=drsuapi.DRSUAPI_EXOP_REPL_OBJ,
+                             repl_obj=repl_obj,
+                             expected_error=expected_error,
+                             partial_attribute_set=partial_attribute_set)
+
+    def _test_repl_secret(self, repl_obj, expected_error, dest_dsa=None):
+        """
+        Checks that REPL_SECRET on an object either succeeds or fails as
+        expected (based on the user's access rights)
+        """
+        self._test_repl_exop(exop=drsuapi.DRSUAPI_EXOP_REPL_SECRET,
+                             repl_obj=repl_obj,
+                             expected_error=expected_error,
+                             dest_dsa=dest_dsa)
+
+    def _test_repl_full(self, expected_error, partial_attribute_set=None):
+        """
+        Checks that a full replication either succeeds or fails as expected
+        (based on the user's access rights)
+        """
+        self._test_repl_exop(exop=drsuapi.DRSUAPI_EXOP_NONE,
+                             repl_obj=self.ldb_dc1.get_default_basedn(),
+                             expected_error=expected_error,
+                             partial_attribute_set=partial_attribute_set)
+
+    def _test_repl_full_on_ou(self, expected_error):
+        """
+        Full replication on a specific OU should always fail (it should be done
+        against a base NC). The error may vary based on the user's access rights
+        """
+        # Just try against the OU created in the test setup
+        self._test_repl_exop(exop=drsuapi.DRSUAPI_EXOP_NONE,
+                             repl_obj=self.ou,
+                             expected_error=expected_error)
+
+    def test_repl_getchanges_userpriv(self):
+        """
+        Tests various replication requests made by a user with only GET_CHANGES
+        rights. Some requests will be accepted, but most will be rejected.
+        """
+
+        # Assign the user GET_CHANGES rights
+        self.sd_utils.dacl_add_ace(self.base_dn, self.acl_mod_get_changes)
+
+        self._test_repl_single_obj(repl_obj=self.ou,
+                                   expected_error=werror.WERR_DS_DRA_ACCESS_DENIED)
+
+        self._test_repl_secret(repl_obj=self.ou,
+                               expected_error=werror.WERR_DS_DRA_ACCESS_DENIED)
+        self._test_repl_secret(repl_obj=self.user_dn,
+                               expected_error=werror.WERR_DS_DRA_ACCESS_DENIED)
+        self._test_repl_secret(repl_obj=self.user_dn,
+                               dest_dsa=self.ldb_dc1.get_ntds_GUID(),
+                               expected_error=werror.WERR_DS_DRA_ACCESS_DENIED)
+
+        self._test_repl_full(expected_error=werror.WERR_DS_DRA_ACCESS_DENIED)
+        self._test_repl_full_on_ou(expected_error=werror.WERR_DS_CANT_FIND_EXPECTED_NC)
+
+        # Partial Attribute Sets don't require GET_ALL_CHANGES rights, so we
+        # expect the following to succeed
+        self._test_repl_single_obj(repl_obj=self.ou,
+                                   expected_error=None,
+                                   partial_attribute_set=self.get_partial_attribute_set())
+        self._test_repl_full(expected_error=None,
+                             partial_attribute_set=self.get_partial_attribute_set())
+
+    def test_repl_getallchanges_userpriv(self):
+        """
+        Tests various replication requests made by a user with only
+        GET_ALL_CHANGES rights. Note that assigning these rights is possible,
+        but doesn't make a lot of sense. We test it anyway for consistency.
+        """
+
+        # Assign the user GET_ALL_CHANGES rights
+        self.sd_utils.dacl_add_ace(self.base_dn, self.acl_mod_get_all_changes)
+
+        # We can expect to get the same responses as an unprivileged user,
+        # i.e. we have permission to see the results, but don't have permission
+        # to ask
+        self.test_repl_no_userpriv()
+
+    def test_repl_both_userpriv(self):
+        """
+        Tests various replication requests made by a privileged user (i.e. has
+        both GET_CHANGES and GET_ALL_CHANGES). We expect any valid requests
+        to be accepted.
+        """
+
+        # Assign the user both GET_CHANGES and GET_ALL_CHANGES rights
+        both_rights = self.acl_mod_get_changes + self.acl_mod_get_all_changes
+        self.sd_utils.dacl_add_ace(self.base_dn, both_rights)
+
+        self._test_repl_single_obj(repl_obj=self.ou,
+                                   expected_error=None)
+
+        self._test_repl_secret(repl_obj=self.ou,
+                               expected_error=werror.WERR_DS_DRA_DB_ERROR)
+        self._test_repl_secret(repl_obj=self.user_dn,
+                               expected_error=werror.WERR_DS_DRA_DB_ERROR)
+        self._test_repl_secret(repl_obj=self.user_dn,
+                               dest_dsa=self.ldb_dc1.get_ntds_GUID(),
+                               expected_error=None)
+
+        self._test_repl_full(expected_error=None)
+        self._test_repl_full_on_ou(expected_error=werror.WERR_DS_CANT_FIND_EXPECTED_NC)
+
+        self._test_repl_single_obj(repl_obj=self.ou,
+                                   expected_error=None,
+                                   partial_attribute_set=self.get_partial_attribute_set())
+        self._test_repl_full(expected_error=None,
+                             partial_attribute_set=self.get_partial_attribute_set())
+
+    def test_repl_no_userpriv(self):
+        """
+        Tests various replication requests made by a unprivileged user.
+        We expect all these requests to be rejected.
+        """
+
+        self._test_repl_single_obj(repl_obj=self.ou,
+                                   expected_error=werror.WERR_DS_DRA_BAD_DN)
+
+        self._test_repl_secret(repl_obj=self.ou,
+                               expected_error=werror.WERR_DS_DRA_BAD_DN)
+        self._test_repl_secret(repl_obj=self.user_dn,
+                               expected_error=werror.WERR_DS_DRA_BAD_DN)
+        self._test_repl_secret(repl_obj=self.user_dn,
+                               dest_dsa=self.ldb_dc1.get_ntds_GUID(),
+                               expected_error=werror.WERR_DS_DRA_BAD_DN)
+
+        self._test_repl_full(expected_error=werror.WERR_DS_DRA_ACCESS_DENIED)
+        self._test_repl_full_on_ou(expected_error=werror.WERR_DS_DRA_BAD_DN)
+
+        self._test_repl_single_obj(repl_obj=self.ou,
+                                   expected_error=werror.WERR_DS_DRA_BAD_DN,
+                                   partial_attribute_set=self.get_partial_attribute_set())
+        self._test_repl_full(expected_error=werror.WERR_DS_DRA_ACCESS_DENIED,
+                             partial_attribute_set=self.get_partial_attribute_set())
 
-        req8 = self._exop_req8(dest_dsa=None,
-                               invocation_id=self.ldb_dc1.get_invocation_id(),
-                               nc_dn_str=self.ldb_dc1.get_default_basedn(),
-                               exop=drsuapi.DRSUAPI_EXOP_NONE,
-                               replica_flags=drsuapi.DRSUAPI_DRS_WRIT_REP)
-        (level, ctr) = self.user_drs.DsGetNCChanges(self.user_drs_handle, 8, req8)
-        self.assertEqual(ctr.extended_ret, drsuapi.DRSUAPI_EXOP_ERR_NONE)
 
-- 
2.7.4


From 190522f5f04db9d27952405491995e7acaba043d Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 16 Aug 2017 15:09:55 +1200
Subject: [PATCH 09/14] s4-drsuapi: Change REPL_SECRET error code to match
 Windows

The existing SOURCE_DISABLED error code doesn't seem to make a lot of
sense. Window sends back an ACCESS_DENIED error in the same situation,
which seems more appropriate.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 source4/rpc_server/drsuapi/getncchanges.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index 705d068..1b39c17 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -1178,7 +1178,7 @@ static WERROR getncchanges_repl_secret(struct drsuapi_bind_state *b_state,
 	if (b_state->sam_ctx_system == NULL) {
 		/* this operation needs system level access */
 		ctr6->extended_ret = DRSUAPI_EXOP_ERR_ACCESS_DENIED;
-		return WERR_DS_DRA_SOURCE_DISABLED;
+		return WERR_DS_DRA_ACCESS_DENIED;
 	}
 
 	/*
-- 
2.7.4


From f05ed43ace3e7c0441e66e30749f3df056c1132f Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 14 Aug 2017 15:31:08 +1200
Subject: [PATCH 10/14] selftest: GetNCChanges can 'accept' a repeated bad
 request

In theory, if we send the exact same rejected request again, we should
get the same response back from the DC. However, we don't - the request
is accepted if we send it a second time.

This patch updates the repl_rodc test to demonstrate the problem (which
now causes the test to fail).

Note that although the bad GetNCChanges request is not rejected outright,
the response that gets sent back is empty - it has no objects in it, so
it's not an actual security hole. It is annoying problem for writing
self-tests though.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 selftest/knownfail.d/repl_rodc          | 3 +++
 source4/torture/drs/python/repl_rodc.py | 7 +++++++
 2 files changed, 10 insertions(+)
 create mode 100644 selftest/knownfail.d/repl_rodc

diff --git a/selftest/knownfail.d/repl_rodc b/selftest/knownfail.d/repl_rodc
new file mode 100644
index 0000000..0e8e534
--- /dev/null
+++ b/selftest/knownfail.d/repl_rodc
@@ -0,0 +1,3 @@
+samba4.drs.repl_rodc.python\(ad_dc_ntvfs\).repl_rodc.DrsRodcTestCase.test_rodc_repl_secrets\(ad_dc_ntvfs\)
+
+
diff --git a/source4/torture/drs/python/repl_rodc.py b/source4/torture/drs/python/repl_rodc.py
index 01c9c6d..ca3744c 100644
--- a/source4/torture/drs/python/repl_rodc.py
+++ b/source4/torture/drs/python/repl_rodc.py
@@ -202,6 +202,13 @@ class DrsRodcTestCase(drs_base.DrsBaseTestCase):
         except WERRORError as (enum, estr):
             self.assertEquals(enum, 8630) # ERROR_DS_DRA_SECRETS_DENIED
 
+        # send the same request again and we should get the same response
+        try:
+            (level, ctr) = self.rodc_drs.DsGetNCChanges(self.rodc_drs_handle, 10, req10)
+            self.fail("Successfully replicated secrets to an RODC that shouldn't have been replicated.")
+        except WERRORError as (enum, estr):
+            self.assertEquals(enum, 8630) # ERROR_DS_DRA_SECRETS_DENIED
+
         # Retry with Administrator credentials, ignores password replication groups
         (level, ctr) = self.drs.DsGetNCChanges(self.drs_handle, 10, req10)
 
-- 
2.7.4


From 897e1d3d0b674a94c7c4ab8f17c356df20d30a79 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 16 Aug 2017 16:20:37 +1200
Subject: [PATCH 11/14] s4-drsuapi: Set getnc_state *after* we've checked
 request is valid

We were creating the getnc_state (and storing it on the connection)
before we had done some basic checks that the request was valid. If the
request was not valid and we returned early with an error, then the
partially-initialized getnc_state was left hanging on the connection.
The next request that got sent on the connection would try to use this,
rather than creating a new getnc_state from scratch.

The main side-effect of this was if you sent an invalid GetNCChanges
request twice, then it could be rejected the first time and accepted the
second time.

Note that although an invalid request was accepted, it would typically
not return any objects, so it would not actually leak any secure
information.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 selftest/knownfail.d/repl_rodc            |  3 --
 source4/rpc_server/drsuapi/getncchanges.c | 55 +++++++++++++++++--------------
 2 files changed, 31 insertions(+), 27 deletions(-)
 delete mode 100644 selftest/knownfail.d/repl_rodc

diff --git a/selftest/knownfail.d/repl_rodc b/selftest/knownfail.d/repl_rodc
deleted file mode 100644
index 0e8e534..0000000
--- a/selftest/knownfail.d/repl_rodc
+++ /dev/null
@@ -1,3 +0,0 @@
-samba4.drs.repl_rodc.python\(ad_dc_ntvfs\).repl_rodc.DrsRodcTestCase.test_rodc_repl_secrets\(ad_dc_ntvfs\)
-
-
diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index 1b39c17..6646ccd 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -2221,8 +2221,8 @@ allowed:
 				 ldb_dn_get_linearized(new_dn),
 				 ldb_dn_get_linearized(getnc_state->ncRoot_dn),
 				 ldb_dn_get_linearized(getnc_state->last_dn)));
-			talloc_free(getnc_state);
-			getnc_state = NULL;
+			TALLOC_FREE(getnc_state);
+			b_state->getncchanges_state = NULL;
 		}
 	}
 
@@ -2235,8 +2235,8 @@ allowed:
 				 ldb_dn_get_linearized(getnc_state->ncRoot_dn),
 				 (ret > 0) ? "older" : "newer",
 				 ldb_dn_get_linearized(getnc_state->last_dn)));
-			talloc_free(getnc_state);
-			getnc_state = NULL;
+			TALLOC_FREE(getnc_state);
+			b_state->getncchanges_state = NULL;
 		}
 	}
 
@@ -2248,39 +2248,25 @@ allowed:
 			NULL
 		};
 		uint32_t nc_instanceType;
+		struct ldb_dn *ncRoot_dn;
 
-		getnc_state = talloc_zero(b_state, struct drsuapi_getncchanges_state);
-		if (getnc_state == NULL) {
-			return WERR_NOT_ENOUGH_MEMORY;
-		}
-		b_state->getncchanges_state = getnc_state;
-		getnc_state->ncRoot_dn = drs_ObjectIdentifier_to_dn(getnc_state, sam_ctx, ncRoot);
-		if (getnc_state->ncRoot_dn == NULL) {
+		ncRoot_dn = drs_ObjectIdentifier_to_dn(mem_ctx, sam_ctx, ncRoot);
+		if (ncRoot_dn == NULL) {
 			return WERR_NOT_ENOUGH_MEMORY;
 		}
 
 		ret = dsdb_search_dn(sam_ctx, mem_ctx, &res,
-				     getnc_state->ncRoot_dn, attrs,
+				     ncRoot_dn, attrs,
 				     DSDB_SEARCH_SHOW_DELETED |
 				     DSDB_SEARCH_SHOW_RECYCLED);
 		if (ret != LDB_SUCCESS) {
 			DBG_WARNING("Failed to find ncRoot_dn %s\n",
-				    ldb_dn_get_linearized(getnc_state->ncRoot_dn));
+				    ldb_dn_get_linearized(ncRoot_dn));
 			return WERR_DS_CANT_FIND_EXPECTED_NC;
 		}
-		getnc_state->ncRoot_guid = samdb_result_guid(res->msgs[0],
-							     "objectGUID");
 		nc_instanceType = ldb_msg_find_attr_as_int(res->msgs[0],
 							   "instanceType",
 							   0);
-		TALLOC_FREE(res);
-		ncRoot->guid = getnc_state->ncRoot_guid;
-
-		/* find out if we are to replicate Schema NC */
-		ret = ldb_dn_compare_base(ldb_get_schema_basedn(sam_ctx),
-					  getnc_state->ncRoot_dn);
-
-		getnc_state->is_schema_nc = (0 == ret);
 
 		if (req10->extended_op != DRSUAPI_EXOP_NONE) {
 			r->out.ctr->ctr6.extended_ret = DRSUAPI_EXOP_ERR_SUCCESS;
@@ -2296,7 +2282,7 @@ allowed:
 		case DRSUAPI_EXOP_NONE:
 			if ((nc_instanceType & INSTANCE_TYPE_IS_NC_HEAD) == 0) {
 				const char *dn_str
-					= ldb_dn_get_linearized(getnc_state->ncRoot_dn);
+					= ldb_dn_get_linearized(ncRoot_dn);
 
 				DBG_NOTICE("Rejecting full replication on "
 					   "not NC %s", dn_str);
@@ -2354,6 +2340,27 @@ allowed:
 				 (unsigned)req10->extended_op));
 			return WERR_DS_DRA_NOT_SUPPORTED;
 		}
+
+		/* Initialize the state we'll store over the replication cycle */
+		getnc_state = talloc_zero(b_state, struct drsuapi_getncchanges_state);
+		if (getnc_state == NULL) {
+			return WERR_NOT_ENOUGH_MEMORY;
+		}
+		b_state->getncchanges_state = getnc_state;
+
+		getnc_state->ncRoot_dn = ncRoot_dn;
+		talloc_steal(getnc_state, ncRoot_dn);
+
+		getnc_state->ncRoot_guid = samdb_result_guid(res->msgs[0],
+							     "objectGUID");
+		ncRoot->guid = getnc_state->ncRoot_guid;
+
+		/* find out if we are to replicate Schema NC */
+		ret = ldb_dn_compare_base(ldb_get_schema_basedn(sam_ctx),
+					  ncRoot_dn);
+		getnc_state->is_schema_nc = (0 == ret);
+
+		TALLOC_FREE(res);
 	}
 
 	if (!ldb_dn_validate(getnc_state->ncRoot_dn) ||
-- 
2.7.4


From 884137f471ac9a68a33919d6e933b08626668116 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 16 Aug 2017 15:00:31 +1200
Subject: [PATCH 12/14] selftest: Update getnc_unpriv tests to pass against
 Samba

In general Windows seems to return BAD_DN rather than ACCESS_DENIED for
an unprivileged user. In the the long-term, it's unrealistic to think
that Samba and Windows will agree exactly on every error code returned.
So for the tests to be maintainable and pass against Windows and Samba,
they need to handle differences in expected errors. To get around this
problem, I've changed the expected_error to be a set, so that multiple
error codes (one for Microsoft, one for Samba) can be specified for each
test case. This approach also highlights the cases where Microsoft and
Samba currently differ.

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 selftest/knownfail.d/getnc_unpriv          | 10 -------
 source4/torture/drs/python/getnc_unpriv.py | 48 +++++++++++++++++-------------
 2 files changed, 28 insertions(+), 30 deletions(-)
 delete mode 100644 selftest/knownfail.d/getnc_unpriv

diff --git a/selftest/knownfail.d/getnc_unpriv b/selftest/knownfail.d/getnc_unpriv
deleted file mode 100644
index 40977f9..0000000
--- a/selftest/knownfail.d/getnc_unpriv
+++ /dev/null
@@ -1,10 +0,0 @@
-samba4.drs.getnc_unpriv.python\(vampire_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_no_userpriv\(vampire_dc\)
-samba4.drs.getnc_unpriv.python\(vampire_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_getchanges_userpriv\(vampire_dc\)
-samba4.drs.getnc_unpriv.python\(vampire_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_getallchanges_userpriv\(vampire_dc\)
-samba4.drs.getnc_unpriv.python\(vampire_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_both_userpriv\(vampire_dc\)
-samba4.drs.getnc_unpriv.python\(vampire_dc\)\(vampire_dc\)
-samba4.drs.getnc_unpriv.python\(promoted_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_no_userpriv\(promoted_dc\)
-samba4.drs.getnc_unpriv.python\(promoted_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_getchanges_userpriv\(promoted_dc\)
-samba4.drs.getnc_unpriv.python\(promoted_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_getallchanges_userpriv\(promoted_dc\)
-samba4.drs.getnc_unpriv.python\(promoted_dc\).getnc_unpriv.DrsReplicaSyncUnprivTestCase.test_repl_both_userpriv\(promoted_dc\)
-samba4.drs.getnc_unpriv.python\(promoted_dc\)\(promoted_dc\)
diff --git a/source4/torture/drs/python/getnc_unpriv.py b/source4/torture/drs/python/getnc_unpriv.py
index 41d9611..5e4b681 100644
--- a/source4/torture/drs/python/getnc_unpriv.py
+++ b/source4/torture/drs/python/getnc_unpriv.py
@@ -111,8 +111,8 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
                                                             8, req8)
                 self.fail("Should have failed with user denied access")
             except WERRORError as (enum, estr):
-                self.assertEquals(enum, expected_error,
-                                  "Got unexpected error: %s" % estr)
+                self.assertTrue(enum in expected_error,
+                                "Got unexpected error: %s" % estr)
 
     def _test_repl_single_obj(self, repl_obj, expected_error,
                               partial_attribute_set=None):
@@ -165,18 +165,19 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
         self.sd_utils.dacl_add_ace(self.base_dn, self.acl_mod_get_changes)
 
         self._test_repl_single_obj(repl_obj=self.ou,
-                                   expected_error=werror.WERR_DS_DRA_ACCESS_DENIED)
+                                   expected_error=[werror.WERR_DS_DRA_ACCESS_DENIED])
 
         self._test_repl_secret(repl_obj=self.ou,
-                               expected_error=werror.WERR_DS_DRA_ACCESS_DENIED)
+                               expected_error=[werror.WERR_DS_DRA_ACCESS_DENIED])
         self._test_repl_secret(repl_obj=self.user_dn,
-                               expected_error=werror.WERR_DS_DRA_ACCESS_DENIED)
+                               expected_error=[werror.WERR_DS_DRA_ACCESS_DENIED])
         self._test_repl_secret(repl_obj=self.user_dn,
                                dest_dsa=self.ldb_dc1.get_ntds_GUID(),
-                               expected_error=werror.WERR_DS_DRA_ACCESS_DENIED)
+                               expected_error=[werror.WERR_DS_DRA_ACCESS_DENIED])
 
-        self._test_repl_full(expected_error=werror.WERR_DS_DRA_ACCESS_DENIED)
-        self._test_repl_full_on_ou(expected_error=werror.WERR_DS_CANT_FIND_EXPECTED_NC)
+        self._test_repl_full(expected_error=[werror.WERR_DS_DRA_ACCESS_DENIED])
+        self._test_repl_full_on_ou(expected_error=[werror.WERR_DS_CANT_FIND_EXPECTED_NC,
+                                                   werror.WERR_DS_DRA_ACCESS_DENIED])
 
         # Partial Attribute Sets don't require GET_ALL_CHANGES rights, so we
         # expect the following to succeed
@@ -215,16 +216,20 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
         self._test_repl_single_obj(repl_obj=self.ou,
                                    expected_error=None)
 
+        # Microsoft returns DB_ERROR, Samba returns ACCESS_DENIED        
         self._test_repl_secret(repl_obj=self.ou,
-                               expected_error=werror.WERR_DS_DRA_DB_ERROR)
+                               expected_error=[werror.WERR_DS_DRA_DB_ERROR,
+                                               werror.WERR_DS_DRA_ACCESS_DENIED])
         self._test_repl_secret(repl_obj=self.user_dn,
-                               expected_error=werror.WERR_DS_DRA_DB_ERROR)
+                               expected_error=[werror.WERR_DS_DRA_DB_ERROR,
+                                               werror.WERR_DS_DRA_ACCESS_DENIED])
+        # Note that Windows accepts this but Samba rejects it
         self._test_repl_secret(repl_obj=self.user_dn,
                                dest_dsa=self.ldb_dc1.get_ntds_GUID(),
-                               expected_error=None)
+                               expected_error=[werror.WERR_DS_DRA_ACCESS_DENIED])
 
         self._test_repl_full(expected_error=None)
-        self._test_repl_full_on_ou(expected_error=werror.WERR_DS_CANT_FIND_EXPECTED_NC)
+        self._test_repl_full_on_ou(expected_error=[werror.WERR_DS_CANT_FIND_EXPECTED_NC])
 
         self._test_repl_single_obj(repl_obj=self.ou,
                                    expected_error=None,
@@ -238,24 +243,27 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
         We expect all these requests to be rejected.
         """
 
+        # Microsoft usually returns BAD_DN, Samba returns ACCESS_DENIED
+        usual_error = [werror.WERR_DS_DRA_BAD_DN, werror.WERR_DS_DRA_ACCESS_DENIED]
+
         self._test_repl_single_obj(repl_obj=self.ou,
-                                   expected_error=werror.WERR_DS_DRA_BAD_DN)
+                                   expected_error=usual_error)
 
         self._test_repl_secret(repl_obj=self.ou,
-                               expected_error=werror.WERR_DS_DRA_BAD_DN)
+                               expected_error=usual_error)
         self._test_repl_secret(repl_obj=self.user_dn,
-                               expected_error=werror.WERR_DS_DRA_BAD_DN)
+                               expected_error=usual_error)
         self._test_repl_secret(repl_obj=self.user_dn,
                                dest_dsa=self.ldb_dc1.get_ntds_GUID(),
-                               expected_error=werror.WERR_DS_DRA_BAD_DN)
+                               expected_error=usual_error)
 
-        self._test_repl_full(expected_error=werror.WERR_DS_DRA_ACCESS_DENIED)
-        self._test_repl_full_on_ou(expected_error=werror.WERR_DS_DRA_BAD_DN)
+        self._test_repl_full(expected_error=[werror.WERR_DS_DRA_ACCESS_DENIED])
+        self._test_repl_full_on_ou(expected_error=usual_error)
 
         self._test_repl_single_obj(repl_obj=self.ou,
-                                   expected_error=werror.WERR_DS_DRA_BAD_DN,
+                                   expected_error=usual_error,
                                    partial_attribute_set=self.get_partial_attribute_set())
-        self._test_repl_full(expected_error=werror.WERR_DS_DRA_ACCESS_DENIED,
+        self._test_repl_full(expected_error=[werror.WERR_DS_DRA_ACCESS_DENIED],
                              partial_attribute_set=self.get_partial_attribute_set())
 
 
-- 
2.7.4


From 4ad09e7462e81b9c072678aa9fac7a06ceab63b7 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 17 Aug 2017 11:36:24 +1200
Subject: [PATCH 13/14] s4-drsuapi/selftest: Add extra tests for invalid DNs

Add some test cases to check for requests for invalid/non-existent DNs.
This exercises the first return case added in commit:
  s4-drsuapi: Refuse to replicate an NC is that not actually an NC

I've also updated the error code returned here to match Windows.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 source4/rpc_server/drsuapi/getncchanges.c  |  2 +-
 source4/torture/drs/python/getnc_unpriv.py | 39 ++++++++++++++++++++++++++----
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index 6646ccd..afed782 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -2262,7 +2262,7 @@ allowed:
 		if (ret != LDB_SUCCESS) {
 			DBG_WARNING("Failed to find ncRoot_dn %s\n",
 				    ldb_dn_get_linearized(ncRoot_dn));
-			return WERR_DS_CANT_FIND_EXPECTED_NC;
+			return WERR_DS_DRA_BAD_DN;
 		}
 		nc_instanceType = ldb_msg_find_attr_as_int(res->msgs[0],
 							   "instanceType",
diff --git a/source4/torture/drs/python/getnc_unpriv.py b/source4/torture/drs/python/getnc_unpriv.py
index 5e4b681..5914072 100644
--- a/source4/torture/drs/python/getnc_unpriv.py
+++ b/source4/torture/drs/python/getnc_unpriv.py
@@ -145,14 +145,14 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
                              expected_error=expected_error,
                              partial_attribute_set=partial_attribute_set)
 
-    def _test_repl_full_on_ou(self, expected_error):
+    def _test_repl_full_on_ou(self, repl_obj, expected_error):
         """
         Full replication on a specific OU should always fail (it should be done
         against a base NC). The error may vary based on the user's access rights
         """
         # Just try against the OU created in the test setup
         self._test_repl_exop(exop=drsuapi.DRSUAPI_EXOP_NONE,
-                             repl_obj=self.ou,
+                             repl_obj=repl_obj,
                              expected_error=expected_error)
 
     def test_repl_getchanges_userpriv(self):
@@ -166,6 +166,10 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
 
         self._test_repl_single_obj(repl_obj=self.ou,
                                    expected_error=[werror.WERR_DS_DRA_ACCESS_DENIED])
+        bad_ou = "OU=bad_obj,%s" % self.ou
+        self._test_repl_single_obj(repl_obj=bad_ou,
+                                   expected_error=[werror.WERR_DS_DRA_BAD_DN,
+                                                   werror.WERR_DS_DRA_ACCESS_DENIED])
 
         self._test_repl_secret(repl_obj=self.ou,
                                expected_error=[werror.WERR_DS_DRA_ACCESS_DENIED])
@@ -174,9 +178,15 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
         self._test_repl_secret(repl_obj=self.user_dn,
                                dest_dsa=self.ldb_dc1.get_ntds_GUID(),
                                expected_error=[werror.WERR_DS_DRA_ACCESS_DENIED])
+        self._test_repl_secret(repl_obj=bad_ou,
+                               expected_error=[werror.WERR_DS_DRA_BAD_DN])
 
         self._test_repl_full(expected_error=[werror.WERR_DS_DRA_ACCESS_DENIED])
-        self._test_repl_full_on_ou(expected_error=[werror.WERR_DS_CANT_FIND_EXPECTED_NC,
+        self._test_repl_full_on_ou(repl_obj=self.ou,
+                                   expected_error=[werror.WERR_DS_CANT_FIND_EXPECTED_NC,
+                                                   werror.WERR_DS_DRA_ACCESS_DENIED])
+        self._test_repl_full_on_ou(repl_obj=bad_ou,
+                                   expected_error=[werror.WERR_DS_DRA_BAD_NC,
                                                    werror.WERR_DS_DRA_ACCESS_DENIED])
 
         # Partial Attribute Sets don't require GET_ALL_CHANGES rights, so we
@@ -215,6 +225,9 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
 
         self._test_repl_single_obj(repl_obj=self.ou,
                                    expected_error=None)
+        bad_ou = "OU=bad_obj,%s" % self.ou
+        self._test_repl_single_obj(repl_obj=bad_ou,
+                                   expected_error=[werror.WERR_DS_DRA_BAD_DN])
 
         # Microsoft returns DB_ERROR, Samba returns ACCESS_DENIED        
         self._test_repl_secret(repl_obj=self.ou,
@@ -228,8 +241,15 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
                                dest_dsa=self.ldb_dc1.get_ntds_GUID(),
                                expected_error=[werror.WERR_DS_DRA_ACCESS_DENIED])
 
+        self._test_repl_secret(repl_obj=bad_ou,
+                               expected_error=[werror.WERR_DS_DRA_BAD_DN])
+
         self._test_repl_full(expected_error=None)
-        self._test_repl_full_on_ou(expected_error=[werror.WERR_DS_CANT_FIND_EXPECTED_NC])
+        self._test_repl_full_on_ou(repl_obj=self.ou,
+                                   expected_error=[werror.WERR_DS_CANT_FIND_EXPECTED_NC])
+        self._test_repl_full_on_ou(repl_obj=bad_ou,
+                                   expected_error=[werror.WERR_DS_DRA_BAD_NC,
+                                                   werror.WERR_DS_DRA_BAD_DN])
 
         self._test_repl_single_obj(repl_obj=self.ou,
                                    expected_error=None,
@@ -248,6 +268,9 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
 
         self._test_repl_single_obj(repl_obj=self.ou,
                                    expected_error=usual_error)
+        bad_ou = "OU=bad_obj,%s" % self.ou
+        self._test_repl_single_obj(repl_obj=bad_ou,
+                                   expected_error=usual_error)
 
         self._test_repl_secret(repl_obj=self.ou,
                                expected_error=usual_error)
@@ -256,9 +279,15 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
         self._test_repl_secret(repl_obj=self.user_dn,
                                dest_dsa=self.ldb_dc1.get_ntds_GUID(),
                                expected_error=usual_error)
+        self._test_repl_secret(repl_obj=bad_ou,
+                               expected_error=usual_error)
 
         self._test_repl_full(expected_error=[werror.WERR_DS_DRA_ACCESS_DENIED])
-        self._test_repl_full_on_ou(expected_error=usual_error)
+        self._test_repl_full_on_ou(repl_obj=self.ou,
+                                   expected_error=usual_error)
+        self._test_repl_full_on_ou(repl_obj=bad_ou,
+                                   expected_error=[werror.WERR_DS_DRA_BAD_NC,
+                                                   werror.WERR_DS_DRA_ACCESS_DENIED])
 
         self._test_repl_single_obj(repl_obj=self.ou,
                                    expected_error=usual_error,
-- 
2.7.4


From 11f795ee2e6b4bf70491626568a17a413352d875 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 17 Aug 2017 12:30:30 +1200
Subject: [PATCH 14/14] selftest: Use a unique(ish) OU for every run of
 getnc_unpriv

An intermittent problem I noticed with tests in the past is that the
setup can fail to create the base OU because it already exists.
I believe this is because the previous testenv DC has replicated out the
test object, but not its deletion at the point that the next testenv DC
starts running the test.

This only seemed to happen very occassionally (I haven't seen it
happen with getnc_unpriv yet, but I also haven't run it through the
autobuild yet).

Using same randomness in the test OU should help avoid this sort of
problem, and it matches what some other replication tests do.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 source4/torture/drs/python/getnc_unpriv.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/source4/torture/drs/python/getnc_unpriv.py b/source4/torture/drs/python/getnc_unpriv.py
index 5914072..edf0f80 100644
--- a/source4/torture/drs/python/getnc_unpriv.py
+++ b/source4/torture/drs/python/getnc_unpriv.py
@@ -40,6 +40,7 @@ from samba import werror, WERRORError
 from samba import sd_utils
 import ldb
 from ldb import SCOPE_BASE
+import random
 
 from samba.dcerpc import drsuapi, security
 from samba.credentials import DONT_USE_KERBEROS
@@ -51,13 +52,19 @@ class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
         super(DrsReplicaSyncUnprivTestCase, self).setUp()
         self.get_changes_user = "get-changes-user"
         self.base_dn = self.ldb_dc1.get_default_basedn()
-        self.ou = "OU=test_getncchanges,%s" % self.base_dn
         self.user_pass = samba.generate_random_password(12, 16)
+
+        # add some randomness to the test OU. (Deletion of the last test's
+        # objects can be slow to replicate out. So the OU created by a previous
+        # testenv may still exist at this point).
+        rand = random.randint(1, 10000000)
+        test_ou = "OU=test_getnc_unpriv%d" %rand
+        self.ou = "%s,%s" %(test_ou, self.base_dn)
         self.ldb_dc1.add({
             "dn": self.ou,
             "objectclass": "organizationalUnit"})
         self.ldb_dc1.newuser(self.get_changes_user, self.user_pass,
-                             userou="OU=test_getncchanges")
+                             userou=test_ou)
         (self.drs, self.drs_handle) = self._ds_bind(self.dnsname_dc1)
 
         self.sd_utils = sd_utils.SDUtils(self.ldb_dc1)
-- 
2.7.4



More information about the samba-technical mailing list