From f46042eea00755ac5e55b305d78f1ec865993b82 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 dedfc3766d516da0fde1e1d2820ef51134e3588e 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 b03a1638f5da9f8e1344a520ebe4312f7cc2c4a3 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) [npower@samba.org Adjusted for 4.15 filename change smb2-service.c -> service.c] --- source3/smbd/proto.h | 4 +++- source3/smbd/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 46d3f2e5156..33d53962e66 100644 --- a/source3/smbd/proto.h +++ b/source3/smbd/proto.h @@ -1127,7 +1127,9 @@ connection_struct *make_connection(struct smb_request *req, const char *service_in, const char *pdev, uint64_t vuid, NTSTATUS *status); -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/service.c b/source3/smbd/service.c index ef7c14d92d0..b07239cc6fe 100644 --- a/source3/smbd/service.c +++ b/source3/smbd/service.c @@ -1111,7 +1111,9 @@ connection_struct *make_connection(struct smb_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 fb7267409e5f41bfe160d2e4ce1a4ba61d7a0780 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) [npower@samba.org Adjusted for 4.15 filename change smb2-service.c -> service.c] --- source3/smbd/files.c | 2 +- source3/smbd/proto.h | 2 +- source3/smbd/service.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source3/smbd/files.c b/source3/smbd/files.c index 196c0008e31..582ca0566cc 100644 --- a/source3/smbd/files.c +++ b/source3/smbd/files.c @@ -755,7 +755,7 @@ NTSTATUS parent_pathref(TALLOC_CTX *mem_ctx, Close all open files for a connection. ****************************************************************************/ -void file_close_conn(connection_struct *conn) +void file_close_conn(connection_struct *conn, enum file_close_type close_type) { files_struct *fsp, *next; diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h index 33d53962e66..d937d3d9615 100644 --- a/source3/smbd/proto.h +++ b/source3/smbd/proto.h @@ -397,7 +397,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/service.c b/source3/smbd/service.c index b07239cc6fe..d4dc81bf986 100644 --- a/source3/smbd/service.c +++ b/source3/smbd/service.c @@ -1120,7 +1120,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 fe65b252c7462e8f9c380586d7f613e3e778d4f3 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) [npower@samba.org Adjusted for 4.15 only file_close_conn needs to differentiate between SHUTDOWN_CLOSE & ERROR_CLOSE] --- source3/smbd/files.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/smbd/files.c b/source3/smbd/files.c index 582ca0566cc..3f90c2e247e 100644 --- a/source3/smbd/files.c +++ b/source3/smbd/files.c @@ -770,7 +770,7 @@ void file_close_conn(connection_struct *conn, enum file_close_type close_type) */ fsp->op->global->durable = false; } - close_file(NULL, fsp, SHUTDOWN_CLOSE); + close_file(NULL, fsp, close_type); } } -- 2.35.3