Bug 9462 - Users can not be given write permissions any more by default
Summary: Users can not be given write permissions any more by default
Alias: None
Product: Samba 4.0
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.0.0rc5
Hardware: All All
: P5 regression (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
Depends on:
Blocks: 8622
  Show dependency treegraph
Reported: 2012-12-04 22:48 UTC by Michael Adam
Modified: 2013-01-17 12:29 UTC (History)
5 users (show)

See Also:

git-am fixes for 4.0.0 (10.54 KB, patch)
2012-12-04 23:47 UTC, Jeremy Allison
no flags Details
git-am fix for master and 4.0.0 (9.03 KB, patch)
2012-12-05 20:25 UTC, Jeremy Allison
no flags Details
my version of a patch for master (4.79 KB, patch)
2012-12-05 20:42 UTC, Michael Adam
no flags Details
patchset for 4.0 (20.92 KB, patch)
2012-12-05 21:05 UTC, Michael Adam
no flags Details
final patchset for master (11.63 KB, patch)
2012-12-06 00:10 UTC, Michael Adam
obnox: review+
final patchset for v4-0-test (27.75 KB, patch)
2012-12-06 00:13 UTC, Michael Adam
no flags Details
final. corrected patchset for v4-0-test (33.47 KB, patch)
2012-12-06 10:07 UTC, Michael Adam
obnox: review+
metze: review+

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Adam 2012-12-04 22:48:31 UTC
I observed the following regression in current master and 4.0 code:

With the default settings, one can not give write permissions (in acls) to other users than the owner.

In commit c251a6b0442abc13bc8be4ff8de324c1d7706a78 in master,
the use of the security mask (and directory security mask) in the
code for calculating posix acls were replaced by uses of create mask
and directory mask. Therefore all additional aces were restricted by
the create mask and directory mask, respectively.

Now the defaults of these are set to 0744 and 0755, respectively,
so one can only set r-- or r-x permissions to named aces via the
nt_set_acl calls.

This does not match the original intention of the create/directory mask
parameters, which was to only restrict the maximum rights for newly
created files (like the umask in unix).

Currently the only workaround would be to set create mask and
directory mask to 0777, therenby making all new files "shared by default".

We should change this to only apply the create mask/directory mask
values when the set_nt_acl code is called as part of a create operation
(as opposed to an explicit setting set secdesc call).

For 4.0, this will probably need to be a slightly hacky patch, because
any conceptually better change I currently can imagine, would require
adding a means to hand in to the SMB_VFS_FSET_NT_ACL() call the
information that the call is done as part of a create operation.
Comment 1 Jeremy Allison 2012-12-04 23:02:33 UTC
Hideous patch forthcoming :-).
Comment 2 Jeremy Allison 2012-12-04 23:47:35 UTC
Created attachment 8280 [details]
git-am fixes for 4.0.0

Patch that works here. Yes, I know how ugly this is (you don't need to point that out). Merely submit a better fix if you can see one :-).


Comment 3 Stefan Metzmacher 2012-12-05 14:14:35 UTC
What about using no mask at all?

If we'll have a NTACL we'll need to have a working posix acl.
Comment 4 Michael Adam 2012-12-05 17:05:14 UTC
I am currently doing more testing with this patch and working on an alternative patch:

Metze is right that this change changes the result of the NT-ACL-->posixACL
mapping depending on whether we are in a create or not. This has impact on
the acl_xattr functionality!

Thinking a bit about it:

Why don't we let the mask parameters only in the original posix file creation (open_file()). As long as the share is posix-permissions only (no acls), the
mask will stick. But as soon as we have ACLS / inheritance, we should not
apply the masks, also not in create: The acls are there for a purpose.
We should treat them correctly. Note that currently, we only apply the
masks for acls when they come through inheritance, _not_ in the case where
the client specifies an acl on create. This is also inconsistent.

Currently testing a patch that does precisely this... more later...
Comment 5 Jeremy Allison 2012-12-05 19:32:23 UTC
I don't understand some of the comments. In the default (non-create) case we have no mask at all. Can you explain more fully please ?

I understand the inconsistency when create + ACL is done, yes - we should ensure the masks are applied there.

Comment 6 Jeremy Allison 2012-12-05 20:07:27 UTC
Actually, I've been thinking about this further, and I finally understand metze's somewhat cryptic comment :-).

