Bug 422554 - permission order in usershare_acl
Summary: permission order in usershare_acl
Status: RESOLVED FIXED
Alias: None
Product: kdenetwork-filesharing
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Nate Graham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-06 21:11 UTC by Yichao Yu
Modified: 2020-08-21 10:33 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In: 20.12


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Yichao Yu 2020-06-06 21:11:19 UTC
SUMMARY

To be fair, I'm not sure if this should be frameworks/kio or network/kdenetwork-filesharing .

The permissiong window shows an "everyone" as well as an entry for each samba users with the "everyone" being the first one. The GUI strongly suggests that different permissions can be configured for different users with the everyone as fallback.

However, in the generated usershare_acl, everyone (S-1-1-0) is also the first one, before any user ones. Even though I haven't been able to find the document for this configure field yet, from local testing, this field is order sensitive and samba will use the first match. This essentially means that as long as the "fallback" is set, it'll be used for everyone...

STEPS TO REPRODUCE
1. Configure samba usershare
2. Share a directory from dolphin properties dialog
3. Change everyone to deny and the user to read/write (note: might need to workaround https://invent.kde.org/network/kdenetwork-filesharing/-/commit/447735830b0749f0d896dd9408c3287f66d496e9#note_56261)

OBSERVED RESULT
The user will not be able to access the share.
Moving the `S-1-1-0:d` in usershare_acl in the samba usershare configure file to the end fixes this.

EXPECTED RESULT
The user should have read/write permission whereas no one else should.

SOFTWARE/OS VERSIONS
Linux/KDE Plasma: ArchLinux
KDE Plasma Version: 5.18.5
KDE Frameworks Version: 5.70.0
Qt Version: 5.15.0

ADDITIONAL INFORMATION
Samba version 4.12.3
Comment 1 Harald Sitter 2020-06-17 11:47:53 UTC
It's not specified that order matters https://www.samba.org/samba/docs/current/man-html/net.8.html so for all we know it's a bug.

That being said, from what I recall on windows this is documented and ACEs are in fact processed until an explicit access or denial is found to be applicable to a request. That would match what you are seeing.
Comment 2 Harald Sitter 2020-06-17 11:55:01 UTC
for the record:
https://docs.microsoft.com/en-us/windows/win32/secauthz/how-dacls-control-access-to-an-object

&

https://docs.microsoft.com/en-us/windows/win32/secauthz/order-of-aces-in-a-dacl

Which if that is what samba follows means ACLS ought to be sorted thusly:

- all users
  - all D
  - all R
  - all F
- all groups
  - all D
  - all R
  - all F
- everyone

I think! (haven't really read up on this in detail; plus someone needs to talk to upstream to fix their docs anyway)
Comment 3 Harald Sitter 2020-08-13 15:15:12 UTC
Windows behaves exactly the same as we do. And the actual eval order on the server is

- Everyone
- Group
- User

each with denial taking precedence over permission. Everyone being denied denies everyone. Any group the user is a member of being denied denies. Any user rules matching a denial are denied.
They do aggressively warn about setting deny rules though, because denial always wins regardless of whatever other ACEs are available. Which seems like a reasonable enough way to deal with it. It keeps the UI simpler and means the application rule is easily graspable.

In particular samba's behaviour is unexpectedly out of line here. Samba's ACE checking `se_access_check` always appears to accept either an applicable denial or approval. So, which ever rule applies first wins and that is why the usershare ACL order matters, there's no sorting being applied to the input ACL. The entire low level ACL management is rather awkward in general though and really should be replaced with something more user friendly.
Comment 4 Yichao Yu 2020-08-13 15:50:15 UTC
Have to say I don't really like the windows behavior you described though having an aggressive warning would at least fix my initial confusion.

I think my bottom line is that there should be a way to set the permission such that it is denied for every (current and future) user but allowed for one user. This seem to be a setting that is allowed by samba. As long as the UI allows this (and similar setting for groups maybe) and avoid confusion I'd be fairly happy about it.......
Comment 5 Harald Sitter 2020-08-13 18:00:26 UTC
Just configure that on the file system. The share access configuration literally only controls who can log in on that share from a samba POV, beyond that is has zero practical impact. The most reasonable share configuration is to set Everyone:f and leave permissions to the filesystems. It's also how window's actual share dialog for users works, not the advanced ACL dialog I was talking about earlier. The only reason you ever need more intricate share ACLs is when your file system actually doesn't support ACLs, which is an utter edge case because this isn't the 90s 😂
Comment 6 Bug Janitor Service 2020-08-20 12:05:41 UTC
A possibly relevant merge request was started @ https://invent.kde.org/network/kdenetwork-filesharing/-/merge_requests/5
Comment 7 Harald Sitter 2020-08-21 10:33:50 UTC
Git commit f3c12f1201699b1c3de55fe56e9dbea5553996dc by Harald Sitter.
Committed on 20/08/2020 at 11:57.
Pushed by sitter into branch 'master'.

fix up ACE ordering

the order of entries within the ACL have meaning as samba aborts
permission lookup on matching denials but combines read and full access.
since our ACL table mimics windows' we'll follow its behavior as it's
also fairly easy to explain

"a denial always denies, no matter what other rules exist"

to that end we'll now sort the ACL to first list denials, then reads,
then full access

- if foo is denied they'll not be let in
- if bar is read they get let in
- if Everyone is denied nobody gets in

should we later add group support this further becomes

- if groupFoo is denied and bar is a member of it then bar is denied
- if groupBar is read and foo is a member of it then foo is still denied
- if groupFoo is fullacces and bar is a member they'll get fullaccess
FIXED-IN: 20.12

M  +39   -10   samba/filepropertiesplugin/model.cpp
M  +1    -0    samba/filepropertiesplugin/sambausershareplugin.cpp

https://invent.kde.org/network/kdenetwork-filesharing/commit/f3c12f1201699b1c3de55fe56e9dbea5553996dc
Comment 8 Harald Sitter 2020-08-21 10:33:58 UTC
Git commit 6e7878af23a5db01afaa51ed03f34e69dc31b4e0 by Harald Sitter.
Committed on 20/08/2020 at 12:02.
Pushed by sitter into branch 'master'.

rejigger acl page and add a sheet to be shown when using a denial

denials always win so they shouldn't really be used unless one is
suuuuuuuuuuuuper sure about it.
to that end we'll throw up a warning that tries to explain that denials
probably aren't what the user wants and also that denying usually isn't
necessary because filesystem permissions still apply, so even when using
a liberal share ACL the filesystem ACL would still prevent other users
from reading one's private data for example

this was a bit awkward to implement because sheets need to overlay
something yet overlaying the columnlayout messes with the layout size
itself compacting the tableview away.
equally we cannot use a regular page because pages come with a whole
bunch of padding that looks very weird here and I've not found a way to
disable it either. instead we now have a new toplevel item which merely
acts as the thing the sheet can overlay. the item then contains the
previous layout and elements so they retain the sizing even when
overlayed
FIXED-IN: 20.12

M  +173  -132  samba/filepropertiesplugin/qml/ACLPage.qml

https://invent.kde.org/network/kdenetwork-filesharing/commit/6e7878af23a5db01afaa51ed03f34e69dc31b4e0