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

tmpfiles: symlinks are followed in non-terminal path components (CVE-2018-6954) #7986

Closed
orlitzky opened this Issue Jan 24, 2018 · 14 comments

Comments

5 participants
@orlitzky
Copy link

orlitzky commented Jan 24, 2018

Sorry to keep harassing you with these. I think open() is following symlinks that don't appear as the last path component. In other words, if we are at the point where tmpfiles is about to open(/var/lib/systemd-exploit-recursive/foo/passwd,...), then I can replace the "foo" component with a symlink to /etc, resulting in open(/etc/passwd,...) and a fairly easy root exploit for any Z type.

Same disclaimer: I'm running tmpfiles from a git checkout, but not booting systemd. Now I have fs.protected_hardlinks=1 and fs.protected_symlinks=1. I'm using the same tmpfiles.d entry as before,

$ cat /etc/tmpfiles.d/exploit-recursive.conf 
d /var/lib/systemd-exploit-recursive 0755 mjo mjo
Z /var/lib/systemd-exploit-recursive 0755 mjo mjo

I start the service once, so that /var/lib/systemd-exploit-recursive exists:

$ sudo ./build/systemd-tmpfiles --create

Now, as the owner (mjo) of /var/lib/systemd-exploit-recursive, I can do, in another terminal,

$ cd /var/lib/systemd-exploit-recursive
$ mkdir foo
$ cd foo
$ echo $(seq 1 500000) | xargs touch
$ touch passwd
$ cd ..

The 500,000 dummy files buy some time to swap in the symlink before the loop gets to passwd. Now, restart the service back in the first terminal...

$ sudo ./build/systemd-tmpfiles --create

And while that is busy looping on the 500,000 dummy files, swap out foo with a symlink in the second terminal:

$ mv foo bar && ln -s /etc ./foo

(I've used "mv" because "rm" is ironically slow at deleting my own dummy files.)

The end result is that I wind up as the owner of /etc/passwd, and the fs.protected_symlinks sysctl doesn't protect against this.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Jan 24, 2018

hmm, yeah, the current code there sucks. when traversing recursively through directory trees we should never use absolute paths, but always operate with openat() and friends and only operate on one level at a time. We are doing that at most other places these days, but tmpfiles is a hold-out in this regard...

@williamh

This comment has been minimized.

Copy link

williamh commented Jan 24, 2018

@poettering You will eventually hit the same issue I am hitting with OpenRC. There are valid cases for symlinks, e.g. at least /var/run and /var/lock, so if you force all non-terminal path components to not be symlinks you might break things. I'm not sure there is much that can be done about this.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Jan 24, 2018

@williamh well, if we descend into the tree to re-chown() one level at a time, and strictly only use openat() then there never are non-terminal symlinks we have to care about really, all of them are terminal.

See https://github.com/systemd/systemd/blob/master/src/core/chown-recursive.c#L62 for example, which is a recursive chown() used for systemd's DynamicUser=1 functionality that re-chown()s a directory tree for the UID assigned to a service shortly before invoking the service, if that's necessary.

As you can see we'll never ever descend into symlink stuff, there's a superficial check through the stat data, but then when we are about to enter the subdir, we open it with O_DIRECTORY|O_NOFOLLOW which means if the thing gets replaced by a symlink right there we'll fail the whole thing, and hence are protected.

And yeah, we should make tmpfiles.c work the same way. The only reason we currently don't is that tmpfiles.c is much much older.

(in fact the code I linked isn't perfect either. By making use of O_PATH we could make the whole thing even better, as the user couldn't replace the inode between our stat and openat calls at all)

@orlitzky orlitzky changed the title tmpfiles: symlinks are followed in non-terminal path components tmpfiles: symlinks are followed in non-terminal path components (CVE-2018-6954) Feb 13, 2018

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Feb 19, 2018

@poettering, would something like fbuihuu/systemd-opensuse-next@f213ea2 be ok ?

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Feb 19, 2018

BTW it's based on v234 but I'll rebase it on top of master if it's the way to go.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Feb 26, 2018

@fbuihuu yes, something like that. I added a number of comments there. Any chance you can rework this accordingly and post as PR for further review?

Key though should be that we operate as long as we can with O_PATH fds of everything (because that pins the node and allows us to operate on it, adjusting xattrs, acls, mode and ownership while being sure that it wont be replaced underneath us). Only when we actually want a regular fd (for the chattr ioctl) or a directory fd (to enumerate child nodes) we should convert the O_PATH fd into a regular fd by opening its /proc/self/fd/%i link. With that approach we can neatly avoid most issues, as we nothing can change under our view...

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Feb 28, 2018

Key though should be that we operate as long as we can with O_PATH fds of everything (because that pins the node and allows us to operate on it, adjusting xattrs, acls, mode and ownership while being sure that it wont be replaced underneath us).

Hmm... I don't see why O_PATH is the key here and where its "pin the node" feature comes from. As long as a file/dir is opened (with or without O_PATH flag) and you get a valid file descriptor, it remains "pinned" until the fd is closed, no ?

In my understanding the key here was to use ...at() syscalls as we only resolve pathnames only once.

O_PATH might be useful in our case but only because it doesn't involve access to the filesystem object which makes the operation slighly cheaper, nothing else.

No ?

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Feb 28, 2018

O_PATH allows us to pin any kind of file object, i.e. also symlinks, device nodes, sockets, regardless of the type, and without following/triggering it, and we can apply all kinds of attribute changes to it.

If we wouldn't use O_PATH then we can only realistically deal with regular files and directories, but given that these are not the only kind of nodes handling them is always racy: we wouldn't want to open() device nodes or fifos (since doing so as non-trivial effects on drivers and apps behind them), and we can't open sockets or symlinks (since that's simply not defined or does something weird), but if we'd check beforehand what they are and only then decide whether to open() them then things would be racy since they might have been swapped out in the meantime. Moreover, not using O_PATH always means that for regular files/dirs we could use fchmod()/fchown(), but for symlinks/device nodes/sockets/symlinks we'd have to use chmod()/chown()/… on the paths. O_PATH makes all these races and different codepaths go away: we can open the node right-away, regardless of its type and then invoke chmod()/chown()/… via the /proc/self/fd/%i pseudo-symlink, which works for all kinds of objects the same and requires no checking of type beforehand.

