From a0835984c052c1ed776b2cf187fc34e04cbbec33 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 28 Mar 2022 18:09:20 -0700 Subject: [PATCH 1/6] s3: tests.py: Only run smb2.rename against fileserver. No need to run this against nt4_dc or ad_dc. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15038 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (Back-ported from commit e01c5992b061d8ed54645fff52a73418013340ab) --- source3/selftest/tests.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index 7fbc5e385fc..ce95536b129 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -1015,6 +1015,8 @@ for t in tests: plansmbtorture4testsuite("smb2.async_dosmode", "simpleserver", "//$SERVER_IP/async_dosmode_shadow_copy2 -U$USERNAME%$PASSWORD") + elif t == "smb2.rename": + plansmbtorture4testsuite(t, "fileserver", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD') elif t == "rpc.wkssvc": plansmbtorture4testsuite(t, "ad_member", '//$SERVER/tmp -U$DC_USERNAME%$DC_PASSWORD') elif t == "rpc.srvsvc": -- 2.32.0 From 8f3d004aa34ffea3bf85c7bd2afd481383a32bb2 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 28 Mar 2022 18:23:05 -0700 Subject: [PATCH 2/6] s4: torture: Add CHECK_VAL macro to smb2/rename.c. Not yet used. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15038 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit e862a2d9ec4e7bec1dd58490e9dee47d543b9154) --- source4/torture/smb2/rename.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/source4/torture/smb2/rename.c b/source4/torture/smb2/rename.c index 5055cf24344..60234769797 100644 --- a/source4/torture/smb2/rename.c +++ b/source4/torture/smb2/rename.c @@ -30,6 +30,20 @@ #include "librpc/gen_ndr/security.h" +#define CHECK_VAL(v, correct) \ + do { \ + if ((v) != (correct)) { \ + torture_result(torture, \ + TORTURE_FAIL, \ + "(%s): wrong value for %s got " \ + "0x%llx - should be 0x%llx\n", \ + __location__, #v, \ + (unsigned long long)v, \ + (unsigned long long)correct); \ + ret = false; \ + goto done; \ + }} while (0) + #define CHECK_STATUS(status, correct) do { \ if (!NT_STATUS_EQUAL(status, correct)) { \ torture_result(torture, TORTURE_FAIL, \ -- 2.32.0 From 3765a752c02ca064124548f8fa5e959521dd8b18 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 28 Mar 2022 18:24:27 -0700 Subject: [PATCH 3/6] s4: torture: Add CHECK_CREATED macro to smb2/rename.c. Not yet used. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15038 Reviewed-by: Ralph Boehme Signed-off-by: Jeremy Allison (cherry picked from commit 4725ef5c96395dc2f48fab1160a3312d95e21416) --- source4/torture/smb2/rename.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/source4/torture/smb2/rename.c b/source4/torture/smb2/rename.c index 60234769797..c8d3c95d166 100644 --- a/source4/torture/smb2/rename.c +++ b/source4/torture/smb2/rename.c @@ -44,6 +44,14 @@ goto done; \ }} while (0) +#define CHECK_CREATED(__io, __created, __attribute) \ + do { \ + CHECK_VAL((__io)->out.create_action, NTCREATEX_ACTION_ ## __created); \ + CHECK_VAL((__io)->out.size, 0); \ + CHECK_VAL((__io)->out.file_attr, (__attribute)); \ + CHECK_VAL((__io)->out.reserved2, 0); \ + } while(0) + #define CHECK_STATUS(status, correct) do { \ if (!NT_STATUS_EQUAL(status, correct)) { \ torture_result(torture, TORTURE_FAIL, \ -- 2.32.0 From bc74174688fd06e408b7a18206f978c85e89f52a Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 28 Mar 2022 18:25:54 -0700 Subject: [PATCH 4/6] s4: torture: Add test_smb2_close_full_information() test to smb2.rename. Creates a file, opens it again on two different connections and then renames it. When we close and ask for SMB2_CLOSE_FLAGS_FULL_INFORMATION we expect this to succeed and return valid data on the handles that did not do the rename request. This currently succeeds by accident on master, so we are not adding a knownfail.d/ file here. When we back-port this test to 4.16.next, 4.15.next we will add a knownfail.d file. The rename request zeros out the fsp->fsp_name->st field on the handles that are open but are not being renamed, marking them as INVALID_STAT. This should not happen on any open handle. Fix to follow will preserve the field on rename in both the local connection and different connection case. Master gets away with this as in this branch, openat_pathref_fsp(), which we use in the setup_close_full_information() call to fetch the SMB2_CLOSE_FLAGS_FULL_INFORMATION data doesn't require an existing VALID_STAT struct in order to open the file. This hides the fact the rename zeroed out fsp->fsp_name->st. 4.16.x and 4.15.x don't have this fix, so expose the bug. Regardless, even in master we should not zero out any fsp->fsp_name->st values on rename. Add knownfail.d/rename-full-info for 4.16.x, 4.15.x. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15038 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (Back-ported from commit 1301e6461393601a4d43cfc465a05114e6ae4662) --- selftest/knownfail.d/rename-full-info | 1 + source4/torture/smb2/rename.c | 125 ++++++++++++++++++++++++++ 2 files changed, 126 insertions(+) create mode 100644 selftest/knownfail.d/rename-full-info diff --git a/selftest/knownfail.d/rename-full-info b/selftest/knownfail.d/rename-full-info new file mode 100644 index 00000000000..86ac19f4e48 --- /dev/null +++ b/selftest/knownfail.d/rename-full-info @@ -0,0 +1 @@ +^samba3.smb2.rename.close-full-information\(fileserver\) diff --git a/source4/torture/smb2/rename.c b/source4/torture/smb2/rename.c index c8d3c95d166..1b3dca8bcba 100644 --- a/source4/torture/smb2/rename.c +++ b/source4/torture/smb2/rename.c @@ -1437,6 +1437,127 @@ static bool torture_smb2_rename_dir_bench(struct torture_context *tctx, return true; } +static bool test_smb2_close_full_information(struct torture_context *torture, + struct smb2_tree *tree1, + struct smb2_tree *tree2) +{ + union smb_close cl; + struct smb2_create io = {0}; + struct smb2_handle h1 = {{0}}; + struct smb2_handle h2 = {{0}}; + struct smb2_handle h3 = {{0}}; + union smb_setfileinfo sinfo; + NTSTATUS status; + const char *fname_src = "request.dat"; + const char *fname_dst = "renamed.dat"; + bool ret = true; + + /* Start with a tidy share. */ + smb2_util_unlink(tree1, fname_src); + smb2_util_unlink(tree1, fname_dst); + + /* Create the test file, and leave it open. */ + io.in.fname = fname_src; + io.in.desired_access = SEC_FILE_READ_DATA | SEC_FILE_READ_ATTRIBUTE; + io.in.create_disposition = NTCREATEX_DISP_CREATE; + io.in.share_access = NTCREATEX_SHARE_ACCESS_READ | + NTCREATEX_SHARE_ACCESS_WRITE | + NTCREATEX_SHARE_ACCESS_DELETE; + status = smb2_create(tree1, tree1, &io); + CHECK_STATUS(status, NT_STATUS_OK); + h1 = io.out.file.handle; + CHECK_CREATED(&io, CREATED, FILE_ATTRIBUTE_ARCHIVE); + + /* Open the test file on the second connection. */ + ZERO_STRUCT(io); + io.in.fname = fname_src; + io.in.desired_access = SEC_FILE_READ_DATA | SEC_FILE_READ_ATTRIBUTE; + io.in.create_disposition = NTCREATEX_DISP_OPEN; + io.in.share_access = NTCREATEX_SHARE_ACCESS_READ | + NTCREATEX_SHARE_ACCESS_WRITE | + NTCREATEX_SHARE_ACCESS_DELETE; + status = smb2_create(tree2, tree2, &io); + CHECK_STATUS(status, NT_STATUS_OK); + h2 = io.out.file.handle; + + /* Now open for rename on the first connection. */ + ZERO_STRUCT(io); + io.in.fname = fname_src; + io.in.desired_access = SEC_STD_DELETE | SEC_FILE_READ_ATTRIBUTE; + io.in.create_disposition = NTCREATEX_DISP_OPEN; + io.in.share_access = NTCREATEX_SHARE_ACCESS_READ | + NTCREATEX_SHARE_ACCESS_WRITE | + NTCREATEX_SHARE_ACCESS_DELETE; + status = smb2_create(tree1, tree1, &io); + CHECK_STATUS(status, NT_STATUS_OK); + h3 = io.out.file.handle; + + /* Do the rename. */ + ZERO_STRUCT(sinfo); + sinfo.rename_information.level = RAW_SFILEINFO_RENAME_INFORMATION; + sinfo.rename_information.in.file.handle = h3; + sinfo.rename_information.in.new_name = fname_dst; + status = smb2_setinfo_file(tree1, &sinfo); + CHECK_STATUS(status, NT_STATUS_OK); + + /* And close h3. */ + ZERO_STRUCT(cl.smb2); + cl.smb2.level = RAW_CLOSE_SMB2; + cl.smb2.in.file.handle = h3; + status = smb2_close(tree1, &cl.smb2); + CHECK_STATUS(status, NT_STATUS_OK); + ZERO_STRUCT(h3); + + /* + * Close h1 with SMB2_CLOSE_FLAGS_FULL_INFORMATION. + * Ensure we get data. + */ + ZERO_STRUCT(cl.smb2); + cl.smb2.level = RAW_CLOSE_SMB2; + cl.smb2.in.file.handle = h1; + cl.smb2.in.flags = SMB2_CLOSE_FLAGS_FULL_INFORMATION; + status = smb2_close(tree1, &cl.smb2); + CHECK_STATUS(status, NT_STATUS_OK); + ZERO_STRUCT(h1); + CHECK_VAL(cl.smb2.out.file_attr, 0x20); + + /* + * Wait 3 seconds for name change to propagate + * to the other connection. + */ + sleep(3); + + /* + * Close h2 with SMB2_CLOSE_FLAGS_FULL_INFORMATION. + * This is on connection2. + * Ensure we get data. + */ + ZERO_STRUCT(cl.smb2); + cl.smb2.level = RAW_CLOSE_SMB2; + cl.smb2.in.file.handle = h2; + cl.smb2.in.flags = SMB2_CLOSE_FLAGS_FULL_INFORMATION; + status = smb2_close(tree2, &cl.smb2); + CHECK_STATUS(status, NT_STATUS_OK); + ZERO_STRUCT(h2); + CHECK_VAL(cl.smb2.out.file_attr, 0x20); + + done: + + if (h1.data[0] != 0 || h1.data[1] != 0) { + smb2_util_close(tree1, h1); + } + if (h2.data[0] != 0 || h2.data[1] != 0) { + smb2_util_close(tree2, h2); + } + if (h3.data[0] != 0 || h3.data[1] != 0) { + smb2_util_close(tree1, h3); + } + + smb2_util_unlink(tree1, fname_src); + smb2_util_unlink(tree1, fname_dst); + + return ret; +} /* basic testing of SMB2 rename @@ -1483,6 +1604,10 @@ struct torture_suite *torture_smb2_rename_init(TALLOC_CTX *ctx) "rename_dir_bench", torture_smb2_rename_dir_bench); + torture_suite_add_2smb2_test(suite, + "close-full-information", + test_smb2_close_full_information); + suite->description = talloc_strdup(suite, "smb2.rename tests"); return suite; -- 2.32.0 From d81e77f794700c8098e24c7fe84dfa2800a89368 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 28 Mar 2022 18:42:18 -0700 Subject: [PATCH 5/6] s3: smbd: Preserve the fsp->fsp_name->st bufs across rename_open_files() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15038 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 5e1aa469ae61af0442f432e0a2e3bf8c8709616a) --- source3/smbd/reply.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index 9b6ebb029fa..40313f3209c 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -6958,6 +6958,7 @@ static void rename_open_files(connection_struct *conn, for(fsp = file_find_di_first(conn->sconn, id, false); fsp; fsp = file_find_di_next(fsp, false)) { + SMB_STRUCT_STAT fsp_orig_sbuf; struct file_id_buf idbuf; /* fsp_name is a relative path under the fsp. To change this for other sharepaths we need to manipulate relative paths. */ @@ -6976,10 +6977,24 @@ static void rename_open_files(connection_struct *conn, fsp_str_dbg(fsp), smb_fname_str_dbg(smb_fname_dst)); + /* + * The incoming smb_fname_dst here has an + * invalid stat struct (it must not have + * existed for the rename to succeed). + * Preserve the existing stat from the + * open fsp after fsp_set_smb_fname() + * overwrites with the invalid stat. + * + * We will do an fstat before returning + * any of this metadata to the client anyway. + */ + fsp_orig_sbuf = fsp->fsp_name->st; status = fsp_set_smb_fname(fsp, smb_fname_dst); if (NT_STATUS_IS_OK(status)) { did_rename = True; new_name_hash = fsp->name_hash; + /* Restore existing stat. */ + fsp->fsp_name->st = fsp_orig_sbuf; } } -- 2.32.0 From a6147b343fa35c8e5a38895a536e3090ca3ce933 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 28 Mar 2022 18:39:55 -0700 Subject: [PATCH 6/6] s3: smbd: Preserve the fsp->fsp_name->st buf across a MSG_SMB_FILE_RENAME message. Remove knownfail.d/rename-full-info BUG: https://bugzilla.samba.org/show_bug.cgi?id=15038 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (Back-ported from commit 06bfac2125da5e4d37a596d1213912f0c698e69e) --- selftest/knownfail.d/rename-full-info | 1 - source3/smbd/open.c | 20 ++++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) delete mode 100644 selftest/knownfail.d/rename-full-info diff --git a/selftest/knownfail.d/rename-full-info b/selftest/knownfail.d/rename-full-info deleted file mode 100644 index 86ac19f4e48..00000000000 --- a/selftest/knownfail.d/rename-full-info +++ /dev/null @@ -1 +0,0 @@ -^samba3.smb2.rename.close-full-information\(fileserver\) diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 32df203e5ae..a94b85f8f20 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -4831,16 +4831,36 @@ void msg_file_was_renamed(struct messaging_context *msg_ctx, } if (strcmp(fsp->conn->connectpath, msg->servicepath) == 0) { + SMB_STRUCT_STAT fsp_orig_sbuf; NTSTATUS status; DBG_DEBUG("renaming file %s from %s -> %s\n", fsp_fnum_dbg(fsp), fsp_str_dbg(fsp), smb_fname_str_dbg(smb_fname)); + + /* + * The incoming smb_fname here has an + * invalid stat struct from synthetic_smb_fname() + * above. + * Preserve the existing stat from the + * open fsp after fsp_set_smb_fname() + * overwrites with the invalid stat. + * + * (We could just copy this into + * smb_fname->st, but keep this code + * identical to the fix in rename_open_files() + * for clarity. + * + * We will do an fstat before returning + * any of this metadata to the client anyway. + */ + fsp_orig_sbuf = fsp->fsp_name->st; status = fsp_set_smb_fname(fsp, smb_fname); if (!NT_STATUS_IS_OK(status)) { DBG_DEBUG("fsp_set_smb_fname failed: %s\n", nt_errstr(status)); } + fsp->fsp_name->st = fsp_orig_sbuf; } else { /* TODO. JRA. */ /* -- 2.32.0