[PATCHES] smbd: allow reading files when only FILE_EXECUTE access is granted
Uri Simchoni
uri at samba.org
Sun Aug 14 03:47:57 UTC 2016
Hi,
Attached is a fix to https://bugzilla.samba.org/show_bug.cgi?id=12149.
As it turns out, a Windows SMB2 server (not SMB1) would allow reading
files (SMB2 READ) if FILE_EXECUTE was granted to the handle, even if
FILE_READ_DATA wasn't.
I discussed this with dochelp, and it appears to be a behavior not
documented in [MS-SMB2] - probably the document will be fixed.
One thing I wanted to know when handling this was whether a READ_DATA is
implicitly granted (which affects other operations too) or is it the
READ processing that also allows the read based on FILE_EXECUTE. The
surprising answer from dochelp was tht READ_DATA is implicitly granted.
Doing some more investigations and torture tests against 2012R2 revealed
that the picture is more complicated than that. The experimental results
can be summarized as follows:
Operation Reveals
-------------------------------
READ Succeeds if either READ_DATA or EXECUTE is
requested
COPYCHUNK-SRC Succeeds if either READ_DATA or EXECUTE is
requested
COPYCHUNK-DST Succeeds only if READ_DATA is requested
GETINFO-ALL Does not return READ_DATA in the handle's
access mask
The conceptual model to explain this (based on discussions with dochelp
and the observed bahavior) is as follows:
1. The SMB2 server maintains an Open.GrantedAccess variable as
documented in [MS-SMB2]
2. But an open file also has a file handle, and this handle has its own
granted access mask maintained in the Windows kernel.
3. An SMB2 server would add FILE_READ_DATA to its copy of the mask if
FILE_EXECUTE is granted, but the kernel file object does not have this
FILE_READ_DATA (IOW, this is added AFTER the open in the kernel).
4. In some operations, the SMB2 server is doing the access check and
then calling APIs which bypass security. READ falls under this category,
and hence a READ is allowed because it relates to the SMB2 server's
granted mask
5. In other operations, the SMB2 server simply relays the request to the
kernel. GETINFO is an example - no FILE_READ_DATA here.
6. Still in a third class, there's a mix. In the case of COPYCHUNK, the
SMB2 server is doing the READ_DATA access check on source handle and
WRITE/APPEND_DATA access check on dest handle, and if those pass, it
calls ioctl (or whatever Windows equivalent). The ioctl opcode however,
has bits that say if the handle must be readable, writable, or both - in
COPYCHUNK it has to be both (ever wondered why the dest handle has to be
readable? that's why) - so this check is done within the kernel and
hence FILE_EXECUTE on the dest handle won't buy you COPYCHUNK rights.
The implicit READ_DATA behavior does not exist in SMB1, this is already
covered by existing torture tests.
The fix is composed of a few patches that add the torture tests for
READ, COPYCHUNK, and GETINFO, and then a couple of patches to fix the
issue by reusing the "FLAGS2_READ_PERMIT_EXECUTE" from SMB1. While
reusing a flag from SMB1 may seem like a hack, I do believe the right
solution is to raise a flag on the request by common smb2 code. So if
the SMB1 flag is not acceptable we can have a common flag that's being
raised both by SMB2 and by SMB1 only if SMB1's flags2 has
FLAGS2_READ_PERMIT_EXECUTE.
Review appreciated.
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 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 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 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 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 9d0f89a6d45b82fbb6fe4023a11ea0ef5880dec8 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 11 Aug 2016 08:20:55 +0300
Subject: [PATCH 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, 13 insertions(+), 7 deletions(-)
diff --git a/source4/torture/smb2/ioctl.c b/source4/torture/smb2/ioctl.c
index 8e7f69a..a95ef28 100644
--- a/source4/torture/smb2/ioctl.c
+++ b/source4/torture/smb2/ioctl.c
@@ -273,20 +273,26 @@ static bool test_setup_create_fill(struct torture_context *torture,
uint32_t file_attributes)
{
bool ok;
+ struct smb2_handle create_handle;
smb2_util_unlink(tree, fname);
- ok = test_setup_open(torture, tree, mem_ctx,
- fname,
- fh,
- desired_access,
- file_attributes);
- torture_assert(torture, ok, "file open");
+ ok = test_setup_open(torture, tree, mem_ctx, fname, &create_handle,
+ SEC_RIGHTS_FILE_WRITE, file_attributes);
+ torture_assert(torture, ok, "file create");
if (size > 0) {
- ok = write_pattern(torture, tree, mem_ctx, *fh, 0, size, 0);
+ ok = write_pattern(torture, tree, mem_ctx, create_handle, 0,
+ size, 0);
torture_assert(torture, ok, "write pattern");
}
+
+ smb2_util_close(tree, create_handle);
+
+ 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 3aeb7a2f826ad6d3bf91b88cf08e0fca919b0720 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 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 a95ef28..ec926bd 100644
--- a/source4/torture/smb2/ioctl.c
+++ b/source4/torture/smb2/ioctl.c
@@ -1245,16 +1245,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");
}
@@ -1278,15 +1328,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");
}
@@ -1310,15 +1359,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 e7d6c28153d799fbc8130be79a211fe5080004ea 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 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 2feb8c817ee2c461bb68a4d6c6c258ef39cdf5e3 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 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