DevHeads.net

Review Request 108711: kcmdatetimehelper: Hardcode PATH because $PATH is empty here.

Review request for kde-workspace, Christoph Feck and Oswald Buddenhagen.

Description

Diffs
kcontrol/dateandtime/helper.cpp 5a946d8

Diff: <a href="http://git.reviewboard.kde.org/r/108711/diff/" title="http://git.reviewboard.kde.org/r/108711/diff/">http://git.reviewboard.kde.org/r/108711/diff/</a>

Testing

Thanks,

Kevin Kofler

Comments

Re: Review Request 108711: kcmdatetimehelper: Hardcode PATH beca

By Kevin Kofler at 02/02/2013 - 04:27

(Updated Feb. 2, 2013, 8:27 a.m.)

Review request for kde-workspace, Christoph Feck and Oswald Buddenhagen.

Description (updated)
Unfortunately, we cannot rely on the $PATH environment variable in KAuth helpers, because D-Bus activation clears it. So we have to use a reasonable default for the KStandardDirs::findExe search path, and actually use the return value of KStandardDirs::findExe in the calls to KProcess::execute.

This fixes things so hwclock and zic actually get found. See: <a href="https://bugzilla.redhat.com/show_bug.cgi?id=906854" title="https://bugzilla.redhat.com/show_bug.cgi?id=906854">https://bugzilla.redhat.com/show_bug.cgi?id=906854</a> . This got noticed in Fedora 18 because it does not always create /etc/localtime, so the fallback code operating on /etc/localtime triggered an error.

Diffs
kcontrol/dateandtime/helper.cpp 5a946d8

Diff: <a href="http://git.reviewboard.kde.org/r/108711/diff/" title="http://git.reviewboard.kde.org/r/108711/diff/">http://git.reviewboard.kde.org/r/108711/diff/</a>

Testing (updated)
Builds against at least 4.10.0 and 4.9.5.

Works at runtime on Fedora 18, see: <a href="https://bugzilla.redhat.com/show_bug.cgi?id=906854#c12" title="https://bugzilla.redhat.com/show_bug.cgi?id=906854#c12">https://bugzilla.redhat.com/show_bug.cgi?id=906854#c12</a> (The reporter encountered another issue, apparently because ktimezoned also misbehaves when /etc/localtime is absent, but at least this particular issue is confirmed fixed.)

Thanks,

Kevin Kofler

Re: Review Request 108711: kcmdatetimehelper: Hardcode PATH beca

By Commit Hook at 02/06/2013 - 18:26

This review has been submitted with commit c517bbc175a3fbb09f13a8468a6987fdbc547d23 by Kevin Kofler to branch KDE/4.10.

- Commit Hook

On Feb. 2, 2013, 8:27 a.m., Kevin Kofler wrote:

Re: Review Request 108711: kcmdatetimehelper: Hardcode PATH beca

By Kevin Kofler at 02/06/2013 - 18:32

… and merged to master with merge commit e99a0e5b1223b7a673b0bc29c280d962c6e8dfb6.

- Kevin

On Feb. 2, 2013, 8:27 a.m., Kevin Kofler wrote:

Re: Review Request 108711: kcmdatetimehelper: Hardcode PATH beca

By Konstantinos Smanis at 02/04/2013 - 11:53

We can do better than hardcoding a reasonable default. We can launch a login shell (1) and 'echo $PATH' the user's PATH. This has many advantages:

1. We don't miss paths (e.g. /usr/local/bin, /usr/local/sbin etc.).
2. We honor the precedence of paths as set in $PATH by the user.
3. We only use the user's PATH (DBus activation works for non-root users too).

I am currently working on this approach for kcm-grub2 which also misbehaves when $PATH is not set. If you are interested, you may restrain from committing until I post a link to the commit.

(1) A login shell is needed to properly source /etc/profile, ~/.profile and/or other shell-specific login scripts (such as ~/.bash_profile for Bash).

- Konstantinos Smanis

On Feb. 2, 2013, 8:27 a.m., Kevin Kofler wrote:

Re: Review Request 108711: kcmdatetimehelper: Hardcode PATH beca

By =?utf-8?Q?Thoma... at 02/08/2013 - 18:37

Privilegue raising does rarely happen through the one major hole in the wall (except on windows - one could *always* rely on outlook express ;-) but a cascade of "minor" weaknesses.

There's a reason why scripts cannot be suid'ed - you can cause far to much shit by that.
There's also a reason why scripts usually runnig on root privs should *always* operate on absolute paths - despite nobody should be able to add a malicious path there ... except the stupid (home/hobby/underpaid) admin.

In the end, what happens if you add PATH evaluation into a suid process, is that you place a door into a wall.
The door is locked, so no problem - so far.
The problem occurs if someone looses the key, what would not have been a problem if there was no door in the first place.

