From c96021ea68ae8e4ce0be0ebb325833ff3262aa55 Mon Sep 17 00:00:00 2001 From: Uri Simchoni 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 Reviewed-by: David Disseldorp (cherry picked from commit 20b9a5bd74fafbca4b7cc7952c27033edcf0eeb8) --- 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 9d6909793a8988ddd35540ba194a0b58c5f1ee58 Mon Sep 17 00:00:00 2001 From: Uri Simchoni 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 Reviewed-by: David Disseldorp (cherry picked from commit 1b06acafa4e9ea91a50e5ed85da881187057da6e) --- 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 8e26a8f792b021accc0bd22255707fd114140184 Mon Sep 17 00:00:00 2001 From: Uri Simchoni 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 Reviewed-by: David Disseldorp (backported from commit 55a9d35cabaea6e98211fc058b788cedf9b7b22a) --- selftest/knownfail | 3 ++ source4/torture/smb2/read.c | 74 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/selftest/knownfail b/selftest/knownfail index 0973e06..e83b085 100644 --- a/selftest/knownfail +++ b/selftest/knownfail @@ -340,3 +340,6 @@ # we don't allow auth_level_connect anymore... # ^samba3.blackbox.rpcclient.*ncacn_np.*with.*connect.*rpcclient # we don't allow auth_level_connect anymore +#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 80eb41778fa2dd60b19604d4108cded3c305fe3f Mon Sep 17 00:00:00 2001 From: Uri Simchoni 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 Reviewed-by: David Disseldorp (cherry picked from commit 7dc9f582066d500bf57000891560610e8d2e208c) --- 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 b5c2a74b15a78c5cbb7eea18eda118b2acf64899 Mon Sep 17 00:00:00 2001 From: Uri Simchoni Date: Mon, 15 Aug 2016 23:39:50 +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 Reviewed-by: David Disseldorp (cherry picked from commit 6ce0304eda4b464972defcecd591fab03428bd03) --- 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 9ff068afd898ac40436f23a9d2dbc99c879fa532 Mon Sep 17 00:00:00 2001 From: Uri Simchoni 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 Reviewed-by: David Disseldorp (cherry picked from commit 5bf11f6f5b4dab4cba4b00674bcb76138fb55974) --- 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 e83b085..3e70adb 100644 --- a/selftest/knownfail +++ b/selftest/knownfail @@ -343,3 +343,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 0d0339bbadfbacd4929da151198d971de64d2895 Mon Sep 17 00:00:00 2001 From: Uri Simchoni 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 Reviewed-by: David Disseldorp Autobuild-User(master): David Disseldorp Autobuild-Date(master): Tue Aug 16 15:21:03 CEST 2016 on sn-devel-144 (cherry picked from commit 3e42b69d5e1216b6af570a09d58040d281bbbf17) --- 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 01798ac..9051bed 100644 --- a/source3/smbd/smb2_ioctl_network_fs.c +++ b/source3/smbd/smb2_ioctl_network_fs.c @@ -116,8 +116,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 5b5ff00e61d3370a640bd5587a982f964da643ee Mon Sep 17 00:00:00 2001 From: Uri Simchoni 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 Reviewed-by: David Disseldorp Autobuild-User(master): David Disseldorp Autobuild-Date(master): Thu Aug 18 18:58:22 CEST 2016 on sn-devel-144 (backported from commit a6073e6130d39dac58f1e6ea9f41ec4ab34c3e29) --- 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 3e70adb..40ac696 100644 --- a/selftest/knownfail +++ b/selftest/knownfail @@ -340,10 +340,7 @@ # we don't allow auth_level_connect anymore... # ^samba3.blackbox.rpcclient.*ncacn_np.*with.*connect.*rpcclient # we don't allow auth_level_connect anymore -#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