The Samba-Bugzilla – Bug 4652
smbc_setxattr doesn't sort the acls correctly
Last modified: 2007-12-12 21:04:39 UTC
When using smbc_setxattr to restore acls on files residing on a windows fileshare the acls are not sorted according to Windows Explorers security interface.
If we try to view the security properties after setting acls with smbc_xattr we get the following error
"The permissions on 'Foo' are incorrectly ordered, which may cause some entries to be ineffective. Press OK to continue and sort the permissions correctly, or Cancel to reset the permissions."
I also tested this with smbcacls and the same problem occurs.
We currently (intentionally) put Denied ACE entries before Allowed ones. Is Windows Explorer telling you that it should be different than that, or are you seeing them not in that order, or is there some entirely different issue that we don't take into account?
Yes, Windows Explorer is complaining and below are the results of some tests I conducted.
First I just tested to get and set the acls of a file that only has default acl. This worked without complaint.
Then I added a user called Lena (BBI-DEV\Lena) and retrieved the ACL using smbc_getxattr.
I then immediately restored the ACL and the retrieved them again using smbc_getxattr and smbc_setxattr. Now somehow Lena is the last entry. If I now go to Windows Explorer it complains about the wrong order.
After Windows Explorer has sorted them I used smbc_getxattr to retrieve them again and now Lena is sorted to the beginning again.
I also did some test where I put Denied ACE on Lena but the problem is the same.
As I recall from my own experience writing some ACL handling code the non-inherited ACEs should come before the inherited-ACEs.
I believe this is the problem here as we can see that BBI-DEV\Lena ACE(which is not inherited) is last in the list after smbc_setxattr has done its magic and after Windows Explorer has reordered the ACE BBI-DEV\Lena is positioned in the beginning.
Somehow we need a sort routine that take the inherited flag into account.
Hi again Derrell,
Just to clarify the sort order with both denied and inheritance.
1. Denied non-inherited ACEs
2. Allowed non-inherited ACEs
3. Denied inherited ACEs
4. Allowed inherited ACEs
Hopefully this will ease the implementation.
I found a MS document that describes the order of ACEs. It differs slightly from what you found imperically. Specifically, ALL inherited ACEs follow all non-inherited ACEs. I will implement the sort routine according to the documentation, and hopefully the documentated order is acceptable. :-)
Created attachment 2866 [details]
libsmbclient patch to properly sort ACEs
Henrik, will you please try the attached patch and let me know if it solves the problems you were seeing.
You are on a coding roll today!
First smbc_removexattr and now this. You're on fire! =)
I'll do some more extensive testing tomorrow but it looks really promising.
However I got some warnings doing compile regarding pointers.
In function 'cacl_set':
libsmb/libsmbclient.c:5223: warning: passing argument 1 of 'prs_mem_free' from incompatible pointer type
libsmb/libsmbclient.c:5224: warning: passing argument 1 of 'prs_mem_free' from incompatible pointer type
libsmb/libsmbclient.c:5243: warning: passing argument 1 of 'prs_mem_free' from incompatible pointer type
libsmb/libsmbclient.c:5244: warning: passing argument 1 of 'prs_mem_free' from incompatible pointer type
I think you have an old version. Those warnings were from my initial fix for the crash in removexattr. Which svn branch are you using?
I just checked in the recent patch that you indicate works properly. "svn up" should solve the problem.
Simo and Volker feel the sorting is (at least sometimes) inappropriate. Rather than remove sorting which would break backward compatibility, look for a way to allow the knowledgeable user to specify that no sorting is to be done.
Derrell, I'm of the mind that this should be a dumb interface.
The user should have to know about this. You can of course, provide a
function ptr interface for handling sorting but ny default it
should just work. Good APIs should be hard to use incorrectly.
This fix has been in since before 3.2. Closing.