Bug #10890

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

Added by intrigeri 2016-01-10 20:02:40 . Updated 2016-08-16 13:46:11 .

Status:
Confirmed
Priority:
Elevated
Assignee:
Category:
Target version:
Start date:
2016-01-10
Due date:
% Done:

0%

Feature Branch:
Type of work:
Code
Blueprint:

Starter:
Affected tool:
I2P
Deliverable for:

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 via Environment=, and have ExecStart use “sh -c” to create the temporary directory with mktemp, 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.