libwbclient: duplicate primary group in returned sids array (was github PR #102)

Isaac Boukris iboukris at gmail.com
Tue Jan 15 12:58:32 UTC 2019


Hi,

On Wed, Jan 9, 2019 at 10:26 PM Isaac Boukris <iboukris at gmail.com> wrote:
>
> On Tue, Jan 8, 2019 at 10:17 PM Volker Lendecke
> <Volker.Lendecke at sernet.de> wrote:
> >
> > On Tue, Jan 08, 2019 at 12:36:59PM +0200, Isaac Boukris via samba-technical wrote:
> > > A while ego, I was testing wbcAuthenticateUserEx with NTLM against
> > > Windows and noticed that the primary group is being referenced twice
> > > in returned wbcAuthUserInfo.sids array.
> > >
> > > The way it happens as far as I recall, is because the DC returns the
> > > primary group in NETLOGON_VALIDATION_SAM_INFO, both as PrimaryGroupId
> > > and in GroupIds. Then libwbclient copies it to a single array where
> > > array[0] is the user SID, array[1] is the PrimaryGroupId and then it
> > > adds the primary group again as a part of the groups from GroupIds.
> > >
> > > This patch skips the second addition, as it seems odd to have
> > > duplicate SIDs in a flat array (even though the first two elements are
> > > fixed). If this looks right, I'll try to add a test and resubmit on
> > > gitlab.
> >
> > Of course this looks functionally right. One small nit-pick: We always
> > do { } even if there's only one statement in the "if"-clause.
>
> Thanks, I'll fix in next round.
>
> > The other question is: Is this too specific? Should we filter all
> > duplicate SIDs in case they happen?
>
> To my understanding, there are no real duplicates SIDs in the DC
> response or in winbind, they just use a different data structure,
> like:
> struct {
>     UserId,
>     PrimaryGroupId,
>     GroupIds[],  (all groups, including the primary but not including uid)
>     ...
> }
>
> So yes, the primary group is referenced twice but in different
> contexts. It only becomes an issue when libwbclient stacks all these
> SIDs in a single array as now it looks like a duplicate.
>
> > And -- is this the right place? My
> > gut feeling would be to keep the winbind clients as simple as possible
> > and put complexity into winbind itself. Should we filter the primary
> > group RID inside winbind? Really a genuine question, I don't have a
> > firm opinion yet.
>
> The transition from info3 struct to a single array (with reserved
> positions) happens in wbclient, hence it seemed the right place to
> skip the primary group id.
>
> On Tue, Jan 8, 2019 at 11:02 PM Jeremy Allison <jra at samba.org> wrote:
> >
> > This looks right to me, but shouldn't you also scan for and
> > ignore user_rid as well, so we only get the first two entries
> > once ? Not sure if this can happen, but just makes it impossible
> > anyway.
>
> It should not happen as the uid is not referenced in GroupIds (because
> it's not a group, I guess). So I don't think we should filter it,
> instead, I'll try to add a test that shows there are no duplicates in
> the returned array.

This isn't as simple as I thought and it looks like the duplicate SID
in wbclient is just the tip of the iceberg. My previous tests were
against Windows DC, but it turns out it is not reproducible with a
Samba DC .. because Samba does not add the primary group to the rid
array to begin with, which may have real impact on authorization.

See bugs:
https://bugzilla.samba.org/show_bug.cgi?id=13136
https://bugzilla.samba.org/show_bug.cgi?id=11362

I managed to get Samba DC to respond correctly (that is, same as
Windows) for both PAC and SamLogon requests, by refactoring the logic
in authsam_make_user_info_dc() and adding the primary rid twice.. I
only tested it very basically and it would need more work and tests,
but I think it heads in the right direction, please take a look (once
we sort this out, we can revisit the wbclient patch).

WIP patch attached, pipeline at (running):
https://gitlab.com/samba-team/devel/samba/pipelines/43412018

Thanks!
-------------- next part --------------
From cba5f8782902776ef7c5bebf8fb0aadec098a91a Mon Sep 17 00:00:00 2001
From: Isaac Boukris <iboukris at gmail.com>
Date: Tue, 15 Jan 2019 13:58:52 +0200
Subject: [PATCH 1/2] sam.c: fix incorrect check of talloc_new() allocation

Signed-off-by: Isaac Boukris <iboukris at gmail.com>
---
 source4/auth/sam.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/source4/auth/sam.c b/source4/auth/sam.c
index 709e901b45b..6253ad0445c 100644
--- a/source4/auth/sam.c
+++ b/source4/auth/sam.c
@@ -364,7 +364,7 @@ _PUBLIC_ NTSTATUS authsam_make_user_info_dc(TALLOC_CTX *mem_ctx,
 	NT_STATUS_HAVE_NO_MEMORY(user_info_dc);
 
 	tmp_ctx = talloc_new(user_info_dc);
-	if (user_info_dc == NULL) {
+	if (tmp_ctx == NULL) {
 		TALLOC_FREE(user_info_dc);
 		return NT_STATUS_NO_MEMORY;
 	}
-- 
2.14.4


From d1eecf834d2b2be7d70de0fce2e923825d598c07 Mon Sep 17 00:00:00 2001
From: Isaac Boukris <iboukris at gmail.com>
Date: Tue, 15 Jan 2019 14:17:32 +0200
Subject: [PATCH 2/2] [WIP]: Add primary group to domain group array in
 user-info token

This is required for both PAC token and SamLogon response.

See:
https://bugzilla.samba.org/show_bug.cgi?id=13136
https://bugzilla.samba.org/show_bug.cgi?id=11362

Signed-off-by: Isaac Boukris <iboukris at gmail.com>
---
 source4/auth/sam.c | 45 ++++++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/source4/auth/sam.c b/source4/auth/sam.c
index 6253ad0445c..2a46d43de9d 100644
--- a/source4/auth/sam.c
+++ b/source4/auth/sam.c
@@ -350,6 +350,7 @@ _PUBLIC_ NTSTATUS authsam_make_user_info_dc(TALLOC_CTX *mem_ctx,
 	char *filter = NULL;
 	/* SIDs for the account and his primary group */
 	struct dom_sid *account_sid;
+	struct dom_sid *primary_group_sid;
 	struct dom_sid_buf buf;
 	const char *primary_group_dn;
 	DATA_BLOB primary_group_blob;
@@ -369,14 +370,6 @@ _PUBLIC_ NTSTATUS authsam_make_user_info_dc(TALLOC_CTX *mem_ctx,
 		return NT_STATUS_NO_MEMORY;
 	}
 
-	sids = talloc_array(user_info_dc, struct dom_sid, 2);
-	if (sids == NULL) {
-		TALLOC_FREE(user_info_dc);
-		return NT_STATUS_NO_MEMORY;
-	}
-
-	num_sids = 2;
-
 	account_sid = samdb_result_dom_sid(user_info_dc, msg, "objectSid");
 	if (account_sid == NULL) {
 		TALLOC_FREE(user_info_dc);
@@ -385,13 +378,18 @@ _PUBLIC_ NTSTATUS authsam_make_user_info_dc(TALLOC_CTX *mem_ctx,
 
 	status = dom_sid_split_rid(tmp_ctx, account_sid, &domain_sid, NULL);
 	if (!NT_STATUS_IS_OK(status)) {
-		talloc_free(user_info_dc);
+		TALLOC_FREE(user_info_dc);
 		return status;
 	}
 
-	sids[PRIMARY_USER_SID_INDEX] = *account_sid;
-	sids[PRIMARY_GROUP_SID_INDEX] = *domain_sid;
-	sid_append_rid(&sids[PRIMARY_GROUP_SID_INDEX], ldb_msg_find_attr_as_uint(msg, "primaryGroupID", ~0));
+	primary_group_sid = dom_sid_dup(tmp_ctx, domain_sid);
+	if (primary_group_sid == NULL) {
+		TALLOC_FREE(user_info_dc);
+		return NT_STATUS_NO_MEMORY;
+	}
+
+	sid_append_rid(primary_group_sid,
+		       ldb_msg_find_attr_as_uint(msg, "primaryGroupID", ~0));
 
 	/*
 	 * Filter out builtin groups from this token. We will search
@@ -407,7 +405,7 @@ _PUBLIC_ NTSTATUS authsam_make_user_info_dc(TALLOC_CTX *mem_ctx,
 	primary_group_dn = talloc_asprintf(
 		tmp_ctx,
 		"<SID=%s>",
-		dom_sid_str_buf(&sids[PRIMARY_GROUP_SID_INDEX], &buf));
+		dom_sid_str_buf(primary_group_sid, &buf));
 	if (primary_group_dn == NULL) {
 		TALLOC_FREE(user_info_dc);
 		return NT_STATUS_NO_MEMORY;
@@ -420,11 +418,8 @@ _PUBLIC_ NTSTATUS authsam_make_user_info_dc(TALLOC_CTX *mem_ctx,
 	 * <SID=S-...> format of DN and then let it expand
 	 * them, as long as they meet the filter - so only
 	 * domain groups, not builtin groups
-	 *
-	 * The primary group is still treated specially, so we set the
-	 * 'only childs' flag to true
 	 */
-	status = dsdb_expand_nested_groups(sam_ctx, &primary_group_blob, true, filter,
+	status = dsdb_expand_nested_groups(sam_ctx, &primary_group_blob, false, filter,
 					   user_info_dc, &sids, &num_sids);
 	if (!NT_STATUS_IS_OK(status)) {
 		talloc_free(user_info_dc);
@@ -445,8 +440,20 @@ _PUBLIC_ NTSTATUS authsam_make_user_info_dc(TALLOC_CTX *mem_ctx,
 		}
 	}
 
-	user_info_dc->sids = sids;
-	user_info_dc->num_sids = num_sids;
+	user_info_dc->sids = talloc_array(user_info_dc, struct dom_sid,
+							num_sids + 2);
+	if (user_info_dc->sids == NULL) {
+		TALLOC_FREE(user_info_dc);
+		return NT_STATUS_NO_MEMORY;
+        }
+	user_info_dc->num_sids = num_sids + 2;
+	for (i = 0; i < num_sids; i++) {
+		user_info_dc->sids[i + 2] = sids[i];
+	}
+	TALLOC_FREE(sids);
+
+	user_info_dc->sids[PRIMARY_USER_SID_INDEX] = *account_sid;
+	user_info_dc->sids[PRIMARY_GROUP_SID_INDEX] = *primary_group_sid;
 
 	user_info_dc->info = info = talloc_zero(user_info_dc, struct auth_user_info);
 	NT_STATUS_HAVE_NO_MEMORY(user_info_dc->info);
-- 
2.14.4



More information about the samba-technical mailing list