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

[Thomas Lübking]

> On Feb. 4, 2013, 3:53 p.m., Konstantinos Smanis wrote:
> > 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 wrote:
>     Here you are: http://commits.kde.org/kcm-grub2/7c5beb979fdf9dd14abfffb0e24d4f69b11ca985
> 
> Thomas Lübking wrote:
>     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?)
> 
> Kevin Kofler wrote:
>     * 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 Kofler wrote:
>     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.
> 
> Konstantinos Smanis wrote:
>     @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.

> Afaik modifying the root's $PATH requires root privileges.
Yes. However the admin which has set path to /bin:/sbin/:usr/bin is not your problem anyway - you've to worry about the weak setups, where the admin abused /etc/profile as /etc/skel (don't say that does not happen, i have actually seen "." in PATH in /etc/profile)

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


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108711/#review26617
-----------------------------------------------------------


On Feb. 2, 2013, 8:27 a.m., Kevin Kofler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108711/
> -----------------------------------------------------------
> 
> (Updated Feb. 2, 2013, 8:27 a.m.)
> 
> 
> Review request for kde-workspace, Christoph Feck and Oswald Buddenhagen.
> 
> 
> Description
> -------
> 
> 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: https://bugzilla.redhat.com/show_bug.cgi?id=906854 . 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: http://git.reviewboard.kde.org/r/108711/diff/
> 
> 
> Testing
> -------
> 
> Builds against at least 4.10.0 and 4.9.5.
> 
> Works at runtime on Fedora 18, see: https://bugzilla.redhat.com/show_bug.cgi?id=906854#c12 (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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130208/29be04e7/attachment.htm>