Bug 6865 - acl_xattr module: Has dependency that inherit acls = yes or xattrs are removed
acl_xattr module: Has dependency that inherit acls = yes or xattrs are removed
Status: RESOLVED FIXED
Product: Samba 3.4
Classification: Unclassified
Component: VFS Modules
3.4.1
Other Windows XP
: P3 normal
: ---
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-01 22:49 UTC by Barry Sabsevitz
Modified: 2009-11-20 16:25 UTC (History)
1 user (show)

See Also:


Attachments
strace output shows Samba removing xattr that stores Windows ACL when I add a new user ACL to the folder. (149.63 KB, text/plain)
2009-11-01 23:20 UTC, Barry Sabsevitz
no flags Details
Patch for 3.5.0 and master (2.93 KB, patch)
2009-11-06 23:52 UTC, Jeremy Allison
no flags Details
Patch to overlay previous patch. (3.13 KB, patch)
2009-11-11 20:34 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Barry Sabsevitz 2009-11-01 22:49:05 UTC
While testing the acl_xattr module, it was found that if you don't have inherit acls = yes in the smb.conf file, you can lose the xattr that stores the NT ACL permissions. 

To reproduce:
1. Create a folder
2. Change the permissions on all ACLs on that folder to have read and list folder rights.
3. Add a second user ACL to this folder and also give read and list folder rights.
4. Copy an executable into this folder (i.e. wordpad.exe)

You will see the xattr on wordpad.exe gets created and then removed. The reason for this is:

