[SCM] Samba Shared Repository - branch master updated

Noel Power npower at samba.org
Wed Aug 17 09:55:02 UTC 2022


The branch, master has been updated
       via  f92bacbe216 s3/smbd: Use after free when iterating smbd_server_connection->connections
       via  0bdfb5a5e60 s3/smbd: Use after free when iterating smbd_server_connection->connections
      from  123f1c07c41 s3:utils remove documentation of -l as alias for --long

https://git.samba.org/?p=samba.git;a=shortlog;h=master


- Log -----------------------------------------------------------------
commit f92bacbe216d2d74ea3ccf3fe0df5c1cc9860996
Author: Jeremy Allison <jra at samba.org>
Date:   Fri Jul 22 16:28:03 2022 +0100

    s3/smbd: Use after free when iterating smbd_server_connection->connections
    
    Change conn_free() to just use a destructor. We now
    catch any other places where we may have forgetten to
    call conn_free() - it's implicit on talloc_free(conn).
    
    BUG: https://bugzilla.samba.org/show_bug.cgi?id=15128
    
    Based on code from Noel Power <noel.power at suse.com>.
    
    Signed-off-by: Jeremy Allison <jra at samba.org>
    Reviewed-by: Noel Power <npower at samba.org>
    
    Autobuild-User(master): Noel Power <npower at samba.org>
    Autobuild-Date(master): Wed Aug 17 09:54:06 UTC 2022 on sn-devel-184

commit 0bdfb5a5e60df214c088df0782c4a1bcc2a4944a
Author: Jeremy Allison <jra at samba.org>
Date:   Tue Aug 16 13:51:27 2022 -0700

    s3/smbd: Use after free when iterating smbd_server_connection->connections
    
    In SMB2 smbd_smb2_tree_connect() we create a new conn struct
    inside make_connection_smb2() then move the ownership to tcon using:
    
            tcon->compat = talloc_move(tcon, &compat_conn);
    
    so the lifetime of tcon->compat is tied directly to tcon.
    
    Inside smbXsrv_tcon_disconnect() we have:
    
     908                 ok = chdir_current_service(tcon->compat);
     909                 if (!ok) {
     910                         status = NT_STATUS_INTERNAL_ERROR;
     911                         DEBUG(0, ("smbXsrv_tcon_disconnect(0x%08x, '%s'): "
     912                                   "chdir_current_service() failed: %s\n",
     913                                   tcon->global->tcon_global_id,
     914                                   tcon->global->share_name,
     915                                   nt_errstr(status)));
     916                         tcon->compat = NULL;
     917                         return status;
     918                 }
     919
     920                 close_cnum(tcon->compat, vuid);
     921                 tcon->compat = NULL;
    
    If chdir_current_service(tcon->compat) fails, we return status without ever having
    called close_cnum(tcon->compat, vuid), leaving the conn pointer left in the linked
    list sconn->connections.
    
    The caller frees tcon and (by ownership) tcon->compat, still leaving the
    freed tcon->compat pointer on the sconn->connections linked list.
    
    When deadtime_fn() fires and walks the sconn->connections list it
    indirects this freed pointer. We must call close_cnum() on error also.
    
    Valgrind trace from Noel Power <noel.power at suse.com> is:
    
    ==6432== Invalid read of size 8
    ==6432==    at 0x52CED3A: conn_lastused_update (conn_idle.c:38)
    ==6432==    by 0x52CEDB1: conn_idle_all (conn_idle.c:54)
    ==6432==    by 0x5329971: deadtime_fn (smb2_process.c:1566)
    ==6432==    by 0x5DA2339: smbd_idle_event_handler (util_event.c:45)
    ==6432==    by 0x685F2F8: tevent_common_invoke_timer_handler (tevent_timed.c:376)
    
    ==6432==  Address 0x19074b88 is 232 bytes inside a block of size 328 free'd
    ==6432==    at 0x4C3451B: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==6432==    by 0x5B38521: _tc_free_internal (talloc.c:1222)
    ==6432==    by 0x5B39463: _tc_free_children_internal (talloc.c:1669)
    ==6432==    by 0x5B38404: _tc_free_internal (talloc.c:1184)
    ==6432==    by 0x5B39463: _tc_free_children_internal (talloc.c:1669)
    ==6432==    by 0x5B38404: _tc_free_internal (talloc.c:1184)
    ==6432==    by 0x5B39463: _tc_free_children_internal (talloc.c:1669)
    ==6432==    by 0x5B38404: _tc_free_internal (talloc.c:1184)
    ==6432==    by 0x5B39463: _tc_free_children_internal (talloc.c:1669)
    ==6432==    by 0x5B38404: _tc_free_internal (talloc.c:1184)
    ==6432==    by 0x5B385C5: _talloc_free_internal (talloc.c:1248)
    ==6432==    by 0x5B3988D: _talloc_free (talloc.c:1792)
    ==6432==    by 0x5349B22: smbd_smb2_flush_send_queue (smb2_server.c:4828)
    
    ==6432==  Block was alloc'd at
    ==6432==    at 0x4C332EF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==6432==    by 0x5B378D9: __talloc_with_prefix (talloc.c:783)
    ==6432==    by 0x5B37A73: __talloc (talloc.c:825)
    ==6432==    by 0x5B37E0C: _talloc_named_const (talloc.c:982)
    ==6432==    by 0x5B3A8ED: _talloc_zero (talloc.c:2421)
    ==6432==    by 0x539873A: conn_new (conn.c:70)
    ==6432==    by 0x532D692: make_connection_smb2 (smb2_service.c:909)
    ==6432==    by 0x5352B5E: smbd_smb2_tree_connect (smb2_tcon.c:344)
    
    https://bugzilla.samba.org/show_bug.cgi?id=15128
    
    Signed-off-by: Jeremy Allison <jra at samba.org>
    Reviewed-by: Noel Power <npower at samba.org>

