[PATCH] Patch for bug 13052
Ralph Böhme
slow at samba.org
Tue Oct 10 10:45:47 UTC 2017
Hi,
On Mon, Oct 09, 2017 at 04:24:33PM +0000, Ralph Böhme via samba-technical wrote:
> Please review&push if happy. The patchset just survived a private autobuild and
> was reported by a customer to fix the problem.
Volker suggested an additional check for the sid type returned from lookupnames,
updated patchset attached. With this we catch misconfigurations like:
idmap config Administrator : backend = rid
idmap config Administrator : range = 400000 - 499999
and log:
wb_xids2sids_init_dom_maps_lookupname_done: SID
S-1-5-21-1302242140-3407493554-1668119891-500 for idmap domain name
'administrator' not a domain SID
While at it I factored out wb_xids2sids_init_dom_maps_lookupname_next() (smaller
diff!) in the first commit. Can you please re-review the first commit. The other
patches are unmodfied besides the minor changes for the long line and the
missing space.
Thanks!
-slow
-------------- next part --------------
From 860f4ddbb08d5b7e2c7f345c669b83def11726eb Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Mon, 25 Sep 2017 13:25:57 +0200
Subject: [PATCH 1/4] winbindd: add domain SID to idmap mapping domains
Fetch the domain SID for every domain in the idmap-domain map. This is
in preperation of passing the domain SID as an additional argument to
xid2sid requests to the idmap child.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13052
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_xids2sids.c | 162 ++++++++++++++++++++++++++++++++++++----
1 file changed, 147 insertions(+), 15 deletions(-)
diff --git a/source3/winbindd/wb_xids2sids.c b/source3/winbindd/wb_xids2sids.c
index 15e94c4203f..8850e63c8b4 100644
--- a/source3/winbindd/wb_xids2sids.c
+++ b/source3/winbindd/wb_xids2sids.c
@@ -23,11 +23,13 @@
#include "idmap_cache.h"
#include "librpc/gen_ndr/ndr_winbind_c.h"
#include "librpc/gen_ndr/ndr_netlogon.h"
+#include "passdb/lookup_sid.h"
struct wb_xids2sids_dom_map {
unsigned low_id;
unsigned high_id;
const char *name;
+ struct dom_sid sid;
};
/*
@@ -93,6 +95,7 @@ static bool wb_xids2sids_add_dom(const char *domname,
dom_maps = tmp;
map = &dom_maps[num_maps];
+ ZERO_STRUCTP(map);
map->name = talloc_move(dom_maps, &name);
}
@@ -102,30 +105,138 @@ static bool wb_xids2sids_add_dom(const char *domname,
return false;
}
-static void wb_xids2sids_init_dom_maps(void)
+struct wb_xids2sids_init_dom_maps_state {
+ struct tevent_context *ev;
+ struct tevent_req *req;
+ size_t dom_idx;
+};
+
+static void wb_xids2sids_init_dom_maps_lookupname_next(
+ struct wb_xids2sids_init_dom_maps_state *state);
+
+static void wb_xids2sids_init_dom_maps_lookupname_done(
+ struct tevent_req *subreq);
+
+static struct tevent_req *wb_xids2sids_init_dom_maps_send(
+ TALLOC_CTX *mem_ctx, struct tevent_context *ev)
{
- if (dom_maps != NULL) {
- return;
+ struct tevent_req *req = NULL;
+ struct wb_xids2sids_init_dom_maps_state *state = NULL;
+
+ req = tevent_req_create(mem_ctx, &state,
+ struct wb_xids2sids_init_dom_maps_state);
+ if (req == NULL) {
+ return NULL;
}
+ *state = (struct wb_xids2sids_init_dom_maps_state) {
+ .ev = ev,
+ .req = req,
+ .dom_idx = 0,
+ };
+ if (dom_maps != NULL) {
+ tevent_req_done(req);
+ return tevent_req_post(req, ev);
+ }
/*
* Put the passdb idmap domain first. We always need to try
* there first.
*/
- dom_maps = talloc_array(NULL, struct wb_xids2sids_dom_map, 1);
- if (dom_maps == NULL) {
- return;
+ dom_maps = talloc_zero_array(NULL, struct wb_xids2sids_dom_map, 1);
+ if (tevent_req_nomem(dom_maps, req)) {
+ return tevent_req_post(req, ev);
}
dom_maps[0].low_id = 0;
dom_maps[0].high_id = UINT_MAX;
dom_maps[0].name = talloc_strdup(dom_maps, get_global_sam_name());
- if (dom_maps[0].name == NULL) {
+ if (tevent_req_nomem(dom_maps[0].name, req)) {
TALLOC_FREE(dom_maps);
- return;
+ return tevent_req_post(req, ev);
}
lp_scan_idmap_domains(wb_xids2sids_add_dom, NULL);
+
+ wb_xids2sids_init_dom_maps_lookupname_next(state);
+ if (!tevent_req_is_in_progress(req)) {
+ tevent_req_post(req, ev);
+ }
+ return req;
+}
+
+static void wb_xids2sids_init_dom_maps_lookupname_next(
+ struct wb_xids2sids_init_dom_maps_state *state)
+{
+ struct tevent_req *subreq = NULL;
+
+ if (state->dom_idx == talloc_array_length(dom_maps)) {
+ tevent_req_done(state->req);
+ return;
+ }
+
+ if (strequal(dom_maps[state->dom_idx].name, "*")) {
+ state->dom_idx++;
+ if (state->dom_idx == talloc_array_length(dom_maps)) {
+ tevent_req_done(state->req);
+ return;
+ }
+ }
+
+ subreq = wb_lookupname_send(state,
+ state->ev,
+ dom_maps[state->dom_idx].name,
+ "",
+ LOOKUP_NAME_NO_NSS);
+ if (tevent_req_nomem(subreq, state->req)) {
+ return;
+ }
+ tevent_req_set_callback(subreq,
+ wb_xids2sids_init_dom_maps_lookupname_done,
+ state->req);
+}
+
+static void wb_xids2sids_init_dom_maps_lookupname_done(
+ struct tevent_req *subreq)
+{
+ struct tevent_req *req = tevent_req_callback_data(
+ subreq, struct tevent_req);
+ struct wb_xids2sids_init_dom_maps_state *state = tevent_req_data(
+ req, struct wb_xids2sids_init_dom_maps_state);
+ enum lsa_SidType type;
+ NTSTATUS status;
+
+ status = wb_lookupname_recv(subreq,
+ &dom_maps[state->dom_idx].sid,
+ &type);
+ TALLOC_FREE(subreq);
+ if (!NT_STATUS_IS_OK(status)) {
+ DBG_WARNING("Lookup domain name '%s' failed '%s'\n",
+ dom_maps[state->dom_idx].name,
+ nt_errstr(status));
+
+ state->dom_idx++;
+ wb_xids2sids_init_dom_maps_lookupname_next(state);
+ return;
+ }
+
+ if (type != SID_NAME_DOMAIN) {
+ DBG_WARNING("SID %s for idmap domain name '%s' "
+ "not a domain SID\n",
+ sid_string_dbg(&dom_maps[state->dom_idx].sid),
+ dom_maps[state->dom_idx].name);
+
+ ZERO_STRUCT(dom_maps[state->dom_idx].sid);
+ }
+
+ state->dom_idx++;
+ wb_xids2sids_init_dom_maps_lookupname_next(state);
+
+ return;
+}
+
+static NTSTATUS wb_xids2sids_init_dom_maps_recv(struct tevent_req *req)
+{
+ return tevent_req_simple_recv_ntstatus(req);
}
struct wb_xids2sids_dom_state {
@@ -185,7 +296,6 @@ static struct tevent_req *wb_xids2sids_dom_send(
/* already mapped */
continue;
}
-
state->dom_xids[state->num_dom_xids++] = id;
}
@@ -321,6 +431,7 @@ struct wb_xids2sids_state {
};
static void wb_xids2sids_done(struct tevent_req *subreq);
+static void wb_xids2sids_init_dom_maps_done(struct tevent_req *subreq);
struct tevent_req *wb_xids2sids_send(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,
@@ -329,7 +440,6 @@ struct tevent_req *wb_xids2sids_send(TALLOC_CTX *mem_ctx,
{
struct tevent_req *req, *subreq;
struct wb_xids2sids_state *state;
- size_t num_domains;
req = tevent_req_create(mem_ctx, &state,
struct wb_xids2sids_state);
@@ -371,22 +481,44 @@ struct tevent_req *wb_xids2sids_send(TALLOC_CTX *mem_ctx,
}
}
- wb_xids2sids_init_dom_maps();
- num_domains = talloc_array_length(dom_maps);
+ subreq = wb_xids2sids_init_dom_maps_send(
+ state, state->ev);
+ if (tevent_req_nomem(subreq, req)) {
+ return tevent_req_post(req, ev);
+ }
+ tevent_req_set_callback(subreq, wb_xids2sids_init_dom_maps_done, req);
+ return req;
+}
+
+static void wb_xids2sids_init_dom_maps_done(struct tevent_req *subreq)
+{
+ struct tevent_req *req = tevent_req_callback_data(
+ subreq, struct tevent_req);
+ struct wb_xids2sids_state *state = tevent_req_data(
+ req, struct wb_xids2sids_state);
+ size_t num_domains;
+ NTSTATUS status;
+
+ status = wb_xids2sids_init_dom_maps_recv(subreq);
+ TALLOC_FREE(subreq);
+ if (tevent_req_nterror(req, status)) {
+ return;
+ }
+ num_domains = talloc_array_length(dom_maps);
if (num_domains == 0) {
tevent_req_done(req);
- return tevent_req_post(req, ev);
+ return;
}
subreq = wb_xids2sids_dom_send(
state, state->ev, &dom_maps[state->dom_idx],
state->xids, state->num_xids, state->sids);
if (tevent_req_nomem(subreq, req)) {
- return tevent_req_post(req, ev);
+ return;
}
tevent_req_set_callback(subreq, wb_xids2sids_done, req);
- return req;
+ return;
}
static void wb_xids2sids_done(struct tevent_req *subreq)
--
2.13.5
From 28cf34a9e72bddc1cce44776b1b4f1aa4f1a066e Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Mon, 25 Sep 2017 15:39:39 +0200
Subject: [PATCH 2/4] winbindd: pass domain SID to wbint_UnixIDs2Sids
This makes the domain SID available to the idmap child for
wbint_UnixIDs2Sids mapping request. It's not used yet anywhere, this
comes in the next commit.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13052
Signed-off-by: Ralph Boehme <slow at samba.org>
Reviewed-by: Volker Lendecke <vl at samba.org>
---
librpc/idl/winbind.idl | 1 +
source3/include/idmap.h | 5 +++++
source3/winbindd/idmap.c | 4 +++-
source3/winbindd/idmap_proto.h | 3 ++-
source3/winbindd/wb_xids2sids.c | 5 +++--
source3/winbindd/winbindd_dual_srv.c | 3 ++-
6 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/librpc/idl/winbind.idl b/librpc/idl/winbind.idl
index 737d66abe70..f5e3507bff5 100644
--- a/librpc/idl/winbind.idl
+++ b/librpc/idl/winbind.idl
@@ -58,6 +58,7 @@ interface winbind
NTSTATUS wbint_UnixIDs2Sids(
[in,string,charset(UTF8)] char *domain_name,
+ [in] dom_sid domain_sid,
[in] uint32 num_ids,
[in,out] unixid xids[num_ids],
[out] dom_sid sids[num_ids]
diff --git a/source3/include/idmap.h b/source3/include/idmap.h
index 75d2e45b174..8d80643e6e9 100644
--- a/source3/include/idmap.h
+++ b/source3/include/idmap.h
@@ -37,6 +37,11 @@ struct wbint_userinfo;
struct idmap_domain {
const char *name;
+ /*
+ * dom_sid is currently only initialized in the unixids_to_sids request,
+ * so don't rely on this being filled out everywhere!
+ */
+ struct dom_sid dom_sid;
struct idmap_methods *methods;
NTSTATUS (*query_user)(struct idmap_domain *domain,
struct wbint_userinfo *info);
diff --git a/source3/winbindd/idmap.c b/source3/winbindd/idmap.c
index 6e70b44c425..bfac7f86432 100644
--- a/source3/winbindd/idmap.c
+++ b/source3/winbindd/idmap.c
@@ -600,7 +600,8 @@ NTSTATUS idmap_allocate_gid(struct unixid *id)
}
NTSTATUS idmap_backend_unixids_to_sids(struct id_map **maps,
- const char *domain_name)
+ const char *domain_name,
+ struct dom_sid domain_sid)
{
struct idmap_domain *dom = NULL;
NTSTATUS status;
@@ -621,6 +622,7 @@ NTSTATUS idmap_backend_unixids_to_sids(struct id_map **maps,
return NT_STATUS_NONE_MAPPED;
}
+ dom->dom_sid = domain_sid;
status = dom->methods->unixids_to_sids(dom, maps);
DBG_DEBUG("unixid_to_sids for domain %s returned %s\n",
diff --git a/source3/winbindd/idmap_proto.h b/source3/winbindd/idmap_proto.h
index f4fc2c22739..a36d6c2f5bb 100644
--- a/source3/winbindd/idmap_proto.h
+++ b/source3/winbindd/idmap_proto.h
@@ -34,7 +34,8 @@ void idmap_close(void);
NTSTATUS idmap_allocate_uid(struct unixid *id);
NTSTATUS idmap_allocate_gid(struct unixid *id);
NTSTATUS idmap_backend_unixids_to_sids(struct id_map **maps,
- const char *domain_name);
+ const char *domain_name,
+ struct dom_sid domain_sid);
struct idmap_domain *idmap_find_domain(const char *domname);
/* The following definitions come from winbindd/idmap_nss.c */
diff --git a/source3/winbindd/wb_xids2sids.c b/source3/winbindd/wb_xids2sids.c
index 8850e63c8b4..a2a4493bde8 100644
--- a/source3/winbindd/wb_xids2sids.c
+++ b/source3/winbindd/wb_xids2sids.c
@@ -306,7 +306,7 @@ static struct tevent_req *wb_xids2sids_dom_send(
child = idmap_child();
subreq = dcerpc_wbint_UnixIDs2Sids_send(
- state, ev, child->binding_handle, dom_map->name,
+ state, ev, child->binding_handle, dom_map->name, dom_map->sid,
state->num_dom_xids, state->dom_xids, state->dom_sids);
if (tevent_req_nomem(subreq, req)) {
return tevent_req_post(req, ev);
@@ -409,7 +409,8 @@ static void wb_xids2sids_dom_gotdc(struct tevent_req *subreq)
child = idmap_child();
subreq = dcerpc_wbint_UnixIDs2Sids_send(
state, state->ev, child->binding_handle, state->dom_map->name,
- state->num_dom_xids, state->dom_xids, state->dom_sids);
+ state->dom_map->sid, state->num_dom_xids,
+ state->dom_xids, state->dom_sids);
if (tevent_req_nomem(subreq, req)) {
return;
}
diff --git a/source3/winbindd/winbindd_dual_srv.c b/source3/winbindd/winbindd_dual_srv.c
index f79915c7e69..9fb15e9b0ab 100644
--- a/source3/winbindd/winbindd_dual_srv.c
+++ b/source3/winbindd/winbindd_dual_srv.c
@@ -230,7 +230,8 @@ NTSTATUS _wbint_UnixIDs2Sids(struct pipes_struct *p,
maps[i]->xid = r->in.xids[i];
}
- status = idmap_backend_unixids_to_sids(maps, r->in.domain_name);
+ status = idmap_backend_unixids_to_sids(maps, r->in.domain_name,
+ r->in.domain_sid);
if (!NT_STATUS_IS_OK(status)) {
TALLOC_FREE(maps);
return status;
--
2.13.5
From 5752bc9772d44242bbaa6c94c941e21285dc20a7 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Mon, 25 Sep 2017 15:42:08 +0200
Subject: [PATCH 3/4] winbindd: idmap_rid: don't rely on the static domain list
The domain list in the idmap child is inherited from the parent winbindd
process and may not contain all domains in case enumerating trusted
domains didn't finish before the first winbind request that triggers the
idmap child fork comes along.
The previous commits added the domain SID as an additional argument to
the wbint_UnixIDs2Sids request, storing the domain SID in struct
idmap_domain.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13052
Signed-off-by: Ralph Boehme <slow at samba.org>
Reviewed-by: Volker Lendecke <vl at samba.org>
---
source3/winbindd/idmap_rid.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/source3/winbindd/idmap_rid.c b/source3/winbindd/idmap_rid.c
index 10088b5a27a..6ebb4a191a8 100644
--- a/source3/winbindd/idmap_rid.c
+++ b/source3/winbindd/idmap_rid.c
@@ -54,7 +54,6 @@ static NTSTATUS idmap_rid_initialize(struct idmap_domain *dom)
static NTSTATUS idmap_rid_id_to_sid(struct idmap_domain *dom, struct id_map *map)
{
- struct winbindd_domain *domain;
struct idmap_rid_context *ctx;
ctx = talloc_get_type(dom->private_data, struct idmap_rid_context);
@@ -66,12 +65,13 @@ static NTSTATUS idmap_rid_id_to_sid(struct idmap_domain *dom, struct id_map *map
return NT_STATUS_NONE_MAPPED;
}
- domain = find_domain_from_name_noinit(dom->name);
- if (domain == NULL ) {
+ if (is_null_sid(&dom->dom_sid)) {
+ DBG_INFO("idmap domain '%s' without SID\n", dom->name);
return NT_STATUS_NO_SUCH_DOMAIN;
}
- sid_compose(map->sid, &domain->sid, map->xid.id - dom->low_id + ctx->base_rid);
+ sid_compose(map->sid, &dom->dom_sid,
+ map->xid.id - dom->low_id + ctx->base_rid);
map->status = ID_MAPPED;
map->xid.type = ID_TYPE_BOTH;
--
2.13.5
From abf5c3c6dd8bb958cecff3da3e7347156d7a2a6a Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Mon, 9 Oct 2017 13:29:05 +0200
Subject: [PATCH 4/4] winbindd: idmap_rid: error code for failing id-to-sid
mapping request
NT_STATUS_NO_SUCH_DOMAIN triggers complete request failure in the parent
winbindd. By returning NT_STATUS_NONE_MAPPED winbindd lets the individual
mapping fail but keeps processing any remaining mapping requests.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13052
Signed-off-by: Ralph Boehme <slow at samba.org>
Reviewed-by: Volker Lendecke <vl at samba.org>
---
source3/winbindd/idmap_rid.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/winbindd/idmap_rid.c b/source3/winbindd/idmap_rid.c
index 6ebb4a191a8..b066ba3c50f 100644
--- a/source3/winbindd/idmap_rid.c
+++ b/source3/winbindd/idmap_rid.c
@@ -67,7 +67,7 @@ static NTSTATUS idmap_rid_id_to_sid(struct idmap_domain *dom, struct id_map *map
if (is_null_sid(&dom->dom_sid)) {
DBG_INFO("idmap domain '%s' without SID\n", dom->name);
- return NT_STATUS_NO_SUCH_DOMAIN;
+ return NT_STATUS_NONE_MAPPED;
}
sid_compose(map->sid, &dom->dom_sid,
--
2.13.5
More information about the samba-technical
mailing list