Ultimately, the ideal solution here was imo a compile time resolution, ie. a CFLAG, so that your distro (or yourself) choose the path of zic and hwclock on compilation and compile that into the binary.
The still less dangerous solution was to have the resolvable path as compile time resolution (same as above, but adding what Kevin did in his patch)

The only problematic aspect would be if
- you compile yourself
- you don't have zic/hwclock installed, or the installation path may change and you forget about that
- you don't know (how) to add the compile time paths (or PATH)

But the good thing is that
- joe admin is not involved
- distros and experienced users can still align to their specifics w/o having to patch the code.

- Thomas

On Feb. 2, 2013, 8:27 a.m., Kevin Kofler wrote:

Re: Review Request 108711: kcmdatetimehelper: Hardcode PATH beca

By Konstantinos Smanis at 02/04/2013 - 17:01

@Thomas: Afaik modifying the root's $PATH requires root privileges. It is defined in /etc/profile and /root/.profile usually. Not messing with root's $PATH is definitely a precaution but I can't see how one could take advantage of it.

@Kevin:
* Afaik not all helpers run as root, you can drop privileges in the .service/.conf files to the desired user (i.e. apache).
* I already commented on D-Bus precautions.
* Sure, this is just a suggestion, you are free to pass on it. Personally I find hardcoding stuff less elegant than overengineered solutions.
* Going off-topic, kcm-grub2 is definitely a typical application that needs a KAuth helper. I agree that the implementation can be vastly improved but I can't see how this is related to the proposal above.
* Lastly, completely unrelated as it may be, I can't but agree that the workaround used before my commit simply sucked.

Feel free to enlighten me if you find a valid compromise scenario.

- Konstantinos

On Feb. 2, 2013, 8:27 a.m., Kevin Kofler wrote:

Re: Review Request 108711: kcmdatetimehelper: Hardcode PATH beca

By Kevin Kofler at 02/04/2013 - 15:51

Oh, and looking at your commit, what you did before was even worse, of course. Passing PATH from the main application to the helper is totally unacceptable for the same reason as passing an arbitrary file is, see my previous comment and Thomas Lübking's comment.

- Kevin

On Feb. 2, 2013, 8:27 a.m., Kevin Kofler wrote:

Re: Review Request 108711: kcmdatetimehelper: Hardcode PATH beca

By Kevin Kofler at 02/04/2013 - 15:39

* This code (and any KAuth helper, actually) always runs as root.
* We're looking for 2 core system executables, the chances they are in /usr/local are near zero, and I'd also share Thomas Lübking's security concerns. (D-Bus clears the path for a reason.)
* Spawning a login shell looks like a really overengineered and ugly solution, considering the above.
* kcm-grub2 is the last piece of software I'd model a KAuth helper on, given that your KAuth actions are implemented in a totally insecure way. (Last I checked, the process running as the user passes an arbitrary file to the helper running as root, defeating the whole purpose of finegrained PolicyKit permissions. Give a user that PolicyKit permission and you essentially gave him root. Of course, it's not a problem if you stick to the default auth_admin policy, but if some local admin tries to change it, ouch!)

- Kevin

On Feb. 2, 2013, 8:27 a.m., Kevin Kofler wrote:

Re: Review Request 108711: kcmdatetimehelper: Hardcode PATH beca

By =?utf-8?Q?Thoma... at 02/04/2013 - 14:59

Question (for the particular case and in general):
This is about a suid root:dbus call (thus env clearing for dbus system activation) correct?
Ie. the called application is executed with root privs?

In this case there should afaics rather not be _any_ PATH resolution at all and checking the usual suspects is about the last acceptable process.

Otherwise one could place a random binary "zic" or "hwclock" into the $PATH and gain a root shell (or are there further protections against this?)

- Thomas

On Feb. 2, 2013, 8:27 a.m., Kevin Kofler wrote:

Re: Review Request 108711: kcmdatetimehelper: Hardcode PATH beca

By Konstantinos Smanis at 02/04/2013 - 14:45

Here you are: <a href="http://commits.kde.org/kcm-grub2/7c5beb979fdf9dd14abfffb0e24d4f69b11ca985" title="http://commits.kde.org/kcm-grub2/7c5beb979fdf9dd14abfffb0e24d4f69b11ca985">http://commits.kde.org/kcm-grub2/7c5beb979fdf9dd14abfffb0e24d4f69b11ca985</a>

- Konstantinos

On Feb. 2, 2013, 8:27 a.m., Kevin Kofler wrote:

Re: Review Request 108711: kcmdatetimehelper: Hardcode PATH beca

By Oswald Buddenhagen at 02/02/2013 - 10:10

Ship it!

Ship It!

- Oswald Buddenhagen

On Feb. 2, 2013, 8:27 a.m., Kevin Kofler wrote: