[PATCH] fix use after free in smbd

[Jeremy Allison]

On Thu, Mar 17, 2016 at 12:59:52AM +0100, Michael Adam wrote:
> Hi,
> 
> Writing a new test case to instrument the
> durable reconnect code more, we observed a
> segfault would occur now and then. The analysis
> showed a use-after free:
> 
> vfs_default_durable_reconnect():
>   fsp_new() ==> this does DLIST_ADD(fsp->conn->sconn->files, fsp)
>   if (fsp->oplock_type == LEASE_OPLOCK) {
>     find_fsp_lease(fsp, &key, l) ==> this fills conn->fsp_fi_cache
>     if (client guids not equal) {
>       fsp_free(fsp) ==> this does DLIST_REMOVE(fsp->conn->sconn->files, fsp)
>   }
> 
> so after this code we have the fsp_fi_cache still pointing to the
> free'd memory. The next call to find_fsp_lease will use the cache
> and hence access the freed memory.
> 
> Attached find a patch to fix this use-after-free.
> 
> The fix consists in invalidating the cache in fsp_free() instead
> of just in its wrapper file_free().
> 
> Review appreciated.

LGTM, pushed as I just created the same patch :-).


> From c51c3717709dfcbff6960513155de8df19276c57 Mon Sep 17 00:00:00 2001
> From: Michael Adam <obnox at samba.org>
> Date: Wed, 16 Mar 2016 23:57:33 +0100
> Subject: [PATCH] smbd: fix use after free via conn->fsp_fi_cache
> 
> Some instrumentation of the the durable reconnect
> code uncovered a problem in the fsp_new, fsp_free pair:
> 
> vfs_default_durable_reconnect():
>   fsp_new() ==> this does DLIST_ADD(fsp->conn->sconn->files, fsp)
>   if (fsp->oplock_type == LEASE_OPLOCK) {
>     find_fsp_lease(fsp, &key, l) ==> this fills conn->fsp_fi_cache
>     if (client guids not equal) {
>       fsp_free(fsp) ==> this does DLIST_REMOVE(fsp->conn->sconn->files, fsp)
>   }
> 
> so after this code we have the fsp_fi_cache still pointing to the
> free'd memory. The next call to find_fsp_lease will use the cache
> and hence access the freed memory.
> 
> The fix consists in invalidating the cache in fsp_free() instead
> of just in its wrapper file_free().
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=11799
> 
> Pair-Programmed-With: Guenther Deschner <gd at samba.org>
> 
> Signed-off-by: Michael Adam <obnox at samba.org>
> Signed-off-by: Guenther Deschner <gd at samba.org>
> ---
>  source3/smbd/files.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/source3/smbd/files.c b/source3/smbd/files.c
> index 8fefddd..3e2b3d7 100644
> --- a/source3/smbd/files.c
> +++ b/source3/smbd/files.c
> @@ -478,6 +478,10 @@ void fsp_free(files_struct *fsp)
>  {
>  	struct smbd_server_connection *sconn = fsp->conn->sconn;
>  
> +	if (fsp == sconn->fsp_fi_cache.fsp) {
> +		ZERO_STRUCT(sconn->fsp_fi_cache);
> +	}
> +
>  	DLIST_REMOVE(sconn->files, fsp);
>  	SMB_ASSERT(sconn->num_files > 0);
>  	sconn->num_files--;
> @@ -540,11 +544,6 @@ void file_free(struct smb_request *req, files_struct *fsp)
>  		remove_smb2_chained_fsp(fsp);
>  	}
>  
> -	/* Closing a file can invalidate the positive cache. */
> -	if (fsp == sconn->fsp_fi_cache.fsp) {
> -		ZERO_STRUCT(sconn->fsp_fi_cache);
> -	}
> -
>  	/* Drop all remaining extensions. */
>  	vfs_remove_all_fsp_extensions(fsp);
>  
> -- 
> 2.5.0
>