[PATCHES] winbindd: fix sid->xid for SID History SIDs
Ralph Böhme
slow at samba.org
Tue Apr 18 21:15:21 UTC 2017
On Wed, Apr 12, 2017 at 11:46:19AM +0300, Uri Simchoni wrote:
> On 04/10/2017 08:24 PM, Ralph Böhme wrote:
> > On Fri, Apr 07, 2017 at 09:23:43PM +0300, Uri Simchoni wrote:
> >> On 04/07/2017 08:32 PM, Ralph Böhme wrote:
> >>> On Mon, Apr 03, 2017 at 07:50:08AM +0300, Uri Simchoni via samba-technical wrote:
> >>>> Hi,
> >>>>
> >>>> Here's a V2 of the patch set.
> >>>
> >>> just fyi, I have this on my list.
> >>>
> >>> -slow
> >>>
> >> Thanks.
> >> Here's a rebased patch set, with 4/6 adapted to the recent change in
> >> wb_lookupsids_bulk(). Otherwise the same.
> >
> > looks like this still has the O(N^2) performance issue I mentioned or did I miss
> > anything?
> >
> > Besides that, attached is a simple fix for sids2xids on-top of my proposed
> > cleanup for wb_lookupsids [1] that should already address the SID-history if the
> > domain is still around.
> >
> > Your patch could go on-top as a more complete patch for the case that the
> > "historic domain" is behind a one-way trust (where we don't have the domain in
> > our list of trusted domains).
> >
> > Thoughts?
> >
> > -slow
> >
> > [1] <https://lists.samba.org/archive/samba-technical/2017-April/119944.html>
> >
>
> Pushed the simple fix, I think for backports we can use just "no
> match->default domain", or, if we want the bugfix to be based on
> backported patches, only backport [1/6] of the recent simplification
> patch set and the "don_sid_in_donain" fix.
backport for 4.5 and 4.6 attached. Can you test it?
> Regarding my patch and O(N^2) - the search is O(NxM) where M is the
> number of added domain sids in the lookup. Notice that the domain sids
> are added, unconditionally, to the back of the resolving list, and the
> subsequent search is over the added stuff only.
>
> Please indicate if you'd like me to rebase this patch set on top of
> recent commits or drop this - as far as I'm concerned, the main
> objective of not polluting the id-mapping cache is achieved, and
> anything else is just trying to get it right for more cases.
More correct is always good, but it comes at a cost: it makes the code slightly
more complicated. So given the prospect of moving the idmap backend decision to
be based on SIDs instead of domain names, which will also solve the more exotic
SID-history use-cases, maybe it makes sense to stop here.
-slow
-------------- next part --------------
From e4b57ecab1dd622940e75e9d602bf0bc71270d9f Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 4 Apr 2017 14:51:09 +0200
Subject: [PATCH 1/2] winbindd: handling of SIDs without domain reference in
wb_sids2xids_lookupsids_done()
This lets wb_sids2xids_lookupsids_done() deal with wp_lookupsids
returning UINT32_MAX as domain index for SIDs from unknown domains.
Call find_domain_from_sid_noinit() to search our list of known
domains. If a matching domain is found, use it's name, otherwise use the
empty string "". This needed to handle Samba DCs which always returns
sid_index UINT32_MAX for unknown SIDs, even from known domains.
Currently the wb_lookupsids adds these fake domains with an empty string
as domain name, but that's not the correct place to do it. We need the
domain name as it gets passed to the idmap child where the choise of
idmap backend is based on the domain name. This will possibly be changed
in the future to be based on domain SIDs, not the name.
Prerequisite for bug: https://bugzilla.samba.org/show_bug.cgi?id=12702
Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Ralph Boehme <slow at samba.org>
Reviewed-by: Jeremy Allison <jra at samba.org>
(cherry picked from commit 1efaeb072e55735421191fbae9cc586db6d07bb1)
---
source3/winbindd/wb_sids2xids.c | 33 +++++++++++++++++++++++++++------
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/source3/winbindd/wb_sids2xids.c b/source3/winbindd/wb_sids2xids.c
index 9bb8fa8..dc90bdf 100644
--- a/source3/winbindd/wb_sids2xids.c
+++ b/source3/winbindd/wb_sids2xids.c
@@ -185,20 +185,41 @@ static void wb_sids2xids_lookupsids_done(struct tevent_req *subreq)
}
for (i=0; i<state->num_non_cached; i++) {
+ const struct dom_sid *sid = &state->non_cached[i];
struct dom_sid dom_sid;
- struct lsa_DomainInfo *info;
struct lsa_TranslatedName *n = &names->names[i];
struct wbint_TransID *t = &state->ids.ids[i];
int domain_index;
+ const char *domain_name = NULL;
- sid_copy(&dom_sid, &state->non_cached[i]);
- sid_split_rid(&dom_sid, &t->rid);
+ if (n->sid_index != UINT32_MAX) {
+ const struct lsa_DomainInfo *info;
- info = &domains->domains[n->sid_index];
- t->type = lsa_SidType_to_id_type(n->sid_type);
+ info = &domains->domains[n->sid_index];
+ domain_name = info->name.string;
+ }
+ if (domain_name == NULL) {
+ struct winbindd_domain *wb_domain = NULL;
+
+ /*
+ * This is needed to handle Samba DCs
+ * which always return sid_index == UINT32_MAX for
+ * unknown sids.
+ */
+ wb_domain = find_domain_from_sid_noinit(sid);
+ if (wb_domain != NULL) {
+ domain_name = wb_domain->name;
+ }
+ }
+ if (domain_name == NULL) {
+ domain_name = "";
+ }
+ sid_copy(&dom_sid, sid);
+ sid_split_rid(&dom_sid, &t->rid);
+ t->type = lsa_SidType_to_id_type(n->sid_type);
domain_index = init_lsa_ref_domain_list(
- state, &state->idmap_doms, info->name.string, &dom_sid);
+ state, &state->idmap_doms, domain_name, &dom_sid);
if (domain_index == -1) {
tevent_req_oom(req);
return;
--
2.9.3
From 5763b27d4853a29686b1d3df600a5f20097e63fe Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Mon, 10 Apr 2017 14:28:18 +0200
Subject: [PATCH 2/2] winbindd: only use the domain name from lookup sids if
the domain matches
With the use of sIDHistory it happens that two sids map to the same name:
S-1-5-21-1387724271-3540671778-1971508351-1115 DOMAIN2\d1u1 (1)
S-1-5-21-3293503978-489118715-2763867031-1106 DOMAIN2\d1u1 (1)
On the net it looks like this:
lsa_LookupSids: struct lsa_LookupSids
in: struct lsa_LookupSids
handle : *
handle: struct policy_handle
handle_type : 0x00000000 (0)
uuid : 344f3586-7de4-4e1d-96a9-8c6c23e4b2f0
sids : *
sids: struct lsa_SidArray
num_sids : 0x00000002 (2)
sids : *
sids: ARRAY(2)
sids: struct lsa_SidPtr
sid : *
sid : S-1-5-21-1387724271-3540671778-1971508351-1115
sids: struct lsa_SidPtr
sid : *
sid : S-1-5-21-3293503978-489118715-2763867031-1106
names : *
names: struct lsa_TransNameArray
count : 0x00000000 (0)
names : NULL
level : LSA_LOOKUP_NAMES_ALL (1)
count : *
count : 0x00000000 (0)
lsa_LookupSids: struct lsa_LookupSids
out: struct lsa_LookupSids
domains : *
domains : *
domains: struct lsa_RefDomainList
count : 0x00000001 (1)
domains : *
domains: ARRAY(1)
domains: struct lsa_DomainInfo
name: struct lsa_StringLarge
length : 0x000e (14)
size : 0x0010 (16)
string : *
string : 'DOMAIN2'
sid : *
sid : S-1-5-21-1387724271-3540671778-1971508351
max_size : 0x00000020 (32)
names : *
names: struct lsa_TransNameArray
count : 0x00000002 (2)
names : *
names: ARRAY(7)
names: struct lsa_TranslatedName
sid_type : SID_NAME_USER (1)
name: struct lsa_String
length : 0x0008 (8)
size : 0x0008 (8)
string : *
string : 'd1u1'
sid_index : 0x00000000 (0)
names: struct lsa_TranslatedName
sid_type : SID_NAME_USER (1)
name: struct lsa_String
length : 0x0008 (8)
size : 0x0008 (8)
string : *
string : 'd1u1'
sid_index : 0x00000000 (0)
count : *
count : 0x00000002 (2)
result : NT_STATUS_OK
So the name for S-1-5-21-3293503978-489118715-2763867031-1106 has
S-1-5-21-1387724271-3540671778-1971508351 in referenced lsa_DomainInfo
structure. In that case we should not use the domain name from lsa_DomainInfo,
because we would use the wrong idmap backend.
For the case where the domain part of the sIDHistory sid is a still existing
domain, which can be found our internal list of trusted domains, we now use the
correct idmap backend: the idmap domain from the historic SID.
If the historic domain does no longer exist, we will fallback to the default
idmap domain.
The next step would be doing a lookup sid call for the domain sid, which may
help with one-way trusts.
The long term goal needs to be that idmap backends are based on sids only and
only the smb.conf allows names to be used which will be converted to sids on
startup.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12702
Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Ralph Boehme <slow at samba.org>
Reviewed-by: Uri Simchoni <uri at samba.org>
Autobuild-User(master): Uri Simchoni <uri at samba.org>
Autobuild-Date(master): Wed Apr 12 16:43:30 CEST 2017 on sn-devel-144
(cherry picked from commit 9d419c3fe3654f038fbc978ecb7fa87cf8a5cc3b)
---
source3/winbindd/wb_sids2xids.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/source3/winbindd/wb_sids2xids.c b/source3/winbindd/wb_sids2xids.c
index dc90bdf..b8ad300 100644
--- a/source3/winbindd/wb_sids2xids.c
+++ b/source3/winbindd/wb_sids2xids.c
@@ -194,9 +194,13 @@ static void wb_sids2xids_lookupsids_done(struct tevent_req *subreq)
if (n->sid_index != UINT32_MAX) {
const struct lsa_DomainInfo *info;
+ bool match;
info = &domains->domains[n->sid_index];
- domain_name = info->name.string;
+ match = dom_sid_in_domain(info->sid, sid);
+ if (match) {
+ domain_name = info->name.string;
+ }
}
if (domain_name == NULL) {
struct winbindd_domain *wb_domain = NULL;
--
2.9.3
More information about the samba-technical
mailing list