[PATCH] Two dependent bugfixes for VSS bugs 13455 and 13688

Ralph Böhme slow at samba.org
Fri Nov 23 19:51:38 UTC 2018 

Hi!

Here's a combined patchset to fix two bugs in VSS and shadow_copy2.

https://bugzilla.samba.org/show_bug.cgi?id=13455
https://bugzilla.samba.org/show_bug.cgi?id=13688

Both are real world problems: a user reported he couldn't restore a previous 
version from a Windows 2016 client from a Samba server with shadow_copy2.

13455 comes second in the patchset and builds upon test infrastructure I'm 
adding for the other one. To avoid confusion I'm providing them both in one 
patchset.

13688 deals with permissions on snapshot files, there's an ongoing inquiry with 
dochelp on expected behaviour:

https://lists.samba.org/archive/cifs-protocol/2018-November/003185.html

I don't think the outcome will affect the basic fix I'm applying here, it may 
just turn out that there are more corner cases that require additional fixes.

The behaviour was verified against a Windows 2016 SMB server.

CI: https://gitlab.com/samba-team/devel/samba/pipelines/37690017

Please review & push if happy. Thanks!

-slow

-- 
Ralph Boehme, Samba Team                https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/
GPG-Fingerprint   FAE2C6088A24252051C559E4AA1E9B7126399E46
-------------- next part --------------
From e92e99d971c6b80573d0d10122bde7d06cce8b2d Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 23 Nov 2018 10:07:29 +0100
Subject: [PATCH 01/10] vfs_error_inject: add pwrite

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13688

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/modules/vfs_error_inject.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/source3/modules/vfs_error_inject.c b/source3/modules/vfs_error_inject.c
index bb5477a449f..9f0a25fb73f 100644
--- a/source3/modules/vfs_error_inject.c
+++ b/source3/modules/vfs_error_inject.c
@@ -88,8 +88,26 @@ static int vfs_error_inject_chdir(vfs_handle_struct *handle,
 	return SMB_VFS_NEXT_CHDIR(handle, smb_fname);
 }
 
+static ssize_t vfs_error_inject_pwrite(vfs_handle_struct *handle,
+				       files_struct *fsp,
+				       const void *data,
+				       size_t n,
+				       off_t offset)
+{
+	int error;
+
+	error = inject_unix_error("pwrite", handle);
+	if (error != 0) {
+		errno = error;
+		return -1;
+	}
+
+	return SMB_VFS_NEXT_PWRITE(handle, fsp, data, n, offset);
+}
+
 static struct vfs_fn_pointers vfs_error_inject_fns = {
 	.chdir_fn = vfs_error_inject_chdir,
+	.pwrite_fn = vfs_error_inject_pwrite,
 };
 
 static_decl_vfs;
-- 
2.17.2


From ba78a440c9841571254086bb561d63bd485ae2fc Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 23 Nov 2018 10:18:10 +0100
Subject: [PATCH 02/10] vfs_error_inject: add EBADF error

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13688

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/modules/vfs_error_inject.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/source3/modules/vfs_error_inject.c b/source3/modules/vfs_error_inject.c
index 9f0a25fb73f..c8c3ea4701f 100644
--- a/source3/modules/vfs_error_inject.c
+++ b/source3/modules/vfs_error_inject.c
@@ -28,6 +28,7 @@ struct unix_error_map {
 	int error;
 } unix_error_map_array[] = {
 	{	"ESTALE",	ESTALE	},
+	{	"EBADF",	EBADF	},
 };
 
 static int find_unix_error_from_string(const char *err_str)
-- 
2.17.2


From 33b9a029860c94d791cf5b500109c795e9d5e2ec Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 14 Nov 2018 13:45:11 +0100
Subject: [PATCH 03/10] s4:torture: add a test-suite for VSS

This test will not be run from the main torture test runner in selftest,
as there we don't pass the required arguments 'twrp_file' and
'twrp_snapshot'.

The test needs a carefully prepared environment with provisioned
snapshot data, so the test will be started from a blackbox test
script. That comes next.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13688

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source4/torture/smb2/create.c | 87 +++++++++++++++++++++++++++++++++++
 source4/torture/smb2/smb2.c   |  1 +
 2 files changed, 88 insertions(+)

diff --git a/source4/torture/smb2/create.c b/source4/torture/smb2/create.c
index ead56eb5c40..68447935bd0 100644
--- a/source4/torture/smb2/create.c
+++ b/source4/torture/smb2/create.c
@@ -1733,6 +1733,82 @@ static bool test_dir_alloc_size(struct torture_context *tctx,
 	return ret;
 }
 
