<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/108711/">http://git.reviewboard.kde.org/r/108711/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 4th, 2013, 3:53 p.m. UTC, <b>Konstantinos Smanis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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).</pre>
</blockquote>
<p>On February 4th, 2013, 6:45 p.m. UTC, <b>Konstantinos Smanis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Here you are: http://commits.kde.org/kcm-grub2/7c5beb979fdf9dd14abfffb0e24d4f69b11ca985</pre>
</blockquote>
<p>On February 4th, 2013, 6:59 p.m. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?)</pre>
</blockquote>
<p>On February 4th, 2013, 7:39 p.m. UTC, <b>Kevin Kofler</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">* 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!)</pre>
</blockquote>
<p>On February 4th, 2013, 7:51 p.m. UTC, <b>Kevin Kofler</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On February 4th, 2013, 9:01 p.m. UTC, <b>Konstantinos Smanis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">@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. </pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> 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.</pre>
<br />
<p>- Thomas</p>
<br />
<p>On February 2nd, 2013, 8:27 a.m. UTC, Kevin Kofler wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kde-workspace, Christoph Feck and Oswald Buddenhagen.</div>
<div>By Kevin Kofler.</div>
<p style="color: grey;"><i>Updated Feb. 2, 2013, 8:27 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kcontrol/dateandtime/helper.cpp <span style="color: grey">(5a946d8)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108711/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>