Discussion:
D12795: Re-allow running Dolphin as the root user (but still not using sudo)
Vlad Zagorodniy
2018-05-10 21:28:14 UTC
Permalink
zzag added a comment.


Shouldn't it print to stderr instead of stdout?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-10 21:39:14 UTC
Permalink
ngraham added a comment.


The problem is that `USER` is always set to "root" when you use `sudo`. That's why `SUDO_USER` exists, as far as I know.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Vlad Zagorodniy
2018-05-10 22:27:48 UTC
Permalink
zzag added a comment.
Post by Nathaniel Graham
The problem is that `USER` is always set to "root" when you use `sudo`. That's why `SUDO_USER` exists, as far as I know.
It looks like kdesu sets KDESU_USER.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Elvis Angelaccio
2018-05-10 21:18:55 UTC
Permalink
elvisangelaccio requested changes to this revision.
elvisangelaccio added a comment.
This revision now requires changes to proceed.


Hmm, actually this doesn't catch `kdesu dolphin` because that doesn't set `SUDO_USER`...

Can you try checking also the `USER` variable?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Mark Gaiser
2018-05-10 19:30:32 UTC
Permalink
markg accepted this revision.
markg added a comment.
This revision is now accepted and ready to land.
I'm sorry, I think I confused you with a stupid typo in the test plan section. :-( Fixed now.
hahaha, that's oke :)

i'm going to give a ship it as it makes sense to me now.
You probably want to wait for others, but for me making it better then it is now is "ship it worthy".

REPOSITORY
R318 Dolphin

BRANCH
allow-execution-as-root-user (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Mark Gaiser
2018-05-10 18:41:53 UTC
Permalink
markg added a comment.
Right: if our uid is zero, but `$SUDO_USER` is set to something, then we're actually a non-root user using sudo, not the root user itself. We have to check `$SUDO_USER` and not `$USER` because `$USER` is set to "root" for the sudo use case.
Does it not work for you? It works for me. Though if there's a better way, I'm all ears. As I'm sure you've noticed by know, I'm a //terrible// programmer, though I'm trying to improve all the time!
I can't test it as dolphin doesn't compile anymore here.
That is due to some change in Baloo DateFormats that is not in the latest released version. Which is all fine, but i don't really feel like setting up the whole KDE environment again (kinda takes a while).

I understand what you say and try to do, but i don't get the usecase (i think you're doing fine as a developer btw).
Why: login as root, followed by **sudo** dolphin?
If it works in the exact way as mentioned in the line above (or the 2nd testcase line) then i don't understand why that works.

if (getuid() == 0 && getenv("SUDO_USER")) {
==
if ROOT && SUDO_USER IS NON NULL
aka
if both root and sudo are set.

Then..
Error.
Log in as the root user and run ~~`sudo~~ dolphin; you can do it.
Sorry if this comes across as nitpicking but i just find it " slightly" confusing and am trying to get it clear for myself :)

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Mark Gaiser
2018-05-10 00:28:27 UTC
Permalink
markg added a comment.
Log in as the root user and run `sudo dolphin; you can do it.
how?
The code basically does:

if (ROOT && SUDO_USER) {
std::cout << ....
}

I'm either missing something or there is a typo in that testcase (drop the sudo).

And a bit of irony, the application one likely uses to become root (in a plasma environment) is... Konsole. Which is also prone of this "really important" security bug.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-10 18:44:04 UTC
Permalink
ngraham edited the test plan for this revision.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-10 18:44:37 UTC
Permalink
ngraham added a comment.


I'm sorry, I think I confused you with a stupid typo in the test plan section. :-( Fixed now.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Mark Gaiser
2018-05-11 18:00:58 UTC
Permalink
markg added a comment.
Post by Vlad Zagorodniy
Post by Nathaniel Graham
The problem is that `USER` is always set to "root" when you use `sudo`. That's why `SUDO_USER` exists, as far as I know.
It looks like kdesu sets KDESU_USER.
Why is kdesu doing it's own thing?
I always considered kdesu to be the "graphical sudo" version.. Can't it just set the environment variables that sudo uses? Perhaps set additional ones if needed, but that it is - in terms of environment variables - compatible with sudo?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-09 23:21:24 UTC
Permalink
ngraham created this revision.
Restricted Application added a project: Dolphin.
Restricted Application edited subscribers, added: kfm-devel; removed: Dolphin.
ngraham requested review of this revision.

REVISION SUMMARY
From the discussion in D12732 <https://phabricator.kde.org/D12732>, it turned out that nobody really objects to allowing dolphin to run as the actual root user, since in that an environment there is no additional security vulnerability beyond what you're already exposing yourself to. So, let's re-enable it.

TEST PLAN
Log in as normal user and run `sudo dolphin`; you get an error message.
Log in as the root user and run `sudo dolphin; you can do it.

REPOSITORY
R318 Dolphin

BRANCH
allow-execution-as-root-user (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D12795

AFFECTED FILES
src/main.cpp

To: ngraham
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Elvis Angelaccio
2018-05-10 20:57:05 UTC
Permalink
elvisangelaccio added a comment.


I'm ok with this change, but I'd also like to hear from @graesslin first. Also, we should submit a similar patch against Kate.

REPOSITORY
R318 Dolphin

BRANCH
allow-execution-as-root-user (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-11 01:44:20 UTC
Permalink
ngraham edited the test plan for this revision.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-11 01:40:13 UTC
Permalink
ngraham updated this revision to Diff 33966.
ngraham added a comment.


Handle the kdesu case

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12795?vs=33918&id=33966

BRANCH
allow-execution-as-root-user (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D12795

AFFECTED FILES
src/main.cpp

To: ngraham, markg, elvisangelaccio
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-10 04:06:09 UTC
Permalink
ngraham added a comment.


Right: if our uid is zero, but `$SUDO_USER` is set to something, then we're actually a non-root user using sudo, not the root user itself. We have to check `$SUDO_USER` and not `$USER` because `$USER` is set to "root" for the sudo use case.

Does it not work for you? It works for me. Though if there's a better way, I'm all ears. As I'm sure you've noticed by know, I'm a //terrible// programmer, though I'm trying to improve all the time!

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-11 01:43:02 UTC
Permalink
ngraham edited the summary of this revision.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Vlad Zagorodniy
2018-05-11 18:19:58 UTC
Permalink
zzag added a comment.
Post by Mark Gaiser
Why is kdesu doing it's own thing?
I always considered kdesu to be the "graphical sudo" version.. Can't it just set the environment variables that sudo uses? Perhaps set additional ones if needed, but that it is - in terms of environment variables - compatible with sudo?
That's not the only problem with kdesu:

- it sets wrong USER
- it doesn't set KDESU_UID
- it doesn't set KDESU_GID
- it doesn't set KDESU_COMMAND
- etc

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-14 14:25:04 UTC
Permalink
ngraham added a comment.


Is this okay now, @elvisangelaccio?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Mark Gaiser
2018-05-14 16:32:23 UTC
Permalink
markg added a comment.


Just curious, am i right in assuming that this "run as root" is ultimately going to ge gone as soon as KAuth is in place?
And if so, how would Dolphin - as root - then work on non-plasma environments?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nicolas Fella
2018-05-14 16:34:54 UTC
Permalink
nicolasfella added a comment.
Post by Mark Gaiser
Just curious, am i right in assuming that this "run as root" is ultimately going to ge gone as soon as KAuth is in place?
And if so, how would Dolphin - as root - then work on non-plasma environments?
I guess dolphin as root will still be relevant for the Kali Linux case. AFAIK the solution we aim for is relying on polkit, which should work across DEs

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-14 16:36:54 UTC
Permalink
ngraham added a comment.


Right, this patch just re-enables running Dolphin as the actual root user (not sudo/kdesu) to fix Kali and other legitimate root user use cases. PolKit support in KIO is definitely the preferred solution for when using Dolphin with a normal user account.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-14 16:55:33 UTC
Permalink
ngraham edited the summary of this revision.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Mark Gaiser
2018-05-20 19:04:11 UTC
Permalink
markg added a comment.
- "You broke my workflow using `sudo dolphin` to edit root-owned files."
- "You broke my use of a root GUI session in $DISTRO."
These are good reactions. Not so good reactions are sending hate mails to developers. If that happens I cannot take them serious. I tried to look up one of the incidents, but I cannot find it on google+ anymore. This has nothing to do that we change something users dislike. We do that constantly. Any change angers users. You will someday also notice this. You are relatively young in the KDE business. With time you will notice that you do changes that angers your users. You do something which improves things and it angers some other users. That's something which happens and we have to live with. We just cannot please all users. The important question is how you react to the change: explain the use cases you have or just let out your anger at random persons. In the latter case it's to me a clear sign that you don't have real arguments.
I did the change in kate with the information about sudoedit which motivated the change in dolphin AFAIK. This broke peoples workflows. It broke it really hard. Nevertheless the overwhelming feedback I got was: "wow awesome I didn't know that sudoedit exists, that's much better than my workflow before." It's totally fine to question users' workflows. With the change in kate I questioned the workflow of running the gui to edit as root in general. That would also be the answer to the first question. You need to edit files as root? Use sudoedit.
Now instead of trying to support the users' workflow of running the complete session as root I would question it. Why do they run it as root? What's the real usecase they have for it? Maybe there is no need, maybe they don't need it and their workflow improves when we can suggest them something better? If I get a request for a new feature I normally ask what's the actual usecase for it. Very often I have a feeling things are requested out of ignorance on how else something can be achieved. We don't need to follow and implement everything in the sake of usability. Usability is also questioning the users and provide the best workflow for them. And that might not be the workflow they are used to. I question that running a session as root is the best workflow for the issues user's try to solve with it.
Yes I used to do that 15 years ago as well. I logged into the suse session as root to do stuff. That was before I learned about sudo, before polkit existed. Workflows change and that's good so. I learned that using Kubuntu with sudo is much easier to use than logging into as root. It was new to me, I wasn't used to the concept of not having a root user. But my workflows improved. It improved because the distribution took away a part I used to have.
It's great if there are better alternatives and in the case of kate it looks good. I also didn't know about sudoedit and seems to be a good tradeoff between security and usability as you get both.
In the case of dolphin you really are destroying a workflow with no alternative.

Some arguments where i had to run root before.

1. A while ago (don't know when or which update) but some update completely ruined my plasma setup. I had to start in either gnome or openbox to remove some configuration files for them to be re-generated.

In this case i did that in a new gui session as root. I believe i started thunar (which shows a warning if you run it as root but allows it) and that is because dolphin wasn't running!
Sure, i could've done this completely on the terminal and just rm the files i wanted to remove. I also could have logged in as the same user under either gnome or openbox which would also have worked (with the risk of seeing errors in .xsession-errors that were not of Plasma or just more difficult in general to figure out what caused an issue). To not pollute those logs with unrelated stuff i logged in as root. Imho, a very valid use case.

2. Another more recent use case was with setting up a home media kodi thingy on a odroid. It all just runs as root. That is a given and just how that particular image worked. Now i had a need for looking up something so i installed a login manager, openbox and dolphin and ran openbox on there for a moment. When i wanted to start dolphin form the command line i was greeted with: "Executing Dolphin as root is not possible." message... We both can come up with a dozen reasons to work around this, but it really makes very little sense to go through those hoops on a single-user device (which the _vast_ majority of plasma users is [1].

In both cases either the command line or thunar was my "fix" but i think the above case are very valid ones. Ones you break for a really minor insignificant security hole that we've been living with for as long as X11 exists (i think).

It's great that you focus on security, more people should do that! In that regard, lots of kudos for that mentality! :)
But you can also go too far. In my opinion blocking dolphin to run under root is a step too far.
You would also have to run a malicious application which is quite unlikely if you stick to vendor packages (but sure, there probably is a very small chance that a malicious package lands in the dist repository).

In all you have to weigh the pros and cons.
The chance that you get infected by a malicious application that does actually exploit this vulnerability is in my opinion really really really slim.
There are much more possible exploits for that: https://www.exploit-db.com/local/
I don't think the cons (allow dolphin to run as root) outweigh the pros (security).

[1] I don't have numbers but i'm very sure the vast majority of plasma users is the only user on that specific hardware.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Martin Flöser
2018-05-20 19:17:54 UTC
Permalink
graesslin added a comment.
Post by Mark Gaiser
You would also have to run a malicious application which is quite unlikely if you stick to vendor packages (but sure, there probably is a very small chance that a malicious package lands in the dist repository).
nope, sorry. The exploit I wrote would work through a drive-by download through an Internet browser. The world we live in sucks :-(
Has the security hole in the web browser that allowed the exploit been fixed?
Unfortunately drive-by downloads are a common thing for browsers. It does not have much to do with security fixes in browsers. It's more of a common thing. In the case of linux the most easy way to get a program running is having the user run chromium. Chromium has the unfortunate default that downloads triggered by the website are automatically downloaded and stored into ~/Downloads. Thus we have a default drive-by download problem. Now to get this into a running binary all you need is to exploit any vulnerability in a file parser running automatically (in our case that would be baloo). Doing that: trivial. Once you have some code running everything is simple. The complete session is unprotected. You get into autostart, etc. etc.

If you are really lucky the browser allows to save to other locations than Download. Then you can just drive-by download a binary, put it into autostart and wait for the session restart.

Getting into a Linux session is damn easy - unfortunately.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Mark Gaiser
2018-05-20 19:57:57 UTC
Permalink
markg added a comment.
The ideas of sandbox baloo_file_extractor are after all based on my sandboxing for kscreenlocker.
Then could you help review it, at least?
I never used an exploit. What I would use is the chrome to download behavior. That is not fixed, it's still the default.
Ah, so the problem is that the user actually //opened// a malicious file. On macOS at least, Finder prompts the user before they can open a file that was auto-downloaded. Perhaps we need to do the same.
baloo is just one example. Every program on the user's system can be abused to it. You can also hope that the user just clicks it. Download a video, which uses a vulnerability in vlc, download a zip file which uses a vulnerability in gzip. There are just so many ways. All you need is a simple bug in an application.
Sounds like you've just described why security is hard. :) You've also described the proper response to a security threat: doing the hard work to harden apps, not the easy and lazy approach of simply disabling a feature that's potentially vulnerable to them. It's not like we should disable opening videos in VLC or zip files in Ark just because there are security vulnerabilities.
Since you've said you prefer to stay in the KWin world, ultimately this is the Dolphin maintainers' decision. We've heard lots of arguments on both sides, now I think it's fine for someone with some authority here to make a decision. However I would note that while not a maintainer, I'm someone who's actively involved in Dolphin's development and who submits a lot of patches, so I hope that counts for more than nothing.
That would be @emmanuelp .
While he is the maintainer, the most active one currently seems to be @elvisangelaccio (by just looking over the past ~3 commit pages https://cgit.kde.org/dolphin.git/log/src).

For the record, this initial change of disallowing dolphin as root should have never been merged with the reason to keep things in "sync" with kate and no further discussion on dolphin for this at all.
Kate != Dolphin, different use cases and different considerations.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-20 17:52:33 UTC
Permalink
ngraham added a comment.


Martin, listening to our users isn't being bullied. If we make a change that upsets our users, that's a really good sign that we might want to reconsider it. Angering our users is completely contrary to the reason why we're doing this in the first place. If we anger our users and drive some of them away, what's the point? We should not dig in our heels and double down; we should instead //listen//. This is what I have tried to do when I encounter this user anger. When I listen, what I hear is two legitimate points:

- "You broke my workflow using `sudo dolphin` to edit root-owned files."
- "You broke my use of a root GUI session in $DISTRO."

For the first point, I tell that PolKit support in KIO is coming, end encourage patience. For the second point, I have no answer. This patch attempts to remedy that situation.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-20 19:12:00 UTC
Permalink
ngraham added a comment.
Post by Mark Gaiser
You would also have to run a malicious application which is quite unlikely if you stick to vendor packages (but sure, there probably is a very small chance that a malicious package lands in the dist repository).
nope, sorry. The exploit I wrote would work through a drive-by download through an Internet browser. The world we live in sucks :-(
Has the security hole in the web browser that allowed the exploit been fixed?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Martin Flöser
2018-05-20 19:19:50 UTC
Permalink
graesslin added a comment.


For more information about Drive-by downloads see: https://en.wikipedia.org/wiki/Drive-by_download

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-20 14:18:39 UTC
Permalink
ngraham marked 2 inline comments as done.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-20 20:18:14 UTC
Permalink
ngraham added a comment.
Actually I find it an interesting discussion going far further than Dolphin. If we can raise more awareness for security issues through such discussions, that's good. For example it just motivated D13008 <https://phabricator.kde.org/D13008>
In that patch, your reasoning for prohibit running KWin as root is that "We cannot guarantee that running KWin as root is secure". <https://phabricator.kde.org/D13008>

In the discussion here, you said that "new vulnerabilities are discovered each and every day" <https://phabricator.kde.org/D12795#265648>. That means we can't guarantee security anywhere either.

Ergo, by your reasoning, we should prohibit running all software because we can't guarantee that it's secure.

I'm sure that this isn't what you meant and that I must be misinterpreting you (it happens all the time!). Can you point out the flaw in my logic so I can understand your actual position?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-20 17:38:14 UTC
Permalink
ngraham added inline comments.

INLINE COMMENTS
graesslin wrote in main.cpp:47
Honestly I don't think we need to support that. If a user really wants that they can patch dolphin. We don't have to support every user wish. If I would have supported every user wish KWin would be an unmaintainable monster.
Also I think it's a really stupid idea to run the session as root. Yes users might do that but we are not obliged to support their crazy ideas. (That reminds me: I need to patch kwin_wayland to disallow running as root)
Security and usability are always in direct competition to each user. Sometimes it's important to lean more towards security and sometimes it's important to go more towards usability. The usability gain here is rather low while at the same time it's a security risk. The improvement you suggest here only benefits a very small subset of our user group (most distros just don't allow logging in as root anyway). Given that I would say that this is a case that the benefits for security are more important than the benefits for usability.
If you're *already* using a root session, what *additional* security is gained by preventing the use of the file manager? Couldn't malicious software own your terminal too?

I know you're against root session use. I'm not in favor of it myself. But IMHO it's not our jobs as DE providers to make this decision for our users or their distros. This change has broken Kali, a popular KDE-using distro. openSUSE has already patched it out. Kali may eventually have to patch it out too, or switch to another DE. I assume that's not what we want...

Ultimately Dolphin's maintainer should make the call, but I really think that this is a case where we shouldn't destroy a part of the user experience in the interests of security. We shouldn't take the lazy way out of just saying, "access blocked, too bad, my job is finished." That's not in line with the focus on Usability and Productivity that the KDE community has voted on. With this patch, I've tried to moderate the security check by re-allowing a use case that does not actually represent an additional security vulnerability, while preserving the original intention. I will let the Dolphin maintainers make the call in the end.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Elvis Angelaccio
2018-05-20 10:05:07 UTC
Permalink
elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS
main.cpp:48
if (getuid() == 0) {
- std::cout << "Executing Dolphin as root is not possible." << std::endl;
- return EXIT_FAILURE;
+ if (getenv("SUDO_USER")) {
+ std::cout << "Executing Dolphin with sudo is not possible due to unfixable security vulnerabilities." << std::endl;
Please use `!qEnvironmentVariableIsEmpty()`. Same below.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Martin Flöser
2018-05-20 18:25:16 UTC
Permalink
graesslin added inline comments.

INLINE COMMENTS
graesslin wrote in main.cpp:47
We also have a focus on security. Your change destroys part of a security improvement we did. Please don't argument with the focus on Usability as security is also a focus.
I give you something to think about for the usability aspect. Users harassed me because of this change, although I have not done it. This has happened several times. Do we want to crave for the small amount of users who go so far as to send hate mail to core KDE developers? I strongly suggest to not give in to such bullying behavior.
Yes insecure distributions like openSUSE decide to patch - and that's fine. That's their decision. We don't need to make their decision our default. Distributions can easily patch this. That's their job. They integrate our products with their product. If they think running dolphin as root is a valid usecase, they can patch.
It's a little bit lame, but overall it's just another case of: https://xkcd.com/1172/

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Martin Flöser
2018-05-20 14:37:32 UTC
Permalink
graesslin added inline comments.

INLINE COMMENTS
main.cpp:47
+ // Prohibit using sudo or kdesu (but allow using the root user directly)
if (getuid() == 0) {
+ if (!qEnvironmentVariableIsEmpty("SUDO_USER")) {
what about:
su root
DISPLAY=:0 dolphin

why should that be allowed given that it has the same risk?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Martin Flöser
2018-05-20 17:26:06 UTC
Permalink
graesslin added inline comments.

INLINE COMMENTS
ngraham wrote in main.cpp:47
Determined people will always get around security measures that annoy them. How would you suggest that we re-enable running Dolphin as the actual root user so that we can un-break people for whom this is important or necessary>
Honestly I don't think we need to support that. If a user really wants that they can patch dolphin. We don't have to support every user wish. If I would have supported every user wish KWin would be an unmaintainable monster.

Also I think it's a really stupid idea to run the session as root. Yes users might do that but we are not obliged to support their crazy ideas. (That reminds me: I need to patch kwin_wayland to disallow running as root)

Security and usability are always in direct competition to each user. Sometimes it's important to lean more towards security and sometimes it's important to go more towards usability. The usability gain here is rather low while at the same time it's a security risk. The improvement you suggest here only benefits a very small subset of our user group (most distros just don't allow logging in as root anyway). Given that I would say that this is a case that the benefits for security are more important than the benefits for usability.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Martin Flöser
2018-05-21 08:37:31 UTC
Permalink
graesslin added a comment.
Actually I find it an interesting discussion going far further than Dolphin. If we can raise more awareness for security issues through such discussions, that's good. For example it just motivated D13008 <https://phabricator.kde.org/D13008>
In that patch, your reasoning for prohibiting running KWin as root is that "We cannot guarantee that running KWin as root is secure". <https://phabricator.kde.org/D13008>
In the discussion here, you said of other software (including our own) that "new vulnerabilities are discovered each and every day" <https://phabricator.kde.org/D12795#265648>. That means we can't guarantee security anywhere else either.
Ergo, by your reasoning, we should prohibit running all software because we can't guarantee that it's secure.
Erm no. In this and the other software it's about running software as root while at the same time other software is running as a less privileged user. What KWin cannot guarantee is that it is secure if it is running as root and other applications are running as less privileged users. KWin as the windowing system is connected through a socket with applications running on the same or different host. KWin was developed as an X11 window manager with the situation that the X server was running as root while the window manager (KWin) was running as a less privileged user and all other applications are running as the same user. Attacking KWin didn't make any sense. You couldn't gain more privileges compared to what the application already has. The thing to attack was the X Server.

Now with Wayland the situation changes. KWin moves into the place the X Server used to be. If KWin runs with higher privileges than other applications it is an attack target. But KWin was not developed for such a situation. Thus it cannot guarantee that this is secure for this usecase. Especially as it loads toolkits which don't guarantee this (most crashes in KWin are after all in libGL).

What's important is to evaluate the threat level of an application. KWin_Wayland has a very low threat level. It runs as the same user as all other software in the session. Trying to attack KWin doesn't gain you anything. On the other hand if we have sandboxed applications attacking KWin becomes interesting again. Then the windowing system would be a way to get more privileges than the application currently has. That is something we haven't protected yet. We could say KWin needs to take care of it, or that the sandbox needs to take care of it.

In the case of random applications the threat level is quite different. Let's take gwenview. The threat level of gwenview are the image formats it loads and parses. A malicious drafted image could abuse gwenview to get code executed. That is a threat level KWin doesn't have.

So in summary it's always important to look at the threat level and how to protect or mitigate against it.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Mark Gaiser
2018-05-17 23:45:29 UTC
Permalink
markg accepted this revision.
markg added a comment.


I think this one has been open for long enough for people to catch it if they want to voice their opinion.
@elvisangelaccio still needs to make his red light green but after that i'd just ship it.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Mark Gaiser
2018-05-20 19:33:10 UTC
Permalink
markg added a comment.
Post by Mark Gaiser
You would also have to run a malicious application which is quite unlikely if you stick to vendor packages (but sure, there probably is a very small chance that a malicious package lands in the dist repository).
nope, sorry. The exploit I wrote would work through a drive-by download through an Internet browser. The world we live in sucks :-(
You've just completely ignored my valid use cases (which you asked for some comments earlier).

Sure, drive-by-downloads is then an issue.. That is completely unrelated to this issue. It has nothing to do with Dolphin.
It is "one of the steps" needed to potentially exploit the system. But this requires stupid users who click on everything apparently. I personally never trust downloads from those drive-by pages unless i'm expecting the page to give me a download.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-17 22:02:44 UTC
Permalink
ngraham added a reviewer: Dolphin.
ngraham added a comment.


@elvisangelaccio? Anyone else?
[...] from a _user_ standpoint, it's something else altogether. For a user, pretty much EVERY SINGLE TIME, it wasn't actually a security attack at all, it was just a latent bug that got exposed. And the keyword here is that it was _latent_, and things used to work, and the hardening patch did something - probably fairly drastic - to turn it from "dangerous" to "benign" from a security perspective.
So from a user standpoint, the hardening was just a big nasty annoyance, and probably made their workflow _break_, without actually helping their case at all, because they never really saw the original bug as a problem to begin with.
[...]
when adding hardening features, you as a security person should always see that hardening to be the _endpoint_, but not the immediate goal. When adding hardening features, the first step should *ALWAYS* be "just report it". Not killing things, not even stopping the access. Report it. Nothing else.
"Do no harm" should be your mantra for any new hardening work. And that "do no harm" may feel antithetical to the whole point. You go "but that doesn't work - then the bug still exists".
But remember - keep your eye on the endpoint, and that this is just the first step. You need to not piss off users, and you need to not piss of developers. Because if you as a security person just piss off users, and piss off developers, I'm not going to take your work, and I'm going to call you a bad security person.
Because in the end, those users really do matter. Without those users, your system may be "secure", but all your security work was still just masturbation. You didn't do anything useful at all in the end.
This patch refines the security check and un-breaks a legitimate use case that we broke over a year ago (causing much user anger and pushing some to use a different file manager). I'd like to get it in if there are no objections.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Martin Flöser
2018-05-20 19:09:14 UTC
Permalink
graesslin added a comment.
Post by Mark Gaiser
You would also have to run a malicious application which is quite unlikely if you stick to vendor packages (but sure, there probably is a very small chance that a malicious package lands in the dist repository).
nope, sorry. The exploit I wrote would work through a drive-by download through an Internet browser. The world we live in sucks :-(

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Martin Flöser
2018-05-20 19:07:21 UTC
Permalink
graesslin added a comment.
I've deal with a lot of criticism in my time, Martin. But I have a thick enough skin to avoid taking it personally or letting it affect my judgment. If some jerks send hate mail, that is not a good reason to say "Now I //really// won't do it!" and punishing everyone else over it.
Don't get me wrong. That's not what I'm doing here. I don't want to have a thick skin, I expect people to behave. Nobody would say to my face "you are an asshole for removing this feature". On the Internet people do. If that happens in bug reports I expect users to step up and tell the person. If it happens through google+, reddit or similar things, I report to the hosting provider. If I see that the person is from Germany, I'll consider going to the police (yep insulting is a punishable act in Germany).

What I tend to do is to not punish users for their behavior, instead I do not reward them. I do not give their requests additional attention. It stays in the queue with the many items of "yeah would be nice if we had infinite time". It doesn't affect my judgment, neither to the positive nor to the negative. What I see quite often is that users try to play to get their pet issues to the front of the queue. And that's something I don't reward. Be it through insulting, be it through requesting users to comment on bug, be it to request to tell to you :-)

In this case my judgment is that I would not have implemented the change. But now that we have it, I think it would be more harming to remove it or alter it. We always have to fight for security. We see bullshit recommendations from EFF to not encrypt mails, although PGP is not broken. It's a bad time for security, we need to fight. Thus I have a bad feeling with the thought of weakening the constraints here.
But for the root use case, I continue to not see any harm in re-enabling this.
the harm is that you cannot enable it without having ways where it can be run in the user session. I found a hole in your change. That's the problem. The security suffers by giving it to the weird use case.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-20 19:26:43 UTC
Permalink
ngraham added a comment.
Post by Martin Flöser
Unfortunately drive-by downloads are a common thing for browsers. It does not have much to do with security fixes in browsers. It's more of a common thing.
On the contrary, drive-by-downloads are a major concern for browser vendors to fix. If someone reports one to them, they're //very motivated to fix it. Was the exploit that you used ever reported?
Post by Martin Flöser
Now to get this into a running binary all you need is to exploit any vulnerability in a file parser running automatically (in our case that would be baloo). Doing that: trivial. Once you have some code running everything is simple. The complete session is unprotected. You get into autostart, etc. etc.
OK, so let's harden Baloo! An excellent plan. With your security skills, would yo like to help out with the code review on D8532 <https://phabricator.kde.org/D8532>?

This is exactly what Linus Torvalds is talking about in https://lkml.org/lkml/2017/11/21/356. Simply blocking the access is the easy, lazy way out that doesn't actually provide much real security (if we push our users to instead use other file managers as root or sudo, we haven't really gained any security). The //real// way to secure things is to attack things closer to the source: harden the browsers, sandbox `baloo_file_extractor`, etc. Since you care so much about our security, would you like to help out with those?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Martin Flöser
2018-05-20 18:14:55 UTC
Permalink
graesslin added a comment.
- "You broke my workflow using `sudo dolphin` to edit root-owned files."
- "You broke my use of a root GUI session in $DISTRO."
These are good reactions. Not so good reactions are sending hate mails to developers. If that happens I cannot take them serious. I tried to look up one of the incidents, but I cannot find it on google+ anymore. This has nothing to do that we change something users dislike. We do that constantly. Any change angers users. You will someday also notice this. You are relatively young in the KDE business. With time you will notice that you do changes that angers your users. You do something which improves things and it angers some other users. That's something which happens and we have to live with. We just cannot please all users. The important question is how you react to the change: explain the use cases you have or just let out your anger at random persons. In the latter case it's to me a clear sign that you don't have real arguments.

I did the change in kate with the information about sudoedit which motivated the change in dolphin AFAIK. This broke peoples workflows. It broke it really hard. Nevertheless the overwhelming feedback I got was: "wow awesome I didn't know that sudoedit exists, that's much better than my workflow before." It's totally fine to question users' workflows. With the change in kate I questioned the workflow of running the gui to edit as root in general. That would also be the answer to the first question. You need to edit files as root? Use sudoedit.

Now instead of trying to support the users' workflow of running the complete session as root I would question it. Why do they run it as root? What's the real usecase they have for it? Maybe there is no need, maybe they don't need it and their workflow improves when we can suggest them something better? If I get a request for a new feature I normally ask what's the actual usecase for it. Very often I have a feeling things are requested out of ignorance on how else something can be achieved. We don't need to follow and implement everything in the sake of usability. Usability is also questioning the users and provide the best workflow for them. And that might not be the workflow they are used to. I question that running a session as root is the best workflow for the issues user's try to solve with it.

Yes I used to do that 15 years ago as well. I logged into the suse session as root to do stuff. That was before I learned about sudo, before polkit existed. Workflows change and that's good so. I learned that using Kubuntu with sudo is much easier to use than logging into as root. It was new to me, I wasn't used to the concept of not having a root user. But my workflows improved. It improved because the distribution took away a part I used to have.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-20 19:48:39 UTC
Permalink
ngraham added a comment.
The ideas of sandbox baloo_file_extractor are after all based on my sandboxing for kscreenlocker.
Then could you help review it, at least?
I never used an exploit. What I would use is the chrome to download behavior. That is not fixed, it's still the default.
Ah, so the problem is that the user actually //opened// a malicious file. On macOS at least, Finder prompts the user before they can open a file that was auto-downloaded. Perhaps we need to do the same.
baloo is just one example. Every program on the user's system can be abused to it. You can also hope that the user just clicks it. Download a video, which uses a vulnerability in vlc, download a zip file which uses a vulnerability in gzip. There are just so many ways. All you need is a simple bug in an application.
Sounds like you've just described why security is hard. :) You've also described the proper response to a security threat: doing the hard work to harden apps, not the easy and lazy approach of simply disabling a feature that's potentially vulnerable to them. It's not like we should disable opening videos in VLC or zip files in Ark just because there are security vulnerabilities.

Since you've said you prefer to stay in the KWin world, ultimately this is the Dolphin maintainers' decision. We've heard lots of arguments on both sides, now I think it's fine for someone with some authority here to make a decision. However I would note that while not a maintainer, I'm someone who's actively involved in Dolphin's development and who submits a lot of patches, so I hope that counts for more than nothing.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Mark Gaiser
2018-05-20 13:43:33 UTC
Permalink
markg added inline comments.

INLINE COMMENTS
elvisangelaccio wrote in main.cpp:48
Please use `!qEnvironmentVariableIsEmpty()`. Same below.
Why did Qt even add such a non-qt name?
qgetenv("SUDO_USER").isEmpty()

is more Qt-ish. The qgetenv gives a QByteArray and the isEmpty is just a check on the bytearray and verbose enough imho.

I'm fine with any of the options though.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Martin Flöser
2018-05-20 19:39:50 UTC
Permalink
graesslin added a comment.
Post by Mark Gaiser
Post by Mark Gaiser
You would also have to run a malicious application which is quite unlikely if you stick to vendor packages (but sure, there probably is a very small chance that a malicious package lands in the dist repository).
nope, sorry. The exploit I wrote would work through a drive-by download through an Internet browser. The world we live in sucks :-(
You've just completely ignored my valid use cases (which you asked for some comments earlier).
Nope. I haven't asked for use cases. I read them and I don't think they are reason enough to change it. Sorry :-)
Post by Mark Gaiser
Sure, drive-by-downloads is then an issue.. That is completely unrelated to this issue. It has nothing to do with Dolphin.
It is "one of the steps" needed to potentially exploit the system. But this requires stupid users who click on everything apparently. I personally never trust downloads from those drive-by pages unless i'm expecting the page to give me a download.
The whole idea of drive-by downloads is to make them happen without the user noticing. So please no victim blaming.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Christoph Feck
2018-05-20 13:46:51 UTC
Permalink
cfeck added a comment.


qgetenv("SUDO_USER").isEmpty()

is slower than

qEnvironmentVariableIsEmpty("SUDO_USER")

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Martin Flöser
2018-05-20 17:44:59 UTC
Permalink
graesslin added inline comments.

INLINE COMMENTS
ngraham wrote in main.cpp:47
If you're *already* using a root session, what *additional* security is gained by preventing the use of the file manager? Couldn't malicious software own your terminal too?
I know you're against root session use. I'm not in favor of it myself. But IMHO it's not our jobs as DE providers to make this decision for our users or their distros. This change has broken Kali, a popular KDE-using distro. openSUSE has already patched it out. Kali may eventually have to patch it out too, or switch to another DE. I assume that's not what we want...
Ultimately Dolphin's maintainer should make the call, but I really think that this is a case where we shouldn't destroy a part of the user experience in the interests of security. We shouldn't take the lazy way out of just saying, "access blocked, too bad, my job is finished." That's not in line with the focus on Usability and Productivity that the KDE community has voted on. With this patch, I've tried to moderate the security check by re-allowing a use case that does not actually represent an additional security vulnerability, while preserving the original intention. I will let the Dolphin maintainers make the call in the end.
We also have a focus on security. Your change destroys part of a security improvement we did. Please don't argument with the focus on Usability as security is also a focus.

I give you something to think about for the usability aspect. Users harassed me because of this change, although I have not done it. This has happened several times. Do we want to crave for the small amount of users who go so far as to send hate mail to core KDE developers? I strongly suggest to not give in to such bullying behavior.

Yes insecure distributions like openSUSE decide to patch - and that's fine. That's their decision. We don't need to make their decision our default. Distributions can easily patch this. That's their job. They integrate our products with their product. If they think running dolphin as root is a valid usecase, they can patch.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Martin Flöser
2018-05-20 19:38:00 UTC
Permalink
graesslin added a comment.
Post by Nathaniel Graham
Post by Martin Flöser
Unfortunately drive-by downloads are a common thing for browsers. It does not have much to do with security fixes in browsers. It's more of a common thing.
On the contrary, drive-by-downloads are a major concern for browser vendors to fix. If someone reports one to them, they're //very motivated to fix it. Was the exploit that you used ever reported?
I never used an exploit. What I would use is the chrome to download behavior. That is not fixed, it's still the default.
Post by Nathaniel Graham
Post by Martin Flöser
Now to get this into a running binary all you need is to exploit any vulnerability in a file parser running automatically (in our case that would be baloo). Doing that: trivial. Once you have some code running everything is simple. The complete session is unprotected. You get into autostart, etc. etc.
OK, so let's harden Baloo! An excellent plan.
baloo is just one example. Every program on the user's system can be abused to it. You can also hope that the user just clicks it. Download a video, which uses a vulnerability in vlc, download a zip file which uses a vulnerability in gzip. There are just so many ways. All you need is a simple bug in an application.

As long as browsers are not in a sandbox and not run as a different user, allowing to save files directly to the hard disk, we need to see them as a threat to the user. Yes browser vendors care about drive-by download. Nevertheless they are currently state of the art and that won't change. We need to accept that this is currently the threat level we have to protect against. Hardening kate was one of the ideas I had to protect here. Help to ensure that applications cannot gain root after they got installed.
Post by Nathaniel Graham
This is exactly what Linus Torvalds is talking about in https://lkml.org/lkml/2017/11/21/356. Simply blocking the access is the easy, lazy way out that doesn't actually provide much real security (if we push our users to instead use other file managers as root or sudo, we haven't really gained any security). The //real// way to secure things is to attack things closer to the source: harden the browsers, sandbox `baloo_file_extractor`, etc. Since you care so much about our security, would you like to help out with those?
I rather stay on KWin :-) I help on security there by pushing Wayland and getting rid of that huge insecure X11 nightmare. I help where I can. The ideas of sandbox baloo_file_extractor are after all based on my sandboxing for kscreenlocker.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-20 14:15:52 UTC
Permalink
ngraham updated this revision to Diff 34532.
ngraham added a comment.


Use qEnvironmentVariableIsEmpty()

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12795?vs=33966&id=34532

BRANCH
allow-execution-as-root-user (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D12795

AFFECTED FILES
src/main.cpp

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Martin Flöser
2018-05-20 20:02:39 UTC
Permalink
graesslin added a comment.
The ideas of sandbox baloo_file_extractor are after all based on my sandboxing for kscreenlocker.
Then could you help review it, at least?
Given that it's based on my code I suggest that someone else reviews as there is a chance that if I did a mistake, which got repeated, I wouldn't notice either.
I never used an exploit. What I would use is the chrome to download behavior. That is not fixed, it's still the default.
Ah, so the problem is that the user actually //opened// a malicious file. On macOS at least, Finder prompts the user before they can open a file that was auto-downloaded. Perhaps we need to do the same.
It's one of the problems. To explain again: a website can trigger downloads in Chromium. They are by default saved to the user's home directory without (!) asking or informing the user about it. If there is any application running which monitors the directory and loads the files, you have lost.

Examples are:

- baloo indexing
- dolphin showing previews
- gwenview showing previews
- music player picking up new audio files
- video players picking up new audio files
- many more

They all act on the new data, because usability suggests to show the new downloaded files. And that's good! But it opens a risk. If there is a vulnerability, the user gets owned. Unfortunately we cannot harden against all, because new vulnerabilities are discovered each and every day. So yes mitigation is unfortunately one of the things we need and have to consider as strategy. What you describe from OSX does not protect against such problems. Unless we change the operating system and get every application to implement the new hooks to save downloaded files.
baloo is just one example. Every program on the user's system can be abused to it. You can also hope that the user just clicks it. Download a video, which uses a vulnerability in vlc, download a zip file which uses a vulnerability in gzip. There are just so many ways. All you need is a simple bug in an application.
Sounds like you've just described why security is hard. :) You've also described the proper response to a security threat: doing the hard work to harden apps, not the easy and lazy approach of simply disabling a feature that's potentially vulnerable to them. It's not like we should disable opening videos in VLC or zip files in Ark just because there are security vulnerabilities.
no, hardening does not help. It's part of the picture, but mitigation is as well.
Since you've said you prefer to stay in the KWin world, ultimately this is the Dolphin maintainers' decision. We've heard lots of arguments on both sides, now I think it's fine for someone with some authority here to make a decision. However I would note that while not a maintainer, I'm someone who's actively involved in Dolphin's development and who submits a lot of patches, so I hope that counts for more than nothing.
Actually I find it an interesting discussion going far further than Dolphin. If we can raise more awareness for security issues through such discussions, that's good. For example it just motivated D13008 <https://phabricator.kde.org/D13008>

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-20 18:38:07 UTC
Permalink
ngraham added a subscriber: chinmoyr.
ngraham added a comment.


I've deal with a lot of criticism in my time, Martin. But I have a thick enough skin to avoid taking it personally or letting it affect my judgment. If some jerks send hate mail, that is not a good reason to say "Now I //really// won't do it!" and punishing everyone else over it.

There are indeed legitimate use cases for using a root session from time to time, IMHO. I recently experienced this myself when I was setting up a new GNOME-based RHEL build machine at work recently(and yes, it needed a GUI so we could perform UI tests on it) and the only user I had available was the root user. There was no network connection yet so there were no security implications, and also the box is behind a beefy enterprise intranet firewall. I logged in as root and made some configuration changes, no big deal. It occurred to me that if the machine was using an up-to-date KDE-based distro, this would have been annoying for no good reason. Yes: I could have done everything via the command line. Of course. But not as quickly, for many tasks. For example, I was able to configure the network settings in 15 seconds via the GUI. Via the command line, it would have taken me 15 minutes to look up the way to do it on this particular platform and version (the methods and files in question are subtly different between CentOS 5 6 and 7, and Ubuntu 12, 14, and 16). I am an IT professional, and from time to time I find myself using a root GUI session. It's not ideal, but in the real world, it does happen.

Again, I am willing to accept breaking the `sudo dolphin` use case because PolKit support is coming (hopefully soon, thanks to @chinmoyr), which does indeed represent a superior alternative. In this case, I will deal with user anger in the short term and attempt to soothe frayed nerves by promising that something better is coming.

But for the root use case, I continue to not see any harm in re-enabling this. It adds no support burdens--in fact it will remove some since we can close some Bugzilla tickets and prevent a measure of user anger). And again: it's not our jobs to make these decisions for our users. We should treat them like capable adults who can make their own decisions. "Sorry, this is too insecure, you can't do it at all" is just insulting, especially for people who understand that the barrier is a policy decisions rather than a technical impediment. This was the purpose of the proposed warning in D12732 <https://phabricator.kde.org/D12732>: not to block access, but to say, "You're on your own here, and if you get hurt, we warned you!" Intelligent adults can make their own decisions, especially if they're provided with adequate warning about the potential consequences.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-20 16:23:40 UTC
Permalink
ngraham added inline comments.

INLINE COMMENTS
graesslin wrote in main.cpp:47
su root
DISPLAY=:0 dolphin
why should that be allowed given that it has the same risk?
Determined people will always get around security measures that annoy them. How would you suggest that we re-enable running Dolphin as the actual root user so that we can un-break people for whom this is important or necessary>

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-25 18:35:56 UTC
Permalink
ngraham added a comment.


I'm still waiting on any other #dolphin <https://phabricator.kde.org/tag/dolphin/> folks (in particular, @elvisangelaccio) to make the call here. I think we've heard a lot of arguments on both sides, now it's time to make a decision.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Elvis Angelaccio
2018-05-26 15:38:14 UTC
Permalink
elvisangelaccio added a comment.


@ngraham Did you submit the same patch against Kate?

I can approve the current version of this patch, but I think we need to keep Kate and Dolphin aligned.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-26 21:06:26 UTC
Permalink
ngraham added a comment.
Post by Elvis Angelaccio
@ngraham Did you submit the same patch against Kate?
Now I have: https://phabricator.kde.org/D13138

I wanted to keep the discussion limited to one patch if possible so we wouldn't have to go through the same thing twice.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Mark Gaiser
2018-05-27 12:44:58 UTC
Permalink
markg accepted this revision.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-31 18:17:32 UTC
Permalink
ngraham added a comment.


The change for Kate was accepted and just landed. Since @elvisangelaccio gave his written approval for this once the companion patch for Kate went in, I'll land it now.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
Nathaniel Graham
2018-05-31 18:18:27 UTC
Permalink
This revision was not accepted when it landed; it landed in state "Needs Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R318:40453cb627a3: Re-allow running Dolphin as the root user (but still not using sudo) (authored by ngraham).

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12795?vs=34532&id=35272

REVISION DETAIL
https://phabricator.kde.org/D12795

AFFECTED FILES
src/main.cpp

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, spoorun, navarromorales, isidorov, firef, andrebarros
F. Fox
2018-12-04 10:49:28 UTC
Permalink
firef added a comment.


"Prohibiting the use of Dolphin as the actual root user (not using sudo or kdesu) breaks legitimate use cases for using the root user. "

This means that the original prohibition caused great harm to the KDE community and was a grave mistake.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12795

To: ngraham, markg, elvisangelaccio, #dolphin
Cc: firef, chinmoyr, cfeck, elvisangelaccio, mmustac, Fuchs, markg, graesslin, nicolasfella, zzag, kfm-devel, emmanuelp, alexde, sourabhboss, feverfew, spoorun, navarromorales, andrebarros, mikesomov
Loading...