[PATCH] Deny file rename if file has open streams
Ralph Böhme
slow at samba.org
Sun May 27 14:57:54 UTC 2018
Hi!
Just stumpled across this one: Windows denies renaming a file that has open
streams with NT_STATUS_ACCESS_DENIED.
Please review & push if happy.
-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
-------------- next part --------------
From 4aac8b18d869551d969412b21ac58da3ea4ac0eb Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 26 May 2018 16:30:47 +0200
Subject: [PATCH 1/6] selftest: run smb2.streams tests against a share with
vfs_streams_xattr
The tests are currently only run against streams_depot, where stream IO
is handle based, compared to streams_xattr which is path
based. vfs_streams_xattr is also used much more in real world setups, so
we should run our tests against it.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13451
Signed-off-by: Ralph Boehme <slow at samba.org>
---
selftest/knownfail | 3 +++
source3/selftest/tests.py | 4 ++++
2 files changed, 7 insertions(+)
diff --git a/selftest/knownfail b/selftest/knownfail
index 8c70d6a6172..7aa314a3528 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -177,6 +177,9 @@
^samba3.smb2.streams.rename
^samba3.smb2.streams.rename2
^samba3.smb2.streams.attributes
+^samba3.smb2.streams streams_xattr.rename\(nt4_dc\)
+^samba3.smb2.streams streams_xattr.rename2\(nt4_dc\)
+^samba3.smb2.streams streams_xattr.attributes\(nt4_dc\)
^samba3.smb2.getinfo.complex
^samba3.smb2.getinfo.fsinfo # quotas don't work yet
^samba3.smb2.setinfo.setinfo
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index bd84d001041..983f0ac1ae6 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -568,6 +568,10 @@ tests= base + raw + smb2 + rpc + unix + local + rap + nbt + libsmbclient + idmap
plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%%$PASSWORD '
'--option=torture:smburl=smb://$USERNAME:$PASSWORD@$SERVER/tmp '
'--option=torture:replace_smbconf=%s' % os.path.join(srcdir(), "testdata/samba3/smb_new.conf"))
+ elif t == "smb2.streams":
+ plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
+ plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
+ plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/streams_xattr -U$USERNAME%$PASSWORD', 'streams_xattr')
else:
plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
--
2.13.6
From 503671d568a6c66ff4995e52641fc3a42563f739 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 26 May 2018 16:07:14 +0200
Subject: [PATCH 2/6] s4:torture/smb2/streams: try to rename basefile while is
has open streams
This tests the following:
- create a file with a stream
- open the the stream and keep it open
- on a second connection, try to rename the basefile, this should fail
with NT_STATUS_ACCESS_DENIED
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13451
Signed-off-by: Ralph Boehme <slow at samba.org>
---
selftest/knownfail.d/samba3.smb2.streams | 2 +
source4/torture/smb2/streams.c | 82 ++++++++++++++++++++++++++++++++
2 files changed, 84 insertions(+)
create mode 100644 selftest/knownfail.d/samba3.smb2.streams
diff --git a/selftest/knownfail.d/samba3.smb2.streams b/selftest/knownfail.d/samba3.smb2.streams
new file mode 100644
index 00000000000..26d40a67bda
--- /dev/null
+++ b/selftest/knownfail.d/samba3.smb2.streams
@@ -0,0 +1,2 @@
+samba3.smb2.streams.basefile-rename-with-open-stream\(.*\)
+samba3.smb2.streams streams_xattr.basefile-rename-with-open-stream\(nt4_dc\)
diff --git a/source4/torture/smb2/streams.c b/source4/torture/smb2/streams.c
index d302bf923c9..b39d96d4924 100644
--- a/source4/torture/smb2/streams.c
+++ b/source4/torture/smb2/streams.c
@@ -1830,6 +1830,86 @@ static bool test_stream_attributes(struct torture_context *tctx,
return ret;
}
+static bool test_basefile_rename_with_open_stream(struct torture_context *tctx,
+ struct smb2_tree *tree)
+{
+ bool ret = true;
+ NTSTATUS status;
+ struct smb2_tree *tree2 = NULL;
+ struct smb2_create create, create2;
+ struct smb2_handle h1 = {{0}}, h2 = {{0}};
+ const char *fname = "test_rename_openfile";
+ const char *sname = "test_rename_openfile:foo";
+ const char *fname_renamed = "test_rename_openfile_renamed";
+ union smb_setfileinfo sinfo;
+ const char *data = "test data";
+
+ ret = torture_smb2_connection(tctx, &tree2);
+ torture_assert_goto(tctx, ret == true, ret, done,
+ "torture_smb2_connection failed\n");
+
+ torture_comment(tctx, "Creating file with stream\n");
+
+ ZERO_STRUCT(create);
+ create.in.desired_access = SEC_FILE_ALL;
+ create.in.share_access = NTCREATEX_SHARE_ACCESS_MASK;
+ create.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
+ create.in.create_disposition = NTCREATEX_DISP_OPEN_IF;
+ create.in.impersonation_level = SMB2_IMPERSONATION_IMPERSONATION;
+ create.in.fname = sname;
+
+ status = smb2_create(tree, tctx, &create);
+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+ "smb2_create failed\n");
+
+ h1 = create.out.file.handle;
+
+ torture_comment(tctx, "Writing to stream\n");
+
+ status = smb2_util_write(tree, h1, data, 0, strlen(data));
+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+ "smb2_util_write failed\n");
+
+ torture_comment(tctx, "Renaming base file\n");
+
+ ZERO_STRUCT(create2);
+ create2.in.desired_access = SEC_FILE_ALL;
+ create2.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
+ create2.in.share_access = NTCREATEX_SHARE_ACCESS_MASK;
+ create2.in.create_disposition = NTCREATEX_DISP_OPEN;
+ create2.in.impersonation_level = SMB2_IMPERSONATION_IMPERSONATION;
+ create2.in.fname = fname;
+
+ status = smb2_create(tree2, tctx, &create2);
+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+ "smb2_create failed\n");
+
+ h2 = create2.out.file.handle;
+
+ ZERO_STRUCT(sinfo);
+ sinfo.rename_information.level = RAW_SFILEINFO_RENAME_INFORMATION;
+ sinfo.rename_information.in.file.handle = h2;
+ sinfo.rename_information.in.new_name = fname_renamed;
+
+ status = smb2_setinfo_file(tree2, &sinfo);
+ torture_assert_ntstatus_equal_goto(
+ tctx, status, NT_STATUS_ACCESS_DENIED, ret, done,
+ "smb2_setinfo_file didn't return NT_STATUS_ACCESS_DENIED\n");
+
+ smb2_util_close(tree2, h2);
+
+done:
+ if (!smb2_util_handle_empty(h1)) {
+ smb2_util_close(tree, h1);
+ }
+ if (!smb2_util_handle_empty(h2)) {
+ smb2_util_close(tree2, h2);
+ }
+ smb2_util_unlink(tree, fname);
+ smb2_util_unlink(tree, fname_renamed);
+
+ return ret;
+}
/*
basic testing of streams calls SMB2
@@ -1850,6 +1930,8 @@ struct torture_suite *torture_smb2_streams_init(TALLOC_CTX *ctx)
torture_suite_add_1smb2_test(suite, "attributes", test_stream_attributes);
torture_suite_add_1smb2_test(suite, "delete", test_stream_delete);
torture_suite_add_1smb2_test(suite, "zero-byte", test_zero_byte_stream);
+ torture_suite_add_1smb2_test(suite, "basefile-rename-with-open-stream",
+ test_basefile_rename_with_open_stream);
suite->description = talloc_strdup(suite, "SMB2-STREAM tests");
--
2.13.6
From ecee684568cc1ca3b88a5bbfd60e6627592f40b4 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 26 May 2018 18:33:00 +0200
Subject: [PATCH 3/6] s4:torture/vfs/fruit: adjust test testing basefile rename
to expect failure
Renaming a basefile that has open streams must fail with
NT_STATUS_ACCESS_DENIED.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13451
Signed-off-by: Ralph Boehme <slow at samba.org>
---
selftest/knownfail.d/samba3.vfs.fruit | 3 +++
source4/torture/vfs/fruit.c | 25 ++++---------------------
2 files changed, 7 insertions(+), 21 deletions(-)
diff --git a/selftest/knownfail.d/samba3.vfs.fruit b/selftest/knownfail.d/samba3.vfs.fruit
index 8df25bccb79..bf97dbc5822 100644
--- a/selftest/knownfail.d/samba3.vfs.fruit
+++ b/selftest/knownfail.d/samba3.vfs.fruit
@@ -1 +1,4 @@
^samba3.vfs.fruit streams_depot.OS X AppleDouble file conversion\(nt4_dc\)
+^samba3.vfs.fruit metadata_netatalk.read open rsrc after rename\(nt4_dc\)
+^samba3.vfs.fruit metadata_stream.read open rsrc after rename\(nt4_dc\)
+^samba3.vfs.fruit streams_depot.read open rsrc after rename\(nt4_dc\)
diff --git a/source4/torture/vfs/fruit.c b/source4/torture/vfs/fruit.c
index 9310d051131..4bce5ad661f 100644
--- a/source4/torture/vfs/fruit.c
+++ b/source4/torture/vfs/fruit.c
@@ -3902,7 +3902,6 @@ static bool test_rename_and_read_rsrc(struct torture_context *tctx,
const char *fname_renamed = "test_rename_openfile_renamed";
const char *data = "1234567890";
union smb_setfileinfo sinfo;
- struct smb2_read r;
ret = enable_aapl(tctx, tree);
torture_assert_goto(tctx, ret == true, ret, done, "enable_aapl failed");
@@ -3954,28 +3953,12 @@ static bool test_rename_and_read_rsrc(struct torture_context *tctx,
sinfo.rename_information.in.new_name = fname_renamed;
status = smb2_setinfo_file(tree, &sinfo);
- torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_setinfo_file failed");
-
- smb2_util_close(tree, h2);
-
- ZERO_STRUCT(r);
- r.in.file.handle = h1;
- r.in.length = 10;
- r.in.offset = 0;
-
- torture_comment(tctx, "Read resource fork of renamed file\n");
-
- status = smb2_read(tree, tree, &r);
- torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_read failed");
+ torture_assert_ntstatus_equal_goto(
+ tctx, status, NT_STATUS_ACCESS_DENIED, ret, done,
+ "smb2_setinfo_file failed");
smb2_util_close(tree, h1);
-
- torture_assert_goto(tctx, r.out.data.length == 10, ret, done,
- talloc_asprintf(tctx, "smb2_read returned %jd bytes, expected 10\n",
- (intmax_t)r.out.data.length));
-
- torture_assert_goto(tctx, memcmp(r.out.data.data, data, 10) == 0, ret, done,
- talloc_asprintf(tctx, "Bad data in stream\n"));
+ smb2_util_close(tree, h2);
done:
smb2_util_unlink(tree, fname);
--
2.13.6
From 211073e88db364f7b88176297d9fc572109c89a1 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sun, 27 May 2018 13:01:50 +0200
Subject: [PATCH 4/6] s3:smbd: add private option
NTCREATEX_OPTIONS_PRIVATE_STREAM_BASEOPEN
This will be used to mark basefile opens of streams opens. This is
needed to later implement a function that can determine if a file has
stream opens.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13451
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/include/smb.h | 1 +
source3/smbd/open.c | 7 ++++++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/source3/include/smb.h b/source3/include/smb.h
index 3316f09d94f..0af6bcb421f 100644
--- a/source3/include/smb.h
+++ b/source3/include/smb.h
@@ -415,6 +415,7 @@ Offset Data length.
/* Private options for streams support */
#define NTCREATEX_OPTIONS_PRIVATE_STREAM_DELETE 0x0004
+#define NTCREATEX_OPTIONS_PRIVATE_STREAM_BASEOPEN 0x0008
/* Private options for printer support */
#define NTCREATEX_OPTIONS_PRIVATE_DELETE_ON_CLOSE 0x0008
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 61a42e29a10..5bb42e09d8c 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -5144,6 +5144,7 @@ static NTSTATUS create_file_unixpath(connection_struct *conn,
&& (!(private_flags & NTCREATEX_OPTIONS_PRIVATE_STREAM_DELETE))) {
uint32_t base_create_disposition;
struct smb_filename *smb_fname_base = NULL;
+ uint32_t base_privflags;
if (create_options & FILE_DIRECTORY_FILE) {
status = NT_STATUS_NOT_A_DIRECTORY;
@@ -5194,13 +5195,17 @@ static NTSTATUS create_file_unixpath(connection_struct *conn,
}
}
+ base_privflags = NTCREATEX_OPTIONS_PRIVATE_STREAM_BASEOPEN;
+
/* Open the base file. */
status = create_file_unixpath(conn, NULL, smb_fname_base, 0,
FILE_SHARE_READ
| FILE_SHARE_WRITE
| FILE_SHARE_DELETE,
base_create_disposition,
- 0, 0, 0, NULL, 0, 0, NULL, NULL,
+ 0, 0, 0, NULL, 0,
+ base_privflags,
+ NULL, NULL,
&base_fsp, NULL);
TALLOC_FREE(smb_fname_base);
--
2.13.6
From 6b7d0288fe26083593fe3d2be67fc19e4e85ef6a Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sun, 27 May 2018 13:03:25 +0200
Subject: [PATCH 5/6] s3:locking: add file_has_open_streams()
This can be used to check if a file opened by fsp also has stream opens.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13451
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/locking/locking.c | 33 +++++++++++++++++++++++++++++++++
source3/locking/proto.h | 1 +
2 files changed, 34 insertions(+)
diff --git a/source3/locking/locking.c b/source3/locking/locking.c
index 791878c6383..d1fc96722b9 100644
--- a/source3/locking/locking.c
+++ b/source3/locking/locking.c
@@ -1316,3 +1316,36 @@ struct timespec get_share_mode_write_time(struct share_mode_lock *lck)
}
return d->old_write_time;
}
+
+bool file_has_open_streams(files_struct *fsp)
+{
+ struct share_mode_lock *lock = NULL;
+ struct share_mode_data *d = NULL;
+ uint32_t i;
+
+ lock = get_existing_share_mode_lock(talloc_tos(), fsp->file_id);
+ if (lock == NULL) {
+ return false;
+ }
+ d = lock->data;
+
+ for (i = 0; i < d->num_share_modes; i++) {
+ struct share_mode_entry *e = &d->share_modes[i];
+
+ if (share_mode_stale_pid(d, i)) {
+ continue;
+ }
+ if (fsp->fh->gen_id == e->share_file_id) {
+ continue;
+ }
+ if (e->private_options &
+ NTCREATEX_OPTIONS_PRIVATE_STREAM_BASEOPEN)
+ {
+ TALLOC_FREE(lock);
+ return true;
+ }
+ }
+
+ TALLOC_FREE(lock);
+ return false;
+}
diff --git a/source3/locking/proto.h b/source3/locking/proto.h
index afd5373772a..403729c934a 100644
--- a/source3/locking/proto.h
+++ b/source3/locking/proto.h
@@ -207,6 +207,7 @@ bool is_delete_on_close_set(struct share_mode_lock *lck, uint32_t name_hash);
bool set_sticky_write_time(struct file_id fileid, struct timespec write_time);
bool set_write_time(struct file_id fileid, struct timespec write_time);
struct timespec get_share_mode_write_time(struct share_mode_lock *lck);
+bool file_has_open_streams(files_struct *fsp);
int share_mode_forall(int (*fn)(struct file_id fid,
const struct share_mode_data *data,
void *private_data),
--
2.13.6
From 4f476e3fac721074793ea45bff2079a6a9419dfb Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 26 May 2018 18:32:21 +0200
Subject: [PATCH 6/6] s3:smbd: don't allow renaming basefile if streams are
open
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13451
Signed-off-by: Ralph Boehme <slow at samba.org>
---
selftest/knownfail.d/samba3.smb2.streams | 2 --
selftest/knownfail.d/samba3.vfs.fruit | 3 ---
source3/smbd/reply.c | 4 ++++
3 files changed, 4 insertions(+), 5 deletions(-)
delete mode 100644 selftest/knownfail.d/samba3.smb2.streams
diff --git a/selftest/knownfail.d/samba3.smb2.streams b/selftest/knownfail.d/samba3.smb2.streams
deleted file mode 100644
index 26d40a67bda..00000000000
--- a/selftest/knownfail.d/samba3.smb2.streams
+++ /dev/null
@@ -1,2 +0,0 @@
-samba3.smb2.streams.basefile-rename-with-open-stream\(.*\)
-samba3.smb2.streams streams_xattr.basefile-rename-with-open-stream\(nt4_dc\)
diff --git a/selftest/knownfail.d/samba3.vfs.fruit b/selftest/knownfail.d/samba3.vfs.fruit
index bf97dbc5822..8df25bccb79 100644
--- a/selftest/knownfail.d/samba3.vfs.fruit
+++ b/selftest/knownfail.d/samba3.vfs.fruit
@@ -1,4 +1 @@
^samba3.vfs.fruit streams_depot.OS X AppleDouble file conversion\(nt4_dc\)
-^samba3.vfs.fruit metadata_netatalk.read open rsrc after rename\(nt4_dc\)
-^samba3.vfs.fruit metadata_stream.read open rsrc after rename\(nt4_dc\)
-^samba3.vfs.fruit streams_depot.read open rsrc after rename\(nt4_dc\)
diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index fc56e3234be..1b99664cbd8 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -6643,6 +6643,10 @@ NTSTATUS rename_internals_fsp(connection_struct *conn,
return status;
}
+ if (file_has_open_streams(fsp)) {
+ return NT_STATUS_ACCESS_DENIED;
+ }
+
/* Make a copy of the dst smb_fname structs */
smb_fname_dst = cp_smb_filename(ctx, smb_fname_dst_in);
--
2.13.6
More information about the samba-technical
mailing list