From 14411772657c46e5c0bbcae8a6985ebc7d449c3f Mon Sep 17 00:00:00 2001 From: Samuel Cabrero Date: Wed, 13 Apr 2022 11:01:00 +0200 Subject: [PATCH 1/4] s3:winbind: Do not use domain's private data to store the SAMR pipes The domain's private_data pointer is also used to store a ADS_STRUCT, which is not allocated using talloc and there are many places casting this pointer directly. The recently added samba.tests.pam_winbind_setcred was randomly failing and after debugging it the problem was that kerberos authentication was failing because the time_offset passed to kerberos_return_pac() was wrong. This time_offset was retrieved from ads->auth.time_offset, where the ads pointer was directly casted from domain->private_data but private_data was pointing to a winbind_internal_pipes struct. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15046 Signed-off-by: Samuel Cabrero Reviewed-by: Stefan Metzmacher Reviewed-by: Andreas Schneider (cherry picked from commit e1f29b0970f4cac52a9cd517be6862cf69a1433a) --- source3/winbindd/winbindd.h | 6 ++++++ source3/winbindd/winbindd_ndr.c | 3 +++ source3/winbindd/winbindd_samr.c | 23 ++++++++--------------- 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/source3/winbindd/winbindd.h b/source3/winbindd/winbindd.h index dac4a1fa927..762844502e5 100644 --- a/source3/winbindd/winbindd.h +++ b/source3/winbindd/winbindd.h @@ -43,6 +43,8 @@ #define WB_REPLACE_CHAR '_' +struct winbind_internal_pipes; + struct winbindd_cli_state { struct winbindd_cli_state *prev, *next; /* Linked list pointers */ int sock; /* Open socket from client */ @@ -157,6 +159,10 @@ struct winbindd_domain { void *private_data; + struct { + struct winbind_internal_pipes *samr_pipes; + } backend_data; + /* A working DC */ char *dcname; const char *ping_dcname; diff --git a/source3/winbindd/winbindd_ndr.c b/source3/winbindd/winbindd_ndr.c index 157ce1bff27..36901776b98 100644 --- a/source3/winbindd/winbindd_ndr.c +++ b/source3/winbindd/winbindd_ndr.c @@ -144,6 +144,9 @@ void ndr_print_winbindd_domain(struct ndr_print *ndr, ndr_print_bool(ndr, "startup", r->startup); ndr_print_winbindd_methods(ndr, "backend", r->backend); ndr_print_ptr(ndr, "private_data", r->private_data); + ndr_print_ptr(ndr, + "backend_data.samr_pipes", + r->backend_data.samr_pipes); ndr_print_string(ndr, "dcname", r->dcname); ndr_print_sockaddr_storage(ndr, "dcaddr", &r->dcaddr); ndr_print_time_t(ndr, "last_seq_check", r->last_seq_check); diff --git a/source3/winbindd/winbindd_samr.c b/source3/winbindd/winbindd_samr.c index 6b7a2c3be6a..d5c4e8e1f4f 100644 --- a/source3/winbindd/winbindd_samr.c +++ b/source3/winbindd/winbindd_samr.c @@ -131,8 +131,7 @@ static void cached_internal_pipe_close( struct winbindd_domain *domain = talloc_get_type_abort( private_data, struct winbindd_domain); /* - * domain->private_data is the struct winbind_internal_pipes * - * pointer so freeing it closes the cached pipes. + * Freeing samr_pipes closes the cached pipes. * * We can do a hard close because at the time of this commit * we only use sychronous calls to external pipes. So we can't @@ -141,7 +140,7 @@ static void cached_internal_pipe_close( * get nested event loops. Once we start to get async in * winbind children, we need to check for outstanding calls */ - TALLOC_FREE(domain->private_data); + TALLOC_FREE(domain->backend_data.samr_pipes); } static NTSTATUS open_cached_internal_pipe_conn( @@ -153,7 +152,7 @@ static NTSTATUS open_cached_internal_pipe_conn( { struct winbind_internal_pipes *internal_pipes = NULL; - if (domain->private_data == NULL) { + if (domain->backend_data.samr_pipes == NULL) { TALLOC_CTX *frame = talloc_stackframe(); NTSTATUS status; @@ -190,14 +189,14 @@ static NTSTATUS open_cached_internal_pipe_conn( return NT_STATUS_NO_MEMORY; } - domain->private_data = talloc_move(domain, &internal_pipes); + domain->backend_data.samr_pipes = + talloc_move(domain, &internal_pipes); TALLOC_FREE(frame); } - internal_pipes = talloc_get_type_abort( - domain->private_data, struct winbind_internal_pipes); + internal_pipes = domain->backend_data.samr_pipes; if (samr_domain_hnd) { *samr_domain_hnd = internal_pipes->samr_domain_hnd; @@ -226,23 +225,17 @@ static bool reset_connection_on_error(struct winbindd_domain *domain, struct rpc_pipe_client *p, NTSTATUS status) { - struct winbind_internal_pipes *internal_pipes = NULL; struct dcerpc_binding_handle *b = p->binding_handle; - internal_pipes = talloc_get_type_abort( - domain->private_data, struct winbind_internal_pipes); - if (NT_STATUS_EQUAL(status, NT_STATUS_IO_TIMEOUT) || NT_STATUS_EQUAL(status, NT_STATUS_IO_DEVICE_ERROR)) { - TALLOC_FREE(internal_pipes); - domain->private_data = NULL; + TALLOC_FREE(domain->backend_data.samr_pipes); return true; } if (!dcerpc_binding_handle_is_connected(b)) { - TALLOC_FREE(internal_pipes); - domain->private_data = NULL; + TALLOC_FREE(domain->backend_data.samr_pipes); return true; } -- 2.35.1 From e8b968d461f6f5c6b169ccc88b3a507654620b2c Mon Sep 17 00:00:00 2001 From: Samuel Cabrero Date: Wed, 13 Apr 2022 11:15:35 +0200 Subject: [PATCH 2/4] s3:winbind: Simplify open_cached_internal_pipe_conn() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15046 Signed-off-by: Samuel Cabrero Reviewed-by: Stefan Metzmacher Reviewed-by: Andreas Schneider (cherry picked from commit 91395e660a2b1b69bf74ca0b77aee416e2ac1db3) --- source3/winbindd/winbindd_samr.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/source3/winbindd/winbindd_samr.c b/source3/winbindd/winbindd_samr.c index d5c4e8e1f4f..ebf9c24b9e4 100644 --- a/source3/winbindd/winbindd_samr.c +++ b/source3/winbindd/winbindd_samr.c @@ -150,9 +150,10 @@ static NTSTATUS open_cached_internal_pipe_conn( struct rpc_pipe_client **lsa_pipe, struct policy_handle *lsa_hnd) { - struct winbind_internal_pipes *internal_pipes = NULL; + struct winbind_internal_pipes *internal_pipes = + domain->backend_data.samr_pipes; - if (domain->backend_data.samr_pipes == NULL) { + if (internal_pipes == NULL) { TALLOC_CTX *frame = talloc_stackframe(); NTSTATUS status; @@ -190,14 +191,11 @@ static NTSTATUS open_cached_internal_pipe_conn( } domain->backend_data.samr_pipes = - talloc_move(domain, &internal_pipes); + talloc_steal(domain, internal_pipes); TALLOC_FREE(frame); - } - internal_pipes = domain->backend_data.samr_pipes; - if (samr_domain_hnd) { *samr_domain_hnd = internal_pipes->samr_domain_hnd; } -- 2.35.1 From 428bea9f1a55535569c4a8330da1a637320a904c Mon Sep 17 00:00:00 2001 From: Samuel Cabrero Date: Wed, 13 Apr 2022 11:31:45 +0200 Subject: [PATCH 3/4] s3:winbind: Do not use domain's private data to store the ADS_STRUCT The ADS_STRUCT is not allocated using talloc and there are many places casting this pointer directly so use a typed pointer. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15046 Signed-off-by: Samuel Cabrero Reviewed-by: Stefan Metzmacher Reviewed-by: Andreas Schneider (cherry picked from commit 3cb256439e9ceece26c2de82293c43486543e0cb) --- source3/winbindd/winbindd.h | 2 ++ source3/winbindd/winbindd_ads.c | 10 +++++----- source3/winbindd/winbindd_ndr.c | 3 +++ source3/winbindd/winbindd_pam.c | 6 ++---- 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/source3/winbindd/winbindd.h b/source3/winbindd/winbindd.h index 762844502e5..3cc88367b90 100644 --- a/source3/winbindd/winbindd.h +++ b/source3/winbindd/winbindd.h @@ -44,6 +44,7 @@ #define WB_REPLACE_CHAR '_' struct winbind_internal_pipes; +struct ads_struct; struct winbindd_cli_state { struct winbindd_cli_state *prev, *next; /* Linked list pointers */ @@ -161,6 +162,7 @@ struct winbindd_domain { struct { struct winbind_internal_pipes *samr_pipes; + struct ads_struct *ads_conn; } backend_data; /* A working DC */ diff --git a/source3/winbindd/winbindd_ads.c b/source3/winbindd/winbindd_ads.c index 6f01ef6e334..d350f160223 100644 --- a/source3/winbindd/winbindd_ads.c +++ b/source3/winbindd/winbindd_ads.c @@ -269,10 +269,10 @@ static ADS_STRUCT *ads_cached_connection(struct winbindd_domain *domain) } DEBUG(10,("ads_cached_connection\n")); - ads_cached_connection_reuse((ADS_STRUCT **)&domain->private_data); + ads_cached_connection_reuse(&domain->backend_data.ads_conn); - if (domain->private_data) { - return (ADS_STRUCT *)domain->private_data; + if (domain->backend_data.ads_conn != NULL) { + return domain->backend_data.ads_conn; } /* the machine acct password might have change - fetch it every time */ @@ -303,7 +303,7 @@ static ADS_STRUCT *ads_cached_connection(struct winbindd_domain *domain) } status = ads_cached_connection_connect( - (ADS_STRUCT **)&domain->private_data, + &domain->backend_data.ads_conn, domain->alt_name, domain->name, NULL, password, realm, @@ -322,7 +322,7 @@ static ADS_STRUCT *ads_cached_connection(struct winbindd_domain *domain) return NULL; } - return (ADS_STRUCT *)domain->private_data; + return domain->backend_data.ads_conn; } /* Query display info for a realm. This is the basic user list fn */ diff --git a/source3/winbindd/winbindd_ndr.c b/source3/winbindd/winbindd_ndr.c index 36901776b98..94ce9d73747 100644 --- a/source3/winbindd/winbindd_ndr.c +++ b/source3/winbindd/winbindd_ndr.c @@ -147,6 +147,9 @@ void ndr_print_winbindd_domain(struct ndr_print *ndr, ndr_print_ptr(ndr, "backend_data.samr_pipes", r->backend_data.samr_pipes); + ndr_print_ptr(ndr, + "backend_data.ads_conn", + r->backend_data.ads_conn); ndr_print_string(ndr, "dcname", r->dcname); ndr_print_sockaddr_storage(ndr, "dcaddr", &r->dcaddr); ndr_print_time_t(ndr, "last_seq_check", r->last_seq_check); diff --git a/source3/winbindd/winbindd_pam.c b/source3/winbindd/winbindd_pam.c index c2fcc399ab8..84c3720c19f 100644 --- a/source3/winbindd/winbindd_pam.c +++ b/source3/winbindd/winbindd_pam.c @@ -677,7 +677,6 @@ static NTSTATUS winbindd_raw_kerberos_login(TALLOC_CTX *mem_ctx, fstring name_namespace, name_domain, name_user; time_t ticket_lifetime = 0; time_t renewal_until = 0; - ADS_STRUCT *ads; time_t time_offset = 0; const char *user_ccache_file; struct PAC_LOGON_INFO *logon_info = NULL; @@ -716,9 +715,8 @@ static NTSTATUS winbindd_raw_kerberos_login(TALLOC_CTX *mem_ctx, /* 2nd step: * get kerberos properties */ - if (domain->private_data) { - ads = (ADS_STRUCT *)domain->private_data; - time_offset = ads->auth.time_offset; + if (domain->backend_data.ads_conn != NULL) { + time_offset = domain->backend_data.ads_conn->auth.time_offset; } -- 2.35.1 From 8544f6a0f5ba8a1d64a159864f1a2a67f1f69e8b Mon Sep 17 00:00:00 2001 From: Samuel Cabrero Date: Wed, 13 Apr 2022 11:34:18 +0200 Subject: [PATCH 4/4] s3:winbind: Remove no longer used domain's private_data pointer BUG: https://bugzilla.samba.org/show_bug.cgi?id=15046 Signed-off-by: Samuel Cabrero Reviewed-by: Stefan Metzmacher Reviewed-by: Andreas Schneider (cherry picked from commit a6d6ae3cfcd64a85f82ec5b12253ca0e237d95bb) --- source3/winbindd/winbindd.h | 4 ---- source3/winbindd/winbindd_ndr.c | 1 - 2 files changed, 5 deletions(-) diff --git a/source3/winbindd/winbindd.h b/source3/winbindd/winbindd.h index 3cc88367b90..fe286a9a686 100644 --- a/source3/winbindd/winbindd.h +++ b/source3/winbindd/winbindd.h @@ -156,10 +156,6 @@ struct winbindd_domain { */ struct winbindd_methods *backend; - /* Private data for the backends (used for connection cache) */ - - void *private_data; - struct { struct winbind_internal_pipes *samr_pipes; struct ads_struct *ads_conn; diff --git a/source3/winbindd/winbindd_ndr.c b/source3/winbindd/winbindd_ndr.c index 94ce9d73747..b393586a692 100644 --- a/source3/winbindd/winbindd_ndr.c +++ b/source3/winbindd/winbindd_ndr.c @@ -143,7 +143,6 @@ void ndr_print_winbindd_domain(struct ndr_print *ndr, ndr_print_time_t(ndr, "startup_time", r->startup_time); ndr_print_bool(ndr, "startup", r->startup); ndr_print_winbindd_methods(ndr, "backend", r->backend); - ndr_print_ptr(ndr, "private_data", r->private_data); ndr_print_ptr(ndr, "backend_data.samr_pipes", r->backend_data.samr_pipes); -- 2.35.1