From 47926cc5769dd02ef017ec43030db98a5765754b Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Thu, 11 Jan 2018 09:23:05 +0100 Subject: [PATCH 1/4] s3:winbindd: Improve logic so it is easier to understand BUG: https://bugzilla.samba.org/show_bug.cgi?id=13209 Signed-off-by: Andreas Schneider Reviewed-by: Ralph Boehme (cherry picked from commit 264249db0f5515d8333d16218f1553ae9f0e7193) --- source3/winbindd/winbindd_pam.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/source3/winbindd/winbindd_pam.c b/source3/winbindd/winbindd_pam.c index de3e3f5cc81..f60a90ceb18 100644 --- a/source3/winbindd/winbindd_pam.c +++ b/source3/winbindd/winbindd_pam.c @@ -1683,22 +1683,24 @@ static NTSTATUS winbindd_dual_pam_auth_samlogon( true, /* interactive */ &authoritative, &info3); - if (NT_STATUS_IS_OK(result)) { - result = map_info3_to_validation(mem_ctx, - info3, - &validation_level, - &validation); - TALLOC_FREE(info3); - if (!NT_STATUS_IS_OK(result)) { - goto done; - } - } /* * We need to try the remote NETLOGON server if this is * not authoritative (for example on the RODC). */ if (authoritative != 0) { + if (NT_STATUS_IS_OK(result)) { + result = map_info3_to_validation( + mem_ctx, + info3, + &validation_level, + &validation); + TALLOC_FREE(info3); + if (!NT_STATUS_IS_OK(result)) { + goto done; + } + } + goto done; } } -- 2.15.1 From ac546ddbac286cfd3d94a1b31acd4e4dcd747a63 Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Thu, 11 Jan 2018 09:27:50 +0100 Subject: [PATCH 2/4] s3:winbind: Use a goto for cleaning up at the end BUG: https://bugzilla.samba.org/show_bug.cgi?id=13209 Signed-off-by: Andreas Schneider Reviewed-by: Ralph Boehme (cherry picked from commit 00d176c6c592af59cc14271de4af1614578090a3) --- source3/winbindd/winbindd_pam.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/source3/winbindd/winbindd_pam.c b/source3/winbindd/winbindd_pam.c index f60a90ceb18..c7d2ca434fc 100644 --- a/source3/winbindd/winbindd_pam.c +++ b/source3/winbindd/winbindd_pam.c @@ -889,14 +889,14 @@ NTSTATUS append_auth_data(TALLOC_CTX *mem_ctx, const char *name_user) { struct netr_SamInfo3 *info3 = NULL; - NTSTATUS result; + NTSTATUS result = NT_STATUS_UNSUCCESSFUL; result = map_validation_to_info3(talloc_tos(), validation_level, validation, &info3); if (!NT_STATUS_IS_OK(result)) { - return result; + goto out; } if (request_flags & WBFLAG_PAM_USER_SESSION_KEY) { @@ -919,8 +919,7 @@ NTSTATUS append_auth_data(TALLOC_CTX *mem_ctx, if (!NT_STATUS_IS_OK(result)) { DEBUG(10,("Failed to append Unix Username: %s\n", nt_errstr(result))); - TALLOC_FREE(info3); - return result; + goto out; } } @@ -931,8 +930,7 @@ NTSTATUS append_auth_data(TALLOC_CTX *mem_ctx, if (!NT_STATUS_IS_OK(result)) { DEBUG(10,("Failed to append INFO3 (NDR): %s\n", nt_errstr(result))); - TALLOC_FREE(info3); - return result; + goto out; } } @@ -943,8 +941,7 @@ NTSTATUS append_auth_data(TALLOC_CTX *mem_ctx, if (!NT_STATUS_IS_OK(result)) { DEBUG(10,("Failed to append INFO3 (TXT): %s\n", nt_errstr(result))); - TALLOC_FREE(info3); - return result; + goto out; } } @@ -954,13 +951,14 @@ NTSTATUS append_auth_data(TALLOC_CTX *mem_ctx, if (!NT_STATUS_IS_OK(result)) { DEBUG(10,("Failed to append AFS token: %s\n", nt_errstr(result))); - TALLOC_FREE(info3); - return result; + goto out; } } + result = NT_STATUS_OK; +out: TALLOC_FREE(info3); - return NT_STATUS_OK; + return result; } static NTSTATUS winbindd_dual_pam_auth_cached(struct winbindd_domain *domain, -- 2.15.1 From a3c4fd31a3218be82e0c6b312217dd9c04b4b719 Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Thu, 11 Jan 2018 09:37:22 +0100 Subject: [PATCH 3/4] s3:winbind: Use a stackframe and cleanup when leaving BUG: https://bugzilla.samba.org/show_bug.cgi?id=13209 Signed-off-by: Andreas Schneider Reviewed-by: Ralph Boehme (cherry picked from commit bfc727f0b2d837a97fc9eb94a8811f23a656c4e4) --- source3/winbindd/winbindd_pam.c | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/source3/winbindd/winbindd_pam.c b/source3/winbindd/winbindd_pam.c index c7d2ca434fc..01d08c1e2b3 100644 --- a/source3/winbindd/winbindd_pam.c +++ b/source3/winbindd/winbindd_pam.c @@ -56,16 +56,17 @@ static NTSTATUS append_info3_as_txt(TALLOC_CTX *mem_ctx, union netr_Validation *validation) { struct netr_SamInfo3 *info3 = NULL; - char *ex; + char *ex = NULL; uint32_t i; - NTSTATUS status; + NTSTATUS status = NT_STATUS_UNSUCCESSFUL; + TALLOC_CTX *frame = talloc_stackframe(); - status = map_validation_to_info3(talloc_tos(), + status = map_validation_to_info3(frame, validation_level, validation, &info3); if (!NT_STATUS_IS_OK(status)) { - return status; + goto out; } resp->data.auth.info3.logon_time = @@ -120,10 +121,10 @@ static NTSTATUS append_info3_as_txt(TALLOC_CTX *mem_ctx, validation->sam6->principal_name.string); } - ex = talloc_strdup(mem_ctx, ""); + ex = talloc_strdup(frame, ""); if (ex == NULL) { - TALLOC_FREE(info3); - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto out; } for (i=0; i < info3->base.groups.count; i++) { @@ -131,36 +132,36 @@ static NTSTATUS append_info3_as_txt(TALLOC_CTX *mem_ctx, info3->base.groups.rids[i].rid, info3->base.groups.rids[i].attributes); if (ex == NULL) { - TALLOC_FREE(info3); - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto out; } } for (i=0; i < info3->sidcount; i++) { char *sid; - sid = dom_sid_string(mem_ctx, info3->sids[i].sid); + sid = dom_sid_string(frame, info3->sids[i].sid); if (sid == NULL) { - TALLOC_FREE(info3); - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto out; } ex = talloc_asprintf_append_buffer(ex, "%s:0x%08X\n", sid, info3->sids[i].attributes); if (ex == NULL) { - TALLOC_FREE(info3); - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto out; } - - talloc_free(sid); } - resp->extra_data.data = ex; resp->length += talloc_get_size(ex); + resp->extra_data.data = talloc_move(mem_ctx, &ex); - TALLOC_FREE(info3); - return NT_STATUS_OK; + status = NT_STATUS_OK; +out: + TALLOC_FREE(frame); + return status; } static NTSTATUS append_info3_as_ndr(TALLOC_CTX *mem_ctx, -- 2.15.1 From 622f1e360d8840851fd955202f13994785ed1e0b Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Thu, 11 Jan 2018 09:06:31 +0100 Subject: [PATCH 4/4] s3:rpc_client: Clenup copy_netr_SamInfo3() code This gets rid of some strange macro and makes sure we clenaup at the end. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13209 Signed-off-by: Andreas Schneider Reviewed-by: Ralph Boehme (cherry picked from commit 05ebafd91ee2dd511372ce63d656e9fc6735ee28) --- source3/auth/auth_util.c | 14 ++++--- source3/auth/server_info.c | 45 +++++++++++++-------- source3/rpc_client/util_netlogon.c | 80 ++++++++++++++++++++++---------------- source3/rpc_client/util_netlogon.h | 5 ++- source3/winbindd/winbindd_pam.c | 9 +++-- 5 files changed, 92 insertions(+), 61 deletions(-) diff --git a/source3/auth/auth_util.c b/source3/auth/auth_util.c index 5bb5a69dfa7..f543b33eead 100644 --- a/source3/auth/auth_util.c +++ b/source3/auth/auth_util.c @@ -1008,6 +1008,7 @@ static struct auth_serversupplied_info *copy_session_info_serverinfo_guest(TALLO struct auth_serversupplied_info *server_info) { struct auth_serversupplied_info *dst; + NTSTATUS status; dst = make_server_info(mem_ctx); if (dst == NULL) { @@ -1055,8 +1056,10 @@ static struct auth_serversupplied_info *copy_session_info_serverinfo_guest(TALLO dst->lm_session_key = data_blob_talloc(dst, src->session_key.data, src->session_key.length); - dst->info3 = copy_netr_SamInfo3(dst, server_info->info3); - if (!dst->info3) { + status = copy_netr_SamInfo3(dst, + server_info->info3, + &dst->info3); + if (!NT_STATUS_IS_OK(status)) { TALLOC_FREE(dst); return NULL; } @@ -1433,9 +1436,10 @@ NTSTATUS make_server_info_info3(TALLOC_CTX *mem_ctx, result->unix_name = talloc_strdup(result, found_username); /* copy in the info3 */ - result->info3 = copy_netr_SamInfo3(result, info3); - if (result->info3 == NULL) { - nt_status = NT_STATUS_NO_MEMORY; + nt_status = copy_netr_SamInfo3(result, + info3, + &result->info3); + if (!NT_STATUS_IS_OK(nt_status)) { goto out; } diff --git a/source3/auth/server_info.c b/source3/auth/server_info.c index 20d43d237fa..78981751286 100644 --- a/source3/auth/server_info.c +++ b/source3/auth/server_info.c @@ -63,11 +63,14 @@ struct auth_serversupplied_info *make_server_info(TALLOC_CTX *mem_ctx) NTSTATUS serverinfo_to_SamInfo2(struct auth_serversupplied_info *server_info, struct netr_SamInfo2 *sam2) { - struct netr_SamInfo3 *info3; + struct netr_SamInfo3 *info3 = NULL; + NTSTATUS status; - info3 = copy_netr_SamInfo3(sam2, server_info->info3); - if (!info3) { - return NT_STATUS_NO_MEMORY; + status = copy_netr_SamInfo3(sam2, + server_info->info3, + &info3); + if (!NT_STATUS_IS_OK(status)) { + return status; } if (server_info->session_key.length) { @@ -96,11 +99,14 @@ NTSTATUS serverinfo_to_SamInfo2(struct auth_serversupplied_info *server_info, NTSTATUS serverinfo_to_SamInfo3(const struct auth_serversupplied_info *server_info, struct netr_SamInfo3 *sam3) { - struct netr_SamInfo3 *info3; + struct netr_SamInfo3 *info3 = NULL; + NTSTATUS status; - info3 = copy_netr_SamInfo3(sam3, server_info->info3); - if (!info3) { - return NT_STATUS_NO_MEMORY; + status = copy_netr_SamInfo3(sam3, + server_info->info3, + &info3); + if (!NT_STATUS_IS_OK(status)) { + return status; } if (server_info->session_key.length) { @@ -133,7 +139,8 @@ NTSTATUS serverinfo_to_SamInfo6(struct auth_serversupplied_info *server_info, struct netr_SamInfo6 *sam6) { struct pdb_domain_info *dominfo; - struct netr_SamInfo3 *info3; + struct netr_SamInfo3 *info3 = NULL; + NTSTATUS status; if ((pdb_capabilities() & PDB_CAP_ADS) == 0) { DEBUG(10,("Not adding validation info level 6 " @@ -146,9 +153,11 @@ NTSTATUS serverinfo_to_SamInfo6(struct auth_serversupplied_info *server_info, return NT_STATUS_NO_MEMORY; } - info3 = copy_netr_SamInfo3(sam6, server_info->info3); - if (!info3) { - return NT_STATUS_NO_MEMORY; + status = copy_netr_SamInfo3(sam6, + server_info->info3, + &info3); + if (!NT_STATUS_IS_OK(status)) { + return status; } if (server_info->session_key.length) { @@ -335,11 +344,15 @@ NTSTATUS create_info3_from_pac_logon_info(TALLOC_CTX *mem_ctx, struct netr_SamInfo3 **pp_info3) { NTSTATUS status; - struct netr_SamInfo3 *info3 = copy_netr_SamInfo3(mem_ctx, - &logon_info->info3); - if (info3 == NULL) { - return NT_STATUS_NO_MEMORY; + struct netr_SamInfo3 *info3 = NULL; + + status = copy_netr_SamInfo3(mem_ctx, + &logon_info->info3, + &info3); + if (!NT_STATUS_IS_OK(status)) { + return status; } + status = merge_resource_sids(logon_info, info3); if (!NT_STATUS_IS_OK(status)) { TALLOC_FREE(info3); diff --git a/source3/rpc_client/util_netlogon.c b/source3/rpc_client/util_netlogon.c index ac804f84196..15c769ffe41 100644 --- a/source3/rpc_client/util_netlogon.c +++ b/source3/rpc_client/util_netlogon.c @@ -62,45 +62,52 @@ NTSTATUS copy_netr_SamBaseInfo(TALLOC_CTX *mem_ctx, return NT_STATUS_OK; } -#undef RET_NOMEM - -#define RET_NOMEM(ptr) do { \ - if (!ptr) { \ - TALLOC_FREE(info3); \ - return NULL; \ - } } while(0) - -struct netr_SamInfo3 *copy_netr_SamInfo3(TALLOC_CTX *mem_ctx, - const struct netr_SamInfo3 *orig) +NTSTATUS copy_netr_SamInfo3(TALLOC_CTX *mem_ctx, + const struct netr_SamInfo3 *in, + struct netr_SamInfo3 **pout) { - struct netr_SamInfo3 *info3; + struct netr_SamInfo3 *info3 = NULL; unsigned int i; - NTSTATUS status; + NTSTATUS status = NT_STATUS_UNSUCCESSFUL; info3 = talloc_zero(mem_ctx, struct netr_SamInfo3); - if (!info3) return NULL; + if (info3 == NULL) { + status = NT_STATUS_NO_MEMORY; + goto out; + } - status = copy_netr_SamBaseInfo(info3, &orig->base, &info3->base); + status = copy_netr_SamBaseInfo(info3, &in->base, &info3->base); if (!NT_STATUS_IS_OK(status)) { - TALLOC_FREE(info3); - return NULL; + goto out; } - if (orig->sidcount) { - info3->sidcount = orig->sidcount; + if (in->sidcount) { + info3->sidcount = in->sidcount; info3->sids = talloc_array(info3, struct netr_SidAttr, - orig->sidcount); - RET_NOMEM(info3->sids); - for (i = 0; i < orig->sidcount; i++) { + in->sidcount); + if (info3->sids == NULL) { + status = NT_STATUS_NO_MEMORY; + goto out; + } + + for (i = 0; i < in->sidcount; i++) { info3->sids[i].sid = dom_sid_dup(info3->sids, - orig->sids[i].sid); - RET_NOMEM(info3->sids[i].sid); - info3->sids[i].attributes = - orig->sids[i].attributes; + in->sids[i].sid); + if (info3->sids[i].sid == NULL) { + status = NT_STATUS_NO_MEMORY; + goto out; + } + info3->sids[i].attributes = in->sids[i].attributes; } } - return info3; + *pout = info3; + info3 = NULL; + + status = NT_STATUS_OK; +out: + TALLOC_FREE(info3); + return status; } NTSTATUS map_validation_to_info3(TALLOC_CTX *mem_ctx, @@ -108,7 +115,7 @@ NTSTATUS map_validation_to_info3(TALLOC_CTX *mem_ctx, union netr_Validation *validation, struct netr_SamInfo3 **info3_p) { - struct netr_SamInfo3 *info3; + struct netr_SamInfo3 *info3 = NULL; struct netr_SamInfo6 *info6 = NULL; NTSTATUS status; @@ -122,10 +129,13 @@ NTSTATUS map_validation_to_info3(TALLOC_CTX *mem_ctx, return NT_STATUS_INVALID_PARAMETER; } - info3 = copy_netr_SamInfo3(mem_ctx, validation->sam3); - if (info3 == NULL) { - return NT_STATUS_NO_MEMORY; + status = copy_netr_SamInfo3(mem_ctx, + validation->sam3, + &info3); + if (!NT_STATUS_IS_OK(status)) { + return status; } + break; case 6: if (validation->sam6 == NULL) { @@ -186,16 +196,18 @@ NTSTATUS map_info3_to_validation(TALLOC_CTX *mem_ctx, union netr_Validation **_validation) { union netr_Validation *validation = NULL; + NTSTATUS status; validation = talloc_zero(mem_ctx, union netr_Validation); if (validation == NULL) { return NT_STATUS_NO_MEMORY; } - validation->sam3 = copy_netr_SamInfo3(mem_ctx, info3); - if (validation->sam3 == NULL) { - TALLOC_FREE(validation); - return NT_STATUS_NO_MEMORY; + status = copy_netr_SamInfo3(mem_ctx, + info3, + &validation->sam3); + if (!NT_STATUS_IS_OK(status)) { + return status; } * _validation_level = 3; diff --git a/source3/rpc_client/util_netlogon.h b/source3/rpc_client/util_netlogon.h index 80c7bff99d1..8b3a372ee8e 100644 --- a/source3/rpc_client/util_netlogon.h +++ b/source3/rpc_client/util_netlogon.h @@ -25,8 +25,9 @@ NTSTATUS copy_netr_SamBaseInfo(TALLOC_CTX *mem_ctx, const struct netr_SamBaseInfo *in, struct netr_SamBaseInfo *out); -struct netr_SamInfo3 *copy_netr_SamInfo3(TALLOC_CTX *mem_ctx, - const struct netr_SamInfo3 *orig); +NTSTATUS copy_netr_SamInfo3(TALLOC_CTX *mem_ctx, + const struct netr_SamInfo3 *in, + struct netr_SamInfo3 **pout); NTSTATUS map_validation_to_info3(TALLOC_CTX *mem_ctx, uint16_t validation_level, union netr_Validation *validation, diff --git a/source3/winbindd/winbindd_pam.c b/source3/winbindd/winbindd_pam.c index 01d08c1e2b3..9a61cd3a578 100644 --- a/source3/winbindd/winbindd_pam.c +++ b/source3/winbindd/winbindd_pam.c @@ -2916,10 +2916,11 @@ NTSTATUS winbindd_pam_auth_pac_send(struct winbindd_cli_state *state, * returning a copy talloc'ed off * the state->mem_ctx. */ - info3_copy = copy_netr_SamInfo3(state->mem_ctx, - &logon_info->info3); - if (info3_copy == NULL) { - return NT_STATUS_NO_MEMORY; + result = copy_netr_SamInfo3(state->mem_ctx, + &logon_info->info3, + &info3_copy); + if (!NT_STATUS_IS_OK(result)) { + return result; } } } -- 2.15.1