Bug 9190 - Regression (change in behavior) of default acl masks
Regression (change in behavior) of default acl masks
Status: RESOLVED FIXED
Product: Samba 4.0
Classification: Unclassified
Component: File services
4.0.0rc1
All All
: P5 regression
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-21 09:10 UTC by Andreas Schneider
Modified: 2012-10-29 10:54 UTC (History)
4 users (show)

See Also:


Attachments
git-am fix for 4.0.0rc3 (11.73 KB, patch)
2012-10-02 18:50 UTC, Jeremy Allison
no flags Details
git-am fix for 4.0.0rc3 (29.86 KB, patch)
2012-10-04 18:37 UTC, Jeremy Allison
no flags Details
git-am fix for 4.0.0rc3 (30.92 KB, patch)
2012-10-04 19:08 UTC, Jeremy Allison
no flags Details
git-am fix for 4.0.0rc3 (32.19 KB, patch)
2012-10-04 20:56 UTC, Jeremy Allison
asn: review+
jra: review? (ddiss)
metze: review+
Details
Documentation fixes on top of attachment 7993 (2.54 KB, patch)
2012-10-10 06:50 UTC, Stefan Metzmacher
kseeger: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Schneider 2012-09-21 09:10:05 UTC
Description of problem:

This bug is confirmed to exist in 3.6.x and 3.5.10. It worked correctly in 3.5.4 and is fixed in 4.0.

If you set a default ACL for a group to be rwx and create the directory with mkdir on a shell the group is granted rwx. If you do the same but connect to a samba share the effective rights will be r-x. Cause the mask is not rwx it is r-x.

How to reproduce:

magrathea:~ # mkdir /tmp/samba
magrathea:~ # getfacl /tmp/samba
getfacl: Removing leading '/' from absolute path names
# file: tmp/samba
# owner: root
# group: root
user::rwx
group::r-x
other::r-x

magrathea:~ # setfacl -m u:bob:rwx /tmp/samba/
magrathea:~ # setfacl -d -m u:bob:rwx /tmp/samba/
magrathea:~ # setfacl -m g:users:rwx /tmp/samba/
magrathea:~ # setfacl -d -m g:users:rwx /tmp/samba/
magrathea:~ # getfacl /tmp/samba/
getfacl: Removing leading '/' from absolute path names
# file: tmp/samba/
# owner: root
# group: root
user::rwx
user:bob:rwx
group::r-x
group:users:rwx
mask::rwx
other::r-x
default:user::rwx
default:user:bob:rwx
default:group::r-x
default:group:users:rwx
default:mask::rwx
default:other::r-x

Switch to user bob:
magrathea:~ # su - bob
bob@magrathea:~> mkdir /tmp/samba/bob_creates_dir_with_unix
bob@magrathea:~> getfacl /tmp/samba/bob_creates_dir_with_unix
getfacl: Removing leading '/' from absolute path names
# file: tmp/samba/bob_creates_dir_with_unix
# owner: bob
# group: users
user::rwx
user:bob:rwx
group::r-x
group:users:rwx
mask::rwx
other::r-x
default:user::rwx
default:user:bob:rwx
default:group::r-x
default:group:users:rwx
default:mask::rwx
default:other::r-x

Now lets do the same with Samba:
smb.conf -> http://xor.cryptomilk.org/samba/smb.conf

bob@magrathea:~> smbclient //localhost/guru -Ubob%secret -c "mkdir bob_creates_dir_with_samba"
Domain=[DISCWORLD] OS=[Unix] Server=[Samba 3.6.8-GIT-64c7d3e-test]
bob@magrathea:~> getfacl /tmp/samba/bob_creates_dir_with_samba/
getfacl: Removing leading '/' from absolute path names
# file: tmp/samba/bob_creates_dir_with_samba/
# owner: bob
# group: users
user::rwx
user:bob:rwx                    #effective:r-x
group::r-x
group:users:rwx                 #effective:r-x
mask::r-x
other::r-x
default:user::rwx
default:user:bob:rwx
default:group::r-x
default:group:users:rwx
default:mask::rwx
default:other::r-x
Comment 1 Jeremy Allison 2012-09-28 19:21:16 UTC
Confirmed reproduced with 3.6.current.

Marking as regression to make sure I fix this before next release.

Jeremy.
Comment 2 Jeremy Allison 2012-09-28 20:35:54 UTC
Ok, this is a config error, not a bug (yeah, I know this is complex and sucks).

It's a side effect of the fix for bug #7734.

The problem in #7734 was that we were not applying the create masks to files created with "inherit acls".

Check out the smb.conf definitions for :

create mask
directory mask

These specify masks that are applied to *ALL* files or directories created by smbd through a Windows (non-unix extensions) connection. People take it on trust that any file or directory created by Samba obeys these masks - they allow those masks to be set on a share definition, and the admin *knows* that no files will violate those masks.

So the earlier 3.5.x we considered a bug. You could argue whether "inherit acls" should take precedence over "create mask" or "directory mask" but we decided to make the mask parameters take precedence.

