[PATCHES][BUG 13673] smbd: Fix DELETE_ON_CLOSE behaviour on files with READ_ONLY attribute
Jeremy Allison
jra at samba.org
Fri Nov 2 20:53:55 UTC 2018
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 ?
Reviewing now...
Jeremy.
> 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