From d2cc851750dab6ea4761139b7cb7edf96acdc532 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sun, 14 Aug 2022 18:51:30 +0200 Subject: [PATCH 1/3] s4/libcli/smb2: avoid using smb2_composite_setpathinfo() in smb2_util_setatr() smb2_composite_setpathinfo() uses SEC_FLAG_MAXIMUM_ALLOWED which can have unwanted side effects like breaking oplocks if the effective access includes [READ|WRITE]_DATA. For changing the DOS attributes we only need SEC_FILE_WRITE_ATTRIBUTE. With this change test_smb2_oplock_batch25() doesn't trigger an oplock break anymore. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15153 Signed-off-by: Ralph Boehme Reviewed-by: Stefan Metzmacher (cherry picked from commit 66e40690bdd41800a01333ce4243bd62ee2b1894) --- source4/libcli/smb2/util.c | 37 ++++++++++++++++++++++++++++------- source4/torture/smb2/oplock.c | 10 ++-------- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/source4/libcli/smb2/util.c b/source4/libcli/smb2/util.c index b2efabb5a443..f86a149c646a 100644 --- a/source4/libcli/smb2/util.c +++ b/source4/libcli/smb2/util.c @@ -88,14 +88,37 @@ NTSTATUS smb2_util_mkdir(struct smb2_tree *tree, const char *dname) */ NTSTATUS smb2_util_setatr(struct smb2_tree *tree, const char *name, uint32_t attrib) { - union smb_setfileinfo io; - - ZERO_STRUCT(io); - io.basic_info.level = RAW_SFILEINFO_BASIC_INFORMATION; - io.basic_info.in.file.path = name; - io.basic_info.in.attrib = attrib; + struct smb2_create cr = {0}; + struct smb2_handle h1 = {{0}}; + union smb_setfileinfo setinfo; + NTSTATUS status; + + cr = (struct smb2_create) { + .in.desired_access = SEC_FILE_WRITE_ATTRIBUTE, + .in.share_access = NTCREATEX_SHARE_ACCESS_MASK, + .in.create_disposition = FILE_OPEN, + .in.fname = name, + }; + status = smb2_create(tree, tree, &cr); + if (!NT_STATUS_IS_OK(status)) { + return status; + } + h1 = cr.out.file.handle; - return smb2_composite_setpathinfo(tree, &io); + setinfo = (union smb_setfileinfo) { + .basic_info.level = RAW_SFILEINFO_BASIC_INFORMATION, + .basic_info.in.file.handle = h1, + .basic_info.in.attrib = attrib, + }; + + status = smb2_setinfo_file(tree, &setinfo); + if (!NT_STATUS_IS_OK(status)) { + smb2_util_close(tree, h1); + return status; + } + + smb2_util_close(tree, h1); + return NT_STATUS_OK; } diff --git a/source4/torture/smb2/oplock.c b/source4/torture/smb2/oplock.c index 6bf7567ddb1a..8f275bafcdb7 100644 --- a/source4/torture/smb2/oplock.c +++ b/source4/torture/smb2/oplock.c @@ -2957,15 +2957,9 @@ static bool test_smb2_oplock_batch25(struct torture_context *tctx, h1 = io.smb2.out.file.handle; CHECK_VAL(io.smb2.out.oplock_level, SMB2_OPLOCK_LEVEL_BATCH); - torture_comment(tctx, "changing the file attribute info should trigger " - "a break and a violation\n"); - status = smb2_util_setatr(tree1, fname, FILE_ATTRIBUTE_HIDDEN); - torture_assert_ntstatus_equal(tctx, status, NT_STATUS_SHARING_VIOLATION, - "Incorrect status"); - - torture_wait_for_oplock_break(tctx); - CHECK_VAL(break_info.count, 1); + torture_assert_ntstatus_ok(tctx, status, "Setting attributes " + "shouldn't trigger an oplock break"); smb2_util_close(tree1, h1); smb2_util_close(tree1, h); -- 2.37.2 From d6423337ffb929e4a452997b505ea34f17b7f480 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 19 Aug 2022 17:29:55 +0200 Subject: [PATCH 2/3] smbtorture: check required access for SMB2-GETINFO BUG: https://bugzilla.samba.org/show_bug.cgi?id=15153 Signed-off-by: Ralph Boehme Reviewed-by: Stefan Metzmacher (cherry picked from commit 9b2d28157107602fcbe659664cf9ca25f08bb30b) --- selftest/knownfail | 1 + selftest/knownfail.d/samba3.smb2.getinfo | 2 + source4/torture/smb2/getinfo.c | 147 +++++++++++++++++++++++ 3 files changed, 150 insertions(+) create mode 100644 selftest/knownfail.d/samba3.smb2.getinfo diff --git a/selftest/knownfail b/selftest/knownfail index 27ca06aa7f25..0db680820bf0 100644 --- a/selftest/knownfail +++ b/selftest/knownfail @@ -176,6 +176,7 @@ ^samba4.smb2.oplock.stream1 # samba 4 oplocks are a mess ^samba4.smb2.oplock.statopen1\(ad_dc_ntvfs\)$ # fails with ACCESS_DENIED on a SYNCHRONIZE_ACCESS open ^samba4.smb2.getinfo.complex # streams on directories does not work +^samba4.smb2.getinfo.getinfo_access\(ad_dc_ntvfs\) # Access checks not implemented ^samba4.smb2.getinfo.qfs_buffercheck # S4 does not do the INFO_LENGTH_MISMATCH/BUFFER_OVERFLOW thingy ^samba4.smb2.getinfo.qfile_buffercheck # S4 does not do the INFO_LENGTH_MISMATCH/BUFFER_OVERFLOW thingy ^samba4.smb2.getinfo.qsec_buffercheck # S4 does not do the BUFFER_TOO_SMALL thingy diff --git a/selftest/knownfail.d/samba3.smb2.getinfo b/selftest/knownfail.d/samba3.smb2.getinfo new file mode 100644 index 000000000000..dbef40c83f96 --- /dev/null +++ b/selftest/knownfail.d/samba3.smb2.getinfo @@ -0,0 +1,2 @@ +^samba3.smb2.getinfo.getinfo_access\(nt4_dc\) +^samba3.smb2.getinfo.getinfo_access\(ad_dc\) diff --git a/source4/torture/smb2/getinfo.c b/source4/torture/smb2/getinfo.c index 5c9097f42002..539090ac71ab 100644 --- a/source4/torture/smb2/getinfo.c +++ b/source4/torture/smb2/getinfo.c @@ -783,6 +783,151 @@ static bool torture_smb2_getinfo(struct torture_context *tctx) return ret; } +#undef LEVEL +#define LEVEL(l, u, ua, ra) \ + .name = #l, \ + .level = l, \ + .unrestricted = u, \ + .unrestricted_access = ua, \ + .required_access = ra + +static struct { + const char *name; + uint16_t level; + bool unrestricted; + uint32_t unrestricted_access; + uint32_t required_access; +} file_levels_access[] = { + /* + * The following info levels are not checked: + * - FileFullEaInformation and FileIdInformation: + * not implemented by the s4/libcli/raw + * - all pipe infolevels: that should be tested elsewhere by RPC tests + */ + + /* + * The following allow unrestricted access, so requesting + * SEC_FILE_READ_ATTRIBUTE works, SEC_FILE_READ_ATTRIBUTE or + * SEC_FILE_READ_EA as well of course. + */ + { LEVEL(RAW_FILEINFO_STANDARD_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_INTERNAL_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_ACCESS_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_POSITION_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_MODE_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_ALIGNMENT_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_ALT_NAME_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_STREAM_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_COMPRESSION_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_NORMALIZED_NAME_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_EA_INFORMATION, true, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_EA) }, + + /* + * The following require either SEC_FILE_READ_ATTRIBUTE or + * SEC_FILE_READ_EA. + */ + { LEVEL(RAW_FILEINFO_BASIC_INFORMATION, false, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_ALL_INFORMATION, false, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_NETWORK_OPEN_INFORMATION, false, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_ATTRIBUTE_TAG_INFORMATION, false, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_SMB2_ALL_INFORMATION, false, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_ATTRIBUTE) }, + { LEVEL(RAW_FILEINFO_SMB2_ALL_EAS, false, SEC_STD_SYNCHRONIZE, SEC_FILE_READ_EA) }, + /* Also try SEC_FILE_READ_ATTRIBUTE to show that it is the wrong one */ + { LEVEL(RAW_FILEINFO_SMB2_ALL_EAS, false, SEC_FILE_READ_ATTRIBUTE, SEC_FILE_READ_EA) }, + { LEVEL(RAW_FILEINFO_SEC_DESC, false, SEC_STD_SYNCHRONIZE, SEC_STD_READ_CONTROL) }, + /* Also try SEC_FILE_READ_ATTRIBUTE to show that it is the wrong one */ + { LEVEL(RAW_FILEINFO_SEC_DESC, false, SEC_FILE_READ_ATTRIBUTE, SEC_STD_READ_CONTROL) } +}; + + +/* + test fileinfo levels +*/ +static bool torture_smb2_getfinfo_access(struct torture_context *tctx, + struct smb2_tree *tree) +{ + const char *fname = "torture_smb2_getfinfo_access"; + struct smb2_handle hfile; + NTSTATUS status; + bool ret = true; + int i; + + smb2_deltree(tree, fname); + + torture_setup_complex_file(tctx, tree, fname); + + for (i = 0; i < ARRAY_SIZE(file_levels_access); i++) { + union smb_fileinfo finfo; + NTSTATUS expected_status; + + /* + * First open with unrestricted_access, SEC_STD_SYNCHRONIZE for + * most tests, info levels with unrestricted=true should allow + * this. + */ + status = torture_smb2_testfile_access( + tree, fname, &hfile, file_levels_access[i].unrestricted_access); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "Unable to open test file\n"); + + if (file_levels_access[i].level == RAW_FILEINFO_SEC_DESC) { + finfo.query_secdesc.in.secinfo_flags = 0x7; + } + if (file_levels_access[i].level == RAW_FILEINFO_SMB2_ALL_EAS) { + finfo.all_eas.in.continue_flags = + SMB2_CONTINUE_FLAG_RESTART; + } + + finfo.generic.level = file_levels_access[i].level; + finfo.generic.in.file.handle = hfile; + + if (file_levels_access[i].unrestricted) { + expected_status = NT_STATUS_OK; + } else { + expected_status = NT_STATUS_ACCESS_DENIED; + } + + status = smb2_getinfo_file(tree, tree, &finfo); + torture_assert_ntstatus_equal_goto( + tctx, status, expected_status, ret, done, + talloc_asprintf(tctx, "Level %s failed\n", + file_levels_access[i].name)); + + smb2_util_close(tree, hfile); + + /* + * Now open with expected access, getinfo should work. + */ + status = torture_smb2_testfile_access( + tree, fname, &hfile, file_levels_access[i].required_access); + torture_assert_ntstatus_ok_goto( + tctx, status, ret, done, + "Unable to open test file\n"); + + if (file_levels_access[i].level == RAW_FILEINFO_SEC_DESC) { + finfo.query_secdesc.in.secinfo_flags = 0x7; + } + if (file_levels_access[i].level == RAW_FILEINFO_SMB2_ALL_EAS) { + finfo.all_eas.in.continue_flags = + SMB2_CONTINUE_FLAG_RESTART; + } + finfo.generic.level = file_levels_access[i].level; + finfo.generic.in.file.handle = hfile; + + status = smb2_getinfo_file(tree, tree, &finfo); + torture_assert_ntstatus_ok_goto( + tctx, status, ret, done, + talloc_asprintf(tctx, "%s on file", + file_levels_access[i].name)); + + smb2_util_close(tree, hfile); + } + +done: + smb2_deltree(tree, fname); + return ret; +} + struct torture_suite *torture_smb2_getinfo_init(TALLOC_CTX *ctx) { struct torture_suite *suite = torture_suite_create( @@ -800,5 +945,7 @@ struct torture_suite *torture_smb2_getinfo_init(TALLOC_CTX *ctx) torture_smb2_fileinfo_grant_read); torture_suite_add_simple_test(suite, "normalized", torture_smb2_fileinfo_normalized); + torture_suite_add_1smb2_test(suite, "getinfo_access", + torture_smb2_getfinfo_access); return suite; } -- 2.37.2 From e802efc27ff2aa65648f4afa66d2a5ca029c12d7 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sun, 14 Aug 2022 18:46:24 +0200 Subject: [PATCH 3/3] smbd: implement access checks for SMB2-GETINFO as per MS-SMB2 3.3.5.20.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The spec lists the following as requiring special access: - for requiring FILE_READ_ATTRIBUTES: FileBasicInformation FileAllInformation FileNetworkOpenInformation FileAttributeTagInformation - for requiring FILE_READ_EA: FileFullEaInformation All other infolevels are unrestricted. We ignore the IPC related infolevels: FilePipeInformation FilePipeLocalInformation FilePipeRemoteInformation BUG: https://bugzilla.samba.org/show_bug.cgi?id=15153 RN: Missing SMB2-GETINFO access checks from MS-SMB2 3.3.5.20.1 Signed-off-by: Ralph Boehme Reviewed-by: Stefan Metzmacher Autobuild-User(master): Ralph Böhme Autobuild-Date(master): Tue Aug 23 12:54:08 UTC 2022 on sn-devel-184 (cherry picked from commit 6d493a9d568c08cfe5242821ccbd5a5ee1fe5284) --- selftest/knownfail | 2 -- selftest/knownfail.d/samba3.smb2.getinfo | 2 -- source3/smbd/smb2_getinfo.c | 28 ++++++++++++++++++++++++ 3 files changed, 28 insertions(+), 4 deletions(-) delete mode 100644 selftest/knownfail.d/samba3.smb2.getinfo diff --git a/selftest/knownfail b/selftest/knownfail index 0db680820bf0..f130d2dc3c5a 100644 --- a/selftest/knownfail +++ b/selftest/knownfail @@ -208,10 +208,8 @@ ^samba3.smb2.oplock.stream1 ^samba3.smb2.streams.rename ^samba3.smb2.streams.rename2 -^samba3.smb2.streams.attributes1\(.*\) ^samba3.smb2.streams streams_xattr.rename\(nt4_dc\) ^samba3.smb2.streams streams_xattr.rename2\(nt4_dc\) -^samba3.smb2.streams streams_xattr.attributes1\(nt4_dc\) ^samba3.smb2.getinfo.complex ^samba3.smb2.getinfo.fsinfo # quotas don't work yet ^samba3.smb2.setinfo.setinfo diff --git a/selftest/knownfail.d/samba3.smb2.getinfo b/selftest/knownfail.d/samba3.smb2.getinfo deleted file mode 100644 index dbef40c83f96..000000000000 --- a/selftest/knownfail.d/samba3.smb2.getinfo +++ /dev/null @@ -1,2 +0,0 @@ -^samba3.smb2.getinfo.getinfo_access\(nt4_dc\) -^samba3.smb2.getinfo.getinfo_access\(ad_dc\) diff --git a/source3/smbd/smb2_getinfo.c b/source3/smbd/smb2_getinfo.c index 0320dcc5fdef..23322e7b85f3 100644 --- a/source3/smbd/smb2_getinfo.c +++ b/source3/smbd/smb2_getinfo.c @@ -303,6 +303,34 @@ static struct tevent_req *smbd_smb2_getinfo_send(TALLOC_CTX *mem_ctx, ZERO_STRUCT(write_time_ts); + /* + * MS-SMB2 3.3.5.20.1 "Handling SMB2_0_INFO_FILE" + * + * FileBasicInformation, FileAllInformation, + * FileNetworkOpenInformation, FileAttributeTagInformation + * require FILE_READ_ATTRIBUTES. + * + * FileFullEaInformation requires FILE_READ_EA. + */ + switch (in_file_info_class) { + case FSCC_FILE_BASIC_INFORMATION: + case FSCC_FILE_ALL_INFORMATION: + case FSCC_FILE_NETWORK_OPEN_INFORMATION: + case FSCC_FILE_ATTRIBUTE_TAG_INFORMATION: + if (!(fsp->access_mask & SEC_FILE_READ_ATTRIBUTE)) { + tevent_req_nterror(req, NT_STATUS_ACCESS_DENIED); + return tevent_req_post(req, ev); + } + break; + + case FSCC_FILE_FULL_EA_INFORMATION: + if (!(fsp->access_mask & SEC_FILE_READ_EA)) { + tevent_req_nterror(req, NT_STATUS_ACCESS_DENIED); + return tevent_req_post(req, ev); + } + break; + } + switch (in_file_info_class) { case FSCC_FILE_FULL_EA_INFORMATION: file_info_level = SMB2_FILE_FULL_EA_INFORMATION; -- 2.37.2