[PATCH] Fixes for winbindd

[Andrew Bartlett]

On Fri, 2014-07-25 at 12:19 +0200, Volker Lendecke wrote:
> On Fri, Jul 25, 2014 at 02:41:10PM +1200, Garming Sam wrote:
> > Hi,
> > 
> > During an auto-build, we managed to come across a segfault in
> > winbindd as a result of an RODC DNS update.
> > 
> > We have to ensure that the dns_names parameter is preserved on a
> > long term memory context.
> > 
> > The second patch fixes a use after free error when the first error happens.
> > 
> > 
> > Please review and push.
> > 
> > 
> > Cheers,
> > 
> > 
> > Garming Sam
> 
> > >From 5bc91f9e43f44e0476c1083a8bd5145c094492f8 Mon Sep 17 00:00:00 2001
> > From: Andrew Bartlett <abartlet at samba.org>
> > Date: Thu, 24 Jul 2014 15:54:58 +1200
> > Subject: [PATCH 1/2] libcli/auth: Ensure that the dns_names in/out parameter
> >  is preserved
> > 
> > This is in dcerpc_netr_DsrUpdateReadOnlyServerDnsRecords, which has
> > status variables filled in by the server and placed in this in/out
> > array.
> > 
> > This showed up as a segfault in winbindd during RODC DNS update.
> > 
> > Andrew Bartlett
> > 
> > Signed-off-by: Garming Sam <garming at catalyst.net.nz>
> > Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> > Pair-programmed-with: Garming Sam <garming at catalyst.net.nz>
> > ---
> >  libcli/auth/netlogon_creds_cli.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libcli/auth/netlogon_creds_cli.c b/libcli/auth/netlogon_creds_cli.c
> > index d709e6a..a461dc6 100644
> > --- a/libcli/auth/netlogon_creds_cli.c
> > +++ b/libcli/auth/netlogon_creds_cli.c
> > @@ -2754,7 +2754,14 @@ static void netlogon_creds_cli_DsrUpdateReadOnlyServerDnsRecords_done(struct tev
> >  	NTSTATUS result;
> >  	bool ok;
> >  
> > -	status = dcerpc_netr_DsrUpdateReadOnlyServerDnsRecords_recv(subreq, state,
> > +	/*
> > +	 * We use state->dns_names as the memory context, as this is
> > +	 * the only in/out variable and it has been overwritten by the
> > +	 * out parameter from the server.
> > +	 *
> > +	 * We need to preserve the return value until the caller can use it.
> > +	 */
> > +	status = dcerpc_netr_DsrUpdateReadOnlyServerDnsRecords_recv(subreq, state->dns_names,
> 
> Can you explain that a bit more closely? As far as I can
> see, netlogon_creds_cli_DsrUpdateReadOnlyServerDnsRecords is
> only used in a synchronous fashion. And as "state" lives as
> long as the sync function ponders the request, at least
> inside that function this should not make a difference. What
> is the sequence of talloc_free()'s that leads to the
> segfault you saw?

netlogon_creds_cli_DsrUpdateReadOnlyServerDnsRecords_send() creates
state on mem_ctx, which is frame in
netlogon_creds_cli_DsrUpdateReadOnlyServerDnsRecords()

	struct netlogon_creds_cli_DsrUpdateReadOnlyServerDnsRecords_state *state;
	struct tevent_req *subreq;

	req = tevent_req_create(mem_ctx, &state,
				struct netlogon_creds_cli_DsrUpdateReadOnlyServerDnsRecords_state);
	if (req == NULL) {
		return NULL;
	}

The state->dns_names is an in/out parameter as dns_names to
netlogon_creds_cli_DsrUpdateReadOnlyServerDnsRecords() and has a longer
life time. 

The issue isn't the async, it is that the memory needs to be around
longer than the state variable.

The typical approach of a talloc_steal won't work, due to the way the
ref variable is overwritten.  The struct NL_DNS_NAME_INFO_ARRAY
dns_names is the original input array, but the elements are overwritten
in dcerpc_netr_DsrUpdateReadOnlyServerDnsRecords_done()

> > As found during investigation of the previous commit, when the RPC
> > call fails totally, we must only try and send one error reply.
> > 
> > Andrew Bartlett
> > 
> > Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> > Signed-off-by: Garming Sam <garming at catalyst.net.nz>
> > Pair-programmed-with: Garming Sam <garming at catalyst.net.nz>
> > ---
> >  source3/winbindd/winbindd_irpc.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/source3/winbindd/winbindd_irpc.c b/source3/winbindd/winbindd_irpc.c
> > index cf58a08..aeaea71 100644
> > --- a/source3/winbindd/winbindd_irpc.c
> > +++ b/source3/winbindd/winbindd_irpc.c
> > @@ -51,6 +51,7 @@ static void wb_irpc_forward_callback(struct tevent_req *subreq)
> >  		DEBUG(0,("RPC callback failed for %s - %s\n",
> >  			 opname, nt_errstr(status)));
> >  		irpc_send_reply(st->msg, status);
> > +		return;
> >  	}
> >  
> >  	irpc_send_reply(st->msg, status);
> 
> Reviewed-by: Me.

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba