When using "inherit acls = yes" I noticed that the inherited ACEs are lacking the inherited flag. [root@allfreebsd12 ~]# getfacl /zfs/dir/ # file: /zfs/dir/ # owner: root # group: wheel group:samba:rwxpDdaARWcCos:fd-----:allow owner@:rwxp--aARWcCos:-------:allow group@:r-x---a-R-c--s:-------:allow everyone@:r-x---a-R-c--s:-------:allow [root@allfreebsd12 ~]# smbclient -U slow%x //localhost/test -c "put file dir/foo" putting file file as \dir\foo (0.0 kb/s) (average 0.0 kb/s) [root@allfreebsd12 ~]# getfacl /zfs/dir/foo # file: /zfs/dir/foo # owner: slow # group: wheel group:samba:rwxpDdaARWcCo-:-------:allow Works just fine at the ZFS layer: [root@allfreebsd12 ~]# touch /zfs/dir/bar [root@allfreebsd12 ~]# getfacl /zfs/dir/bar # file: /zfs/dir/bar # owner: root # group: wheel group:samba:rwxpDdaARWcCos:------I:allow owner@:rw-p--aARWcCos:-------:allow group@:r-----a-R-c--s:-------:allow everyone@:r-----a-R-c--s:-------:allow Not sure where in the stack this gets screwed. This is with [root@allfreebsd12 ~]# smbd -V Version 4.13.1 but I guess newer versions will also have this problem, as I'm not aware of any recent bugfixes in this area.
Hi Ralph, I'll take a look at this as well (today or tomorrow). By default we don't set "inherit acls = yes" on ZFS because the kernel / ZFS will handle ACL inheritance for us. One additional item to look at (just to exclude one more area of potential issues) is the version of libsunacl on the FreeBSD server. A few years back I added support for ACE_INHERITED_ACE ("I" flag was being stripped). "pkg info libsunacl" should show version 1.0.1.
(In reply to Andrew Walker from comment #1) Oh, yes, you did set inherit acls at some point in time... this was originally observed on a FreeNAS system with Samba 4.10.8 (iirc). :) So i went ahead and tried to reproduce it on a testsystem in the lab and picked a FreeBSD box: Same problem.
Yikes. 4.10.8 would have been a pre-release version of our product (11.3-BETA1). For better or worse we allow users to override default share configuration options (and append things to the smb.conf). Once a user hits permissions issues, most users default to applying all the things. I was able to reproduce the issue. I'll start investigating tomorrow as well.
Created attachment 16430 [details] log of inherit_new_acl()
Sorry uploaded wrong log file. Next attachment should be right one. Looks like things are getting garbled in inherit_new_acl() possible after se_create_child_secdesc(). Haven't looked closely at it yet.
Created attachment 16431 [details] Correct log
Okay. Maybe not a bug since the SD doesn't have SEC_DESC_DACL_AUTO_INHERITED set... my memory is a bit fuzzy about what the correct control flags are here so I will probably need many cups of coffee and some sleep before thinking about this.
PS C:\Shares\SHARE> Get-Acl .\DIR\ |format-list Path : Microsoft.PowerShell.Core\FileSystem::C:\Shares\SHARE\DIR\ Owner : BUILTIN\Administrators Group : HOMEDOM\Domain Users Access : CREATOR OWNER Allow FullControl NT AUTHORITY\SYSTEM Allow FullControl BUILTIN\Administrators Allow FullControl BUILTIN\Users Allow CreateFiles, AppendData, Synchronize BUILTIN\Users Allow ReadAndExecute, Synchronize Audit : Sddl : O:BAG:DUD:PAI(A;OICIIO;FA;;;CO)(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;CI;0x100006;;;BU)(A;OICI;0x1200a9;;;BU) ^^^ Windows PS Microsoft.PowerShell.Core\FileSystem::\\192.168.0.221\share> get-acl foo | format-list Path : Microsoft.PowerShell.Core\FileSystem::\\192.168.0.221\share\foo Owner : O:S-1-22-1-0 Group : G:S-1-22-2-0 Access : Everyone Allow ReadAndExecute, Synchronize S-1-5-21-2152865990-1588067200-718635612-1000 Allow FullControl S-1-22-1-0 Allow Write, ReadAndExecute, ChangePermissions, TakeOwnership, Synchronize S-1-22-2-0 Allow ReadAndExecute, Synchronize Audit : Sddl : O:S-1-22-1-0G:S-1-22-2-0D:P(A;;0x1200a9;;;WD)(A;OICI;FA;;;S-1-5-21-2152865990-1588067200-718635612-1000)(A;;0x 1e01bf;;;S-1-22-1-0)(A;;0x1200a9;;;S-1-22-2-0) ^^^ samba with vfs_zfsacl Looks like control flags are PAI on Windows on parent directory when auto-inheritance is disabled. We're only setting P in this case. Switching to SEC_DESC_DACL_PROTECTED|SEC_DESC_DACL_AUTO_INHERITED in this case appears to fix the issue. I'll throw a WIP patch here for you to verify.
Created attachment 16432 [details] WIP patch
Fixed version: PS Microsoft.PowerShell.Core\FileSystem::\\192.168.0.221\share> get-acl foo | format-list Path : Microsoft.PowerShell.Core\FileSystem::\\192.168.0.221\share\foo Owner : O:S-1-22-1-0 Group : G:S-1-22-2-0 Access : Everyone Allow ReadAndExecute, Synchronize S-1-5-21-2152865990-1588067200-718635612-1000 Allow FullControl S-1-22-1-0 Allow DeleteSubdirectoriesAndFiles, Write, ReadAndExecute, ChangePermissions, TakeOwnership, Synchronize S-1-22-2-0 Allow ReadAndExecute, Synchronize Audit : Sddl : O:S-1-22-1-0G:S-1-22-2-0D:PAI(A;;0x1200a9;;;WD)(A;OICI;FA;;;S-1-5-21-2152865990-1588067200-718635612-1000)(A;; 0x1e01ff;;;S-1-22-1-0)(A;;0x1200a9;;;S-1-22-2-0) Looks more correct and addressed the issue I could reproduce. Please let me know if it seems to fix your issue. If you're building from freebsd ports, you can run "make patch" from the samba port's directory, then manually edit work/<samba version>/source3/modules/vfs_zfsacl.c, then continue with "make install".
Thanks! I'll review this patch later, but wanted to point out that the problem I was describing is about ACE flags, ACE inherited flag in particular, not ACL control flags...
Created attachment 16433 [details] WIP patch 2 This is a corrected WIP patch that adds SEC_DESC_AUTO_INHERITED to the control flags in all cases (not just where we want PROTECTED).
>Thanks! I'll review this patch later, but wanted to point out that the problem I was describing is about ACE flags, ACE inherited flag in particular, not ACL control flags... source3/smbd/open.c inherit_new_acl() - calls VFS_GET_NT_ACL_AT() and vfs_zfsacl creates SD without SEC_DESC_DACL_AUTO_INHERITED set. source3/smbd/open.c inherit_new_acl() calls se_create_child_secdesc() using the SD of the parent directory. libcli/security/secdesc.c se_create_child_secdesc() generates a child SD and removes SEC_ACE_FLAG_INHERITED_ACE if SEC_DESC_DACL_AUTO_INHERITED is not set. I'm not quite sure that this is quite the correct use of that control flag, but since it appears in my limited testing that AI is always set on Windows, it's perhaps moot.
Hm, not sure if this is the correct way to fix, still wrapping my head around this... But as apparently this also affects the acl_xattr module: $ bin/smbcacls -U slow%x //localhost/test "" REVISION:1 CONTROL:SR|DP OWNER:SLOWSERVER\slow GROUP:BUILTIN\Administrators ACL:Everyone:ALLOWED/0x0/FULL ACL:Everyone:ALLOWED/OI|CI|IO/READ ACL:Creator Owner:ALLOWED/OI|CI|IO/FULL ACL:Creator Group:ALLOWED/OI|CI|IO/READ ACL:BUILTIN\Administrators:ALLOWED/0x0/FULL ACL:SLOWSERVER\slow:ALLOWED/0x0/FULL ACL:SLOWSERVER\slow:ALLOWED/OI|CI/FULL ACL:SLOWSERVER\named:ALLOWED/OI|CI/FULL $ bin/smbclient -U slow%x //localhost/test -c "put WHATSNEW.txt" putting file WHATSNEW.txt as \WHATSNEW.txt (331.1 kb/s) (average 331.1 kb/s) $ bin/smbcacls -U slow%x //localhost/test "WHATSNEW.txt" REVISION:1 CONTROL:SR|DP OWNER:Unix User\tcpdump GROUP:Unix Group\tcpdump ACL:Everyone:ALLOWED/0x0/READ ACL:Unix User\tcpdump:ALLOWED/0x0/FULL ACL:Unix Group\tcpdump:ALLOWED/0x0/READ ACL:SLOWSERVER\slow:ALLOWED/0x0/FULL ACL:SLOWSERVER\named:ALLOWED/0x0/FULL ...I guess we need a different fix. Fwiw, I'm really bemazed that our tests did not catch this.
Control on your parent directory with acl_xattr looks wrong: >CONTROL:SR|DP I believe it should be following in order to avoid the issue. CONTROL:SR|DI|DP
For reference, here's a Windows server: root@fbsd12:/usr/ports/net/samba # smbcacls //192.168.0.36/SHARE / -j -U 'joiner%<redacted>' | jq '.control' { "Self Relative": true, "RM Control Valid": false, "SACL Protected": false, "DACL Protected": false, "SACL Auto Inherited": false, "DACL Auto Inherited": true, "SACL Inheritance Required": false, "DACL Inheritance Required": false, "Server Security": false, "DACL Trusted": false, "SACL Defaulted": false, "SACL Present": false, "DACL Defaulted": false, "DACL Present": true, "Group Defaulted": false, "Owner Defaulted": false }
(In reply to Andrew Walker from comment #16) I thing we need something like <https://gitlab.com/samba-team/samba/-/merge_requests/1793> With this I get the correct ACL and ACE flags on a created file: $ bin/smbcacls -U slow%x //localhost/test "WHATSNEW.txt" REVISION:1 CONTROL:SR|DI|DP OWNER:Unix User\tcpdump GROUP:Unix Group\tcpdump ACL:Everyone:ALLOWED/I/READ ACL:Unix User\tcpdump:ALLOWED/I/FULL ACL:Unix Group\tcpdump:ALLOWED/I/READ ACL:SLOWSERVER\slow:ALLOWED/I/FULL ACL:SLOWSERVER\named:ALLOWED/I/FULL
Jeremy, can you take a look please? The original logic from commit cf29863c69b36224564c27ef1610010b943857c0 is yours. Cf https://gitlab.com/samba-team/samba/-/merge_requests/1793 were I'm testing a fix.
I did notice the comment: /* * We need to remove SEC_ACE_FLAG_INHERITED_ACE here * if present because it should only be set if the * parent has the AUTO_INHERITED bit set in the * type/control field. If we don't it will slip through * and create DACLs with incorrectly ordered ACEs * when there are CREATOR_OWNER or CREATOR_GROUP * ACEs. */ I haven't looked closely at it, but we may want to make sure we're not introducing a bug where they can be mis-ordered. FYI, MS has documented algorithms related to this in MS-DTYP 2.5.3.4.
(In reply to Andrew Walker from comment #19) yeah, this might need a bit more work. Cf <https://gitlab.com/samba-team/samba/-/merge_requests/1793/diffs?commit_id=607af694ebc75e16eed19258b82c8d4af8d17885> for references to MS-DTYP.
(In reply to Andrew Walker from comment #13) I just learned that Windows does infact only set SEC_ACE_FLAG_INHERITED_ACE in an inherited ACE *if the parent DACL has SEC_DESC_DACL_AUTO_INHERITED set*. So as Windows seems to set SEC_DESC_DACL_AUTO_INHERITED by default (it is eg set on / in Windows and all childs, so unless you mess with the flag, it will be set), this seems to be a problem of our acl_xattr VFS module.
Created attachment 16438 [details] Possible patch Pipeline: https://gitlab.com/samba-team/devel/samba/-/pipelines/253063078
Should we also make a similar change to the general nfs4_acl code or is the change to vfs_zfsacl sufficient?
(In reply to Ralph Böhme from comment #22) Changing the subject of this PR and continuing discussion here only for the inheritance semantics deficiencies in vfs_acl_xattr. The problem is basically, that it's easy to end up with a configuration on a share using vfs_acl_xattr where SEC_DESC_DACL_AUTO_INHERITED is not set in the share root SD. As a result, any object created in the share will also lack the ACE_INHERITED flag in all ACEs. The acl_xattr VFS module will not return the flag by default when a share has just been created and configured, but no ACLs have been explicitly setup by an admin yet. Example starting from scratch, no ACL configured yet: smb.conf: [test] path = /srv/samba/test vfs objects = acl_xattr acl_xattr:ignore system acls = yes acl_xattr:default acl style = everyone $ ls -al /srv/samba/test total 16 drwxrwx---. 3 slow slow 4096 May 25 16:39 . drwxr-xr-x. 7 root root 4096 May 17 12:29 .. $ getfattr -m "" /srv/samba/test getfattr: Removing leading '/' from absolute path names security.selinux $ bin/smbcacls -U "slow%Passw0rd" "//localhost/test" "" REVISION:1 CONTROL:SR|DP OWNER:SLOWSERVER\slow GROUP:BUILTIN\Administrators ACL:Everyone:ALLOWED/0x0/FULL Later, if an admin configures an ACL on the share root and sets up ACEs as desired from a Windows system, the client will generally set the SEC_DESC_DACL_AUTO_INHERITED on the share root. $ bin/smbcacls -U "slow%Passw0rd" "//localhost/test" "" REVISION:1 CONTROL:SR|PD|SI|DI|DP OWNER:SLOWSERVER\slow GROUP:BUILTIN\Administrators ACL:Everyone:ALLOWED/0x0/FULL ACL:S-1-5-21-1237673632-702085425-2222082532:ALLOWED/OI|CI/FULL But when eg using eg smbcacls it's easy to miss this step of adding that flag and just add some ACEs. As a result, all objects created in the container will lack the INHERITED_ACE flag in the ACL which further deviates from the behaviour you would get from a Windows box by default. To provide a better out-of-the-box experience, as long as there's no ACL set yet on the share root and we're either creating the ACL from the filesystem or we're synthesizing it in one of the default forms, we should set the flag SEC_DESC_DACL_AUTO_INHERITED. Going to push updated patches to the MR associated with this bug in a second.