[PATCHES][BUG 13673] smbd: Fix DELETE_ON_CLOSE behaviour on files with READ_ONLY attribute
Christof Schmitt
cs at samba.org
Fri Nov 2 22:33:28 UTC 2018
On Fri, Nov 02, 2018 at 02:37:44PM -0700, Jeremy Allison wrote:
> On Fri, Nov 02, 2018 at 02:31:10PM -0700, Christof Schmitt wrote:
> > On Fri, Nov 02, 2018 at 02:07:35PM -0700, Jeremy Allison wrote:
> > > On Fri, Nov 02, 2018 at 02:01:11PM -0700, Christof Schmitt wrote:
> > > > On Fri, Nov 02, 2018 at 01:53:55PM -0700, Jeremy Allison via samba-technical wrote:
> > > > > On Fri, Nov 02, 2018 at 01:29:55PM -0700, Christof Schmitt via samba-technical wrote:
> > > > > > 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
> > > > >
> > > > > Oh this is a great catch ! How did you find it ?
> > > >
> > > > Long story. For a special case we had some testing done of trying to
> > > > delete files with READONLY set. This showed differences between trying
> > > > this from Powershell or cmd.exe. It turns out that this triggers two
> > > > different patterns (CREATE then SETINFO-DELETE_ON_CLOSE vs.
> > > > CREATE-DELET_ON_CLOSE). Samba allowing one while denying the other
> > > > seemed inconsistent.
> > > >
> > > > >
> > > > > Reviewing now...
> > > >
> > > > Please hold on. This fails the base.delete.deltest12 test for Samba, but
> > > > the same succeeds against a Windows server:
> > > >
> > > > $ bin/smbtorture ... base.delete.deltest12 smb2.delete-on-close-perms.READONLY
> > > > smbtorture 4.10.0pre1-DEVELOPERBUILD
> > > > Using seed 1541192134
> > > > time: 2018-11-02 20:55:34.630579
> > > > test: deltest12
> > > > time: 2018-11-02 20:55:34.630684
> > > > time: 2018-11-02 20:55:37.730715
> > > > success: deltest12
> > > > time: 2018-11-02 20:55:37.739500
> > > > test: READONLY
> > > > time: 2018-11-02 20:55:37.741927
> > > > Creating file with READ_ONLY attribute.
> > > > Testing CREATE with DELETE_ON_CLOSE on READ_ONLY attribute file.
> > > > Testing setting DELETE_ON_CLOSE disposition on file with READONLY
> > > > attribute.
> > > > time: 2018-11-02 20:55:42.679877
> > > > success: READONLY
> > > >
> > > > I will look into this, maybe this is different in SMB1 vs. SMB2/3.
> > >
> > > Thanks. Yes this very well could be different between SMB1 & SMB2
> > > on Windows. Can you ensure the torture tests pass against recent
> > > Windows then we'll implement that behavior in Samba.
> >
> > The above smbtorture is against a Windows 2012R2, so these work against
> > Windows. I think the issue is that base.delete.deltest12 is testing a
> > CREATE on a new file while smb2.delete-on-close-perms.READONLY is
> > testing against an existing file. My assumption now is that we need two
> > checks to handle both cases
> > can_set_delete_on_close(fsp, new_dos_attributes);
> > and
> > can_set_delete_on_close(fsp, existing_dos_attributes);
>
> Yep, just came to that conclusion myself :-).
>
> Something like this ?
>
> diff --git a/source3/smbd/open.c b/source3/smbd/open.c
> index d6359aac0c6..45ac33483d1 100644
> --- a/source3/smbd/open.c
> +++ b/source3/smbd/open.c
> @@ -3732,8 +3732,13 @@ 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);
> + if (info == FILE_WAS_CREATED) {
> + status = can_set_delete_on_close(fsp,
> + new_dos_attributes);
> + } else {
> + status = can_set_delete_on_close(fsp,
> + existing_dos_attributes);
> + }
>
> if (!NT_STATUS_IS_OK(status)) {
> /* Remember to delete the mode we just added. */
>
Yes. The attached patches cover both cases now. I also discovered that
when NT_STATUS_CANNOT_DELETE is returne for a new file, the file is
still created. The easiest fix seems to be to unlink again. The question
here is whether there is a good way to move the check for the new file
case before actually creating the file.
Christof
-------------- next part --------------
From a7652f14cdf3d4be38806d97bf52ca78ea3bf5ab 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 | 119 +++++++++++++++++++++++++
2 files changed, 121 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..12cdb8540b8 100644
--- a/source4/torture/smb2/delete-on-close.c
+++ b/source4/torture/smb2/delete-on-close.c
@@ -580,6 +580,124 @@ 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");
+
+ create = (struct smb2_create) { };
+ create.in.desired_access = SEC_RIGHTS_DIR_ALL;
+ create.in.create_options = NTCREATEX_OPTIONS_NON_DIRECTORY_FILE |
+ NTCREATEX_OPTIONS_DELETE_ON_CLOSE;
+ 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_equal_goto(tctx, status, expected_status, ret,
+ done, "Unexpected status for CREATE "
+ "of new file.\n");
+
+ if (delete_readonly) {
+ 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, "Creating file with READ_ONLY attribute.\n");
+
+ create = (struct smb2_create) { };
+ 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 +713,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 c866e58c418bdd68a8d9bc613bec892eb175e66c 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.
Also ensure that when STATUS_CANNOT_DELETE is returned for an existing
file, the file is not left behind.
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 | 20 +++++++++++++++++++-
2 files changed, 19 insertions(+), 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..9068f147e72 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -3732,8 +3732,26 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
/* Handle strange delete on close create semantics. */
if (create_options & FILE_DELETE_ON_CLOSE) {
+ if (new_file_created) {
+ status = can_set_delete_on_close(fsp,
+ new_dos_attributes);
+ if (!NT_STATUS_IS_OK(status)) {
+ int ret;
- status = can_set_delete_on_close(fsp, new_dos_attributes);
+ ret = SMB_VFS_UNLINK(conn, fsp->fsp_name);
+ if (ret != 0) {
+ DBG_INFO("Unlink of new file %s failed "
+ "after DELETE_ON_CLOSE check "
+ "returned %s: %s\n",
+ smb_fname_str_dbg(smb_fname),
+ nt_errstr(status),
+ strerror(errno));
+ }
+ }
+ } else {
+ 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 b62782a65a7f8cd63bb3af9bb4eb6c4f56a43903 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 4937421fc17696ece83b69399b5a5262fd95d8ca 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