libwbclient: duplicate primary group in returned sids array (was github PR #102)
Isaac Boukris
iboukris at gmail.com
Sat Jan 19 05:49:41 UTC 2019
On Thu, Jan 17, 2019 at 3:34 AM Isaac Boukris <iboukris at gmail.com> wrote:
>
> I'll try to add a new test that checks that the DC adds the primary
> group to both places, and another one that wbclient ignores the second
> reference, and post again.
Patch attached, CI running at:
https://gitlab.com/samba-team/samba/merge_requests/200
-------------- 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/3] 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.18.1
From 7111fd562ebb94444931a8d0a81608e5a4351da1 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/3] DC: Add PrimaryGroupId to rid array in user-info response
This is required for both PAC token and SamLogon response.
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 | 43 +++++++++++++++++++-------------
source4/torture/rpc/remote_pac.c | 12 +++++++++
2 files changed, 37 insertions(+), 18 deletions(-)
diff --git a/source4/auth/sam.c b/source4/auth/sam.c
index 6253ad0445c..8997dd77899 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);
@@ -389,9 +382,14 @@ _PUBLIC_ NTSTATUS authsam_make_user_info_dc(TALLOC_CTX *mem_ctx,
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);
diff --git a/source4/torture/rpc/remote_pac.c b/source4/torture/rpc/remote_pac.c
index ab10013356b..bd587191124 100644
--- a/source4/torture/rpc/remote_pac.c
+++ b/source4/torture/rpc/remote_pac.c
@@ -893,6 +893,18 @@ static bool test_S2U4Self(struct torture_context *tctx,
torture_assert(tctx, !dom_sid_in_domain(builtin_domain, &netlogon_user_info_dc->sids[i]), "Returned BUILTIN domian in groups from NETLOGON SamLogon reply");
}
+ /* Check that PrimaryGroupId is included in rid array of DC response */
+ torture_assert(tctx, netlogon_user_info_dc->num_sids > 2,
+ "Expecting at least three SIDs (uid and gid twice)");
+ for (i = 2; i < netlogon_user_info_dc->num_sids; i++) {
+ if (dom_sid_equal(&netlogon_user_info_dc->sids[1],
+ &netlogon_user_info_dc->sids[i])) {
+ break;
+ }
+ torture_assert(tctx, i != (netlogon_user_info_dc->num_sids -1),
+ "PrimaryGroupId not included in rid array");
+ }
+
return true;
}
--
2.18.1
From 6fd972118b9c1fcbc28d6b95950a06ff4b19933f Mon Sep 17 00:00:00 2001
From: Isaac Boukris <iboukris at gmail.com>
Date: Mon, 18 Sep 2017 19:16:21 +0300
Subject: [PATCH 3/3] wbclient: skip duplicate primary group in returned sids
array
When we copy the SIDs from info3 struct of winbind response
to wbcAuthUserInfo.sids array, skip the primary group to avoid
a duplicate reference.
Signed-off-by: Isaac Boukris <iboukris at gmail.com>
---
nsswitch/libwbclient/tests/wbclient.c | 19 +++++++++++++++++++
nsswitch/libwbclient/wbc_pam.c | 5 +++++
2 files changed, 24 insertions(+)
diff --git a/nsswitch/libwbclient/tests/wbclient.c b/nsswitch/libwbclient/tests/wbclient.c
index d10794297b8..0cc072fa03e 100644
--- a/nsswitch/libwbclient/tests/wbclient.c
+++ b/nsswitch/libwbclient/tests/wbclient.c
@@ -753,6 +753,9 @@ static bool test_wbc_authenticate_user_int(struct torture_context *tctx,
struct wbcAuthUserParams params;
struct wbcAuthUserInfo *info = NULL;
struct wbcAuthErrorInfo *error = NULL;
+ char first[WBC_SID_STRING_BUFLEN];
+ char second[WBC_SID_STRING_BUFLEN];
+ uint32_t i, j;
wbcErr ret;
ret = wbcAuthenticateUser(cli_credentials_get_username(
@@ -770,6 +773,22 @@ static bool test_wbc_authenticate_user_int(struct torture_context *tctx,
ret = wbcAuthenticateUserEx(¶ms, &info, &error);
torture_assert_wbc_equal(tctx, ret, WBC_ERR_SUCCESS,
"wbcAuthenticateUserEx of %s failed", params.account_name);
+
+ for (i = 0; i < info->num_sids; i++) {
+ torture_assert(tctx, WBC_SID_STRING_BUFLEN >
+ wbcSidToStringBuf(&info->sids[i].sid,
+ first, sizeof(first)),
+ "wbcSidToStringBuf()");
+ for (j = i + 1; j < info->num_sids; j++) {
+ torture_assert(tctx, WBC_SID_STRING_BUFLEN >
+ wbcSidToStringBuf(&info->sids[j].sid,
+ second, sizeof(second)),
+ "wbcSidToStringBuf()");
+ torture_assert(tctx, strcmp(first, second) != 0,
+ "Duplicate SID in info->sids array");
+ }
+ }
+
wbcFreeMemory(info);
info = NULL;
diff --git a/nsswitch/libwbclient/wbc_pam.c b/nsswitch/libwbclient/wbc_pam.c
index e4cd2963012..48795b9e0f4 100644
--- a/nsswitch/libwbclient/wbc_pam.c
+++ b/nsswitch/libwbclient/wbc_pam.c
@@ -197,6 +197,11 @@ static wbcErr wbc_create_auth_info(const struct winbindd_response *resp,
BAIL_ON_WBC_ERROR(wbc_status);
}
+ /* Skip primary group, already added */
+ if (rid == resp->data.auth.info3.group_rid) {
+ continue;
+ }
+
if (!sid_attr_compose(&i->sids[sn], &domain_sid,
rid, attrs)) {
wbc_status = WBC_ERR_INVALID_SID;
--
2.18.1
More information about the samba-technical
mailing list