Bug 8631 - POSIX ACE x permission becomes rx following mapping to and from a DACL
POSIX ACE x permission becomes rx following mapping to and from a DACL
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: File services
3.6.1
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks: 8595
  Show dependency treegraph
 
Reported: 2011-11-25 14:24 UTC by David Disseldorp
Modified: 2011-12-01 19:48 UTC (History)
2 users (show)

See Also:


Attachments
Patch returning to old behaviour or not mapping x to read_attr (1.96 KB, patch)
2011-11-25 18:00 UTC, David Disseldorp
no flags Details
patch (1.96 KB, patch)
2011-11-25 18:09 UTC, David Disseldorp
no flags Details
Alternate patch (777 bytes, patch)
2011-11-28 23:34 UTC, Jeremy Allison
no flags Details
git-am fix for 3.6.x (1.34 KB, patch)
2011-11-29 22:50 UTC, Jeremy Allison
ddiss: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Disseldorp 2011-11-25 14:24:11 UTC
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.
Comment 1 David Disseldorp 2011-11-25 14:40:12 UTC
cc'ing Jeremy, I think it's safe to assume that he holds the most knowledge of the POSIX ACL mapping code :)
Comment 2 David Disseldorp 2011-11-25 15:08:08 UTC
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)
Comment 3 David Disseldorp 2011-11-25 15:27:01 UTC
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.
Comment 4 David Disseldorp 2011-11-25 18:00:09 UTC
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.
Comment 5 David Disseldorp 2011-11-25 18:09:43 UTC
Created attachment 7146 [details]
patch

that should of course be a bitwise NOT
Comment 6 Jeremy Allison 2011-11-28 23:34:49 UTC
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.
Comment 7 David Disseldorp 2011-11-29 10:26:48 UTC
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.
Comment 8 Jeremy Allison 2011-11-29 19:26:20 UTC
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 :-).
Comment 9 Jeremy Allison 2011-11-29 22:50:54 UTC
Created attachment 7149 [details]
git-am fix for 3.6.x
Comment 10 David Disseldorp 2011-11-29 23:05:43 UTC
(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 11 Jeremy Allison 2011-11-29 23:06:59 UTC
Comment on attachment 7149 [details]
git-am fix for 3.6.x

This fix also applies cleanly to 3.5.x.
Comment 12 Jeremy Allison 2011-11-29 23:55:39 UTC
Re-assigning to Karolin for inclusion in 3.5.x and 3.6.x.
Jeremy.
Comment 13 Karolin Seeger 2011-12-01 19:48:07 UTC
Pushed to v3-6-test and v3-5-test.
Closing out bug report.

Thanks!