[PATCH] s3:winbindd: Do not use domain SID from LookupSids for idmap
Christof Schmitt
cs at samba.org
Fri Sep 12 12:47:16 MDT 2014
On Mon, Sep 08, 2014 at 10:21:14PM +0200, Volker Lendecke wrote:
> On Thu, Sep 04, 2014 at 01:25:43PM -0700, Christof Schmitt wrote:
> > The attached patch fixes a proble where invalid id mappings have been
> > used for objects that have been migrated from another domain. LookupSids
> > returns the SID of the new domain where the migrated object resides, and
> > the problem is that this domain SID cannot be combined with the original
> > RID. The RID likely has changed during the migration.
>
> Can you give examples? I think I don't fully understand what
> is going on here.
One example i have seen is a group that has been migrated from a domain
to a different one. The membership in this group is established through
a reference of the previous SID (S-DOM1-RID1). The codepath discussed
here issues a LookupSids, and that returns the SID of the new domain
(S-DOM2). The mistake in the code is combining the new domain SID with
the previous RID to (S-DOM2-RID1).
The easiest fix i can see is just using the old SID, as far as i can see
the code in 3.6 did the same.
> > + struct dom_sid *orig_sid = &state->non_cached[i];
> > + struct lsa_DomainInfo *lsa_dom =
> > + &state->domains->domains[n->sid_index];
> > +
> > + if (!dom_sid_in_domain(lsa_dom->sid, orig_sid)) {
>
> One question regarding this if-statement: Do we need it at
> all functionally? Performance-wise couldn't we play tricks
> to copy orig_sid into lsa_dom->sid?
Based on our discussion, i changed the patches to build a new
lsaRefDomains array (see attachment). We don't know whether users from
different domains are in the same idmap query and my first approach
could replace the wrong SIDs.
I think the better approach would be to query the RIDs of the users
(e.g. with LookupNames?), but that would be a larger change and i would
like to fix the issue with the wrong SID quickly.
Regards,
Christof
-------------- next part --------------
From f689bea316ab5bb16dbdea0eac4053a6dd1a6124 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Thu, 11 Sep 2014 16:11:06 -0700
Subject: [PATCH 1/2] s3: Move init_lsa_ref_domain_list to lib
This will be used in the next patch in winbind.
Signed-off-by: Christof Schmitt <cs at samba.org>
---
source3/include/lsa.h | 25 +++++++++++++
source3/lib/lsa.c | 67 +++++++++++++++++++++++++++++++++++
source3/rpc_server/lsa/srv_lsa_nt.c | 48 +------------------------
source3/rpc_server/wscript_build | 2 +-
source3/wscript_build | 4 ++
5 files changed, 98 insertions(+), 48 deletions(-)
create mode 100644 source3/include/lsa.h
create mode 100644 source3/lib/lsa.c
diff --git a/source3/include/lsa.h b/source3/include/lsa.h
new file mode 100644
index 0000000..7681aed
--- /dev/null
+++ b/source3/include/lsa.h
@@ -0,0 +1,25 @@
+/*
+ * Helper functions related to the LSA server
+ *
+ * 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/>.
+ */
+#ifndef LSA_H
+#define LSA_H
+
+int init_lsa_ref_domain_list(TALLOC_CTX *mem_ctx,
+ struct lsa_RefDomainList *ref,
+ const char *dom_name,
+ struct dom_sid *dom_sid);
+
+#endif
diff --git a/source3/lib/lsa.c b/source3/lib/lsa.c
new file mode 100644
index 0000000..0046fda
--- /dev/null
+++ b/source3/lib/lsa.c
@@ -0,0 +1,67 @@
+/*
+ * Helper functions related to the LSA server
+ *
+ * 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/>.
+ */
+
+/***************************************************************************
+ init_lsa_ref_domain_list - adds a domain if it's not already in, returns index.
+***************************************************************************/
+
+#include "includes.h"
+#include "libcli/security/dom_sid.h"
+#include "librpc/gen_ndr/lsa.h"
+#include "lsa.h"
+
+int init_lsa_ref_domain_list(TALLOC_CTX *mem_ctx,
+ struct lsa_RefDomainList *ref,
+ const char *dom_name,
+ struct dom_sid *dom_sid)
+{
+ int num = 0;
+
+ if (dom_name != NULL) {
+ for (num = 0; num < ref->count; num++) {
+ if (dom_sid_equal(dom_sid, ref->domains[num].sid)) {
+ return num;
+ }
+ }
+ } else {
+ num = ref->count;
+ }
+
+ if (num >= LSA_REF_DOMAIN_LIST_MULTIPLIER) {
+ /* index not found, already at maximum domain limit */
+ return -1;
+ }
+
+ ref->count = num + 1;
+ ref->max_size = LSA_REF_DOMAIN_LIST_MULTIPLIER;
+
+ ref->domains = talloc_realloc(mem_ctx, ref->domains,
+ struct lsa_DomainInfo, ref->count);
+ if (!ref->domains) {
+ return -1;
+ }
+
+ ZERO_STRUCT(ref->domains[num]);
+
+ ref->domains[num].name.string = dom_name;
+ ref->domains[num].sid = dom_sid_dup(mem_ctx, dom_sid);
+ if (!ref->domains[num].sid) {
+ return -1;
+ }
+
+ return num;
+}
diff --git a/source3/rpc_server/lsa/srv_lsa_nt.c b/source3/rpc_server/lsa/srv_lsa_nt.c
index 68a2a2c..67909aa 100644
--- a/source3/rpc_server/lsa/srv_lsa_nt.c
+++ b/source3/rpc_server/lsa/srv_lsa_nt.c
@@ -49,6 +49,7 @@
#include "../librpc/gen_ndr/ndr_wkssvc.h"
#include "../libcli/auth/libcli_auth.h"
#include "../libcli/lsarpc/util_lsarpc.h"
+#include "lsa.h"
#undef DBGC_CLASS
#define DBGC_CLASS DBGC_RPC_SRV
@@ -98,53 +99,6 @@ const struct generic_mapping lsa_trusted_domain_mapping = {
};
/***************************************************************************
- init_lsa_ref_domain_list - adds a domain if it's not already in, returns the index.
-***************************************************************************/
-
-static int init_lsa_ref_domain_list(TALLOC_CTX *mem_ctx,
- struct lsa_RefDomainList *ref,
- const char *dom_name,
- struct dom_sid *dom_sid)
-{
- int num = 0;
-
- if (dom_name != NULL) {
- for (num = 0; num < ref->count; num++) {
- if (dom_sid_equal(dom_sid, ref->domains[num].sid)) {
- return num;
- }
- }
- } else {
- num = ref->count;
- }
-
- if (num >= LSA_REF_DOMAIN_LIST_MULTIPLIER) {
- /* index not found, already at maximum domain limit */
- return -1;
- }
-
- ref->count = num + 1;
- ref->max_size = LSA_REF_DOMAIN_LIST_MULTIPLIER;
-
- ref->domains = talloc_realloc(mem_ctx, ref->domains,
- struct lsa_DomainInfo, ref->count);
- if (!ref->domains) {
- return -1;
- }
-
- ZERO_STRUCT(ref->domains[num]);
-
- init_lsa_StringLarge(&ref->domains[num].name, dom_name);
- ref->domains[num].sid = dom_sid_dup(mem_ctx, dom_sid);
- if (!ref->domains[num].sid) {
- return -1;
- }
-
- return num;
-}
-
-
-/***************************************************************************
initialize a lsa_DomainInfo structure.
***************************************************************************/
diff --git a/source3/rpc_server/wscript_build b/source3/rpc_server/wscript_build
index a058186..481d39c 100755
--- a/source3/rpc_server/wscript_build
+++ b/source3/rpc_server/wscript_build
@@ -64,7 +64,7 @@ bld.SAMBA3_SUBSYSTEM('RPC_INITSHUTDOWN',
bld.SAMBA3_SUBSYSTEM('RPC_LSARPC',
source='''lsa/srv_lsa_nt.c
../../librpc/gen_ndr/srv_lsa.c''',
- deps='SRV_ACCESS_CHECK')
+ deps='SRV_ACCESS_CHECK LIBLSA')
bld.SAMBA3_SUBSYSTEM('RPC_NETDFS',
source='''dfs/srv_dfs_nt.c
diff --git a/source3/wscript_build b/source3/wscript_build
index 740ab76..52a3ec2 100755
--- a/source3/wscript_build
+++ b/source3/wscript_build
@@ -827,6 +827,9 @@ bld.SAMBA3_SUBSYSTEM('INIT_SAMR',
source='rpc_client/init_samr.c',
deps='samba-util')
+bld.SAMBA3_SUBSYSTEM('LIBLSA',
+ source='lib/lsa.c')
+
########################## BINARIES #################################
bld.SAMBA3_BINARY('smbd/smbd',
@@ -971,6 +974,7 @@ bld.SAMBA3_BINARY('winbindd/winbindd',
WB_REQTRANS
TDB_VALIDATE
MESSAGING
+ LIBLSA
''',
enabled=bld.env.build_winbind,
install_path='${SBINDIR}')
--
1.7.1
From 45c8ab148824a6d8709c77aa92d9e3c2f0655205 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Thu, 11 Sep 2014 16:39:21 -0700
Subject: [PATCH 2/2] s3-winbindd: Do not use domain SID from LookupSids for Sids2UnixIDs call
Create a new lsa_RefDomainList and populate it with the domain SID from
the original query. That avoids the problem that for migrated objects,
LookupSids returns the SID of the new domain, and combining that with
the RID from the input results in an invalid SID.
A better fix would be querying the RID of the user in the new domain,
but the approach here at least avoids id mappings entries for invalid
SIDs.
Signed-off-by: Christof Schmitt <cs at samba.org>
---
source3/winbindd/wb_sids2xids.c | 33 ++++++++++++++++++++++++++++++---
1 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/source3/winbindd/wb_sids2xids.c b/source3/winbindd/wb_sids2xids.c
index 519a710..3e6f235 100644
--- a/source3/winbindd/wb_sids2xids.c
+++ b/source3/winbindd/wb_sids2xids.c
@@ -23,6 +23,7 @@
#include "../libcli/security/security.h"
#include "idmap_cache.h"
#include "librpc/gen_ndr/ndr_winbind_c.h"
+#include "lsa.h"
struct wb_sids2xids_state {
struct tevent_context *ev;
@@ -38,6 +39,19 @@ struct wb_sids2xids_state {
struct lsa_RefDomainList *domains;
struct lsa_TransNameArray *names;
+ /*
+ * Domain array to use for the idmap call. The output from
+ * lookupsids cannot be used directly since for migrated
+ * objects the returned domain SID can be different that the
+ * original one. The new domain SID cannot be combined with
+ * the RID from the previous domain.
+ *
+ * The proper way would be asking for the correct RID in the
+ * new domain, but this approach avoids id mappings for
+ * invalid SIDs.
+ */
+ struct lsa_RefDomainList *idmap_doms;
+
struct wbint_TransIDArray ids;
};
@@ -162,13 +176,26 @@ static void wb_sids2xids_lookupsids_done(struct tevent_req *subreq)
return;
}
+ state->idmap_doms = talloc_zero(state, struct lsa_RefDomainList);
+ if (tevent_req_nomem(state->idmap_doms, req)) {
+ return;
+ }
+
for (i=0; i<state->num_non_cached; i++) {
+ struct dom_sid dom_sid;
+ struct lsa_DomainInfo *info;
struct lsa_TranslatedName *n = &state->names->names[i];
struct wbint_TransID *t = &state->ids.ids[i];
+ sid_copy(&dom_sid, &state->non_cached[i]);
+ sid_split_rid(&dom_sid, &t->rid);
+
+ info = &state->domains->domains[n->sid_index];
t->type = lsa_SidType_to_id_type(n->sid_type);
- t->domain_index = n->sid_index;
- sid_peek_rid(&state->non_cached[i], &t->rid);
+ t->domain_index = init_lsa_ref_domain_list(state,
+ state->idmap_doms,
+ info->name.string,
+ &dom_sid);
t->xid.id = UINT32_MAX;
t->xid.type = t->type;
}
@@ -176,7 +203,7 @@ static void wb_sids2xids_lookupsids_done(struct tevent_req *subreq)
child = idmap_child();
subreq = dcerpc_wbint_Sids2UnixIDs_send(
- state, state->ev, child->binding_handle, state->domains,
+ state, state->ev, child->binding_handle, state->idmap_doms,
&state->ids);
if (tevent_req_nomem(subreq, req)) {
return;
--
1.7.1
More information about the samba-technical
mailing list