Skip to content
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-2018-21269: checkpath root privilege escalation following non-terminal symlinks #201

Closed
orlitzky opened this issue Jan 24, 2018 · 60 comments

Comments

@orlitzky
Copy link
Contributor

Let's use a separate issue for this so we don't conflate it with the race condition fix in #195. With the following service 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 (at my leisure) with a symlink to /etc...

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

and the next time the service is restarted, checkpath will change ownership of /etc/passwd:

$ 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
$ ls -l /etc/passwd
-rw-r--r-- 1 mjo mjo 1.5K 2018-01-16 10:01 /etc/passwd

Even if the service script checks the return value of checkpath, I have a moment to replace "bar" with a symlink. The systemd tmpfiles implementation has a similar issue systemd/systemd#7986

@williamh
Copy link
Contributor

I don't see a way to avoid this easily, because /var/run in your example can be a symlinkk, and if it is it is completely legit.

@chutz
Copy link
Contributor

chutz commented Jan 24, 2018

There is no real way around this, as @williamh pointed out there are completely legitimate reasons to have symlinks in the path.

The only way to prevent this would be to open each path component separately with openat(..., O_RDONLY | O_PATH | O_NOFOLLOW) starting with open("/", O_RDONLY | O_PATH);, but you still have to make a way to bypass it for things like /var/run

@orlitzky
Copy link
Contributor Author

Well systemd was able to do it by saying "your service won't work unless you use /run", so there is always that option =)

Considering that /var/run and /run are the only two non-temporary paths that checkpath should encounter... are there any other symlinks we should be worried about? If not, a "THIS WILL NOT WORK IN THE NEXT RELEASE" warning should get people finally motivated to remove the "var/" from "/var/run".

@chutz
Copy link
Contributor

chutz commented Jan 24, 2018

Exploiting this is not easy. Generally init scripts are poking paths that are owned by root and not writable by users (or just the daemon user).

Also, since checkpath is usually used in init scripts (many/most of which run before sshd is running, all of which generally run before console logins are enabled) exploiting this would be rather tricky. If someone has a particular daemon that might be victim to this, they can practically eliminate the exploit vector with before sshd in depend().

@chutz
Copy link
Contributor

chutz commented Jan 24, 2018

We are not systemd, telling people how their systems should be configured is not really an option. Also /run isn't a thing on some BSDs, which makes any such solution non-portable (once again we don't have systemd's luxury of saying "this won't work without Linux 4.14 and glibc 2.23").

@orlitzky
Copy link
Contributor Author

A non-symlink /var/run shouldn't be a problem. The warning would only be shown when /var/run is a symlink pointing to /run.

(I much prefer OpenRC's portable approach to systemd's)

@chutz
Copy link
Contributor

chutz commented Jan 24, 2018

The problem is that you then can't write init scripts that work on both BSD and Linux without dirty "if linux" hacks. Currently, you can just write to /var/run and it will do the right thing on both Linux and BSD.

@orlitzky
Copy link
Contributor Author

Of course you can, you can do what the service script guide tells you to do, and use the runstatedir from the build system =)

@orlitzky
Copy link
Contributor Author

@chutz whoops, I made that comment in another issue, not this one: #202

@williamh
Copy link
Contributor

I do not have any plans for removing /var/run in Gentoo, and we don't know what other Linux distros are doing, so I'm not convinced checkpath should blow up in this situation.

@orlitzky
Copy link
Contributor Author

orlitzky commented Jan 24, 2018

I'm not suggesting anything that would break compatibility on any system. If checkpath encounters a symlink, it would tell you (i.e. the packager) to use the real path instead. That can be done in the build system if upstream accepts it, or in the ebuild/rpm/deb/whatever.

It would make a bunch of people update their 15 year old init scripts finally, but nothing would break. Same as the runscript -> openrc-run transition.

@vapier
Copy link
Member

vapier commented Jan 24, 2018

