Bug 9054 - vfs_acl_xattr module in Samba 3.6.6 ignores Sgid bit of directories
Summary: vfs_acl_xattr module in Samba 3.6.6 ignores Sgid bit of directories
Status: NEW
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: File services (show other bugs)
Version: 3.6.6
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Volker Lendecke
QA Contact: Samba QA Contact
URL:
Keywords:
: 8938 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-07-19 11:25 UTC by Arvid Requate
Modified: 2016-12-05 15:20 UTC (History)
3 users (show)

See Also:


Attachments
Patch proposal (1.07 KB, patch)
2012-09-26 17:00 UTC, Arvid Requate
no flags Details
Patch proposal for samba4 git (1.42 KB, patch)
2012-09-26 17:01 UTC, Arvid Requate
no flags Details
Patch proposal for samba4 git (v 0.2) (1.42 KB, patch)
2012-09-26 17:37 UTC, Arvid Requate
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Arvid Requate 2012-07-19 11:25:10 UTC
The vfs_acl_xattr module in Samba 3.6.6 seems to disregard the Sgid bit of directories. The corresponding group is not inherited to files created in the directory via smbclient (nor is the Sgid bit inherited by directories).

In contrast default fACLs for a group are inherited properly.
Without the vfs_acl_xattr the Sgid bit is honored.
The workaround from Bug 7996 did not change the behaviour. 


### =======================
## example share definition
[vfstest]
path = /vfstest
vfs objects = acl_xattr
msdfs root = no
writeable = yes
browseable = yes
public = no
dos filemode = no
hide unreadable = no
create mode = 0744
directory mode = 0755
force create mode = 0
force directory mode = 0
security mask = 0777
directory security mask = 0777
force security mode = 0
force directory security mode = 0
locking = 1
blocking locks = 1
strict locking = 0
oplocks = 1
level2 oplocks = 1
fake oplocks = 0
csc policy = manual
nt acl support = 1
inherit acls = 1
inherit owner = no
inherit permissions = yes
### =======================



root@master:~# mkdir /vfstest/folder1
root@master:~# chgrp 'Domain Users' /vfstest/folder1/
root@master:~# chmod 2770 /vfstest/folder1/
root@master:~# cd /home/Administrator
root@master:~# smbclient //$(hostname)/vfstest -U Administrator \
                     -c 'cd folder1; put /etc/hosts file1'

root@master:~# ls -ld /vfstest/folder1/file1 
-rwxr-xr-x 1 Administrator Domain Admins 809 19. Jul 10:58 /vfstest/folder1/file1

root@master:~# getfacl /vfstest/folder1/file1 
# file: vfstest/folder1/file1
# owner: Administrator
# group: Domain\040Admins
user::rwx
group::r-x
other::r-x

root@master:~# getfattr -m '' -d /vfstest/folder1/file1
# file: vfstest/folder1/file1
security.NTACL=0sAwADAAAAAgAEAAIAAQD2GH5yPAGmXJFnegWNEWUFjTLdUd8ClSVst+HRsYZidAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAASAZAAAAIAAAAAAAAAAnAAAAAEFAAAAAAAFFQAAAEIvYDOj1mthqGnm7PQBAAABBQAAAAAABRUAAABCL2Azo9ZrYahp5uwAAgAAAgBkAAMAAAAAACQA/wEfAAEFAAAAAAAFFQAAAEIvYDOj1mthqGnm7PQBAAAAACQAqQASAAEFAAAAAAAFFQAAAEIvYDOj1mthqGnm7AACAAAAABQAqQASAAEBAAAAAAABAAAAAA==
Comment 1 Arvid Requate 2012-09-26 17:00:03 UTC
Created attachment 7940 [details]
Patch proposal

This patch for the inherit_new_acl Funktion in vfs_acl_common.c adds a check for the SGID bit on the parent folder. If it is found, the group_sid is inherited from the parent.
Comment 2 Arvid Requate 2012-09-26 17:01:51 UTC
Created attachment 7941 [details]
Patch proposal for samba4 git

