Bug #10890
Use less risky code for /tmp handling in I2P initscript and systemd unit file
0%
Description
On Wheezy, 0.9.23-2~deb7u+1, the initscript does:
NAME="i2p"
[...]
I2PTEMP="/tmp/${NAME}-daemon"
[...]
do_start()
{
[...]
[ -d $I2PTEMP ] || mkdir $I2PTEMP > /dev/null 2>&1
[...]
chown -Rf $I2PUSER:$I2PUSER $I2PTEMP $RUN > /dev/null 2>&1
… and then $I2PTEMP
is passed to /usr/sbin/wrapper
as wrapper.java.additional.12=-Di2p.dir.temp=$I2PTEMP
.
On Jessie, 0.9.23-2~deb8u+1, the systemd unit file does:
User=i2psvc
PermissionsStartOnly=true
[...]
ExecStartPre=/bin/mkdir -p /tmp/i2p-daemon
ExecStartPre=/bin/chown -R ${I2PUSER}:${I2PUSER} /var/log/i2p /run/i2p /tmp/i2p-daemon
… and then essentially the same thing happens as with sysvinit. Note with PermissionsStartOnly=true
, ExecStartPre=
commands are run as root and User=
only applies to the ExecStart=
command.
At least one issues is that the chown
can be used by an attacker, who can run code locally and win the race with a symlink, to make systemd / the initscript recursively chown
any directory it wants to i2psvc:i2psvc
. I suspect the consequences may be severe, given the i2psvc
user has unrestricted access to the Internet. E.g. any setuid binary in that directory gets the same unrestricted access. For the initscript, this is a typical TOCTU situation. For the systemd unitfile case, the race is easier to win: there’s no check done immediately before creating the directory, so creating the symlink at any time before the mkdir
is run is enough.
After booting Tails 1.8.1 with I2P enabled, I see a bunch of files created as i2psvc
in that directory. There is a non-constant component in these files’ name, so maybe this is not exploitable. Still, one single unsafe usage of /tmp/i2p-daemon
would make the whole thing dangerous.
In practice, at first glance we should be fine thanks to fs.protected_symlinks
, but if we’re going to keep this code, then I’d like someone who’s better than me at this kind of things to look at it… unless the root cause of the potential problem is fixed, which would be my preferred solution: it’s probably not worth spending much time auditing if this risky stuff is entirely safe, while we can quite simply make it obviously safe.
Subtasks
History
#1 Updated by intrigeri 2016-01-10 20:08:29
- Assignee set to kytv
kytv, can you please take care of leading this? Ideally, just fix the root cause of these potential problems. Worst case, audit it, make sure it’s safe, and have e.g. jvoisin ACK your analysis :)
#2 Updated by intrigeri 2016-01-10 22:46:59
- Private changed from Yes to No
jvoisin had a look and says it seems safe.
#3 Updated by intrigeri 2016-01-10 22:55:50
- Subject changed from Potential privilege escalation to Tor-exempt user via I2P initscript and systemd unit file to Use less risky code for /tmp handling in I2P initscript and systemd unit file
- Type of work changed from Security Audit to Code
Implementation wise, I see two options:
- Stop setting
I2P_ARGS
viaEnvironment=
, and haveExecStart
use “sh -c” to create the temporary directory withmktemp
, get its output, and pass it to/usr/sbin/wrapper
along with the other needed arguments. Then the chown isn’t needed for/tmp/i2p-daemon
since it’ll have been created as the right user from the beginning. - Use
tmpfiles.d(5)
so that the temporary directory is created before any non-root things potentially controlled by an attacker are running, which eliminates the race. This is a bit more declarative, but a bit less self-contained.
… but there are certainly more good ways to do it :)
#4 Updated by intrigeri 2016-01-10 23:03:44
… and jvoisin reminds me of PrivateTmp=
, which seems to be the best solution.
(And if you look at https://git-tails.immerda.ch/tails/tree/config/chroot_local-includes/lib/systemd/system?h=devel you’ll see a few more hardening options you could use, but that’s off-topic.)
#5 Updated by anonym 2016-03-29 18:10:14
- Target version changed from Tails_2.2 to Tails_2.3
#6 Updated by anonym 2016-05-08 05:10:30
- Target version changed from Tails_2.3 to Tails_2.4
#7 Updated by anonym 2016-06-08 01:35:02
- Target version changed from Tails_2.4 to Tails_2.5
#8 Updated by BitingBird 2016-07-01 11:31:23
- Assignee deleted (
kytv) - Target version deleted (
Tails_2.5)
no news from kytv -> removing assignee and target version
#9 Updated by anonym 2016-08-16 08:46:32
- Priority changed from Normal to Elevated
#10 Updated by anonym 2016-08-16 13:46:11
Just verified that this still is the case in 0.9.26-1~deb8u+1.