Samba 3.6.1 configured with default POSIX ACL permissions mapping. Consider a file with the following POSIX ACL, note the entry for "other" granting explicit execute permission. sneezy:~ # getfacl /mnt/file.txt # file: mnt/file.txt # owner: CINDER\administrator # group: CINDER\domain\users user::rwx group::r-x other::--x Adding a new ACE (full control for user "gah") to the ACL from a Windows client via Samba results in the implicit granting of read permission to "other": sneezy:~ # getfacl /mnt/file.txt # file: mnt/file.txt # owner: CINDER\administrator # group: CINDER\domain\users user::rwx user:CINDER\134gah:rwx group::r-x mask::rwx other::r-x The implicit granting of read permission to any ACE with execute permission is due to the existing POSIX ACL mapping algorithm: In the get DACL path map_canon_ace_perms() maps S_IXUSR to FILE_EXECUTE, SEC_STD_READ_CONTROL, FILE_READ_ATTRIBUTES and SYNCHRONIZE_ACCESS permissions. In the set DACL path map_nt_perms() checks for any of FILE_READ_DATA, FILE_READ_EA or FILE_READ_ATTRIBUTES. If set then the "r" flag is set in the corresponding POSIX Access Control entry.
cc'ing Jeremy, I think it's safe to assume that he holds the most knowledge of the POSIX ACL mapping code :)
looking back through commit history, it appears that this behavour was introduced in 2008 with 54eaf2de74b4779919ae97b54abceb3878894bf6: diff --git a/source3/include/smb.h b/source3/include/smb.h index 8b64877..fdbad2a 100644 --- a/source3/include/smb.h +++ b/source3/include/smb.h @@ -1228,7 +1228,7 @@ struct bitmap { #define FILE_GENERIC_WRITE (STD_RIGHT_READ_CONTROL_ACCESS|FILE_WRITE_DATA|FILE_WRITE_ATTRIBUTES|\ FILE_WRITE_EA|FILE_APPEND_DATA|SYNCHRONIZE_ACCESS) -#define FILE_GENERIC_EXECUTE (STANDARD_RIGHTS_EXECUTE_ACCESS|\ +#define FILE_GENERIC_EXECUTE (STANDARD_RIGHTS_EXECUTE_ACCESS|FILE_READ_ATTRIBUTES|\ FILE_EXECUTE|SYNCHRONIZE_ACCESS)
In reply to comment #2) > looking back through commit history, it appears that this behavour was > introduced in 2008 with 54eaf2de74b4779919ae97b54abceb3878894bf6: Aside from reverting the FILE_GENERIC_EXECUTE change added in 54eaf2d, another potential option is to disregard the FILE_READ_ATTRIBUTES Windows permission is when granting POSIX read permission.
Created attachment 7145 [details] Patch returning to old behaviour or not mapping x to read_attr Given the sensitivity and complexity of the ACL mapping code I'm reluctant to propose anything for master until it's been thoroughly tested and reviewed.
Created attachment 7146 [details] patch that should of course be a bitwise NOT
Created attachment 7147 [details] Alternate patch Does the following also work ? Might be a smaller fix here - thank goodness this is only done on ACL set not get. Jeremy.
Thanks for taking a look at this Jeremy :) (In reply to comment #6) > Created attachment 7147 [details] > Alternate patch > > Does the following also work ? Might be a smaller fix here - thank goodness > this is only done on ACL set not get. This change also fixes the issue. I'm curious why this is preferred over reverting back to the pre 54eaf2d behaviour, in which case a single FILE_READ_ATTRIBUTES permission is stored as Unix r.
The pre 54eaf2d behaviour doesn't take into account the fact that READ_ATTRIBUTES is implicitly granted. It's also a smaller patch (which I feel safer with). Sorry, not trying to refuse your work (I'm very grateful to you for finding this) it's just I'm trying to make the "minimum necessary change" (*). Jeremy. (*) See: https://en.wikipedia.org/wiki/The_End_of_Eternity for details :-).
Created attachment 7149 [details] git-am fix for 3.6.x
(In reply to comment #8) > The pre 54eaf2d behaviour doesn't take into account the fact that > READ_ATTRIBUTES is implicitly granted. > > It's also a smaller patch (which I feel safer with). Sorry, not trying to > refuse your work (I'm very grateful to you for finding this) it's just I'm > trying to make the "minimum necessary change" (*). Fair enough, I'm happy so long as it gets fixed :) > > Jeremy. > > (*) See: > > https://en.wikipedia.org/wiki/The_End_of_Eternity /me adds to reading list
Comment on attachment 7149 [details] git-am fix for 3.6.x This fix also applies cleanly to 3.5.x.
Re-assigning to Karolin for inclusion in 3.5.x and 3.6.x. Jeremy.
Pushed to v3-6-test and v3-5-test. Closing out bug report. Thanks!