Use less risky code for /tmp handling in I2P initscript and systemd unit file

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.

Original created by @intrigeri on 10890 (Redmine)