[PATCHES][BUG 13673] smbd: Fix DELETE_ON_CLOSE behaviour on files with READ_ONLY attribute

Christof Schmitt cs at samba.org
Fri Nov 2 20:29:55 UTC 2018


smbd allowed CREATE on a file with the READONLY attribute set and
DELETE_ON_CLOSE requested, even though MS-FSA states that this should be
denied. On the other hand, SETINFO for DELETE_ON_CLOSE is correctly
rejected.

See the attached patches for a fix. This also adds a smbtorture test for
this case and the Samba override 'delete readonly=yes'

gitlab pipeline
https://gitlab.com/samba-team/devel/samba/pipelines/35263504

Christof
-------------- next part --------------
From 26452449e999f3197971f5188660899a352e757d Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Fri, 2 Nov 2018 10:49:53 -0700
Subject: [PATCH 1/4] smbtorture: Add test for DELETE_ON_CLOSE on files with
 READ_ONLY attribute

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13673

Signed-off-by: Christof Schmitt <cs at samba.org>
---
 selftest/knownfail                     |  2 +
 source4/torture/smb2/delete-on-close.c | 95 ++++++++++++++++++++++++++
 2 files changed, 97 insertions(+)

diff --git a/selftest/knownfail b/selftest/knownfail
index 16c2274daec..ed1383cace8 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -356,3 +356,5 @@
 ^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).ntlmdisabled.NtlmDisabledTests.test_ntlm_connection\(ad_dc_no_ntlm\)
 ^samba.tests.ntlmdisabled.python\(ktest\).python3.ntlmdisabled.NtlmDisabledTests.test_samr_change_password\(ktest\)
 ^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).python3.ntlmdisabled.NtlmDisabledTests.test_ntlm_connection\(ad_dc_no_ntlm\)
+^samba3.smb2.delete-on-close-perms.READONLY\(nt4_dc\)
+^samba3.smb2.delete-on-close-perms.READONLY\(ad_dc\)
diff --git a/source4/torture/smb2/delete-on-close.c b/source4/torture/smb2/delete-on-close.c
index 2312df285a3..9651c9dae29 100644
--- a/source4/torture/smb2/delete-on-close.c
+++ b/source4/torture/smb2/delete-on-close.c
@@ -580,6 +580,100 @@ static bool test_doc_find_and_set_doc(struct torture_context *tctx, struct smb2_
 	return true;
 }
 