In current Samba4 git the function moved to source3/smbd/open.c
Comment 3 Arvid Requate 2012-09-26 17:37:21 UTC
Created attachment 7942 [details]
Patch proposal for samba4 git (v 0.2)
Comment 4 Andrew Bartlett 2012-10-02 06:10:12 UTC
(In reply to comment #3)
> Created attachment 7942 [details]
> Patch proposal for samba4 git (v 0.2)

At first glance, the patch seems reasonable (TODOs aside), but as I dig into it some more, I have concerns.

The desire to have sgid means something even when we have full NT ACL semantics is a good one, but if we are to do that, perhaps we should directly map the owning group from the directory back into a SID, rather than assume that the SID matches in the NT ACL?

There are reasons, such as either an ACL being set with the ntvfs file server or the SID <-> GID mapping not being one-to-one that these may not match up directly.  I'm not sure if these matter in the real world.

In general, I'm also a little concerned about possible races here - ACL inheritance in user-space is a dodgy thing at the best time, much like ACL evaluation. 

How would you propose this interacts with 'inheirt owner', as it seems that inherit owner overrides this?
Comment 5 Andrew Bartlett 2012-10-02 06:16:10 UTC
One other thing:  What is the proposed interaction between this and a stored NT ACL?

The reason I ask is that I'm proposing to change vfs_acl_xattr to hash an unmapped POSIX ACL (and file owner/mode).  That would mean that setting an SGID on a directory would invalidate the NT ACL (falling back to a mapped POSIX ACL), as it indicates that a change was made on the POSIX side.  Would this match the proposed use case?
Comment 6 Arvid Requate 2012-10-08 16:47:13 UTC
> The desire to have sgid means something even when we have full NT ACL semantics
> is a good one, but if we are to do that, perhaps we should directly map the
> owning group from the directory back into a SID, rather than assume that the
> SID matches in the NT ACL?
> 
> There are reasons, such as either an ACL being set with the ntvfs file server
> or the SID <-> GID mapping not being one-to-one that these may not match up
> directly.  I'm not sure if these matter in the real world.

Maybe, this is true. I simply followed the coding pattern of "inherit owner". Maybe that would have to be fixed then as well. If we would lookup the GID from the parent directory and map the SID again, then we would have to update it in parent_desc->group_sid  for se_create_child_secdesc as well? I would have guessed that the parent_desc->group_sid is authoritative. At least that's what is used for calculation of the security descrption in se_create_child_secdesc a few lines later.

> In general, I'm also a little concerned about possible races here - ACL
> inheritance in user-space is a dodgy thing at the best time, much like ACL
> evaluation. 

I agree, but this is a general problem pertatining also to the inheritance of pure NTACLs?

> How would you propose this interacts with 'inheirt owner', as it seems that
> inherit owner overrides this?

Yes, I would follow the pattern of samba options beeing authoritative.
Comment 7 Arvid Requate 2012-10-08 16:58:17 UTC
> One other thing:  What is the proposed interaction between this and a stored
> NTACL?
> 
> The reason I ask is that I'm proposing to change vfs_acl_xattr to hash an
> unmapped POSIX ACL (and file owner/mode).  That would mean that setting an SGID
> on a directory would invalidate the NT ACL (falling back to a mapped POSIX
> ACL), as it indicates that a change was made on the POSIX side.  Would this
> match the proposed use case?

inherit_new_acl finally calls SMB_VFS_FSET_NT_ACL and the implementation fset_nt_acl_common updates the hash. So I think the proposed approach to this bug would play nicely with this?

Since inherit_new_acl currently is only called by create_file_unixpath (or create_file_acl_common in samba 3.6.6), parent-SGID would only be considered during file creation.
Comment 8 Andrew Bartlett 2012-10-16 07:16:58 UTC
I was more thinking about how we get the sgid there in the first place, that is by changing the posix permission on the directory.
Comment 9 Björn Jacke 2016-12-05 15:20:30 UTC
*** Bug 8938 has been marked as a duplicate of this bug. ***