From 77ef1e9b25043fe01385d52c1aa48e8c0a320db9 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 --- source3/selftest/tests.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index 2bfb38fdfff..cae09571fe1 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -1017,6 +1017,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 cae37eaf61467ff943ac5e39b694a6559ac6fea1 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 --- 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 4a4a14dccac2e719f54e6adc4b78249f9e18782a 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 Signed-off-by: Jeremy Allison --- 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 1ca76bce4acf730a5704e62372412c5fa80f335f 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. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15038 Signed-off-by: Jeremy Allison --- source4/torture/smb2/rename.c | 125 ++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) 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 f6f9300f13edc2c2c800bbc13d032144b2572ee9 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 --- source3/smbd/reply.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index 1677d997fe9..31f2d4d7a08 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -6960,6 +6960,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. */ @@ -6978,10 +6979,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 580740c4105e07fcfcb3d34287845845989ccbbb 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. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15038 Signed-off-by: Jeremy Allison --- source3/smbd/open.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 0d64e534149..51419d4272f 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -4887,16 +4887,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