So to fix this you need to set:

create mask = 0777
directory mask = 0777

in the share definition of your smb.conf and then the inherited ACL will behave as you expect.

Now I need to check into the 4.0.0rc1 code and see why it's *not* doing the same :-).

Cheers,

Jeremy.
Comment 3 Jeremy Allison 2012-09-28 20:38:41 UTC
Actually, thinking about it some more I think we also apply these masks to any POSIX (unix extensions) create files or directories as well - so they really are an absolute mask on anything Samba creates.
Comment 4 Jeremy Allison 2012-09-28 22:26:14 UTC
Ok, I've figured out why it works on 4.0.0rc and above.

We do an extra step in 4.0, as we only deal with the NT ACL in the core fileserver code now - the POSIX ACL layer is hidden below the VFS.

So rather than doing the NT ACL inheritance when creating a new object inside the Windows ACL vfs (i.e. the acl_xattr or acl_tdb layer) we create the file (which leaves the mask r-x as expected) - but then we do an additional Windows ACL set *afterwards*.

The Windows ACL set here doesn't look at the create mask or directory mask parameters as it's considered modifying the ACL on an already existing file.

The problem is as the create file/directory isn't atomic, we don't have a way to tell the NT -> POSIX mapping layer to treat this as an ACL set on file create, rather than an ACL modify (which uses the "[directory] security mask" parameter instead).

Oh. I've thought of a really ugly way to fix this...

Inside inherit_new_acl() we save off the original "[directory] security mask" settings and replace them with the settings of "create mask" and "directory mask" before calling the SMB_VFS_FSET_NT_ACL() call...

Patch to follow.

Jeremy.
Comment 5 Jeremy Allison 2012-09-29 00:17:47 UTC
Ok, I'm moving this to a 4.0.0rc1 bug. We're actually not enforcing the correct masks on the ACL set.

Patch to follow.

Jeremy.
Comment 6 Jeremy Allison 2012-09-29 00:23:11 UTC
There's also a subsidiary bug in that in 3.5.x and 3.6.x we don't apply the security masks to the SMB_ACL_USER or SMB_ACL_GROUP entries in the POSIX ACL when setting it. I'll upload separate patches for that.

Jeremy.
Comment 7 Andreas Schneider 2012-10-01 10:35:17 UTC
Thank you very much Jeremy for looking into this. Could you please open another bug for the 3.6 and 3.5 security mask fixes and keep me in the loop?

Thanks!
Comment 8 Jeremy Allison 2012-10-02 18:50:36 UTC
Created attachment 7979 [details]
git-am fix for 4.0.0rc3

Here is the patchset that went into master. It's broken down into micro-patches so you can see what is going on.

With this fix the create mask/directory mask will be applied to all files/directories created by smbd.

Jeremy.
Comment 9 Jeremy Allison 2012-10-03 00:29:51 UTC
(In reply to comment #7)
> Thank you very much Jeremy for looking into this. Could you please open another
> bug for the 3.6 and 3.5 security mask fixes and keep me in the loop?
> 
> Thanks!

Done:

https://bugzilla.samba.org/show_bug.cgi?id=9236

Jeremy.
Comment 10 Jeremy Allison 2012-10-03 22:10:49 UTC
Comment on attachment 7979 [details]
git-am fix for 4.0.0rc3

Remove review - goal is to remove these parameters for 4.0.0rc3.
Comment 11 Jeremy Allison 2012-10-04 18:37:21 UTC
Created attachment 7991 [details]
git-am fix for 4.0.0rc3

Removes :

security mask
force security mode
directory mask
force directory security mode

parameters.
Comment 12 Jeremy Allison 2012-10-04 19:08:48 UTC
Created attachment 7992 [details]
git-am fix for 4.0.0rc3

Got list of removed parameters right in commit message :-).

Added update to WHATSNEW.txt.
Comment 13 Jeremy Allison 2012-10-04 20:56:37 UTC
Created attachment 7993 [details]
git-am fix for 4.0.0rc3

Ho hum, make sure the masks are set right for the acl make test also.
Comment 14 Stefan Metzmacher 2012-10-10 06:50:38 UTC
Created attachment 8033 [details]
Documentation fixes on top of attachment 7993 [details]
Comment 15 Andreas Schneider 2012-10-11 07:40:51 UTC
Comment on attachment 7993 [details]
git-am fix for 4.0.0rc3

This looks fine for me, but I would like to have another reviewer giving + too.
Comment 16 Jeremy Allison 2012-10-11 15:39:43 UTC
Thanks. I'll wait until David or Metze give it their blessing.

Jeremy.
Comment 17 Stefan Metzmacher 2012-10-24 08:05:18 UTC
Comment on attachment 7993 [details]
git-am fix for 4.0.0rc3

Looks ok
Comment 18 Stefan Metzmacher 2012-10-24 08:06:12 UTC
Karolin, please pick this and review the docs fix
Comment 19 Karolin Seeger 2012-10-29 10:54:48 UTC
Pushed to autobuild-v4-0-test.
Closing out bug report.

Thanks!