[PATCH] Fixes for winbindd

[Volker Lendecke]

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?

> 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.

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de