it should really behave the same as tmpfiles. if tmpfiles is going the route of disallowing it, then so should checkpath. most users of checkpath should be converted to tmpfiles too which further drives home this point.

if you really want to leave an escape hatch, you could add a command line option like chown's -L/-P options where -P is the default. then everyone can eat their cookies w/out making the default behavior insecure.

@williamh
Copy link
Contributor

I haven't tested yet, but It looks like, if the path we are going to adjust exists, I can use realpath() to flush out symbolic links and generate a warning. For example,

 realpath(path, real_path);
if (strcmp(path, real_path))
  /* warn about using the real path */ 

@vapier
Copy link
Member

vapier commented Jan 25, 2018

technically that suffers from a TOCTOU race still

@williamh
Copy link
Contributor

@vapier yes, it does, but I'm at the point right now where I'm thinking about narrowing the window as much as possible since I don't know of a way to completely remove the race.
Opentmpfiles is the portable version of tmpfiles, since systemd's tmpfiles is not portable.
The use of checkpath vs tmpfiles is a separate issue that we should look at, but probably not on this bug.

@orlitzky
Copy link
Contributor Author

yes, it does, but I'm at the point right now where I'm thinking about narrowing the window as much as possible since I don't know of a way to completely remove the race.

The approach that Lennart suggested should work: start at / and get its FD with open, which should be safe to do because it's the root. Then for every path component afterwards, use openat() with a relative path instead of open(), to make sure that you don't accidentally follow any symlinks along the way. The O_DIRECTORY flag should be used at least, and O_NOFOLLOW and O_PATH too where available.

@vapier
Copy link
Member

vapier commented Jan 25, 2018

my point about checkpath-vs-tmpfiles is that, since they practically serve the exact same purpose, they should behave the same.

wrt opentmpfiles, that project doesn't really matter here. systemd's tmpfiles implementation is de-facto standard, so its behavior is what others (like opentmpfiles) should be implementing.

doing a recursive openat walk from / via the path components would solve the TOCTOU issue, but again run into the problem of /var/run->/run. not sure that's been decided on. i'd hate to encode a whitelist of permissible symlinks.

@williamh
Copy link
Contributor

@vapier sure, I don't plan on encoding a list of acceptable symlinks, the idea is just to complain if the path contains symlinks.

@orlitzky
Copy link
Contributor Author

The three flags O_DIRECTORY, O_NOFOLLOW, and O_PATH should ensure that openat() fails when it encounters a symlink. Since we'll need to keep the old implementation around for a while (during the grace period), we could do something like,

if (!new_implementation(path)) {
  show_warning();
  old_implementation(path);
}

Then after however-long, we just delete the fallback.

@williamh
Copy link
Contributor

@orlitzky I don't see that there is going to be a separate implementation to keep since all we are doing is scanning the path and warning if there are any symbolic links.

@orlitzky
Copy link
Contributor Author

openat() with O_NOFOLLOW is going to return an error when it encounters a symlink, so you'll have to do something else at that point during the deprecation period. You could weave the safe/unsafe versions together, falling back to open() when openat fails or something like that.

@williamh
Copy link
Contributor

@chutz @orlitzky the more I think about this issue, I don't see that there is anything we can do about it because doing something about it would require that we mandate that all paths referred to by checkpath could never contain symlinks.
I will keep this open for now in case someone comes up with a better idea.

@orlitzky
Copy link
Contributor Author

My vote is to ban them with a deprecation period, so that people have to replace /var/run and /run with e.g. @runstatedir@ and then take that value from the upstream build system or package.

Otherwise, we have a bunch of init scripts with root privilege escalations that need to be fixed or worked around.

@williamh
Copy link
Contributor

@orlitzky You are just looking at one possible symlink. What if, for example, /var is a symlink (this is allowed by fhs). Also, there is no restriction requiring checkpath to only be used for certain paths, so maybe someone is doing something with checkpath under /opt/package. Either /opt or /opt/package might legitimately be a symlink.

@orlitzky
Copy link
Contributor Author

