[PATCH] Re: Problem related to ID_TYPE_BOTH -Need suggestion
Michael Adam
obnox at samba.org
Fri Jul 26 07:39:26 MDT 2013
Hi Abhi, Andrew, list...
On 2013-07-19 at 20:58 +1000, Andrew Bartlett wrote:
> On Fri, 2013-07-19 at 16:23 +0530, Abhidnya S Joshi wrote:
> > Hi Andrew,
> >
> > For NFS access we are using auth_sys sec type and no kerberos. We
> > create user on client with same id given by autorid to access NFS
> > share. (Here user's uid and group memberships come from the client).
> > In this case from NFS server side, nss_winbind will not come into
> > consideration.
>
> Then I can only suggest that we fix nss_winbind, and you fix any
> manually created client UID/GID values to match. A redhat style 'user
> private group' would do it nicely.
The winbind changes that introduced ID_TYPE_BOTH already created
what Andrew calls "redhat style user private groups". (and also
the other way around - 'group private users'... ;-)
But as this discussion pointed out, it was not done quite completely.
The attached patchset fixes this in that it extends the
WINBIND_GETGROUPS call to return the 'user private group' as part
of the gid list when the user sid is mapped with ID_TYPE_BOTH.
As a second part of the patch, the multiple calls to sids2xids
(one for each SID in the windows token) is replaced by a single
call, which was a "hygiene item" from my last changes to this
code...
Metze has worked with me on the patches and hence they are
already reviewed, so I will simply push them soon if no one
else does it. ;-)
The patchset also cleanly applys to v4-0-test and I will
create a bugreport.
Regarding the NFS: It is the non-deterministic (or
not-deterministic-enough) nature of idmap_autorid that forbids
the use of nss_winbind on NFS clients if the file server uses
autorid.
Abhidnya: If you create users and groups on the nfs client,
you should make sure to create them in the same way as
on the file server (now).
Cheers - Michael
-------------- next part --------------
From 82a09a73165c6a3f95e3a65e3eb69346928f741a Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Fri, 26 Jul 2013 11:31:41 +0200
Subject: [PATCH 1/4] s3:winbind: fix gid counting and error handling in the
getgroups implementation
Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Michael Adam <obnox at samba.org>
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
source3/winbindd/winbindd_getgroups.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/source3/winbindd/winbindd_getgroups.c b/source3/winbindd/winbindd_getgroups.c
index 1774901..bed8877 100644
--- a/source3/winbindd/winbindd_getgroups.c
+++ b/source3/winbindd/winbindd_getgroups.c
@@ -156,18 +156,22 @@ static void winbindd_getgroups_sid2gid_done(struct tevent_req *subreq)
status = wb_sids2xids_recv(subreq, &xid);
TALLOC_FREE(subreq);
+ if (NT_STATUS_EQUAL(status, NT_STATUS_NONE_MAPPED) ||
+ NT_STATUS_EQUAL(status, STATUS_SOME_UNMAPPED))
+ {
+ status = NT_STATUS_OK;
+ }
+ if (tevent_req_nterror(req, status)) {
+ return;
+ }
+
if (xid.type == ID_TYPE_GID || xid.type == ID_TYPE_BOTH) {
state->gids[state->num_gids] = (gid_t)xid.id;
+ state->num_gids += 1;
} else {
state->gids[state->num_gids] = (uid_t)-1;
}
- /*
- * In case of failure, just continue with the next gid
- */
- if (NT_STATUS_IS_OK(status)) {
- state->num_gids += 1;
- }
state->next_sid += 1;
if (state->next_sid >= state->num_sids) {
--
1.7.9.5
From 174aae3e000626362eb716271673f010ba090853 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Fri, 26 Jul 2013 11:32:34 +0200
Subject: [PATCH 2/4] s3:winbind: fix the getgroups implementation to include
the user sid's GID in case of ID_TYPE_BOTH
This is important for acl checks on the unix level where only a group ace
has been added to the ACL for the user sid, e.g. when accessing Files with
nfs or local unix processes.
Signed-off-by: Michael Adam <obnox at samba.org>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
---
source3/winbindd/winbindd_getgroups.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/source3/winbindd/winbindd_getgroups.c b/source3/winbindd/winbindd_getgroups.c
index bed8877..3d52ce8 100644
--- a/source3/winbindd/winbindd_getgroups.c
+++ b/source3/winbindd/winbindd_getgroups.c
@@ -124,15 +124,17 @@ static void winbindd_getgroups_gettoken_done(struct tevent_req *subreq)
/*
* Convert the group SIDs to gids. state->sids[0] contains the user
- * sid, so start at index 1.
+ * sid. If the idmap backend uses ID_TYPE_BOTH, we might need the
+ * the id of the user sid in the list of group sids, so map the
+ * complete token.
*/
- state->gids = talloc_array(state, gid_t, state->num_sids-1);
+ state->gids = talloc_array(state, gid_t, state->num_sids);
if (tevent_req_nomem(state->gids, req)) {
return;
}
state->num_gids = 0;
- state->next_sid = 1;
+ state->next_sid = 0;
subreq = wb_sids2xids_send(state, state->ev,
&state->sids[state->next_sid], 1);
--
1.7.9.5
From d5c8600329b96441a551813e11c70e668404a080 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Fri, 26 Jul 2013 12:25:27 +0200
Subject: [PATCH 3/4] s3:winbind: change getgroups to only do one sids2xids
call instead of many
Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Michael Adam <obnox at samba.org>
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
source3/winbindd/winbindd_getgroups.c | 68 ++++++++++++++++++++-------------
1 file changed, 42 insertions(+), 26 deletions(-)
diff --git a/source3/winbindd/winbindd_getgroups.c b/source3/winbindd/winbindd_getgroups.c
index 3d52ce8..445de3d 100644
--- a/source3/winbindd/winbindd_getgroups.c
+++ b/source3/winbindd/winbindd_getgroups.c
@@ -29,7 +29,6 @@ struct winbindd_getgroups_state {
enum lsa_SidType type;
int num_sids;
struct dom_sid *sids;
- int next_sid;
int num_gids;
gid_t *gids;
};
@@ -129,15 +128,8 @@ static void winbindd_getgroups_gettoken_done(struct tevent_req *subreq)
* complete token.
*/
- state->gids = talloc_array(state, gid_t, state->num_sids);
- if (tevent_req_nomem(state->gids, req)) {
- return;
- }
- state->num_gids = 0;
- state->next_sid = 0;
-
subreq = wb_sids2xids_send(state, state->ev,
- &state->sids[state->next_sid], 1);
+ state->sids, state->num_sids);
if (tevent_req_nomem(subreq, req)) {
return;
}
@@ -151,12 +143,19 @@ static void winbindd_getgroups_sid2gid_done(struct tevent_req *subreq)
struct winbindd_getgroups_state *state = tevent_req_data(
req, struct winbindd_getgroups_state);
NTSTATUS status;
- struct unixid xid;
+ struct unixid *xids;
+ int i;
- xid.type = ID_TYPE_NOT_SPECIFIED;
- xid.id = UINT32_MAX;
+ xids = talloc_array(state, struct unixid, state->num_sids);
+ if (tevent_req_nomem(xids, req)) {
+ return;
+ }
+ for (i=0; i < state->num_sids; i++) {
+ xids[i].type = ID_TYPE_NOT_SPECIFIED;
+ xids[i].id = UINT32_MAX;
+ }
- status = wb_sids2xids_recv(subreq, &xid);
+ status = wb_sids2xids_recv(subreq, xids);
TALLOC_FREE(subreq);
if (NT_STATUS_EQUAL(status, NT_STATUS_NONE_MAPPED) ||
NT_STATUS_EQUAL(status, STATUS_SOME_UNMAPPED))
@@ -167,26 +166,43 @@ static void winbindd_getgroups_sid2gid_done(struct tevent_req *subreq)
return;
}
- if (xid.type == ID_TYPE_GID || xid.type == ID_TYPE_BOTH) {
- state->gids[state->num_gids] = (gid_t)xid.id;
- state->num_gids += 1;
- } else {
- state->gids[state->num_gids] = (uid_t)-1;
+ state->gids = talloc_array(state, gid_t, state->num_sids);
+ if (tevent_req_nomem(state->gids, req)) {
+ return;
}
+ state->num_gids = 0;
- state->next_sid += 1;
+ for (i=0; i < state->num_sids; i++) {
+ bool include_gid = false;
- if (state->next_sid >= state->num_sids) {
- tevent_req_done(req);
- return;
+ switch (xids[i].type) {
+ case ID_TYPE_NOT_SPECIFIED:
+ case ID_TYPE_UID:
+ break;
+ case ID_TYPE_GID:
+ case ID_TYPE_BOTH:
+ include_gid = true;
+ break;
+ }
+
+ if (!include_gid) {
+ continue;
+ }
+
+ state->gids[state->num_gids] = (gid_t)xids[i].id;
+ state->num_gids += 1;
}
- subreq = wb_sids2xids_send(state, state->ev,
- &state->sids[state->next_sid], 1);
- if (tevent_req_nomem(subreq, req)) {
+ /*
+ * This should not fail, as it does not do any reallocation,
+ * just updating the talloc size.
+ */
+ state->gids = talloc_realloc(state, state->gids, gid_t, state->num_gids);
+ if (tevent_req_nomem(state->gids, req)) {
return;
}
- tevent_req_set_callback(subreq, winbindd_getgroups_sid2gid_done, req);
+
+ tevent_req_done(req);
}
NTSTATUS winbindd_getgroups_recv(struct tevent_req *req,
--
1.7.9.5
From ebd286432cd6867a74faf4c3042d4e66cb32cec7 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Fri, 26 Jul 2013 12:26:30 +0200
Subject: [PATCH 4/4] s3:winbind: add a warning DEBUG message when skipping a
sid from the mapped GID list
This presents a potential security problem when ACLs contain DENY ACEs.
Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Michael Adam <obnox at samba.org>
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
source3/winbindd/winbindd_getgroups.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/source3/winbindd/winbindd_getgroups.c b/source3/winbindd/winbindd_getgroups.c
index 445de3d..b899beb 100644
--- a/source3/winbindd/winbindd_getgroups.c
+++ b/source3/winbindd/winbindd_getgroups.c
@@ -174,10 +174,16 @@ static void winbindd_getgroups_sid2gid_done(struct tevent_req *subreq)
for (i=0; i < state->num_sids; i++) {
bool include_gid = false;
+ const char *debug_missing = NULL;
switch (xids[i].type) {
case ID_TYPE_NOT_SPECIFIED:
+ debug_missing = "not specified";
+ break;
case ID_TYPE_UID:
+ if (i != 0) {
+ debug_missing = "uid";
+ }
break;
case ID_TYPE_GID:
case ID_TYPE_BOTH:
@@ -186,6 +192,18 @@ static void winbindd_getgroups_sid2gid_done(struct tevent_req *subreq)
}
if (!include_gid) {
+ if (debug_missing == NULL) {
+ continue;
+ }
+
+ DEBUG(10, ("WARNING: skipping unix id (%u) for sid %s "
+ "from group list because the idmap type "
+ "is %s. "
+ "This might be a security problem when ACLs "
+ "contain DENY ACEs!\n",
+ (unsigned)xids[i].id,
+ sid_string_tos(&state->sids[i]),
+ debug_missing));
continue;
}
--
1.7.9.5
-------------- next part --------------
From 56d188cb818829404760f2051a514d8865ea6dcb Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Fri, 26 Jul 2013 11:31:41 +0200
Subject: [PATCH 1/4] s3:winbind: fix gid counting and error handling in the
getgroups implementation
Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Michael Adam <obnox at samba.org>
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
source3/winbindd/winbindd_getgroups.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/source3/winbindd/winbindd_getgroups.c b/source3/winbindd/winbindd_getgroups.c
index 1774901..bed8877 100644
--- a/source3/winbindd/winbindd_getgroups.c
+++ b/source3/winbindd/winbindd_getgroups.c
@@ -156,18 +156,22 @@ static void winbindd_getgroups_sid2gid_done(struct tevent_req *subreq)
status = wb_sids2xids_recv(subreq, &xid);
TALLOC_FREE(subreq);
+ if (NT_STATUS_EQUAL(status, NT_STATUS_NONE_MAPPED) ||
+ NT_STATUS_EQUAL(status, STATUS_SOME_UNMAPPED))
+ {
+ status = NT_STATUS_OK;
+ }
+ if (tevent_req_nterror(req, status)) {
+ return;
+ }
+
if (xid.type == ID_TYPE_GID || xid.type == ID_TYPE_BOTH) {
state->gids[state->num_gids] = (gid_t)xid.id;
+ state->num_gids += 1;
} else {
state->gids[state->num_gids] = (uid_t)-1;
}
- /*
- * In case of failure, just continue with the next gid
- */
- if (NT_STATUS_IS_OK(status)) {
- state->num_gids += 1;
- }
state->next_sid += 1;
if (state->next_sid >= state->num_sids) {
--
1.7.9.5
From 9f21a90a202eff78b4c9e7c5950016092f720224 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Fri, 26 Jul 2013 11:32:34 +0200
Subject: [PATCH 2/4] s3:winbind: fix the getgroups implementation to include
the user sid's GID in case of ID_TYPE_BOTH
This is important for acl checks on the unix level where only a group ace
has been added to the ACL for the user sid, e.g. when accessing Files with
nfs or local unix processes.
Signed-off-by: Michael Adam <obnox at samba.org>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
---
source3/winbindd/winbindd_getgroups.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/source3/winbindd/winbindd_getgroups.c b/source3/winbindd/winbindd_getgroups.c
index bed8877..3d52ce8 100644
--- a/source3/winbindd/winbindd_getgroups.c
+++ b/source3/winbindd/winbindd_getgroups.c
@@ -124,15 +124,17 @@ static void winbindd_getgroups_gettoken_done(struct tevent_req *subreq)
/*
* Convert the group SIDs to gids. state->sids[0] contains the user
- * sid, so start at index 1.
+ * sid. If the idmap backend uses ID_TYPE_BOTH, we might need the
+ * the id of the user sid in the list of group sids, so map the
+ * complete token.
*/
- state->gids = talloc_array(state, gid_t, state->num_sids-1);
+ state->gids = talloc_array(state, gid_t, state->num_sids);
if (tevent_req_nomem(state->gids, req)) {
return;
}
state->num_gids = 0;
- state->next_sid = 1;
+ state->next_sid = 0;
subreq = wb_sids2xids_send(state, state->ev,
&state->sids[state->next_sid], 1);
--
1.7.9.5
From f64355ff5442b059f9b60202c4912ba196605ba8 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Fri, 26 Jul 2013 12:25:27 +0200
Subject: [PATCH 3/4] s3:winbind: change getgroups to only do one sids2xids
call instead of many
Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Michael Adam <obnox at samba.org>
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
source3/winbindd/winbindd_getgroups.c | 68 ++++++++++++++++++++-------------
1 file changed, 42 insertions(+), 26 deletions(-)
diff --git a/source3/winbindd/winbindd_getgroups.c b/source3/winbindd/winbindd_getgroups.c
index 3d52ce8..445de3d 100644
--- a/source3/winbindd/winbindd_getgroups.c
+++ b/source3/winbindd/winbindd_getgroups.c
@@ -29,7 +29,6 @@ struct winbindd_getgroups_state {
enum lsa_SidType type;
int num_sids;
struct dom_sid *sids;
- int next_sid;
int num_gids;
gid_t *gids;
};
@@ -129,15 +128,8 @@ static void winbindd_getgroups_gettoken_done(struct tevent_req *subreq)
* complete token.
*/
- state->gids = talloc_array(state, gid_t, state->num_sids);
- if (tevent_req_nomem(state->gids, req)) {
- return;
- }
- state->num_gids = 0;
- state->next_sid = 0;
-
subreq = wb_sids2xids_send(state, state->ev,
- &state->sids[state->next_sid], 1);
+ state->sids, state->num_sids);
if (tevent_req_nomem(subreq, req)) {
return;
}
@@ -151,12 +143,19 @@ static void winbindd_getgroups_sid2gid_done(struct tevent_req *subreq)
struct winbindd_getgroups_state *state = tevent_req_data(
req, struct winbindd_getgroups_state);
NTSTATUS status;
- struct unixid xid;
+ struct unixid *xids;
+ int i;
- xid.type = ID_TYPE_NOT_SPECIFIED;
- xid.id = UINT32_MAX;
+ xids = talloc_array(state, struct unixid, state->num_sids);
+ if (tevent_req_nomem(xids, req)) {
+ return;
+ }
+ for (i=0; i < state->num_sids; i++) {
+ xids[i].type = ID_TYPE_NOT_SPECIFIED;
+ xids[i].id = UINT32_MAX;
+ }
- status = wb_sids2xids_recv(subreq, &xid);
+ status = wb_sids2xids_recv(subreq, xids);
TALLOC_FREE(subreq);
if (NT_STATUS_EQUAL(status, NT_STATUS_NONE_MAPPED) ||
NT_STATUS_EQUAL(status, STATUS_SOME_UNMAPPED))
@@ -167,26 +166,43 @@ static void winbindd_getgroups_sid2gid_done(struct tevent_req *subreq)
return;
}
- if (xid.type == ID_TYPE_GID || xid.type == ID_TYPE_BOTH) {
- state->gids[state->num_gids] = (gid_t)xid.id;
- state->num_gids += 1;
- } else {
- state->gids[state->num_gids] = (uid_t)-1;
+ state->gids = talloc_array(state, gid_t, state->num_sids);
+ if (tevent_req_nomem(state->gids, req)) {
+ return;
}
+ state->num_gids = 0;
- state->next_sid += 1;
+ for (i=0; i < state->num_sids; i++) {
+ bool include_gid = false;
- if (state->next_sid >= state->num_sids) {
- tevent_req_done(req);
- return;
+ switch (xids[i].type) {
+ case ID_TYPE_NOT_SPECIFIED:
+ case ID_TYPE_UID:
+ break;
+ case ID_TYPE_GID:
+ case ID_TYPE_BOTH:
+ include_gid = true;
+ break;
+ }
+
+ if (!include_gid) {
+ continue;
+ }
+
+ state->gids[state->num_gids] = (gid_t)xids[i].id;
+ state->num_gids += 1;
}
- subreq = wb_sids2xids_send(state, state->ev,
- &state->sids[state->next_sid], 1);
- if (tevent_req_nomem(subreq, req)) {
+ /*
+ * This should not fail, as it does not do any reallocation,
+ * just updating the talloc size.
+ */
+ state->gids = talloc_realloc(state, state->gids, gid_t, state->num_gids);
+ if (tevent_req_nomem(state->gids, req)) {
return;
}
- tevent_req_set_callback(subreq, winbindd_getgroups_sid2gid_done, req);
+
+ tevent_req_done(req);
}
NTSTATUS winbindd_getgroups_recv(struct tevent_req *req,
--
1.7.9.5
From 6821a398121430326b1f464e4756e60f057dd1af Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Fri, 26 Jul 2013 12:26:30 +0200
Subject: [PATCH 4/4] s3:winbind: add a warning DEBUG message when skipping a
sid from the mapped GID list
This presents a potential security problem when ACLs contain DENY ACEs.
Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Michael Adam <obnox at samba.org>
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
source3/winbindd/winbindd_getgroups.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/source3/winbindd/winbindd_getgroups.c b/source3/winbindd/winbindd_getgroups.c
index 445de3d..b899beb 100644
--- a/source3/winbindd/winbindd_getgroups.c
+++ b/source3/winbindd/winbindd_getgroups.c
@@ -174,10 +174,16 @@ static void winbindd_getgroups_sid2gid_done(struct tevent_req *subreq)
for (i=0; i < state->num_sids; i++) {
bool include_gid = false;
+ const char *debug_missing = NULL;
switch (xids[i].type) {
case ID_TYPE_NOT_SPECIFIED:
+ debug_missing = "not specified";
+ break;
case ID_TYPE_UID:
+ if (i != 0) {
+ debug_missing = "uid";
+ }
break;
case ID_TYPE_GID:
case ID_TYPE_BOTH:
@@ -186,6 +192,18 @@ static void winbindd_getgroups_sid2gid_done(struct tevent_req *subreq)
}
if (!include_gid) {
+ if (debug_missing == NULL) {
+ continue;
+ }
+
+ DEBUG(10, ("WARNING: skipping unix id (%u) for sid %s "
+ "from group list because the idmap type "
+ "is %s. "
+ "This might be a security problem when ACLs "
+ "contain DENY ACEs!\n",
+ (unsigned)xids[i].id,
+ sid_string_tos(&state->sids[i]),
+ debug_missing));
continue;
}
--
1.7.9.5
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 215 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20130726/f881cf6d/attachment.pgp>
More information about the samba-technical
mailing list