Probably not using masks at all is the best solution. I'll update a new patchset. Applying the masks on create+SD is also the wrong thing to do (I'm reversing my original opinion here).

Comment 7 Jeremy Allison 2012-12-05 20:25:38 UTC
Created attachment 8286 [details]
git-am fix for master and 4.0.0

I think this is all we need to do this right..

Comment 8 Michael Adam 2012-12-05 20:42:33 UTC
Created attachment 8287 [details]
my version of a patch for master

This is the my version of the change I have been working on earlier today.

It looks bigger but is similar to jeremy's patch.

But it reduces the apply_default_perms() to two specialized functions:
- ensure_minimal_owner_ace_perms() which does what jeremy's patch changes apply_default_perms() to do, but reduces the argument list.
- trim_ace_perms() that does the masking of the ace perms so that they only contain the USR portion of the access bits (not 100% certain whether this is needed at all).

Then the places where these specialized functions are called are cleaned.
Comment 9 Michael Adam 2012-12-05 21:05:20 UTC
Created attachment 8288 [details]
patchset for 4.0

Patchset for 4.0.

This also backports the prerequisite patch
"Modify ensure_canon_entry_valid() into ensure_canon_entry_valid_on_set() - makes the logic clearer."

Still testing / verifying.
Comment 10 Michael Adam 2012-12-06 00:10:33 UTC
Created attachment 8289 [details]
final patchset for master

This is the final patchset for master including documentation fix after testing and discussion with Jeremy.

Jeremy: Please push to master if you agree.
Comment 11 Michael Adam 2012-12-06 00:13:49 UTC
Created attachment 8290 [details]
final patchset for v4-0-test

Final patchset for 4.0 after discussion with Jeremy and testing.
This picks one additional prerequisite patch and includes the docs patch.
Comment 12 Michael Adam 2012-12-06 09:31:26 UTC
Comment on attachment 8290 [details]
final patchset for v4-0-test

this patch accidentially reverts one change that had been applied in v4-0-test already but came after the "ensure_canon_entry_valid_on_set()" patch in master. 

correct patch following...
Comment 13 Michael Adam 2012-12-06 10:07:49 UTC
Created attachment 8292 [details]
final. corrected patchset for v4-0-test

The original patchset accidentially introduced a revert commit e122c7d24b10119c9ea4d65e0099ff1690394457 in conflict resolution.
The reason was that the patches had been applied in different order.

For my patchset, I need to cherry-pick patch 47ebc8fbc93ee1eb9640d9ca30275fcfc3b50026 which creates ensure_canon_entry_valid_on_set(). Originally with conflict resolution.
This accidentially introduced an implicit revert of commit e122c7d24b10119c9ea4d65e0099ff1690394457.
The reason was that the patches had been applied in different order.
This led to the creation of bug #9469...

This new patchset first reverts 2 patches that had been applied in wrong order, then cleanly cherry-picks the ensure_canon_entry_valid_on_set() patch and the two reverted ones again, cleanly. Then the two patches for this bug are cleanly cherry-picked from master on top.

The diff of the resulting branch against v4-0-test with the old patchset is exaclty the accidentially reverted patch e122c7d24b10119c9ea4d65e0099ff1690394457, so this one is correct.
Comment 14 Stefan Metzmacher 2012-12-06 15:53:15 UTC
Comment on attachment 8292 [details]
final. corrected patchset for v4-0-test

Looks good
Comment 15 Stefan Metzmacher 2012-12-06 15:53:53 UTC
Karolin, please pick this to v4-0-test
Comment 16 Karolin Seeger 2012-12-07 08:30:09 UTC
Pushed to autobuild-v4-0-test.
Comment 17 Karolin Seeger 2012-12-07 10:58:11 UTC
Pushed to v4-0-test.
Closing out bug report.