[PATCH] fix use after free in smbd

[Michael Adam]

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.

Thanks - Michael

-------------- next part --------------
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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160317/df8d5a05/signature.sig>