+static bool test_twrp_write(struct torture_context *tctx, struct smb2_tree *tree)
+{
+	struct smb2_create io;
+	struct smb2_handle h1 = {{0}};
+	NTSTATUS status;
+	bool ret = true;
+	char *p = NULL;
+	struct tm tm;
+	time_t t;
+	uint64_t nttime;
+	const char *file = NULL;
+	const char *snapshot = NULL;
+
+	file = torture_setting_string(tctx, "twrp_file", NULL);
+	if (file == NULL) {
+		torture_skip(tctx, "missing 'twrp_file' option\n");
+	}
+
+	snapshot = torture_setting_string(tctx, "twrp_snapshot", NULL);
+	if (snapshot == NULL) {
+		torture_skip(tctx, "missing 'twrp_snapshot' option\n");
+	}
+
+	torture_comment(tctx, "Testing timewarp (%s) (%s)\n", file, snapshot);
+
+	setenv("TZ", "GMT", 1);
+	p = strptime(snapshot, "@GMT-%Y.%m.%d-%H.%M.%S", &tm);
+	torture_assert_goto(tctx, p != NULL, ret, done, "strptime\n");
+	torture_assert_goto(tctx, *p == '\0', ret, done, "strptime\n");
+
+	t = mktime(&tm);
+	unix_to_nt_time(&nttime, t);
+
+	io = (struct smb2_create) {
+		.in.desired_access = SEC_FILE_READ_DATA,
+		.in.file_attributes = FILE_ATTRIBUTE_NORMAL,
+		.in.create_disposition = NTCREATEX_DISP_OPEN,
+		.in.share_access = NTCREATEX_SHARE_ACCESS_MASK,
+		.in.fname = file,
+		.in.query_maximal_access = true,
+		.in.timewarp = nttime,
+	};
+
+	status = smb2_create(tree, tctx, &io);
+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+					"smb2_create\n");
+	smb2_util_close(tree, io.out.file.handle);
+
+	ret = io.out.maximal_access & (SEC_FILE_READ_DATA | SEC_FILE_WRITE_DATA);
+	torture_assert_goto(tctx, ret, ret, done, "Bad access\n");
+
+	io = (struct smb2_create) {
+		.in.desired_access = SEC_FILE_READ_DATA|SEC_FILE_WRITE_DATA,
+		.in.file_attributes = FILE_ATTRIBUTE_NORMAL,
+		.in.create_disposition = NTCREATEX_DISP_OPEN,
+		.in.share_access = NTCREATEX_SHARE_ACCESS_MASK,
+		.in.fname = file,
+		.in.timewarp = nttime,
+	};
+
+	status = smb2_create(tree, tctx, &io);
+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+					"smb2_create\n");
+	h1 = io.out.file.handle;
+
+	status = smb2_util_write(tree, h1, "123", 0, 3);
+	torture_assert_ntstatus_equal_goto(tctx, status,
+					   NT_STATUS_MEDIA_WRITE_PROTECTED,
+					   ret, done, "smb2_create\n");
+
+	smb2_util_close(tree, h1);
+
+done:
+	return ret;
+}
+
 /*
    basic testing of SMB2 read
 */
@@ -1758,3 +1834,14 @@ struct torture_suite *torture_smb2_create_init(TALLOC_CTX *ctx)
 
 	return suite;
 }
+
+struct torture_suite *torture_smb2_twrp_init(TALLOC_CTX *ctx)
+{
+	struct torture_suite *suite = torture_suite_create(ctx, "twrp");
+
+	torture_suite_add_1smb2_test(suite, "write", test_twrp_write);
+
+	suite->description = talloc_strdup(suite, "SMB2-TWRP tests");
+
+	return suite;
+}
diff --git a/source4/torture/smb2/smb2.c b/source4/torture/smb2/smb2.c
index 6f9884e3c27..a835dc7c050 100644
--- a/source4/torture/smb2/smb2.c
+++ b/source4/torture/smb2/smb2.c
@@ -154,6 +154,7 @@ NTSTATUS torture_smb2_init(TALLOC_CTX *ctx)
 	torture_suite_add_suite(suite, torture_smb2_read_init(suite));
 	torture_suite_add_suite(suite, torture_smb2_aio_delay_init(suite));
 	torture_suite_add_suite(suite, torture_smb2_create_init(suite));
+	torture_suite_add_suite(suite, torture_smb2_twrp_init(suite));
 	torture_suite_add_suite(suite, torture_smb2_acls_init(suite));
 	torture_suite_add_suite(suite, torture_smb2_notify_init(suite));
 	torture_suite_add_suite(suite, torture_smb2_notify_inotify_init(suite));
-- 
2.17.2


From 683abec1f938b0ef24e66d0307803c21bdf7abf3 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 23 Nov 2018 10:18:44 +0100
Subject: [PATCH 04/10] s3:script/tests: add a test for VSS write behaviour

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13688

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 selftest/knownfail.d/samba3.blackbox          |  1 +
 selftest/target/Samba3.pm                     |  9 +++
 .../script/tests/test_shadow_copy_torture.sh  | 76 +++++++++++++++++++
 source3/selftest/tests.py                     |  3 +-
 4 files changed, 88 insertions(+), 1 deletion(-)
 create mode 100644 selftest/knownfail.d/samba3.blackbox
 create mode 100755 source3/script/tests/test_shadow_copy_torture.sh

diff --git a/selftest/knownfail.d/samba3.blackbox b/selftest/knownfail.d/samba3.blackbox
new file mode 100644
index 00000000000..16537e58aeb
--- /dev/null
+++ b/selftest/knownfail.d/samba3.blackbox
@@ -0,0 +1 @@
+^samba3.blackbox.shadow_copy_torture.writing to shadow copy of a file\(fileserver\)
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index 363840e4521..0e1ffd1101a 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -2190,6 +2190,15 @@ sub provision($$$$$$$$$)
 	vfs objects = shadow_copy2
 	shadow:mountpoint = $shadow_mntdir
 	wide links = yes