Hence yes, O_PATH is required to fix these races fully.

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Feb 28, 2018

Ok I see now your point now. I'll update the PR.

Thanks.

fbuihuu added a commit to fbuihuu/systemd that referenced this issue Mar 5, 2018

tmpfiles: don't resolve pathnames when traversing recursively through…
… directory trees

Otherwise we can be fooled if one path component is replaced underneath us.

The patch achieves that by always operating at file descriptor level (by using
*at() helpers) and by making sure we do not any path resolution when traversing
direcotry trees.

However this is not always possible, for instance when listing the content of a
directory or some operations don't provide the *at() helpers or others (such as
fchmodat()) don't have the AT_EMPTY_PATH flag. In such cases we operate on
/proc/self/fd/%i pseudo-symlink instead, which works the same for all kinds of
objects and requires no checking of type beforehand.

Also O_PATH flag is used when opening file objects in order to prevent
undesired behaviors: device nodes from reacting, automounts from
triggering, etc...

Fixes: systemd#7986
Fixes: CVE-2018-6954
@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Mar 5, 2018

@poettering PR available here #8358

@yuwata yuwata added the has-pr label Mar 5, 2018

@keszybz keszybz closed this in 936f6bd Mar 5, 2018

@orlitzky

This comment has been minimized.

Copy link

orlitzky commented Mar 7, 2018

I don't think this is entirely fixed... the recursive part looks good, but the tmpfiles.d entries still wind up making one call to e.g. path_set_perms that will traverse non-terminal symlinks. You have to get a little bit more creative with the proof-of-concept:

d /var/lib/systemd-exploit-recursive 0755 mjo mjo
d /var/lib/systemd-exploit-recursive/foo 0755 mjo mjo
f /var/lib/systemd-exploit-recursive/foo/bar 0755 mjo mjo

Now after "foo" is created, I can mv foo oldfoo && ln -s /etc/env.d ./foo. If I'm fast enough, tmpfiles will call path_set_perms on foo/bar, traversing the "foo" symlink, and allowing me to write arbitrary bash code into the env.d directory.

The problem is pretty much the same: the initial open() gets you the wrong fd, and then procfs trick doesn't help because it safely resolves the path for the wrong fd.

@yuwata yuwata reopened this Mar 8, 2018

@yuwata yuwata removed the has-pr label Mar 8, 2018

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Mar 8, 2018

Well we're supposed to follow symlinks in the path except for the final component... so I don't how such (dangerous) tmpfiles directives could be fixed.

Unless we introduce a safe mode (perhaps enabled by default) which makes tmpfiles refuse to follow symlinks which are not owned by root.

@orlitzky

This comment has been minimized.

Copy link

orlitzky commented Mar 8, 2018

Well we're supposed to follow symlinks in the path except for the final component...

The spec says only "does not follow symlinks", so it would be easy to interpret that in whichever way is convenient =)

The real question is whether or not changing this behavior would break anything. It certainly could, if people have /var/lib or some other common tmpfiles paths as symlinks -- but maybe they're all owned by root?

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Apr 26, 2018

Ok, I got some time to prep an RFC which hopefully will fix this issue completely, see #8822

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