Attachment 17479 Details for Bug 15128 – patch for v4.17, v4.16, v.15

[patch] patch for v4.17, v4.16, v.15

15128-v4.17-test.patch (text/plain), 7.35 KB, created by Noel Power on 2022-08-17 10:21:56 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Noel Power

Created: 2022-08-17 10:21:56 UTC

Size: 7.35 KB

patch

obsolete

>From 82d8a7b2a16159dd8303ec54089fc2181d590d5a Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Tue, 16 Aug 2022 13:51:27 -0700
>Subject: [PATCH 1/2] 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@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@samba.org>
>Reviewed-by: Noel Power <npower@samba.org>
>(cherry picked from commit 0bdfb5a5e60df214c088df0782c4a1bcc2a4944a)
>---
> source3/smbd/smbXsrv_tcon.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
>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;
> 		}
>-- 
>2.35.3
>
>
>From 6e0fbf1be5e62babed8a8eaa9ae8a335beaf6598 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Fri, 22 Jul 2022 16:28:03 +0100
>Subject: [PATCH 2/2] 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@suse.com>.
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Noel Power <npower@samba.org>
>
>Autobuild-User(master): Noel Power <npower@samba.org>
>Autobuild-Date(master): Wed Aug 17 09:54:06 UTC 2022 on sn-devel-184
>
>(cherry picked from commit f92bacbe216d2d74ea3ccf3fe0df5c1cc9860996)
>---
> source3/smbd/conn.c | 36 +++++++++++++++++++++++++-----------
> 1 file changed, 25 insertions(+), 11 deletions(-)
>
>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);
> }
> 
> /*
>-- 
>2.35.3
>

Flags:

jra: review-

Actions: View

Attachments on bug 15128: 17440 | 17441 | 17442 | 17443 | 17444 | 17457 | 17458 | 17459 | 17478 | 17479 | 17481 | 17482 | 17483