From 02ed99343d19fd0845531ad99a46b1dd5b8a7a4f 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 --- 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 692e776d10c..314fc79a3a6 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 800b7221cfd..49c37cbb4d9 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 6fbcac40d2e..1472af7d059 100644 --- a/source3/smbd/dosmode.c +++ b/source3/smbd/dosmode.c @@ -1076,15 +1076,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 9ad9173e49c..9928eb99750 100644 --- a/source3/smbd/file_access.c +++ b/source3/smbd/file_access.c @@ -192,6 +192,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. */ @@ -220,11 +221,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 e4994d2a79f..b2f317a1db9 100644 --- a/source3/smbd/smb1_reply.c +++ b/source3/smbd/smb1_reply.c @@ -6970,8 +6970,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 55dca5e8031..7c43a4e923d 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 bf5170f07d4..49bddf5d0e2 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) { @@ -474,16 +479,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 f3cc48b7d56..ab123999569 100644 --- a/source3/smbd/smb2_reply.c +++ b/source3/smbd/smb2_reply.c @@ -1142,6 +1142,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; @@ -1185,11 +1187,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 b0383b3cc19..22e0f2f3020 100644 --- a/source3/smbd/smb2_trans2.c +++ b/source3/smbd/smb2_trans2.c @@ -4018,8 +4018,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) { @@ -4922,8 +4923,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.34.1