From f5eb449cac82de61c4eadc1b134c92896f69aacf Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 20 Dec 2023 18:01:57 +0100 Subject: [PATCH] smbd: use check_any_access_fsp() for all access checks Replaces the direct access to fsp->access_mask with a call to check_any_access_fsp() which allows doing additional checks if needed. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13688 Signed-off-by: Ralph Boehme Reviewed-by: Stefan Metzmacher (cherry picked from commit 02ed99343d19fd0845531ad99a46b1dd5b8a7a4f) --- source3/modules/vfs_acl_common.c | 7 ++++-- source3/modules/vfs_nfs4acl_xattr.c | 7 ++++-- source3/smbd/dir.c | 5 +++-- source3/smbd/dosmode.c | 20 +++++++++-------- source3/smbd/file_access.c | 10 +++++---- source3/smbd/notify.c | 5 +++-- source3/smbd/smb1_reply.c | 5 +++-- source3/smbd/smb2_flush.c | 4 +++- source3/smbd/smb2_getinfo.c | 8 +++---- source3/smbd/smb2_nttrans.c | 45 ++++++++++++++++++++++--------------- source3/smbd/smb2_reply.c | 10 +++++---- source3/smbd/smb2_trans2.c | 10 +++++---- 12 files changed, 82 insertions(+), 54 deletions(-) diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c index 341806b09a4..8f611c485ae 100644 --- a/source3/modules/vfs_acl_common.c +++ b/source3/modules/vfs_acl_common.c @@ -738,10 +738,13 @@ static NTSTATUS set_underlying_acl(vfs_handle_struct *handle, files_struct *fsp, /* We got access denied here. If we're already root, or we didn't need to do a chown, or the fsp isn't open with WRITE_OWNER access, just return. */ - if (get_current_uid(handle->conn) == 0 || !chown_needed || - !(fsp->access_mask & SEC_STD_WRITE_OWNER)) { + if (get_current_uid(handle->conn) == 0 || !chown_needed) { return NT_STATUS_ACCESS_DENIED; } + status = check_any_access_fsp(fsp, SEC_STD_WRITE_OWNER); + if (!NT_STATUS_IS_OK(status)) { + return status; + } /* * Only allow take-ownership, not give-ownership. That's the way Windows diff --git a/source3/modules/vfs_nfs4acl_xattr.c b/source3/modules/vfs_nfs4acl_xattr.c index cecafcf50b8..1fd3519ca02 100644 --- a/source3/modules/vfs_nfs4acl_xattr.c +++ b/source3/modules/vfs_nfs4acl_xattr.c @@ -368,11 +368,14 @@ static NTSTATUS nfs4acl_xattr_fset_nt_acl(vfs_handle_struct *handle, } if (get_current_uid(handle->conn) == 0 || - chown_needed == false || - !(fsp->access_mask & SEC_STD_WRITE_OWNER)) + chown_needed == false) { return NT_STATUS_ACCESS_DENIED; } + status = check_any_access_fsp(fsp, SEC_STD_WRITE_OWNER); + if (!NT_STATUS_IS_OK(status)) { + return status; + } /* * Only allow take-ownership, not give-ownership. That's the way Windows diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c index 59fbe548501..6cdbec19a54 100644 --- a/source3/smbd/dir.c +++ b/source3/smbd/dir.c @@ -218,11 +218,12 @@ NTSTATUS dptr_create(connection_struct *conn, return NT_STATUS_INVALID_PARAMETER; } - if (!(fsp->access_mask & SEC_DIR_LIST)) { + status = check_any_access_fsp(fsp, SEC_DIR_LIST); + if (!NT_STATUS_IS_OK(status)) { DBG_INFO("dptr_create: directory %s " "not open for LIST access\n", fsp_str_dbg(fsp)); - return NT_STATUS_ACCESS_DENIED; + return status; } status = OpenDir_fsp(NULL, conn, fsp, wcard, attr, &dir_hnd); if (!NT_STATUS_IS_OK(status)) { diff --git a/source3/smbd/dosmode.c b/source3/smbd/dosmode.c index 18d2322cf0e..5402180cf26 100644 --- a/source3/smbd/dosmode.c +++ b/source3/smbd/dosmode.c @@ -1068,15 +1068,17 @@ NTSTATUS file_set_sparse(connection_struct *conn, * Windows Server 2008 & 2012 permit FSCTL_SET_SPARSE if any of the * following access flags are granted. */ - if ((fsp->access_mask & (FILE_WRITE_DATA - | FILE_WRITE_ATTRIBUTES - | SEC_FILE_APPEND_DATA)) == 0) { - DEBUG(9,("file_set_sparse: fname[%s] set[%u] " - "access_mask[0x%08X] - access denied\n", - smb_fname_str_dbg(fsp->fsp_name), - sparse, - fsp->access_mask)); - return NT_STATUS_ACCESS_DENIED; + status = check_any_access_fsp(fsp, + FILE_WRITE_DATA + | FILE_WRITE_ATTRIBUTES + | SEC_FILE_APPEND_DATA); + if (!NT_STATUS_IS_OK(status)) { + DBG_DEBUG("fname[%s] set[%u] " + "access_mask[0x%08X] - access denied\n", + smb_fname_str_dbg(fsp->fsp_name), + sparse, + fsp->access_mask); + return status; } if (fsp->fsp_flags.is_directory) { diff --git a/source3/smbd/file_access.c b/source3/smbd/file_access.c index 476aead590d..7c4c7156d24 100644 --- a/source3/smbd/file_access.c +++ b/source3/smbd/file_access.c @@ -191,6 +191,7 @@ bool directory_has_default_acl_fsp(struct files_struct *fsp) NTSTATUS can_set_delete_on_close(files_struct *fsp, uint32_t dosmode) { + NTSTATUS status; /* * Only allow delete on close for writable files. */ @@ -219,11 +220,12 @@ NTSTATUS can_set_delete_on_close(files_struct *fsp, uint32_t dosmode) * intent. */ - if (!(fsp->access_mask & DELETE_ACCESS)) { - DEBUG(10,("can_set_delete_on_close: file %s delete on " + status = check_any_access_fsp(fsp, DELETE_ACCESS); + if (!NT_STATUS_IS_OK(status)) { + DBG_DEBUG("file %s delete on " "close flag set but delete access denied.\n", - fsp_str_dbg(fsp))); - return NT_STATUS_ACCESS_DENIED; + fsp_str_dbg(fsp)); + return status; } /* Don't allow delete on close for non-empty directories. */ diff --git a/source3/smbd/notify.c b/source3/smbd/notify.c index 399d7800249..156fbf700c3 100644 --- a/source3/smbd/notify.c +++ b/source3/smbd/notify.c @@ -295,8 +295,9 @@ NTSTATUS change_notify_create(struct files_struct *fsp, * Setting a changenotify needs READ/LIST access * on the directory handle. */ - if (!(fsp->access_mask & SEC_DIR_LIST)) { - return NT_STATUS_ACCESS_DENIED; + status = check_any_access_fsp(fsp, SEC_DIR_LIST); + if (!NT_STATUS_IS_OK(status)) { + return status; } if (fsp->notify != NULL) { diff --git a/source3/smbd/smb1_reply.c b/source3/smbd/smb1_reply.c index 92cc7bf7dd1..9fbfc39c69f 100644 --- a/source3/smbd/smb1_reply.c +++ b/source3/smbd/smb1_reply.c @@ -6965,8 +6965,9 @@ void reply_setattrE(struct smb_request *req) goto out; } - if (!(fsp->access_mask & FILE_WRITE_ATTRIBUTES)) { - reply_nterror(req, NT_STATUS_ACCESS_DENIED); + status = check_any_access_fsp(fsp, FILE_WRITE_ATTRIBUTES); + if (!NT_STATUS_IS_OK(status)) { + reply_nterror(req, status); goto out; } diff --git a/source3/smbd/smb2_flush.c b/source3/smbd/smb2_flush.c index 3fec7908a46..35f545ae3bd 100644 --- a/source3/smbd/smb2_flush.c +++ b/source3/smbd/smb2_flush.c @@ -128,6 +128,7 @@ static struct tevent_req *smbd_smb2_flush_send(TALLOC_CTX *mem_ctx, struct smb_request *smbreq; bool is_compound = false; bool is_last_in_compound = false; + NTSTATUS status; req = tevent_req_create(mem_ctx, &state, struct smbd_smb2_flush_state); @@ -167,7 +168,8 @@ static struct tevent_req *smbd_smb2_flush_send(TALLOC_CTX *mem_ctx, * they can be flushed. */ - if ((fsp->access_mask & flush_access) != 0) { + status = check_any_access_fsp(fsp, flush_access); + if (NT_STATUS_IS_OK(status)) { allow_dir_flush = true; } diff --git a/source3/smbd/smb2_getinfo.c b/source3/smbd/smb2_getinfo.c index 51283eb174e..d3067a53ab9 100644 --- a/source3/smbd/smb2_getinfo.c +++ b/source3/smbd/smb2_getinfo.c @@ -315,15 +315,15 @@ static struct tevent_req *smbd_smb2_getinfo_send(TALLOC_CTX *mem_ctx, case FSCC_FILE_ALL_INFORMATION: case FSCC_FILE_NETWORK_OPEN_INFORMATION: case FSCC_FILE_ATTRIBUTE_TAG_INFORMATION: - if (!(fsp->access_mask & SEC_FILE_READ_ATTRIBUTE)) { - tevent_req_nterror(req, NT_STATUS_ACCESS_DENIED); + status = check_any_access_fsp(fsp, SEC_FILE_READ_ATTRIBUTE); + if (tevent_req_nterror(req, status)) { return tevent_req_post(req, ev); } break; case FSCC_FILE_FULL_EA_INFORMATION: - if (!(fsp->access_mask & SEC_FILE_READ_EA)) { - tevent_req_nterror(req, NT_STATUS_ACCESS_DENIED); + status = check_any_access_fsp(fsp, SEC_FILE_READ_EA); + if (tevent_req_nterror(req, status)) { return tevent_req_post(req, ev); } break; diff --git a/source3/smbd/smb2_nttrans.c b/source3/smbd/smb2_nttrans.c index 1312d9bfc36..c58cfadcf92 100644 --- a/source3/smbd/smb2_nttrans.c +++ b/source3/smbd/smb2_nttrans.c @@ -114,20 +114,23 @@ NTSTATUS set_sd(files_struct *fsp, struct security_descriptor *psd, /* Ensure we have the rights to do this. */ if (security_info_sent & SECINFO_OWNER) { - if (!(fsp->access_mask & SEC_STD_WRITE_OWNER)) { - return NT_STATUS_ACCESS_DENIED; + status = check_any_access_fsp(fsp, SEC_STD_WRITE_OWNER); + if (!NT_STATUS_IS_OK(status)) { + return status; } } if (security_info_sent & SECINFO_GROUP) { - if (!(fsp->access_mask & SEC_STD_WRITE_OWNER)) { - return NT_STATUS_ACCESS_DENIED; + status = check_any_access_fsp(fsp, SEC_STD_WRITE_OWNER); + if (!NT_STATUS_IS_OK(status)) { + return status; } } if (security_info_sent & SECINFO_DACL) { - if (!(fsp->access_mask & SEC_STD_WRITE_DAC)) { - return NT_STATUS_ACCESS_DENIED; + status = check_any_access_fsp(fsp, SEC_STD_WRITE_DAC); + if (!NT_STATUS_IS_OK(status)) { + return status; } /* Convert all the generic bits. */ if (psd->dacl) { @@ -136,15 +139,17 @@ NTSTATUS set_sd(files_struct *fsp, struct security_descriptor *psd, } if (security_info_sent & SECINFO_SACL) { - if (!(fsp->access_mask & SEC_FLAG_SYSTEM_SECURITY)) { - return NT_STATUS_ACCESS_DENIED; + status = check_any_access_fsp(fsp, SEC_FLAG_SYSTEM_SECURITY); + if (!NT_STATUS_IS_OK(status)) { + return status; } /* * Setting a SACL also requires WRITE_DAC. * See the smbtorture3 SMB2-SACL test. */ - if (!(fsp->access_mask & SEC_STD_WRITE_DAC)) { - return NT_STATUS_ACCESS_DENIED; + status = check_any_access_fsp(fsp, SEC_STD_WRITE_DAC); + if (!NT_STATUS_IS_OK(status)) { + return status; } /* Convert all the generic bits. */ if (psd->sacl) { @@ -407,16 +412,20 @@ static NTSTATUS smbd_fetch_security_desc(connection_struct *conn, * Get the permissions to return. */ - if ((security_info_wanted & SECINFO_SACL) && - !(fsp->access_mask & SEC_FLAG_SYSTEM_SECURITY)) { - DEBUG(10, ("Access to SACL denied.\n")); - return NT_STATUS_ACCESS_DENIED; + if (security_info_wanted & SECINFO_SACL) { + status = check_any_access_fsp(fsp, SEC_FLAG_SYSTEM_SECURITY); + if (!NT_STATUS_IS_OK(status)) { + DBG_DEBUG("Access to SACL denied.\n"); + return status; + } } - if ((security_info_wanted & (SECINFO_DACL|SECINFO_OWNER|SECINFO_GROUP)) && - !(fsp->access_mask & SEC_STD_READ_CONTROL)) { - DEBUG(10, ("Access to DACL, OWNER, or GROUP denied.\n")); - return NT_STATUS_ACCESS_DENIED; + if (security_info_wanted & (SECINFO_DACL|SECINFO_OWNER|SECINFO_GROUP)) { + status = check_any_access_fsp(fsp, SEC_STD_READ_CONTROL); + if (!NT_STATUS_IS_OK(status)) { + DBG_DEBUG("Access to DACL, OWNER, or GROUP denied.\n"); + return status; + } } status = refuse_symlink_fsp(fsp); diff --git a/source3/smbd/smb2_reply.c b/source3/smbd/smb2_reply.c index a7c63c0df58..4d92e83c57a 100644 --- a/source3/smbd/smb2_reply.c +++ b/source3/smbd/smb2_reply.c @@ -1140,6 +1140,8 @@ ssize_t sendfile_short_send(struct smbXsrv_connection *xconn, static NTSTATUS can_rename(connection_struct *conn, files_struct *fsp, uint16_t dirtype) { + NTSTATUS status; + if (fsp->fsp_name->twrp != 0) { /* Get the error right, this is what Windows returns. */ return NT_STATUS_NOT_SAME_DEVICE; @@ -1183,11 +1185,11 @@ static NTSTATUS can_rename(connection_struct *conn, files_struct *fsp, return NT_STATUS_OK; } - if (fsp->access_mask & (DELETE_ACCESS|FILE_WRITE_ATTRIBUTES)) { - return NT_STATUS_OK; + status = check_any_access_fsp(fsp, DELETE_ACCESS | FILE_WRITE_ATTRIBUTES); + if (!NT_STATUS_IS_OK(status)) { + return status; } - - return NT_STATUS_ACCESS_DENIED; + return NT_STATUS_OK; } /**************************************************************************** diff --git a/source3/smbd/smb2_trans2.c b/source3/smbd/smb2_trans2.c index 2a90629ce61..d39f5880efa 100644 --- a/source3/smbd/smb2_trans2.c +++ b/source3/smbd/smb2_trans2.c @@ -4051,8 +4051,9 @@ NTSTATUS smb_set_file_size(connection_struct *conn, fsp_get_io_fd(fsp) != -1) { /* Handle based call. */ - if (!(fsp->access_mask & FILE_WRITE_DATA)) { - return NT_STATUS_ACCESS_DENIED; + status = check_any_access_fsp(fsp, FILE_WRITE_DATA); + if (!NT_STATUS_IS_OK(status)) { + return status; } if (vfs_set_filelen(fsp, size) == -1) { @@ -4962,8 +4963,9 @@ static NTSTATUS smb_set_file_allocation_info(connection_struct *conn, fsp_get_io_fd(fsp) != -1) { /* Open file handle. */ - if (!(fsp->access_mask & FILE_WRITE_DATA)) { - return NT_STATUS_ACCESS_DENIED; + status = check_any_access_fsp(fsp, FILE_WRITE_DATA); + if (!NT_STATUS_IS_OK(status)) { + return status; } /* Only change if needed. */ -- 2.11.4.GIT