From 1225359d07c99725f694928039834d56ffb31f93 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 16 Aug 2022 13:51:27 -0700 Subject: [PATCH 1/5] 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 6cd3b26535d0fd1560fa34dd9028db54994ee34b Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 22 Jul 2022 16:28:03 +0100 Subject: [PATCH 2/5] 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 From 7fedaa08560eec62e21ae41baa32ff537d52a0f7 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 17 Aug 2022 11:35:29 -0700 Subject: [PATCH 3/5] s3: smbd: Add "enum file_close_type close_type" parameter to close_cnum(). Not yet used, but needed so we can differentiate between SHUTDOWN_CLOSE and ERROR_CLOSE in smbXsrv_tcon_disconnect() if we fail to chdir. In that case we want to close the fd, but not run any delete-on-close actions. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15128 Signed-off-by: Jeremy Allison Reviewed-by: Noel Power (cherry picked from commit 9203d17106c0e55a30813ff1ed76869c7581a343) --- source3/smbd/proto.h | 4 +++- source3/smbd/smb2_service.c | 4 +++- source3/smbd/smbXsrv_tcon.c | 4 ++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h index 5ac0f713958..b8d0a524061 100644 --- a/source3/smbd/proto.h +++ b/source3/smbd/proto.h @@ -1053,7 +1053,9 @@ NTSTATUS make_connection_snum(struct smbXsrv_connection *xconn, int snum, struct smbXsrv_session *session, const char *pdev); -void close_cnum(connection_struct *conn, uint64_t vuid); +void close_cnum(connection_struct *conn, + uint64_t vuid, + enum file_close_type close_type); /* The following definitions come from smbd/session.c */ struct sessionid; diff --git a/source3/smbd/smb2_service.c b/source3/smbd/smb2_service.c index e7a38241515..4aa09c966c5 100644 --- a/source3/smbd/smb2_service.c +++ b/source3/smbd/smb2_service.c @@ -932,7 +932,9 @@ connection_struct *make_connection_smb2(struct smbd_smb2_request *req, Close a cnum. ****************************************************************************/ -void close_cnum(connection_struct *conn, uint64_t vuid) +void close_cnum(connection_struct *conn, + uint64_t vuid, + enum file_close_type close_type) { char rootpath[2] = { '/', '\0'}; struct smb_filename root_fname = { .base_name = rootpath }; diff --git a/source3/smbd/smbXsrv_tcon.c b/source3/smbd/smbXsrv_tcon.c index b515b19e88f..8707082edd6 100644 --- a/source3/smbd/smbXsrv_tcon.c +++ b/source3/smbd/smbXsrv_tcon.c @@ -921,12 +921,12 @@ NTSTATUS smbXsrv_tcon_disconnect(struct smbXsrv_tcon *tcon, uint64_t vuid) * removed from the linked list * conn->sconn->connections. */ - close_cnum(tcon->compat, vuid); + close_cnum(tcon->compat, vuid, ERROR_CLOSE); tcon->compat = NULL; return status; } - close_cnum(tcon->compat, vuid); + close_cnum(tcon->compat, vuid, SHUTDOWN_CLOSE); tcon->compat = NULL; } -- 2.35.3 From 55ab129425fab7c3c98f8423be7dbdb17c00a4f8 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 17 Aug 2022 11:39:36 -0700 Subject: [PATCH 4/5] s3: smbd: Add "enum file_close_type close_type" parameter to file_close_conn(). Not yet used. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15128 Signed-off-by: Jeremy Allison Reviewed-by: Noel Power (cherry picked from commit 7005a6354df5522d9f665fb30052c458dfc93124) --- source3/smbd/files.c | 2 +- source3/smbd/proto.h | 2 +- source3/smbd/smb2_service.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source3/smbd/files.c b/source3/smbd/files.c index a6c41f2b928..fb9454b015d 100644 --- a/source3/smbd/files.c +++ b/source3/smbd/files.c @@ -1339,7 +1339,7 @@ static struct files_struct *file_close_conn_fn( return NULL; } -void file_close_conn(connection_struct *conn) +void file_close_conn(connection_struct *conn, enum file_close_type close_type) { struct file_close_conn_state state = { .conn = conn }; diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h index b8d0a524061..c4a33014515 100644 --- a/source3/smbd/proto.h +++ b/source3/smbd/proto.h @@ -383,7 +383,7 @@ void fsp_set_gen_id(files_struct *fsp); NTSTATUS file_new(struct smb_request *req, connection_struct *conn, files_struct **result); NTSTATUS fsp_bind_smb(struct files_struct *fsp, struct smb_request *req); -void file_close_conn(connection_struct *conn); +void file_close_conn(connection_struct *conn, enum file_close_type close_type); bool file_init_global(void); bool file_init(struct smbd_server_connection *sconn); void file_close_user(struct smbd_server_connection *sconn, uint64_t vuid); diff --git a/source3/smbd/smb2_service.c b/source3/smbd/smb2_service.c index 4aa09c966c5..5affea6b3e4 100644 --- a/source3/smbd/smb2_service.c +++ b/source3/smbd/smb2_service.c @@ -941,7 +941,7 @@ void close_cnum(connection_struct *conn, const struct loadparm_substitution *lp_sub = loadparm_s3_global_substitution(); - file_close_conn(conn); + file_close_conn(conn, close_type); change_to_root_user(); -- 2.35.3 From 686e7a41f01f85b64f976a08725be8e6341142a5 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 17 Aug 2022 11:43:47 -0700 Subject: [PATCH 5/5] s3: smbd: Plumb close_type parameter through close_file_in_loop(), file_close_conn() Allows close_file_in_loop() to differentiate between SHUTDOWN_CLOSE (previously it only used this close type) and ERROR_CLOSE - called on error from smbXsrv_tcon_disconnect() in the error path. In that case we want to close the fd, but not run any delete-on-close actions. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15128 Signed-off-by: Jeremy Allison Reivewed-by: Noel Power Autobuild-User(master): Noel Power Autobuild-Date(master): Thu Aug 18 14:10:18 UTC 2022 on sn-devel-184 (cherry picked from commit cf5f7b1489930f6d64c3e3512f116ccf286d4605) --- source3/smbd/files.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/source3/smbd/files.c b/source3/smbd/files.c index fb9454b015d..b494a8b789a 100644 --- a/source3/smbd/files.c +++ b/source3/smbd/files.c @@ -1258,7 +1258,8 @@ NTSTATUS parent_pathref(TALLOC_CTX *mem_ctx, return NT_STATUS_OK; } -static bool close_file_in_loop(struct files_struct *fsp) +static bool close_file_in_loop(struct files_struct *fsp, + enum file_close_type close_type) { if (fsp_is_alternate_stream(fsp)) { /* @@ -1276,7 +1277,7 @@ static bool close_file_in_loop(struct files_struct *fsp) fsp->base_fsp->stream_fsp = NULL; fsp->base_fsp = NULL; - close_file_free(NULL, &fsp, SHUTDOWN_CLOSE); + close_file_free(NULL, &fsp, close_type); return NULL; } @@ -1300,7 +1301,7 @@ static bool close_file_in_loop(struct files_struct *fsp) return false; } - close_file_free(NULL, &fsp, SHUTDOWN_CLOSE); + close_file_free(NULL, &fsp, close_type); return true; } @@ -1310,6 +1311,7 @@ static bool close_file_in_loop(struct files_struct *fsp) struct file_close_conn_state { struct connection_struct *conn; + enum file_close_type close_type; bool fsp_left_behind; }; @@ -1331,7 +1333,7 @@ static struct files_struct *file_close_conn_fn( fsp->op->global->durable = false; } - did_close = close_file_in_loop(fsp); + did_close = close_file_in_loop(fsp, state->close_type); if (!did_close) { state->fsp_left_behind = true; } @@ -1341,7 +1343,8 @@ static struct files_struct *file_close_conn_fn( void file_close_conn(connection_struct *conn, enum file_close_type close_type) { - struct file_close_conn_state state = { .conn = conn }; + struct file_close_conn_state state = { .conn = conn, + .close_type = close_type }; files_forall(conn->sconn, file_close_conn_fn, &state); @@ -1427,7 +1430,7 @@ static struct files_struct *file_close_user_fn( return NULL; } - did_close = close_file_in_loop(fsp); + did_close = close_file_in_loop(fsp, SHUTDOWN_CLOSE); if (!did_close) { state->fsp_left_behind = true; } -- 2.35.3