Bug 14631 - Missing inherited flag with "inherit acls = yes" and vfs_zfsacl on FreeBSD
Summary: Missing inherited flag with "inherit acls = yes" and vfs_zfsacl on FreeBSD
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Samba QA Contact
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-02-04 12:14 UTC by Ralph Böhme
Modified: 2021-03-23 14:30 UTC (History)
2 users (show)

See Also:


Attachments
log of inherit_new_acl() (152.58 KB, text/plain)
2021-02-04 22:11 UTC, Andrew Walker
no flags Details
Correct log (9.01 KB, text/plain)
2021-02-04 22:16 UTC, Andrew Walker
no flags Details
WIP patch (1015 bytes, patch)
2021-02-05 02:07 UTC, Andrew Walker
no flags Details
WIP patch 2 (1.57 KB, patch)
2021-02-05 11:58 UTC, Andrew Walker
no flags Details
Possible patch (1.55 KB, patch)
2021-02-08 14:08 UTC, Ralph Böhme
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ralph Böhme 2021-02-04 12:14:01 UTC
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.
Comment 1 Andrew Walker 2021-02-04 20:49:13 UTC
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.
Comment 2 Ralph Böhme 2021-02-04 20:58:18 UTC
(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.
Comment 3 Andrew Walker 2021-02-04 21:51:59 UTC
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.
Comment 4 Andrew Walker 2021-02-04 22:11:18 UTC
Created attachment 16430 [details]
log of inherit_new_acl()
Comment 5 Andrew Walker 2021-02-04 22:15:53 UTC
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.
Comment 6 Andrew Walker 2021-02-04 22:16:23 UTC
Created attachment 16431 [details]
Correct log
Comment 7 Andrew Walker 2021-02-04 22:26:43 UTC
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.
Comment 8 Andrew Walker 2021-02-05 02:01:13 UTC
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.
Comment 9 Andrew Walker 2021-02-05 02:07:38 UTC
Created attachment 16432 [details]
WIP patch
Comment 10 Andrew Walker 2021-02-05 02:14:32 UTC
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".
Comment 11 Ralph Böhme 2021-02-05 06:03:15 UTC
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...
Comment 12 Andrew Walker 2021-02-05 11:58:17 UTC
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).
Comment 13 Andrew Walker 2021-02-05 12:06:04 UTC
>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.
Comment 14 Ralph Böhme 2021-02-05 13:32:35 UTC
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.
Comment 15 Andrew Walker 2021-02-05 13:54:51 UTC
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
Comment 16 Andrew Walker 2021-02-05 13:58:29 UTC
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
}
Comment 17 Ralph Böhme 2021-02-05 14:25:58 UTC
(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
Comment 18 Ralph Böhme 2021-02-05 14:27:17 UTC
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.
Comment 19 Andrew Walker 2021-02-05 14:56:45 UTC
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.
Comment 20 Ralph Böhme 2021-02-05 15:13:16 UTC
(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.
Comment 21 Ralph Böhme 2021-02-08 14:04:46 UTC
(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.
Comment 22 Ralph Böhme 2021-02-08 14:08:43 UTC
Created attachment 16438 [details]
Possible patch

Pipeline: https://gitlab.com/samba-team/devel/samba/-/pipelines/253063078
Comment 23 Andrew Walker 2021-02-08 14:30:48 UTC
Should we also make a similar change to the general nfs4_acl code or is the change to vfs_zfsacl sufficient?