Attachment 17512 Details for Bug 15153 – Patch for 4.15, 4.16 and 4.17 cherry-picked from master

[patch] Patch for 4.15, 4.16 and 4.17 cherry-picked from master

bug15153-v415,v416,v417.patch (text/plain), 14.58 KB, created by Ralph Böhme on 2022-09-05 09:37:40 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ralph Böhme

Created: 2022-09-05 09:37:40 UTC

Size: 14.58 KB

patch

obsolete

>From d2cc851750dab6ea4761139b7cb7edf96acdc532 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Sun, 14 Aug 2022 18:51:30 +0200
>Subject: [PATCH 1/3] s4/libcli/smb2: avoid using smb2_composite_setpathinfo()
> in smb2_util_setatr()
>
>smb2_composite_setpathinfo() uses SEC_FLAG_MAXIMUM_ALLOWED which can
>have unwanted side effects like breaking oplocks if the effective access
>includes [READ|WRITE]_DATA.
>
>For changing the DOS attributes we only need SEC_FILE_WRITE_ATTRIBUTE. With this
>change test_smb2_oplock_batch25() doesn't trigger an oplock break anymore.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15153
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>Reviewed-by: Stefan Metzmacher <metze@samba.org>
>(cherry picked from commit 66e40690bdd41800a01333ce4243bd62ee2b1894)
>---
> source4/libcli/smb2/util.c    | 37 ++++++++++++++++++++++++++++-------
> source4/torture/smb2/oplock.c | 10 ++--------
> 2 files changed, 32 insertions(+), 15 deletions(-)
>
>diff --git a/source4/libcli/smb2/util.c b/source4/libcli/smb2/util.c
>index b2efabb5a443..f86a149c646a 100644
>--- a/source4/libcli/smb2/util.c
>+++ b/source4/libcli/smb2/util.c
>@@ -88,14 +88,37 @@ NTSTATUS smb2_util_mkdir(struct smb2_tree *tree, const char *dname)
> */
> NTSTATUS smb2_util_setatr(struct smb2_tree *tree, const char *name, uint32_t attrib)
> {
>-	union smb_setfileinfo io;
>-	
>-	ZERO_STRUCT(io);
>-	io.basic_info.level = RAW_SFILEINFO_BASIC_INFORMATION;
>-	io.basic_info.in.file.path = name;
>-	io.basic_info.in.attrib = attrib;
>+	struct smb2_create cr = {0};
>+	struct smb2_handle h1 = {{0}};
>+	union smb_setfileinfo setinfo;
>+	NTSTATUS status;
>+
>+	cr = (struct smb2_create) {
>+		.in.desired_access = SEC_FILE_WRITE_ATTRIBUTE,
>+		.in.share_access = NTCREATEX_SHARE_ACCESS_MASK,
>+		.in.create_disposition = FILE_OPEN,
>+		.in.fname = name,
>+	};
>+	status = smb2_create(tree, tree, &cr);
>+	if (!NT_STATUS_IS_OK(status)) {
>+		return status;
>+	}
>+	h1 = cr.out.file.handle;
> 
>-	return smb2_composite_setpathinfo(tree, &io);
>+	setinfo = (union smb_setfileinfo) {
>+		.basic_info.level = RAW_SFILEINFO_BASIC_INFORMATION,
>+		.basic_info.in.file.handle = h1,
>+		.basic_info.in.attrib = attrib,
>+	};
>+
>+	status = smb2_setinfo_file(tree, &setinfo);
>+	if (!NT_STATUS_IS_OK(status)) {
>+		smb2_util_close(tree, h1);
>+		return status;
>+	}
>+
>+	smb2_util_close(tree, h1);
>+	return NT_STATUS_OK;
> }
> 
> 
>diff --git a/source4/torture/smb2/oplock.c b/source4/torture/smb2/oplock.c
>index 6bf7567ddb1a..8f275bafcdb7 100644
>--- a/source4/torture/smb2/oplock.c
>+++ b/source4/torture/smb2/oplock.c
>@@ -2957,15 +2957,9 @@ static bool test_smb2_oplock_batch25(struct torture_context *tctx,
> 	h1 = io.smb2.out.file.handle;
> 	CHECK_VAL(io.smb2.out.oplock_level, SMB2_OPLOCK_LEVEL_BATCH);
> 
>-	torture_comment(tctx, "changing the file attribute info should trigger "
>-			"a break and a violation\n");
>-
> 	status = smb2_util_setatr(tree1, fname, FILE_ATTRIBUTE_HIDDEN);
>-	torture_assert_ntstatus_equal(tctx, status, NT_STATUS_SHARING_VIOLATION,
>-				      "Incorrect status");
>-
>-	torture_wait_for_oplock_break(tctx);
>-	CHECK_VAL(break_info.count, 1);
>+	torture_assert_ntstatus_ok(tctx, status, "Setting attributes "
>+				   "shouldn't trigger an oplock break");
> 
> 	smb2_util_close(tree1, h1);
> 	smb2_util_close(tree1, h);
>-- 
>2.37.2
>
>
>From d6423337ffb929e4a452997b505ea34f17b7f480 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Fri, 19 Aug 2022 17:29:55 +0200
>Subject: [PATCH 2/3] smbtorture: check required access for SMB2-GETINFO
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15153
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>Reviewed-by: Stefan Metzmacher <metze@samba.org>
>(cherry picked from commit 9b2d28157107602fcbe659664cf9ca25f08bb30b)
>---
> selftest/knownfail                       |   1 +
> selftest/knownfail.d/samba3.smb2.getinfo |   2 +
> source4/torture/smb2/getinfo.c           | 147 +++++++++++++++++++++++
> 3 files changed, 150 insertions(+)
> create mode 100644 selftest/knownfail.d/samba3.smb2.getinfo
>
>diff --git a/selftest/knownfail b/selftest/knownfail
>index 27ca06aa7f25..0db680820bf0 100644
>--- a/selftest/knownfail
>+++ b/selftest/knownfail
>@@ -176,6 +176,7 @@
> ^samba4.smb2.oplock.stream1 # samba 4 oplocks are a mess
> ^samba4.smb2.oplock.statopen1$ad_dc_ntvfs$$ # fails with ACCESS_DENIED on a SYNCHRONIZE_ACCESS open
> ^samba4.smb2.getinfo.complex # streams on directories does not work
>+^samba4.smb2.getinfo.getinfo_access$ad_dc_ntvfs$ # Access checks not implemented
> ^samba4.smb2.getinfo.qfs_buffercheck # S4 does not do the INFO_LENGTH_MISMATCH/BUFFER_OVERFLOW thingy
> ^samba4.smb2.getinfo.qfile_buffercheck # S4 does not do the INFO_LENGTH_MISMATCH/BUFFER_OVERFLOW thingy
> ^samba4.smb2.getinfo.qsec_buffercheck # S4 does not do the BUFFER_TOO_SMALL thingy
>diff --git a/selftest/knownfail.d/samba3.smb2.getinfo b/selftest/knownfail.d/samba3.smb2.getinfo
>new file mode 100644
>index 000000000000..dbef40c83f96
>--- /dev/null
>+++ b/selftest/knownfail.d/samba3.smb2.getinfo
>@@ -0,0 +1,2 @@
>+^samba3.smb2.getinfo.getinfo_access$nt4_dc$
>+^samba3.smb2.getinfo.getinfo_access$ad_dc$
>diff --git a/source4/torture/smb2/getinfo.c b/source4/torture/smb2/getinfo.c
>index 5c9097f42002..539090ac71ab 100644
>--- a/source4/torture/smb2/getinfo.c
>+++ b/source4/torture/smb2/getinfo.c
>@@ -783,6 +783,151 @@ static bool torture_smb2_getinfo(struct torture_context *tctx)
> 	return ret;
> }
> 
>+#undef LEVEL
>+#define LEVEL(l, u, ua, ra) \
>+	.name = #l, \
>+	.level = l, \
>+	.unrestricted = u, \
>+	.unrestricted_access = ua, \
>+	.required_access = ra
>+
>+static struct {
>+	const char *name;
>+	uint16_t level;
>+	bool unrestricted;
>+	uint32_t unrestricted_access;
>+	uint32_t required_access;
>+} file_levels_access[] = {
>+	/*
>+	 * The following info levels are not checked:
>+	 * - FileFullEaInformation and FileIdInformation:
>+	 *   not implemented by the s4/libcli/raw
>+	 * - all pipe infolevels: that should be tested elsewhere by RPC tests
>+	 */
>+
>+	/*
>+	 * The following allow unrestricted access, so requesting
>+	 * SEC_FILE_READ_ATTRIBUTE works, SEC_FILE_READ_ATTRIBUTE or
>+	 * SEC_FILE_READ_EA as well of course.
>+	 */
>+	{ LEVEL(RAW_FILEINFO_STANDARD_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_INTERNAL_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_ACCESS_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_POSITION_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_MODE_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_ALIGNMENT_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_ALT_NAME_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_STREAM_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_COMPRESSION_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_NORMALIZED_NAME_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_EA_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_EA) },
>+
>+	/*
>+	 * The following require either SEC_FILE_READ_ATTRIBUTE or
>+	 * SEC_FILE_READ_EA.
>+	 */
>+	{ LEVEL(RAW_FILEINFO_BASIC_INFORMATION, false, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_ALL_INFORMATION, false, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_NETWORK_OPEN_INFORMATION, false, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_ATTRIBUTE_TAG_INFORMATION, false, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_SMB2_ALL_INFORMATION, false, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) },
>+	{ LEVEL(RAW_FILEINFO_SMB2_ALL_EAS, false, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_EA) },
>+	/* Also try SEC_FILE_READ_ATTRIBUTE to show that it is the wrong one */
>+	{ LEVEL(RAW_FILEINFO_SMB2_ALL_EAS, false, SEC_FILE_READ_ATTRIBUTE, SEC_FILE_READ_EA) },
>+	{ LEVEL(RAW_FILEINFO_SEC_DESC, false, SEC_STD_SYNCHRONIZE, SEC_STD_READ_CONTROL) },
>+	/* Also try SEC_FILE_READ_ATTRIBUTE to show that it is the wrong one */
>+	{ LEVEL(RAW_FILEINFO_SEC_DESC, false, SEC_FILE_READ_ATTRIBUTE, SEC_STD_READ_CONTROL) }
>+};
>+
>+
>+/*
>+  test fileinfo levels
>+*/
>+static bool torture_smb2_getfinfo_access(struct torture_context *tctx,
>+					 struct smb2_tree *tree)
>+{
>+	const char *fname = "torture_smb2_getfinfo_access";
>+	struct smb2_handle hfile;
>+	NTSTATUS status;
>+	bool ret = true;
>+	int i;
>+
>+	smb2_deltree(tree, fname);
>+
>+	torture_setup_complex_file(tctx, tree, fname);
>+
>+	for (i = 0; i < ARRAY_SIZE(file_levels_access); i++) {
>+		union smb_fileinfo finfo;
>+		NTSTATUS expected_status;
>+
>+		/*
>+		 * First open with unrestricted_access, SEC_STD_SYNCHRONIZE for
>+		 * most tests, info levels with unrestricted=true should allow
>+		 * this.
>+		 */
>+		status = torture_smb2_testfile_access(
>+			tree, fname, &hfile, file_levels_access[i].unrestricted_access);
>+		torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
>+					   "Unable to open test file\n");
>+
>+		if (file_levels_access[i].level == RAW_FILEINFO_SEC_DESC) {
>+			finfo.query_secdesc.in.secinfo_flags = 0x7;
>+		}
>+		if (file_levels_access[i].level == RAW_FILEINFO_SMB2_ALL_EAS) {
>+			finfo.all_eas.in.continue_flags =
>+				SMB2_CONTINUE_FLAG_RESTART;
>+		}
>+
>+		finfo.generic.level = file_levels_access[i].level;
>+		finfo.generic.in.file.handle = hfile;
>+
>+		if (file_levels_access[i].unrestricted) {
>+			expected_status = NT_STATUS_OK;
>+		} else {
>+			expected_status = NT_STATUS_ACCESS_DENIED;
>+		}
>+
>+		status = smb2_getinfo_file(tree, tree, &finfo);
>+		torture_assert_ntstatus_equal_goto(
>+			tctx, status, expected_status, ret, done,
>+			talloc_asprintf(tctx, "Level %s failed\n",
>+					file_levels_access[i].name));
>+
>+		smb2_util_close(tree, hfile);
>+
>+		/*
>+		 * Now open with expected access, getinfo should work.
>+		 */
>+		status = torture_smb2_testfile_access(
>+			tree, fname, &hfile, file_levels_access[i].required_access);
>+		torture_assert_ntstatus_ok_goto(
>+			tctx, status, ret, done,
>+			"Unable to open test file\n");
>+
>+		if (file_levels_access[i].level == RAW_FILEINFO_SEC_DESC) {
>+			finfo.query_secdesc.in.secinfo_flags = 0x7;
>+		}
>+		if (file_levels_access[i].level == RAW_FILEINFO_SMB2_ALL_EAS) {
>+			finfo.all_eas.in.continue_flags =
>+				SMB2_CONTINUE_FLAG_RESTART;
>+		}
>+		finfo.generic.level = file_levels_access[i].level;
>+		finfo.generic.in.file.handle = hfile;
>+
>+		status = smb2_getinfo_file(tree, tree, &finfo);
>+		torture_assert_ntstatus_ok_goto(
>+			tctx, status, ret, done,
>+			talloc_asprintf(tctx, "%s on file",
>+					file_levels_access[i].name));
>+
>+		smb2_util_close(tree, hfile);
>+	}
>+
>+done:
>+	smb2_deltree(tree, fname);
>+	return ret;
>+}
>+
> struct torture_suite *torture_smb2_getinfo_init(TALLOC_CTX *ctx)
> {
> 	struct torture_suite *suite = torture_suite_create(
>@@ -800,5 +945,7 @@ struct torture_suite *torture_smb2_getinfo_init(TALLOC_CTX *ctx)
> 				      torture_smb2_fileinfo_grant_read);
> 	torture_suite_add_simple_test(suite, "normalized",
> 				      torture_smb2_fileinfo_normalized);
>+	torture_suite_add_1smb2_test(suite, "getinfo_access",
>+				     torture_smb2_getfinfo_access);
> 	return suite;
> }
>-- 
>2.37.2
>
>
>From e802efc27ff2aa65648f4afa66d2a5ca029c12d7 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Sun, 14 Aug 2022 18:46:24 +0200
>Subject: [PATCH 3/3] smbd: implement access checks for SMB2-GETINFO as per
> MS-SMB2 3.3.5.20.1
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>Content-Transfer-Encoding: 8bit
>
>The spec lists the following as requiring special access:
>
>- for requiring FILE_READ_ATTRIBUTES:
>
>  FileBasicInformation
>  FileAllInformation
>  FileNetworkOpenInformation
>  FileAttributeTagInformation
>
>- for requiring FILE_READ_EA:
>
>  FileFullEaInformation
>
>All other infolevels are unrestricted.
>
>We ignore the IPC related infolevels:
>
>  FilePipeInformation
>  FilePipeLocalInformation
>  FilePipeRemoteInformation
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15153
>RN: Missing SMB2-GETINFO access checks from MS-SMB2 3.3.5.20.1
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>Reviewed-by: Stefan Metzmacher <metze@samba.org>
>
>Autobuild-User(master): Ralph BÃ¶hme <slow@samba.org>
>Autobuild-Date(master): Tue Aug 23 12:54:08 UTC 2022 on sn-devel-184
>
>(cherry picked from commit 6d493a9d568c08cfe5242821ccbd5a5ee1fe5284)
>---
> selftest/knownfail                       |  2 --
> selftest/knownfail.d/samba3.smb2.getinfo |  2 --
> source3/smbd/smb2_getinfo.c              | 28 ++++++++++++++++++++++++
> 3 files changed, 28 insertions(+), 4 deletions(-)
> delete mode 100644 selftest/knownfail.d/samba3.smb2.getinfo
>
>diff --git a/selftest/knownfail b/selftest/knownfail
>index 0db680820bf0..f130d2dc3c5a 100644
>--- a/selftest/knownfail
>+++ b/selftest/knownfail
>@@ -208,10 +208,8 @@
> ^samba3.smb2.oplock.stream1
> ^samba3.smb2.streams.rename
> ^samba3.smb2.streams.rename2
>-^samba3.smb2.streams.attributes1$.*$
> ^samba3.smb2.streams streams_xattr.rename$nt4_dc$
> ^samba3.smb2.streams streams_xattr.rename2$nt4_dc$
>-^samba3.smb2.streams streams_xattr.attributes1$nt4_dc$
> ^samba3.smb2.getinfo.complex
> ^samba3.smb2.getinfo.fsinfo # quotas don't work yet
> ^samba3.smb2.setinfo.setinfo
>diff --git a/selftest/knownfail.d/samba3.smb2.getinfo b/selftest/knownfail.d/samba3.smb2.getinfo
>deleted file mode 100644
>index dbef40c83f96..000000000000
>--- a/selftest/knownfail.d/samba3.smb2.getinfo
>+++ /dev/null
>@@ -1,2 +0,0 @@
>-^samba3.smb2.getinfo.getinfo_access$nt4_dc$
>-^samba3.smb2.getinfo.getinfo_access$ad_dc$
>diff --git a/source3/smbd/smb2_getinfo.c b/source3/smbd/smb2_getinfo.c
>index 0320dcc5fdef..23322e7b85f3 100644
>--- a/source3/smbd/smb2_getinfo.c
>+++ b/source3/smbd/smb2_getinfo.c
>@@ -303,6 +303,34 @@ static struct tevent_req *smbd_smb2_getinfo_send(TALLOC_CTX *mem_ctx,
> 
> 		ZERO_STRUCT(write_time_ts);
> 
>+		/*
>+		 * MS-SMB2 3.3.5.20.1 "Handling SMB2_0_INFO_FILE"
>+		 *
>+		 * FileBasicInformation, FileAllInformation,
>+		 * FileNetworkOpenInformation, FileAttributeTagInformation
>+		 * require FILE_READ_ATTRIBUTES.
>+		 *
>+		 * FileFullEaInformation requires FILE_READ_EA.
>+		 */
>+		switch (in_file_info_class) {
>+		case FSCC_FILE_BASIC_INFORMATION:
>+		case FSCC_FILE_ALL_INFORMATION:
>+		case FSCC_FILE_NETWORK_OPEN_INFORMATION:
>+		case FSCC_FILE_ATTRIBUTE_TAG_INFORMATION:
>+			if (!(fsp->access_mask & SEC_FILE_READ_ATTRIBUTE)) {
>+				tevent_req_nterror(req, NT_STATUS_ACCESS_DENIED);
>+				return tevent_req_post(req, ev);
>+			}
>+			break;
>+
>+		case FSCC_FILE_FULL_EA_INFORMATION:
>+			if (!(fsp->access_mask & SEC_FILE_READ_EA)) {
>+				tevent_req_nterror(req, NT_STATUS_ACCESS_DENIED);
>+				return tevent_req_post(req, ev);
>+			}
>+			break;
>+		}
>+
> 		switch (in_file_info_class) {
> 		case FSCC_FILE_FULL_EA_INFORMATION:
> 			file_info_level = SMB2_FILE_FULL_EA_INFORMATION;
>-- 
>2.37.2
>

Flags:

metze: review+

Actions: View

Attachments on bug 15153: 17512