New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CVE-2017-16933: root privilege escalation via prepare-dirs (init script and systemd service file) #5793

Closed
orlitzky opened this Issue Nov 23, 2017 · 12 comments

Comments

Projects
None yet
7 participants
@orlitzky

orlitzky commented Nov 23, 2017

The etc/initsystem/prepare-dirs file calls chown unsafely, leading to a root exploit for the $ICINGA2_USER. The tl;dr is that it's never safe to call chown as root, unless the target and all directories above it are controlled wholly by root.

A few uses of chown in prepare-dirs are exploitable; here's an example:

chown $ICINGA2_USER:$ICINGA2_COMMAND_GROUP $(dirname -- $ICINGA2_PID_FILE)
if [ -f $ICINGA2_PID_FILE ]; then
	chown $ICINGA2_USER:$ICINGA2_GROUP $ICINGA2_PID_FILE
fi

The first line gives away ownership of the directory containing the $ICINGA2_PID_FILE, and the next line calls chown on that file. The exploit is that, after the first line executes, the $ICINGA2_USER can simply replace $ICINGA2_PID_FILE with a link (sym or hard) to a root-owned file. The call to chown will then change ownership of the link's target. That is easily exploitable to gain root, by taking ownership of e.g. /etc/passwd or root's .bashrc file.

To exploit this the first time the service is started, you need to take advantage of the race condition to create a link before the -f test is executed. However, there's a much easier scenario: if the service is started, stopped, and started again (even across reboots, for persistent directories), then the -f test will succeed, and call chown on a path that has been controlled by $ICINGA2_USER since the first time the service was started.

As for the fix, I would entirely eliminate the ability to change the value of $ICINGA2_USER and $ICINGA2_GROUP at runtime, since there is no safe way for you to fix the permissions after it is changed. Once those variables are eliminated, you don't need to call chown on the directories or files: the directories can be created with the correct ownership by make install, and the files will be owned by the user who creates them (namely the icinga runtime user, who will be the one writing to them, and that user will never change).

If you would rather keep those variables, just let the init/service script crash if the permissions are wrong. Users will have to be responsible for fixing the existing permissions if they make a major change like switching an in-use UID/GID on a live system. (This is not mere sadism on my part; there really is no safe way to automate the necessary changes.)

This affects both the init script and systemd service file:

@CMAKE_INSTALL_PREFIX@/lib/icinga2/prepare-dirs $SYSCONFIGFILE

ExecStartPre=@CMAKE_INSTALL_PREFIX@/lib/icinga2/prepare-dirs @ICINGA2_SYSCONFIGFILE@

@orlitzky orlitzky changed the title from Root privilege escalation via prepare-dirs (init script and systemd service file) to CVE-2017-16933: root privilege escalation via prepare-dirs (init script and systemd service file) Nov 24, 2017

@dgoetz

This comment has been minimized.

Show comment
Hide comment
@dgoetz

dgoetz Nov 27, 2017

Member

For systemd we should consider fixing this with RuntimeDirectory and the other systemd settings, but this won't fix initd.

Member

dgoetz commented Nov 27, 2017

For systemd we should consider fixing this with RuntimeDirectory and the other systemd settings, but this won't fix initd.

@orlitzky

This comment has been minimized.

Show comment
Hide comment
@orlitzky

orlitzky Nov 27, 2017

There's also the tmpfiles.d option with systemd, but the same caveat applies. OpenRC now supports tmpfiles, but classic SysV-style init never will.

In my first comment, I mentioned that letting the end user change $ICINGA2_USER and $ICINGA2_GROUP after installation is problematic, but it occurs to me now that the same thing applies to the $ICINGA2_STATE_DIR and the other directory/file variables. If the user changes them after icinga is installed, the permissions are probably going to be wrong on the new directories/files. That leaves you with two options:

  1. Crash, which sucks.
  2. Try to make the directory/file writable by icinga2, which is dangerous.

A priori the second option is dangerous because you don't know what the user is going to set those variables to, and it isn't advertised that you're going to be calling chown on them and giving ownership of them to a restricted user. For example, a user might set the log directory to /var/log, not knowing that you plan on chowning that to icinga2.

