Attachment 18018 Details for Bug 15439 – Possible patch for master

[patch] Possible patch for master

bug15439.patch (text/plain), 8.36 KB, created by Ralph Böhme on 2023-08-01 13:43:22 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ralph Böhme

Created: 2023-08-01 13:43:22 UTC

Size: 8.36 KB

patch

obsolete

>From ac5336c2865b26df01c6e4f52453f71ce6e22965 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Tue, 1 Aug 2023 13:00:54 +0200
>Subject: [PATCH 1/3] smbd: reuse "truncating" variable in open_file()
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>---
> source3/smbd/open.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/source3/smbd/open.c b/source3/smbd/open.c
>index 87719eec06e6..d5d8ff1285c2 100644
>--- a/source3/smbd/open.c
>+++ b/source3/smbd/open.c
>@@ -1390,7 +1390,7 @@ static NTSTATUS open_file(struct smb_request *req,
> 	 * as we always opened files read-write in that release. JRA.
> 	 */
> 
>-	if ((accmode == O_RDONLY) && ((flags & O_TRUNC) == O_TRUNC)) {
>+	if ((accmode == O_RDONLY) && truncating) {
> 		DEBUG(10,("open_file: truncate requested on read-only open "
> 			  "for file %s\n", smb_fname_str_dbg(smb_fname)));
> 		local_flags = (flags & ~O_ACCMODE)|O_RDWR;
>-- 
>2.41.0
>
>
>From 8c342637deeed2965521dd612670890f90f7a42b Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Tue, 1 Aug 2023 12:30:00 +0200
>Subject: [PATCH 2/3] CVE-XXX: smbtorture: test overwrite dispositions on
> read-only file
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15439
>---
> selftest/knownfail.d/samba3.smb2.acls |   1 +
> source4/torture/smb2/acls.c           | 143 ++++++++++++++++++++++++++
> 2 files changed, 144 insertions(+)
> create mode 100644 selftest/knownfail.d/samba3.smb2.acls
>
>diff --git a/selftest/knownfail.d/samba3.smb2.acls b/selftest/knownfail.d/samba3.smb2.acls
>new file mode 100644
>index 000000000000..18df260c0e50
>--- /dev/null
>+++ b/selftest/knownfail.d/samba3.smb2.acls
>@@ -0,0 +1 @@
>+^samba3.smb2.acls.OVERWRITE_READ_ONLY_FILE
>diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c
>index 2f15ad27e48b..017bdf294875 100644
>--- a/source4/torture/smb2/acls.c
>+++ b/source4/torture/smb2/acls.c
>@@ -3023,6 +3023,148 @@ static bool test_mxac_not_granted(struct torture_context *tctx,
> 	return ret;
> }
> 
>+static bool test_overwrite_read_only_file(struct torture_context *tctx,
>+					  struct smb2_tree *tree)
>+{
>+	NTSTATUS status;
>+	struct smb2_create c;
>+	const char *fname = BASEDIR "\\test_overwrite_read_only_file.txt";
>+	struct smb2_handle handle = {{0}};
>+	union smb_fileinfo q;
>+	union smb_setfileinfo set;
>+	struct security_descriptor *sd = NULL, *sd_orig = NULL;
>+	const char *owner_sid = NULL;
>+	int i;
>+	bool ret = true;
>+
>+	struct tcase {
>+		int disposition;
>+		const char *disposition_string;
>+		NTSTATUS expected_status;
>+	} tcases[] = {
>+#define TCASE(d, s) {				\
>+		.disposition = d,		\
>+		.disposition_string = #d,	\
>+		.expected_status = s,		\
>+	}
>+		TCASE(NTCREATEX_DISP_OPEN, NT_STATUS_OK),
>+		TCASE(NTCREATEX_DISP_SUPERSEDE, NT_STATUS_ACCESS_DENIED),
>+		TCASE(NTCREATEX_DISP_OVERWRITE, NT_STATUS_ACCESS_DENIED),
>+		TCASE(NTCREATEX_DISP_OVERWRITE_IF, NT_STATUS_ACCESS_DENIED),
>+	};
>+#undef TCASE
>+
>+	ret = smb2_util_setup_dir(tctx, tree, BASEDIR);
>+	torture_assert_goto(tctx, ret, ret, done, "smb2_util_setup_dir not ok");
>+
>+	c = (struct smb2_create) {
>+		.in.desired_access = SEC_STD_READ_CONTROL |
>+			SEC_STD_WRITE_DAC |
>+			SEC_STD_WRITE_OWNER,
>+		.in.file_attributes = FILE_ATTRIBUTE_NORMAL,
>+		.in.share_access = NTCREATEX_SHARE_ACCESS_READ |
>+			NTCREATEX_SHARE_ACCESS_WRITE,
>+		.in.create_disposition = NTCREATEX_DISP_OPEN_IF,
>+		.in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS,
>+		.in.fname = fname,
>+	};
>+
>+	status = smb2_create(tree, tctx, &c);
>+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
>+					"smb2_create failed\n");
>+	handle = c.out.file.handle;
>+
>+	torture_comment(tctx, "get the original sd\n");
>+
>+	ZERO_STRUCT(q);
>+	q.query_secdesc.level = RAW_FILEINFO_SEC_DESC;
>+	q.query_secdesc.in.file.handle = handle;
>+	q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER;
>+
>+	status = smb2_getinfo_file(tree, tctx, &q);
>+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
>+					"smb2_getinfo_file failed\n");
>+	sd_orig = q.query_secdesc.out.sd;
>+
>+	owner_sid = dom_sid_string(tctx, sd_orig->owner_sid);
>+
>+	sd = security_descriptor_dacl_create(tctx,
>+					0, NULL, NULL,
>+					owner_sid,
>+					SEC_ACE_TYPE_ACCESS_ALLOWED,
>+					SEC_FILE_READ_DATA,
>+					0,
>+					NULL);
>+
>+	ZERO_STRUCT(set);
>+	set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC;
>+	set.set_secdesc.in.file.handle = handle;
>+	set.set_secdesc.in.secinfo_flags = SECINFO_DACL;
>+	set.set_secdesc.in.sd = sd;
>+
>+	status = smb2_setinfo_file(tree, &set);
>+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
>+					"smb2_setinfo_file failed\n");
>+
>+	smb2_util_close(tree, handle);
>+	ZERO_STRUCT(handle);
>+
>+	for (i = 0; i < ARRAY_SIZE(tcases); i++) {
>+		torture_comment(tctx, "Verify open with %s dispostion\n",
>+				tcases[i].disposition_string);
>+
>+		c = (struct smb2_create) {
>+			.in.create_disposition = tcases[i].disposition,
>+			.in.desired_access = SEC_FILE_READ_DATA,
>+			.in.file_attributes = FILE_ATTRIBUTE_NORMAL,
>+			.in.share_access = NTCREATEX_SHARE_ACCESS_MASK,
>+			.in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS,
>+			.in.fname = fname,
>+		};
>+
>+		status = smb2_create(tree, tctx, &c);
>+		smb2_util_close(tree, c.out.file.handle);
>+		torture_assert_ntstatus_equal_goto(
>+			tctx, status, tcases[i].expected_status, ret, done,
>+			"smb2_create failed\n");
>+	};
>+
>+	torture_comment(tctx, "put back original sd\n");
>+
>+	c = (struct smb2_create) {
>+		.in.desired_access = SEC_STD_WRITE_DAC,
>+		.in.file_attributes = FILE_ATTRIBUTE_NORMAL,
>+		.in.share_access = NTCREATEX_SHARE_ACCESS_MASK,
>+		.in.create_disposition = NTCREATEX_DISP_OPEN_IF,
>+		.in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS,
>+		.in.fname = fname,
>+	};
>+
>+	status = smb2_create(tree, tctx, &c);
>+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
>+					"smb2_create failed\n");
>+	handle = c.out.file.handle;
>+
>+	ZERO_STRUCT(set);
>+	set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC;
>+	set.set_secdesc.in.file.handle = handle;
>+	set.set_secdesc.in.secinfo_flags = SECINFO_DACL;
>+	set.set_secdesc.in.sd = sd_orig;
>+
>+	status = smb2_setinfo_file(tree, &set);
>+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
>+					"smb2_setinfo_file failed\n");
>+
>+	smb2_util_close(tree, handle);
>+	ZERO_STRUCT(handle);
>+
>+done:
>+	smb2_util_close(tree, handle);
>+	smb2_util_unlink(tree, fname);
>+	smb2_deltree(tree, BASEDIR);
>+	return ret;
>+}
>+
> /*
>    basic testing of SMB2 ACLs
> */
>@@ -3051,6 +3193,7 @@ struct torture_suite *torture_smb2_acls_init(TALLOC_CTX *ctx)
> 			test_deny1);
> 	torture_suite_add_1smb2_test(suite, "MXAC-NOT-GRANTED",
> 			test_mxac_not_granted);
>+	torture_suite_add_1smb2_test(suite, "OVERWRITE_READ_ONLY_FILE", test_overwrite_read_only_file);
> 
> 	suite->description = talloc_strdup(suite, "SMB2-ACLS tests");
> 
>-- 
>2.41.0
>
>
>From b9126b44a94dc7976597b54aa73e427c11b89ef5 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Tue, 1 Aug 2023 13:04:36 +0200
>Subject: [PATCH 3/3] CVE-XXX: smbd: use open_access_mask for access check in
> open_file()
>
>If the client requested FILE_OVERWRITE[_IF], we're implicitly adding
>FILE_WRITE_DATA to the open_access_mask in open_file_ntcreate(), but for the
>access check we're using access_mask which doesn't contain the additional
>right, which means we can end up truncating a file for which the user has
>only read-only access via an SD.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15439
>---
> selftest/knownfail.d/samba3.smb2.acls | 1 -
> source3/smbd/open.c                   | 4 ++--
> 2 files changed, 2 insertions(+), 3 deletions(-)
> delete mode 100644 selftest/knownfail.d/samba3.smb2.acls
>
>diff --git a/selftest/knownfail.d/samba3.smb2.acls b/selftest/knownfail.d/samba3.smb2.acls
>deleted file mode 100644
>index 18df260c0e50..000000000000
>--- a/selftest/knownfail.d/samba3.smb2.acls
>+++ /dev/null
>@@ -1 +0,0 @@
>-^samba3.smb2.acls.OVERWRITE_READ_ONLY_FILE
>diff --git a/source3/smbd/open.c b/source3/smbd/open.c
>index d5d8ff1285c2..844a25f8058c 100644
>--- a/source3/smbd/open.c
>+++ b/source3/smbd/open.c
>@@ -1442,7 +1442,7 @@ static NTSTATUS open_file(struct smb_request *req,
> 						dirfsp,
> 						fsp,
> 						false,
>-						access_mask);
>+						open_access_mask);
> 
> 				if (!NT_STATUS_IS_OK(status)) {
> 					DBG_DEBUG("smbd_check_access_rights_fsp"
>@@ -1633,7 +1633,7 @@ static NTSTATUS open_file(struct smb_request *req,
> 			status = smbd_check_access_rights_fsp(dirfsp,
> 							      fsp,
> 							      false,
>-							      access_mask);
>+							      open_access_mask);
> 
> 			if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND) &&
> 			    posix_open &&
>-- 
>2.41.0
>

Flags:

jra: review+
slow: ci-passed+

Actions: View

Attachments on bug 15439: 18018 | 18021 | 18069 | 18070 | 18129 | 18130 | 18131 | 18132 | 18146 | 18149