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.
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".
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.
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
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.
Hi Jeremy, sorry it took so long. I will test this tomorrow and hopefully get back to you by the end of the day.
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.
Yes, I'll add forcing "dos filemode = yes" as well when these modules are loaded. Thanks ! Jeremy.
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.
Can you confirm that this fixes the problem please ? If so I'll mark this one closed. Jeremy.
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.
Fixed in 3.5.0. Jeremy.