[PATCH] Version 3 - Fix smbd behavior with SMB2-FLUSH on a directory handle.
Jeremy Allison
jra at samba.org
Wed May 16 22:56:32 UTC 2018
On Fri, May 11, 2018 at 09:23:24AM -0700, Jeremy Allison wrote:
> Changes from V2.
>
> After review from Ralph, clarified
> that *either* FILE_ADD_FILE or
> FILE_ADD_SUBDIRECTORY is enough
> to make flush succeed.
>
> Updated the 'if' statement to
> use a helper variable and improved
> the comment, hopefully this will
> make it easier to understand and
> review.
>
> Added separate tests against
> FILE_ADD_FILE and FILE_ADD_SUBDIRECTORY
> bits to show either is sufficient
> to all dir handle flush (tested
> works against Windows server).
>
> Please review and push if happy !
Ping ! Just got confirmation from Microsoft
doc-help that indeed this is the way that
a Windows server behaves on the wire.
Ralph, if you can take a look I'd appreciate
it.
Cheers,
Jeremy.
> From 247bc5d171659b142343072bf74f1d3809b00bde Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Thu, 10 May 2018 10:26:52 -0700
> Subject: [PATCH 1/2] s3: smbd: Fix SMB2-FLUSH against directories.
>
> Directories opened with either FILE_ADD_FILE or
> FILE_ADD_SUBDIRECTORY can be flushed even if
> they're not writable in the conventional sense.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13428
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> source3/smbd/smb2_flush.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/source3/smbd/smb2_flush.c b/source3/smbd/smb2_flush.c
> index f7d9e964319..c0340103624 100644
> --- a/source3/smbd/smb2_flush.c
> +++ b/source3/smbd/smb2_flush.c
> @@ -23,6 +23,7 @@
> #include "smbd/globals.h"
> #include "../libcli/smb/smb_common.h"
> #include "../lib/util/tevent_ntstatus.h"
> +#include "libcli/security/security.h"
>
> #undef DBGC_CLASS
> #define DBGC_CLASS DBGC_SMB2
> @@ -147,8 +148,29 @@ static struct tevent_req *smbd_smb2_flush_send(TALLOC_CTX *mem_ctx,
> }
>
> if (!CHECK_WRITE(fsp)) {
> - tevent_req_nterror(req, NT_STATUS_ACCESS_DENIED);
> - return tevent_req_post(req, ev);
> + bool allow_dir_flush = false;
> +
> + if (!fsp->is_directory) {
> + tevent_req_nterror(req, NT_STATUS_ACCESS_DENIED);
> + return tevent_req_post(req, ev);
> + }
> +
> + /*
> + * Directories are not writable in the conventional
> + * sense, but if opened with *either*
> + * FILE_ADD_FILE or FILE_ADD_SUBDIRECTORY
> + * they can be flushed.
> + */
> +
> + if ((fsp->access_mask &
> + (FILE_ADD_FILE|FILE_ADD_SUBDIRECTORY)) != 0) {
> + allow_dir_flush = true;
> + }
> +
> + if (allow_dir_flush == false) {
> + tevent_req_nterror(req, NT_STATUS_ACCESS_DENIED);
> + return tevent_req_post(req, ev);
> + }
> }
>
> if (fsp->fh->fd == -1) {
> --
> 2.17.0.441.gb46fe60e1d-goog
>
>
> From cd6826d9319a26be82dab3a6a93772761d49eb56 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Thu, 10 May 2018 11:30:24 -0700
> Subject: [PATCH 2/2] s3: smbtorture: Add new SMB2-DIR-FSYNC test to show
> behavior of FSYNC on directories.
>
> Tests against a directory handle on the root of a share,
> and a directory handle on a sub-directory in a share.
>
> Check SEC_DIR_ADD_FILE and SEC_DIR_ADD_SUBDIR separately,
> either allows flush to succeed.
>
> Passes against Windows.
>
> Regression test for:
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13428
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> selftest/knownfail | 1 +
> source3/selftest/tests.py | 2 +-
> source3/torture/proto.h | 1 +
> source3/torture/test_smb2.c | 270 ++++++++++++++++++++++++++++++++++++
> source3/torture/torture.c | 1 +
> 5 files changed, 274 insertions(+), 1 deletion(-)
>
> diff --git a/selftest/knownfail b/selftest/knownfail
> index a2aeed2690d..8c70d6a6172 100644
> --- a/selftest/knownfail
> +++ b/selftest/knownfail
> @@ -8,6 +8,7 @@
> .*driver.add_driver_timestamps # we only can store dates, not timestamps
> ^samba3.smbtorture_s3.crypt_server\(nt4_dc\).SMB2-SESSION-REAUTH # expected to give ACCESS_DENIED SMB2.1 doesn't have encryption
> ^samba3.smbtorture_s3.crypt_server\(nt4_dc\).SMB2-SESSION-RECONNECT # expected to give CONNECTION_DISCONNECTED, we need to fix the test
> +^samba3.smbtorture_s3.*ad_dc_ntvfs.*SMB2-DIR-FSYNC.*
> ^samba3.smb2.session enc.reconnect # expected to give CONNECTION_DISCONNECTED, we need to fix the test
> ^samba3.raw.session enc # expected to give ACCESS_DENIED as SMB1 encryption isn't used
> ^samba3.smbtorture_s3.crypt_server # expected to give ACCESS_DENIED as SMB1 encryption isn't used
> diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
> index c234679b1cd..790144aa24f 100755
> --- a/source3/selftest/tests.py
> +++ b/source3/selftest/tests.py
> @@ -78,7 +78,7 @@ tests = ["FDPASS", "LOCK1", "LOCK2", "LOCK3", "LOCK4", "LOCK5", "LOCK6", "LOCK7"
> "UID-REGRESSION-TEST", "SHORTNAME-TEST",
> "CASE-INSENSITIVE-CREATE", "SMB2-BASIC", "NTTRANS-FSCTL", "SMB2-NEGPROT",
> "SMB2-SESSION-REAUTH", "SMB2-SESSION-RECONNECT", "SMB2-FTRUNCATE",
> - "SMB2-ANONYMOUS",
> + "SMB2-ANONYMOUS", "SMB2-DIR-FSYNC",
> "CLEANUP1",
> "CLEANUP2",
> "CLEANUP4",
> diff --git a/source3/torture/proto.h b/source3/torture/proto.h
> index 45870c904fb..1634da49315 100644
> --- a/source3/torture/proto.h
> +++ b/source3/torture/proto.h
> @@ -101,6 +101,7 @@ bool run_smb2_tcon_dependence(int dummy);
> bool run_smb2_multi_channel(int dummy);
> bool run_smb2_session_reauth(int dummy);
> bool run_smb2_ftruncate(int dummy);
> +bool run_smb2_dir_fsync(int dummy);
> bool run_chain3(int dummy);
> bool run_local_conv_auth_info(int dummy);
> bool run_local_sprintf_append(int dummy);
> diff --git a/source3/torture/test_smb2.c b/source3/torture/test_smb2.c
> index 897d034f6a9..094a9b84d6e 100644
> --- a/source3/torture/test_smb2.c
> +++ b/source3/torture/test_smb2.c
> @@ -2065,3 +2065,273 @@ bool run_smb2_ftruncate(int dummy)
> }
> return correct;
> }
> +
> +/* Ensure SMB2 flush on directories behaves correctly. */
> +
> +static bool test_dir_fsync(struct cli_state *cli, const char *path)
> +{
> + NTSTATUS status;
> + uint64_t fid_persistent, fid_volatile;
> + uint8_t *dir_data = NULL;
> + uint32_t dir_data_length = 0;
> +
> + /* Open directory - no write abilities. */
> + status = smb2cli_create(cli->conn, cli->timeout, cli->smb2.session,
> + cli->smb2.tcon, path,
> + SMB2_OPLOCK_LEVEL_NONE, /* oplock_level, */
> + SMB2_IMPERSONATION_IMPERSONATION, /* impersonation_level, */
> + SEC_STD_SYNCHRONIZE|
> + SEC_DIR_LIST|
> + SEC_DIR_READ_ATTRIBUTE, /* desired_access, */
> + 0, /* file_attributes, */
> + FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, /* share_access, */
> + FILE_OPEN, /* create_disposition, */
> + FILE_SYNCHRONOUS_IO_NONALERT|FILE_DIRECTORY_FILE, /* create_options, */
> + NULL, /* smb2_create_blobs *blobs */
> + &fid_persistent,
> + &fid_volatile,
> + NULL, NULL, NULL);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smb2cli_create '%s' (readonly) returned %s\n",
> + path,
> + nt_errstr(status));
> + return false;
> + }
> +
> + status = smb2cli_query_directory(
> + cli->conn, cli->timeout, cli->smb2.session, cli->smb2.tcon,
> + 1, 0, 0, fid_persistent, fid_volatile, "*", 0xffff,
> + talloc_tos(), &dir_data, &dir_data_length);
> +
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smb2cli_query_directory returned %s\n",
> + nt_errstr(status));
> + return false;
> + }
> +
> + /* Open directory no write access. Flush should fail. */
> +
> + status = smb2cli_flush(cli->conn, cli->timeout, cli->smb2.session,
> + cli->smb2.tcon, fid_persistent, fid_volatile);
> + if (!NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED)) {
> + printf("smb2cli_flush on a read-only directory returned %s\n",
> + nt_errstr(status));
> + return false;
> + }
> +
> + status = smb2cli_close(cli->conn, cli->timeout, cli->smb2.session,
> + cli->smb2.tcon, 0, fid_persistent, fid_volatile);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smb2cli_close returned %s\n", nt_errstr(status));
> + return false;
> + }
> +
> + /* Open directory write-attributes only. Flush should still fail. */
> +
> + status = smb2cli_create(cli->conn, cli->timeout, cli->smb2.session,
> + cli->smb2.tcon, path,
> + SMB2_OPLOCK_LEVEL_NONE, /* oplock_level, */
> + SMB2_IMPERSONATION_IMPERSONATION, /* impersonation_level, */
> + SEC_STD_SYNCHRONIZE|
> + SEC_DIR_LIST|
> + SEC_DIR_WRITE_ATTRIBUTE|
> + SEC_DIR_READ_ATTRIBUTE, /* desired_access, */
> + 0, /* file_attributes, */
> + FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, /* share_access, */
> + FILE_OPEN, /* create_disposition, */
> + FILE_SYNCHRONOUS_IO_NONALERT|FILE_DIRECTORY_FILE, /* create_options, */
> + NULL, /* smb2_create_blobs *blobs */
> + &fid_persistent,
> + &fid_volatile,
> + NULL, NULL, NULL);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smb2cli_create '%s' (write attr) returned %s\n",
> + path,
> + nt_errstr(status));
> + return false;
> + }
> +
> + status = smb2cli_query_directory(
> + cli->conn, cli->timeout, cli->smb2.session, cli->smb2.tcon,
> + 1, 0, 0, fid_persistent, fid_volatile, "*", 0xffff,
> + talloc_tos(), &dir_data, &dir_data_length);
> +
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smb2cli_query_directory returned %s\n", nt_errstr(status));
> + return false;
> + }
> +
> + status = smb2cli_flush(cli->conn, cli->timeout, cli->smb2.session,
> + cli->smb2.tcon, fid_persistent, fid_volatile);
> + if (!NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED)) {
> + printf("smb2cli_flush on a write-attributes directory "
> + "returned %s\n",
> + nt_errstr(status));
> + return false;
> + }
> +
> + status = smb2cli_close(cli->conn, cli->timeout, cli->smb2.session,
> + cli->smb2.tcon, 0, fid_persistent, fid_volatile);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smb2cli_close returned %s\n", nt_errstr(status));
> + return false;
> + }
> +
> + /* Open directory with SEC_DIR_ADD_FILE access. Flush should now succeed. */
> +
> + status = smb2cli_create(cli->conn, cli->timeout, cli->smb2.session,
> + cli->smb2.tcon, path,
> + SMB2_OPLOCK_LEVEL_NONE, /* oplock_level, */
> + SMB2_IMPERSONATION_IMPERSONATION, /* impersonation_level, */
> + SEC_STD_SYNCHRONIZE|
> + SEC_DIR_LIST|
> + SEC_DIR_ADD_FILE, /* desired_access, */
> + 0, /* file_attributes, */
> + FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, /* share_access, */
> + FILE_OPEN, /* create_disposition, */
> + FILE_SYNCHRONOUS_IO_NONALERT|FILE_DIRECTORY_FILE, /* create_options, */
> + NULL, /* smb2_create_blobs *blobs */
> + &fid_persistent,
> + &fid_volatile,
> + NULL, NULL, NULL);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smb2cli_create '%s' (write FILE access) returned %s\n",
> + path,
> + nt_errstr(status));
> + return false;
> + }
> +
> + status = smb2cli_query_directory(
> + cli->conn, cli->timeout, cli->smb2.session, cli->smb2.tcon,
> + 1, 0, 0, fid_persistent, fid_volatile, "*", 0xffff,
> + talloc_tos(), &dir_data, &dir_data_length);
> +
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smb2cli_query_directory returned %s\n", nt_errstr(status));
> + return false;
> + }
> +
> + status = smb2cli_flush(cli->conn, cli->timeout, cli->smb2.session,
> + cli->smb2.tcon, fid_persistent, fid_volatile);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smb2cli_flush on a directory returned %s\n",
> + nt_errstr(status));
> + return false;
> + }
> +
> + status = smb2cli_close(cli->conn, cli->timeout, cli->smb2.session,
> + cli->smb2.tcon, 0, fid_persistent, fid_volatile);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smb2cli_close returned %s\n", nt_errstr(status));
> + return false;
> + }
> +
> + /* Open directory with SEC_DIR_ADD_FILE access. Flush should now succeed. */
> +
> + status = smb2cli_create(cli->conn, cli->timeout, cli->smb2.session,
> + cli->smb2.tcon, path,
> + SMB2_OPLOCK_LEVEL_NONE, /* oplock_level, */
> + SMB2_IMPERSONATION_IMPERSONATION, /* impersonation_level, */
> + SEC_STD_SYNCHRONIZE|
> + SEC_DIR_LIST|
> + SEC_DIR_ADD_SUBDIR, /* desired_access, */
> + 0, /* file_attributes, */
> + FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, /* share_access, */
> + FILE_OPEN, /* create_disposition, */
> + FILE_SYNCHRONOUS_IO_NONALERT|FILE_DIRECTORY_FILE, /* create_options, */
> + NULL, /* smb2_create_blobs *blobs */
> + &fid_persistent,
> + &fid_volatile,
> + NULL, NULL, NULL);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smb2cli_create '%s' (write DIR access) returned %s\n",
> + path,
> + nt_errstr(status));
> + return false;
> + }
> +
> + status = smb2cli_query_directory(
> + cli->conn, cli->timeout, cli->smb2.session, cli->smb2.tcon,
> + 1, 0, 0, fid_persistent, fid_volatile, "*", 0xffff,
> + talloc_tos(), &dir_data, &dir_data_length);
> +
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smb2cli_query_directory returned %s\n", nt_errstr(status));
> + return false;
> + }
> +
> + status = smb2cli_flush(cli->conn, cli->timeout, cli->smb2.session,
> + cli->smb2.tcon, fid_persistent, fid_volatile);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smb2cli_flush on a directory returned %s\n",
> + nt_errstr(status));
> + return false;
> + }
> +
> + status = smb2cli_close(cli->conn, cli->timeout, cli->smb2.session,
> + cli->smb2.tcon, 0, fid_persistent, fid_volatile);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smb2cli_close returned %s\n", nt_errstr(status));
> + return false;
> + }
> +
> +
> + return true;
> +}
> +
> +bool run_smb2_dir_fsync(int dummy)
> +{
> + struct cli_state *cli = NULL;
> + NTSTATUS status;
> + bool bret = false;
> + const char *dname = "fsync_test_dir";
> +
> + printf("Starting SMB2-DIR-FSYNC\n");
> +
> + if (!torture_init_connection(&cli)) {
> + return false;
> + }
> +
> + status = smbXcli_negprot(cli->conn, cli->timeout,
> + PROTOCOL_SMB2_02, PROTOCOL_SMB2_02);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("smbXcli_negprot returned %s\n", nt_errstr(status));
> + return false;
> + }
> +
> + status = cli_session_setup_creds(cli, torture_creds);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("cli_session_setup returned %s\n", nt_errstr(status));
> + return false;
> + }
> +
> + status = cli_tree_connect(cli, share, "?????", NULL);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("cli_tree_connect returned %s\n", nt_errstr(status));
> + return false;
> + }
> +
> + (void)cli_rmdir(cli, dname);
> + status = cli_mkdir(cli, dname);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("cli_mkdir(%s) returned %s\n",
> + dname,
> + nt_errstr(status));
> + return false;
> + }
> +
> + /* Test on a subdirectory. */
> + bret = test_dir_fsync(cli, dname);
> + if (bret == false) {
> + (void)cli_rmdir(cli, dname);
> + return false;
> + }
> + (void)cli_rmdir(cli, dname);
> +
> + /* Test on the root handle of a share. */
> + bret = test_dir_fsync(cli, "");
> + if (bret == false) {
> + return false;
> + }
> + return true;
> +}
> diff --git a/source3/torture/torture.c b/source3/torture/torture.c
> index 56cc6ff3628..c7ada24ea56 100644
> --- a/source3/torture/torture.c
> +++ b/source3/torture/torture.c
> @@ -11595,6 +11595,7 @@ static struct {
> { "SMB2-MULTI-CHANNEL", run_smb2_multi_channel },
> { "SMB2-SESSION-REAUTH", run_smb2_session_reauth },
> { "SMB2-FTRUNCATE", run_smb2_ftruncate },
> + { "SMB2-DIR-FSYNC", run_smb2_dir_fsync },
> { "CLEANUP1", run_cleanup1 },
> { "CLEANUP2", run_cleanup2 },
> { "CLEANUP3", run_cleanup3 },
> --
> 2.17.0.441.gb46fe60e1d-goog
>
More information about the samba-technical
mailing list