From 111896618de862cfb19852500100a49c47d9f086 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sat, 6 Jul 2024 17:10:21 +0200 Subject: [PATCH 1/2] smbtorture: test creating stream doesn't crash when using "inherit permissions = yes" BUG: https://bugzilla.samba.org/show_bug.cgi?id=15695 Signed-off-by: Ralph Boehme Reviewed-by: Volker Lendecke (cherry picked from commit 09835608307ff2580f1aada84d44feddae17c80f) --- .../samba3.smb2.stream-inherit-perms | 1 + selftest/target/Samba3.pm | 5 ++ source3/selftest/tests.py | 2 + source4/torture/smb2/smb2.c | 2 + source4/torture/smb2/streams.c | 73 +++++++++++++++++++ 5 files changed, 83 insertions(+) create mode 100644 selftest/knownfail.d/samba3.smb2.stream-inherit-perms diff --git a/selftest/knownfail.d/samba3.smb2.stream-inherit-perms b/selftest/knownfail.d/samba3.smb2.stream-inherit-perms new file mode 100644 index 000000000000..fa311ac924d1 --- /dev/null +++ b/selftest/knownfail.d/samba3.smb2.stream-inherit-perms @@ -0,0 +1 @@ +^samba3.smb2.stream-inherit-perms.stream-inherit-perms\(fileserver\) diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm index aea64bf5d5df..a7dd1b20e660 100755 --- a/selftest/target/Samba3.pm +++ b/selftest/target/Samba3.pm @@ -2126,6 +2126,11 @@ sub setup_fileserver comment = Home directories browseable = No read only = No + +[inherit_perms] + path = $share_dir + vfs objects = streams_depot + inherit permissions = yes "; if (defined($more_conf)) { diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index 2de6c8ecd456..88151caea11f 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -1355,6 +1355,8 @@ tests = base + raw + smb2 + rpc + unix + local + rap + nbt + idmap + vfs plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD') plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD') plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/streams_xattr -U$USERNAME%$PASSWORD', 'streams_xattr') + elif t == "smb2.stream-inherit-perms": + plansmbtorture4testsuite(t, "fileserver", '//$SERVER/inherit_perms -U$USERNAME%$PASSWORD') elif t == "smb2.aio_delay": plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/aio_delay_inject -U$USERNAME%$PASSWORD') elif t == "smb2.delete-on-close-perms": diff --git a/source4/torture/smb2/smb2.c b/source4/torture/smb2/smb2.c index 5b6477e47bc3..28a62f49c47d 100644 --- a/source4/torture/smb2/smb2.c +++ b/source4/torture/smb2/smb2.c @@ -178,6 +178,8 @@ NTSTATUS torture_smb2_init(TALLOC_CTX *ctx) torture_suite_add_suite(suite, torture_smb2_oplocks_init(suite)); torture_suite_add_suite(suite, torture_smb2_kernel_oplocks_init(suite)); torture_suite_add_suite(suite, torture_smb2_streams_init(suite)); + torture_suite_add_1smb2_test(suite, "stream-inherit-perms", + test_stream_inherit_perms); torture_suite_add_suite(suite, torture_smb2_ioctl_init(suite)); torture_suite_add_simple_test(suite, "set-sparse-ioctl", test_ioctl_set_sparse); diff --git a/source4/torture/smb2/streams.c b/source4/torture/smb2/streams.c index f18048f7762a..abc1fe219608 100644 --- a/source4/torture/smb2/streams.c +++ b/source4/torture/smb2/streams.c @@ -30,6 +30,7 @@ #include "system/filesys.h" #include "system/locale.h" #include "lib/util/tsort.h" +#include "libcli/security/security_descriptor.h" #define DNAME "teststreams" @@ -2395,6 +2396,78 @@ static bool test_basefile_rename_with_open_stream(struct torture_context *tctx, return ret; } +/* + * Simple test creating a stream on a share with "inherit permissions" + * enabled. This tests specifically bug 15695. + */ +bool test_stream_inherit_perms(struct torture_context *tctx, + struct smb2_tree *tree) +{ + NTSTATUS status; + struct smb2_handle h = {}; + union smb_fileinfo q = {}; + union smb_setfileinfo setinfo = {}; + struct security_descriptor *sd = NULL; + struct security_ace ace = {}; + const char *fname = DNAME "\\test_stream_inherit_perms:stream"; + bool ret = true; + + smb2_deltree(tree, DNAME); + + status = torture_smb2_testdir(tree, DNAME, &h); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "torture_smb2_testdir failed\n"); + + torture_comment(tctx, "getting original sd\n"); + + q.query_secdesc.level = RAW_FILEINFO_SEC_DESC; + q.query_secdesc.in.file.handle = h; + q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER; + + status = smb2_getinfo_file(tree, tctx, &q); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_getinfo_file failed\n"); + + sd = q.query_secdesc.out.sd; + + /* + * Add one explicit non-inheriting ACE which will be stored + * as a non-inheriting POSIX ACE. These are the ACEs that + * "inherit permissions" will want to inherit. + */ + ace.type = SEC_ACE_TYPE_ACCESS_ALLOWED; + ace.access_mask = SEC_STD_ALL; + ace.trustee = *(sd->owner_sid); + + status = security_descriptor_dacl_add(sd, &ace); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "security_descriptor_dacl_add failed\n"); + + setinfo.set_secdesc.level = RAW_SFILEINFO_SEC_DESC; + setinfo.set_secdesc.in.file.handle = h; + setinfo.set_secdesc.in.secinfo_flags = SECINFO_DACL; + setinfo.set_secdesc.in.sd = sd; + + status = smb2_setinfo_file(tree, &setinfo); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_setinfo_file failed"); + + smb2_util_close(tree, h); + ZERO_STRUCT(h); + + /* This triggers the crash */ + status = torture_smb2_testfile(tree, fname, &h); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "torture_smb2_testfile failed"); + +done: + if (!smb2_util_handle_empty(h)) { + smb2_util_close(tree, h); + } + smb2_deltree(tree, DNAME); + return ret; +} + /* basic testing of streams calls SMB2 */ -- 2.46.0 From 9a37f23e04713ee8e2f64d32b3f33e708759ef86 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 5 Jul 2024 16:22:18 +0200 Subject: [PATCH 2/2] smbd: use metadata_fsp(fsp) in copy_access_posix_acl() for SMB_VFS_SYS_ACL_SET_FD When inherting permissions on the created stream, we call into the VFS to fetch the streams security descriptor via inherit_access_posix_acl() -> copy_access_posix_acl() -> SMB_VFS_SYS_ACL_SET_FD() passing the stream fsp which triggers the assert SMB_ASSERT(!fsp_is_alternate_stream(fsp)) in vfswrap_sys_acl_set_fd() in vfs_default. Just passing the base fsp to the VFS fixes this. vfs_streams_depot which *does use* distinct backend filesystem files for the streams, currently does not apply permissions to the stream files at all, so the incomplete behaviour of vfs_streams_depot is not affected by this change. If in the future someone want to fix this defficiency in vfs_streams_depot, the module code can use fsp->stream_fsp to base decisions in VFS ops whether the module should carry out some action. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15695 Signed-off-by: Ralph Boehme Reviewed-by: Volker Lendecke Autobuild-User(master): Volker Lendecke Autobuild-Date(master): Mon Sep 2 08:55:28 UTC 2024 on atb-devel-224 (cherry picked from commit ecb8a99a2c7ba36f9adc50ef13cd8465a0c49b19) --- selftest/knownfail.d/samba3.smb2.stream-inherit-perms | 1 - source3/smbd/posix_acls.c | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) delete mode 100644 selftest/knownfail.d/samba3.smb2.stream-inherit-perms diff --git a/selftest/knownfail.d/samba3.smb2.stream-inherit-perms b/selftest/knownfail.d/samba3.smb2.stream-inherit-perms deleted file mode 100644 index fa311ac924d1..000000000000 --- a/selftest/knownfail.d/samba3.smb2.stream-inherit-perms +++ /dev/null @@ -1 +0,0 @@ -^samba3.smb2.stream-inherit-perms.stream-inherit-perms\(fileserver\) diff --git a/source3/smbd/posix_acls.c b/source3/smbd/posix_acls.c index 0f6a0d52e019..53a9c53f54af 100644 --- a/source3/smbd/posix_acls.c +++ b/source3/smbd/posix_acls.c @@ -4000,7 +4000,9 @@ static int copy_access_posix_acl(struct files_struct *from, goto done; } - ret = SMB_VFS_SYS_ACL_SET_FD(to, SMB_ACL_TYPE_ACCESS, posix_acl); + ret = SMB_VFS_SYS_ACL_SET_FD(metadata_fsp(to), + SMB_ACL_TYPE_ACCESS, + posix_acl); done: -- 2.46.0