+
+[shadow_write]
+	path = $shadow_tstdir
+	comment = previous versions snapshots under mount point
+	vfs objects = shadow_copy2 error_inject
+	aio write size = 0
+	error_inject:pwrite = EBADF
+	shadow:mountpoint = $shadow_tstdir
+
 [dfq]
 	path = $shrdir/dfree
 	vfs objects = acl_xattr fake_acls xattr_tdb fake_dfq
diff --git a/source3/script/tests/test_shadow_copy_torture.sh b/source3/script/tests/test_shadow_copy_torture.sh
new file mode 100755
index 00000000000..d47cd512a20
--- /dev/null
+++ b/source3/script/tests/test_shadow_copy_torture.sh
@@ -0,0 +1,76 @@
+#!/bin/bash
+#
+# Blackbox test for shadow_copy2 VFS.
+#
+
+if [ $# -lt 7 ]; then
+cat <<EOF
+Usage: test_shadow_copy SERVER SERVER_IP DOMAIN USERNAME PASSWORD WORKDIR SMBTORTURE
+EOF
+exit 1;
+fi
+
+SERVER=${1}
+SERVER_IP=${2}
+DOMAIN=${3}
+USERNAME=${4}
+PASSWORD=${5}
+WORKDIR=${6}
+SMBTORTURE="$VALGRIND ${7}"
+shift 7
+
+incdir=`dirname $0`/../../../testprogs/blackbox
+. $incdir/subunit.sh
+
+SNAPSHOT="@GMT-2015.10.31-19.40.30"
+
+failed=0
+
+# build a hierarchy of files, symlinks, and directories
+build_files()
+{
+    local destdir
+    destdir=$1
+
+    echo "$content" > $destdir/foo
+}
+
+# build a snapshots directory
+build_snapshots()
+{
+    local snapdir
+
+    snapdir=$WORKDIR/.snapshots
+
+    mkdir -p $snapdir
+    mkdir $snapdir/$SNAPSHOT
+
+    build_files $snapdir/$SNAPSHOT
+}
+
+test_shadow_copy_write()
+{
+    local msg
+
+    msg=$1
+
+    #delete snapshots from previous tests
+    find $WORKDIR -name ".snapshots" -exec rm -rf {} \; 1>/dev/null 2>&1
+    build_snapshots
+
+    testit "writing to shadow copy of a file" \
+	   $SMBTORTURE \
+	   -U$USERNAME%$PASSWORD \
+	   "//$SERVER/shadow_write" \
+	   --option="torture:twrp_file=foo" \
+	   --option="torture:twrp_snapshot=$SNAPSHOT" \
+	   smb2.twrp.write || \
+        failed=`expr $failed + 1`
+}
+
+build_files $WORKDIR
+
+# test open for writing and write behaviour of snapshoted files
+test_shadow_copy_write "write behaviour of snapshoted files"
+
+exit $failed
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index f3c5c39664b..bfe875e19fa 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -24,7 +24,7 @@ sys.path.insert(0, os.path.normpath(os.path.join(os.path.dirname(__file__), "../
 import selftesthelpers
 from selftesthelpers import bindir, srcdir, scriptdir, binpath
 from selftesthelpers import plantestsuite, samba3srcdir
-from selftesthelpers import smbtorture3, configuration, smbclient3
+from selftesthelpers import smbtorture3, configuration, smbclient3, smbtorture4
 from selftesthelpers import net, wbinfo, dbwrap_tool, rpcclient, python
 from selftesthelpers import smbget, smbcacls, smbcquotas, ntlm_auth3
 from selftesthelpers import valgrindify, smbtorture4_testsuites
@@ -310,6 +310,7 @@ plantestsuite("samba3.blackbox.smbclient_ntlm.plain (%s)" % env, env, [os.path.j
     plantestsuite("samba3.blackbox.offline (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_offline.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/offline', smbclient3])
     plantestsuite("samba3.blackbox.shadow_copy2 NT1 (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_shadow_copy.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/shadow', smbclient3, '-m', 'NT1'])
     plantestsuite("samba3.blackbox.shadow_copy2 SMB3 (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_shadow_copy.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/shadow', smbclient3, '-m', 'SMB3'])
+    plantestsuite("samba3.blackbox.shadow_copy_torture", env, [os.path.join(samba3srcdir, "script/tests/test_shadow_copy_torture.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/shadow', smbtorture4])
     plantestsuite("samba3.blackbox.smbclient.forceuser_validusers (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_forceuser_validusers.sh"), '$SERVER', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH', smbclient3])
     plantestsuite("samba3.blackbox.smbget (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_smbget.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', 'smbget_user', '$PASSWORD', '$LOCAL_PATH/smbget', smbget])
     plantestsuite("samba3.blackbox.netshareenum (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_shareenum.sh"), '$SERVER', '$USERNAME', '$PASSWORD', rpcclient])
-- 
2.17.2


From 6a3bc70c7fd272d8dc5ea2ce84484572bc0de21b Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Thu, 22 Nov 2018 11:02:24 +0100
Subject: [PATCH 05/10] vfs_shadow_copy2: add _already_converted arg to
 shadow_copy2_strip_snapshot_internal()

Not used for now, all existing callers pass NULL.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13688

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/modules/vfs_shadow_copy2.c | 32 ++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
index 79c1ee5cf33..9d49ce77064 100644
--- a/source3/modules/vfs_shadow_copy2.c
+++ b/source3/modules/vfs_shadow_copy2.c
@@ -587,7 +587,8 @@ static bool shadow_copy2_strip_snapshot_internal(TALLOC_CTX *mem_ctx,
 					const char *orig_name,
 					time_t *ptimestamp,
 					char **pstripped,
-					char **psnappath)
+					char **psnappath,
+					bool *_already_converted)
 {
 	struct tm tm;
 	time_t timestamp = 0;
@@ -608,6 +609,10 @@ static bool shadow_copy2_strip_snapshot_internal(TALLOC_CTX *mem_ctx,
 
 	DEBUG(10, (__location__ ": enter path '%s'\n", name));
 
+	if (_already_converted != NULL) {
+		*_already_converted = false;
+	}
+
 	abs_path = make_path_absolute(mem_ctx, priv, name);
 	if (abs_path == NULL) {
 		ret = false;
@@ -630,6 +635,9 @@ static bool shadow_copy2_strip_snapshot_internal(TALLOC_CTX *mem_ctx,
 	}
 
 	if (already_converted) {
+		if (_already_converted != NULL) {
+			*_already_converted = true;
+		}
 		goto out;
 	}
 
@@ -759,6 +767,7 @@ static bool shadow_copy2_strip_snapshot(TALLOC_CTX *mem_ctx,
 					orig_name,
 					ptimestamp,
 					pstripped,
+					NULL,
 					NULL);
 }
 
@@ -1119,12 +1128,14 @@ static int shadow_copy2_rename(vfs_handle_struct *handle,
 
 	if (!shadow_copy2_strip_snapshot_internal(talloc_tos(), handle,
 					 smb_fname_src->base_name,
-					 &timestamp_src, NULL, &snappath_src)) {
+					 &timestamp_src, NULL, &snappath_src,
+					 NULL)) {
 		return -1;
 	}
 	if (!shadow_copy2_strip_snapshot_internal(talloc_tos(), handle,
 					 smb_fname_dst->base_name,
-					 &timestamp_dst, NULL, &snappath_dst)) {
+					 &timestamp_dst, NULL, &snappath_dst,
+					 NULL)) {
 		return -1;
 	}
 	if (timestamp_src != 0) {
@@ -1163,7 +1174,8 @@ static int shadow_copy2_symlink(vfs_handle_struct *handle,
 				link_contents,
 				&timestamp_old,
 				NULL,
-				&snappath_old)) {
+				&snappath_old,
+				NULL)) {
 		return -1;
 	}
 	if (!shadow_copy2_strip_snapshot_internal(talloc_tos(),
@@ -1171,7 +1183,8 @@ static int shadow_copy2_symlink(vfs_handle_struct *handle,
 				new_smb_fname->base_name,
 				&timestamp_new,
 				NULL,
-				&snappath_new)) {
+				&snappath_new,
+				NULL)) {
 		return -1;
 	}
 	if ((timestamp_old != 0) || (timestamp_new != 0)) {
@@ -1202,7 +1215,8 @@ static int shadow_copy2_link(vfs_handle_struct *handle,
 				old_smb_fname->base_name,
 				&timestamp_old,
 				NULL,
-				&snappath_old)) {
+				&snappath_old,
+				NULL)) {
 		return -1;
 	}
 	if (!shadow_copy2_strip_snapshot_internal(talloc_tos(),
@@ -1210,7 +1224,8 @@ static int shadow_copy2_link(vfs_handle_struct *handle,
 				new_smb_fname->base_name,
 				&timestamp_new,
 				NULL,
-				&snappath_new)) {
+				&snappath_new,
+				NULL)) {
 		return -1;
 	}
 	if ((timestamp_old != 0) || (timestamp_new != 0)) {
@@ -1566,7 +1581,8 @@ static int shadow_copy2_chdir(vfs_handle_struct *handle,
 					smb_fname->base_name,
 					&timestamp,
 					&stripped,
-					&snappath)) {
+					&snappath,
+					NULL)) {
 		return -1;
 	}
 	if (stripped != NULL) {
-- 
2.17.2


From 748827a6cbb8d0270c91ce61deb0b14d91a4e449 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Thu, 22 Nov 2018 11:04:54 +0100
Subject: [PATCH 06/10] vfs_shadow_copy2: add
 shadow_copy2_strip_snapshot_converted

Can be used by callers to determine if a path is in fact pointing at a
file in a snapshot. Will be used in the next commit.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13688

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/modules/vfs_shadow_copy2.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
index 9d49ce77064..9d9a84ab4a1 100644
--- a/source3/modules/vfs_shadow_copy2.c
+++ b/source3/modules/vfs_shadow_copy2.c
@@ -771,6 +771,22 @@ static bool shadow_copy2_strip_snapshot(TALLOC_CTX *mem_ctx,
 					NULL);
 }
 
+static bool shadow_copy2_strip_snapshot_converted(TALLOC_CTX *mem_ctx,
+					struct vfs_handle_struct *handle,
+					const char *orig_name,
+					time_t *ptimestamp,
+					char **pstripped,
+					bool *is_converted)
+{
+	return shadow_copy2_strip_snapshot_internal(mem_ctx,
+					handle,
+					orig_name,
+					ptimestamp,
+					pstripped,
+					NULL,
+					is_converted);
+}
+
 static char *shadow_copy2_find_mount_point(TALLOC_CTX *mem_ctx,
 					   vfs_handle_struct *handle)
 {
-- 
2.17.2


From 4eff1c5b846e97ffe28b6b200626a395f3a227e2 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 23 Nov 2018 14:08:15 +0100
Subject: [PATCH 07/10] vfs_shadow_copy2: nicely deal with attempts to open
 previous version for writing

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13688

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 selftest/knownfail.d/samba3.blackbox |   1 -
 source3/modules/vfs_shadow_copy2.c   | 124 ++++++++++++++++++++++++++-
 2 files changed, 122 insertions(+), 3 deletions(-)
 delete mode 100644 selftest/knownfail.d/samba3.blackbox

diff --git a/selftest/knownfail.d/samba3.blackbox b/selftest/knownfail.d/samba3.blackbox
deleted file mode 100644
index 16537e58aeb..00000000000
--- a/selftest/knownfail.d/samba3.blackbox
+++ /dev/null
@@ -1 +0,0 @@
-^samba3.blackbox.shadow_copy_torture.writing to shadow copy of a file\(fileserver\)
diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
index 9d9a84ab4a1..e105a813e23 100644
--- a/source3/modules/vfs_shadow_copy2.c
+++ b/source3/modules/vfs_shadow_copy2.c
@@ -36,6 +36,8 @@
 #include "include/ntioctl.h"
 #include "util_tdb.h"
 #include "lib/util_path.h"
+#include "libcli/security/security.h"
+#include "lib/util/tevent_unix.h"
 
 struct shadow_copy2_config {
 	char *gmt_format;
@@ -1376,15 +1378,27 @@ static int shadow_copy2_open(vfs_handle_struct *handle,
 	time_t timestamp = 0;
 	char *stripped = NULL;
 	char *tmp;
+	bool is_converted = false;
 	int saved_errno = 0;
 	int ret;
 
-	if (!shadow_copy2_strip_snapshot(talloc_tos(), handle,
+	if (!shadow_copy2_strip_snapshot_converted(talloc_tos(), handle,
 					 smb_fname->base_name,
-					 &timestamp, &stripped)) {
+					 &timestamp, &stripped,
+					 &is_converted)) {
 		return -1;
 	}
 	if (timestamp == 0) {
+		if (is_converted) {
+			/*
+			 * Just pave over the user requested mode and use
+			 * O_RDONLY. Later attempts by the client to write on
+			 * the handle will fail in the pwrite() syscall with
+			 * EINVAL which we carefully map to EROFS. In sum, this
+			 * matches Windows behaviour.
+			 */
+			flags = O_RDONLY;
+		}
 		return SMB_VFS_NEXT_OPEN(handle, smb_fname, fsp, flags, mode);
 	}
 
@@ -1398,6 +1412,14 @@ static int shadow_copy2_open(vfs_handle_struct *handle,
 		return -1;
 	}
 
+	/*
+	 * Just pave over the user requested mode and use O_RDONLY. Later
+	 * attempts by the client to write on the handle will fail in the
+	 * pwrite() syscall with EINVAL which we carefully map to EROFS. In sum,
+	 * this matches Windows behaviour.
+	 */
+	flags = O_RDONLY;
+
 	ret = SMB_VFS_NEXT_OPEN(handle, smb_fname, fsp, flags, mode);
 	if (ret == -1) {
 		saved_errno = errno;
@@ -2887,6 +2909,101 @@ static int shadow_copy2_get_quota(vfs_handle_struct *handle,
 	return ret;
 }
 
+static ssize_t shadow_copy2_pwrite(vfs_handle_struct *handle,
+				   files_struct *fsp,
+				   const void *data,
+				   size_t n,
+				   off_t offset)
+{
+	ssize_t nwritten;
+
+	nwritten = SMB_VFS_NEXT_PWRITE(handle, fsp, data, n, offset);
+	if (nwritten == -1) {
+		if (errno == EBADF && fsp->can_write) {
+			errno = EROFS;
+		}
+	}
+
+	return nwritten;
+}
+
+struct shadow_copy2_pwrite_state {
+	vfs_handle_struct *handle;
+	files_struct *fsp;
+	ssize_t ret;
+	struct vfs_aio_state vfs_aio_state;
+};
+
+static void shadow_copy2_pwrite_done(struct tevent_req *subreq);
+
+static struct tevent_req *shadow_copy2_pwrite_send(
+	struct vfs_handle_struct *handle, TALLOC_CTX *mem_ctx,
+	struct tevent_context *ev, struct files_struct *fsp,
+	const void *data, size_t n, off_t offset)
+{
+	struct tevent_req *req = NULL, *subreq = NULL;
+	struct shadow_copy2_pwrite_state *state = NULL;
+
+	req = tevent_req_create(mem_ctx, &state,
+				struct shadow_copy2_pwrite_state);
+	if (req == NULL) {
+		return NULL;
+	}
+	state->handle = handle;
+	state->fsp = fsp;
+
+	subreq = SMB_VFS_NEXT_PWRITE_SEND(state,
+					  ev,
+					  handle,
+					  fsp,
+					  data,
+					  n,
+					  offset);
+	if (tevent_req_nomem(subreq, req)) {
+		return tevent_req_post(req, ev);
+	}
+	tevent_req_set_callback(subreq, shadow_copy2_pwrite_done, req);
+
+	return req;
+}
+
+static void shadow_copy2_pwrite_done(struct tevent_req *subreq)
+{
+	struct tevent_req *req = tevent_req_callback_data(
+		subreq, struct tevent_req);
+	struct shadow_copy2_pwrite_state *state = tevent_req_data(
+		req, struct shadow_copy2_pwrite_state);
+
+	state->ret = SMB_VFS_PWRITE_RECV(subreq, &state->vfs_aio_state);
+	TALLOC_FREE(subreq);
+	if (state->ret == -1) {
+		tevent_req_error(req, state->vfs_aio_state.error);
+		return;
+	}
+
+	tevent_req_done(req);
+}
+
+static ssize_t shadow_copy2_pwrite_recv(struct tevent_req *req,
+					  struct vfs_aio_state *vfs_aio_state)
+{
+	struct shadow_copy2_pwrite_state *state = tevent_req_data(
+		req, struct shadow_copy2_pwrite_state);
+
+	if (tevent_req_is_unix_error(req, &vfs_aio_state->error)) {
+		if ((vfs_aio_state->error == EBADF) &&
+		    state->fsp->can_write)
+		{
+			vfs_aio_state->error = EROFS;
+			errno = EROFS;
+		}
+		return -1;
+	}
+
+	*vfs_aio_state = state->vfs_aio_state;
+	return state->ret;
+}
+
 static int shadow_copy2_connect(struct vfs_handle_struct *handle,
 				const char *service, const char *user)
 {
@@ -3251,6 +3368,9 @@ static struct vfs_fn_pointers vfs_shadow_copy2_fns = {
 	.setxattr_fn = shadow_copy2_setxattr,
 	.chflags_fn = shadow_copy2_chflags,
 	.get_real_filename_fn = shadow_copy2_get_real_filename,
+	.pwrite_fn = shadow_copy2_pwrite,
+	.pwrite_send_fn = shadow_copy2_pwrite_send,
+	.pwrite_recv_fn = shadow_copy2_pwrite_recv,
 	.connectpath_fn = shadow_copy2_connectpath,
 };
 
-- 
2.17.2


From 3bfea63dbf83b6c84adfb48f9ce4ec32cb57b2be Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 23 Nov 2018 14:36:56 +0100
Subject: [PATCH 08/10] s3:selftest: add a VSS test reading a stream

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13455

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 selftest/knownfail.d/samba3.blackbox          |  1 +
 selftest/target/Samba3.pm                     |  2 +-
 .../script/tests/test_shadow_copy_torture.sh  | 38 ++++++++
 source4/torture/smb2/create.c                 | 87 +++++++++++++++++++
 4 files changed, 127 insertions(+), 1 deletion(-)
 create mode 100644 selftest/knownfail.d/samba3.blackbox

diff --git a/selftest/knownfail.d/samba3.blackbox b/selftest/knownfail.d/samba3.blackbox
new file mode 100644
index 00000000000..a15359e6420
--- /dev/null
+++ b/selftest/knownfail.d/samba3.blackbox
@@ -0,0 +1 @@
+^samba3.blackbox.shadow_copy_torture.reading stream of a shadow copy of a file\(fileserver\)
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index 0e1ffd1101a..888f3bd5154 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -2194,7 +2194,7 @@ sub provision($$$$$$$$$)
 [shadow_write]
 	path = $shadow_tstdir
 	comment = previous versions snapshots under mount point
-	vfs objects = shadow_copy2 error_inject
+	vfs objects = shadow_copy2 streams_xattr error_inject
 	aio write size = 0
 	error_inject:pwrite = EBADF
 	shadow:mountpoint = $shadow_tstdir
diff --git a/source3/script/tests/test_shadow_copy_torture.sh b/source3/script/tests/test_shadow_copy_torture.sh
index d47cd512a20..3b05fc50f72 100755
--- a/source3/script/tests/test_shadow_copy_torture.sh
+++ b/source3/script/tests/test_shadow_copy_torture.sh
@@ -48,6 +48,13 @@ build_snapshots()
     build_files $snapdir/$SNAPSHOT
 }
 
+build_stream_on_snapshot()
+{
+    file=$WORKDIR/.snapshots/$SNAPSHOT/foo
+
+    setfattr -n 'user.DosStream.bar:$DATA' -v baz $file || return 1
+}
+
 test_shadow_copy_write()
 {
     local msg
@@ -68,9 +75,40 @@ test_shadow_copy_write()
         failed=`expr $failed + 1`
 }
 
+test_shadow_copy_stream()
+{
+    local msg
+
+    msg=$1
+
+    #delete snapshots from previous tests
+    find $WORKDIR -name ".snapshots" -exec rm -rf {} \; 1>/dev/null 2>&1
+    build_snapshots
+    build_stream_on_snapshot || {
+	subunit_start_test msg
+	subunit_skip_test msg <<EOF
+test_shadow_copy_stream needs an fs with xattrs
+EOF
+	return 0
+    }
+
+    testit "reading stream of a shadow copy of a file" \
+	   $SMBTORTURE \
+	   -U$USERNAME%$PASSWORD \
+	   "//$SERVER/shadow_write" \
+	   --option="torture:twrp_file=foo" \
+	   --option="torture:twrp_stream=bar" \
+	   --option="torture:twrp_stream_size=3" \
+	   --option="torture:twrp_snapshot=$SNAPSHOT" \
+	   smb2.twrp.stream || \
+        failed=`expr $failed + 1`
+}
+
 build_files $WORKDIR
 
 # test open for writing and write behaviour of snapshoted files
 test_shadow_copy_write "write behaviour of snapshoted files"
 
+test_shadow_copy_stream "reading stream of snapshotted file"
+
 exit $failed
diff --git a/source4/torture/smb2/create.c b/source4/torture/smb2/create.c
index 68447935bd0..912efaac272 100644
--- a/source4/torture/smb2/create.c
+++ b/source4/torture/smb2/create.c
@@ -1809,6 +1809,92 @@ static bool test_twrp_write(struct torture_context *tctx, struct smb2_tree *tree
 	return ret;
 }
 
+static bool test_twrp_stream(struct torture_context *tctx,
+			     struct smb2_tree *tree)
+{
+	struct smb2_create io;
+	NTSTATUS status;
+	bool ret = true;
+	char *p = NULL;
+	struct tm tm;
+	time_t t;
+	uint64_t nttime;
+	const char *file = NULL;
+	const char *stream = NULL;
+	const char *snapshot = NULL;
+	int stream_size;
+	char *path = NULL;
+	uint8_t *buf = NULL;
+	struct smb2_handle h1 = {{0}};
+	struct smb2_read r;
+
+	file = torture_setting_string(tctx, "twrp_file", NULL);
+	if (file == NULL) {
+		torture_skip(tctx, "missing 'twrp_file' option\n");
+	}
+
+	stream = torture_setting_string(tctx, "twrp_stream", NULL);
+	if (stream == NULL) {
+		torture_skip(tctx, "missing 'twrp_stream' option\n");
+	}
+
+	snapshot = torture_setting_string(tctx, "twrp_snapshot", NULL);
+	if (snapshot == NULL) {
+		torture_skip(tctx, "missing 'twrp_snapshot' option\n");
+	}
+
+	stream_size = torture_setting_int(tctx, "twrp_stream_size", 0);
+	if (stream_size == 0) {
+		torture_skip(tctx, "missing 'twrp_stream_size' option\n");
+	}
+
+	torture_comment(tctx, "Testing timewarp on stream (%s) (%s)\n",
+			file, snapshot);
+
+	path = talloc_asprintf(tree, "%s:%s", file, stream);
+	torture_assert_not_null_goto(tctx, path, ret, done, "path\n");
+
+	buf = talloc_zero_array(tree, uint8_t, stream_size);
+	torture_assert_not_null_goto(tctx, buf, ret, done, "buf\n");
+
+	setenv("TZ", "GMT", 1);
+	p = strptime(snapshot, "@GMT-%Y.%m.%d-%H.%M.%S", &tm);
+	torture_assert_goto(tctx, p != NULL, ret, done, "strptime\n");
+	torture_assert_goto(tctx, *p == '\0', ret, done, "strptime\n");
+
+	t = mktime(&tm);
+	unix_to_nt_time(&nttime, t);
+
+	io = (struct smb2_create) {
+		.in.desired_access = SEC_FILE_READ_DATA,
+		.in.file_attributes = FILE_ATTRIBUTE_NORMAL,
+		.in.create_disposition = NTCREATEX_DISP_OPEN,
+		.in.share_access = NTCREATEX_SHARE_ACCESS_MASK,
+		.in.fname = path,
+		.in.timewarp = nttime,
+	};
+
+	status = smb2_create(tree, tctx, &io);
+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+					"smb2_create\n");
+	h1 = io.out.file.handle;
+
+	r = (struct smb2_read) {
+		.in.file.handle = h1,
+		.in.length = stream_size,
+		.in.offset = 0,
+	};
+
+	status = smb2_read(tree, tree, &r);
+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+					"smb2_create\n");
+
+	smb2_util_close(tree, h1);
+
+done:
+	return ret;
+}
+
 /*
    basic testing of SMB2 read
 */
@@ -1840,6 +1926,7 @@ struct torture_suite *torture_smb2_twrp_init(TALLOC_CTX *ctx)
 	struct torture_suite *suite = torture_suite_create(ctx, "twrp");
 
 	torture_suite_add_1smb2_test(suite, "write", test_twrp_write);
+	torture_suite_add_1smb2_test(suite, "stream", test_twrp_stream);
 
 	suite->description = talloc_strdup(suite, "SMB2-TWRP tests");
 
-- 
2.17.2


From e3350909ff001ee80d6b50157d7fb8245a1ac868 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 21 Nov 2018 17:13:42 +0100
Subject: [PATCH 09/10] s3:smbd: GMT token handling on paths: prefix, don't
 append

Appending the GMT token leads to a path parsing error when the path
contains a stream suffix in check_path_syntax_internal():

	while (*s) {
		if (stream_started) {
			switch (*s) {
			case '/':
			case '\\':
				return NT_STATUS_OBJECT_NAME_INVALID;
			case ':':
				if (s[1] == '\0') {
					return NT_STATUS_OBJECT_NAME_INVALID;
				}
				if (strchr_m(&s[1], ':')) {
					return NT_STATUS_OBJECT_NAME_INVALID;
				}
				break;
			}
		}

stream_started is a bool that gets set to true when the ':' stream
separator is hit. From that point onward any backslash '\' in the
remaining path is treated as invalid.

Example:

   foo:stream\@GMT-2018.11.21-16.18.59

The ':' after "foor" sets stream_started to true, then the '\' after
"stream" triggers the error.

Fix: prepend the GMT token to the path, that's what we're later doing in
canonicalize_snapshot_path() anyway. For the above example that results
in:

   @GMT-2018.11.21-16.18.59\foo:stream

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13455

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/smbd/smb2_create.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/source3/smbd/smb2_create.c b/source3/smbd/smb2_create.c
index 7f80f6f8138..21bdcaf44e9 100644
--- a/source3/smbd/smb2_create.c
+++ b/source3/smbd/smb2_create.c
@@ -1193,14 +1193,14 @@ static void smbd_smb2_create_before_exec(struct tevent_req *req)
 
 		state->fname = talloc_asprintf(
 			state,
-			"%s\\@GMT-%04u.%02u.%02u-%02u.%02u.%02u",
-			state->fname,
+			"@GMT-%04u.%02u.%02u-%02u.%02u.%02u\\%s",
 			tm->tm_year + 1900,
 			tm->tm_mon + 1,
 			tm->tm_mday,
 			tm->tm_hour,
 			tm->tm_min,
-			tm->tm_sec);
+			tm->tm_sec,
+			state->fname);
 		if (tevent_req_nomem(state->fname, req)) {
 			return;
 		}
-- 
2.17.2


From 199baa9f4d8148c9231b957e7031baf07fb92c44 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 21 Nov 2018 17:20:30 +0100
Subject: [PATCH 10/10] vfs_shadow_copy2: in fstat also convert fsp->fsp_name
 and fsp->base_fsp->fsp_name

Stacked VFS modules might use the file name, not the file
handle. Looking at you, vfs_fruit...

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13455

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 selftest/knownfail.d/samba3.blackbox |  1 -
 source3/modules/vfs_shadow_copy2.c   | 58 ++++++++++++++++++++++++----
 2 files changed, 50 insertions(+), 9 deletions(-)
 delete mode 100644 selftest/knownfail.d/samba3.blackbox

diff --git a/selftest/knownfail.d/samba3.blackbox b/selftest/knownfail.d/samba3.blackbox
deleted file mode 100644
index a15359e6420..00000000000
--- a/selftest/knownfail.d/samba3.blackbox
+++ /dev/null
@@ -1 +0,0 @@
-^samba3.blackbox.shadow_copy_torture.reading stream of a shadow copy of a file\(fileserver\)
diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
index e105a813e23..0ddc01737bb 100644
--- a/source3/modules/vfs_shadow_copy2.c
+++ b/source3/modules/vfs_shadow_copy2.c
@@ -1354,21 +1354,63 @@ static int shadow_copy2_fstat(vfs_handle_struct *handle, files_struct *fsp,
 			      SMB_STRUCT_STAT *sbuf)
 {
 	time_t timestamp = 0;
+	struct smb_filename *orig_smb_fname = NULL;
+	struct smb_filename vss_smb_fname;
+	struct smb_filename *orig_base_smb_fname = NULL;
+	struct smb_filename vss_base_smb_fname;
+	char *stripped = NULL;
+	int saved_errno = 0;
+	bool ok;
 	int ret;
 
+	ok = shadow_copy2_strip_snapshot(talloc_tos(), handle,
+					 fsp->fsp_name->base_name,
+					 &timestamp, &stripped);
+	if (!ok) {
+		return -1;
+	}
+
+	if (timestamp == 0) {
+		TALLOC_FREE(stripped);
+		return SMB_VFS_NEXT_FSTAT(handle, fsp, sbuf);
+	}
+
+	vss_smb_fname = *fsp->fsp_name;
+	vss_smb_fname.base_name = shadow_copy2_convert(talloc_tos(),
+						       handle,
+						       stripped,
+						       timestamp);
+	TALLOC_FREE(stripped);
+	if (vss_smb_fname.base_name == NULL) {
+		return -1;
+	}
+
+	orig_smb_fname = fsp->fsp_name;
+	fsp->fsp_name = &vss_smb_fname;
+
+	if (fsp->base_fsp != NULL) {
+		vss_base_smb_fname = *fsp->base_fsp->fsp_name;
+		vss_base_smb_fname.base_name = vss_smb_fname.base_name;
+		orig_base_smb_fname = fsp->base_fsp->fsp_name;
+		fsp->base_fsp->fsp_name = &vss_base_smb_fname;
+	}
+
 	ret = SMB_VFS_NEXT_FSTAT(handle, fsp, sbuf);
-	if (ret == -1) {
-		return ret;
+	fsp->fsp_name = orig_smb_fname;
+	if (fsp->base_fsp != NULL) {
+		fsp->base_fsp->fsp_name = orig_base_smb_fname;
 	}
-	if (!shadow_copy2_strip_snapshot(talloc_tos(), handle,
-					 fsp->fsp_name->base_name,
-					 &timestamp, NULL)) {
-		return 0;
+	if (ret == -1) {
+		saved_errno = errno;
 	}
-	if (timestamp != 0) {
+
+	if (ret == 0) {
 		convert_sbuf(handle, fsp->fsp_name->base_name, sbuf);
 	}
-	return 0;
+	if (saved_errno != 0) {
+		errno = saved_errno;
+	}
+	return ret;
 }
 
 static int shadow_copy2_open(vfs_handle_struct *handle,
-- 
2.17.2

More information about the samba-technical mailing list