From 51207ca134716a0dee5fd763a6c39204be849eb1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 22 Jan 2018 21:03:53 +0100 Subject: [PATCH 1/7] tmpfiles: change ownership of symlinks too Ownership is supported for symlinks, too, only file modes are not. Support that too. Fixes: #7509 --- src/tmpfiles/tmpfiles.c | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index a090d86a6c..3fdbfca9a2 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -754,6 +754,7 @@ finish: } static int path_set_perms(Item *i, const char *path) { + char fn[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int)]; _cleanup_close_ int fd = -1; struct stat st; @@ -784,14 +785,12 @@ static int path_set_perms(Item *i, const char *path) { if (fstatat(fd, "", &st, AT_EMPTY_PATH) < 0) return log_error_errno(errno, "Failed to fstat() file %s: %m", path); - if (S_ISLNK(st.st_mode)) - log_debug("Skipping mode and owner fix for symlink %s.", path); - else { - char fn[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int)]; - xsprintf(fn, "/proc/self/fd/%i", fd); + xsprintf(fn, "/proc/self/fd/%i", fd); - /* not using i->path directly because it may be a glob */ - if (i->mode_set) { + if (i->mode_set) { + if (S_ISLNK(st.st_mode)) + log_debug("Skipping mode fix for symlink %s.", path); + else { mode_t m = i->mode; if (i->mask_perms) { @@ -806,25 +805,27 @@ static int path_set_perms(Item *i, const char *path) { } if (m == (st.st_mode & 07777)) - log_debug("\"%s\" has right mode %o", path, st.st_mode); + log_debug("\"%s\" has correct mode %o already.", path, st.st_mode); else { - log_debug("chmod \"%s\" to mode %o", path, m); + log_debug("Changing \"%s\" to mode %o.", path, m); + if (chmod(fn, m) < 0) return log_error_errno(errno, "chmod() of %s via %s failed: %m", path, fn); } } + } - if ((i->uid != st.st_uid || i->gid != st.st_gid) && - (i->uid_set || i->gid_set)) { - log_debug("chown \"%s\" to "UID_FMT"."GID_FMT, - path, - i->uid_set ? i->uid : UID_INVALID, - i->gid_set ? i->gid : GID_INVALID); - if (chown(fn, - i->uid_set ? i->uid : UID_INVALID, - i->gid_set ? i->gid : GID_INVALID) < 0) - return log_error_errno(errno, "chown() of %s via %s failed: %m", path, fn); - } + if ((i->uid != st.st_uid || i->gid != st.st_gid) && + (i->uid_set || i->gid_set)) { + log_debug("Changing \"%s\" to owner "UID_FMT":"GID_FMT, + path, + i->uid_set ? i->uid : UID_INVALID, + i->gid_set ? i->gid : GID_INVALID); + + if (chown(fn, + i->uid_set ? i->uid : UID_INVALID, + i->gid_set ? i->gid : GID_INVALID) < 0) + return log_error_errno(errno, "chown() of %s via %s failed: %m", path, fn); } fd = safe_close(fd); From 59793c8f2e29507c2f6cd8bd1e8785bfbefd2952 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 22 Jan 2018 21:10:27 +0100 Subject: [PATCH 2/7] tmpfiles: shortcut path_set_perms() if there's nothing to do No need to open() anything in that case, hence don't. --- src/tmpfiles/tmpfiles.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 3fdbfca9a2..c11a649c2f 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -761,6 +761,9 @@ static int path_set_perms(Item *i, const char *path) { assert(i); assert(path); + if (!i->mode_set && !i->uid_set && !i->gid_set) + goto shortcut; + /* We open the file with O_PATH here, to make the operation * somewhat atomic. Also there's unfortunately no fchmodat() * with AT_SYMLINK_NOFOLLOW, hence we emulate it here via @@ -778,7 +781,6 @@ static int path_set_perms(Item *i, const char *path) { } log_full_errno(level, errno, "Adjusting owner and mode for %s failed: %m", path); - return r; } @@ -830,6 +832,7 @@ static int path_set_perms(Item *i, const char *path) { fd = safe_close(fd); +shortcut: return label_fix(path, false, false); } From dc2335669afddc767eea2757f8d7dfc7a8f927fa Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 22 Jan 2018 21:11:04 +0100 Subject: [PATCH 3/7] tmpfiles: fix check for figuring out whether to call chmod() No need to call chown() if everything matches already. --- src/tmpfiles/tmpfiles.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index c11a649c2f..d733768f27 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -817,8 +817,8 @@ static int path_set_perms(Item *i, const char *path) { } } - if ((i->uid != st.st_uid || i->gid != st.st_gid) && - (i->uid_set || i->gid_set)) { + if ((i->uid_set && i->uid != st.st_uid) || + (i->gid_set && i->gid != st.st_gid)) { log_debug("Changing \"%s\" to owner "UID_FMT":"GID_FMT, path, i->uid_set ? i->uid : UID_INVALID, From 5579f85663d10269e7ac7464be6548c99cea4ada Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 23 Jan 2018 14:03:34 +0100 Subject: [PATCH 4/7] tmpfiles: refuse to chown()/chmod() files which are hardlinked, unless protected_hardlinks sysctl is on Let's add some extra safety. Fixes: #7736 --- src/tmpfiles/tmpfiles.c | 43 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index d733768f27..5b56e7dcdd 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -753,6 +753,39 @@ finish: return r; } +static bool dangerous_hardlinks(void) { + _cleanup_free_ char *value = NULL; + static int cached = -1; + int r; + + /* Check whether the fs.protected_hardlinks sysctl is on. If we can't determine it we assume its off, as that's + * what the upstream default is. */ + + if (cached >= 0) + return cached; + + r = read_one_line_file("/proc/sys/fs/protected_hardlinks", &value); + if (r < 0) { + log_debug_errno(r, "Failed to read fs.protected_hardlinks sysctl: %m"); + return true; + } + + r = parse_boolean(value); + if (r < 0) { + log_debug_errno(r, "Failed to parse fs.protected_hardlinks sysctl: %m"); + return true; + } + + cached = r == 0; + return cached; +} + +static bool hardlink_vulnerable(struct stat *st) { + assert(st); + + return !S_ISDIR(st->st_mode) && st->st_nlink > 1 && dangerous_hardlinks(); +} + static int path_set_perms(Item *i, const char *path) { char fn[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int)]; _cleanup_close_ int fd = -1; @@ -787,6 +820,11 @@ static int path_set_perms(Item *i, const char *path) { if (fstatat(fd, "", &st, AT_EMPTY_PATH) < 0) return log_error_errno(errno, "Failed to fstat() file %s: %m", path); + if (hardlink_vulnerable(&st)) { + log_error("Refusing to set permissions on hardlinked file %s while the fs.protected_hardlinks sysctl is turned off.", path); + return -EPERM; + } + xsprintf(fn, "/proc/self/fd/%i", fd); if (i->mode_set) { @@ -971,6 +1009,11 @@ static int path_set_acls(Item *item, const char *path) { if (fstatat(fd, "", &st, AT_EMPTY_PATH) < 0) return log_error_errno(errno, "Failed to fstat() file %s: %m", path); + if (hardlink_vulnerable(&st)) { + log_error("Refusing to set ACLs on hardlinked file %s while the fs.protected_hardlinks sysctl is turned off.", path); + return -EPERM; + } + if (S_ISLNK(st.st_mode)) { log_debug("Skipping ACL fix for symlink %s.", path); return 0; From 7fa1074831202e1477e0bb5e03b7570592046e99 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 23 Jan 2018 14:14:19 +0100 Subject: [PATCH 5/7] tmpfiles: create parent directories if they are missing for more line types Currently, we create leading directories implicitly for all lines that create directory or directory-like nodes. With this, we also do the same for a number of other lines: f/F, C, p, L, c/b (that is regular files, pipes, symlinks, device nodes as well as file trees we copy). The leading directories are created with te default access mode of 0755. If something else is desired, users should simply declare appropriate "d" lines. Fixes: #7853 --- man/tmpfiles.d.xml | 7 +++++++ src/tmpfiles/tmpfiles.c | 22 ++++++++++++++++++++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/man/tmpfiles.d.xml b/man/tmpfiles.d.xml index 861c6eb1eb..30aa886388 100644 --- a/man/tmpfiles.d.xml +++ b/man/tmpfiles.d.xml @@ -484,6 +484,13 @@ r! /tmp/.X[0-9]*-lock The second line in contrast to the first one would break a running system, and will only be executed with . + + Note that for all line types that result in creation of any kind of file node + (i.e. f/F, + d/D/v/q/Q, + p, L, c/b and C) + leading directories are implicitly created if needed, owned by root with an access mode of 0755. In order to + create them with different modes or ownership make sure to add appropriate d lines. diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 5b56e7dcdd..4d8c36870c 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -1343,14 +1343,24 @@ static int create_item(Item *i) { case CREATE_FILE: case TRUNCATE_FILE: + RUN_WITH_UMASK(0000) + (void) mkdir_parents_label(i->path, 0755); + r = write_one_file(i, i->path); if (r < 0) return r; break; case COPY_FILES: { + + RUN_WITH_UMASK(0000) + (void) mkdir_parents_label(i->path, 0755); + log_debug("Copying tree \"%s\" to \"%s\".", i->argument, i->path); - r = copy_tree(i->argument, i->path, i->uid_set ? i->uid : UID_INVALID, i->gid_set ? i->gid : GID_INVALID, COPY_REFLINK); + r = copy_tree(i->argument, i->path, + i->uid_set ? i->uid : UID_INVALID, + i->gid_set ? i->gid : GID_INVALID, + COPY_REFLINK); if (r == -EROFS && stat(i->path, &st) == 0) r = -EEXIST; @@ -1392,7 +1402,7 @@ static int create_item(Item *i) { case CREATE_SUBVOLUME_INHERIT_QUOTA: case CREATE_SUBVOLUME_NEW_QUOTA: RUN_WITH_UMASK(0000) - mkdir_parents_label(i->path, 0755); + (void) mkdir_parents_label(i->path, 0755); if (IN_SET(i->type, CREATE_SUBVOLUME, CREATE_SUBVOLUME_INHERIT_QUOTA, CREATE_SUBVOLUME_NEW_QUOTA)) { @@ -1474,6 +1484,8 @@ static int create_item(Item *i) { case CREATE_FIFO: RUN_WITH_UMASK(0000) { + (void) mkdir_parents_label(i->path, 0755); + mac_selinux_create_file_prepare(i->path, S_IFIFO); r = mkfifo(i->path, i->mode); mac_selinux_create_file_clear(); @@ -1516,6 +1528,9 @@ static int create_item(Item *i) { } case CREATE_SYMLINK: { + RUN_WITH_UMASK(0000) + (void) mkdir_parents_label(i->path, 0755); + mac_selinux_create_file_prepare(i->path, S_IFLNK); r = symlink(i->argument, i->path); mac_selinux_create_file_clear(); @@ -1574,6 +1589,9 @@ static int create_item(Item *i) { return 0; } + RUN_WITH_UMASK(0000) + (void) mkdir_parents_label(i->path, 0755); + file_type = i->type == CREATE_BLOCK_DEVICE ? S_IFBLK : S_IFCHR; RUN_WITH_UMASK(0000) { From 45a080944d3bf20175de6fc5fa1e63abd82d9b70 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 23 Jan 2018 14:32:50 +0100 Subject: [PATCH 6/7] hwdb: whitespace fix to make "ninja test" work again Fixes: #7975 --- hwdb/60-keyboard.hwdb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hwdb/60-keyboard.hwdb b/hwdb/60-keyboard.hwdb index da486cfde2..ebfc86a2e9 100644 --- a/hwdb/60-keyboard.hwdb +++ b/hwdb/60-keyboard.hwdb @@ -1349,7 +1349,7 @@ evdev:input:b0003v1038p0310* KEYBOARD_KEY_70030=f9 KEYBOARD_KEY_7002f=f11 KEYBOARD_KEY_70046=f6 - + ########################################################### # Other ########################################################### From 2e276b1d9b475335bd4c10648cce65d4391181c3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 23 Jan 2018 21:16:59 +0100 Subject: [PATCH 7/7] UIDS-GIDS.md: explicitly mention one more user of the overflowuid File systems with only 16bit UID support (i.e. old ext2) also use the overflowuid to map users they can't map. Briefly mention this. --- UIDS-GIDS.md | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/UIDS-GIDS.md b/UIDS-GIDS.md index 71e91faa6a..e19cc88162 100644 --- a/UIDS-GIDS.md +++ b/UIDS-GIDS.md @@ -17,13 +17,14 @@ i.e. 0…4294967295. However, four UIDs are special on Linux: 1. 0 → The `root` super-user 2. 65534 → The `nobody` UID, also called the "overflow" UID or similar. It's - where various subsystems map unmappable users to, for example NFS or user - namespacing. (The latter can be changed with a sysctl during runtime, but - that's not supported on `systemd`. If you do change it you void your - warranty.) Because Fedora is a bit confused the `nobody` user is called - `nfsnobody` there (and they have a different `nobody` user at UID 99). I - hope this will be corrected eventually though. (Also, some distributions - call the `nobody` group `nogroup`. I wish they didn't.) + where various subsystems map unmappable users to, for example file systems + only supporting 16bit UIDs, NFS or user namespacing. (The latter can be + changed with a sysctl during runtime, but that's not supported on + `systemd`. If you do change it you void your warranty.) Because Fedora is a + bit confused the `nobody` user is called `nfsnobody` there (and they have a + different `nobody` user at UID 99). I hope this will be corrected eventually + though. (Also, some distributions call the `nobody` group `nogroup`. I wish + they didn't.) 3. 4294967295, aka "32bit `(uid_t) -1`" → This UID is not a valid user ID, as `setresuid()`, `chown()` and friends treat -1 as a special request to not