Bug 13319 - Round-tripping ACL get/set through vfs_fruit will increase the number of ACE entries without limit.
Round-tripping ACL get/set through vfs_fruit will increase the number of ACE ...
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2018-03-02 17:11 UTC by Jeremy Allison
Modified: 2018-04-10 07:24 UTC (History)
3 users (show)

See Also:


Attachments
Proposed git-am fix for master. (9.25 KB, patch)
2018-03-06 22:08 UTC, Jeremy Allison
no flags Details
git-am fix for 4.8.0rcX, 4.7.next, 4.6.next. (9.89 KB, patch)
2018-03-08 19:19 UTC, Jeremy Allison
jra: review-
Details
git-am fix for 4.8.next. (28.93 KB, patch)
2018-03-19 18:46 UTC, Jeremy Allison
jra: review-
Details
git-am fix for 4.7.next. (28.69 KB, patch)
2018-03-19 18:47 UTC, Jeremy Allison
jra: review-
Details
(final) git-am fix for 4.8.next. (30.13 KB, patch)
2018-03-22 16:32 UTC, Jeremy Allison
slow: review+
Details
(final) git-am fix for 4.7.next. (29.89 KB, patch)
2018-03-22 16:40 UTC, Jeremy Allison
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2018-03-02 17:11:48 UTC
In fruit_fget_nt_acl() the 3 extra ACE entries corresponding to mode/uid/gid are always added to the returned ACL as virtual ACE entries that are not expected to be stored in the file ACL on disk.

In fruit_fset_nt_acl() the client-sent ACL is applied to the file, then the mode entry (if sent) is used to do the CHMOD - but the client-sent ACL is applied without removing any virtual ACE entries.

If a naive client just round-trips get/set/get/set, on every set the 'virtual' ACE entries will be stored in the xattr.

I think the correct action on fruit_fset_nt_acl() is to check for - then *remove* any virtual ACE entries sent by the client before passing down to the underlying SET_NT_ACL call.

(Discovered by code review when designing a similar mechanism for SMB2 UNIX extensions).

Jeremy.
Comment 1 Jeremy Allison 2018-03-06 22:08:09 UTC
Created attachment 14020 [details]
Proposed git-am fix for master.
Comment 2 Jeremy Allison 2018-03-08 19:19:06 UTC
Created attachment 14027 [details]
git-am fix for 4.8.0rcX, 4.7.next, 4.6.next.

Cherry-picked from master.
Comment 3 Jeremy Allison 2018-03-15 00:18:08 UTC
Comment on attachment 14027 [details]
git-am fix for 4.8.0rcX, 4.7.next, 4.6.next.

Damn. There is a remaining bug in this patchset. In the set_acl path I only remove the ACE corresponding to the mode already on the file. I need to remove the ACE entry that matches global_sid_Unix_NFS_Mode+new_permissions as well as global_sid_Unix_NFS_Mode+existing permissions.

Follow-up patch to come on samba-technical.

Jeremy.
Comment 4 Stefan Metzmacher 2018-03-15 09:00:42 UTC
(In reply to Jeremy Allison from comment #3)

Can't you just filter out any aces related to
global_sid_Unix_NFS sids, with some like:

+               cmp = dom_sid_compare_domain(&global_sid_Unix_NFS,
+                                            &user_info_dc->sids[i]);
+               if (cmp == 0) {

I'm using some similar here
https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=35603dd8ef03b87a581d1dd4f9ad4964beb13336
Comment 5 Jeremy Allison 2018-03-15 15:34:51 UTC
Yes, that's what I'm planning to do in a subsequent patch. I need to make sure they don't get stored in the ACL on disk as they are 'virtual' ACE's created on demand on read. Patch to follow :-).
Comment 6 Jeremy Allison 2018-03-15 21:01:41 UTC
FYI, the reason the patch is taking a little while is I'm adding a regression test for this...
Comment 7 Jeremy Allison 2018-03-19 18:46:58 UTC
Created attachment 14058 [details]
git-am fix for 4.8.next.
Comment 8 Jeremy Allison 2018-03-19 18:47:16 UTC
Comment on attachment 14058 [details]
git-am fix for 4.8.next.

Cherry-picked from master.
Comment 9 Jeremy Allison 2018-03-19 18:47:51 UTC
Created attachment 14059 [details]
git-am fix for 4.7.next.

Back-ported from master.
Comment 10 Jeremy Allison 2018-03-19 22:42:25 UTC
Comment on attachment 14058 [details]
git-am fix for 4.8.next.

Argh. Error in patch again (addressing on list).
Comment 11 Jeremy Allison 2018-03-19 22:42:57 UTC
Comment on attachment 14059 [details]
git-am fix for 4.7.next.

Argh. Error in patch again (addressing on list).
Comment 12 Jeremy Allison 2018-03-22 16:32:58 UTC
Created attachment 14068 [details]
(final) git-am fix for 4.8.next.

Cherry-picked from master.
Comment 13 Jeremy Allison 2018-03-22 16:40:30 UTC
Created attachment 14069 [details]
(final) git-am fix for 4.7.next.

Back-ported from master.
Comment 14 Ralph Böhme 2018-03-22 18:00:12 UTC
Reassigning to Karolin for inclusion in 4.8 and 4.7.
Comment 15 Karolin Seeger 2018-03-22 20:57:42 UTC
(In reply to Ralph Böhme from comment #14)
Pushed to autobuild-v4-[8,7]-test
Comment 16 Karolin Seeger 2018-04-10 07:24:04 UTC
(In reply to Karolin Seeger from comment #15)
Pushed to both branches.
Closing out bug report.

Thanks!