Bug 5818 - smbcacls: sorts ACEs improperly and loses inheritance
Summary: smbcacls: sorts ACEs improperly and loses inheritance
Status: NEW
Alias: None
Product: Samba 3.2
Classification: Unclassified
Component: Client tools (show other bugs)
Version: unspecified
Hardware: All All
: P3 normal
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-08 10:49 UTC by Paul Fertser
Modified: 2024-04-11 00:03 UTC (History)
1 user (show)

See Also:


Attachments
Proposed patch (2.96 KB, patch)
2008-10-08 10:53 UTC, Paul Fertser
no flags Details
Patch I actually pushed. (8.72 KB, patch)
2008-10-14 18:12 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Fertser 2008-10-08 10:49:45 UTC
I was unable to propely manipulate ACLs on a w2k3 share. Windows
clients complained about the order of ACEs after any change to
ACL. And after any SD modification all w2k inheritance semantics was
lost.

Patch attached.
Comment 1 Paul Fertser 2008-10-08 10:53:16 UTC
Created attachment 3670 [details]
Proposed patch

Tested on a w2k3 share. Before the change windows clients complained
about wrong ACEs order and objects in the affected container were
created with SEC_ACE_FLAG_INHERITED_ACE unset (nt4 semantic
afaik). This patch fixed the issues.
Comment 2 Jeremy Allison 2008-10-14 18:12:16 UTC
Created attachment 3677 [details]
Patch I actually pushed.

This is what I pushed. I added features to get/set the inherit flags as symbolic names to make things easier to experiment with. I am still not convinced of the need for SEC_DESC_DACL_AUTO_INHERIT_REQ on the DACL on set - can you please explain what caused you to set this in your patch ?
Jeremy.
Comment 3 Derrell Lipman 2008-10-15 13:14:14 UTC
See also the ace_compare() function in source3/libsmb/libsmb_xattr.c.  I'd fixed a similar problem a while ago, and found additional documentation mentioned in the comment of that function.

Derrell
Comment 4 Paul Fertser 2008-10-17 07:18:41 UTC
Thanks a lot for looking into this. Your patch is definitely much fancier :)  I hope you'll find the following typescript convincing enough: Here i use your version unmodified (i have //venus/work mounted to ~/venus-work) pavel@paulfertser:~$ mkdir venus-work/p.fertser/test-dir pavel@paulfertser:~$ smbcacls -k //venus/work p.fertser/test-dir REVISION:1 CONTROL:0x8404 OWNER:WORK\P.Fertser GROUP:WORK\Domain users ACL:NT AUTHORITY\SYSTEM:ALLOWED/OI|CI|I/FULL ACL:BUILTIN\Administrators:ALLOWED/OI|CI|I/FULL ACL:WORK\P.Fertser:ALLOWED/I/FULL ACL:CREATOR-OWNER:ALLOWED/OI|CI|IO|I/FULL pavel@paulfertser:~$ smbcacls -k //venus/work p.fertser/test-dir -M "ACL:WORK\P.Fertser:ALLOWED/I/FULL" pavel@paulfertser:~$ smbcacls -k //venus/work p.fertser/test-dir REVISION:1 CONTROL:0x8004 OWNER:WORK\P.Fertser GROUP:WORK\Domain users ACL:NT AUTHORITY\SYSTEM:ALLOWED/OI|CI|I/FULL ACL:BUILTIN\Administrators:ALLOWED/OI|CI|I/FULL ACL:WORK\P.Fertser:ALLOWED/I/FULL ACL:CREATOR-OWNER:ALLOWED/OI|CI|IO|I/FULL pavel@paulfertser:~$ mkdir venus-work/p.fertser/test-dir/test-subdir pavel@paulfertser:~$ smbcacls -k //venus/work p.fertser/test-dir/test-subdir REVISION:1 CONTROL:0x8004 OWNER:WORK\P.Fertser GROUP:WORK\Domain users ACL:NT AUTHORITY\SYSTEM:ALLOWED/OI|CI/FULL ACL:BUILTIN\Administrators:ALLOWED/OI|CI/FULL ACL:WORK\P.Fertser:ALLOWED/0x0/FULL ACL:CREATOR-OWNER:ALLOWED/OI|CI|IO|I/FULL  You see how all ACEs (except the one having "generic SID") in the subdir don't have "I" set?  And now the same but with SEC_DESC_DACL_AUTO_INHERIT_REQ set:  pavel@paulfertser:~$ mkdir venus-work/p.fertser/test-dir pavel@paulfertser:~$ smbcacls-paul -k //venus/work p.fertser/test-dir REVISION:1 CONTROL:0x8404 OWNER:WORK\P.Fertser GROUP:WORK\Domain users ACL:NT AUTHORITY\SYSTEM:ALLOWED/OI|CI|I/FULL ACL:BUILTIN\Administrators:ALLOWED/OI|CI|I/FULL ACL:WORK\P.Fertser:ALLOWED/I/FULL ACL:CREATOR-OWNER:ALLOWED/OI|CI|IO|I/FULL pavel@paulfertser:~$ smbcacls-paul -k //venus/work p.fertser/test-dir -M "ACL:WORK\P.Fertser:ALLOWED/I/FULL" pavel@paulfertser:~$ smbcacls-paul -k //venus/work p.fertser/test-dir REVISION:1 CONTROL:0x8404 OWNER:WORK\P.Fertser GROUP:WORK\Domain users ACL:NT AUTHORITY\SYSTEM:ALLOWED/OI|CI|I/FULL ACL:BUILTIN\Administrators:ALLOWED/OI|CI|I/FULL ACL:WORK\P.Fertser:ALLOWED/I/FULL ACL:CREATOR-OWNER:ALLOWED/OI|CI|IO|I/FULL pavel@paulfertser:~$ mkdir venus-work/p.fertser/test-dir/test-subdir pavel@paulfertser:~$ smbcacls-paul -k //venus/work p.fertser/test-dir/test-subdir REVISION:1 CONTROL:0x8404 OWNER:WORK\P.Fertser GROUP:WORK\Domain users ACL:NT AUTHORITY\SYSTEM:ALLOWED/OI|CI|I/FULL ACL:BUILTIN\Administrators:ALLOWED/OI|CI|I/FULL ACL:WORK\P.Fertser:ALLOWED/I/FULL ACL:CREATOR-OWNER:ALLOWED/OI|CI|IO|I/FULL  As far as i understand, the latter is the correct behavior for w2k (and later) systems while the previous is nt4-like.
Comment 5 Paul Fertser 2008-10-17 07:21:43 UTC
Shame on me, my previous comment is unreadable. I hope this one will
be better.

