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
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

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.