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:46:22 UTC 2017
Hi,
Sorry for the delay. I just realized that the bug-fix portion of this
work hasn't been delivered. Could we get that change (attached) into
master and 4.7? I'll send the rest of the patches in a follow-up email
shortly.
Thanks,
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 525ba70c6db9d565d8433d8f76b661517256e887 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Fri, 4 Aug 2017 11:44:19 +1200
Subject: [PATCH] s4-drsuapi: Avoid segfault when replicating as a non-admin
with GUID_DRS_GET_CHANGES
Users who are not administrator do not get b_state->sam_ctx_system filled in.
We should probably use the 'sam_ctx' variable in all cases (instead of
b_state->sam_ctx*), but I'll make this change in a separate patch, so
that the bug fix remains independent from other tidy-ups.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12946
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 | 2 +-
source4/selftest/tests.py | 5 ++
source4/torture/drs/python/getnc_unpriv.py | 117 +++++++++++++++++++++++++++++
3 files changed, 123 insertions(+), 1 deletion(-)
create mode 100644 source4/torture/drs/python/getnc_unpriv.py
diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index 096162d..b98f14c 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -2250,7 +2250,7 @@ allowed:
return WERR_NOT_ENOUGH_MEMORY;
}
- ret = dsdb_find_guid_by_dn(b_state->sam_ctx_system,
+ ret = dsdb_find_guid_by_dn(b_state->sam_ctx,
getnc_state->ncRoot_dn,
&getnc_state->ncRoot_guid);
if (ret != LDB_SUCCESS) {
diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py
index 2152573..8aeba34 100755
--- a/source4/selftest/tests.py
+++ b/source4/selftest/tests.py
@@ -853,6 +853,11 @@ for env in ['vampire_dc', 'promoted_dc']:
name="samba4.drs.getnc_exop.python(%s)" % env,
environ={'DC1': "$DC_SERVER", 'DC2': '$%s_SERVER' % env.upper()},
extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD'])
+ planoldpythontestsuite(env, "getnc_unpriv",
+ extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')],
+ name="samba4.drs.getnc_unpriv.python(%s)" % env,
+ environ={'DC1': "$DC_SERVER", 'DC2': '$%s_SERVER' % env.upper()},
+ extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD'])
planoldpythontestsuite(env, "linked_attributes_drs",
extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')],
name="samba4.drs.linked_attributes_drs.python(%s)" % env,
diff --git a/source4/torture/drs/python/getnc_unpriv.py b/source4/torture/drs/python/getnc_unpriv.py
new file mode 100644
index 0000000..b1725f4
--- /dev/null
+++ b/source4/torture/drs/python/getnc_unpriv.py
@@ -0,0 +1,117 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+#
+# Tests replication scenarios with different user privileges
+#
+# Copyright (C) Kamen Mazdrashki <kamenim at samba.org> 2011
+# Copyright (C) Andrew Bartlett <abartlet at samba.org> 2017
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+#
+# Usage:
+# export DC1=dc1_dns_name
+# export DC2=dc2_dns_name
+# export SUBUNITRUN=$samba4srcdir/scripting/bin/subunitrun
+# PYTHONPATH="$PYTHONPATH:$samba4srcdir/torture/drs/python" $SUBUNITRUN getnc_unpriv -U"$DOMAIN/$DC_USERNAME"%"$DC_PASSWORD"
+#
+
+import drs_base
+import samba.tests
+
+from samba import sd_utils
+import ldb
+from ldb import SCOPE_BASE
+
+from samba.dcerpc import drsuapi
+from samba.credentials import DONT_USE_KERBEROS
+
+class DrsReplicaSyncUnprivTestCase(drs_base.DrsBaseTestCase):
+ """Confirm the behaviour of DsGetNCChanges for unprivileged users"""
+
+ def setUp(self):
+ 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)
+ self.ldb_dc1.add({
+ "dn": self.ou,
+ "objectclass": "organizationalUnit"})
+ self.ldb_dc1.newuser(self.get_changes_user, self.user_pass,
+ userou="OU=test_getncchanges")
+ (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)
+
+ # We set DONT_USE_KERBEROS to avoid a race with getting the
+ # user replicated to our selected KDC
+ self.user_creds = self.insta_creds(template=self.get_credentials(),
+ username=self.get_changes_user,
+ userpass=self.user_pass,
+ kerberos_state=DONT_USE_KERBEROS)
+ (self.user_drs, self.user_drs_handle) = self._ds_bind(self.dnsname_dc1,
+ self.user_creds)
+
+ def tearDown(self):
+ try:
+ self.ldb_dc1.delete(self.ou, ["tree_delete:1"])
+ except ldb.LdbError as (enum, string):
+ if enum == ldb.ERR_NO_SUCH_OBJECT:
+ pass
+ super(DrsReplicaSyncUnprivTestCase, self).tearDown()
+
+ def test_do_single_repl(self):
+ """
+ Make sure that DRSU_EXOP_REPL_OBJ works as a less-privileged
+ 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,
+ 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])
+
+ def test_do_full_repl(self):
+ """
+ Make sure that full replication works as a less-privileged
+ 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,
+ 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
More information about the samba-technical
mailing list