Created attachment 17988 [details] client to trigger use-after-free in aio_del_req_from_fsp() If an aio_req_fsp_link (lnk) is still outstanding when the smbd server shuts down, talloc can free the files_struct (fsp) that lnk->fsp refers to before calling the destructor aio_del_req_from_fsp() for the lnk, causing a used-after-free. The attached client program causes this on my setup. It negotiates a 3.1.1 connection, opens the IPC$ share, opens the "samr" pipe, and sends an FSCTL_PIPE_TRANSCEIVE request with garbage content. The fsp is allocated in fsp_new() in files.c, called via open_np_file(). The lnk is allocated in aio_add_req_to_fsp() in smb2_aio.c, called from smbd_smb2_request_process_ioctl(). A crash backtrace from the reference to the freed fsp (with a debugging malloc that fills freed blocks with junk): Program received signal SIGBUS, Bus error. Object-specific hardware error. 0x0000000001b5265b in aio_del_req_from_fsp (lnk=0x816515960) at ../../source3/smbd/smb2_aio.c:78 78 if (fsp->aio_requests[i] == req) { (gdb) where #0 0x0000000001b5265b in aio_del_req_from_fsp (lnk=0x816515960) at ../../source3/smbd/smb2_aio.c:78 #1 0x0000000002a39df8 in _tc_free_internal (tc=0x816515900, location=0x1454e15 "../../source3/smbd/server_exit.c:187") at ../../lib/talloc/talloc.c:1161 #2 0x0000000002a36785 in _tc_free_children_internal (tc=0x816513230, ptr=0x816513290, location=0x1454e15 "../../source3/smbd/server_exit.c:187") at ../../lib/talloc/talloc.c:1672 #3 0x0000000002a39fd7 in _tc_free_internal (tc=0x816513230, location=0x1454e15 "../../source3/smbd/server_exit.c:187") at ../../lib/talloc/talloc.c:1187 #4 0x0000000002a36785 in _tc_free_children_internal (tc=0x816512760, ptr=0x8165127c0, location=0x1454e15 "../../source3/smbd/server_exit.c:187") at ../../lib/talloc/talloc.c:1672 #5 0x0000000002a39fd7 in _tc_free_internal (tc=0x816512760, location=0x1454e15 "../../source3/smbd/server_exit.c:187") at ../../lib/talloc/talloc.c:1187 #6 0x0000000002a36785 in _tc_free_children_internal (tc=0x813505080, ptr=0x8135050e0, location=0x1454e15 "../../source3/smbd/server_exit.c:187") at ../../lib/talloc/talloc.c:1672 #7 0x0000000002a39fd7 in _tc_free_internal (tc=0x813505080, location=0x1454e15 "../../source3/smbd/server_exit.c:187") at ../../lib/talloc/talloc.c:1187 #8 0x0000000002a35cbe in _talloc_free_internal (ptr=0x8135050e0, location=0x1454e15 "../../source3/smbd/server_exit.c:187") at ../../lib/talloc/talloc.c:1251 #9 0x0000000002a369df in _talloc_free (ptr=0x8135050e0, location=0x1454e15 "../../source3/smbd/server_exit.c:187") at ../../lib/talloc/talloc.c:1795 #10 0x0000000001baf58d in exit_server_common (how=SERVER_EXIT_NORMAL, reason=0x15086cd "NT_STATUS_END_OF_FILE") at ../../source3/smbd/server_exit.c:187 #11 0x0000000001baf7b7 in smbd_exit_server_cleanly ( explanation=0x15086cd "NT_STATUS_END_OF_FILE") at ../../source3/smbd/server_exit.c:246 #12 0x0000000001c37307 in exit_server_cleanly ( reason=0x15086cd "NT_STATUS_END_OF_FILE") at ../../source3/lib/smbd_shim.c:113 #13 0x0000000001b557a6 in smbd_server_connection_terminate_ex ( xconn=0x8135050e0, reason=0x15086cd "NT_STATUS_END_OF_FILE", location=0x1497d9d "../../source3/smbd/smb2_server.c:5141") at ../../source3/smbd/smb2_server.c:1794 #14 0x0000000001b63989 in smbd_smb2_connection_handler (ev=0x80413b490, fde=0x813508800, flags=1, private_data=0x8135050e0) at ../../source3/smbd/smb2_server.c:5141 #15 0x0000000001f383b6 in tevent_common_invoke_fd_handler (fde=0x813508800, flags=1, removed=0x0) at ../../lib/tevent/tevent_fd.c:142 #16 0x0000000001f3ddda in poll_event_loop_poll (ev=0x80413b490, tvalp=0x7fffffffdfb8) at ../../lib/tevent/tevent_poll.c:569 #17 0x0000000001f3d654 in poll_event_loop_once (ev=0x80413b490, location=0x153d27b "../../source3/smbd/smb2_process.c:2020") at ../../lib/tevent/tevent_poll.c:626 #18 0x0000000001f367a4 in _tevent_loop_once (ev=0x80413b490, location=0x153d27b "../../source3/smbd/smb2_process.c:2020") at ../../lib/tevent/tevent.c:823 #19 0x0000000001f36ba3 in tevent_common_loop_wait (ev=0x80413b490, location=0x153d27b "../../source3/smbd/smb2_process.c:2020") at ../../lib/tevent/tevent.c:949 #20 0x0000000001f36c45 in _tevent_loop_wait (ev=0x80413b490, location=0x153d27b "../../source3/smbd/smb2_process.c:2020") at ../../lib/tevent/tevent.c:968 #21 0x0000000001b40590 in smbd_process (ev_ctx=0x80413b490, msg_ctx=0x80413bb20, sock_fd=26, interactive=true) at ../../source3/smbd/smb2_process.c:2020 #22 0x0000000001dbe958 in smbd_accept_connection (ev=0x80413b490, fde=0x813500850, flags=1, private_data=0x813500580) at ../../source3/smbd/server.c:978 #23 0x0000000001f383b6 in tevent_common_invoke_fd_handler (fde=0x813500850, flags=1, removed=0x0) at ../../lib/tevent/tevent_fd.c:142 #24 0x0000000001f3ddda in poll_event_loop_poll (ev=0x80413b490, tvalp=0x7fffffffe478) at ../../lib/tevent/tevent_poll.c:569 #25 0x0000000001f3d654 in poll_event_loop_once (ev=0x80413b490, location=0x149bfd0 "../../source3/smbd/server.c:1373") at ../../lib/tevent/tevent_poll.c:626 #26 0x0000000001f367a4 in _tevent_loop_once (ev=0x80413b490, location=0x149bfd0 "../../source3/smbd/server.c:1373") at ../../lib/tevent/tevent.c:823 #27 0x0000000001f36ba3 in tevent_common_loop_wait (ev=0x80413b490, location=0x149bfd0 "../../source3/smbd/server.c:1373") at ../../lib/tevent/tevent.c:949 #28 0x0000000001f36c45 in _tevent_loop_wait (ev=0x80413b490, location=0x149bfd0 "../../source3/smbd/server.c:1373") at ../../lib/tevent/tevent.c:968 #29 0x0000000001dbbd76 in smbd_parent_loop (ev_ctx=0x80413b490, parent=0x80414d190) at ../../source3/smbd/server.c:1373 #30 0x0000000001db9824 in main (argc=2, argv=0x7fffffffea70) at ../../source3/smbd/server.c:2130
Hmm. Of course smbXsrv_session_logoff_all() inside exit_server_common() should have taken care of this.. exit_server_common() -> smbXsrv_session_logoff_all() -> dbwrap_traverse() -> smbXsrv_session_logoff_all_callback() -> smbXsrv_session_clear_and_logoff() -> file_close_user() -> smb2srv_tcon_disconnect_all() -> smbXsrv_tcon_disconnect_all() -> dbwrap_traverse() -> smbXsrv_tcon_disconnect_all_callback() -> smbXsrv_tcon_disconnect() -> close_cnum(tcon->compat, vuid, SHUTDOWN_CLOSE); -> file_close_conn(conn, close_type); -> file_close_conn_fn() -> close_file_in_loop() -> close_file_free() Needs further investigation.
(In reply to Jeremy Allison from comment #1) If you tell me more exactly what smbXsrv_session_logoff_all() &c should have done in this case, I will try to see whether that's happening in my situation.
Looking into this one now.
Valgrind trace: ==116781== Invalid read of size 4 ==116781== at 0x495C1DF: aio_del_req_from_fsp (smb2_aio.c:77) ==116781== by 0x4B7FA85: _tc_free_internal (talloc.c:1158) ==116781== by 0x4B80DE4: _tc_free_children_internal (talloc.c:1669) ==116781== by 0x4B7FCB6: _tc_free_internal (talloc.c:1184) ==116781== by 0x4B80DE4: _tc_free_children_internal (talloc.c:1669) ==116781== by 0x4B7FCB6: _tc_free_internal (talloc.c:1184) ==116781== by 0x4B80DE4: _tc_free_children_internal (talloc.c:1669) ==116781== by 0x4B7FCB6: _tc_free_internal (talloc.c:1184) ==116781== by 0x4B7FF18: _talloc_free_internal (talloc.c:1248) ==116781== by 0x4B81212: _talloc_free (talloc.c:1792) ==116781== by 0x49AF60F: exit_server_common (server_exit.c:187) ==116781== by 0x49AF7F7: smbd_exit_server_cleanly (server_exit.c:246) ==116781== Address 0x9e2f258 is 408 bytes inside a block of size 440 free'd ==116781== at 0x48470E4: free (vg_replace_malloc.c:872) ==116781== by 0x4B7FE71: _tc_free_internal (talloc.c:1222) ==116781== by 0x4B7FF18: _talloc_free_internal (talloc.c:1248) ==116781== by 0x4B81212: _talloc_free (talloc.c:1792) ==116781== by 0x48E7506: fsp_free (files.c:1913) ==116781== by 0x48E77EE: file_free (files.c:1973) ==116781== by 0x492B695: close_file_free (close.c:1691) ==116781== by 0x48E63DE: close_file_in_loop (files.c:1507) ==116781== by 0x48E674F: file_close_user_fn (files.c:1636) ==116781== by 0x48E68A4: files_forall (files.c:1672) ==116781== by 0x48E67AE: file_close_user (files.c:1648) ==116781== by 0x49A57D1: smbXsrv_session_logoff (smbXsrv_session.c:1862) ==116781== Block was alloc'd at ==116781== at 0x484486F: malloc (vg_replace_malloc.c:381) ==116781== by 0x4B7EF25: __talloc_with_prefix (talloc.c:783) ==116781== by 0x4B7F0BF: __talloc (talloc.c:825) ==116781== by 0x4B7F4EB: _talloc_named_const (talloc.c:982) ==116781== by 0x4B826E2: _talloc_zero (talloc.c:2421) ==116781== by 0x48E1C06: fsp_new (files.c:45) ==116781== by 0x48E20CD: file_new (files.c:148) ==116781== by 0x48F53FB: open_np_file (smb2_pipes.c:46) ==116781== by 0x497C4B9: smbd_smb2_create_send (smb2_create.c:896) ==116781== by 0x497AA12: smbd_smb2_request_process_create (smb2_create.c:291) ==116781== by 0x4969E07: smbd_smb2_request_dispatch (smb2_server.c:3472) ==116781== by 0x496FD67: smbd_smb2_advance_incoming (smb2_server.c:5089) ==116781== ==116781== Invalid read of size 4 ==116781== at 0x495C1F1: aio_del_req_from_fsp (smb2_aio.c:82) ==116781== by 0x4B7FA85: _tc_free_internal (talloc.c:1158) ==116781== by 0x4B80DE4: _tc_free_children_internal (talloc.c:1669) ==116781== by 0x4B7FCB6: _tc_free_internal (talloc.c:1184) ==116781== by 0x4B80DE4: _tc_free_children_internal (talloc.c:1669) ==116781== by 0x4B7FCB6: _tc_free_internal (talloc.c:1184) ==116781== by 0x4B80DE4: _tc_free_children_internal (talloc.c:1669) ==116781== by 0x4B7FCB6: _tc_free_internal (talloc.c:1184) ==116781== by 0x4B7FF18: _talloc_free_internal (talloc.c:1248) ==116781== by 0x4B81212: _talloc_free (talloc.c:1792) ==116781== by 0x49AF60F: exit_server_common (server_exit.c:187) ==116781== by 0x49AF7F7: smbd_exit_server_cleanly (server_exit.c:246) ==116781== Address 0x9e2f258 is 408 bytes inside a block of size 440 free'd ==116781== at 0x48470E4: free (vg_replace_malloc.c:872) ==116781== by 0x4B7FE71: _tc_free_internal (talloc.c:1222) ==116781== by 0x4B7FF18: _talloc_free_internal (talloc.c:1248) ==116781== by 0x4B81212: _talloc_free (talloc.c:1792) ==116781== by 0x48E7506: fsp_free (files.c:1913) ==116781== by 0x48E77EE: file_free (files.c:1973) ==116781== by 0x492B695: close_file_free (close.c:1691) ==116781== by 0x48E63DE: close_file_in_loop (files.c:1507) ==116781== by 0x48E674F: file_close_user_fn (files.c:1636) ==116781== by 0x48E68A4: files_forall (files.c:1672) ==116781== by 0x48E67AE: file_close_user (files.c:1648) ==116781== by 0x49A57D1: smbXsrv_session_logoff (smbXsrv_session.c:1862) ==116781== Block was alloc'd at ==116781== at 0x484486F: malloc (vg_replace_malloc.c:381) ==116781== by 0x4B7EF25: __talloc_with_prefix (talloc.c:783) ==116781== by 0x4B7F0BF: __talloc (talloc.c:825) ==116781== by 0x4B7F4EB: _talloc_named_const (talloc.c:982) ==116781== by 0x4B826E2: _talloc_zero (talloc.c:2421) ==116781== by 0x48E1C06: fsp_new (files.c:45) ==116781== by 0x48E20CD: file_new (files.c:148) ==116781== by 0x48F53FB: open_np_file (smb2_pipes.c:46) ==116781== by 0x497C4B9: smbd_smb2_create_send (smb2_create.c:896) ==116781== by 0x497AA12: smbd_smb2_request_process_create (smb2_create.c:291) ==116781== by 0x4969E07: smbd_smb2_request_dispatch (smb2_server.c:3472) ==116781== by 0x496FD67: smbd_smb2_advance_incoming (smb2_server.c:5089) ==116781==
Here is the fix (raw patch). diff --git a/source3/smbd/close.c b/source3/smbd/close.c index 2b180e0c718..052b5eaf260 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -1630,6 +1630,7 @@ NTSTATUS close_file_smb(struct smb_request *req, SMB_ASSERT(fsp->stream_fsp == NULL); if (fsp->fake_file_handle != NULL) { + assert_no_pending_aio(fsp, close_type); status = close_fake_file(req, fsp); } else if (fsp->print_file != NULL) { /* FIXME: return spool errors */ pipe handles are instantiated as "fake_file_handles" but can still have a pending aio struct attached to them. We were forgetting to delete these pending aio's before close, meaning the destructor was still triggered after the fsp was already freed. Make them delete the pending aio's in the same way as for normal files and directories.
https://gitlab.com/samba-team/samba/-/merge_requests/3273
This bug was referenced in samba master: 82e88f70f181300f6f98691f6680839a94470e13 3f32bf887d4425655e81da0b2234cbca3b1d56e6 ea062c3b0d4dbb1f0682f808ac893bf36a6fb194 66398dd03c46633b474438dddb771caa2d245e64 11280f1705c0faa1729f5aeaa1b6a1f79ab5a199
Created attachment 18112 [details] git-am fix for 4.19.next, 4.18.next. Cherry-picked from master. Applies cleanly to 4.19.ext, 4.18.next.
Reassigning to Jule for inclusion in 4.18 and 4.19.
Pushed to autobuild-v4-{19,18}-test.
This bug was referenced in samba v4-18-test: 4baff9de6b2dc93d81c25874c78f31f91b16d260 b3a881f89ae089e3ec8e603e96eda5b1388e0cb0 721513a219d8b543a3f6a284d05ddc2c99717afe db1fbcc0263d8ce3854a65bda5b54b38a9d4d66d f869013c616d706007f11ebfcaa0f8af4cadc230
This bug was referenced in samba v4-19-test: 3ac075735c191bb69c2d841a44c6e9c3bf75d3bf 68b8a5c438d296a30f37cc180f9699d49fef70b4 f3d07e123ec254fdfea8f2c36c3a6532be8b369e 09e00c0a6c500d88d2d43cd1538ec10fda6b7692 c30984f095d2be4fe431544073dcad2a4c45d3d5
Closing out bug report. Thanks!
This bug was referenced in samba v4-18-stable (Release samba-4.18.7): 4baff9de6b2dc93d81c25874c78f31f91b16d260 b3a881f89ae089e3ec8e603e96eda5b1388e0cb0 721513a219d8b543a3f6a284d05ddc2c99717afe db1fbcc0263d8ce3854a65bda5b54b38a9d4d66d f869013c616d706007f11ebfcaa0f8af4cadc230
This bug was referenced in samba v4-19-stable (Release samba-4.19.2): 3ac075735c191bb69c2d841a44c6e9c3bf75d3bf 68b8a5c438d296a30f37cc180f9699d49fef70b4 f3d07e123ec254fdfea8f2c36c3a6532be8b369e 09e00c0a6c500d88d2d43cd1538ec10fda6b7692 c30984f095d2be4fe431544073dcad2a4c45d3d5