Show a warning when running as the root user
ClosedPublic

Authored by ngraham on May 6 2018, 11:52 PM.

Details

Summary

Now that Dolphin can be run as the root user again, let's show a warning.

Test Plan

When run with the root user account:

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham requested review of this revision.May 6 2018, 11:52 PM
ngraham created this revision.
ngraham edited the summary of this revision. (Show Details)May 6 2018, 11:59 PM
ngraham edited the test plan for this revision. (Show Details)
ngraham updated this revision to Diff 33739.May 7 2018, 3:39 AM

Make the sudo message more dire and mention security issues

ngraham edited the test plan for this revision. (Show Details)May 7 2018, 3:45 AM
markg added a subscriber: markg.May 7 2018, 10:43 AM

I wonder if the check is accurate.

I think you need to replace the == root string check to either the one it was before or something like:
KUser(getuid()).isSuperUser() (this basically is getuid() == 0)

I wonder if the check is accurate.

I think you need to replace the == root string check to either the one it was before or something like:
KUser(getuid()).isSuperUser() (this basically is getuid() == 0)

It's accurate. getuid() == 0 returns true for a regular user using sudo as well as the actual root user. Capturing that distinction is a part of this patch.

mmustac added a subscriber: mmustac.May 7 2018, 3:36 PM

Shouldn't this be a warning instead an error message? I can understand that the signal color red was used here because of the security risk this action may provide but it's not used correctly here in my opinion.

Fuchs added a subscriber: Fuchs.May 7 2018, 3:57 PM

I like the idea in general, I agree though that, despite the severity, it is technically not an error and should not be shown as such, but rather a warning indeed.

I do think we should, despite the potential dangers, allow it but discourage it, as done here. There are plenty of other ways in which users can do a lot of damage, and there are (few, but some) uses cases where you want a file manager (or text editor, since the same is done there) with root priviledges, at least until better alternatives are available.

So +1 from me, but with a suggestion to change from error to warning, despite severity.

ngraham updated this revision to Diff 33774.May 7 2018, 4:08 PM

Use clearer logic and change the color to orange, for "Warning"

ngraham edited the summary of this revision. (Show Details)May 7 2018, 4:12 PM
ngraham edited the test plan for this revision. (Show Details)

I don't think this is a good idea. There is a reason we have that check there, and it must be the very first thing done in main(). Showing a fancy warning message in the dolphin view would be too late (see Martin's exploit).

I know the current situation is not ideal (given that kio is not polkit-ready yet - we are almost there though!). But we shouldn't leave the door open to a clear vulnerability that could affect every dolphin user.

What we can do is a build-time switch in cmake, so that if someone (or some distro) wants to remove the root check, they can do easily do so without patching the code.

I don't think this is a good idea. There is a reason we have that check there, and it must be the very first thing done in main(). Showing a fancy warning message in the dolphin view would be too late (see Martin's exploit).

I know the current situation is not ideal (given that kio is not polkit-ready yet - we are almost there though!). But we shouldn't leave the door open to a clear vulnerability that could affect every dolphin user.

Please read the arguments I made above. All of those concerns are addressed. I'll repeat them:

  1. It is inappropriate for an app to refuse to run on top of an insecure environment. The problem should be fixed at the level of the insecure environment. Did anybody ever submit an X11 patch? We don't hack around problems, we fix them.
  1. There needs to be a distinction made between running dolphin using sudo (you've now vulnerable to the exploit) and running as the root user (your whole user session was already vulnerable since you're running all GUI software as root anyway). It doesn't make sense to "secure" dolphin in a root session that is already inherently insecure.
  1. It is illogical to damage the user experience in the name of security; we fail at the goal of securing the user if the user becomes unable to use our software in the first place (e.g. for the Kali distro). If you lose your house key, you don't barricade the door so that nobody can go in or out until the locksmith comes. It is user-hostile, inappropriate, and illogical to remove a feature before its replacement is ready.

Our users are not children. Most of them are professionals or enthusiasts. Let's treat them like adults and provide a warning, but ultimately let them make their own decision. Once PolKit support is finally available to the public, we can revisit the issue of how best to wean users off doing sudo dolphin in a humane, user-friendly manner. Until then, I believe that the only professional course of action is to re-add the missing feature, despite its known security vulnerability. If the vulnerability is really so severe as to have warranted the drastic action originally taken here, someone should submit an X11 patch ASAP.

ngraham edited the summary of this revision. (Show Details)May 7 2018, 7:55 PM

Did anybody ever submit an X11 patch? We don't hack around problems, we fix them.

This is nothing a patch can fix. This is a fundamental design flaw of the X11 protocol and one of the major reasons for the creation of Wayland.

Did anybody ever submit an X11 patch? We don't hack around problems, we fix them.

This is nothing a patch can fix. This is a fundamental design flaw of the X11 protocol and one of the major reasons for the creation of Wayland.

Imagine that the vulnerability affected non-sudo user sessions with Dolphin. Would it be appropriate to entirely disable using Dolphin on X11 due to the vulnerability, and require Wayland despite the fact that our support for it is not finished yet? It's the same thing with disabling the feature entirely before polkit support is available: It doesn't make sense to disable features in the name of security before their replacements are available.

For a root GUI session, there is no extra vulnerability beyond what you're already vulnerable to by running a root GUI session, right? Does anybody have a reasonable argument to make against at least reverting this for the root GUI session use case?

ngraham edited the summary of this revision. (Show Details)May 7 2018, 8:59 PM
zzag added a subscriber: zzag.May 7 2018, 10:23 PM
zzag added inline comments.
src/dolphinviewcontainer.cpp
112

Why snake case?

ngraham updated this revision to Diff 33801.May 8 2018, 2:21 AM

Kill the snake

ngraham marked an inline comment as done.May 8 2018, 2:22 AM

I wonder if the check is accurate.

I think you need to replace the == root string check to either the one it was before or something like:
KUser(getuid()).isSuperUser() (this basically is getuid() == 0)

It's accurate. getuid() == 0 returns true for a regular user using sudo as well as the actual root user. Capturing that distinction is a part of this patch.

As i said, only for the root part (not the sudo part).
For instance, you can call dolphin with: "USER='' dolphin" which circumvents the whole check whereas getuid() would work regardless of environment variables ;)
Anyhow, we're on nitpicking terrain. Your version is fine for me as well and much more readable, but people focused on security will likely see a potential "security issue" with your code.

I am slightly surprised that this "feature" (not being able to run dolphin as root) even got in. It's a killer feature (in the negative sense). I sometimes need to run my GUI as root just because there is no user environment setup yet or when the GUI KDE session is somehow broken. Then i tend to start openbox as root and use dolphin as file management. I haven't had to do this in a while though.
It all seems to have been triggered by: https://marc.info/?l=kwrite-devel&m=145192458018333&w=2
And then pushed (outside of phabricator, why) by @emmanuelp in this commit: https://cgit.kde.org/dolphin.git/commit/src/main.cpp?id=0bdd8e0b0516555c6233fdc7901e9b417cf89791

So what is the real bug.. Well, this quote describes it:

Now I sat down and implemented the attached exploit. The key idea is to use an
embedded konsole window in a root owned process and send it key events. See
the attached README as well.

That is from the before mentioned link on marc.info.

Why is that an exploit again? You are root to gain root... you are root already!
I don't see why that is a bug.

As the embedded terminal is the real problem (is that even used "that" much?), perhaps "fix" that by just not allowing to run anything in there that requires elevated privileges? That would be a sensible fix and one you can expect as a user. You could go more user friendly and notify the user to open Konsole (or another terminal emulator) for executing commands that require elevated privilege.

I'm all for getting rid of this "security fix" as it's currently done. Running dolphin as root is a valuable thing to have in my opinion.

So what is the real bug.. Well, this quote describes it:

Now I sat down and implemented the attached exploit. The key idea is to use an
embedded konsole window in a root owned process and send it key events. See
the attached README as well.

That is from the before mentioned link on marc.info.

Why is that an exploit again? You are root to gain root... you are root already!
I don't see why that is a bug.

I still have not seen this objection addressed at all...

Please read the arguments I made above. All of those concerns are addressed. I'll repeat them:

  1. It is inappropriate for an app to refuse to run on top of an insecure environment. The problem should be fixed at the level of the insecure environment. Did anybody ever submit an X11 patch? We don't hack around problems, we fix them.

You cannot fix X11. That's why people are working on Wayland.

  1. There needs to be a distinction made between running dolphin using sudo (you've now vulnerable to the exploit) and running as the root user (your whole user session was already vulnerable since you're running all GUI software as root anyway). It doesn't make sense to "secure" dolphin in a root session that is already inherently insecure.

This I can agree with. But that's not what this patch is doing ;)
(this patch brings back the vulnerability also to non-root sessions).

  1. It is illogical to damage the user experience in the name of security; we fail at the goal of securing the user if the user becomes unable to use our software in the first place (e.g. for the Kali distro). If you lose your house key, you don't barricade the door so that nobody can go in or out until the locksmith comes. It is user-hostile, inappropriate, and illogical to remove a feature before its replacement is ready.

Finding the right trade-off between security and usability is a hard problem, yes. Note that running Dolphin as root is not a "feature". It's just something that happened to be possible but it has all sort of problems and it's never been supported by design.

Our users are not children. Most of them are professionals or enthusiasts. Let's treat them like adults and provide a warning, but ultimately let them make their own decision. Once PolKit support is finally available to the public, we can revisit the issue of how best to wean users off doing sudo dolphin in a humane, user-friendly manner. Until then, I believe that the only professional course of action is to re-add the missing feature, despite its known security vulnerability. If the vulnerability is really so severe as to have warranted the drastic action originally taken here, someone should submit an X11 patch ASAP.

See answers above.

For a root GUI session, there is no extra vulnerability beyond what you're already vulnerable to by running a root GUI session, right? Does anybody have a reasonable argument to make against at least reverting this for the root GUI session use case?

I'd be ok if we revert the change only for the Kali use case (by checking the env variables). Note that POSIX doesn't say that the user with UID 0 must have root as name, but we can probably live assuming that no-one is going to rename their root user ;)

I am slightly surprised that this "feature" (not being able to run dolphin as root) even got in. It's a killer feature (in the negative sense). I sometimes need to run my GUI as root just because there is no user environment setup yet or when the GUI KDE session is somehow broken. Then i tend to start openbox as root and use dolphin as file management. I haven't had to do this in a while though.
It all seems to have been triggered by: https://marc.info/?l=kwrite-devel&m=145192458018333&w=2
And then pushed (outside of phabricator, why) by @emmanuelp in this commit: https://cgit.kde.org/dolphin.git/commit/src/main.cpp?id=0bdd8e0b0516555c6233fdc7901e9b417cf89791

The change was reviewed in D4634, Emmanuel just synced Dolphin with Kate.

So what is the real bug.. Well, this quote describes it:

Now I sat down and implemented the attached exploit. The key idea is to use an
embedded konsole window in a root owned process and send it key events. See
the attached README as well.

That is from the before mentioned link on marc.info.

Why is that an exploit again? You are root to gain root... you are root already!
I don't see why that is a bug.

It's not dolphin that is gaining root. Any non-root process can trivially gain root access if there is a dolphin or kate or konsole window running as root.

markg added a comment.May 8 2018, 10:25 PM
  1. It is illogical to damage the user experience in the name of security; we fail at the goal of securing the user if the user becomes unable to use our software in the first place (e.g. for the Kali distro). If you lose your house key, you don't barricade the door so that nobody can go in or out until the locksmith comes. It is user-hostile, inappropriate, and illogical to remove a feature before its replacement is ready.

Finding the right trade-off between security and usability is a hard problem, yes. Note that running Dolphin as root is not a "feature". It's just something that happened to be possible but it has all sort of problems and it's never been supported by design.

Please do enlighten us with "all sort of problems" as i know none.

In fact, till a few years ago there was nothing stopping you from running the whole of plasma session as root. Now some apps went on the "security focused" way by just not allowing them to run as root. Effectively making it impossible to run plasma as root.
Sure, there are lots of reasons why you should not do that and i probably agree with all, but "sometimes"... there is just no escape.

Why is that an exploit again? You are root to gain root... you are root already!
I don't see why that is a bug.

It's not dolphin that is gaining root. Any non-root process can trivially gain root access if there is a dolphin or kate or konsole window running as root.

I was looking over the exploit code and thought the same. Any app with a terminal would have this "potential issue".

But when can this issue be abused?
I can only think of one hypothetical case. A multi-seat environment where one of the seats is running as root where a non-root seat could then exploit that root seat. I say hypothetical because i have no idea if that really works.
But even if it would works, i'd be willing to bet that the vast majority of KDE installations is single-seat only. One computer with one KDE session. Even much of those installed in corporate environments likely have a single desktop per seat.
For what is this protection then?

AFAIU the exploit works like this:

A user has a normal, non-root session running which is infected with a malicious program. The program now waits until the user is running 'sudo dolphin' and now can use the exploit to gain elevated privileges.

If the user is running as root anyway there is no additional harm, because most likely the malicious program will have root privileges anyway.

markg added a comment.May 8 2018, 10:43 PM

AFAIU the exploit works like this:

A user has a normal, non-root session running which is infected with a malicious program. The program now waits until the user is running 'sudo dolphin' and now can use the exploit to gain elevated privileges.

If the user is running as root anyway there is no additional harm, because most likely the malicious program will have root privileges anyway.

Which is not that often for Dolphin, but is much more likely for Konsole.
How to guard that, a terminal emulator by nature!
I don't think you can... (don't bring up wayland.. we're still in an X11 era whether you want it or not).

zzag added a comment.May 9 2018, 1:03 AM

I was looking over the exploit code and thought the same. Any app with a terminal would have this "potential issue".

Assuming that a user is in wheel group, running konsole (not as root) itself is not safe. ;-)

Restricted Application added a project: Dolphin. · View Herald TranscriptMay 9 2018, 7:57 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
markg added a comment.May 9 2018, 12:42 PM

I missed this reply so responding on it now.

I don't think this is a good idea. There is a reason we have that check there, and it must be the very first thing done in main(). Showing a fancy warning message in the dolphin view would be too late (see Martin's exploit).

The "reason" you mention (I've read the exploit code and mails around it) can hardly be called a reason. Yes, it is possible...
But then nothing with a terminal in an X11 GUI is safe. Also, Linux has been doing just fine for decades with X11 with this "exploit" laying there like forever.
Sure, there might be "a" potential risk, but it's darn freaking small!
A user would have to install a malicious application which is already quite unlikely if the user installs packages from the distribution package manager.
How often do we - as developers! - even install packages from outside our distribution package manager? We are more at risk then the average linux user and i dare saying that even we have near 0 risk.

Really, what is the risk for the user here? 0.00000001%?
Some risk is imho acceptable. I'd call this one acceptable.

I know the current situation is not ideal (given that kio is not polkit-ready yet - we are almost there though!). But we shouldn't leave the door open to a clear vulnerability that could affect every dolphin user.

What we can do is a build-time switch in cmake, so that if someone (or some distro) wants to remove the root check, they can do easily do so without patching the code.

I cannot believe you even suggest such a solution.

Sorry for being so heavily against this, but you have security with real threads (those need to be fixed) and you have security paranoia.. This issue in my opinion falls in the latter category.
If you absolutely must fix it then just tear out that embedded konsole. It's a neat feature but i value dolphin as root much more then an embedded konsole (do a google search for dolphin as root and be stunned by the sheer number of people asking how to run it as root as it isn't possible now anymore).

ngraham added a comment.EditedMay 9 2018, 1:15 PM

I have to agree with Mark. Life is full of risks that we trade off. It rarely makes sense to mitigate a risk by destroying our ability to do the potentially risky thing in the first place.

I brought up the example of a compromised door lock before; the current approach taken here is akin to nailing your door shut so nobody can enter it--yourself included--even if they've gotten the key. Security measures that burden or even destroy an important use case aren't actually security at all: they're developer laziness.

Consider the following dialogue:

Developer: "Oh this feature is dangerous, I'll turn it off."
User: "Okay, what do I use instead of that feature?"
Developer: "Nothing; its replacement hasn't been implemented yet."
User: "Um, what's the timeframe for implementing it?"
Developer: "I don't know, I'm not involved with that project and don't plan to contribute to it."
User: "Okay, then could you consider delaying turning it off until the replacement is ready?"
Developer: "No, the security threat is too great."
User: "So what am I supposed to do if I want or need to USE that feature?"
Developer: "Not my problem."
User: "I'm angry!"
Developer: "I don't see why; I've protected you from a terrible security vulnerability."
User: "But in the process, you removed a feature that I use!"
Developer, "Not my problem."

Surely Mark and I are not the only ones who see the absurdity in this. Security measures that damage the user experience are the laziest kind. I mean, we could all could be far more secure from the thread of online malware if you disabled the web browser, right? Surely we can do better than this.

I've opened D12795: Re-allow running Dolphin as the root user (but still not using sudo) to implement the simpler and less controversial change of allowing Dolphin to run with the root user (but still not using sudo).

graesslin requested changes to this revision.May 20 2018, 2:07 PM

-2 from me. That is against our focus of providing secure software.

What you can do instead is starting a KMessageBox through an external process which drops back to the normal user. There inform the user about the risks, maybe with a link to a dedicated page on kde.org where we explain the attack on Dolphin through running as root. From within the message box one could provide a way to launch dolphin nevertheless as root (e.g. env variable I_KNOW_THAT_ROOT_CAN_BREAK_ME=1).

What's absolutely important is to do all checks before the QGuiApplication is constructed. QGuiApplication opens the connection to X. At that point the application is potentially owned. X11 as a protocol is way too insecure. There are millions of ways to get an application running as root to execute arbitrary commands from a non-root user through X11.

src/dolphinviewcontainer.cpp
128

If you show a message you are owned. That's the problem when running applications on X11 as root. Just opening the connection is sufficient to get owned. In my scratch repos you can find an example application to execute arbitrary commands through a dolphin running as root. See: https://cgit.kde.org/scratch/graesslin/exploit-dophin-root-x11.git/

The warning in the gui is too late.

This revision now requires changes to proceed.May 20 2018, 2:07 PM

@chinmoyr makes editable root owned files in Kate/KWrite, through KAuth (if i'm not wrong) is he there a plans to do so for Dolphin? Is here applicable this method?

Right, hopefully PolKit in KIO will make it in at some point...

If D12795 is accepted and PolKit in KIO lands, I'll be willing to abandon this on the grounds that it's no longer needed, because both prior use cases are properly handled.

What you can do instead is starting a KMessageBox through an external process which drops back to the normal user. There inform the user about the risks, maybe with a link to a dedicated page on kde.org where we explain the attack on Dolphin through running as root. From within the message box one could provide a way to launch dolphin nevertheless as root (e.g. env variable I_KNOW_THAT_ROOT_CAN_BREAK_ME=1).

env variable doesn't work. A malicious program running as user could write it into the env variables loaded at startup. What could work is using a command line argument. Of course without QCommandLineParser.

@chinmoyr makes editable root owned files in Kate/KWrite, through KAuth (if i'm not wrong) is he there a plans to do so for Dolphin? Is here applicable this method?

BTW: KAuth integration has also some security flaws which aren't resolved to date: https://phabricator.kde.org/D12513

Now that D12795 has landed, what do Dolphin folks think about reviving this (without any of the main.cpp changes) so that Dolphin shows a warning when run as the root user?

markg accepted this revision.May 31 2018, 7:36 PM

Now that D12795 has landed, what do Dolphin folks think about reviving this (without any of the main.cpp changes) so that Dolphin shows a warning when run as the root user?

+1 from me.
Consider the green tick as a +1 :)

I'm guessing you need to refactor the current patch though. As it still shows a change in main.cpp.

This revision is now accepted and ready to land.May 31 2018, 7:36 PM
ngraham planned changes to this revision.May 31 2018, 7:39 PM

Right, I didn't want to make a change that folks would reject. I can re-work the patch now. :)

ngraham updated this revision to Diff 35301.Jun 1 2018, 3:14 AM

Rebase on master and adjust to recent changes

This revision is now accepted and ready to land.Jun 1 2018, 3:14 AM
ngraham retitled this revision from Implement a more user-friendly run-as-root-or-sudo behavior to Show a warning when running as the root user.Jun 1 2018, 3:17 AM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
This comment was removed by elvisangelaccio.
This revision was automatically updated to reflect the committed changes.