From 44396d7bade98b41fb655e30d84195793684f593 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 20 Dec 2023 18:32:25 +0100 Subject: [PATCH] smbd: replace CHECK_WRITE() macro with calls to check_any_access_fsp() The additional check if fd underlying fd is valid and not -1 should not be done at this place. I actually would prefer an write to fail with EBADF if this happens, as it's likely easier to debug why this happened. These days we should always have a valid fd. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13688 Signed-off-by: Ralph Boehme Reviewed-by: Stefan Metzmacher (cherry picked from commit 995a31c8d4c1789c16bae6b8196f2565d4b1dfdb) --- source3/include/smb_macros.h | 5 ----- source3/modules/offload_token.c | 7 +++++-- source3/smbd/smb1_reply.c | 32 ++++++++++++++++++++------------ source3/smbd/smb2_flush.c | 3 ++- source3/smbd/smb2_write.c | 6 ++++-- 5 files changed, 31 insertions(+), 22 deletions(-) diff --git a/source3/include/smb_macros.h b/source3/include/smb_macros.h index 42ff9ffb0d4..1851709774a 100644 --- a/source3/include/smb_macros.h +++ b/source3/include/smb_macros.h @@ -74,11 +74,6 @@ (fsp_get_io_fd(fsp) != -1) && \ (((fsp)->fsp_flags.can_read))) -#define CHECK_WRITE(fsp) \ - ((fsp)->fsp_flags.can_write && \ - (!(fsp)->fsp_flags.is_pathref) && \ - (fsp_get_io_fd(fsp) != -1)) - #define ERROR_WAS_LOCK_DENIED(status) (NT_STATUS_EQUAL((status), NT_STATUS_LOCK_NOT_GRANTED) || \ NT_STATUS_EQUAL((status), NT_STATUS_FILE_LOCK_CONFLICT) ) diff --git a/source3/modules/offload_token.c b/source3/modules/offload_token.c index 901991daf28..3b71a0028fb 100644 --- a/source3/modules/offload_token.c +++ b/source3/modules/offload_token.c @@ -259,6 +259,8 @@ NTSTATUS vfs_offload_token_check_handles(uint32_t fsctl, files_struct *src_fsp, files_struct *dst_fsp) { + NTSTATUS status; + if (src_fsp->vuid != dst_fsp->vuid) { DBG_INFO("copy chunk handles not in the same session.\n"); return NT_STATUS_ACCESS_DENIED; @@ -317,10 +319,11 @@ NTSTATUS vfs_offload_token_check_handles(uint32_t fsctl, * * A non writable dst handle also doesn't make sense for other fsctls. */ - if (!CHECK_WRITE(dst_fsp)) { + status = check_any_access_fsp(dst_fsp, FILE_WRITE_DATA|FILE_APPEND_DATA); + if (!NT_STATUS_IS_OK(status)) { DBG_INFO("dest handle not writable (%s).\n", smb_fname_str_dbg(dst_fsp->fsp_name)); - return NT_STATUS_ACCESS_DENIED; + return status; } /* * - The Open.GrantedAccess of the destination file does not include diff --git a/source3/smbd/smb1_reply.c b/source3/smbd/smb1_reply.c index 63ff98fb22b..92cc7bf7dd1 100644 --- a/source3/smbd/smb1_reply.c +++ b/source3/smbd/smb1_reply.c @@ -3926,8 +3926,9 @@ void reply_writebraw(struct smb_request *req) return; } - if (!CHECK_WRITE(fsp)) { - reply_nterror(req, NT_STATUS_ACCESS_DENIED); + status = check_any_access_fsp(fsp, FILE_WRITE_DATA|FILE_APPEND_DATA); + if (!NT_STATUS_IS_OK(status)) { + reply_nterror(req, status); error_to_writebrawerr(req); END_PROFILE(SMBwritebraw); return; @@ -4157,8 +4158,9 @@ void reply_writeunlock(struct smb_request *req) return; } - if (!CHECK_WRITE(fsp)) { - reply_nterror(req, NT_STATUS_ACCESS_DENIED); + status = check_any_access_fsp(fsp, FILE_WRITE_DATA|FILE_APPEND_DATA); + if (!NT_STATUS_IS_OK(status)) { + reply_nterror(req, status); END_PROFILE(SMBwriteunlock); return; } @@ -4292,8 +4294,9 @@ void reply_write(struct smb_request *req) return; } - if (!CHECK_WRITE(fsp)) { - reply_nterror(req, NT_STATUS_ACCESS_DENIED); + status = check_any_access_fsp(fsp, FILE_WRITE_DATA|FILE_APPEND_DATA); + if (!NT_STATUS_IS_OK(status)) { + reply_nterror(req, status); END_PROFILE(SMBwrite); return; } @@ -4571,8 +4574,9 @@ void reply_write_and_X(struct smb_request *req) goto out; } - if (!CHECK_WRITE(fsp)) { - reply_nterror(req, NT_STATUS_ACCESS_DENIED); + status = check_any_access_fsp(fsp, FILE_WRITE_DATA|FILE_APPEND_DATA); + if (!NT_STATUS_IS_OK(status)) { + reply_nterror(req, status); goto out; } @@ -5283,6 +5287,7 @@ void reply_writeclose(struct smb_request *req) struct timespec mtime; files_struct *fsp; struct lock_struct lock; + NTSTATUS status; START_PROFILE(SMBwriteclose); @@ -5298,8 +5303,9 @@ void reply_writeclose(struct smb_request *req) END_PROFILE(SMBwriteclose); return; } - if (!CHECK_WRITE(fsp)) { - reply_nterror(req, NT_STATUS_ACCESS_DENIED); + status = check_any_access_fsp(fsp, FILE_WRITE_DATA|FILE_APPEND_DATA); + if (!NT_STATUS_IS_OK(status)) { + reply_nterror(req, status); END_PROFILE(SMBwriteclose); return; } @@ -6086,6 +6092,7 @@ void reply_printwrite(struct smb_request *req) int numtowrite; const char *data; files_struct *fsp; + NTSTATUS status; START_PROFILE(SMBsplwr); @@ -6108,8 +6115,9 @@ void reply_printwrite(struct smb_request *req) return; } - if (!CHECK_WRITE(fsp)) { - reply_nterror(req, NT_STATUS_ACCESS_DENIED); + status = check_any_access_fsp(fsp, FILE_WRITE_DATA|FILE_APPEND_DATA); + if (!NT_STATUS_IS_OK(status)) { + reply_nterror(req, status); END_PROFILE(SMBsplwr); return; } diff --git a/source3/smbd/smb2_flush.c b/source3/smbd/smb2_flush.c index 2d3e6319697..3fec7908a46 100644 --- a/source3/smbd/smb2_flush.c +++ b/source3/smbd/smb2_flush.c @@ -150,7 +150,8 @@ static struct tevent_req *smbd_smb2_flush_send(TALLOC_CTX *mem_ctx, return tevent_req_post(req, ev); } - if (!CHECK_WRITE(fsp)) { + status = check_any_access_fsp(fsp, FILE_WRITE_DATA|FILE_APPEND_DATA); + if (!NT_STATUS_IS_OK(status)) { bool allow_dir_flush = false; uint32_t flush_access = FILE_ADD_FILE | FILE_ADD_SUBDIRECTORY; diff --git a/source3/smbd/smb2_write.c b/source3/smbd/smb2_write.c index a7af20173a1..2675997b0fd 100644 --- a/source3/smbd/smb2_write.c +++ b/source3/smbd/smb2_write.c @@ -24,6 +24,7 @@ #include "../libcli/smb/smb_common.h" #include "../lib/util/tevent_ntstatus.h" #include "rpc_server/srv_pipe_hnd.h" +#include "libcli/security/security.h" #undef DBGC_CLASS #define DBGC_CLASS DBGC_SMB2 @@ -339,8 +340,9 @@ static struct tevent_req *smbd_smb2_write_send(TALLOC_CTX *mem_ctx, return req; } - if (!CHECK_WRITE(fsp)) { - tevent_req_nterror(req, NT_STATUS_ACCESS_DENIED); + status = check_any_access_fsp(fsp, FILE_WRITE_DATA|FILE_APPEND_DATA); + if (!NT_STATUS_IS_OK(status)) { + tevent_req_nterror(req, status); return tevent_req_post(req, ev); } -- 2.11.4.GIT