Thanks a lot for looking into this. Your patch is definitely much
fancier :)

I hope you'll find the following typescript convincing enough:
Here i use your version unmodified (i have //venus/work mounted to
~/venus-work)

pavel@paulfertser:~$ mkdir venus-work/p.fertser/test-dir

pavel@paulfertser:~$ smbcacls -k //venus/work p.fertser/test-dir

REVISION:1

CONTROL:0x8404

OWNER:WORK\P.Fertser

GROUP:WORK\Domain users

ACL:NT AUTHORITY\SYSTEM:ALLOWED/OI|CI|I/FULL

ACL:BUILTIN\Administrators:ALLOWED/OI|CI|I/FULL

ACL:WORK\P.Fertser:ALLOWED/I/FULL

ACL:CREATOR-OWNER:ALLOWED/OI|CI|IO|I/FULL

pavel@paulfertser:~$ smbcacls -k //venus/work p.fertser/test-dir -M "ACL:WORK\P.Fertser:ALLOWED/I/FULL"

pavel@paulfertser:~$ smbcacls -k //venus/work p.fertser/test-dir

REVISION:1

CONTROL:0x8004

OWNER:WORK\P.Fertser

GROUP:WORK\Domain users

ACL:NT AUTHORITY\SYSTEM:ALLOWED/OI|CI|I/FULL

ACL:BUILTIN\Administrators:ALLOWED/OI|CI|I/FULL

ACL:WORK\P.Fertser:ALLOWED/I/FULL

ACL:CREATOR-OWNER:ALLOWED/OI|CI|IO|I/FULL

pavel@paulfertser:~$ mkdir venus-work/p.fertser/test-dir/test-subdir

pavel@paulfertser:~$ smbcacls -k //venus/work p.fertser/test-dir/test-subdir

REVISION:1

CONTROL:0x8004

OWNER:WORK\P.Fertser

GROUP:WORK\Domain users

ACL:NT AUTHORITY\SYSTEM:ALLOWED/OI|CI/FULL

ACL:BUILTIN\Administrators:ALLOWED/OI|CI/FULL

ACL:WORK\P.Fertser:ALLOWED/0x0/FULL

ACL:CREATOR-OWNER:ALLOWED/OI|CI|IO|I/FULL

You see how all ACEs (except the one having "generic SID") in the
subdir don't have "I" set?  And now the same but with
SEC_DESC_DACL_AUTO_INHERIT_REQ set:

pavel@paulfertser:~$ mkdir venus-work/p.fertser/test-dir

pavel@paulfertser:~$ smbcacls-paul -k //venus/work p.fertser/test-dir

REVISION:1

CONTROL:0x8404

OWNER:WORK\P.Fertser

GROUP:WORK\Domain users

ACL:NT AUTHORITY\SYSTEM:ALLOWED/OI|CI|I/FULL

ACL:BUILTIN\Administrators:ALLOWED/OI|CI|I/FULL

ACL:WORK\P.Fertser:ALLOWED/I/FULL

ACL:CREATOR-OWNER:ALLOWED/OI|CI|IO|I/FULL

pavel@paulfertser:~$ smbcacls-paul -k //venus/work p.fertser/test-dir -M "ACL:WORK\P.Fertser:ALLOWED/I/FULL"

pavel@paulfertser:~$ smbcacls-paul -k //venus/work p.fertser/test-dir

REVISION:1

CONTROL:0x8404

OWNER:WORK\P.Fertser

GROUP:WORK\Domain users

ACL:NT AUTHORITY\SYSTEM:ALLOWED/OI|CI|I/FULL

ACL:BUILTIN\Administrators:ALLOWED/OI|CI|I/FULL

ACL:WORK\P.Fertser:ALLOWED/I/FULL

ACL:CREATOR-OWNER:ALLOWED/OI|CI|IO|I/FULL

pavel@paulfertser:~$ mkdir venus-work/p.fertser/test-dir/test-subdir

pavel@paulfertser:~$ smbcacls-paul -k //venus/work p.fertser/test-dir/test-subdir

REVISION:1

CONTROL:0x8404

OWNER:WORK\P.Fertser

GROUP:WORK\Domain users

ACL:NT AUTHORITY\SYSTEM:ALLOWED/OI|CI|I/FULL

ACL:BUILTIN\Administrators:ALLOWED/OI|CI|I/FULL

ACL:WORK\P.Fertser:ALLOWED/I/FULL

ACL:CREATOR-OWNER:ALLOWED/OI|CI|IO|I/FULL

As far as i understand, the latter is the correct behavior for w2k
(and later) systems while the previous is nt4-like.