checkpath: race condition in link detection

[orlitzky]

(Thanks to @poettering for pointing this out)

The checkpath helper tries to avoid operating on links, because they can be used to trick OpenRC into calling either chown() or chmod() on a path of some non-root user's choosing. For example, in

checkpath --directory --owner foo /run/foo
checkpath --file --owner foo /run/foo/bar

the "foo" user can replace bar with a link in between restarts of the service. If checkpath follows that link, then "foo" can take ownership of arbitrary files on the system.

The code that tries to avoid this can be found in checkpath.c,

if (lstat(path, &st) || trunc) {
...
if ((type != inode_dir) && (st.st_nlink > 1)) {
    eerror("%s: chown: %s %s", applet, "Too many hard links to", path);
    return -1;
}
if (S_ISLNK(st.st_mode)) {
    eerror("%s: chown: %s %s", applet, path, " is a symbolic link");
    return -1;
}

einfo("%s: correcting owner", path);
if (chown(path, uid, gid)) {
    eerror("%s: chown: %s", applet, strerror(errno));
    return -1;
}

There is a TOCTOU vulnerability there that allows a swift attacker to switch out path with a link. You can verify this by putting a sleep(10); after the stat but before the st_nlink and S_ISLNK tests, and then clobbering normal files with links in another terminal while checkpath runs.

There are really three separate problems here, with differing levels of I-know-how-to-fix-them, so I'll list all three separately.

Following symlinks

According to the existing code, symlinks should never be followed. The only problem in this regard is the race condition that allows an attacker to replace a regular file with a symlink between the stat and e.g chown. I think this can be solved by using a different library function... one that doesn't follow symlinks, like lchown or fchownat with AT_SYMLINK_NOFOLLOW.

Following hard links

I don't know of any good way to prevent this. You can tighten the window a bit, or enforce certain restrictions (like fs.protected_hardlinks) when available, but I see no easy solution. Perhaps the best way to avoid the hard link exploit is not to ignore them, per se, but to ensure that no problematic hard links can be created in the first place. That brings us to...

Operating in a directory that is not owned by root

When root does anything in a directory that he doesn't own, you're asking for trouble. Recall that this can be exploited,

checkpath --directory --owner foo /run/foo
checkpath --file --owner foo /run/foo/bar

and notice that this cannot (at least the first time that the service is started):

checkpath --file --owner foo /run/foo/bar
checkpath --directory --owner foo /run/foo

The "foo" user can't replace /run/foo/bar because he does not yet own /run/foo. Unfortunately, if the service is re-started, the same exploit presents itself.

I would like to say that the solution to this problem is to refuse to do anything in a directory that is not owned by root. That would force people to reorder their checkpath calls safely, but it would also invalidate a lot of existing service scripts that "fix" pre-existing permissions; for example, postgresql:

for file in postgresql pg_hba pg_ident ; do
    if [ -f "/etc/postgresql/${file}" ] ; then
        checkpath -f -m 0600 -o postgres:postgres ${file}
        ...
    fi
done

That all takes place in /etc/postgresql which is owned by postgres:postgres. In my opinion, those files should be created once with the correct ownership, and if root changes that, then so be it. But what do you do there if /etc/postgresql/postgresql.conf has the wrong owner? Emit a warning, saying that you're refusing to touch it because it's in a directory not owned by root? I can't think of anything better.

I'm not claiming that this is feasible, but it would fix both the hard link and symlink issues at once, and maybe nudge people not to do some weird stuff in their service scripts.

[williamh]

@orlitzky I think I have been able to tighten down the window and make sure we don't follow symlinks as much as possible. I'm still not sure what to do about the har dlink issue. It looks like we can disable it at the kernel level for Linux, but that won't help us on the *bsd side. Take a look at my changes and let me know what you think about the symlink issue, then add comments about the hardlink issue.

[orlitzky]

Symlinks are still being followed here, just not as the last component of the path. For example, with this init script,

#!/sbin/openrc-run

start_pre() {
  checkpath --owner mjo --directory /run/foo
  checkpath --owner mjo --directory /run/foo/bar
  checkpath --owner mjo --file /run/foo/bar/passwd
}

start() {
    :
}

stop() {
    :
}

I can replace /run/foo/bar with a symlink to /etc...

$ cd /run/foo
$ rm -r bar && ln -s /etc ./bar

and then when the service is restarted...

$ sudo /etc/init.d/openrc-symlink-exploit restart
 * /run/foo/bar: creating directory
 * checkpath: unable to open directory: Too many levels of symbolic links
 * /run/foo/bar/passwd: correcting owner

I've gained ownership of /etc/passwd:

$ ls -l /etc/passwd
-rw-r--r-- 1 mjo mjo 1.5K 2018-01-16 10:01 /etc/passwd

[williamh]

@mjo You are also not checking the return code of checkpath. Try checking the return code each time and aborting if checkpath fails.

[williamh]

@mjo Another thing to consider is that following symlinks like that can be legit and we have no way to know when it isn't. For example, if someone wants to write a script that will work both on *bsd and Linux, they will probably set the pidfile to /var/run/foo.pid.

[orlitzky]

You are also not checking the return code of checkpath. Try checking the return code each time and aborting if checkpath fails.

I agree with you 100% there, but as a practical matter, literally no one checks the return value from checkpath because it's supposed to "just fix it." Running git grep checkpath in ::gentoo turns up a ton of these:

$ cat app-admin/graylog2/files/initd-r1p-admin/graylog2/files/initd
#!/sbin/openrc-run
...

start() {
  ...

  checkpath -d -o "${GRAYLOG_USER}:${GRAYLOG_GROUP}" -m750 "${GRAYLOG_DATA_DIR}"
  checkpath -d -o "${GRAYLOG_USER}:${GRAYLOG_GROUP}" -m750 "${GRAYLOG_DATA_DIR}/data"
  checkpath -d -o "${GRAYLOG_USER}:${GRAYLOG_GROUP}" -m750 "${GRAYLOG_DATA_DIR}/data/contentpacks"
  checkpath -d -o "${GRAYLOG_USER}:${GRAYLOG_GROUP}" -m750 "${GRAYLOG_DATA_DIR}/data/journal"

With the current implementation, the whole init script should probably fail if checkpath does, because something's really wrong.

Another thing to consider is that following symlinks like that can be legit and we have no way to know when it isn't. For example, if someone wants to write a script that will work both on *bsd and Linux, they will probably set the pidfile to /var/run/foo.pid.

We know it's legit if it's in a directory writable only by root. OTOH https://github.com/OpenRC/openrc/blob/master/service-script-guide.md tells you to set these paths in the build system.

[orlitzky]

Oh, and even if I did check the return value of checkpath, there would still be an exploitable race condition: I could replace the dir with a symlink after checkpath returned 0.

[orlitzky]

I opened a separate issue for the symlinks higher up in the path.

As far as the hardlink detection goes, the fix you've already committed is probably the best we can do.

[williamh]

I think this issue is about tracking the hard link fix, so I am going to close it. If you disagree, comment and I'll re-open it. I will move the discussion of following symlinks to the new issue you opened, and I will also be opening an issue against the service script guide for the /run and /var/run example.

[orlitzky]

I think this issue is about tracking the hard link fix, so I am going to close it.

It also included the symlink race condition, but yeah, it's as good as it's going to get. With fs.protected_hardlinks=0, both OpenRC's checkpath and systemd's tmpfiles now have the same tiny window between open() and fstat(), and no one has a clue how to make it go away.