Bug 466786

Summary: "make me a group member" button didn't work because the samba user shares group didn't contain the text "samba", but this wasn't mentioned anywhere
Product: [Frameworks and Libraries] kdenetwork-filesharing Reporter: kinghat <madLyfe>
Component: generalAssignee: Unassigned bugs mailing-list <unassigned-bugs>
Status: RESOLVED FIXED    
Severity: normal CC: kde, nate, schM0ggi, sitter
Priority: NOR Keywords: usability
Version: unspecified   
Target Milestone: ---   
Platform: Other   
OS: Linux   
Latest Commit: Version Fixed In: 23.04
Sentry Crash Report:
Attachments: error on adding user to usershares group

Description kinghat 2023-03-03 18:59:03 UTC
Created attachment 156968 [details]
error on adding user to usershares group

SUMMARY
trying to share the default "Music" dir in home on fedora and the "make me a group member" button didn't function properly. manually adding myself to the group and restarting did.


STEPS TO REPRODUCE
1. open dolphin, right click on properties on directory in home(default "Music" dir in my case, go to share tab.
2. i wasnt in the "usershares" group so it asked me to become apart of said group.
3. click button to add oneself to the group.

OBSERVED RESULT
clicking the  "make me a group member" button errors as shown in attachment.

EXPECTED RESULT
should be added to usershares group and given a notice on what to do next. might need to restart or logout/in?

SOFTWARE/OS VERSIONS
Operating System: Fedora Linux 37
KDE Plasma Version: 5.27.2
KDE Frameworks Version: 5.103.0
Qt Version: 5.15.8
Kernel Version: 6.1.14-200.fc37.x86_64 (64-bit)
Graphics Platform: Wayland
Processors: 4 × Intel® Core™ i7-5600U CPU @ 2.60GHz
Memory: 11.6 GiB of RAM
Comment 1 Nate Graham 2023-03-07 21:48:43 UTC
Unfortunately it seems like we're hitting this error condition:

                    QString helpfulActionErrorText = job->errorString();
                    if (helpfulActionErrorText.isEmpty()) {
                        // unknown error :(
                        helpfulActionErrorText = xi18nc("@info", "Failed to make user <resource>%1</resource> a member of group <resource>%2</resource>", user, group);
                    }
                    Q_EMIT helpfulActionError(helpfulActionErrorText);

So the backend process didn't give us any indication of what actually wrong wrong. :/

Is this 100% reproducible for you? If you remove your user from the appropriate group, reboot, and use the setup wizard again, does it happen again?
Comment 2 Harald Sitter 2023-03-07 22:01:23 UTC
Pretty sure the group name must contain 'samba' for hardening reasons.
Comment 3 Nate Graham 2023-03-07 22:17:21 UTC
Where's that restriction enforced?
Comment 5 Nate Graham 2023-03-07 22:21:03 UTC
Aha.

Are you sure that's a good idea to keep? In principle the distro can set the group name to anything. It doesn't seem ideal to declare otherwise valid distro configs to be unauthorized, especially without bubbling up a notification of this to the user. We should do that at a minimum.
Comment 6 Nate Graham 2023-03-07 22:24:56 UTC
We also have differing definitions of valid group names in different places in the code. In authhelper.cpp, we want the group to contain "samba" but in groupmanager.cpp, we only check for whether the group name is "root".
Comment 7 Harald Sitter 2023-03-07 22:27:20 UTC
Not sure I understand the question. Yes, we need some hardening ^^
Comment 8 Harald Sitter 2023-03-07 22:30:08 UTC
Mind that there is a difference between an invalid group name (e.g. the directory is group owned by root) indicative of the setup being incorrect and an unauthorized group name.
Comment 9 Bug Janitor Service 2023-03-07 22:35:21 UTC
A possibly relevant merge request was started @ https://invent.kde.org/network/kdenetwork-filesharing/-/merge_requests/40
Comment 10 Bug Janitor Service 2023-03-07 23:55:03 UTC
A possibly relevant merge request was started @ https://invent.kde.org/network/kdenetwork-filesharing/-/merge_requests/41
Comment 11 Nate Graham 2023-03-10 13:56:39 UTC
Git commit 64998586b683d1268a260ae228eb35c3bdb1e55a by Nate Graham.
Committed on 10/03/2023 at 13:49.
Pushed by ngraham into branch 'master'.

Fix incorrect conditions and instructions in group setup

Right now the setup wizard checks for whether the group contains the
text "root", and if it does, it errors out and recommends that you
change the group owner to "usershares." But this isn't valid either; in
authhelper.cpp, it will only accept a group with the word "samba" in it,
plus a few other conditions.

This commit makes the frontend code for the wizard match the backend
conditions, so never recommends doing something invalid, which will then
fail.
FIXED-IN: 23.04

M  +1    -0    samba/filepropertiesplugin/authhelper.cpp
M  +7    -3    samba/filepropertiesplugin/groupmanager.cpp

https://invent.kde.org/network/kdenetwork-filesharing/commit/64998586b683d1268a260ae228eb35c3bdb1e55a
Comment 12 kinghat 2023-03-10 14:47:09 UTC
> Is this 100% reproducible for you? If you remove your user from the
> appropriate group, reboot, and use the setup wizard again, does it happen
> again?

do you still need clarification here?
Comment 13 Nate Graham 2023-03-10 14:54:13 UTC
No, we found and fixed the issue. Thanks for reporting it!
Comment 14 Nate Graham 2023-03-10 14:54:42 UTC
Also worth reporting to Fedora that the group name should be called "sambashares", not "usershares".
Comment 15 kinghat 2023-03-10 15:12:26 UTC
(In reply to Nate Graham from comment #14)
> Also worth reporting to Fedora that the group name should be called
> "sambashares", not "usershares".

hopefully the correct spot: https://pagure.io/fedora-kde/SIG/issue/109
Comment 16 Marc Deop 2023-03-10 16:31:48 UTC
(In reply to Harald Sitter from comment #7)
> Not sure I understand the question. Yes, we need some hardening ^^

Why does kde need to do some hardening via group name? I am not sure I understand.

The hardening would be done by the samba administrators and deciding whether to allow the usershares functionality and under what group (via write permissions I think).

Or am I missing something?
Comment 17 Harald Sitter 2023-03-10 17:01:20 UTC
One can ask the auth helper to make the user a member of any group
Comment 18 Marc Deop 2023-03-10 18:41:26 UTC
(In reply to Harald Sitter from comment #17)
> One can ask the auth helper to make the user a member of any group

That is very nice but the question remains: Why does kde need to do some hardening via group name?
Comment 19 Harald Sitter 2023-03-10 18:49:42 UTC
Yeah, I don't understand your question.

Because you can pass any old group in we need to limit the amount of garbage groups you can put in to abuse the system.
Comment 20 Nate Graham 2023-03-10 18:50:31 UTC
(In reply to Harald Sitter from comment #19)
> Yeah, I don't understand your question.
> 
> Because you can pass any old group in we need to limit the amount of garbage
> groups you can put in to abuse the system.
Who is "you"? Where can the user change the group they're asking to be made a member of?
Comment 21 Harald Sitter 2023-03-10 18:53:25 UTC
you = any program, including malware.
Comment 22 Nate Graham 2023-03-10 18:59:58 UTC
How exactly would that do that? Woudn't they need local root access to modify the kauth files to change the group name?
Comment 23 Marc Deop 2023-03-10 19:15:46 UTC
(In reply to Harald Sitter from comment #19)
> Yeah, I don't understand your question.
> 
> Because you can pass any old group in we need to limit the amount of garbage
> groups you can put in to abuse the system.

Let me rephrase the question then:

What makes a group name `sambausers` safer than a group name `thisismyfavouritegroupname` ?

Either would be set by our system (in our Fedora's usecase by the "samba-usershares" package).
Comment 24 Harald Sitter 2023-03-10 19:20:30 UTC
(In reply to Nate Graham from comment #22)
> How exactly would that do that? Woudn't they need local root access to
> modify the kauth files to change the group name?

The group name is not encoded in any kauth files?
Comment 25 Nate Graham 2023-03-10 19:23:35 UTC
Then where does the group get set? Can you help explain what exactly the vulnerability is here?

It's clear you understand it, but Marc and I don't, and we're not domain experts, so it would be helpful if you could be a bit more verbose and imagine that you're explaining the vulnerability in question and how restricting certain group names plugs it.
Comment 26 Harald Sitter 2023-03-10 19:43:14 UTC
This call here https://invent.kde.org/network/kdenetwork-filesharing/-/blob/master/samba/filepropertiesplugin/groupmanager.cpp#L120 may be made by any application that has access to the bus. They may request becoming member of any group because of how the function works. The only thing standing between a malicious application making this request to push the user into the wheel group or root group or admin group and then exploit the access that comes with that is the group filtering.
Comment 27 Nate Graham 2023-03-10 20:08:10 UTC
I see, so KAuth files are public and any apps can try to use them. Is there no way to restrict them to only specific apps, where we can ensure more security.
Comment 28 schM0ggi 2023-03-10 22:33:59 UTC
Hi,

sorry to kind of abuse your report for this but:

(In reply to kinghat from comment #0)
> SUMMARY
> trying to share the default "Music" dir in home on fedora and the "make me a
> group member" button didn't function properly. manually adding myself to the
> group and restarting did.

could you please explain how exactly you were able to share the directory after manually adding your user to the group "usershares"?
I'm asking because of my report I did create a few months ago. https://bugs.kde.org/show_bug.cgi?id=463067

I ran into a dead end because kdenetwork-filesharing plugin asks for creating a Samba user when trying to share something.
It asks this although a Samba user (the name is the same like my system user) is already present. I did hop that it was fixed as the plugin was updated several times since I made that report. Sadly, I have still that issue. Sharing something is not possible. :/
Comment 29 Nate Graham 2023-03-17 19:29:47 UTC
Git commit d3ead0674aff6714b67ba9d3a9dfd38a57a66eb3 by Nate Graham.
Committed on 15/03/2023 at 17:33.
Pushed by ngraham into branch 'master'.

Show appropriate error messages when KAuth actions fail

Let's provide the user with the context needed to understand what went
wrong so they can figure out who's fault it is or how they might be able
to fix it.

M  +37   -9    samba/filepropertiesplugin/authhelper.cpp

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