The Samba-Bugzilla – Attachment 18223 Details for
Bug 13688
Windows 2016 fails to restore previous version of a file from a shadow_copy2 snapshot
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch for 4.18 backported from master
bug13688-v418.patch (text/plain), 52.40 KB, created by
Ralph Böhme
on 2024-01-09 10:01:01 UTC
(
hide
)
Description:
Patch for 4.18 backported from master
Filename:
MIME Type:
Creator:
Ralph Böhme
Created:
2024-01-09 10:01:01 UTC
Size:
52.40 KB
patch
obsolete
>From c53a9ed4ff567f02f6a9843efbef247f20646f10 Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Thu, 21 Dec 2023 19:40:21 +0100 >Subject: [PATCH 01/13] selftest: remove error_inject from shadow_write share > >Frankly, I can't remember why I added this as part of bug 13688. The >goal of the corresponding test is to verify a write on a read-only >file handle fails. As the file is opened O_RDONLY, the write will fail >anyway and there's no need to inject the error. > >To make things worse, having the error injected meant we didn't notice >when the underlying logic of forcing the open to be done with O_RDONLY >was done as O_RDWR, resulting in the write on the handle to succeed. > >This happened when we introduced reopen_from_fsp(): the initial >pathref open of a path with a twrp value was correctly detected and >handled by shadow_copy2_openat(). However, when converting the pathref >open to a real one via reopen_from_fsp(), shadow_copy2_openat() only >sees the magic /proc/fd path and has no way of inferring that this was >originating from a prevous version open with a twrp value. > >Tl;dr: we can just remove this error injection, it is not needed, the >correct fix is to implement this in the SMB layer which is done in the >subsequent commits. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13688 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 78119edba013583555069271bb61134c12c2c135) >--- > selftest/knownfail.d/samba3.blackbox.shadow_copy_torture | 1 + > selftest/target/Samba3.pm | 4 +--- > 2 files changed, 2 insertions(+), 3 deletions(-) > create mode 100644 selftest/knownfail.d/samba3.blackbox.shadow_copy_torture > >diff --git a/selftest/knownfail.d/samba3.blackbox.shadow_copy_torture b/selftest/knownfail.d/samba3.blackbox.shadow_copy_torture >new file mode 100644 >index 000000000000..16537e58aeb7 >--- /dev/null >+++ b/selftest/knownfail.d/samba3.blackbox.shadow_copy_torture >@@ -0,0 +1 @@ >+^samba3.blackbox.shadow_copy_torture.writing to shadow copy of a file\(fileserver\) >diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm >index 60775433de26..acee7dc68f08 100755 >--- a/selftest/target/Samba3.pm >+++ b/selftest/target/Samba3.pm >@@ -3408,9 +3408,7 @@ sub provision($$) > [shadow_write] > path = $shadow_tstdir > comment = previous versions snapshots under mount point >- vfs objects = shadow_copy2 streams_xattr error_inject >- aio write size = 0 >- error_inject:pwrite = EBADF >+ vfs objects = shadow_copy2 streams_xattr > shadow:mountpoint = $shadow_tstdir > shadow:fixinodes = yes > smbd async dosmode = yes >-- >2.43.0 > > >From 8bd06518af51ad85d5f1611d4b7f46660d3dfcea Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Fri, 22 Dec 2023 10:40:39 +0100 >Subject: [PATCH 02/13] s4/libcli/raw: implemement > RAW_SFILEINFO_LINK_INFORMATION > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13688 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit c62484bc2c60ebac42635793d94cb8e62629acbf) >--- > source4/libcli/raw/rawsetfileinfo.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > >diff --git a/source4/libcli/raw/rawsetfileinfo.c b/source4/libcli/raw/rawsetfileinfo.c >index fec99d30b957..0ef509405091 100644 >--- a/source4/libcli/raw/rawsetfileinfo.c >+++ b/source4/libcli/raw/rawsetfileinfo.c >@@ -119,6 +119,20 @@ bool smb_raw_setfileinfo_passthru(TALLOC_CTX *mem_ctx, > parms->full_ea_information.in.eas.eas, 4); > return true; > >+ case RAW_SFILEINFO_LINK_INFORMATION: >+ NEED_BLOB(20); >+ memset(blob->data, 0, blob->length); >+ >+ PUSH_LE_U8(blob->data, 0, parms->link_information.in.overwrite); >+ PUSH_LE_U64(blob->data, 8, parms->link_information.in.root_fid); >+ >+ len = smbcli_blob_append_string( >+ NULL, mem_ctx, blob, >+ parms->link_information.in.new_name, >+ STR_UNICODE | STR_TERMINATE); >+ PUSH_LE_U32(blob->data, 16, len - 2); >+ return true; >+ > /* Unhandled levels */ > case RAW_SFILEINFO_PIPE_INFORMATION: > case RAW_SFILEINFO_VALID_DATA_INFORMATION: >-- >2.43.0 > > >From 37afa92882cbc31738602e2306bdf4d0b1ca9334 Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Fri, 15 Dec 2023 19:55:23 +0100 >Subject: [PATCH 03/13] smbtorture: expand smb2.twrp.write test > >Test more modifying operations are blocked and access masks are correct. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13688 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 537eedfe2a79fba2e1f062f14ba7a0c5f8f70a88) >--- > source4/torture/smb2/create.c | 245 +++++++++++++++++++++++++++++++++- > 1 file changed, 242 insertions(+), 3 deletions(-) > >diff --git a/source4/torture/smb2/create.c b/source4/torture/smb2/create.c >index d9fa2217953c..24c3cace3c41 100644 >--- a/source4/torture/smb2/create.c >+++ b/source4/torture/smb2/create.c >@@ -2080,6 +2080,18 @@ static bool test_twrp_write(struct torture_context *tctx, struct smb2_tree *tree > uint64_t nttime; > const char *file = NULL; > const char *snapshot = NULL; >+ uint32_t expected_access; >+ union smb_fileinfo getinfo; >+ union smb_setfileinfo setinfo; >+ struct security_descriptor *sd = NULL, *sd_orig = NULL; >+ const char *owner_sid = NULL; >+ struct create_disps_tests { >+ const char *file; >+ uint32_t create_disposition; >+ uint32_t create_options; >+ NTSTATUS expected_status; >+ }; >+ struct create_disps_tests *cd_test = NULL; > > file = torture_setting_string(tctx, "twrp_file", NULL); > if (file == NULL) { >@@ -2121,11 +2133,102 @@ static bool test_twrp_write(struct torture_context *tctx, struct smb2_tree *tree > "smb2_create\n"); > smb2_util_close(tree, io.out.file.handle); > >- ret = io.out.maximal_access & (SEC_FILE_READ_DATA | SEC_FILE_WRITE_DATA); >- torture_assert_goto(tctx, ret, ret, done, "Bad access\n"); >+ expected_access = SEC_RIGHTS_FILE_ALL & >+ ~(SEC_FILE_EXECUTE | SEC_DIR_DELETE_CHILD); >+ >+ torture_assert_int_equal_goto(tctx, >+ io.out.maximal_access & expected_access, >+ expected_access, >+ ret, done, "Bad access\n"); >+ >+ { >+ /* >+ * Test create dispositions >+ */ >+ struct create_disps_tests cd_tests[] = { >+ { >+ .file = file, >+ .create_disposition = NTCREATEX_DISP_OPEN, >+ .expected_status = NT_STATUS_OK, >+ }, >+ { >+ .file = file, >+ .create_disposition = NTCREATEX_DISP_OPEN_IF, >+ .expected_status = NT_STATUS_OK, >+ }, >+ { >+ .file = file, >+ .create_disposition = NTCREATEX_DISP_OVERWRITE, >+ .expected_status = NT_STATUS_MEDIA_WRITE_PROTECTED, >+ }, >+ { >+ .file = file, >+ .create_disposition = NTCREATEX_DISP_OVERWRITE_IF, >+ .expected_status = NT_STATUS_MEDIA_WRITE_PROTECTED, >+ }, >+ { >+ .file = file, >+ .create_disposition = NTCREATEX_DISP_SUPERSEDE, >+ .expected_status = NT_STATUS_MEDIA_WRITE_PROTECTED, >+ }, >+ { >+ .file = "newfile", >+ .create_disposition = NTCREATEX_DISP_OPEN_IF, >+ .expected_status = NT_STATUS_MEDIA_WRITE_PROTECTED, >+ }, >+ { >+ .file = "newfile", >+ .create_disposition = NTCREATEX_DISP_OVERWRITE_IF, >+ .expected_status = NT_STATUS_MEDIA_WRITE_PROTECTED, >+ }, >+ { >+ .file = "newfile", >+ .create_disposition = NTCREATEX_DISP_CREATE, >+ .expected_status = NT_STATUS_MEDIA_WRITE_PROTECTED, >+ }, >+ { >+ .file = "newfile", >+ .create_disposition = NTCREATEX_DISP_SUPERSEDE, >+ .expected_status = NT_STATUS_MEDIA_WRITE_PROTECTED, >+ }, >+ { >+ .file = "newdir", >+ .create_disposition = NTCREATEX_DISP_OPEN_IF, >+ .create_options = NTCREATEX_OPTIONS_DIRECTORY, >+ .expected_status = NT_STATUS_MEDIA_WRITE_PROTECTED, >+ }, >+ { >+ .file = "newdir", >+ .create_disposition = NTCREATEX_DISP_CREATE, >+ .create_options = NTCREATEX_OPTIONS_DIRECTORY, >+ .expected_status = NT_STATUS_MEDIA_WRITE_PROTECTED, >+ }, >+ { >+ .file = NULL, >+ }, >+ }; >+ >+ for (cd_test = &cd_tests[0]; cd_test->file != NULL; cd_test++) { >+ io = (struct smb2_create) { >+ .in.fname = cd_test->file, >+ .in.create_disposition = cd_test->create_disposition, >+ .in.create_options = cd_test->create_options, >+ >+ .in.desired_access = SEC_FILE_READ_DATA, >+ .in.file_attributes = FILE_ATTRIBUTE_NORMAL, >+ .in.share_access = NTCREATEX_SHARE_ACCESS_MASK, >+ .in.timewarp = nttime, >+ }; >+ >+ status = smb2_create(tree, tctx, &io); >+ torture_assert_ntstatus_equal_goto( >+ tctx, status, cd_test->expected_status, ret, done, >+ "Bad status\n"); >+ } >+ } > > io = (struct smb2_create) { >- .in.desired_access = SEC_FILE_READ_DATA|SEC_FILE_WRITE_DATA, >+ .in.desired_access = expected_access, > .in.file_attributes = FILE_ATTRIBUTE_NORMAL, > .in.create_disposition = NTCREATEX_DISP_OPEN, > .in.share_access = NTCREATEX_SHARE_ACCESS_MASK, >@@ -2143,9 +2246,145 @@ static bool test_twrp_write(struct torture_context *tctx, struct smb2_tree *tree > NT_STATUS_MEDIA_WRITE_PROTECTED, > ret, done, "smb2_create\n"); > >+ /* >+ * Verify access mask >+ */ >+ >+ ZERO_STRUCT(getinfo); >+ getinfo.generic.level = RAW_FILEINFO_ACCESS_INFORMATION; >+ getinfo.generic.in.file.handle = h1; >+ >+ status = smb2_getinfo_file(tree, tree, &getinfo); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_getinfo_file\n"); >+ >+ torture_assert_int_equal_goto( >+ tctx, >+ getinfo.access_information.out.access_flags, >+ expected_access, >+ ret, done, >+ "Bad access mask\n"); >+ >+ /* >+ * Check we can't set various things >+ */ >+ >+ ZERO_STRUCT(getinfo); >+ getinfo.query_secdesc.level = RAW_FILEINFO_SEC_DESC; >+ getinfo.query_secdesc.in.file.handle = h1; >+ getinfo.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER; >+ >+ status = smb2_getinfo_file(tree, tctx, &getinfo); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_getinfo_file\n"); >+ >+ sd_orig = getinfo.query_secdesc.out.sd; >+ owner_sid = dom_sid_string(tctx, sd_orig->owner_sid); >+ >+ sd = security_descriptor_dacl_create(tctx, >+ 0, NULL, NULL, >+ owner_sid, >+ SEC_ACE_TYPE_ACCESS_ALLOWED, >+ SEC_FILE_WRITE_DATA, >+ 0, >+ NULL); >+ >+ /* Try to set ACL */ >+ >+ ZERO_STRUCT(setinfo); >+ setinfo.set_secdesc.level = RAW_SFILEINFO_SEC_DESC; >+ setinfo.set_secdesc.in.file.handle = h1; >+ setinfo.set_secdesc.in.secinfo_flags = SECINFO_DACL; >+ setinfo.set_secdesc.in.sd = sd; >+ >+ status = smb2_setinfo_file(tree, &setinfo); >+ torture_assert_ntstatus_equal_goto( >+ tctx, >+ status, >+ NT_STATUS_MEDIA_WRITE_PROTECTED, >+ ret, done, >+ "smb2_setinfo_file\n"); >+ >+ /* Try to delete */ >+ >+ ZERO_STRUCT(setinfo); >+ setinfo.generic.level = RAW_SFILEINFO_DISPOSITION_INFORMATION; >+ setinfo.disposition_info.in.delete_on_close = 1; >+ setinfo.generic.in.file.handle = h1; >+ >+ status = smb2_setinfo_file(tree, &setinfo); >+ torture_assert_ntstatus_equal_goto( >+ tctx, >+ status, >+ NT_STATUS_MEDIA_WRITE_PROTECTED, >+ ret, done, >+ "smb2_setinfo_file\n"); >+ >+ ZERO_STRUCT(setinfo); >+ setinfo.basic_info.in.attrib = FILE_ATTRIBUTE_HIDDEN; >+ setinfo.generic.level = RAW_SFILEINFO_BASIC_INFORMATION; >+ setinfo.generic.in.file.handle = h1; >+ >+ status = smb2_setinfo_file(tree, &setinfo); >+ torture_assert_ntstatus_equal_goto( >+ tctx, >+ status, >+ NT_STATUS_MEDIA_WRITE_PROTECTED, >+ ret, done, >+ "smb2_setinfo_file\n"); >+ >+ /* Try to truncate */ >+ >+ ZERO_STRUCT(setinfo); >+ setinfo.generic.level = SMB_SFILEINFO_END_OF_FILE_INFORMATION; >+ setinfo.generic.in.file.handle = h1; >+ setinfo.end_of_file_info.in.size = 0x100000; >+ >+ status = smb2_setinfo_file(tree, &setinfo); >+ torture_assert_ntstatus_equal_goto( >+ tctx, >+ status, >+ NT_STATUS_MEDIA_WRITE_PROTECTED, >+ ret, done, >+ "smb2_setinfo_file\n"); >+ >+ /* Try to set a hardlink */ >+ >+ ZERO_STRUCT(setinfo); >+ setinfo.generic.level = RAW_SFILEINFO_LINK_INFORMATION; >+ setinfo.generic.in.file.handle = h1; >+ setinfo.link_information.in.new_name = "hardlink"; >+ >+ status = smb2_setinfo_file(tree, &setinfo); >+ torture_assert_ntstatus_equal_goto( >+ tctx, >+ status, >+ NT_STATUS_NOT_SAME_DEVICE, >+ ret, done, >+ "smb2_setinfo_file\n"); >+ >+ /* Try to rename */ >+ >+ ZERO_STRUCT(setinfo); >+ setinfo.rename_information.level = RAW_SFILEINFO_RENAME_INFORMATION; >+ setinfo.rename_information.in.file.handle = h1; >+ setinfo.rename_information.in.new_name = "renamed"; >+ >+ status = smb2_setinfo_file(tree, &setinfo); >+ torture_assert_ntstatus_equal_goto( >+ tctx, >+ status, >+ NT_STATUS_NOT_SAME_DEVICE, >+ ret, done, >+ "smb2_setinfo_file\n"); >+ > smb2_util_close(tree, h1); >+ ZERO_STRUCT(h1); > > done: >+ if (!smb2_util_handle_empty(h1)) { >+ smb2_util_close(tree, h1); >+ } > return ret; > } > >-- >2.43.0 > > >From 45a967ae3eed4db66757fa49a1a009b838dc86bd Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Tue, 19 Dec 2023 13:06:55 +0100 >Subject: [PATCH 04/13] smbd: return the correct error in can_rename() > >This is what Windows returns for this case. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13688 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 276c5bd851ab6ab818a49d9c47f6b96de8024778) >--- > source3/smbd/smb2_reply.c | 5 +++++ > 1 file changed, 5 insertions(+) > >diff --git a/source3/smbd/smb2_reply.c b/source3/smbd/smb2_reply.c >index 9476c69b73ca..a4055c00a312 100644 >--- a/source3/smbd/smb2_reply.c >+++ b/source3/smbd/smb2_reply.c >@@ -1156,6 +1156,11 @@ ssize_t sendfile_short_send(struct smbXsrv_connection *xconn, > static NTSTATUS can_rename(connection_struct *conn, files_struct *fsp, > uint16_t dirtype) > { >+ if (fsp->fsp_name->twrp != 0) { >+ /* Get the error right, this is what Windows returns. */ >+ return NT_STATUS_NOT_SAME_DEVICE; >+ } >+ > if (!CAN_WRITE(conn)) { > return NT_STATUS_MEDIA_WRITE_PROTECTED; > } >-- >2.43.0 > > >From a071390bc6ce86885ef09b4c14beb4e5d551a6a2 Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Thu, 21 Dec 2023 16:27:42 +0100 >Subject: [PATCH 05/13] smbd: set fsp_flags.is_fsa to true on printer file > handles > >Printer file handles went through SMB_VFS_CREATE_FILE() and are network >callable, so it makes sense to set this on them. > >This ensures that check_access_fsp() doesn't take the codepath calling >smbd_check_access_rights_fsp(), but just checks the request rights from >fsp->access_mask. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13688 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 76c8fe16bff36a29fa326355256b50737d04bd85) >--- > source3/printing/printspoolss.c | 1 + > 1 file changed, 1 insertion(+) > >diff --git a/source3/printing/printspoolss.c b/source3/printing/printspoolss.c >index 31117a4b7438..94404f7682ab 100644 >--- a/source3/printing/printspoolss.c >+++ b/source3/printing/printspoolss.c >@@ -244,6 +244,7 @@ NTSTATUS print_spool_open(files_struct *fsp, > fsp->sent_oplock_break = NO_BREAK_SENT; > fsp->fsp_flags.is_directory = false; > fsp->fsp_flags.delete_on_close = false; >+ fsp->fsp_flags.is_fsa = true; > > fsp->print_file = pf; > >-- >2.43.0 > > >From bf44a38daaf168c930746d8a4bce202d07523a59 Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Thu, 21 Dec 2023 10:58:09 +0100 >Subject: [PATCH 06/13] smbd: rename check_access_fsp() to > check_any_access_fsp() > >The semantics of the access check in check_access_fsp() itself is to >allow access if *at least* one or more rights of the rights in >access_mask are allowed. The name check_any_access_fsp() better >reflects this. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13688 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 96b577c380fa914eb1ffa95849c82bdb88aa1ec6) >--- > source3/smbd/proto.h | 4 ++-- > source3/smbd/smb2_ioctl_filesys.c | 6 +++--- > source3/smbd/smb2_trans2.c | 15 ++++++++++----- > 3 files changed, 15 insertions(+), 10 deletions(-) > >diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h >index 26cafeba091f..f3f998a9c0f2 100644 >--- a/source3/smbd/proto.h >+++ b/source3/smbd/proto.h >@@ -1114,8 +1114,8 @@ NTSTATUS smb_set_file_disposition_info(connection_struct *conn, > files_struct *fsp, > struct smb_filename *smb_fname); > NTSTATUS refuse_symlink_fsp(const struct files_struct *fsp); >-NTSTATUS check_access_fsp(struct files_struct *fsp, >- uint32_t access_mask); >+NTSTATUS check_any_access_fsp(struct files_struct *fsp, >+ uint32_t access_mask); > uint64_t smb_roundup(connection_struct *conn, uint64_t val); > bool samba_private_attr_name(const char *unix_ea_name); > NTSTATUS get_ea_value_fsp(TALLOC_CTX *mem_ctx, >diff --git a/source3/smbd/smb2_ioctl_filesys.c b/source3/smbd/smb2_ioctl_filesys.c >index 36429b8fd352..6cc53d4828ed 100644 >--- a/source3/smbd/smb2_ioctl_filesys.c >+++ b/source3/smbd/smb2_ioctl_filesys.c >@@ -378,7 +378,7 @@ static NTSTATUS fsctl_set_cmprn(TALLOC_CTX *mem_ctx, > } > > /* WRITE_DATA permission is required, WRITE_ATTRIBUTES is not */ >- status = check_access_fsp(fsp, FILE_WRITE_DATA); >+ status = check_any_access_fsp(fsp, FILE_WRITE_DATA); > if (!NT_STATUS_IS_OK(status)) { > return status; > } >@@ -426,7 +426,7 @@ static NTSTATUS fsctl_zero_data(TALLOC_CTX *mem_ctx, > } > > /* WRITE_DATA permission is required */ >- status = check_access_fsp(fsp, FILE_WRITE_DATA); >+ status = check_any_access_fsp(fsp, FILE_WRITE_DATA); > if (!NT_STATUS_IS_OK(status)) { > return status; > } >@@ -616,7 +616,7 @@ static NTSTATUS fsctl_qar(TALLOC_CTX *mem_ctx, > } > > /* READ_DATA permission is required */ >- status = check_access_fsp(fsp, FILE_READ_DATA); >+ status = check_any_access_fsp(fsp, FILE_READ_DATA); > if (!NT_STATUS_IS_OK(status)) { > return status; > } >diff --git a/source3/smbd/smb2_trans2.c b/source3/smbd/smb2_trans2.c >index fa9e8b4509b3..80a932566b28 100644 >--- a/source3/smbd/smb2_trans2.c >+++ b/source3/smbd/smb2_trans2.c >@@ -72,8 +72,13 @@ NTSTATUS refuse_symlink_fsp(const files_struct *fsp) > return NT_STATUS_OK; > } > >-NTSTATUS check_access_fsp(struct files_struct *fsp, >- uint32_t access_mask) >+/** >+ * Check that one or more of the rights in access_mask are >+ * allowed. Iow, access_mask can contain more then one right and >+ * it is sufficient having only one of those granted to pass. >+ **/ >+NTSTATUS check_any_access_fsp(struct files_struct *fsp, >+ uint32_t access_mask) > { > if (!fsp->fsp_flags.is_fsa) { > return smbd_check_access_rights_fsp(fsp->conn->cwd_fsp, >@@ -677,7 +682,7 @@ NTSTATUS set_ea(connection_struct *conn, files_struct *fsp, > return status; > } > >- status = check_access_fsp(fsp, FILE_WRITE_EA); >+ status = check_any_access_fsp(fsp, FILE_WRITE_EA); > if (!NT_STATUS_IS_OK(status)) { > return status; > } >@@ -4822,7 +4827,7 @@ static NTSTATUS smb_set_file_basic_info(connection_struct *conn, > return NT_STATUS_INVALID_HANDLE; > } > >- status = check_access_fsp(fsp, FILE_WRITE_ATTRIBUTES); >+ status = check_any_access_fsp(fsp, FILE_WRITE_ATTRIBUTES); > if (!NT_STATUS_IS_OK(status)) { > return status; > } >@@ -4893,7 +4898,7 @@ static NTSTATUS smb_set_info_standard(connection_struct *conn, > DEBUG(10,("smb_set_info_standard: file %s\n", > smb_fname_str_dbg(smb_fname))); > >- status = check_access_fsp(fsp, FILE_WRITE_ATTRIBUTES); >+ status = check_any_access_fsp(fsp, FILE_WRITE_ATTRIBUTES); > if (!NT_STATUS_IS_OK(status)) { > return status; > } >-- >2.43.0 > > >From 5a898712c17c507e36b555dfee566da17f7920e2 Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Thu, 21 Dec 2023 10:58:09 +0100 >Subject: [PATCH 07/13] smbd: fix check_any_access_fsp() for non-fsa fsps > >smbd_check_access_rights_fsp() requires *all* rights in access_mask to >be granted by the underlying ACL, but the semantics of this function >is supposed to grant access if any one of the rights in >access_requested is allowed. > >Fix this by looping over the requested access mask. If >smbd_check_access_rights_fsp() returns sucess, mask will be non-null >and when assigned to access_granted, the subsequent check will pass, >fail otherwise. > >I'm not doing an early exit on purpose because a subsequent commit >adds additional security checks that are done in the subsequent code >path common for fsa and non-fsa fsps. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13688 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit bf497819e61131cfa6469971596af3aa9bd4bb49) >--- > source3/smbd/proto.h | 2 +- > source3/smbd/smb2_trans2.c | 38 +++++++++++++++++++++++++++++--------- > 2 files changed, 30 insertions(+), 10 deletions(-) > >diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h >index f3f998a9c0f2..eea56f6e0a82 100644 >--- a/source3/smbd/proto.h >+++ b/source3/smbd/proto.h >@@ -1115,7 +1115,7 @@ NTSTATUS smb_set_file_disposition_info(connection_struct *conn, > struct smb_filename *smb_fname); > NTSTATUS refuse_symlink_fsp(const struct files_struct *fsp); > NTSTATUS check_any_access_fsp(struct files_struct *fsp, >- uint32_t access_mask); >+ uint32_t access_requested); > uint64_t smb_roundup(connection_struct *conn, uint64_t val); > bool samba_private_attr_name(const char *unix_ea_name); > NTSTATUS get_ea_value_fsp(TALLOC_CTX *mem_ctx, >diff --git a/source3/smbd/smb2_trans2.c b/source3/smbd/smb2_trans2.c >index 80a932566b28..f6a3166719e5 100644 >--- a/source3/smbd/smb2_trans2.c >+++ b/source3/smbd/smb2_trans2.c >@@ -73,20 +73,40 @@ NTSTATUS refuse_symlink_fsp(const files_struct *fsp) > } > > /** >- * Check that one or more of the rights in access_mask are >- * allowed. Iow, access_mask can contain more then one right and >+ * Check that one or more of the rights in access mask are >+ * allowed. Iow, access_requested can contain more then one right and > * it is sufficient having only one of those granted to pass. > **/ > NTSTATUS check_any_access_fsp(struct files_struct *fsp, >- uint32_t access_mask) >+ uint32_t access_requested) > { >- if (!fsp->fsp_flags.is_fsa) { >- return smbd_check_access_rights_fsp(fsp->conn->cwd_fsp, >- fsp, >- false, >- access_mask); >+ uint32_t access_granted = 0; >+ NTSTATUS status; >+ >+ if (fsp->fsp_flags.is_fsa) { >+ access_granted = fsp->access_mask; >+ } else { >+ uint32_t mask = 1; >+ >+ while (mask != 0) { >+ if (!(mask & access_requested)) { >+ mask <<= 1; >+ continue; >+ } >+ >+ status = smbd_check_access_rights_fsp( >+ fsp->conn->cwd_fsp, >+ fsp, >+ false, >+ mask); >+ if (NT_STATUS_IS_OK(status)) { >+ break; >+ } >+ mask <<= 1; >+ } >+ access_granted = mask; > } >- if (!(fsp->access_mask & access_mask)) { >+ if ((access_granted & access_requested) == 0) { > return NT_STATUS_ACCESS_DENIED; > } > return NT_STATUS_OK; >-- >2.43.0 > > >From 4eba27394d7f55c2c4e4ff78cb82b6890c27be7b Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Fri, 22 Dec 2023 11:19:38 +0100 >Subject: [PATCH 08/13] smbd: return correct error when trying to create a > hardlink to a VSS file > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13688 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit a0ae45be770a13373c148a689b9761f14c4f942c) >--- > source3/smbd/smb2_trans2.c | 5 +++++ > 1 file changed, 5 insertions(+) > >diff --git a/source3/smbd/smb2_trans2.c b/source3/smbd/smb2_trans2.c >index f6a3166719e5..03639f218db7 100644 >--- a/source3/smbd/smb2_trans2.c >+++ b/source3/smbd/smb2_trans2.c >@@ -3904,6 +3904,11 @@ NTSTATUS hardlink_internals(TALLOC_CTX *ctx, > goto out; > } > >+ if (smb_fname_old->twrp != 0) { >+ status = NT_STATUS_NOT_SAME_DEVICE; >+ goto out; >+ } >+ > status = parent_pathref(talloc_tos(), > conn->cwd_fsp, > smb_fname_old, >-- >2.43.0 > > >From 5b390fb1cc59345a538f505066da05ae90ad5bea Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Wed, 20 Dec 2023 18:03:22 +0100 >Subject: [PATCH 09/13] smbd: set fsp->fsp_flags.can_write to false for access > to previous-versions > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13688 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit ee3035218df4cfd68b6aab6825c78f2b85234c6c) >--- > source3/smbd/files.c | 3 +++ > source3/smbd/open.c | 3 +++ > 2 files changed, 6 insertions(+) > >diff --git a/source3/smbd/files.c b/source3/smbd/files.c >index d3f6b629264a..32388f133f37 100644 >--- a/source3/smbd/files.c >+++ b/source3/smbd/files.c >@@ -1977,6 +1977,9 @@ NTSTATUS dup_file_fsp( > to->fsp_flags.can_write = > CAN_WRITE(from->conn) && > ((access_mask & (FILE_WRITE_DATA | FILE_APPEND_DATA)) != 0); >+ if (from->fsp_name->twrp != 0) { >+ to->fsp_flags.can_write = false; >+ } > to->fsp_flags.modified = from->fsp_flags.modified; > to->fsp_flags.is_directory = from->fsp_flags.is_directory; > to->fsp_flags.aio_write_behind = from->fsp_flags.aio_write_behind; >diff --git a/source3/smbd/open.c b/source3/smbd/open.c >index c848243c9b58..e804d0969077 100644 >--- a/source3/smbd/open.c >+++ b/source3/smbd/open.c >@@ -1678,6 +1678,9 @@ static NTSTATUS open_file(struct smb_request *req, > fsp->fsp_flags.can_write = > CAN_WRITE(conn) && > ((access_mask & (FILE_WRITE_DATA | FILE_APPEND_DATA)) != 0); >+ if (fsp->fsp_name->twrp != 0) { >+ fsp->fsp_flags.can_write = false; >+ } > fsp->print_file = NULL; > fsp->fsp_flags.modified = false; > fsp->sent_oplock_break = NO_BREAK_SENT; >-- >2.43.0 > > >From 1fe96c45b3f27b6e6a0bafae482c3db2923b0b15 Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Wed, 20 Dec 2023 18:32:25 +0100 >Subject: [PATCH 10/13] 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 <slow@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(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 42ff9ffb0d45..1851709774a4 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 901991daf28d..3b71a0028fb7 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 abdc40fd2b28..9129005dcaeb 100644 >--- a/source3/smbd/smb1_reply.c >+++ b/source3/smbd/smb1_reply.c >@@ -3626,8 +3626,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; >@@ -3857,8 +3858,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; > } >@@ -3992,8 +3994,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; > } >@@ -4271,8 +4274,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; > } > >@@ -4983,6 +4987,7 @@ void reply_writeclose(struct smb_request *req) > struct timespec mtime; > files_struct *fsp; > struct lock_struct lock; >+ NTSTATUS status; > > START_PROFILE(SMBwriteclose); > >@@ -4998,8 +5003,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; > } >@@ -5799,6 +5805,7 @@ void reply_printwrite(struct smb_request *req) > int numtowrite; > const char *data; > files_struct *fsp; >+ NTSTATUS status; > > START_PROFILE(SMBsplwr); > >@@ -5821,8 +5828,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 5c8c171e418e..6d1c3022c21e 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 9fa87765b8f3..b5a5aeba80bc 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.43.0 > > >From b72f6c17a335a2f187a737f9b3a53f6727db68c6 Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Wed, 20 Dec 2023 18:01:57 +0100 >Subject: [PATCH 11/13] 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 <slow@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(backported from commit 02ed99343d19fd0845531ad99a46b1dd5b8a7a4f) >[slow@samba.org: vfs_acl_common.c: different chown_needed check] >--- > 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 81e1116b20b0..cd987b47f0a3 100644 >--- a/source3/modules/vfs_acl_common.c >+++ b/source3/modules/vfs_acl_common.c >@@ -742,10 +742,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 == false || >- !(fsp->access_mask & SEC_STD_WRITE_OWNER)) { >+ if (get_current_uid(handle->conn) == 0 || 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/modules/vfs_nfs4acl_xattr.c b/source3/modules/vfs_nfs4acl_xattr.c >index cecafcf50b8d..1fd3519ca020 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 eb263132adf7..4f4ea42405b3 100644 >--- a/source3/smbd/dir.c >+++ b/source3/smbd/dir.c >@@ -233,11 +233,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 5a88cd059b04..f0d66557ded8 100644 >--- a/source3/smbd/dosmode.c >+++ b/source3/smbd/dosmode.c >@@ -1086,15 +1086,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 476aead590dc..7c4c7156d245 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 c18ad0027b67..68f8539beeee 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 9129005dcaeb..740fb5fa9f78 100644 >--- a/source3/smbd/smb1_reply.c >+++ b/source3/smbd/smb1_reply.c >@@ -6657,8 +6657,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 6d1c3022c21e..3bd0b2a93bd6 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 f5156147bc3c..cc6f39548a9a 100644 >--- a/source3/smbd/smb2_getinfo.c >+++ b/source3/smbd/smb2_getinfo.c >@@ -316,15 +316,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 84defa3f0528..54186f850873 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 a4055c00a312..8d8d8e0bc715 100644 >--- a/source3/smbd/smb2_reply.c >+++ b/source3/smbd/smb2_reply.c >@@ -1156,6 +1156,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; >@@ -1199,11 +1201,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 03639f218db7..b5d73bdf1fa5 100644 >--- a/source3/smbd/smb2_trans2.c >+++ b/source3/smbd/smb2_trans2.c >@@ -4167,8 +4167,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) { >@@ -4981,8 +4982,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.43.0 > > >From 9c4b78a7dc1a2b18fc8c683f14a782395b48c7cb Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Wed, 20 Dec 2023 15:09:59 +0100 >Subject: [PATCH 12/13] smbd: check for previous versions in > check_any_access_fsp() > >Now that check_any_access_fsp() is broadly used consistently to >restrict access for all modifying operations, we can add a check for >previous versions to check_any_access_fsp() and it gets enforced >consistently. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13688 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit fd4e41144a819b4403340e4a28664ac586722b41) >--- > .../samba3.blackbox.shadow_copy_torture | 1 - > source3/smbd/smb2_trans2.c | 22 +++++++++++++++++-- > 2 files changed, 20 insertions(+), 3 deletions(-) > delete mode 100644 selftest/knownfail.d/samba3.blackbox.shadow_copy_torture > >diff --git a/selftest/knownfail.d/samba3.blackbox.shadow_copy_torture b/selftest/knownfail.d/samba3.blackbox.shadow_copy_torture >deleted file mode 100644 >index 16537e58aeb7..000000000000 >--- a/selftest/knownfail.d/samba3.blackbox.shadow_copy_torture >+++ /dev/null >@@ -1 +0,0 @@ >-^samba3.blackbox.shadow_copy_torture.writing to shadow copy of a file\(fileserver\) >diff --git a/source3/smbd/smb2_trans2.c b/source3/smbd/smb2_trans2.c >index b5d73bdf1fa5..57ee3512581f 100644 >--- a/source3/smbd/smb2_trans2.c >+++ b/source3/smbd/smb2_trans2.c >@@ -80,6 +80,8 @@ NTSTATUS refuse_symlink_fsp(const files_struct *fsp) > NTSTATUS check_any_access_fsp(struct files_struct *fsp, > uint32_t access_requested) > { >+ const uint32_t ro_access = SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE; >+ uint32_t ro_access_granted = 0; > uint32_t access_granted = 0; > NTSTATUS status; > >@@ -100,15 +102,31 @@ NTSTATUS check_any_access_fsp(struct files_struct *fsp, > false, > mask); > if (NT_STATUS_IS_OK(status)) { >- break; >+ access_granted |= mask; >+ if (fsp->fsp_name->twrp == 0) { >+ /* >+ * We can only optimize >+ * the non-snapshot case >+ */ >+ break; >+ } > } > mask <<= 1; > } >- access_granted = mask; > } > if ((access_granted & access_requested) == 0) { > return NT_STATUS_ACCESS_DENIED; > } >+ >+ if (fsp->fsp_name->twrp == 0) { >+ return NT_STATUS_OK; >+ } >+ >+ ro_access_granted = access_granted & ro_access; >+ if ((ro_access_granted & access_requested) == 0) { >+ return NT_STATUS_MEDIA_WRITE_PROTECTED; >+ } >+ > return NT_STATUS_OK; > } > >-- >2.43.0 > > >From dfdcbcaa4d060484337b72d8da5c72c7d869863c Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Fri, 15 Dec 2023 11:59:36 +0100 >Subject: [PATCH 13/13] smbd: move access override for previous versions to the > SMB layer >MIME-Version: 1.0 >Content-Type: text/plain; charset=UTF-8 >Content-Transfer-Encoding: 8bit > >Doing the previous version access checks and semantics at the SMB >layer means we can simplify the shadow_copy2 and remove the kludge. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13688 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> > >Autobuild-User(master): Ralph Böhme <slow@samba.org> >Autobuild-Date(master): Mon Jan 8 16:58:26 UTC 2024 on atb-devel-224 > >(backported from commit f14a7065690b00e3c6af2c1f0b0aec51c1e0b372) >[slow@samba.org: vfs_shadow_copy2.c: no TALLOC_FREE() in context] >[slow@samba.org: open.c: assign result from calculate_open_access_flags()] >--- > source3/modules/vfs_shadow_copy2.c | 30 ++++------------------ > source3/smbd/open.c | 40 ++++++++++++++++++++++++++++-- > 2 files changed, 43 insertions(+), 27 deletions(-) > >diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c >index a2c9d3ce4c98..5bd5b87fb235 100644 >--- a/source3/modules/vfs_shadow_copy2.c >+++ b/source3/modules/vfs_shadow_copy2.c >@@ -1556,7 +1556,6 @@ static int shadow_copy2_openat(vfs_handle_struct *handle, > struct smb_filename *smb_fname = NULL; > time_t timestamp = 0; > char *stripped = NULL; >- bool is_converted = false; > int saved_errno = 0; > int ret; > bool ok; >@@ -1573,26 +1572,15 @@ static int shadow_copy2_openat(vfs_handle_struct *handle, > return -1; > } > >- ok = shadow_copy2_strip_snapshot_converted(talloc_tos(), >- handle, >- smb_fname, >- ×tamp, >- &stripped, >- &is_converted); >+ ok = shadow_copy2_strip_snapshot(talloc_tos(), >+ handle, >+ smb_fname, >+ ×tamp, >+ &stripped); > if (!ok) { > return -1; > } > if (timestamp == 0) { >- if (is_converted) { >- /* >- * Just pave over the user requested mode and use >- * O_RDONLY. Later attempts by the client to write on >- * the handle will fail in the pwrite() syscall with >- * EINVAL which we carefully map to EROFS. In sum, this >- * matches Windows behaviour. >- */ >- how.flags &= ~(O_WRONLY | O_RDWR | O_CREAT); >- } > return SMB_VFS_NEXT_OPENAT(handle, > dirfsp, > smb_fname_in, >@@ -1613,14 +1601,6 @@ static int shadow_copy2_openat(vfs_handle_struct *handle, > } > TALLOC_FREE(stripped); > >- /* >- * Just pave over the user requested mode and use O_RDONLY. Later >- * attempts by the client to write on the handle will fail in the >- * pwrite() syscall with EINVAL which we carefully map to EROFS. In sum, >- * this matches Windows behaviour. >- */ >- how.flags &= ~(O_WRONLY | O_RDWR | O_CREAT); >- > ret = SMB_VFS_NEXT_OPENAT(handle, > dirfsp, > smb_fname, >diff --git a/source3/smbd/open.c b/source3/smbd/open.c >index e804d0969077..edc72bb03f07 100644 >--- a/source3/smbd/open.c >+++ b/source3/smbd/open.c >@@ -3672,7 +3672,8 @@ static int disposition_to_open_flags(uint32_t create_disposition) > } > > static int calculate_open_access_flags(uint32_t access_mask, >- uint32_t private_flags) >+ uint32_t private_flags, >+ NTTIME twrp) > { > bool need_write, need_read; > >@@ -3681,6 +3682,15 @@ static int calculate_open_access_flags(uint32_t access_mask, > * mean the same thing under DOS and Unix. > */ > >+ if (twrp != 0) { >+ /* >+ * Pave over the user requested mode and force O_RDONLY for the >+ * file handle. Windows allows opening a VSS file with O_RDWR, >+ * even though actual writes on the handle will fail. >+ */ >+ return O_RDONLY; >+ } >+ > need_write = (access_mask & (FILE_WRITE_DATA | FILE_APPEND_DATA)); > if (!need_write) { > return O_RDONLY; >@@ -3811,6 +3821,7 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn, > bool posix_open = False; > bool new_file_created = False; > bool first_open_attempt = true; >+ bool is_twrp = (smb_fname_atname->twrp != 0); > NTSTATUS fsp_open = NT_STATUS_ACCESS_DENIED; > mode_t new_unx_mode = (mode_t)0; > mode_t unx_mode = (mode_t)0; >@@ -3968,6 +3979,9 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn, > smb_fname_str_dbg(smb_fname) )); > return NT_STATUS_OBJECT_NAME_NOT_FOUND; > } >+ if (is_twrp) { >+ return NT_STATUS_MEDIA_WRITE_PROTECTED; >+ } > break; > > case FILE_CREATE: >@@ -3983,11 +3997,24 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn, > } > return NT_STATUS_OBJECT_NAME_COLLISION; > } >+ if (is_twrp) { >+ return NT_STATUS_MEDIA_WRITE_PROTECTED; >+ } > break; > > case FILE_SUPERSEDE: > case FILE_OVERWRITE_IF: >+ if (is_twrp) { >+ return NT_STATUS_MEDIA_WRITE_PROTECTED; >+ } >+ break; > case FILE_OPEN_IF: >+ if (is_twrp) { >+ if (!file_existed) { >+ return NT_STATUS_MEDIA_WRITE_PROTECTED; >+ } >+ create_disposition = FILE_OPEN; >+ } > break; > default: > return NT_STATUS_INVALID_PARAMETER; >@@ -4060,7 +4087,9 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn, > * mean the same thing under DOS and Unix. > */ > >- flags = calculate_open_access_flags(access_mask, private_flags); >+ flags = calculate_open_access_flags(access_mask, >+ private_flags, >+ smb_fname->twrp); > > /* > * Currently we only look at FILE_WRITE_THROUGH for create options. >@@ -4788,6 +4817,10 @@ static NTSTATUS open_directory(connection_struct *conn, > return status; > } > >+ if (smb_fname_atname->twrp != 0) { >+ return NT_STATUS_MEDIA_WRITE_PROTECTED; >+ } >+ > status = mkdir_internal(conn, > parent_dir_fname, > smb_fname_atname, >@@ -4816,6 +4849,9 @@ static NTSTATUS open_directory(connection_struct *conn, > status = NT_STATUS_OK; > info = FILE_WAS_OPENED; > } else { >+ if (smb_fname_atname->twrp != 0) { >+ return NT_STATUS_MEDIA_WRITE_PROTECTED; >+ } > status = mkdir_internal(conn, > parent_dir_fname, > smb_fname_atname, >-- >2.43.0 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Flags:
metze
:
review+
Actions:
View
Attachments on
bug 13688
:
14671
|
14713
|
14722
|
18222
| 18223