From e16b37187252c6bc0a1c997c02fa6f01a5ccee3d Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 2 Oct 2024 14:07:49 +0200 Subject: [PATCH 1/5] smbtorture: prepare test_overwrite_read_only_file() for more subtests BUG: https://bugzilla.samba.org/show_bug.cgi?id=15732 Signed-off-by: Ralph Boehme Reviewed-by: Stefan Metzmacher (cherry picked from commit f88e52a6f487a216dbb805fabc08e862abb9b643) --- source4/torture/smb2/acls.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c index 019886ebcd21..f9c1158ee47c 100644 --- a/source4/torture/smb2/acls.c +++ b/source4/torture/smb2/acls.c @@ -2993,7 +2993,7 @@ static bool test_overwrite_read_only_file(struct torture_context *tctx, struct smb2_tree *tree) { NTSTATUS status; - struct smb2_create c; + struct smb2_create c = {}; const char *fname = BASEDIR "\\test_overwrite_read_only_file.txt"; struct smb2_handle handle = {{0}}; union smb_fileinfo q; @@ -3007,12 +3007,15 @@ static bool test_overwrite_read_only_file(struct torture_context *tctx, int disposition; const char *disposition_string; NTSTATUS expected_status; - } tcases[] = { + }; + #define TCASE(d, s) { \ .disposition = d, \ .disposition_string = #d, \ .expected_status = s, \ } + + struct tcase fs_tcases[] = { TCASE(NTCREATEX_DISP_OPEN, NT_STATUS_OK), TCASE(NTCREATEX_DISP_SUPERSEDE, NT_STATUS_ACCESS_DENIED), TCASE(NTCREATEX_DISP_OVERWRITE, NT_STATUS_ACCESS_DENIED), @@ -3075,12 +3078,12 @@ static bool test_overwrite_read_only_file(struct torture_context *tctx, smb2_util_close(tree, handle); ZERO_STRUCT(handle); - for (i = 0; i < ARRAY_SIZE(tcases); i++) { + for (i = 0; i < ARRAY_SIZE(fs_tcases); i++) { torture_comment(tctx, "Verify open with %s disposition\n", - tcases[i].disposition_string); + fs_tcases[i].disposition_string); c = (struct smb2_create) { - .in.create_disposition = tcases[i].disposition, + .in.create_disposition = fs_tcases[i].disposition, .in.desired_access = SEC_FILE_READ_DATA, .in.file_attributes = FILE_ATTRIBUTE_NORMAL, .in.share_access = NTCREATEX_SHARE_ACCESS_MASK, @@ -3091,7 +3094,7 @@ static bool test_overwrite_read_only_file(struct torture_context *tctx, status = smb2_create(tree, tctx, &c); smb2_util_close(tree, c.out.file.handle); torture_assert_ntstatus_equal_goto( - tctx, status, tcases[i].expected_status, ret, done, + tctx, status, fs_tcases[i].expected_status, ret, done, "smb2_create failed\n"); }; -- 2.47.0 From 7cd780d33bc043bb42202c71f661c69e236ad08c Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 2 Oct 2024 18:17:17 +0200 Subject: [PATCH 2/5] smbtorture: fix smb2.notify.mask test The strange function custom_smb2_create() was somehow causing NT_STATUS_DELETE_PENDING failures: failure: mask [ (../../source4/torture/smb2/notify.c:490) Incorrect status NT_STATUS_DELETE_PENDING - should be NT_STATUS_OK ] I couldn't figure out what was causing this exactly, but after doing these cleanups the error went away. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15732 Signed-off-by: Ralph Boehme Reviewed-by: Stefan Metzmacher (cherry picked from commit 4591f27ca81dff997ef7474565fc9c373abfa4a9) --- source4/torture/smb2/notify.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/source4/torture/smb2/notify.c b/source4/torture/smb2/notify.c index 0aadc50c6077..b76cb7967aa2 100644 --- a/source4/torture/smb2/notify.c +++ b/source4/torture/smb2/notify.c @@ -944,6 +944,7 @@ static bool torture_smb2_notify_mask(struct torture_context *torture, notify.smb2.out.changes[0].action, \ notify.smb2.in.completion_filter); \ ret = false; \ + goto done; \ } else if (notify.smb2.out.changes[0].action != Action) { \ torture_result(torture, TORTURE_FAIL, \ "ERROR: nchanges=%d action=%d " \ @@ -953,6 +954,7 @@ static bool torture_smb2_notify_mask(struct torture_context *torture, Action, \ notify.smb2.in.completion_filter); \ ret = false; \ + goto done; \ } else if (strcmp(notify.smb2.out.changes[0].name.s, \ "tname1") != 0) { \ torture_result(torture, TORTURE_FAIL, \ @@ -963,6 +965,7 @@ static bool torture_smb2_notify_mask(struct torture_context *torture, notify.smb2.in.completion_filter, \ notify.smb2.out.changes[0].name.s); \ ret = false; \ + goto done; \ } \ } \ } while (0); \ @@ -1016,14 +1019,12 @@ static bool torture_smb2_notify_mask(struct torture_context *torture, torture_comment(torture, "Testing rename file\n"); ZERO_STRUCT(sinfo); sinfo.rename_information.level = RAW_SFILEINFO_RENAME_INFORMATION; - sinfo.rename_information.in.file.handle = h1; sinfo.rename_information.in.overwrite = true; sinfo.rename_information.in.root_fid = 0; sinfo.rename_information.in.new_name = BASEDIR_MSK "\\tname2"; NOTIFY_MASK_TEST("Testing rename file", - smb2_util_close(tree2, custom_smb2_create(tree2, - torture, &(io1.smb2)));, - smb2_setinfo_file(tree2, &sinfo);, + torture_smb2_testfile(tree2, BASEDIR_MSK "\\tname1", &h2);, + (sinfo.rename_information.in.file.handle = h2, smb2_setinfo_file(tree2, &sinfo));, smb2_util_unlink(tree2, BASEDIR_MSK "\\tname2");, NOTIFY_ACTION_OLD_NAME, FILE_NOTIFY_CHANGE_FILE_NAME, 2); @@ -1031,21 +1032,19 @@ static bool torture_smb2_notify_mask(struct torture_context *torture, torture_comment(torture, "Testing rename dir\n"); ZERO_STRUCT(sinfo); sinfo.rename_information.level = RAW_SFILEINFO_RENAME_INFORMATION; - sinfo.rename_information.in.file.handle = h1; sinfo.rename_information.in.overwrite = true; sinfo.rename_information.in.root_fid = 0; sinfo.rename_information.in.new_name = BASEDIR_MSK "\\tname2"; NOTIFY_MASK_TEST("Testing rename dir", - smb2_util_mkdir(tree2, BASEDIR_MSK "\\tname1");, - smb2_setinfo_file(tree2, &sinfo);, - smb2_util_rmdir(tree2, BASEDIR_MSK "\\tname2");, + torture_smb2_testdir(tree2, BASEDIR_MSK "\\tname1", &h2);, + (sinfo.rename_information.in.file.handle = h2, smb2_setinfo_file(tree2, &sinfo));, + (smb2_util_close(tree2, h2), smb2_util_rmdir(tree2, BASEDIR_MSK "\\tname2"));, NOTIFY_ACTION_OLD_NAME, FILE_NOTIFY_CHANGE_DIR_NAME, 2); torture_comment(torture, "Testing set path attribute\n"); NOTIFY_MASK_TEST("Testing set path attribute", - smb2_util_close(tree2, custom_smb2_create(tree2, - torture, &(io.smb2)));, + torture_setup_simple_file(torture, tree2, BASEDIR_MSK "\\tname1");, smb2_util_setatr(tree2, BASEDIR_MSK "\\tname1", FILE_ATTRIBUTE_HIDDEN);, smb2_util_unlink(tree2, BASEDIR_MSK "\\tname1");, @@ -1055,12 +1054,10 @@ static bool torture_smb2_notify_mask(struct torture_context *torture, torture_comment(torture, "Testing set path write time\n"); ZERO_STRUCT(sinfo); sinfo.generic.level = RAW_SFILEINFO_BASIC_INFORMATION; - sinfo.generic.in.file.handle = h1; sinfo.basic_info.in.write_time = 1000; NOTIFY_MASK_TEST("Testing set path write time", - smb2_util_close(tree2, custom_smb2_create(tree2, - torture, &(io1.smb2)));, - smb2_setinfo_file(tree2, &sinfo);, + torture_setup_simple_file(torture, tree2, BASEDIR_MSK "\\tname1");, + (sinfo.generic.in.file.handle = h2, smb2_setinfo_file(tree2, &sinfo));, smb2_util_unlink(tree2, BASEDIR_MSK "\\tname1");, NOTIFY_ACTION_MODIFIED, FILE_NOTIFY_CHANGE_LAST_WRITE, 1); @@ -1073,13 +1070,12 @@ static bool torture_smb2_notify_mask(struct torture_context *torture, else { ZERO_STRUCT(sinfo); sinfo.generic.level = RAW_SFILEINFO_BASIC_INFORMATION; - sinfo.generic.in.file.handle = h1; sinfo.basic_info.in.create_time = 0; torture_comment(torture, "Testing set file create time\n"); NOTIFY_MASK_TEST("Testing set file create time", smb2_create_complex_file(torture, tree2, BASEDIR_MSK "\\tname1", &h2);, - smb2_setinfo_file(tree2, &sinfo);, + (sinfo.generic.in.file.handle = h2, smb2_setinfo_file(tree2, &sinfo));, (smb2_util_close(tree2, h2), smb2_util_unlink(tree2, BASEDIR_MSK "\\tname1"));, NOTIFY_ACTION_MODIFIED, @@ -1088,7 +1084,6 @@ static bool torture_smb2_notify_mask(struct torture_context *torture, ZERO_STRUCT(sinfo); sinfo.generic.level = RAW_SFILEINFO_BASIC_INFORMATION; - sinfo.generic.in.file.handle = h1; sinfo.basic_info.in.access_time = 0; torture_comment(torture, "Testing set file access time\n"); NOTIFY_MASK_TEST("Testing set file access time", @@ -1096,7 +1091,7 @@ static bool torture_smb2_notify_mask(struct torture_context *torture, tree2, BASEDIR_MSK "\\tname1", &h2);, - smb2_setinfo_file(tree2, &sinfo);, + (sinfo.generic.in.file.handle = h2, smb2_setinfo_file(tree2, &sinfo));, (smb2_util_close(tree2, h2), smb2_util_unlink(tree2, BASEDIR_MSK "\\tname1"));, NOTIFY_ACTION_MODIFIED, @@ -1104,7 +1099,6 @@ static bool torture_smb2_notify_mask(struct torture_context *torture, ZERO_STRUCT(sinfo); sinfo.generic.level = RAW_SFILEINFO_BASIC_INFORMATION; - sinfo.generic.in.file.handle = h1; sinfo.basic_info.in.change_time = 0; torture_comment(torture, "Testing set file change time\n"); NOTIFY_MASK_TEST("Testing set file change time", @@ -1112,7 +1106,7 @@ static bool torture_smb2_notify_mask(struct torture_context *torture, tree2, BASEDIR_MSK "\\tname1", &h2);, - smb2_setinfo_file(tree2, &sinfo);, + (sinfo.generic.in.file.handle = h2, smb2_setinfo_file(tree2, &sinfo));, (smb2_util_close(tree2, h2), smb2_util_unlink(tree2, BASEDIR_MSK "\\tname1"));, NOTIFY_ACTION_MODIFIED, -- 2.47.0 From 0f4887eccd4a009daae7286100ebdbee806ac910 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 2 Oct 2024 14:08:36 +0200 Subject: [PATCH 3/5] smbtorture: add subtests for overwrite dispositions vs sharemodes BUG: https://bugzilla.samba.org/show_bug.cgi?id=15732 Signed-off-by: Ralph Boehme Reviewed-by: Stefan Metzmacher (cherry picked from commit 849afe05ade140898b1eab9b28d46edc8357c844) --- selftest/knownfail.d/samba3.smb2.acls | 1 + source4/torture/smb2/acls.c | 109 +++++++++++++++++++++++++- 2 files changed, 108 insertions(+), 2 deletions(-) create mode 100644 selftest/knownfail.d/samba3.smb2.acls diff --git a/selftest/knownfail.d/samba3.smb2.acls b/selftest/knownfail.d/samba3.smb2.acls new file mode 100644 index 000000000000..18df260c0e50 --- /dev/null +++ b/selftest/knownfail.d/samba3.smb2.acls @@ -0,0 +1 @@ +^samba3.smb2.acls.OVERWRITE_READ_ONLY_FILE diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c index f9c1158ee47c..0459d6547dc7 100644 --- a/source4/torture/smb2/acls.c +++ b/source4/torture/smb2/acls.c @@ -2994,8 +2994,10 @@ static bool test_overwrite_read_only_file(struct torture_context *tctx, { NTSTATUS status; struct smb2_create c = {}; + struct smb2_create c2 = {}; const char *fname = BASEDIR "\\test_overwrite_read_only_file.txt"; struct smb2_handle handle = {{0}}; + struct smb2_handle h2 = {}; union smb_fileinfo q; union smb_setfileinfo set; struct security_descriptor *sd = NULL, *sd_orig = NULL; @@ -3021,6 +3023,12 @@ static bool test_overwrite_read_only_file(struct torture_context *tctx, TCASE(NTCREATEX_DISP_OVERWRITE, NT_STATUS_ACCESS_DENIED), TCASE(NTCREATEX_DISP_OVERWRITE_IF, NT_STATUS_ACCESS_DENIED), }; + + struct tcase sharing_tcases[] = { + TCASE(NTCREATEX_DISP_SUPERSEDE, NT_STATUS_SHARING_VIOLATION), + TCASE(NTCREATEX_DISP_OVERWRITE, NT_STATUS_SHARING_VIOLATION), + TCASE(NTCREATEX_DISP_OVERWRITE_IF, NT_STATUS_SHARING_VIOLATION), + }; #undef TCASE ret = smb2_util_setup_dir(tctx, tree, BASEDIR); @@ -3124,11 +3132,108 @@ static bool test_overwrite_read_only_file(struct torture_context *tctx, torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_setinfo_file failed\n"); - smb2_util_close(tree, handle); + status = smb2_util_close(tree, handle); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_util_close failed\n"); ZERO_STRUCT(handle); + for (i = 0; i < ARRAY_SIZE(sharing_tcases); i++) { + struct tcase *tcase = &sharing_tcases[i]; + + torture_comment(tctx, "Verify %s disposition\n", + tcase->disposition_string); + + torture_comment(tctx, "Read-nonly open file with SHARE_READ\n"); + + c = (struct smb2_create) { + .in.desired_access = SEC_FILE_READ_DATA, + .in.file_attributes = FILE_ATTRIBUTE_NORMAL, + .in.share_access = NTCREATEX_SHARE_ACCESS_READ, + .in.create_disposition = NTCREATEX_DISP_OPEN_IF, + .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, + .in.fname = fname, + }; + + status = smb2_create(tree, tctx, &c); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_create failed\n"); + handle = c.out.file.handle; + + torture_comment(tctx, "A second open with %s must return %s\n", + tcase->disposition_string, nt_errstr(tcase->expected_status)); + + c2 = (struct smb2_create) { + .in.desired_access = SEC_FILE_READ_DATA, + .in.file_attributes = FILE_ATTRIBUTE_NORMAL, + .in.share_access = NTCREATEX_SHARE_ACCESS_READ, + .in.create_disposition = tcase->disposition, + .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, + .in.fname = fname, + }; + + status = smb2_create(tree, tctx, &c2); + torture_assert_ntstatus_equal_goto(tctx, status, + tcase->expected_status, + ret, done, + "Wrong status code\n"); + + status = smb2_util_close(tree, handle); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_util_close failed\n"); + ZERO_STRUCT(handle); + + torture_comment(tctx, "First open with %s\n", + tcase->disposition_string); + + c = (struct smb2_create) { + .in.desired_access = SEC_FILE_READ_DATA, + .in.file_attributes = FILE_ATTRIBUTE_NORMAL, + .in.share_access = NTCREATEX_SHARE_ACCESS_READ, + .in.create_disposition = tcase->disposition, + .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, + .in.fname = fname, + }; + + status = smb2_create(tree, tctx, &c); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_create failed\n"); + handle = c.out.file.handle; + + torture_comment(tctx, "A second read-only open with SHARE_READ " + "must work\n"); + + c = (struct smb2_create) { + .in.desired_access = SEC_FILE_READ_DATA, + .in.file_attributes = FILE_ATTRIBUTE_NORMAL, + .in.share_access = NTCREATEX_SHARE_ACCESS_READ, + .in.create_disposition = NTCREATEX_DISP_OPEN, + .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, + .in.fname = fname, + }; + + status = smb2_create(tree, tctx, &c); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_create failed\n"); + h2 = c.out.file.handle; + + status = smb2_util_close(tree, handle); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_util_close failed\n"); + ZERO_STRUCT(handle); + + status = smb2_util_close(tree, h2); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_util_close failed\n"); + ZERO_STRUCT(h2); + } + done: - smb2_util_close(tree, handle); + if (!smb2_util_handle_empty(handle)) { + smb2_util_close(tree, handle); + } + if (!smb2_util_handle_empty(h2)) { + smb2_util_close(tree, h2); + } smb2_util_unlink(tree, fname); smb2_deltree(tree, BASEDIR); return ret; -- 2.47.0 From 331ec0992661bc79dca5de0a186f49448b686c67 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 2 Oct 2024 14:09:33 +0200 Subject: [PATCH 4/5] smbd: fix share access check for overwrite dispostions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BUG: https://bugzilla.samba.org/show_bug.cgi?id=15732 Signed-off-by: Ralph Boehme Reviewed-by: Stefan Metzmacher Autobuild-User(master): Ralph Böhme Autobuild-Date(master): Mon Oct 14 12:23:04 UTC 2024 on atb-devel-224 (cherry picked from commit 6140c3177a0330f42411618c3fca28930ea02a21) --- selftest/knownfail.d/samba3.smb2.acls | 1 - source3/smbd/open.c | 6 +++++- 2 files changed, 5 insertions(+), 2 deletions(-) delete mode 100644 selftest/knownfail.d/samba3.smb2.acls diff --git a/selftest/knownfail.d/samba3.smb2.acls b/selftest/knownfail.d/samba3.smb2.acls deleted file mode 100644 index 18df260c0e50..000000000000 --- a/selftest/knownfail.d/samba3.smb2.acls +++ /dev/null @@ -1 +0,0 @@ -^samba3.smb2.acls.OVERWRITE_READ_ONLY_FILE diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 22a39ac35ef6..ef3f65fea0fc 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -3389,6 +3389,7 @@ static NTSTATUS check_and_store_share_mode( struct share_mode_lock *lck, uint32_t create_disposition, uint32_t access_mask, + uint32_t open_access_mask, uint32_t share_access, int oplock_request, const struct smb2_lease *lease, @@ -3415,7 +3416,7 @@ static NTSTATUS check_and_store_share_mode( status = handle_share_mode_lease(fsp, lck, create_disposition, - access_mask, + open_access_mask, share_access, oplock_request, lease, @@ -3745,6 +3746,7 @@ struct open_ntcreate_lock_state { struct smb_request *req; uint32_t create_disposition; uint32_t access_mask; + uint32_t open_access_mask; uint32_t share_access; int oplock_request; const struct smb2_lease *lease; @@ -3773,6 +3775,7 @@ static void open_ntcreate_lock_add_entry(struct share_mode_lock *lck, lck, state->create_disposition, state->access_mask, + state->open_access_mask, state->share_access, state->oplock_request, state->lease, @@ -4407,6 +4410,7 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn, .req = req, .create_disposition = create_disposition, .access_mask = access_mask, + .open_access_mask = open_access_mask, .share_access = share_access, .oplock_request = oplock_request, .lease = lease, -- 2.47.0 From c518cc44deef83d540732985eb8f495845902cca Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 25 Oct 2024 17:22:57 +0200 Subject: [PATCH 5/5] smbd: fix sharing access check for directories This was missing from commit 6140c3177a0330f42411618c3fca28930ea02a21 and causes all opens of directories to be handled as stat opens, bypassing the sharemode check. Not adding a test at this time, as my (hopefully) soon to be merged Directory Leases branch has a test which actually detected this problem. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15732 Signed-off-by: Ralph Boehme Reviewed-by: Stefan Metzmacher (cherry picked from commit 20206a335a6af71b99f6441df145feea6563cf5a) --- source3/smbd/open.c | 1 + 1 file changed, 1 insertion(+) diff --git a/source3/smbd/open.c b/source3/smbd/open.c index ef3f65fea0fc..6c0a4dc0c55a 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -5109,6 +5109,7 @@ static NTSTATUS open_directory(connection_struct *conn, .req = req, .create_disposition = create_disposition, .access_mask = access_mask, + .open_access_mask = access_mask, .share_access = share_access, .oplock_request = NO_OPLOCK, .lease = NULL, -- 2.47.0