[PATCHES] winbindd: fix sid->xid for SID History SIDs
Uri Simchoni
uri at samba.org
Mon Apr 3 04:50:08 UTC 2017
Hi,
Here's a V2 of the patch set.
To address the concerns in the comments:
- Resolve domain sid instead of <dom-sid>-513 - done. Domain sids didn't
undergo "bulk resolving", so this is fixed too..
- Efficiency of finding domain name - done by just adding (exactly once)
the domain sid to the resolve list, instead of adding only if it's not
already there. This way we guarantee the domain sids are nicely tucked
at the end of the list, which narrows the search. This change may cause
a domain sid to appear twice in the lookup list. This works (on Windows
and Samba DCs), and IMHO, the extra complexity of avoiding it is not
worth it because it should be a rare-if-non-existent case to do
sid-to-xid for a domain sid.
- Shouldn't lookup domain name if we know it already - not handled.
Since the domain lookup is added to the bulk lookup, I don't believe
doing so would have a noticeable impact. I would prefer to keep the fix
small (for backports) and add such an optimization when the possible
gain is to avoid the lookup entirely.
- Tests - As Metze said, we can't really test it yet (except manually).
The one thing I'm not entirely comfortable with and would add a test for
is that assertion that looking up domain SID twice in the lookup list
works - I can add a wbinfo test for that to make sure we don't regress,
if the concept is acceptable.
Thanks,
Uri.
On 03/20/2017 06:49 AM, Uri Simchoni via samba-technical wrote:
> Hi,
>
> We few weeks ago we've discussed SID history and id-mapping -
> https://lists.samba.org/archive/samba-technical/2017-February/118771.html.
>
> Attached is a proposed initial fix for the issue, which focuses on
> avoiding wrong results.
>
> The fix finds the domain of the SID by resolving a SID with same domain
> component and an RID of 513 (domain users), which hopefully never gets
> migrated.
>
> We've discussed other means such as smb.conf stuff or netsamlogon - I
> think those methods can come on top of this method, because if they
> don't work we should always fall back to something. The added resolving
> doesn't cost much because it's in the same round-trip.
>
> The key thing about this fix is that doesn't try to translate sid->xid
> in any possible case (such as when old domain is gone and forgotten), it
> just avoids getting the *wrong* result. As such, it's a good minimal fix
> that can be applied to stable versions. For master, we can add the
> smb.conf-based stuff, that will support more cases.
>
> Review appreciated.
> Thanks,
> Uri.
>
-------------- next part --------------
From 241a1026fe636e825a335af6d85e822b04002dc9 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Fri, 17 Mar 2017 00:18:08 +0200
Subject: [PATCH v2 1/6] add_sid_to_array() - support pre-allocated arrays
Only grow array if needed.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12702
Signed-off-by: Uri Simchoni <uri at samba.org>
---
libcli/security/util_sid.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/libcli/security/util_sid.c b/libcli/security/util_sid.c
index 0709a7a..0e42eb4 100644
--- a/libcli/security/util_sid.c
+++ b/libcli/security/util_sid.c
@@ -344,6 +344,13 @@ NTSTATUS add_sid_to_array(TALLOC_CTX *mem_ctx, const struct dom_sid *sid,
return NT_STATUS_INTEGER_OVERFLOW;
}
+ if (*sids != NULL && talloc_array_length(*sids) >= (*num) + 1) {
+ sid_copy(&((*sids)[*num]), sid);
+ *num += 1;
+
+ return NT_STATUS_OK;
+ }
+
tmp = talloc_realloc(mem_ctx, *sids, struct dom_sid, (*num)+1);
if (tmp == NULL) {
*num = 0;
--
2.9.3
From e21915d8ff3e2d41863d7830a5ee038b0f25bea2 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Wed, 29 Mar 2017 21:59:40 +0300
Subject: [PATCH v2 2/6] libcli: introduce extend_sid_array()
This function adds the elements of array2 to
the back of array1.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12702
Signed-off-by: Uri Simchoni <uri at samba.org>
---
libcli/security/dom_sid.h | 5 +++++
libcli/security/util_sid.c | 24 ++++++++++++++++++++++++
2 files changed, 29 insertions(+)
diff --git a/libcli/security/dom_sid.h b/libcli/security/dom_sid.h
index bdcec94..735806d 100644
--- a/libcli/security/dom_sid.h
+++ b/libcli/security/dom_sid.h
@@ -102,6 +102,11 @@ NTSTATUS add_sid_to_array(TALLOC_CTX *mem_ctx, const struct dom_sid *sid,
struct dom_sid **sids, uint32_t *num);
NTSTATUS add_sid_to_array_unique(TALLOC_CTX *mem_ctx, const struct dom_sid *sid,
struct dom_sid **sids, uint32_t *num_sids);
+NTSTATUS extend_sid_array(TALLOC_CTX *mem_ctx,
+ struct dom_sid **array1,
+ uint32_t *num_sids,
+ const struct dom_sid *array2,
+ uint32_t num_extend);
void del_sid_from_array(const struct dom_sid *sid, struct dom_sid **sids,
uint32_t *num);
bool add_rid_to_array_unique(TALLOC_CTX *mem_ctx,
diff --git a/libcli/security/util_sid.c b/libcli/security/util_sid.c
index 0e42eb4..a6af017 100644
--- a/libcli/security/util_sid.c
+++ b/libcli/security/util_sid.c
@@ -384,6 +384,30 @@ NTSTATUS add_sid_to_array_unique(TALLOC_CTX *mem_ctx, const struct dom_sid *sid,
}
/********************************************************************
+ Add SIDs of array2 to the back of array1.
+********************************************************************/
+
+NTSTATUS extend_sid_array(TALLOC_CTX *mem_ctx,
+ struct dom_sid **array1,
+ uint32_t *num_sids,
+ const struct dom_sid *array2,
+ uint32_t num_extend)
+{
+ uint32_t i;
+ NTSTATUS status;
+
+ for (i = 0; i < num_extend; ++i) {
+ status =
+ add_sid_to_array(mem_ctx, &array2[i], array1, num_sids);
+ if (!NT_STATUS_IS_OK(status)) {
+ return status;
+ }
+ }
+
+ return NT_STATUS_OK;
+}
+
+/********************************************************************
Remove SID from an array
********************************************************************/
--
2.9.3
From a1956dabc16d70fb91f5601e156b28c98e082e7e Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Fri, 17 Mar 2017 23:45:31 +0200
Subject: [PATCH v2 3/6] libcli: introduce is_domain[_principa]l_sid()
Add routines to indicate whether an SID belongs to a
domain, as opposed to "special", alias SIDs.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12702
Signed-off-by: Uri Simchoni <uri at samba.org>
---
libcli/security/dom_sid.h | 3 +++
libcli/security/util_sid.c | 20 ++++++++++++++++++++
2 files changed, 23 insertions(+)
diff --git a/libcli/security/dom_sid.h b/libcli/security/dom_sid.h
index 735806d..ee0f67a 100644
--- a/libcli/security/dom_sid.h
+++ b/libcli/security/dom_sid.h
@@ -113,5 +113,8 @@ bool add_rid_to_array_unique(TALLOC_CTX *mem_ctx,
uint32_t rid, uint32_t **pp_rids, size_t *p_num);
bool is_null_sid(const struct dom_sid *sid);
+bool is_domain_sid(const struct dom_sid *sid);
+
+bool is_domain_principal_sid(const struct dom_sid *sid);
#endif /*_DOM_SID_H_*/
diff --git a/libcli/security/util_sid.c b/libcli/security/util_sid.c
index a6af017..69ee521 100644
--- a/libcli/security/util_sid.c
+++ b/libcli/security/util_sid.c
@@ -93,6 +93,9 @@ const struct dom_sid global_sid_Anonymous = /* Anonymous login */
/* S-1-5-9 */
const struct dom_sid global_sid_Enterprise_DCs = /* Enterprise DCs */
{ 1, 1, {0,0,0,0,0,5}, {9,0,0,0,0,0,0,0,0,0,0,0,0,0,0}};
+/* S-1-5-21 */
+const struct dom_sid global_sid_non_unique =
+{ 1, 1, {0,0,0,0,0,5}, {21,0,0,0,0,0,0,0,0,0,0,0,0,0,0}};
/* S-1-5-32 */
const struct dom_sid global_sid_Builtin = /* Local well-known domain */
{ 1, 1, {0,0,0,0,0,5}, {32,0,0,0,0,0,0,0,0,0,0,0,0,0,0}};
@@ -465,3 +468,20 @@ bool is_null_sid(const struct dom_sid *sid)
static const struct dom_sid null_sid = {0};
return dom_sid_equal(sid, &null_sid);
}
+
+/*
+ * The following couple of routines check whether an SID belongs to
+ * a domain or is a domain SID (as opposed to aliases of all sorts)
+ * See [MS-DTYP] 2.4.2.4
+ */
+bool is_domain_sid(const struct dom_sid *sid)
+{
+ return sid_compare_domain(sid, &global_sid_non_unique) == 0 &&
+ sid->num_auths == 4;
+}
+
+bool is_domain_principal_sid(const struct dom_sid *sid)
+{
+ return sid_compare_domain(sid, &global_sid_non_unique) == 0 &&
+ sid->num_auths == 5;
+}
--
2.9.3
From 3fd4c2de7a549d8e7c51b6af79ec78648099ad24 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Sun, 2 Apr 2017 00:04:32 +0300
Subject: [PATCH v2 4/6] winbindd: bulk-resolve domain SIDs
This change changes SID resolving as follows:
1. Limit bulk sid resolution to SIDs belonging to a
domain (according to the comment in the source that
was the original intent)
2. Add domain sids to bulk resolution - DCs seem to be
able to handle this (verified using "rpcclient lookupsids")
Adding the domain SIDs means adding them to the list of SIDs
to be resolved incurs little overhead, which is important for
a change-to-come.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12702
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/winbindd/wb_lookupsids.c | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/source3/winbindd/wb_lookupsids.c b/source3/winbindd/wb_lookupsids.c
index 3f48ad7..641ce94 100644
--- a/source3/winbindd/wb_lookupsids.c
+++ b/source3/winbindd/wb_lookupsids.c
@@ -258,7 +258,7 @@ static bool wb_lookupsids_next(struct tevent_req *req,
static bool wb_lookupsids_bulk(const struct dom_sid *sid)
{
- if (sid->num_auths != 5) {
+ if (!is_domain_sid(sid) && !is_domain_principal_sid(sid)) {
/*
* Only do "S-1-5-21-x-y-z-rid" domains via bulk
* lookup
@@ -291,19 +291,6 @@ static bool wb_lookupsids_bulk(const struct dom_sid *sid)
return false;
}
- if (sid_check_is_in_unix_groups(sid) ||
- sid_check_is_unix_groups(sid) ||
- sid_check_is_in_unix_users(sid) ||
- sid_check_is_unix_users(sid) ||
- sid_check_is_in_builtin(sid) ||
- sid_check_is_builtin(sid)) {
- /*
- * These are locally done piece by piece anyway, no
- * need for bulk optimizations.
- */
- return false;
- }
-
/*
* All other SIDs are sent to the DC we're connected to as
* member via a single lsa_lookupsids call.
--
2.9.3
From 9ba654829967523c0e29e3ea3c7fc79637fa4eeb Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Fri, 17 Mar 2017 23:53:00 +0200
Subject: [PATCH v2 5/6] winbindd-sids2xids: resolve domain sids of resolved
sids
When resolving a list of SIDs prior to sid->xid translation,
also resolve the domain SID for each SID on the list.
The domain SID shall be a more reliable source of domain
name than the SID itself, because the SID to be translated
might belong to an account that moved into another domain.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12702
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/winbindd/wb_sids2xids.c | 63 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 62 insertions(+), 1 deletion(-)
diff --git a/source3/winbindd/wb_sids2xids.c b/source3/winbindd/wb_sids2xids.c
index 9bb8fa8..f3add28 100644
--- a/source3/winbindd/wb_sids2xids.c
+++ b/source3/winbindd/wb_sids2xids.c
@@ -25,6 +25,7 @@
#include "librpc/gen_ndr/ndr_winbind_c.h"
#include "librpc/gen_ndr/ndr_netlogon.h"
#include "lsa.h"
+#include "passdb/machine_sid.h"
struct wb_sids2xids_state {
struct tevent_context *ev;
@@ -36,6 +37,7 @@ struct wb_sids2xids_state {
struct dom_sid *non_cached;
uint32_t num_non_cached;
+ uint32_t num_to_resolve;
/*
* Domain array to use for the idmap call. The output from
@@ -60,6 +62,9 @@ struct wb_sids2xids_state {
static bool wb_sids2xids_in_cache(struct dom_sid *sid, struct id_map *map);
+static bool wb_add_domain_sids_to_lookup(TALLOC_CTX *ctx,
+ struct dom_sid **sids,
+ uint32_t *num_sids);
static void wb_sids2xids_lookupsids_done(struct tevent_req *subreq);
static void wb_sids2xids_done(struct tevent_req *subreq);
static void wb_sids2xids_gotdc(struct tevent_req *subreq);
@@ -125,8 +130,15 @@ struct tevent_req *wb_sids2xids_send(TALLOC_CTX *mem_ctx,
return tevent_req_post(req, ev);
}
+ state->num_to_resolve = state->num_non_cached;
+ if (!wb_add_domain_sids_to_lookup(state, &state->non_cached,
+ &state->num_to_resolve)) {
+ tevent_req_oom(req);
+ return tevent_req_post(req, ev);
+ }
+
subreq = wb_lookupsids_send(state, ev, state->non_cached,
- state->num_non_cached);
+ state->num_to_resolve);
if (tevent_req_nomem(subreq, req)) {
return tevent_req_post(req, ev);
}
@@ -154,6 +166,55 @@ static bool wb_sids2xids_in_cache(struct dom_sid *sid, struct id_map *map)
return false;
}
+/*
+ * For each SID in the list (except for "special" ones),
+ * make sure the list also contains the corresponding
+ * domain SID of the same domain. That will be
+ * the basis for determining the id-mapping domain.
+ *
+ * Each domain SID is added exactly
+ * once (that is, even if it had been on the list
+ * before calling this routine).
+ */
+static bool wb_add_domain_sids_to_lookup(TALLOC_CTX *ctx,
+ struct dom_sid **sids,
+ uint32_t *num_sids)
+{
+ uint32_t i, n = *num_sids;
+ uint32_t num_dom_sids = 0;
+ struct dom_sid sid;
+ struct dom_sid *dom_sids = NULL;
+ NTSTATUS status;
+
+ for (i = 0; i < n; ++i) {
+ sid_copy(&sid, &(*sids)[i]);
+ if (!is_domain_principal_sid(&sid)) {
+ /* "special" or domain SID */
+ continue;
+ }
+
+ if (sid_check_is_in_our_sam(&sid)) {
+ /* no need to add our domain -
+ * the lookup is always
+ * correct for ID mapping.
+ */
+ continue;
+ }
+
+ sid_split_rid(&sid, NULL);
+ status =
+ add_sid_to_array_unique(ctx, &sid, &dom_sids, &num_dom_sids);
+ if (!NT_STATUS_IS_OK(status)) {
+ TALLOC_FREE(dom_sids);
+ return false;
+ }
+ }
+
+ status = extend_sid_array(ctx, sids, num_sids, dom_sids, num_dom_sids);
+ TALLOC_FREE(dom_sids);
+ return NT_STATUS_IS_OK(status);
+}
+
static enum id_type lsa_SidType_to_id_type(const enum lsa_SidType sid_type);
static struct wbint_TransIDArray *wb_sids2xids_extract_for_domain_index(
TALLOC_CTX *mem_ctx, const struct wbint_TransIDArray *src,
--
2.9.3
From 97857d8da49bf20dd257741ee85a806c405e4c67 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Fri, 17 Mar 2017 23:58:35 +0200
Subject: [PATCH v2 6/6] winbindd-sids2xids: reliably determine idmap backend
name
For SIDs which belong to domains, determine the backend name
by the resolved name of corresponding domain SID.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12702
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/winbindd/wb_sids2xids.c | 65 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 62 insertions(+), 3 deletions(-)
diff --git a/source3/winbindd/wb_sids2xids.c b/source3/winbindd/wb_sids2xids.c
index f3add28..771b889 100644
--- a/source3/winbindd/wb_sids2xids.c
+++ b/source3/winbindd/wb_sids2xids.c
@@ -216,6 +216,12 @@ static bool wb_add_domain_sids_to_lookup(TALLOC_CTX *ctx,
}
static enum id_type lsa_SidType_to_id_type(const enum lsa_SidType sid_type);
+static const char *
+wb_get_idmap_backend_name(const struct lsa_RefDomainList *domains,
+ const struct lsa_TransNameArray *names,
+ const struct dom_sid *sids,
+ uint32_t idx,
+ uint32_t dom_sids_idx);
static struct wbint_TransIDArray *wb_sids2xids_extract_for_domain_index(
TALLOC_CTX *mem_ctx, const struct wbint_TransIDArray *src,
uint32_t domain_index);
@@ -247,7 +253,9 @@ static void wb_sids2xids_lookupsids_done(struct tevent_req *subreq)
for (i=0; i<state->num_non_cached; i++) {
struct dom_sid dom_sid;
- struct lsa_DomainInfo *info;
+ const char *dom_name =
+ wb_get_idmap_backend_name(domains, names, state->non_cached,
+ i, state->num_non_cached);
struct lsa_TranslatedName *n = &names->names[i];
struct wbint_TransID *t = &state->ids.ids[i];
int domain_index;
@@ -255,11 +263,10 @@ static void wb_sids2xids_lookupsids_done(struct tevent_req *subreq)
sid_copy(&dom_sid, &state->non_cached[i]);
sid_split_rid(&dom_sid, &t->rid);
- info = &domains->domains[n->sid_index];
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, dom_name, &dom_sid);
if (domain_index == -1) {
tevent_req_oom(req);
return;
@@ -318,6 +325,58 @@ static enum id_type lsa_SidType_to_id_type(const enum lsa_SidType sid_type)
return type;
}
+static const char *
+wb_get_idmap_backend_name(const struct lsa_RefDomainList *domains,
+ const struct lsa_TransNameArray *names,
+ const struct dom_sid *sids,
+ uint32_t idx,
+ uint32_t dom_sids_idx)
+{
+ struct dom_sid sid = sids[idx];
+ struct lsa_TranslatedName *n = &names->names[idx];
+ struct lsa_DomainInfo *info = NULL;
+ if (!is_domain_principal_sid(&sid)) {
+ /* "special" SID */
+ goto ret;
+ }
+
+ if (sid_check_is_in_our_sam(&sid)) {
+ /* our domain - the lookup is always
+ * correct for ID mapping.
+ */
+ goto ret;
+ }
+
+ sid_split_rid(&sid, NULL);
+
+ for (idx = dom_sids_idx; idx < names->count; ++idx) {
+ if (!dom_sid_equal(&sid, &sids[idx])) {
+ continue;
+ }
+
+ /* has the DC been able to resolve
+ * the domain SID?
+ */
+ n = &names->names[idx];
+ if (n->sid_type != SID_NAME_DOMAIN) {
+ DBG_DEBUG("the DC returned non-domain SID type of %d "
+ "for domain %s\n",
+ n->sid_type, sid_string_dbg(&sids[idx]));
+ /* default id-mapping backend */
+ return "";
+ }
+
+ goto ret;
+ }
+
+ /* we're not supposed to just fall off the loop... */
+ smb_panic("Have not found domain SID");
+
+ret:
+ info = &domains->domains[n->sid_index];
+ return info->name.string;
+}
+
static void wb_sids2xids_done(struct tevent_req *subreq)
{
struct tevent_req *req = tevent_req_callback_data(
--
2.9.3
More information about the samba-technical
mailing list