There's also a more subtle problem... if the user points (say) the log file variable to an existing file, you have to be careful to set the permissions in a safe order. For example, it's not safe to chown the log file's parent directory, and then chown the log file itself, because in between the two calls there's a race condition: once he owns the directory, he can replace the file with a link. On the other hand, if the parent directory and file are owned by root, it should be safe to chown the file and then the directory afterwards.

So tl;dr I would remove any options that require you to fix ownership or permissions at runtime as root. Having to recompile to change things is annoying, but at least then the ownership/permissions can be set in the build directory (on the newly-built files) and not on the live filesystem.

orlitzky commented Nov 27, 2017

There's also the tmpfiles.d option with systemd, but the same caveat applies. OpenRC now supports tmpfiles, but classic SysV-style init never will.

In my first comment, I mentioned that letting the end user change $ICINGA2_USER and $ICINGA2_GROUP after installation is problematic, but it occurs to me now that the same thing applies to the $ICINGA2_STATE_DIR and the other directory/file variables. If the user changes them after icinga is installed, the permissions are probably going to be wrong on the new directories/files. That leaves you with two options:

  1. Crash, which sucks.
  2. Try to make the directory/file writable by icinga2, which is dangerous.

A priori the second option is dangerous because you don't know what the user is going to set those variables to, and it isn't advertised that you're going to be calling chown on them and giving ownership of them to a restricted user. For example, a user might set the log directory to /var/log, not knowing that you plan on chowning that to icinga2.

There's also a more subtle problem... if the user points (say) the log file variable to an existing file, you have to be careful to set the permissions in a safe order. For example, it's not safe to chown the log file's parent directory, and then chown the log file itself, because in between the two calls there's a race condition: once he owns the directory, he can replace the file with a link. On the other hand, if the parent directory and file are owned by root, it should be safe to chown the file and then the directory afterwards.

So tl;dr I would remove any options that require you to fix ownership or permissions at runtime as root. Having to recompile to change things is annoying, but at least then the ownership/permissions can be set in the build directory (on the newly-built files) and not on the live filesystem.

@dnsmichi

This comment has been minimized.

Show comment
Hide comment
@dnsmichi

dnsmichi Nov 28, 2017

Member

How do other applications solve this issue? I would believe this isn't exclusive to Icinga.

Member

dnsmichi commented Nov 28, 2017

How do other applications solve this issue? I would believe this isn't exclusive to Icinga.

@orlitzky

This comment has been minimized.

Show comment
Hide comment
@orlitzky

orlitzky Nov 28, 2017

Well, I've found about 20 or so that do the same thing and therefore have the same exploit...

I thought about this really hard a year ago when I found the first one, and the best that I could come up with was "don't do that" for runtime-switchable users and "state" directories. We've fixed a bunch of init scripts in Gentoo by eliminating those options, although we have an advantage in the distro: we can create a separate user and run ./configure with all the right options so that we get a sensible end result, mitigating the need for the end user to screw with things.

orlitzky commented Nov 28, 2017

Well, I've found about 20 or so that do the same thing and therefore have the same exploit...

I thought about this really hard a year ago when I found the first one, and the best that I could come up with was "don't do that" for runtime-switchable users and "state" directories. We've fixed a bunch of init scripts in Gentoo by eliminating those options, although we have an advantage in the distro: we can create a separate user and run ./configure with all the right options so that we get a sensible end result, mitigating the need for the end user to screw with things.

@Crunsher

This comment has been minimized.

Show comment
Hide comment
@Crunsher
Member

Crunsher commented Nov 29, 2017

@orlitzky

This comment has been minimized.

Show comment
Hide comment
@orlitzky

orlitzky Nov 29, 2017

The test -e ... || install is only guaranteed to be safe if it takes place in a root-owned directory, specifically if the -e test is performed on a path that the user can't replace.