If /var is a symlink, then you would use the path that it points to instead of "/var". Same thing with /opt and /opt/package. If someone has devised some non-service-script use for checkpath, then they'll get a warning telling them to resolve the symlinks, and they can fix the problem by resolving the symlinks.

The only situation where I see a problem is on a distro like Gentoo where /var is a directory "owned" by the package manager, and someone replaces it with a symlink. In that case, you may find calls to checkpath /var/... winding up in your distro-provided service script. Does anyone actually do that? Just because it can be done doesn't mean that we should support it. Symlinks in the terminal path component are as legitimate as anywhere else, but we still reject them because there's no benefit and it's dangerous.

But if /var starts out as a symlink and is supposed to be a symlink, then you just use where it points to in all of your build scripts and packages.

@williamh
Copy link
Contributor

@orlitzky We do not accept a symlink as the terminal component of the path because we know we want it to be a file, directory or fifo. If we want to allow a symlink, we need another command line switch and syntax, for example: "checkpath -L /link/target:/foo/bar" could create a /foo/bar symlink that points to /link/target. This has nothing to do with how "dangerous" symlinnks are as the terminal component, we just don't support them. We could if we wanted to, but that hasn't been requested so far.

@orlitzky
Copy link
Contributor Author

What I mean is, somebody could decide they want /etc/postgresql-10/postgresql.conf to be under version control, and replace the file with a link to /root/src/config.git/postgresql.conf. In that situation, the init script could still adjust the permissions on the config file (so that it's readable by the daemon) by following the symlink. I don't see how that's conceptually different than replacing the directory /etc/postgresql-10 (or even /etc itself) with a symlink, and yet we outlaw it.

@williamh
Copy link
Contributor

I don't follow your previous comment.
Checkpath doesn't modify non-terminal components of the path.

@orlitzky
Copy link
Contributor Author

With the current OpenRC, /etc/postgresql-10/postgresql.conf cannot be a symlink if I need to call checkpath on it. Why not? It's "legitimate," but so dangerous that we don't allow it.

@williamh
Copy link
Contributor

I don't see that as dangerous, it is just not an allowed option. I think the tmpfiles spec allows you to create and manipulate symlinks, I havent looked.
It would be allowed with a separate option.

checkpath -L

or something similar.

williamh added a commit that referenced this issue Nov 11, 2020
This walks the directory path to the file we are going to create to make
sure that when we create the file and change the ownership and permissions
we are working on the same file.
Also, symbolic links in the path need to be owned by root or the current
user.

On non-linux platforms, we no longer follow non-terminal symbolic links
by default. If you need to do that, add the -s option on the checkpath
command line.

This is for #201.
@williamh
Copy link
Contributor

@orlitzky please check the patch I just added. I will use _GNU_SOURCE and O_PATH reluctantly, since there doesn't appear to be another way to do this, but I'm not sure what I can do for the non-linux side other than not follow non-terminal symlinks by default.

@orlitzky
Copy link
Contributor Author

Won't this always refuse to follow symlinks on linux?

#ifdef O_PATH
	flags |= O_NOFOLLOW;
	flags |= O_PATH;
#else
	if (!symlinks)
		flags |= O_NOFOLLOW;
	flags |= O_RDONLY;
#endif 

I don't think it makes sense to (optionally) follow symlinks on non-linux systems if you'll always refuse to follow them on linux systems., especially when considering that linux is where they can be followed relatively safely. Maybe that's not what's intended though?

@williamh
Copy link
Contributor

I can use O_PATH|O_NOFOLLOW to open non-terminal symlinks without dereferencing (see the openat man page). This allows me to check who owns the symlink, and if root or the current user owns it,, it seems reasonable to follow it because chown will fail if the current user isn't root.
Unfortunately, this check can't happen anywhere else because O_PATH is a Linux specific flag.

@orlitzky
Copy link
Contributor Author

I can use O_PATH|O_NOFOLLOW to open non-terminal symlinks without dereferencing (see the openat man page). This allows me to check who owns the symlink...

OK so far...

and if root or the current user owns it,, it seems reasonable to follow it

But I don't think you ever actually follow it on Linux. You have a descriptor for the link itself in new_dirfd. In the next iteration, when the descriptor for the link is called simply dirfd, the call new_dirfd = openat(dirfd, item, flags); fails because dirfd isn't a descriptor for a directory.

So ultimately, you never follow any symlinks.

@williamh
Copy link
Contributor

I'm not quite following; I don't think dirfd is required to be a descriptor of a directory.

@williamh williamh reopened this Nov 13, 2020
@E5ten
Copy link
Contributor

E5ten commented Nov 13, 2020

I don't think a descriptor for a symlink to a directory is valid as a dirfd for *at functions.

@orlitzky
Copy link
Contributor Author

I don't think a descriptor for a symlink to a directory is valid as a dirfd for *at functions.

Right, the next iteration just fails in the presence of root-owned symlinks.

williamh added a commit that referenced this issue Nov 17, 2020
This walks the directory path to the file we are going to create to make
sure that when we create the file and change the ownership and permissions
we are working on the same file.
Also, symbolic links in the path need to be owned by root or the current
user.

On non-linux platforms, we no longer follow non-terminal symbolic links
by default. If you need to do that, add the -s option on the checkpath
command line.

This is for #201.
williamh added a commit that referenced this issue Nov 18, 2020
This walks the directory path to the file we are going to create to make
sure that when we create the file and change the ownership and permissions
we are working on the same file.
Also, symbolic links in the path need to be owned by root or the current
user.

On non-linux platforms, we no longer follow non-terminal symbolic links
by default. If you need to do that, add the -s option on the checkpath
command line.

This is for #201.
williamh added a commit that referenced this issue Nov 18, 2020
This walks the directory path to the file we are going to manipulate to make
sure that when we create the file and change the ownership and permissions
we are working on the same file.
Also, all non-terminal symbolic links must be owned by root. This will
keep a non-root user from making a symbolic link as described in the
bug. If root creates the symbolic link, it is assumed to be trusted.

On non-linux platforms, we no longer follow non-terminal symbolic links
by default. If you need to do that, add the -s option on the checkpath
command line, but keep in mind that this is not secure.

This is for #201.
@williamh
Copy link
Contributor

@chutz @orlitzky Please take a look at the current patch and let me know what you think. It seems to work here, but it would be helpful if you look it over and test yourself also. Thanks.

williamh added a commit that referenced this issue Nov 18, 2020
This walks the directory path to the file we are going to manipulate to make
sure that when we create the file and change the ownership and permissions
we are working on the same file.
Also, all non-terminal symbolic links must be owned by root. This will
keep a non-root user from making a symbolic link as described in the
bug. If root creates the symbolic link, it is assumed to be trusted.

On non-linux platforms, we no longer follow non-terminal symbolic links
by default. If you need to do that, add the -s option on the checkpath
command line, but keep in mind that this is not secure.

This is for #201.
@orlitzky
Copy link
Contributor Author

What purpose does the -s flag serve in this version?

This is probably the closest you can get if you insist on following root-owned symlinks, but it does introduce a new TOCTOU issue between

if (st.st_uid != 0)
    eerrorx("%s: %s: synbolic link %s not owned by root", applet, path, str);

and

if (readlinkat(dirfd, str, linkpath, linksize) != st.st_size)
    eerrorx("%s: symbolic link destination changed", applet);

The latter is happy to follow a non-root symlink so long as it points to a path of the same length.

@williamh
Copy link
Contributor

@orlitzky The -s is used on systems that do not have the O_PATH
flag for the open() call. In that environment, opening a path with O_NOFOLLOW causes the open to fail if the last component in the path is a symbolic link rather than dereferencing it. So, using -s allows you to dereference symbolic links instead of failing.
Combining O_NOFOLLOW and O_PATH where O_PATH is available is always fine because doing that gives you access to the symlink itself.

if (readlinkat(dirfd, str, linkpath, linksize) != st.st_size)

The reason for using readlinkat() instead of readlink() here was to avoid a toctou, based on the same idea as openat(), so how is there still a toctou?

@orlitzky
Copy link
Contributor Author

@orlitzky The -s is used on systems that do not have the O_PATH
flag for the open() call. In that environment, opening a path with O_NOFOLLOW causes the open to fail if the last component in the path is a symbolic link rather than dereferencing it. So, using -s allows you to dereference symbolic links instead of failing.
Combining O_NOFOLLOW and O_PATH where O_PATH is available is always fine because doing that gives you access to the symlink itself.

Oh, you're right. Nevermind.

if (readlinkat(dirfd, str, linkpath, linksize) != st.st_size)

The reason for using readlinkat() instead of readlink() here was to avoid a toctou, based on the same idea as openat(), so how is there still a toctou?

You check that the descriptor new_dirfd refers to a root-owned link, but then readlinkat operates on the name str rather than the descriptor new_dirfd that you just checked. Under normal conditions they will be the same, but the path component that str refers to can be replaced quickly by a non-root user, and readlinkat will still follow it.

@E5ten
Copy link
Contributor

E5ten commented Nov 19, 2020

Not useful outside of linux, but the man page for readlinkat mentions that if dirfd refers to a symlink, and was opened with O_PATH and O_NOFOLLOW, then you can do readlinkat(<dirfd>, "" to have it resolve the symlink that dirfd refers to. I think that would fix that issue, but again, only on linux.

williamh added a commit that referenced this issue Nov 19, 2020
This walks the directory path to the file we are going to manipulate to make
sure that when we create the file and change the ownership and permissions
we are working on the same file.
Also, all non-terminal symbolic links must be owned by root. This will
keep a non-root user from making a symbolic link as described in the
bug. If root creates the symbolic link, it is assumed to be trusted.

On non-linux platforms, we no longer follow non-terminal symbolic links
by default. If you need to do that, add the -s option on the checkpath
command line, but keep in mind that this is not secure.

This is for #201.
@williamh
Copy link
Contributor

@E5ten @orlitzky @chutz
It looks like we are safe to use rdlinkat(new_dirfd, "", ...) since readlinkat is not going to be called on systems where o_path doesn't exist, so I updated the patch to do that. let me know what you think.

@E5ten
Copy link
Contributor

E5ten commented Nov 19, 2020

Unless there are systems that have O_PATH, but don't support that readlinkat extension, then that part of the change seems fine to me

@williamh
Copy link
Contributor

@E5ten
I do not know of any systems like this, so unless @chutz or @orlitzky object, I will merge this around 23:00 utc.
I will merge this around 23:00 utc if there are no other issues.

@orlitzky
Copy link
Contributor Author

@E5ten @orlitzky @chutz
It looks like we are safe to use rdlinkat(new_dirfd, "", ...) since readlinkat is not going to be called on systems where o_path doesn't exist, so I updated the patch to do that. let me know what you think.

Nice catch, looks good.

I can still prevent checkpath from doing its job entirely on Linux by messing with the link and having it crash. It's hard to imagine a privilege escalation from something like that, though, unless the service script is doing something truly bizarre.

williamh added a commit that referenced this issue Nov 20, 2020
This walks the directory path to the file we are going to manipulate to make
sure that when we create the file and change the ownership and permissions
we are working on the same file.
Also, all non-terminal symbolic links must be owned by root. This will
keep a non-root user from making a symbolic link as described in the
bug. If root creates the symbolic link, it is assumed to be trusted.

On non-linux platforms, we no longer follow non-terminal symbolic links
by default. If you need to do that, add the -s option on the checkpath
command line, but keep in mind that this is not secure.

This fixes #201.
@orlitzky
Copy link
Contributor Author

Found a typo "synbolic" after the fact.

@orlitzky
Copy link
Contributor Author

One more thing! There's a call to mkdir that should now be mkdirat instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants