diff options
| author | Volker Lendecke <vl@samba.org> | 2022-02-03 17:17:07 +0100 |
|---|---|---|
| committer | Jule Anger <janger@samba.org> | 2022-02-14 18:36:26 +0000 |
| commit | cdc5e9e4dbeb3e63e00db2afc3a8c48152885d40 (patch) | |
| tree | d7ef1e62a266be9eb0de84b3841b05ecfaaa21b9 | |
| parent | d44c45cbdbc7e13047ce127ea7ebcac2810a7891 (diff) | |
| download | samba-cdc5e9e4dbeb3e63e00db2afc3a8c48152885d40.tar.gz | |
smbd: Only file_free() a self-created fsp in create_file_unixpath()
This fixes a use-after-free in smb_full_audit_create_file() when
calling SMB_VFS_CREATE_FILE with fsp->fsp_name as smb_fname.
create_file_unixpath() has this comment:
* This is really subtle. If someone passes in an smb_fname
* where smb_fname actually is taken from fsp->fsp_name, then
* the lifetime of these objects is meant to be the same.
so it seems legitimate to call CREATE_FILE this way.
When CREATE_FILE runs into an error, create_file_unixpath() does a
file_free, which also takes fsp->fsp_name with
it. smb_full_audit_create_file() wants to log the failure including
the smb_fname after NEXT_CREATE_FILE has exited, but this will then
use the already free'ed data.
Fix by only doing the file_free() on an fsp that
create_file_unixpath() created itself.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14975
Signed-off-by: Volker Lendecke <vl@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Autobuild-User(master): Jeremy Allison <jra@samba.org>
Autobuild-Date(master): Thu Feb 10 19:11:33 UTC 2022 on sn-devel-184
(cherry picked from commit 434e6d4b4b45757878642d229d26d146792a3878)
Autobuild-User(v4-16-test): Jule Anger <janger@samba.org>
Autobuild-Date(v4-16-test): Mon Feb 14 18:36:26 UTC 2022 on sn-devel-184
| -rw-r--r-- | selftest/knownfail.d/full_audit_crash | 1 | ||||
| -rw-r--r-- | source3/smbd/open.c | 13 |
2 files changed, 12 insertions, 2 deletions
diff --git a/selftest/knownfail.d/full_audit_crash b/selftest/knownfail.d/full_audit_crash deleted file mode 100644 index 9154ea334f2..00000000000 --- a/selftest/knownfail.d/full_audit_crash +++ /dev/null @@ -1 +0,0 @@ -^samba.vfstest.full_audit_segfault.vfstest\(nt4_dc:local\)
\ No newline at end of file diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 5d8251dcef5..a5664b319ad 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -5533,6 +5533,7 @@ static NTSTATUS create_file_unixpath(connection_struct *conn, int info = FILE_WAS_OPENED; files_struct *base_fsp = NULL; files_struct *fsp = NULL; + bool free_fsp_on_error = false; NTSTATUS status; int ret; struct smb_filename *parent_dir_fname = NULL; @@ -5784,6 +5785,11 @@ static NTSTATUS create_file_unixpath(connection_struct *conn, * them from each other. */ smb_fname_fsp_unlink(smb_fname); + + /* + * "fsp" is ours now + */ + free_fsp_on_error = true; } status = fsp_bind_smb(fsp, req); @@ -5807,6 +5813,7 @@ static NTSTATUS create_file_unixpath(connection_struct *conn, if(!NT_STATUS_IS_OK(status)) { goto fail; } + free_fsp_on_error = true; status = fsp_set_smb_fname(fsp, smb_fname); if (!NT_STATUS_IS_OK(status)) { @@ -6053,7 +6060,11 @@ static NTSTATUS create_file_unixpath(connection_struct *conn, * fsp->base_fsp. */ base_fsp = NULL; - close_file_free(req, &fsp, ERROR_CLOSE); + close_file_smb(req, fsp, ERROR_CLOSE); + if (free_fsp_on_error) { + file_free(req, fsp); + fsp = NULL; + } } if (base_fsp != NULL) { close_file_free(req, &base_fsp, ERROR_CLOSE); |