[PATCH] s3:winbindd: Do not use domain SID from LookupSids for idmap
Christof Schmitt
cs at samba.org
Wed Sep 24 17:06:27 MDT 2014
Hi Volker,
have you had a chance to look at this patch? This affects Samba 4.0, 4.1
and the current master, so we should get a fix in place. I also opened
https://bugzilla.samba.org/show_bug.cgi?id=10838 to backport the fix to
4.0 and 4.1 once we have a patch for master.
Christof
On Fri, Sep 12, 2014 at 11:47:16AM -0700, Christof Schmitt wrote:
> 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
> 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