For example, test -e /run/foo || install ... should be safe, because root owns /run. On the other hand, test -e /var/icinga2/foo || install ... might not be, because the "icinga2" user could create foo as a symlink between the -e test and the install command. (This race condition is not as trivial to exploit, but it's there...)

Just like chown, the install command will follow the symlink and helpfully "fix" the ownership on the target for you.

Anyway, what I'm getting at is that, since we don't know what the value of e.g. $ICINGA2_ERROR_LOG will be, we have no idea if its parent directory is one of the ones that it's safe to play with.

orlitzky commented Nov 29, 2017

The test -e ... || install is only guaranteed to be safe if it takes place in a root-owned directory, specifically if the -e test is performed on a path that the user can't replace.

For example, test -e /run/foo || install ... should be safe, because root owns /run. On the other hand, test -e /var/icinga2/foo || install ... might not be, because the "icinga2" user could create foo as a symlink between the -e test and the install command. (This race condition is not as trivial to exploit, but it's there...)

Just like chown, the install command will follow the symlink and helpfully "fix" the ownership on the target for you.

Anyway, what I'm getting at is that, since we don't know what the value of e.g. $ICINGA2_ERROR_LOG will be, we have no idea if its parent directory is one of the ones that it's safe to play with.

@dnsmichi

This comment has been minimized.

Show comment
Hide comment
@dnsmichi

dnsmichi Nov 29, 2017

Member

One thing to note if the application should "crash" if permissions are wrong: We need better error messages and a cold startup permission check to do so. The idea is to allow features register required directories and permissions, and check them as icinga user. If anything is wrong, an error will be logged to stderr or the log with EXIT_FAILURE.

Member

dnsmichi commented Nov 29, 2017

One thing to note if the application should "crash" if permissions are wrong: We need better error messages and a cold startup permission check to do so. The idea is to allow features register required directories and permissions, and check them as icinga user. If anything is wrong, an error will be logged to stderr or the log with EXIT_FAILURE.

@Crunsher

This comment has been minimized.

Show comment
Hide comment
@Crunsher

Crunsher Nov 29, 2017

Member

@orlitzky You I are right, I had hoped install would fail if the directory exists (which in hindsight makes the test void), alas it does not but overwrites

Addendum: This means we need to move these into the core, and create them after we dropped our permissions.

Member

Crunsher commented Nov 29, 2017

@orlitzky You I are right, I had hoped install would fail if the directory exists (which in hindsight makes the test void), alas it does not but overwrites

Addendum: This means we need to move these into the core, and create them after we dropped our permissions.

@prometheanfire

This comment has been minimized.

Show comment
Hide comment
@prometheanfire

prometheanfire Dec 3, 2017

Member

If you have a fix in mind for systemd I'd like to consume it.

Member

prometheanfire commented Dec 3, 2017

If you have a fix in mind for systemd I'd like to consume it.

Crunsher added a commit that referenced this issue Dec 13, 2017

Crunsher added a commit that referenced this issue Dec 14, 2017

Crunsher added a commit that referenced this issue Jan 26, 2018

@Crunsher

This comment has been minimized.

Show comment
Hide comment
@Crunsher

Crunsher Jan 29, 2018

Member

@lazyfrosch Broken link

Member

Crunsher commented Jan 29, 2018

@lazyfrosch Broken link

@lazyfrosch

This comment has been minimized.

Show comment
Hide comment
@lazyfrosch

lazyfrosch Jan 29, 2018

Member

Sorry was testing a Trello integration, please ignore...

Member

lazyfrosch commented Jan 29, 2018

Sorry was testing a Trello integration, please ignore...

Crunsher added a commit that referenced this issue Jan 30, 2018

Crunsher added a commit that referenced this issue Feb 20, 2018

Crunsher added a commit that referenced this issue Feb 20, 2018

Crunsher added a commit that referenced this issue Feb 20, 2018

@Crunsher

This comment has been minimized.

Show comment
Hide comment
@Crunsher

Crunsher Feb 21, 2018

Member

Closed #5850

Member

Crunsher commented Feb 21, 2018

Closed #5850

@Crunsher Crunsher closed this Feb 21, 2018

Crunsher added a commit that referenced this issue Feb 23, 2018

@Crunsher Crunsher added this to the 2.8.2 milestone Feb 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment