[SECURITY] Do not configure kdesu path and remove kdesudo support again
ClosedPublic

Authored by palant on Feb 22 2017, 7:04 PM.

Details

Summary

Note: I restricted access to Krusader project members. If the issue isn't considered severe enough this review can be made public.

The scenario here is a user who one way or another executed malware. This malware will be restricted by user's privileges until in can hijack a sudo session and seize full control of the computer. Krusader currently makes it unnecessarily easy for it because kdesu path is configured. So the malware can change the configuration (only user's privileges requires) to point to its own kdesu look-alike. Next time the user does something like "Start root mode Krusader" it steals the password and can do whatever it needs.

The only way to prevent this is not relying on user-specific configuration in order to locate kdesu, which is what my patch does. I assume that it will work on non-Ubuntu distributions as well. Only issue I noticed with my current solution: if KDE was compiled with a different value for CMAKE_INSTALL_PREFIX than Krusader then kdesu won't be found. In that case KDE_INSTALL_FULL_LIBEXECDIR_KF5 needs to be overridden via cmake parameters.

Elevation prompts running with user's privileges are generally problematic, malware applications might try to mess with them - like making them crash and reading the root password from the core dump. I'm not convinced that kdesu protects against potential manipulation effectively but at least it tries, so the core dump issue is covered. kdesudo on the other hand implements the naive approach without any protection whatsoever. This project also appears abandoned so I essentially reverted D4645. For reference, the issue wasn't that Ubuntu doesn't have kdesu, it's merely not in PATH.

This change is only the first step. User actions are using kdesu without going through krsevices. Krusader path shouldn't be configured either. And there are probably more issues. In the end, what we are doing here is making the attacker's job harder and less likely to go unnoticed. But in theory malware could even install its own malicious Krusader fork - an application file in user's home directory can make sure that it runs instead of the real thing.

Diff Detail

Repository
R167 Krusader
Lint
Lint Skipped
Unit
Unit Tests Skipped
palant created this revision.Feb 22 2017, 7:04 PM
palant created this object with visibility "Krusader (Project)".
palant created this object with edit policy "Krusader (Project)".
martinkostolny accepted this revision.Feb 22 2017, 11:15 PM
martinkostolny added a subscriber: martinkostolny.

I agree, security is now better with this patch. It works on my Arch Linux. Thanks for taking care of this!

As another step towards security maybe we could make use of sudoedit program? Martin Grässlin recently blogged about it: https://blog.martin-graesslin.com/blog/2017/02/editing-files-as-root/

It is quite new to me but I believe we can make use of this at least in our internal editor. I have to check his commits, maybe it will be supported by ReadWritePart in KF which we happen to already use.

PS: there is a small problem with your diff - see my comment inside diff (regarding krresulttable.cpp).

krusader/Konfigurator/krresulttable.cpp
280

There is a trailing spaces block on this line which prevents from applying the patch.

This revision is now accepted and ready to land.Feb 22 2017, 11:15 PM
palant updated this revision to Diff 11659.Feb 23 2017, 8:32 AM
palant marked an inline comment as done.

The bogus whitespace in the patch should be gone now. Looks like I should stop copying patches from the terminal, it messed up whitespace somehow.

palant added a comment.EditedFeb 23 2017, 11:32 AM

Martin, thank you for the pointer to https://blog.martin-graesslin.com/blog/2017/02/editing-files-as-root/, this is very useful information (I'm not really familiar with Linux and X11 security architecture). I created D4738 to address editing files as root. However, the more important implication here is that running Krusader as root is clearly a bad idea. At the very least, this functionality deserves a huge warning pointing out that it isn't safe. But I'd also like to make it unnecessary for the most important scenarios. As discussed on the mailing list, copying and moving files to write protected directories should offer "Execute as root" as another option beyond "Retry."

asensi added a subscriber: asensi.Feb 23 2017, 11:53 AM

Hi!

you shouldn't run graphical applications as root

Only if you run malware. Please, Krusader in root mode is very useful. We can keep it.

Martin pointed me to https://blog.martin-graesslin.com/blog/2017/02/editing-files-as-root/

In that page a lot of comments are written about the problems that would appear. There are two options:

  1. Leave people without Krusader in root mode, etc. (just because someone executes malware, but that is already very harmful)
  2. Continue using Krusader in root mode when needed (and if someone executes malware, he already has huge problems, at least he shouldn't hinder other people).
palant added a comment.EditedFeb 23 2017, 1:58 PM

There is more information in a presentation by the same Martin Gräßlin. I was mostly interested in the details of his proof of concept but the other information turned out very useful as well. So what we are talking about is an open secret and X11 is inherently insecure (e.g. even with the changes here a key logger can still steal the password entered into kdesu). Things will hopefully get better with Wayland, eventually. But from the look of it, there is little point in keeping this review secret.

In D4725#88923, @asensi wrote:

Only if you run malware.

Do you? How would you know?

Drive-by downloads are an issue for Linux as well. If your software (especially browser and plugins) is outdated then you are likely vulnerable. I don't think that all Linux users install updates in a timely fashion but even if they do - Martin Gräßlin mentions QtWebKit as one example where an outdated WebKit version is used. So any application building on that component is vulnerable. You don't need to visit "shady" websites, popular websites and ad services have been affected in the past. And once the malware gets to execute it is easy for it to stay active in your account and you won't even know. Linux is rarely being targeted by malware but that's simply because malware authors don't want to learn new tricks for the little gain.

Please, Krusader in root mode is very useful. We can keep it.

I did not suggest removing this feature, not at this point - I merely suggested adding a warning to it. But it would certainly help if you could tell what you need root mode Krusader for, beyond copying and moving files to protected directories as well as editing in root mode.

Martin pointed me to https://blog.martin-graesslin.com/blog/2017/02/editing-files-as-root/

In that page a lot of comments are written about the problems that would appear.

Yes, and most of these comments can be summed up as "I don't care about security, just don't make me change anything!" Which is summed up nicely by page 23 in the presentation I linked to above. As D4738 demonstrates, there are clearly secure alternatives to sudo kwrite. But it requires people to learn new things.

There are two options:

  1. Leave people without Krusader in root mode, etc. (just because someone executes malware, but that is already very harmful)
  2. Continue using Krusader in root mode when needed (and if someone executes malware, he already has huge problems, at least he shouldn't hinder other people).

I am suggesting a third: make Krusader in root mode unnecessary because people can elevate for specific operations as opposed to constantly running in elevated mode.

Edit: If I understand https://bugs.kde.org/show_bug.cgi?id=179678 correctly, this proposal would achieve exactly that, on a lower level.

asensi added a comment.EditedFeb 23 2017, 3:12 PM

I am suggesting a third: make Krusader in root mode unnecessary because people can elevate for specific operations as opposed to constantly running in elevated mode.

Martin Gräßlin wrote in that page:

GUI Dialog is to late! You are owned then.

most of these comments can be summed up as "I don't care about security, just don't make me change anything!"

That is called a straw man argument, that breaks objective conversations. This is another:

"Most of those comments can be summed up as: I don't care about hindering people when they need to run Krusader as root, just in case I'm running malware"

As we can see, straw man arguments lead people nowhere.

abika requested changes to this revision.Feb 23 2017, 3:30 PM
abika added a subscriber: abika.

TL;DR: I agree with Wladimir and would recommend to remove Krusader root mode altogether because we shouldn't support unsafe usage.


Note: I restricted access to Krusader project members. If the issue isn't considered severe enough this review can be made public.

The setting is public for everybody. If an attacker wanted to exploit it he/she could have done in the last ~15years.

So the malware can change the configuration (only user's privileges requires) to point to its own kdesu look-alike. Next time the user does something like "Start root mode Krusader" it steals the password and can do whatever it needs.

That's true and I haven't thought about it. But this is a general problem and also the case for sudo. Anyway, yes: everywhere Krusader uses kdesu we should only search in bin paths with root access. There are problems with the current patch, please see the comments.

Another topic is if we should support running Qt GUI applications as root and I say: no!
Martin Gräßlins arguments are already enough as justification. And its not like we are making it impossible: anybody can do sudo krusader anytime. But if I know this is a security risk I cannot take on responsibility and provide a desktop icon that suggests "this is safe to use".
I imagine this: Some new Linux user coming from Windows was used to Total Commander, now uses Krusader. He/she want's to copy something to /boot because of a great suggestion from "the internet". This fails but there is Krusader root mode coming to the aid. Next time shitty Linux does not start anymore...

And I myself don't use GUI applications as root for years now but everybody is free to do so.
@Toni: you can simply create the Krusader root icon on your own and/or an User Action for it and you won't loose any functionality. But the point is, our software shouldn't suggest this is a safe way of doing things.

Martin Gräßlins insights are new to me, now that I know about it I have react. At least before publishing another release.

CMakeLists.txt
92

Where did you find KDE_INSTALL_FULL_LIBEXECDIR_KF5?
And why not KDE_INSTALL_LIBEXECDIR?

krusader/Konfigurator/kgdependencies.cpp
71

row index is fixed in D4734, please don't mix up diffs.

krusader/krservices.cpp
88

suggestion: this method is doing something else now for the input "kdesu". So why not moving this to a new method? Better style imo. And string hardcoding is not necessary anymore.

91

This will only work on Ubuntu. E.g. on Arch kdesu is in /usr/bin.

We actually have to go through the list of common "bin" paths: /bin/, /usr/bin, /sbin/, /usr/local/bin/, ...

This revision now requires changes to proceed.Feb 23 2017, 3:30 PM
abika added a comment.Feb 23 2017, 3:50 PM

Wait, screw that. I mixed up arguments.

  • support for running Qt GUI applications as root (including Krusader root mode)?: No, we have to remove that because of potential security risks.
  • support for doing (non-GUI) tasks as root?: well, yes maybe. I cannot recommend it, but if somebody wants to implement it...

That said

I am suggesting a third: make Krusader in root mode unnecessary because people can elevate for specific operations as opposed to constantly running in elevated mode.

is a valid option.

palant added inline comments.Feb 23 2017, 7:42 PM
CMakeLists.txt
92

Here: https://cgit.kde.org/kde-cli-tools.git/tree/kdesu/CMakeLists.txt?id=60d959d59da9dbde1ed9572b649fa3742c33315d#n12 - yes, doesn't seem properly documented.

KDE_INSTALL_LIBEXECDIR is not the same directory. First of all, it's a relative one, you would need KDE_INSTALL_FULL_LIBEXECDIR. On Ubuntu it points to /usr/lib/x86_64-linux-gnu/libexec whereas the install path of kdesu is /usr/lib/x86_64-linux-gnu/libexec/kf5.

krusader/krservices.cpp
91

Don't think so, my understanding is that KDE_LIBEXEC_DIR is going to be /usr/bin on Arch. But any distribution overriding the install target for kdesu should be able to configure Krusader on build accordingly.

abika added inline comments.Feb 23 2017, 8:16 PM
CMakeLists.txt
92
krusader/krservices.cpp
91

KDE_INSTALL_FULL_LIBEXECDIR_KF5 is /usr/lib64/libexec/kf5/ on Arch.

palant added inline comments.Feb 23 2017, 9:00 PM
krusader/krservices.cpp
91

That's strange, how come it worked for Martin on Arch Linux then?

Looking at https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/kde-cli-tools, Arch Linux is indeed overriding KDE_INSTALL_LIBEXECDIR in order to get kdesu installed under /usr/lib/kf5 (/usr/bin/kdesu is merely a symlink). Is it too much to expect them to do the same for Krusader?

palant changed the visibility from "Krusader (Project)" to "Public (No Login Required)".Feb 26 2017, 2:41 PM
palant changed the edit policy from "Krusader (Project)" to "All Users".
palant updated this revision to Diff 11857.Feb 26 2017, 3:57 PM
palant edited edge metadata.
palant marked an inline comment as done.

Uploaded a new patch, now there is an explicit build variable called KDESU_PATH that can be overridden - I added this to the documentation. Also, there is a new KrServices::kdesuPathName() method which can be used to access that path, no more messy logic. I didn't update the error message in krslots.cpp yet - it isn't really correct already, it's much easier to fix it in D4734.

martinkostolny accepted this revision.Feb 27 2017, 9:43 AM

I agree with Wladimir and Alex on this. I'm also fond of the third option: making root-mode unnecessary by providing support for elevating privileges for specific operations operations.

This patch is working on my Arch. Thanks!

palant updated this revision to Diff 11890.Feb 27 2017, 12:26 PM

One more small update - rather than have a special method for kdesu in KrServices, I added a general-purpose KrServices::isExecutable() method. The actual path is available everywhere anyway.

abika accepted this revision.Feb 27 2017, 3:41 PM
This revision is now accepted and ready to land.Feb 27 2017, 3:41 PM
abika added a comment.Feb 27 2017, 4:04 PM

Just found something: in krservices "kdesu" is still defined for tools and never used.

Forgot to reply:

In D4725#89086, @abika wrote:

TL;DR: I agree with Wladimir and would recommend to remove Krusader root mode altogether because we shouldn't support unsafe usage.

...

Martin Gräßlins insights are new to me, now that I know about it I have react. At least before publishing another release.

I would suggest not overreacting. From what it looks like, we are talking about the elephant in the room that everybody knew about all along. I remember talks about X11 not being safe from a long time ago, I merely wasn't really interested in details back then. And it's not like things can be significantly safer with X11, as Martin Gräßlin explains only Wayland has a sane security architecture - and in my understanding almost nobody uses it at this point.

So removing root mode before we have a safer replacement for it would be an overreaction. And implementing that replacement might take a while, especially if we want the solution to be implemented on the KIO level. Not publishing any releases in the meantime isn't going to help anybody. So I'm still suggesting to implement a big warning explaining that running Krusader in root mode isn't safe and should be avoided. This warning needs to suggest alternatives however and it is questionable what we can offer. Well-designed user actions might do as a stopgap solution.

palant updated this revision to Diff 11908.Feb 27 2017, 5:01 PM

Right, I forgot to remove a piece of unused code in KrServices, done that now.

My big apologies, Wladimir, you were right that kdesu could not be found on my Arch linux because of the path differences You and Alex talked about. I've tested this patch on useractions instead of invoking root-mode, that's why it previously worked for me, stupid of me...

If I prepend find_program(KDESU_PATH NAMES kdesu) to your added cmake code, it should work on both Ubuntu and Arch distributions:

# For security reasons, absolute kdesu path is set at build time and is not
# configurable.
if(NOT KDESU_PATH)
    find_program (KDESU_PATH NAMES kdesu)
endif()
if(NOT KDESU_PATH)
    set(KDESU_PATH "${KDE_INSTALL_FULL_LIBEXECDIR_KF5}/kdesu")
endif()
add_definitions( -DKDESU_PATH="${KDESU_PATH}" )

But I really don't know if this is the right approach so take this just as an informative comment :).

If I understand correctly, if the distributor has kdesu installed under /usr/local/bin then find_program() will pick up that path and hardcode it for everybody. Even if we assume that distributors generally have kde-cli-tools installed, this will make build results depend on the state of a particular machine. Doesn't really sound desirable, I'd rather stick with an explicit override for non-standard locations.

abika added a comment.EditedFeb 27 2017, 7:54 PM

I wouldn't say this is "overreacting".

  • The security flaw is real. An attacker can probably get root access by exploiting Krusaders root mode
  • Krusader is the only GUI application that comes with an "execute as root" feature that I know of (anything else?). And yes, there is a reason for it.
  • Again: it is still possible to execute Krusader as root. But we are not advertising it anymore. Those who really need this can still create the desktop entry/action.

I also don't get it: You pointed out the flaw with the "kdesu" config setting yourself and fixed it. At the same time, a security flaw with the very same impact is not that important?

In D4725#90606, @palant wrote:

So removing root mode before we have a safer replacement for it would be an overreaction. And implementing that replacement might take a while, especially if we want the solution to be implemented on the KIO level. Not publishing any releases in the meantime isn't going to help anybody. So I'm still suggesting to implement a big warning explaining that running Krusader in root mode isn't safe and should be avoided. This warning needs to suggest alternatives however and it is questionable what we can offer. Well-designed user actions might do as a stopgap solution.

I value security over features. If we publish something that has less features but is more secure this is actually a good thing.
A warning message is "good enough" for the action. I'm ok with it. But the desktop entry for the root mode must still be removed.

And either way: no, this won't affect any release schedule. I was never implying that.

(BTW: its already time for a new release imho).

In D4725#90687, @abika wrote:
  • Krusader is the only GUI application that comes with an "execute as root" feature that I know of (anything else?).

Actually, there are some not so far away: KDE Partition Manager, K3b and KFloppy.

A discussion about KFloppy

I would suggest to coordinate with the other developers working on this (KAuth/KIO integration, support for elevated privileges for PartionManager (where they tried KAuth but had to revert), KFloppy, Kate, etc). Better share the resources.

Also, as a general rule, try to keep such reviews and discussions public; Martin Graesslin already showed that any X11 applications with root is exploitable, so...

palant added a comment.EditedFeb 28 2017, 9:19 AM
In D4725#90687, @abika wrote:

I also don't get it: You pointed out the flaw with the "kdesu" config setting yourself and fixed it. At the same time, a security flaw with the very same impact is not that important?

You have to consider that the root issue is well beyond our reach. The way I understand Martin Gräßlin, an application running with user's privileges under X11 can record all keyboard input within that X11 session. So it doesn't really matter whether you have a graphical password prompt or a sudo prompt in Konsole - it will be able to record your password. This is a basic design flaw in X11 and we need Wayland in order to address it. At the moment however, a user running Wayland should be a rare exception. So if they need to copy a file to /etc then it doesn't matter whether they run Krusader in root mode or fire up Konsole and do sudo cp there, it isn't safe either way. In my understanding, the only safe way is pressing Ctrl-Alt-F1 and do whatever they need outside X11. But we cannot really expect them to switch to text mode every time (mind you, they shouldn't forget to log out there).

So at this point we are merely making it a bit harder for attackers to exploit the issue. This is a no-brainer for kdesu because we can easily improve security here and the users won't even notice (in fact, for Ubuntu users it is an improvement). On the other hand, even if we remove root mode feature from Krusader users will still need a way to run privileged operations. Should we suggest that they do sudo krusader or a moral equivalent of it? This is counterproductive, it doesn't make them any safer but it teaches them bad practices.

And you have to consider that the issue has been known for ages. Why the sudden urgency?

Also, as a general rule, try to keep such reviews and discussions public; Martin Graesslin already showed that any X11 applications with root is exploitable, so...

Yep, I merely wasn't aware of it when I created this review.

This revision was automatically updated to reflect the committed changes.

I value security over features.

Me too, so I tend to agree with removal of root-mode, but Wladimir already improved current state of Krusader security. So I say lets keep it for a little while. In the meantime I will work on an integration with KAuth for some actions. I just managed to integrate it in KTextEditor ( https://phabricator.kde.org/D4847 ), so one can now edit write-protected document in our internal editor being conveniently asked for password on save.

I would like to integrate KAuth e.g. for directory listing. But there is probably a lot of use-cases where it can be handy. I will appreciate suggestions :).

abika added a comment.Mar 9 2017, 7:12 PM

Actually, there are some not so far away: KDE Partition Manager, K3b and KFloppy.

K3b (last release 2014) and KFloppy do not run the UI as root.
But ok, partition manager does it (probably because all operations required root access anyway). I still think this is (or was initially) a bad idea.

In D4725#90782, @palant wrote:

You have to consider that the root issue is well beyond our reach. The way I understand Martin Gräßlin, an application running with user's privileges under X11 can record all keyboard input within that X11 session. So it doesn't really matter whether you have a graphical password prompt or a sudo prompt in Konsole - it will be able to record your password. This is a basic design flaw in X11 and we need Wayland in order to address it. At the moment however, a user running Wayland should be a rare exception. So if they need to copy a file to /etc then it doesn't matter whether they run Krusader in root mode or fire up Konsole and do sudo cp there, it isn't safe either way. In my understanding, the only safe way is pressing Ctrl-Alt-F1 and do whatever they need outside X11. But we cannot really expect them to switch to text mode every time (mind you, they shouldn't forget to log out there).

I agree that we are not responsible for security issues in X11. But following this argument we can skip any consideration of security: "An attacker with user rights can become root - so we can freely skip password prompts and doing sudo su". The opposite is the correct way: making it as safe as possible.

Should we suggest that they do sudo krusader or a moral equivalent of it? This is counterproductive, it doesn't make them any safer but it teaches them bad practices.

And that's exactly the wrong thing we are doing right now; saying "Run this in root, it is safe." And it is not safe and we know it. But probably not the user. Sorry, I can't agree with this at all.

And you have to consider that the issue has been known for ages. Why the sudden urgency?

Sorry, this is also a nonsense argument: doing stupid things for ages is not a reason for continue doing it.


I still suggest this:

  • remove the root desktop icon
  • add a warning (with "don't show again" option) when invoking the root action

Any objections?

I agree, no objections.