From 82d8a7b2a16159dd8303ec54089fc2181d590d5a Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 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 Reviewed-by: Noel Power (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 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 . Signed-off-by: Jeremy Allison Reviewed-by: Noel Power Autobuild-User(master): Noel Power 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