From 0bdfb5a5e60df214c088df0782c4a1bcc2a4944a Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 16 Aug 2022 13:51:27 -0700 Subject: [PATCH] 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 --- 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; }