Bug 4652 - smbc_setxattr doesn't sort the acls correctly
smbc_setxattr doesn't sort the acls correctly
Status: CLOSED FIXED
Product: Samba 3.0
Classification: Unclassified
Component: libsmbclient
3.0.25
Other Other
: P3 normal
: none
Assigned To: Derrell Lipman
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-23 09:37 UTC by Henrik
Modified: 2007-12-12 21:04 UTC (History)
0 users

See Also:


Attachments
libsmbclient patch to properly sort ACEs (3.63 KB, patch)
2007-08-15 09:56 UTC, Derrell Lipman
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Henrik 2007-05-23 09:37:26 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.

Regards,
Henrik
Comment 1 Derrell Lipman 2007-08-13 13:16:32 UTC
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?

Thanks,

Derrell
Comment 2 Henrik 2007-08-14 02:45:15 UTC
Hi Derrell,

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.
REVISION:1,OWNER:BBI-DEV\Admin,GROUP:BBI-DEV\Ingen,ACL:BBI-DEV\Lena:0/0/0x001f01ff,ACL:BUILTIN\Administratörer:0/16/0x001f01ff,ACL:BUILTIN\Användare:0/16/0x001200a9,ACL:BBI-DEV\Admin:0/16/0x001f01ff,ACL:NT INSTANS\SYSTEM:0/16/0x001f01f

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.
REVISION:1,OWNER:BBI-DEV\Admin,GROUP:BBI-DEV\Ingen,ACL:NT INSTANS\SYSTEM:0/16/0x0001001f,ACL:BUILTIN\Administratörer:0/16/0x001f01ff,ACL:BUILTIN\Användare:0/16/0x001200a9,ACL:BBI-DEV\Admin:0/16/0x001f01ff,ACL:BBI-DEV\Lena:0/0/0x001f01f

After Windows Explorer has sorted them I used smbc_getxattr to retrieve them again and now Lena is sorted to the beginning again.
REVISION:1,OWNER:BBI-DEV\Admin,GROUP:BBI-DEV\Ingen,ACL:BBI-DEV\Lena:0/0/0x0011001f,ACL:BUILTIN\Administratörer:0/16/0x001f01ff,ACL:BUILTIN\Användare:0/16/0x001200a9,ACL:BBI-DEV\Admin:0/16/0x001f01ff,ACL:NT INSTANS\SYSTEM:0/16/0x001f01f

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.

Cheers 
/Henrik


Comment 3 Henrik 2007-08-14 03:00:58 UTC
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.

Cheers,
/Henrik
Comment 4 Derrell Lipman 2007-08-15 09:08:30 UTC
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. :-)

Reference: http://support.microsoft.com/kb/269175
Comment 5 Derrell Lipman 2007-08-15 09:56:56 UTC
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.

Thanks,

Derrell
Comment 6 Henrik 2007-08-15 11:18:20 UTC
Hi Derrell,

It works!

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


Cheers,
Henrik
Comment 7 Derrell Lipman 2007-08-15 12:42:31 UTC
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.

Cheers,

Derrell
Comment 8 Derrell Lipman 2007-08-15 16:05:54 UTC
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.
Comment 9 Gerald (Jerry) Carter 2007-08-15 16:16:25 UTC
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.
Comment 10 Derrell Lipman 2007-12-12 20:57:57 UTC
This fix has been in since before 3.2.  Closing.
Comment 11 Derrell Lipman 2007-12-12 21:04:39 UTC
Closed