+static bool test_doc_read_only(struct torture_context *tctx,
+			       struct smb2_tree *tree)
+{
+	struct smb2_handle dir_handle;
+	union smb_setfileinfo sfinfo = { };
+	struct smb2_create create = { };
+	struct smb2_close close = { };
+	NTSTATUS status, expected_status;
+	bool ret = true, delete_readonly;
+
+	/*
+	 * Allow testing of the Samba 'delete readonly' option.
+	 */
+	delete_readonly = torture_setting_bool(tctx, "delete_readonly", false);
+	expected_status = delete_readonly ?
+		NT_STATUS_OK : NT_STATUS_CANNOT_DELETE;
+
+	smb2_deltree(tree, DNAME);
+
+	status = torture_smb2_testdir(tree, DNAME, &dir_handle);
+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+					"CREATE directory failed\n");
+
+	torture_comment(tctx, "Creating file with READ_ONLY attribute.\n");
+
+	create.in.desired_access = SEC_RIGHTS_DIR_ALL;
+	create.in.create_options = NTCREATEX_OPTIONS_NON_DIRECTORY_FILE;
+	create.in.file_attributes = FILE_ATTRIBUTE_READONLY;
+	create.in.share_access = NTCREATEX_SHARE_ACCESS_READ |
+		NTCREATEX_SHARE_ACCESS_WRITE |
+		NTCREATEX_SHARE_ACCESS_DELETE;
+	create.in.create_disposition = NTCREATEX_DISP_CREATE;
+	create.in.fname = FNAME;
+	status = smb2_create(tree, tctx, &create);
+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+					"CREATE of READONLY file failed.\n");
+
+	close.in.file.handle = create.out.file.handle;
+	status = smb2_close(tree, &close);
+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+					"CLOSE of READONLY file failed.\n");
+
+	torture_comment(tctx, "Testing CREATE with DELETE_ON_CLOSE on "
+			"READ_ONLY attribute file.\n");
+
+	create = (struct smb2_create) { };
+	create.in.desired_access = SEC_RIGHTS_FILE_READ | SEC_STD_DELETE;
+	create.in.create_options = NTCREATEX_OPTIONS_DELETE_ON_CLOSE;
+	create.in.file_attributes = 0;
+	create.in.share_access = NTCREATEX_SHARE_ACCESS_READ |
+		NTCREATEX_SHARE_ACCESS_WRITE |
+		NTCREATEX_SHARE_ACCESS_DELETE;
+	create.in.create_disposition = NTCREATEX_DISP_OPEN;
+	create.in.fname = FNAME;
+	status = smb2_create(tree, tctx, &create);
+	torture_assert_ntstatus_equal_goto(tctx, status,
+					   expected_status, ret, done,
+					   "CREATE returned unexpected "
+					   "status.\n");
+
+	torture_comment(tctx, "Testing setting DELETE_ON_CLOSE disposition on "
+			" file with READONLY attribute.\n");
+
+	create = (struct smb2_create) { };
+	create.in.desired_access = SEC_RIGHTS_FILE_READ | SEC_STD_DELETE;;
+	create.in.create_options = 0;
+	create.in.file_attributes = 0;
+	create.in.share_access = NTCREATEX_SHARE_ACCESS_READ |
+		NTCREATEX_SHARE_ACCESS_WRITE |
+		NTCREATEX_SHARE_ACCESS_DELETE;
+	create.in.create_disposition = NTCREATEX_DISP_OPEN;
+	create.in.fname = FNAME;
+	status = smb2_create(tree, tctx, &create);
+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+					"Opening file failed.\n");
+
+	sfinfo.disposition_info.in.delete_on_close = 1;
+	sfinfo.generic.level = RAW_SFILEINFO_DISPOSITION_INFORMATION;
+	sfinfo.generic.in.file.handle = create.out.file.handle;
+
+	status = smb2_setinfo_file(tree, &sfinfo);
+	torture_assert_ntstatus_equal(tctx, status, expected_status,
+				      "Set DELETE_ON_CLOSE disposition "
+				      "returned un expected status.\n");
+
+	status = smb2_util_close(tree, create.out.file.handle);
+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+					"CLOSE failed\n");
+
+done:
+	smb2_deltree(tree, DNAME);
+	return ret;
+}
+
 
 /*
  *  Extreme testing of Delete On Close and permissions
@@ -595,6 +689,7 @@ struct torture_suite *torture_smb2_doc_init(TALLOC_CTX *ctx)
 	torture_suite_add_1smb2_test(suite, "CREATE_IF", test_doc_create_if);
 	torture_suite_add_1smb2_test(suite, "CREATE_IF Existing", test_doc_create_if_exist);
 	torture_suite_add_1smb2_test(suite, "FIND_and_set_DOC", test_doc_find_and_set_doc);
+	torture_suite_add_1smb2_test(suite,  "READONLY", test_doc_read_only);
 
 	suite->description = talloc_strdup(suite, "SMB2-Delete-on-Close-Perms tests");
 
-- 
2.17.0


From 5ee4b7cc81e7b5dc12d51b401ee9819cab9a3f00 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Fri, 2 Nov 2018 12:08:23 -0700
Subject: [PATCH 2/4] smbd: Fix DELETE_ON_CLOSE behaviour on files with
 READ_ONLY attribute

MS-FSA states that a CREATE with FILE_DELETE_ON_CLOSE on an existing
file with READ_ONLY attribute has to return STATUS_CANNOT_DELETE. This
was missing in smbd as the check used the DOS attributes from the CREATE
instead of the DOS attributes on the existing file.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13673

Signed-off-by: Christof Schmitt <cs at samba.org>
---
 selftest/knownfail  | 2 --
 source3/smbd/open.c | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/selftest/knownfail b/selftest/knownfail
index ed1383cace8..16c2274daec 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -356,5 +356,3 @@
 ^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).ntlmdisabled.NtlmDisabledTests.test_ntlm_connection\(ad_dc_no_ntlm\)
 ^samba.tests.ntlmdisabled.python\(ktest\).python3.ntlmdisabled.NtlmDisabledTests.test_samr_change_password\(ktest\)
 ^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).python3.ntlmdisabled.NtlmDisabledTests.test_ntlm_connection\(ad_dc_no_ntlm\)
-^samba3.smb2.delete-on-close-perms.READONLY\(nt4_dc\)
-^samba3.smb2.delete-on-close-perms.READONLY\(ad_dc\)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index d6359aac0c6..27222c02807 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -3733,7 +3733,7 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
 	/* Handle strange delete on close create semantics. */
 	if (create_options & FILE_DELETE_ON_CLOSE) {
 
-		status = can_set_delete_on_close(fsp, new_dos_attributes);
+		status = can_set_delete_on_close(fsp, existing_dos_attributes);
 
 		if (!NT_STATUS_IS_OK(status)) {
 			/* Remember to delete the mode we just added. */
-- 
2.17.0


From a0bdc438aaf88666d1bd6ea32138a134863db8ad Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Fri, 2 Nov 2018 12:03:51 -0700
Subject: [PATCH 3/4] selftest: Add share to test "delete readonly" option

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13673

Signed-off-by: Christof Schmitt <cs at samba.org>
---
 selftest/target/Samba3.pm | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index f7957cf3a7d..c5c62b24bb2 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -2250,6 +2250,10 @@ sub provision($$$$$$$$$)
 	kernel oplocks = no
 	posix locking = no
 	include = $libdir/delay_inject.conf
+
+[delete_readonly]
+	path = $prefix_abs/share
+	delete readonly = yes
 	";
 	close(CONF);
 
-- 
2.17.0


From 0d23b3986dc503dab075566d80cd62410b58ae1b Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Fri, 2 Nov 2018 12:07:58 -0700
Subject: [PATCH 4/4] selftest: Run smb2.delete-on-close-perms also with
 "delete readonly = yes"

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13673

Signed-off-by: Christof Schmitt <cs at samba.org>
---
 source3/selftest/tests.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 08395c1c096..4253c7f63c0 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -620,6 +620,10 @@ for t in tests:
         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')
+    elif t == "smb2.delete-on-close-perms":
+        plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
+        plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/delete_readonly -U$USERNAME%$PASSWORD --option=torture:delete_readonly=true')
+        plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
     else:
         plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
         plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
-- 
2.17.0



More information about the samba-technical mailing list