> The routine open_file_ntcreate() is invoked and invokes the following code:
> 	 SMB_ASSERT(!file_existed || (lck != NULL));
> 
>  /*
>   * Ensure we pay attention to default ACLs on directories if required.
>   */
> 
>  if ((flags2 & O_CREAT) && lp_inherit_acls(SNUM(conn)) && 
>      (def_acl = directory_has_default_acl(conn, parent_dir))) {
>          unx_mode = 0777;
>  }
> 
> Before this block of code def_acl was 0. And since I didn't have inherit acls set to yes in my smb.conf file, this code block doesn't execute. Later we execute the following code in open_file_ntcreate():
>   /*
>    * Take care of inherited ACLs on created files - if default ACL not
>    * selected.
>    */
> 
>   if (!posix_open && !file_existed && !def_acl) {
> 
>           int saved_errno = errno; /* We might get ENOSYS in the next
>                                     * call.. */
> 
>           if (SMB_VFS_FCHMOD_ACL(fsp, unx_mode) == -1 &&		
>               errno == ENOSYS) {
>                   errno = saved_errno; /* Ignore ENOSYS */
>           }
> 
>   } else if (new_unx_mode) {
> 
>           ...
> 
> Since file_existed is 0 because we are creating wordpad.exe by copying it, and posix_open is 0, and def_acl is 0 (because the first block of code never executed), we enter this block of code and invoke SMB_VFS_CHMOD_ACL(). The call to SMB_VFS_FCHMOD_ACL() invokes the routine fchmod_acl() which in turn invokes sys_acl_set_fd_xattr() in the vfs_acl_xattr module. This routine removes the Windows NT ACL XATTR and then replaces it with a POSIX ACL in an XATTR.

I talked to Jeremy Allison about this and his initial thoughts were that there is a dependency between the acl_xattr module and inherit acls set to yes in the smb.conf file. He was thinking the solution may be to have the acl_xattr module force the setting of inherit acls to yes for the share(s) that use the acl_xattr module.

I verified that by setting inherit acls to yes in the global section of my smb.conf file, this problem disappears.

This problem occurs with Samba 3.4.1 but may also exist in other Samba releases.
Comment 1 Barry Sabsevitz 2009-11-01 23:17:12 UTC
Please note: The behavior where setting inherit acls = yes fixes the problem, occurs because of the change made in Bug # 6802 where we mark the default ACLs on the directory as SEC_ACE_FLAG_OBJECT_INHERIT|SEC_ACE_FLAG_CONTAINER_INHERIT. This causes the code to not invoke the routine that writes a POSIX ACL to the file and removes the NT ACLs.

IMPORTANT:
But, I notice another potential problem. In Samba, anytime I add a user ACL to a folder (i.e. via Windows explorer), Samba tries to add a POSIX ACL and removes the XATTR that has the NT ACL on that folder and all files underneath it. Most of the time, somewhere in Samba, the NT ACL is re-added, so we don't notice the problem. So this begs the question as to whether the acl_xattr module should remove the xattr that stores the Windows ACL when a POSIX ACL is added? I have attached an strace file that occured when I used Windows explorer to add a user ACL to the folder tmp-3. In that folder at the time was 1 file called file1. You will see that Samba added a POSIX ACL to tmp-3, removed the NT ACLs from tmp-3, and then re-added the NT ACL to tmp-3. It did the same thing to the file "file1". 
Comment 2 Barry Sabsevitz 2009-11-01 23:20:40 UTC
Created attachment 4912 [details]
strace output shows Samba removing xattr that stores Windows ACL when I add a new user ACL to the folder.

The attachment is strace output of the samba daemon when I add a new user ACL to a folder. It shows Samba setting a POSIX ACL on the folder and the files beneath it and as a side-effect it removes the xattr that stores the Windows ACLs. It seems to be benign because somewhere in Samba (either by the client re-issuing the request or some code path that I haven't found yet), the Windows NT ACL is re-added. But it begs the question as to whether we should remove the xattr that stores the Windows ACL when a POSIX ACL is added to a folder or file.
Comment 3 Barry Sabsevitz 2009-11-01 23:23:43 UTC
The samba config file I was using was:
clustering = no
inherit acls = yes
workgroup = ACTIVEDIR
security = ADS
password server = 10.30.252.84
realm = ACTIVEDIR.NET
disable netbios = yes
client ntlmv2 auth = yes
winbind use default domain = yes
winbind separator = +
template shell = /bin/bash
client schannel = no
winbind enum users = yes
client use spnego = yes
dns proxy = no
client signing = mandatory
encrypt passwords = yes
winbind enum groups = yes
server signing = mandatory
allow trusted domains = no
winbind nested groups = yes
vfs objects = acl_xattr
idmap config ACTIVEDIR:backend = rid
idmap config ACTIVEDIR:range = 20000000 - 30000000
idmap uid = 20000000-30000000
idmap gid = 20000000-30000000
[barry]
path = /cifs_mnt
read only = no
Comment 4 Jeremy Allison 2009-11-06 23:52:39 UTC
Created attachment 4926 [details]
Patch for 3.5.0 and master

Here's a patch I'm adding to 3.5.0 and master. Please review and confirm it works for you. Thanks !

Jeremy.
Comment 5 Barry Sabsevitz 2009-11-10 20:34:19 UTC
Hi Jeremy, sorry it took so long. I will test this tomorrow and hopefully get back to you by the end of the day.
Comment 6 Barry Sabsevitz 2009-11-10 20:44:28 UTC
Jeremy, I also noticed that when trying to "take permission" by doing the following:

1. Create a folder as user 1.
2. Set ACL on folder to give full control to user 2.
3. Login to Samba share as user 2.
4. Try to take ownership of the folder by using windows explorer to change the owner from user 1 to user 2.

This fails unless we set dos filemode = yes in the smb.conf file. Should there also be a dependency on vfs_acl_xattr module that dos filemode = yes should also be forced?

The routine try_chown() shows you that you will fail (case 4) towards the bottom of the routine if dos filemode = yes is not set.
Comment 7 Jeremy Allison 2009-11-11 20:00:35 UTC
Yes, I'll add forcing "dos filemode = yes" as well when these modules are loaded.
Thanks !
Jeremy.
Comment 8 Jeremy Allison 2009-11-11 20:34:17 UTC
Created attachment 4953 [details]
Patch to overlay previous patch.

Add this patch on top of the other one (will overlay it). We don't need custom functions to set parameters.
Jeremy.
Comment 9 Jeremy Allison 2009-11-20 16:08:09 UTC
Can you confirm that this fixes the problem please ? If so I'll mark this one closed.
Jeremy.
Comment 10 Barry Sabsevitz 2009-11-20 16:13:57 UTC
Jeremy, we took a variant of your patch but due to an internal scheduling issue we couldn't take your patch exactly as you wrote it. The variant that we took was probably 95% close to what you wrote and it did fix our problem.

Thanks.
Comment 11 Jeremy Allison 2009-11-20 16:25:42 UTC
Fixed in 3.5.0.
Jeremy.