From: Samuel Cabrero <scabrero@samba.org>
Date: Wed, 13 Apr 2022 09:01:00 +0000 (+0200)
Subject: s3:winbind: Do not use domain's private data to store the SAMR pipes
X-Git-Tag: talloc-2.3.4~387
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e1f29b0970f4cac52a9cd517be6862cf69a1433a;p=thirdparty%2Fsamba.git

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 <scabrero@samba.org>
Reviewed-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Andreas Schneider <asn@samba.org>
---

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;
 	}