Bug 6874 - Setting ACL from NT client fails when it should pass
Setting ACL from NT client fails when it should pass
Product: Samba 3.4
Classification: Unclassified
Component: File services
Other Windows XP
: P3 normal
: ---
Assigned To: Volker Lendecke
Samba QA Contact
Depends on:
  Show dependency treegraph
Reported: 2009-11-05 21:53 UTC by Barry Sabsevitz
Modified: 2009-11-15 18:33 UTC (History)
1 user (show)

See Also:

Potential patch to fix this bug (756 bytes, patch)
2009-11-05 22:14 UTC, Barry Sabsevitz
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-05 21:53:53 UTC
When using Samba 3.4.1, with the vfs_acl_xattr module, the following fails which should pass based on trying this on Windows:

1. Create a folder as user 1.
2. Create an ACL for user2 that gives user2 the ability to add/remove permissions (WRITE_DACL is set).
3. Login to Samba share as user2 and try to add or remove an ACL.

This will fail.

The root cause is:
1. The fset_nt_acl_xattr() is invoked but then immediately passes this 
request off to any other modules that want to process it via a call to SMB_VFS_NEXT_FSET_NTACL()
2. The posix module (vfs_posixacl.c) receives the request and issues the acl_set_file() library call from the function posixacl_sys_acl_set_file(). This fails because samba is issuing this request using the uid of user2 which is neither the owner of the folder nor root.
3. Because the POSIX module fails the request, the NT module fails.
Comment 1 Barry Sabsevitz 2009-11-05 21:55:26 UTC
I can't take credit for this fix, this fix comes from Jeremy Allison. Here is his response to an e-mail that I sent him:

This is one of those issues where the POSIX security model comes into conflict with the Windows one.

Under POSIX, only the owner of a file or root can modify the ACL. On Windows, anyone with WRITE_DAC access can do so.

The reason that the NT module passes the incoming SD into the underlying layer is to ensure there is an acl to hash to detect underlying layer change.

This is also why setting a POSIX acl deletes the NT ACL xattr on set - so we can detect if a client (or local process) has modified the underlying posix ACL so that the stored NT ACL no longer accurately reflects the mapping.

It's a bug I think. The correct way to fix this is inside the function smbd/nttrans.c:set_sd() to ensure the stored access_mask in the fsp handle struct has the WRITE_DAC bit set, and arbitrarily refuse the ACL set if not. If it does have WRITE_DAC set then wrap the SMB_VFS_FSET_NT_ACL call in an become_root()/unbecome_root() pair.

Comment 2 Barry Sabsevitz 2009-11-05 22:14:31 UTC
Created attachment 4921 [details]
Potential patch to fix this bug

This patch is Jeremy Allison's fix that he sent me as a possible fix to this problem. It is a potential fix to this bug. I verified that it fixes my problem but additional testing may be warranted.
Comment 3 Barry Sabsevitz 2009-11-10 16:55:26 UTC
There is a problem with the above patch. I will upload a new one as soon as it is tested better. The issue is that when a request comes into samba to change the owner on a file, we don't check the SEC_STD_WRITE_DAC bit, we need to check the SEC_STD_WRITE_OWNER bit.

So my proposed code is something like as follows:



if (security_info_sent has OWNER_SECURITY_INFORMATION) 
     check SEC_STD_WRITE_OWNER bit is set in fsp->access_mask;

/* ???? -> does write owner bit apply to group security information too */
if (security_info_sent has GROUP_SECURITY_INFORMATION)
    check SEC_STD_WRITE_OWNER bit is set in fsp->access_mask;

if (security_info_sent has DACL_SECURITY_INFORMATION) 
    check SEC_STD_WRITE_DAC bit is set in fsp->access_mask;

/* Does SACLs use the SEC_STD_WRITE_DAC bit????? */
if (security_info_sent has SACL_SECURITY_INFORMATION)
    check SEC_STD_WRITE_DAC bit is set in fsp->access_mask;

if above checks fail



become_root()    <--- new code
status = SMB_VFS_FSET_NT_ACL(fsp, security_info_sent, psd);
unbecome_root()  <---- new code
Comment 4 Barry Sabsevitz 2009-11-11 00:00:08 UTC
I don't think we can wrap the SMB_VFS_FSET_NT_ACL with a become_root because this path is also used for the chown code path. If we wrap this with root, we lose a lot of protections in try_chown(). Instead, I'm wondering if the fix for this is to wrap the call to acl_set_file() with become_root/unbecome_root(). We already passed all the protection checks by the time we get here so wrapping this should be safe. Additionally, we use Jeremy's suggestion to check the proper ACLs in set_sd(), see comment #3.
Comment 5 Barry Sabsevitz 2009-11-15 18:33:41 UTC
I think I filed a bad bug here. I think the root cause of my issue was that I did not have "dos filemode = yes" set in the smb.conf file. I'm going to close this as a bad defect and will re-open if it is a real problem. Looks like with "dos filemode = yes", the right thing happens.