[PATCH] Fail to copy file with empty FinderInfo from Windows client to Samba share with vfs_fruit
Ralph Böhme
slow at samba.org
Fri Dec 8 11:12:10 UTC 2017
Hi!
Currently when a client sends a AFP_AfpInfo stream where the contained
FinderInfo blob is all 0, vfs_fruit will somewhat hackish delete the stream from
the backing store. This causes subsequent operations on the same filehandle to
fail for certain operations, also depending on the setting of fruit:metadata.
The correct implementation of the stream removal would be setting
delete-on-close on the filehandle, while at the same time checking in
delete-on-close is set in the stream-listing function.
Patch attached, please review & push if happy. Thanks!
-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
-------------- next part --------------
From 538c3c43e6ab47aa7f17e1d438a464d3d4a9942a Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 6 Dec 2017 22:05:23 +0100
Subject: [PATCH 1/5] s4/torture/fruit: ensure AFP_AfpInfo blobs are
0-initialized
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13181
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source4/torture/vfs/fruit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source4/torture/vfs/fruit.c b/source4/torture/vfs/fruit.c
index ace561d0d9c..63c4e18c3c7 100644
--- a/source4/torture/vfs/fruit.c
+++ b/source4/torture/vfs/fruit.c
@@ -909,7 +909,7 @@ static char *torture_afpinfo_pack(TALLOC_CTX *mem_ctx,
{
char *buf;
- buf = talloc_array(mem_ctx, char, AFP_INFO_SIZE);
+ buf = talloc_zero_array(mem_ctx, char, AFP_INFO_SIZE);
if (buf == NULL) {
return NULL;
}
--
2.13.6
From 5054d6877b1aee810332805b8f0a358ec92bc0f6 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Thu, 7 Dec 2017 13:43:02 +0100
Subject: [PATCH 2/5] s4/torture/fruit: enhance zero AFP_AfpInfo stream test
This test more operations in the zeroed out FinderInfo test, ensuring
after zeroing out FinderInfo, operations on the filehandle still work
and that enumerating streams doesn't return the stream anymore.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13181
Signed-off-by: Ralph Boehme <slow at samba.org>
---
selftest/knownfail.d/samba3.vfs.fruit | 2 +
source4/torture/vfs/fruit.c | 87 +++++++++++++++++++++++++++++++++--
2 files changed, 86 insertions(+), 3 deletions(-)
create mode 100644 selftest/knownfail.d/samba3.vfs.fruit
diff --git a/selftest/knownfail.d/samba3.vfs.fruit b/selftest/knownfail.d/samba3.vfs.fruit
new file mode 100644
index 00000000000..1ba64febb20
--- /dev/null
+++ b/selftest/knownfail.d/samba3.vfs.fruit
@@ -0,0 +1,2 @@
+^samba3.vfs.fruit metadata_stream.delete AFP_AfpInfo by writing all 0\(nt4_dc\)
+^samba3.vfs.fruit streams_depot.delete AFP_AfpInfo by writing all 0\(nt4_dc\)
diff --git a/source4/torture/vfs/fruit.c b/source4/torture/vfs/fruit.c
index 63c4e18c3c7..3b36371d2e7 100644
--- a/source4/torture/vfs/fruit.c
+++ b/source4/torture/vfs/fruit.c
@@ -3388,11 +3388,17 @@ static bool test_afpinfo_all0(struct torture_context *tctx,
{
bool ret = true;
NTSTATUS status;
- struct smb2_handle h1;
+ struct smb2_create create;
+ struct smb2_handle h1 = {{0}};
+ struct smb2_handle baseh = {{0}};
+ union smb_setfileinfo setfinfo;
+ union smb_fileinfo getfinfo;
TALLOC_CTX *mem_ctx = talloc_new(tctx);
const char *fname = BASEDIR "\\file";
+ const char *sname = BASEDIR "\\file" AFPINFO_STREAM;
const char *type_creator = "SMB,OLE!";
AfpInfo *info = NULL;
+ char *infobuf = NULL;
const char *streams_basic[] = {
"::$DATA"
};
@@ -3423,13 +3429,88 @@ static bool test_afpinfo_all0(struct torture_context *tctx,
/* Write all 0 to AFP_AfpInfo */
memset(info->afpi_FinderInfo, 0, AFP_FinderSize);
- ret = torture_write_afpinfo(tree, tctx, mem_ctx, fname, info);
- torture_assert_goto(tctx, ret == true, ret, done, "torture_write_afpinfo failed");
+ infobuf = torture_afpinfo_pack(mem_ctx, info);
+ torture_assert_not_null_goto(tctx, infobuf, ret, done,
+ "torture_afpinfo_pack failed\n");
+
+ ZERO_STRUCT(create);
+ create.in.desired_access = SEC_FILE_ALL;
+ create.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
+ create.in.create_disposition = NTCREATEX_DISP_OPEN;
+ create.in.share_access = NTCREATEX_SHARE_ACCESS_MASK;
+ create.in.fname = fname;
+
+ status = smb2_create(tree, mem_ctx, &create);
+ torture_assert_goto(tctx, ret == true, ret, done,
+ "smb2_create failed\n");
+ baseh = create.out.file.handle;
+
+ ZERO_STRUCT(create);
+ create.in.desired_access = SEC_FILE_ALL;
+ create.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
+ create.in.create_disposition = NTCREATEX_DISP_OVERWRITE_IF;
+ create.in.fname = sname;
+
+ status = smb2_create(tree, mem_ctx, &create);
+ torture_assert_goto(tctx, ret == true, ret, done,
+ "smb2_create failed\n");
+ h1 = create.out.file.handle;
+
+ status = smb2_util_write(tree, h1, infobuf, 0, AFP_INFO_SIZE);
+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+ "smb2_util_write failed\n");
+
+ /*
+ * Get stream information on open handle, must return only default
+ * stream, the AFP_AfpInfo stream must not be returned.
+ */
+
+ ZERO_STRUCT(getfinfo);
+ getfinfo.generic.level = RAW_FILEINFO_STREAM_INFORMATION;
+ getfinfo.generic.in.file.handle = baseh;
+
+ status = smb2_getinfo_file(tree, tctx, &getfinfo);
+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+ "get stream info\n");
+
+ torture_assert_int_equal_goto(tctx, getfinfo.stream_info.out.num_streams,
+ 1, ret, done, "stream count");
+
+ smb2_util_close(tree, baseh);
+ ZERO_STRUCT(baseh);
+
+ /*
+ * Try to set some file-basic-info (time) on the stream. This catches
+ * naive implementation mistakes that simply deleted the backing store
+ * from the filesystem in the zero-out step.
+ */
+
+ ZERO_STRUCT(setfinfo);
+ unix_to_nt_time(&setfinfo.basic_info.in.write_time, time(NULL));
+ setfinfo.basic_info.in.attrib = 0x20;
+ setfinfo.generic.level = RAW_SFILEINFO_BASIC_INFORMATION;
+ setfinfo.generic.in.file.handle = h1;
+
+ status = smb2_setinfo_file(tree, &setfinfo);
+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+ "smb2_getinfo_file failed\n");
+
+ ret = check_stream_list(tree, tctx, fname, 1, streams_basic, false);
+ torture_assert_goto(tctx, ret == true, ret, done, "check_stream_list");
+
+ smb2_util_close(tree, h1);
+ ZERO_STRUCT(h1);
ret = check_stream_list(tree, tctx, fname, 1, streams_basic, false);
torture_assert_goto(tctx, ret == true, ret, done, "Bad streams");
done:
+ if (!smb2_util_handle_empty(h1)) {
+ smb2_util_close(tree, h1);
+ }
+ if (!smb2_util_handle_empty(baseh)) {
+ smb2_util_close(tree, baseh);
+ }
smb2_util_unlink(tree, fname);
smb2_util_rmdir(tree, BASEDIR);
return ret;
--
2.13.6
From ed9cb213824d31d20a3355c6ea7348748f9f5d5a Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Thu, 7 Dec 2017 14:56:36 +0100
Subject: [PATCH 3/5] vfs_fruit: factor out delete_invalid_meta_stream() from
fruit_streaminfo_meta_stream()
No change in behaviour, just some refactoring before adding more code to
fruit_streaminfo_meta_stream() in the next commit.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13181
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/modules/vfs_fruit.c | 57 +++++++++++++++++++++++++++------------------
1 file changed, 34 insertions(+), 23 deletions(-)
diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
index 15e42fb4dfd..3e786295f23 100644
--- a/source3/modules/vfs_fruit.c
+++ b/source3/modules/vfs_fruit.c
@@ -4821,39 +4821,17 @@ static int fruit_fstat(vfs_handle_struct *handle, files_struct *fsp,
return rc;
}
-static NTSTATUS fruit_streaminfo_meta_stream(
+static NTSTATUS delete_invalid_meta_stream(
vfs_handle_struct *handle,
- struct files_struct *fsp,
const struct smb_filename *smb_fname,
TALLOC_CTX *mem_ctx,
unsigned int *pnum_streams,
struct stream_struct **pstreams)
{
- struct stream_struct *stream = *pstreams;
- unsigned int num_streams = *pnum_streams;
struct smb_filename *sname = NULL;
- int i;
int ret;
bool ok;
- for (i = 0; i < num_streams; i++) {
- if (strequal_m(stream[i].name, AFPINFO_STREAM)) {
- break;
- }
- }
-
- if (i == num_streams) {
- return NT_STATUS_OK;
- }
-
- if (stream[i].size == AFP_INFO_SIZE) {
- return NT_STATUS_OK;
- }
-
- DBG_ERR("Removing invalid AFPINFO_STREAM size [%"PRIdMAX"] "
- "from [%s]\n", (intmax_t)stream[i].size,
- smb_fname_str_dbg(smb_fname));
-
ok = del_fruit_stream(mem_ctx, pnum_streams, pstreams, AFPINFO_STREAM);
if (!ok) {
return NT_STATUS_INTERNAL_ERROR;
@@ -4877,6 +4855,39 @@ static NTSTATUS fruit_streaminfo_meta_stream(
return NT_STATUS_OK;
}
+static NTSTATUS fruit_streaminfo_meta_stream(
+ vfs_handle_struct *handle,
+ struct files_struct *fsp,
+ const struct smb_filename *smb_fname,
+ TALLOC_CTX *mem_ctx,
+ unsigned int *pnum_streams,
+ struct stream_struct **pstreams)
+{
+ struct stream_struct *stream = *pstreams;
+ unsigned int num_streams = *pnum_streams;
+ int i;
+
+ for (i = 0; i < num_streams; i++) {
+ if (strequal_m(stream[i].name, AFPINFO_STREAM)) {
+ break;
+ }
+ }
+
+ if (i == num_streams) {
+ return NT_STATUS_OK;
+ }
+
+ if (stream[i].size != AFP_INFO_SIZE) {
+ DBG_ERR("Removing invalid AFPINFO_STREAM size [%jd] from [%s]\n",
+ (intmax_t)stream[i].size, smb_fname_str_dbg(smb_fname));
+
+ return delete_invalid_meta_stream(handle, smb_fname, mem_ctx,
+ pnum_streams, pstreams);
+ }
+
+ return NT_STATUS_OK;
+}
+
static NTSTATUS fruit_streaminfo_meta_netatalk(
vfs_handle_struct *handle,
struct files_struct *fsp,
--
2.13.6
From b6e6243442d8f50b2d1024150722c4d6d0456fb8 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Thu, 7 Dec 2017 17:32:35 +0100
Subject: [PATCH 4/5] vfs_fruit: filter out AFP_AfpInfo streams with pending
delete-on-close
This is in preperation of fixing the implementation of removing the
AFP_AfpInfo stream by zeroing the FinderInfo out.
We currently remove the stream blob from the underyling filesystem
backing store, but that results in certain operations to fail on any
still open file-handle.
The fix comes in the next commit which will convert to backing store
delete operation to a set delete-on-close on the stream.
This commit adds filtering on streams that have the delete-on-close
set. It is only needed for the fruit:metadata=stream case, as with
fruit:metadata=netatalk the filtering is already done in
fruit_streaminfo_meta_netatalk().
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13181
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/modules/vfs_fruit.c | 72 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 71 insertions(+), 1 deletion(-)
diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
index 3e786295f23..182a8d5734d 100644
--- a/source3/modules/vfs_fruit.c
+++ b/source3/modules/vfs_fruit.c
@@ -4865,7 +4865,16 @@ static NTSTATUS fruit_streaminfo_meta_stream(
{
struct stream_struct *stream = *pstreams;
unsigned int num_streams = *pnum_streams;
+ struct smb_filename *sname = NULL;
+ char *full_name = NULL;
+ uint32_t name_hash;
+ struct share_mode_lock *lck = NULL;
+ struct file_id id = {0};
+ bool delete_on_close_set;
int i;
+ int ret;
+ NTSTATUS status;
+ bool ok;
for (i = 0; i < num_streams; i++) {
if (strequal_m(stream[i].name, AFPINFO_STREAM)) {
@@ -4885,7 +4894,68 @@ static NTSTATUS fruit_streaminfo_meta_stream(
pnum_streams, pstreams);
}
- return NT_STATUS_OK;
+ /*
+ * Now check if there's a delete-on-close pending on the stream. If so,
+ * hide the stream. This behaviour was verified against a macOS 10.12
+ * SMB server.
+ */
+
+ sname = synthetic_smb_fname(talloc_tos(),
+ smb_fname->base_name,
+ AFPINFO_STREAM_NAME,
+ NULL, 0);
+ if (sname == NULL) {
+ status = NT_STATUS_NO_MEMORY;
+ goto out;
+ }
+
+ ret = SMB_VFS_NEXT_STAT(handle, sname);
+ if (ret != 0) {
+ status = map_nt_error_from_unix(errno);
+ goto out;
+ }
+
+ id = SMB_VFS_NEXT_FILE_ID_CREATE(handle, &sname->st);
+
+ lck = get_existing_share_mode_lock(talloc_tos(), id);
+ if (lck == NULL) {
+ status = NT_STATUS_OK;
+ goto out;
+ }
+
+ full_name = talloc_asprintf(talloc_tos(),
+ "%s%s",
+ sname->base_name,
+ AFPINFO_STREAM);
+ if (full_name == NULL) {
+ status = NT_STATUS_NO_MEMORY;
+ goto out;
+ }
+
+ status = file_name_hash(handle->conn, full_name, &name_hash);
+ if (!NT_STATUS_IS_OK(status)) {
+ goto out;
+ }
+
+ delete_on_close_set = is_delete_on_close_set(lck, name_hash);
+ if (delete_on_close_set) {
+ ok = del_fruit_stream(mem_ctx,
+ pnum_streams,
+ pstreams,
+ AFPINFO_STREAM);
+ if (!ok) {
+ status = NT_STATUS_INTERNAL_ERROR;
+ goto out;
+ }
+ }
+
+ status = NT_STATUS_OK;
+
+out:
+ TALLOC_FREE(sname);
+ TALLOC_FREE(lck);
+ TALLOC_FREE(full_name);
+ return status;
}
static NTSTATUS fruit_streaminfo_meta_netatalk(
--
2.13.6
From a937468831a0f287028fd3832547fc39cc2b2c60 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 6 Dec 2017 22:09:52 +0100
Subject: [PATCH 5/5] vfs_fruit: set delete-on-close for empty finderinfo
We previously removed the stream from the underlying filesystem stream
backing store when the client zeroes out FinderInfo in the AFP_AfpInfo
stream, but this causes certain operations to fail (eg stat) when trying
to access the stream over any file-handle open on that stream.
So instead of deleting, set delete-on-close on the stream. The previous
commit already implemented not to list list streams with delete-on-close
set which is necessary to implemenent correct macOS semantics for this
particular stream.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13181
Signed-off-by: Ralph Boehme <slow at samba.org>
---
selftest/knownfail.d/samba3.vfs.fruit | 2 --
source3/modules/vfs_fruit.c | 60 +++++++++++++++++++++--------------
2 files changed, 36 insertions(+), 26 deletions(-)
delete mode 100644 selftest/knownfail.d/samba3.vfs.fruit
diff --git a/selftest/knownfail.d/samba3.vfs.fruit b/selftest/knownfail.d/samba3.vfs.fruit
deleted file mode 100644
index 1ba64febb20..00000000000
--- a/selftest/knownfail.d/samba3.vfs.fruit
+++ /dev/null
@@ -1,2 +0,0 @@
-^samba3.vfs.fruit metadata_stream.delete AFP_AfpInfo by writing all 0\(nt4_dc\)
-^samba3.vfs.fruit streams_depot.delete AFP_AfpInfo by writing all 0\(nt4_dc\)
diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
index 182a8d5734d..6745e954c08 100644
--- a/source3/modules/vfs_fruit.c
+++ b/source3/modules/vfs_fruit.c
@@ -4091,26 +4091,35 @@ static ssize_t fruit_pwrite_meta_stream(vfs_handle_struct *handle,
size_t n, off_t offset)
{
AfpInfo *ai = NULL;
- int ret;
+ size_t nwritten;
+ bool ok;
ai = afpinfo_unpack(talloc_tos(), data);
if (ai == NULL) {
return -1;
}
- if (ai_empty_finderinfo(ai)) {
- ret = SMB_VFS_NEXT_UNLINK(handle, fsp->fsp_name);
- if (ret != 0 && errno != ENOENT && errno != ENOATTR) {
- DBG_ERR("Can't delete metadata for %s: %s\n",
- fsp_str_dbg(fsp), strerror(errno));
- TALLOC_FREE(ai);
- return -1;
- }
+ nwritten = SMB_VFS_NEXT_PWRITE(handle, fsp, data, n, offset);
+ if (nwritten != n) {
+ return -1;
+ }
+ if (!ai_empty_finderinfo(ai)) {
return n;
}
- return SMB_VFS_NEXT_PWRITE(handle, fsp, data, n, offset);
+ ok = set_delete_on_close(
+ fsp,
+ true,
+ handle->conn->session_info->security_token,
+ handle->conn->session_info->unix_token);
+ if (!ok) {
+ DBG_ERR("set_delete_on_close on [%s] failed\n",
+ fsp_str_dbg(fsp));
+ return -1;
+ }
+
+ return n;
}
static ssize_t fruit_pwrite_meta_netatalk(vfs_handle_struct *handle,
@@ -4121,26 +4130,13 @@ static ssize_t fruit_pwrite_meta_netatalk(vfs_handle_struct *handle,
AfpInfo *ai = NULL;
char *p = NULL;
int ret;
+ bool ok;
ai = afpinfo_unpack(talloc_tos(), data);
if (ai == NULL) {
return -1;
}
- if (ai_empty_finderinfo(ai)) {
- ret = SMB_VFS_REMOVEXATTR(handle->conn,
- fsp->fsp_name,
- AFPINFO_EA_NETATALK);
-
- if (ret != 0 && errno != ENOENT && errno != ENOATTR) {
- DBG_ERR("Can't delete metadata for %s: %s\n",
- fsp_str_dbg(fsp), strerror(errno));
- return -1;
- }
-
- return n;
- }
-
ad = ad_fget(talloc_tos(), handle, fsp, ADOUBLE_META);
if (ad == NULL) {
ad = ad_init(talloc_tos(), handle, ADOUBLE_META);
@@ -4165,6 +4161,22 @@ static ssize_t fruit_pwrite_meta_netatalk(vfs_handle_struct *handle,
}
TALLOC_FREE(ad);
+
+ if (!ai_empty_finderinfo(ai)) {
+ return n;
+ }
+
+ ok = set_delete_on_close(
+ fsp,
+ true,
+ handle->conn->session_info->security_token,
+ handle->conn->session_info->unix_token);
+ if (!ok) {
+ DBG_ERR("set_delete_on_close on [%s] failed\n",
+ fsp_str_dbg(fsp));
+ return -1;
+ }
+
return n;
}
--
2.13.6
More information about the samba-technical
mailing list