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

Mon Feb 4 21:01:32 GMT 2013

> 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.

@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

-----------------------------------------------------------
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/20130204/77a78d77/attachment.htm>