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 dac4a1fa92721c638c1ba1b4153b3a5fa3313755..762844502e556eab01099604eeb34bb475b8bf40 100644 (file)
--- 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 157ce1bff278edc5b4d9e973222ed741db18990b..36901776b98b11568b5b16d461bada50bc9bd98d 100644 (file)
--- 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 6b7a2c3be6a9ecc3c15dac2d2af58ec50d40b984..d5c4e8e1f4f4833d3f4a6bd36646aba3aaccce60 100644 (file)
--- 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;
        }