-----------------------------------------------------------------------

Summary of changes:
 source3/smbd/conn.c         | 36 +++++++++++++++++++++++++-----------
 source3/smbd/smbXsrv_tcon.c |  9 +++++++++
 2 files changed, 34 insertions(+), 11 deletions(-)


Changeset truncated at 500 lines:

diff --git a/source3/smbd/conn.c b/source3/smbd/conn.c
index 044242d5697..776d7af4c12 100644
--- a/source3/smbd/conn.c
+++ b/source3/smbd/conn.c
@@ -24,6 +24,25 @@
 #include "smbd/globals.h"
 #include "lib/util/bitmap.h"
 
+static void conn_free_internal(connection_struct *conn);
+
+/****************************************************************************
+ * Remove a conn struct from conn->sconn->connections
+ * if not already done.
+****************************************************************************/
+
+static int conn_struct_destructor(connection_struct *conn)
+{
+        if (conn->sconn != NULL) {
+		DLIST_REMOVE(conn->sconn->connections, conn);
+		SMB_ASSERT(conn->sconn->num_connections > 0);
+		conn->sconn->num_connections--;
+		conn->sconn = NULL;
+	}
+	conn_free_internal(conn);
+	return 0;
+}
+
 /****************************************************************************
  Return the number of open connections.
 ****************************************************************************/
@@ -115,6 +134,11 @@ connection_struct *conn_new(struct smbd_server_connection *sconn)
 	DLIST_ADD(sconn->connections, conn);
 	sconn->num_connections++;
 
+	/*
+	 * Catches the case where someone forgets to call
+	 * conn_free().
+	 */
+	talloc_set_destructor(conn, conn_struct_destructor);
 	return conn;
 }
 
@@ -212,7 +236,6 @@ static void conn_free_internal(connection_struct *conn)
 	free_namearray(conn->aio_write_behind_list);
 
 	ZERO_STRUCTP(conn);
-	talloc_destroy(conn);
 }
 
 /****************************************************************************
@@ -221,16 +244,7 @@ static void conn_free_internal(connection_struct *conn)
 
 void conn_free(connection_struct *conn)
 {
-	if (conn->sconn == NULL) {
-		conn_free_internal(conn);
-		return;
-	}
-
-	DLIST_REMOVE(conn->sconn->connections, conn);
-	SMB_ASSERT(conn->sconn->num_connections > 0);
-	conn->sconn->num_connections--;
-
-	conn_free_internal(conn);
+	TALLOC_FREE(conn);
 }
 
 /*
diff --git a/source3/smbd/smbXsrv_tcon.c b/source3/smbd/smbXsrv_tcon.c
index 6b105522855..b515b19e88f 100644
--- a/source3/smbd/smbXsrv_tcon.c
+++ b/source3/smbd/smbXsrv_tcon.c
@@ -913,6 +913,15 @@ NTSTATUS smbXsrv_tcon_disconnect(struct smbXsrv_tcon *tcon, uint64_t vuid)
 				  tcon->global->tcon_global_id,
 				  tcon->global->share_name,
 				  nt_errstr(status)));
+			/*
+			 * We must call close_cnum() on
+			 * error, as the caller is going
+			 * to free tcon and tcon->compat
+			 * so we must ensure tcon->compat is
+			 * removed from the linked list
+			 * conn->sconn->connections.
+			 */
+			close_cnum(tcon->compat, vuid);
 			tcon->compat = NULL;
 			return status;
 		}


-- 
Samba Shared Repository



More information about the samba-cvs mailing list