[PATCHES] smbd: allow reading files when only FILE_EXECUTE access is granted
Uri Simchoni
uri at samba.org
Tue Aug 16 08:02:04 UTC 2016
On 08/15/2016 09:54 PM, Uri Simchoni wrote:
> On 08/15/2016 06:37 PM, David Disseldorp wrote:
>> Hi Uri,
>>
>> Thanks for your thorough investigation here. The changes look sane, but
>> samba3.smb2.ioctl.compress_perms appears to be broken by
>> seltest: allow opening files with arbitrary rights in smb2.ioctl tests:
>> UNEXPECTED(failure): samba3.smb2.ioctl fs_specific.compress_perms(nt4_dc)
>> REASON: Exception: Exception: ../source4/torture/smb2/ioctl.c:2486: status was NT_STATUS_INVALID_PARAMETER, expected NT_STATUS_OK: FSCTL_GET_COMPRESSION
>>
>> Cheers, David
>>
> Oh, I see... I need btrfs to reproduce the failure (or, probably,
> Windows :)). Thanks for spotting this - it would have gone unnoticed on
> sn-devel!
>
> Uri
>
>
>
Here's an updated patch set which passes with btrfs. Only the "allow
opening files with arbitrary rights in smb2.ioctl tests" has been
modified, to be more conservative and not change test behavior for
previous tests (a single open if possible).
The smb2.ioctl test mostly passes against an 2012R2 machine with NTFS -
1. The "copy_chunk_src_exceed_multi" fails with
"../source4/torture/smb2/ioctl.c:1531: bad copy chunk response data"
2. The "trim_simple" fails with "../source4/torture/smb2/ioctl.c:4756:
trim not supported". This is before and after the patch.
The compress_perms test passes with both patch versions (whereas with
btrfs it fails with the first version and passes with the second).
Thanks,
Uri.
-------------- next part --------------
From 7d6e561a9bfcd41db6d3e9cecfec221c33aee45a Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 4 Aug 2016 12:59:38 +0300
Subject: [PATCH v2 1/8] s4-smbtorture: use standard macros in smb2.read test
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12149
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source4/torture/smb2/read.c | 22 +++++++---------------
1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/source4/torture/smb2/read.c b/source4/torture/smb2/read.c
index 3600765..c1105a9 100644
--- a/source4/torture/smb2/read.c
+++ b/source4/torture/smb2/read.c
@@ -27,21 +27,13 @@
#include "torture/smb2/proto.h"
-#define CHECK_STATUS(status, correct) do { \
- if (!NT_STATUS_EQUAL(status, correct)) { \
- printf("(%s) Incorrect status %s - should be %s\n", \
- __location__, nt_errstr(status), nt_errstr(correct)); \
- ret = false; \
- goto done; \
- }} while (0)
-
-#define CHECK_VALUE(v, correct) do { \
- if ((v) != (correct)) { \
- printf("(%s) Incorrect value %s=%u - should be %u\n", \
- __location__, #v, (unsigned)v, (unsigned)correct); \
- ret = false; \
- goto done; \
- }} while (0)
+#define CHECK_STATUS(_status, _expected) \
+ torture_assert_ntstatus_equal_goto(torture, _status, _expected, \
+ ret, done, "Incorrect status")
+
+#define CHECK_VALUE(v, correct) \
+ torture_assert_int_equal_goto(torture, v, correct, \
+ ret, done, "Incorrect value")
#define FNAME "smb2_readtest.dat"
#define DNAME "smb2_readtest.dir"
--
2.5.5
From 3cd0d2af3367e21a8f885f194ff60fc5a570b035 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Sun, 31 Jul 2016 14:26:24 +0300
Subject: [PATCH v2 2/8] s4-selftest: add functions which create with desired
access
Add functions which create a file or a directory with
specific desired access.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12149
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source4/torture/smb2/util.c | 36 +++++++++++++++++++++++++++++-------
1 file changed, 29 insertions(+), 7 deletions(-)
diff --git a/source4/torture/smb2/util.c b/source4/torture/smb2/util.c
index 814e398..c9d47ae 100644
--- a/source4/torture/smb2/util.c
+++ b/source4/torture/smb2/util.c
@@ -428,19 +428,20 @@ bool torture_smb2_con_sopt(struct torture_context *tctx,
return true;
}
-
/*
create and return a handle to a test file
+ with a specific access mask
*/
-NTSTATUS torture_smb2_testfile(struct smb2_tree *tree, const char *fname,
- struct smb2_handle *handle)
+NTSTATUS torture_smb2_testfile_access(struct smb2_tree *tree, const char *fname,
+ struct smb2_handle *handle,
+ uint32_t desired_access)
{
struct smb2_create io;
NTSTATUS status;
ZERO_STRUCT(io);
io.in.oplock_level = 0;
- io.in.desired_access = SEC_RIGHTS_FILE_ALL;
+ io.in.desired_access = desired_access;
io.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
io.in.create_disposition = NTCREATEX_DISP_OPEN_IF;
io.in.share_access =
@@ -459,17 +460,29 @@ NTSTATUS torture_smb2_testfile(struct smb2_tree *tree, const char *fname,
}
/*
+ create and return a handle to a test file
+*/
+NTSTATUS torture_smb2_testfile(struct smb2_tree *tree, const char *fname,
+ struct smb2_handle *handle)
+{
+ return torture_smb2_testfile_access(tree, fname, handle,
+ SEC_RIGHTS_FILE_ALL);
+}
+
+/*
create and return a handle to a test directory
+ with specific desired access
*/
-NTSTATUS torture_smb2_testdir(struct smb2_tree *tree, const char *fname,
- struct smb2_handle *handle)
+NTSTATUS torture_smb2_testdir_access(struct smb2_tree *tree, const char *fname,
+ struct smb2_handle *handle,
+ uint32_t desired_access)
{
struct smb2_create io;
NTSTATUS status;
ZERO_STRUCT(io);
io.in.oplock_level = 0;
- io.in.desired_access = SEC_RIGHTS_DIR_ALL;
+ io.in.desired_access = desired_access;
io.in.file_attributes = FILE_ATTRIBUTE_DIRECTORY;
io.in.create_disposition = NTCREATEX_DISP_OPEN_IF;
io.in.share_access = NTCREATEX_SHARE_ACCESS_READ|NTCREATEX_SHARE_ACCESS_WRITE|NTCREATEX_SHARE_ACCESS_DELETE;
@@ -484,6 +497,15 @@ NTSTATUS torture_smb2_testdir(struct smb2_tree *tree, const char *fname,
return NT_STATUS_OK;
}
+/*
+ create and return a handle to a test directory
+*/
+NTSTATUS torture_smb2_testdir(struct smb2_tree *tree, const char *fname,
+ struct smb2_handle *handle)
+{
+ return torture_smb2_testdir_access(tree, fname, handle,
+ SEC_RIGHTS_DIR_ALL);
+}
/*
create a complex file using SMB2, to make it easier to
--
2.5.5
From 0aaf444862a9536920b833e75c732b8d1b731e34 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Sun, 31 Jul 2016 14:29:37 +0300
Subject: [PATCH v2 3/8] s4-selftest: add test for read access check
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12149
Signed-off-by: Uri Simchoni <uri at samba.org>
---
selftest/knownfail | 3 ++
source4/torture/smb2/read.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 77 insertions(+)
diff --git a/selftest/knownfail b/selftest/knownfail
index 397e53c..892a5da 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -286,3 +286,6 @@
^samba4.krb5.kdc.*as-req-aes.*fl2000dc
# nt4_member and ad_member don't support ntlmv1
^samba3.blackbox.smbclient_auth.plain.*_member.*option=clientntlmv2auth=no.member.creds.*as.user
+#new read tests fail
+^samba4.smb2.read.access
+^samba3.smb2.read.access
diff --git a/source4/torture/smb2/read.c b/source4/torture/smb2/read.c
index c1105a9..c4469df 100644
--- a/source4/torture/smb2/read.c
+++ b/source4/torture/smb2/read.c
@@ -226,6 +226,79 @@ done:
return ret;
}
+static bool test_read_access(struct torture_context *torture,
+ struct smb2_tree *tree)
+{
+ bool ret = true;
+ NTSTATUS status;
+ struct smb2_handle h;
+ uint8_t buf[64 * 1024];
+ struct smb2_read rd;
+ TALLOC_CTX *tmp_ctx = talloc_new(tree);
+
+ ZERO_STRUCT(buf);
+
+ /* create a file */
+ smb2_util_unlink(tree, FNAME);
+
+ status = torture_smb2_testfile(tree, FNAME, &h);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ status = smb2_util_write(tree, h, buf, 0, ARRAY_SIZE(buf));
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ status = smb2_util_close(tree, h);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ /* open w/ READ access - success */
+ status = torture_smb2_testfile_access(
+ tree, FNAME, &h, SEC_FILE_READ_ATTRIBUTE | SEC_FILE_READ_DATA);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ ZERO_STRUCT(rd);
+ rd.in.file.handle = h;
+ rd.in.length = 5;
+ rd.in.offset = 0;
+ status = smb2_read(tree, tree, &rd);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ status = smb2_util_close(tree, h);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ /* open w/ EXECUTE access - success */
+ status = torture_smb2_testfile_access(
+ tree, FNAME, &h, SEC_FILE_READ_ATTRIBUTE | SEC_FILE_EXECUTE);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ ZERO_STRUCT(rd);
+ rd.in.file.handle = h;
+ rd.in.length = 5;
+ rd.in.offset = 0;
+ status = smb2_read(tree, tree, &rd);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ status = smb2_util_close(tree, h);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ /* open without READ or EXECUTE access - access denied */
+ status = torture_smb2_testfile_access(tree, FNAME, &h,
+ SEC_FILE_READ_ATTRIBUTE);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ ZERO_STRUCT(rd);
+ rd.in.file.handle = h;
+ rd.in.length = 5;
+ rd.in.offset = 0;
+ status = smb2_read(tree, tree, &rd);
+ CHECK_STATUS(status, NT_STATUS_ACCESS_DENIED);
+
+ status = smb2_util_close(tree, h);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+done:
+ talloc_free(tmp_ctx);
+ return ret;
+}
/*
basic testing of SMB2 read
@@ -237,6 +310,7 @@ struct torture_suite *torture_smb2_read_init(void)
torture_suite_add_1smb2_test(suite, "eof", test_read_eof);
torture_suite_add_1smb2_test(suite, "position", test_read_position);
torture_suite_add_1smb2_test(suite, "dir", test_read_dir);
+ torture_suite_add_1smb2_test(suite, "access", test_read_access);
suite->description = talloc_strdup(suite, "SMB2-READ tests");
--
2.5.5
From c37a333be09c0a7c68e43b6b6ea4df04ec02585d Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Sat, 13 Aug 2016 21:23:34 +0300
Subject: [PATCH v2 4/8] seltest: implicit FILE_READ_DATA non-reporting
This test (passes against Windows Server 2012R2) shows
that the implicit FILE_READ_DATA that is added whenever
FILE_EXECUTE is granted, is not reported back when querying
the handle.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12149
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source4/torture/smb2/getinfo.c | 45 ++++++++++++++++++++++++++++++++++++++++++
source4/torture/smb2/util.c | 27 +++++++++++++++++++++++++
2 files changed, 72 insertions(+)
diff --git a/source4/torture/smb2/getinfo.c b/source4/torture/smb2/getinfo.c
index 4bf4100..82eda75 100644
--- a/source4/torture/smb2/getinfo.c
+++ b/source4/torture/smb2/getinfo.c
@@ -126,6 +126,49 @@ static bool torture_smb2_fileinfo(struct torture_context *tctx, struct smb2_tree
return true;
}
+/*
+ test granted access when desired access includes
+ FILE_EXECUTE and does not include FILE_READ_DATA
+*/
+static bool torture_smb2_fileinfo_grant_read(struct torture_context *tctx)
+{
+ struct smb2_tree *tree;
+ bool ret;
+ struct smb2_handle hfile, hdir;
+ NTSTATUS status;
+ uint32_t file_granted_access, dir_granted_access;
+
+ ret = torture_smb2_connection(tctx, &tree);
+ torture_assert(tctx, ret, "connection failed");
+
+ status = torture_smb2_testfile_access(
+ tree, FNAME, &hfile, SEC_FILE_EXECUTE | SEC_FILE_READ_ATTRIBUTE);
+ torture_assert_ntstatus_ok(tctx, status,
+ "Unable to create test file " FNAME "\n");
+ status =
+ torture_smb2_get_allinfo_access(tree, hfile, &file_granted_access);
+ torture_assert_ntstatus_ok(tctx, status,
+ "Unable to query test file access ");
+ torture_assert_int_equal(tctx, file_granted_access,
+ SEC_FILE_EXECUTE | SEC_FILE_READ_ATTRIBUTE,
+ "granted file access ");
+ smb2_util_close(tree, hfile);
+
+ status = torture_smb2_testdir_access(
+ tree, DNAME, &hdir, SEC_FILE_EXECUTE | SEC_FILE_READ_ATTRIBUTE);
+ torture_assert_ntstatus_ok(tctx, status,
+ "Unable to create test dir " DNAME "\n");
+ status =
+ torture_smb2_get_allinfo_access(tree, hdir, &dir_granted_access);
+ torture_assert_ntstatus_ok(tctx, status,
+ "Unable to query test dir access ");
+ torture_assert_int_equal(tctx, dir_granted_access,
+ SEC_FILE_EXECUTE | SEC_FILE_READ_ATTRIBUTE,
+ "granted dir access ");
+ smb2_util_close(tree, hdir);
+
+ return true;
+}
/*
test fsinfo levels
@@ -444,5 +487,7 @@ struct torture_suite *torture_smb2_getinfo_init(void)
torture_smb2_qfile_buffercheck);
torture_suite_add_simple_test(suite, "qsec_buffercheck",
torture_smb2_qsec_buffercheck);
+ torture_suite_add_simple_test(suite, "granted",
+ torture_smb2_fileinfo_grant_read);
return suite;
}
diff --git a/source4/torture/smb2/util.c b/source4/torture/smb2/util.c
index c9d47ae..d0fc695 100644
--- a/source4/torture/smb2/util.c
+++ b/source4/torture/smb2/util.c
@@ -261,6 +261,33 @@ void torture_smb2_all_info(struct smb2_tree *tree, struct smb2_handle handle)
talloc_free(tmp_ctx);
}
+/*
+ get granted access of a file handle
+*/
+NTSTATUS torture_smb2_get_allinfo_access(struct smb2_tree *tree,
+ struct smb2_handle handle,
+ uint32_t *granted_access)
+{
+ NTSTATUS status;
+ TALLOC_CTX *tmp_ctx = talloc_new(tree);
+ union smb_fileinfo io;
+
+ io.generic.level = RAW_FILEINFO_SMB2_ALL_INFORMATION;
+ io.generic.in.file.handle = handle;
+
+ status = smb2_getinfo_file(tree, tmp_ctx, &io);
+ if (!NT_STATUS_IS_OK(status)) {
+ DEBUG(0, ("getinfo failed - %s\n", nt_errstr(status)));
+ goto out;
+ }
+
+ *granted_access = io.all_info2.out.access_mask;
+
+out:
+ talloc_free(tmp_ctx);
+ return status;
+}
+
/**
* open a smb2 tree connect
*/
--
2.5.5
From a1d43a96a3848e79b8e3091458fc62a44fe2edf4 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Mon, 15 Aug 2016 23:39:50 +0300
Subject: [PATCH v2 5/8] seltest: allow opening files with arbitrary rights in
smb2.ioctl tests
Separate file creation (which requires write access) from the
opening of the file for the test (which might be without write
access).
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12149
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source4/torture/smb2/ioctl.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/source4/torture/smb2/ioctl.c b/source4/torture/smb2/ioctl.c
index 8e7f69a..0aadca2 100644
--- a/source4/torture/smb2/ioctl.c
+++ b/source4/torture/smb2/ioctl.c
@@ -273,20 +273,36 @@ static bool test_setup_create_fill(struct torture_context *torture,
uint32_t file_attributes)
{
bool ok;
+ uint32_t initial_access = desired_access;
+
+ if (size > 0) {
+ initial_access |= SEC_FILE_APPEND_DATA;
+ }
smb2_util_unlink(tree, fname);
ok = test_setup_open(torture, tree, mem_ctx,
fname,
fh,
- desired_access,
+ initial_access,
file_attributes);
- torture_assert(torture, ok, "file open");
+ torture_assert(torture, ok, "file create");
if (size > 0) {
ok = write_pattern(torture, tree, mem_ctx, *fh, 0, size, 0);
torture_assert(torture, ok, "write pattern");
}
+
+ if (initial_access != desired_access) {
+ smb2_util_close(tree, *fh);
+ ok = test_setup_open(torture, tree, mem_ctx,
+ fname,
+ fh,
+ desired_access,
+ file_attributes);
+ torture_assert(torture, ok, "file open");
+ }
+
return true;
}
--
2.5.5
From a98e692cd444a60443797fd7e2aeb96e0aced4db Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 4 Aug 2016 13:12:58 +0300
Subject: [PATCH v2 6/8] s4-smbtorture: pin copychunk exec right behavior
Add tests that show copychunk behavior when the
source and dest handles have execute right instead
of read-data right.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12149
Signed-off-by: Uri Simchoni <uri at samba.org>
---
selftest/knownfail | 4 ++
source4/torture/smb2/ioctl.c | 96 ++++++++++++++++++++++++++++++++------------
2 files changed, 75 insertions(+), 25 deletions(-)
diff --git a/selftest/knownfail b/selftest/knownfail
index 892a5da..41cad44 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -289,3 +289,7 @@
#new read tests fail
^samba4.smb2.read.access
^samba3.smb2.read.access
+#new copychunk tests fail
+^samba4.smb2.ioctl.copy_chunk_bad_access
+^samba3.smb2.ioctl.copy_chunk_bad_access
+^samba3.smb2.ioctl fs_specific.copy_chunk_bad_access
diff --git a/source4/torture/smb2/ioctl.c b/source4/torture/smb2/ioctl.c
index 0aadca2..0aa3714 100644
--- a/source4/torture/smb2/ioctl.c
+++ b/source4/torture/smb2/ioctl.c
@@ -1255,16 +1255,66 @@ static bool test_ioctl_copy_chunk_bad_access(struct torture_context *torture,
struct srv_copychunk_copy cc_copy;
enum ndr_err_code ndr_ret;
bool ok;
+ /* read permission on src */
+ ok = test_setup_copy_chunk(torture, tree, tmp_ctx, 1, /* 1 chunk */
+ &src_h, 4096, /* fill 4096 byte src file */
+ SEC_FILE_READ_DATA | SEC_FILE_READ_ATTRIBUTE,
+ &dest_h, 0, /* 0 byte dest file */
+ SEC_RIGHTS_FILE_ALL, &cc_copy, &ioctl);
+ if (!ok) {
+ torture_fail(torture, "setup copy chunk error");
+ }
- /* no read permission on src */
- ok = test_setup_copy_chunk(torture, tree, tmp_ctx,
- 1, /* 1 chunk */
+ cc_copy.chunks[0].source_off = 0;
+ cc_copy.chunks[0].target_off = 0;
+ cc_copy.chunks[0].length = 4096;
+
+ ndr_ret = ndr_push_struct_blob(
+ &ioctl.smb2.in.out, tmp_ctx, &cc_copy,
+ (ndr_push_flags_fn_t)ndr_push_srv_copychunk_copy);
+ torture_assert_ndr_success(torture, ndr_ret,
+ "ndr_push_srv_copychunk_copy");
+
+ status = smb2_ioctl(tree, tmp_ctx, &ioctl.smb2);
+ torture_assert_ntstatus_equal(torture, status, NT_STATUS_OK,
+ "FSCTL_SRV_COPYCHUNK");
+
+ smb2_util_close(tree, src_h);
+ smb2_util_close(tree, dest_h);
+
+ /* execute permission on src */
+ ok = test_setup_copy_chunk(torture, tree, tmp_ctx, 1, /* 1 chunk */
&src_h, 4096, /* fill 4096 byte src file */
- SEC_RIGHTS_FILE_WRITE,
- &dest_h, 0, /* 0 byte dest file */
- SEC_RIGHTS_FILE_ALL,
- &cc_copy,
- &ioctl);
+ SEC_FILE_EXECUTE | SEC_FILE_READ_ATTRIBUTE,
+ &dest_h, 0, /* 0 byte dest file */
+ SEC_RIGHTS_FILE_ALL, &cc_copy, &ioctl);
+ if (!ok) {
+ torture_fail(torture, "setup copy chunk error");
+ }
+
+ cc_copy.chunks[0].source_off = 0;
+ cc_copy.chunks[0].target_off = 0;
+ cc_copy.chunks[0].length = 4096;
+
+ ndr_ret = ndr_push_struct_blob(
+ &ioctl.smb2.in.out, tmp_ctx, &cc_copy,
+ (ndr_push_flags_fn_t)ndr_push_srv_copychunk_copy);
+ torture_assert_ndr_success(torture, ndr_ret,
+ "ndr_push_srv_copychunk_copy");
+
+ status = smb2_ioctl(tree, tmp_ctx, &ioctl.smb2);
+ torture_assert_ntstatus_equal(torture, status, NT_STATUS_OK,
+ "FSCTL_SRV_COPYCHUNK");
+
+ smb2_util_close(tree, src_h);
+ smb2_util_close(tree, dest_h);
+
+ /* neither read nor execute permission on src */
+ ok = test_setup_copy_chunk(torture, tree, tmp_ctx, 1, /* 1 chunk */
+ &src_h, 4096, /* fill 4096 byte src file */
+ SEC_FILE_READ_ATTRIBUTE, &dest_h,
+ 0, /* 0 byte dest file */
+ SEC_RIGHTS_FILE_ALL, &cc_copy, &ioctl);
if (!ok) {
torture_fail(torture, "setup copy chunk error");
}
@@ -1288,15 +1338,14 @@ static bool test_ioctl_copy_chunk_bad_access(struct torture_context *torture,
smb2_util_close(tree, dest_h);
/* no write permission on dest */
- ok = test_setup_copy_chunk(torture, tree, tmp_ctx,
- 1, /* 1 chunk */
- &src_h, 4096, /* fill 4096 byte src file */
- SEC_RIGHTS_FILE_ALL,
- &dest_h, 0, /* 0 byte dest file */
- (SEC_RIGHTS_FILE_READ
- | SEC_RIGHTS_FILE_EXECUTE),
- &cc_copy,
- &ioctl);
+ ok = test_setup_copy_chunk(
+ torture, tree, tmp_ctx, 1, /* 1 chunk */
+ &src_h, 4096, /* fill 4096 byte src file */
+ SEC_FILE_READ_DATA | SEC_FILE_READ_ATTRIBUTE, &dest_h,
+ 0, /* 0 byte dest file */
+ (SEC_RIGHTS_FILE_ALL &
+ ~(SEC_FILE_WRITE_DATA | SEC_FILE_APPEND_DATA)),
+ &cc_copy, &ioctl);
if (!ok) {
torture_fail(torture, "setup copy chunk error");
}
@@ -1320,15 +1369,12 @@ static bool test_ioctl_copy_chunk_bad_access(struct torture_context *torture,
smb2_util_close(tree, dest_h);
/* no read permission on dest */
- ok = test_setup_copy_chunk(torture, tree, tmp_ctx,
- 1, /* 1 chunk */
+ ok = test_setup_copy_chunk(torture, tree, tmp_ctx, 1, /* 1 chunk */
&src_h, 4096, /* fill 4096 byte src file */
- SEC_RIGHTS_FILE_ALL,
- &dest_h, 0, /* 0 byte dest file */
- (SEC_RIGHTS_FILE_WRITE
- | SEC_RIGHTS_FILE_EXECUTE),
- &cc_copy,
- &ioctl);
+ SEC_FILE_READ_DATA | SEC_FILE_READ_ATTRIBUTE,
+ &dest_h, 0, /* 0 byte dest file */
+ (SEC_RIGHTS_FILE_ALL & ~SEC_FILE_READ_DATA),
+ &cc_copy, &ioctl);
if (!ok) {
torture_fail(torture, "setup copy chunk error");
}
--
2.5.5
From 75dfad6e2d87cb07927349b452d4a98db766de14 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Sat, 13 Aug 2016 00:19:33 +0300
Subject: [PATCH v2 7/8] smbd: look only at handle readability for COPYCHUNK
dest
This commits sets the stage for a change of behavior
in a later commit.
When checking FILE_READ_DATA on the COPYCHUNK dest handle,
only check the handle readability and not the extra right
that may have been added due to the FILE_EXECUTE right.
The check for FILE_READ_DATA always seemed strange for the
dest handle, which is not read. It turns out that in Windows,
this check is not done at the SMB layer, but at a lower layer
that processes the IOCTL request - the IOCTL code has bits
that specify what type of access check needs to be done.
Therefore, this lower layer is unaware of the SMB layer's
practice of granting READ access based on the FILE_EXECUTE
right, and it only checks the handle's readability.
This subtle difference has observable behavior - the
COPYCHUNK source handle can have FILE_EXECUTE right instead
of FILE_READ_DATA, but the dest handle cannot.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12149
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/include/smb_macros.h | 8 ++++++++
source3/smbd/smb2_ioctl_network_fs.c | 4 ++--
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/source3/include/smb_macros.h b/source3/include/smb_macros.h
index 42a9756..f8656c7 100644
--- a/source3/include/smb_macros.h
+++ b/source3/include/smb_macros.h
@@ -56,6 +56,14 @@
((req->flags2 & FLAGS2_READ_PERMIT_EXECUTE) && \
(fsp->access_mask & FILE_EXECUTE))))
+/* An IOCTL readability check (validating read access
+ * when the IOCTL code requires it)
+ * http://social.technet.microsoft.com/wiki/contents/articles/24653.decoding-io-control-codes-ioctl-fsctl-and-deviceiocodes-with-table-of-known-values.aspx
+ * ). On Windows servers, this is done by the IO manager, which is unaware of
+ * the "if execute is granted then also grant read" arrangement.
+ */
+#define CHECK_READ_IOCTL(fsp, req) (((fsp)->fh->fd != -1) && ((fsp)->can_read))
+
#define CHECK_WRITE(fsp) ((fsp)->can_write && ((fsp)->fh->fd != -1))
#define ERROR_WAS_LOCK_DENIED(status) (NT_STATUS_EQUAL((status), NT_STATUS_LOCK_NOT_GRANTED) || \
diff --git a/source3/smbd/smb2_ioctl_network_fs.c b/source3/smbd/smb2_ioctl_network_fs.c
index d8590de..c2b889b 100644
--- a/source3/smbd/smb2_ioctl_network_fs.c
+++ b/source3/smbd/smb2_ioctl_network_fs.c
@@ -117,8 +117,8 @@ static NTSTATUS copychunk_check_handles(uint32_t ctl_code,
* - The Open.GrantedAccess of the destination file does not include
* FILE_READ_DATA, and the CtlCode is FSCTL_SRV_COPYCHUNK.
*/
- if ((ctl_code == FSCTL_SRV_COPYCHUNK)
- && !CHECK_READ(dst_fsp, smb1req)) {
+ if ((ctl_code == FSCTL_SRV_COPYCHUNK) &&
+ !CHECK_READ_IOCTL(dst_fsp, smb1req)) {
DEBUG(5, ("copy chunk no read on dest handle (%s).\n",
smb_fname_str_dbg(dst_fsp->fsp_name) ));
return NT_STATUS_ACCESS_DENIED;
--
2.5.5
From f3734153779c722bac82e4ebda9a47f71260a467 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 4 Aug 2016 14:59:23 +0300
Subject: [PATCH v2 8/8] smbd: allow reading files based on FILE_EXECUTE access
right
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12149
Signed-off-by: Uri Simchoni <uri at samba.org>
---
selftest/knownfail | 7 ++-----
source3/smbd/smb2_glue.c | 16 ++++++++++++++++
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/selftest/knownfail b/selftest/knownfail
index 41cad44..ffcaf06 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -286,10 +286,7 @@
^samba4.krb5.kdc.*as-req-aes.*fl2000dc
# nt4_member and ad_member don't support ntlmv1
^samba3.blackbox.smbclient_auth.plain.*_member.*option=clientntlmv2auth=no.member.creds.*as.user
-#new read tests fail
+#nt-vfs server blocks read with execute access
^samba4.smb2.read.access
-^samba3.smb2.read.access
-#new copychunk tests fail
+#ntvfs server blocks copychunk with execute access on read handle
^samba4.smb2.ioctl.copy_chunk_bad_access
-^samba3.smb2.ioctl.copy_chunk_bad_access
-^samba3.smb2.ioctl fs_specific.copy_chunk_bad_access
diff --git a/source3/smbd/smb2_glue.c b/source3/smbd/smb2_glue.c
index b41775d..0bb34be 100644
--- a/source3/smbd/smb2_glue.c
+++ b/source3/smbd/smb2_glue.c
@@ -48,6 +48,22 @@ struct smb_request *smbd_smb2_fake_smb_request(struct smbd_smb2_request *req)
FLAGS2_32_BIT_ERROR_CODES |
FLAGS2_LONG_PATH_COMPONENTS |
FLAGS2_IS_LONG_NAME;
+
+ /* This is not documented in revision 49 of [MS-SMB2] but should be
+ * added in a later revision (and torture test smb2.read.access
+ * as well as smb2.ioctl_copy_chunk_bad_access against
+ * Server 2012R2 confirms this)
+ *
+ * If FILE_EXECUTE is granted to a handle then the SMB2 server
+ * acts as if FILE_READ_DATA has also been granted. We must still
+ * keep the original granted mask, because with ioctl requests,
+ * access checks are made on the file handle, "below" the SMB2
+ * server, and the object store below the SMB layer is not aware
+ * of this arrangement (see smb2.ioctl.copy_chunk_bad_access
+ * torture test).
+ */
+ smbreq->flags2 |= FLAGS2_READ_PERMIT_EXECUTE;
+
if (IVAL(inhdr, SMB2_HDR_FLAGS) & SMB2_HDR_FLAG_DFS) {
smbreq->flags2 |= FLAGS2_DFS_PATHNAMES;
}
--
2.5.5
More information about the samba-technical
mailing list