From 08c7c3216bd52e65a32e087f8d2e052492024cf4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 3 Mar 2020 10:18:32 +0100 Subject: [PATCH 1/3] sysusers: many different errnos to express one condition See https://bugzilla.redhat.com/show_bug.cgi?id=1807768. It turns out that sysusers cannot query if the group exists: Failed to check if group dnsmasq already exists: No such process ... Failed to check if group systemd-timesync already exists: No such process When the same command is executed later, the issue does not occur. Not sure why the behaviour in the initial transaction is different. But let's accept all errors that the man pages list. We check if the user/group exists before creating anyway, so this seems pretty safe. --- src/sysusers/sysusers.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index 2771fd959f..1b1f19e817 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -94,6 +94,12 @@ STATIC_DESTRUCTOR_REGISTER(database_groups, set_free_freep); STATIC_DESTRUCTOR_REGISTER(uid_range, freep); STATIC_DESTRUCTOR_REGISTER(arg_root, freep); +static int errno_is_not_exists(int code) { + /* See getpwnam(3) and getgrnam(3): those codes and others can be returned if the user or group are + * not found. */ + return IN_SET(code, 0, ENOENT, ESRCH, EBADF, EPERM); +} + static int load_user_database(void) { _cleanup_fclose_ FILE *f = NULL; const char *passwd_path; @@ -971,7 +977,7 @@ static int add_user(Item *i) { return 0; } - if (!IN_SET(errno, 0, ENOENT)) + if (!errno_is_not_exists(errno)) return log_error_errno(errno, "Failed to check if user %s already exists: %m", i->name); } @@ -1108,7 +1114,7 @@ static int get_gid_by_name(const char *name, gid_t *gid) { *gid = g->gr_gid; return 0; } - if (!IN_SET(errno, 0, ENOENT)) + if (!errno_is_not_exists(errno)) return log_error_errno(errno, "Failed to check if group %s already exists: %m", name); } From 6cb356ca9fe063846cfb883ef484f7e7e411096c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 3 Mar 2020 11:51:50 +0100 Subject: [PATCH 2/3] basic/fs-util: add a version of chmod_and_chown that doesn not use /proc --- src/basic/fs-util.c | 46 +++++++++++++++++++++++++++++++++++++++++ src/basic/fs-util.h | 1 + src/test/test-fs-util.c | 45 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index f8095e85d8..558cafbcaf 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -272,6 +272,52 @@ int fchmod_and_chown(int fd, mode_t mode, uid_t uid, gid_t gid) { return do_chown || do_chmod; } +int chmod_and_chown_unsafe(const char *path, mode_t mode, uid_t uid, gid_t gid) { + bool do_chown, do_chmod; + struct stat st; + + assert(path); + + /* Change ownership and access mode of the specified path, see description of fchmod_and_chown(). + * Should only be used on trusted paths. */ + + if (lstat(path, &st) < 0) + return -errno; + + do_chown = + (uid != UID_INVALID && st.st_uid != uid) || + (gid != GID_INVALID && st.st_gid != gid); + + do_chmod = + !S_ISLNK(st.st_mode) && /* chmod is not defined on symlinks */ + ((mode != MODE_INVALID && ((st.st_mode ^ mode) & 07777) != 0) || + do_chown); /* If we change ownership, make sure we reset the mode afterwards, since chown() + * modifies the access mode too */ + + if (mode == MODE_INVALID) + mode = st.st_mode; /* If we only shall do a chown(), save original mode, since chown() might break it. */ + else if ((mode & S_IFMT) != 0 && ((mode ^ st.st_mode) & S_IFMT) != 0) + return -EINVAL; /* insist on the right file type if it was specified */ + + if (do_chown && do_chmod) { + mode_t minimal = st.st_mode & mode; /* the subset of the old and the new mask */ + + if (((minimal ^ st.st_mode) & 07777) != 0) + if (chmod(path, minimal & 07777) < 0) + return -errno; + } + + if (do_chown) + if (lchown(path, uid, gid) < 0) + return -errno; + + if (do_chmod) + if (chmod(path, mode & 07777) < 0) + return -errno; + + return do_chown || do_chmod; +} + int fchmod_umask(int fd, mode_t m) { mode_t u; int r; diff --git a/src/basic/fs-util.h b/src/basic/fs-util.h index 78d68be9fd..6b9ade2ec1 100644 --- a/src/basic/fs-util.h +++ b/src/basic/fs-util.h @@ -34,6 +34,7 @@ int readlink_and_make_absolute(const char *p, char **r); int chmod_and_chown(const char *path, mode_t mode, uid_t uid, gid_t gid); int fchmod_and_chown(int fd, mode_t mode, uid_t uid, gid_t gid); +int chmod_and_chown_unsafe(const char *path, mode_t mode, uid_t uid, gid_t gid); int fchmod_umask(int fd, mode_t mode); int fchmod_opath(int fd, mode_t m); diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index d0c6fb82bf..d97ccfda3b 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -802,6 +802,50 @@ static void test_chmod_and_chown(void) { assert_se(S_ISLNK(st.st_mode)); } +static void test_chmod_and_chown_unsafe(void) { + _cleanup_(rm_rf_physical_and_freep) char *d = NULL; + _unused_ _cleanup_umask_ mode_t u = umask(0000); + struct stat st; + const char *p; + + if (geteuid() != 0) + return; + + log_info("/* %s */", __func__); + + assert_se(mkdtemp_malloc(NULL, &d) >= 0); + + p = strjoina(d, "/reg"); + assert_se(mknod(p, S_IFREG | 0123, 0) >= 0); + + assert_se(chmod_and_chown_unsafe(p, S_IFREG | 0321, 1, 2) >= 0); + assert_se(chmod_and_chown_unsafe(p, S_IFDIR | 0555, 3, 4) == -EINVAL); + + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISREG(st.st_mode)); + assert_se((st.st_mode & 07777) == 0321); + + p = strjoina(d, "/dir"); + assert_se(mkdir(p, 0123) >= 0); + + assert_se(chmod_and_chown_unsafe(p, S_IFDIR | 0321, 1, 2) >= 0); + assert_se(chmod_and_chown_unsafe(p, S_IFREG | 0555, 3, 4) == -EINVAL); + + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISDIR(st.st_mode)); + assert_se((st.st_mode & 07777) == 0321); + + p = strjoina(d, "/lnk"); + assert_se(symlink("idontexist", p) >= 0); + + assert_se(chmod_and_chown_unsafe(p, S_IFLNK | 0321, 1, 2) >= 0); + assert_se(chmod_and_chown_unsafe(p, S_IFREG | 0555, 3, 4) == -EINVAL); + assert_se(chmod_and_chown_unsafe(p, S_IFDIR | 0555, 3, 4) == -EINVAL); + + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISLNK(st.st_mode)); +} + int main(int argc, char *argv[]) { test_setup_logging(LOG_INFO); @@ -819,6 +863,7 @@ int main(int argc, char *argv[]) { test_fsync_directory_of_file(); test_rename_noreplace(); test_chmod_and_chown(); + test_chmod_and_chown_unsafe(); return 0; } From 1fb5a5edc7c175ea0cd85a1e3a5af8d54084a891 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 3 Mar 2020 11:58:07 +0100 Subject: [PATCH 3/3] sysusers: do not require /proc to be mounted We're operating on known paths in root-owned directories here, so the detour through toctou-safe methods that require /proc to be mounted is not necessary. Should fix https://bugzilla.redhat.com/show_bug.cgi?id=1807768. --- src/sysusers/sysusers.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index 1b1f19e817..f7cc7e0900 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -199,7 +199,7 @@ static int load_group_database(void) { static int make_backup(const char *target, const char *x) { _cleanup_close_ int src = -1; _cleanup_fclose_ FILE *dst = NULL; - _cleanup_free_ char *temp = NULL; + _cleanup_free_ char *dst_tmp = NULL; char *backup; struct timespec ts[2]; struct stat st; @@ -216,7 +216,7 @@ static int make_backup(const char *target, const char *x) { if (fstat(src, &st) < 0) return -errno; - r = fopen_temporary_label(target, x, &dst, &temp); + r = fopen_temporary_label(target, x, &dst, &dst_tmp); if (r < 0) return r; @@ -230,7 +230,7 @@ static int make_backup(const char *target, const char *x) { backup = strjoina(x, "-"); /* Copy over the access mask */ - r = fchmod_and_chown(fileno(dst), st.st_mode & 07777, st.st_uid, st.st_gid); + r = chmod_and_chown_unsafe(dst_tmp, st.st_mode & 07777, st.st_uid, st.st_gid); if (r < 0) log_warning_errno(r, "Failed to change access mode or ownership of %s: %m", backup); @@ -243,7 +243,7 @@ static int make_backup(const char *target, const char *x) { if (r < 0) goto fail; - if (rename(temp, backup) < 0) { + if (rename(dst_tmp, backup) < 0) { r = -errno; goto fail; } @@ -251,7 +251,7 @@ static int make_backup(const char *target, const char *x) { return 0; fail: - (void) unlink(temp); + (void) unlink(dst_tmp); return r; } @@ -345,13 +345,13 @@ static int putsgent_with_members(const struct sgrp *sg, FILE *gshadow) { } #endif -static int sync_rights(FILE *from, FILE *to) { +static int sync_rights(FILE *from, const char *to) { struct stat st; if (fstat(fileno(from), &st) < 0) return -errno; - return fchmod_and_chown(fileno(to), st.st_mode & 07777, st.st_uid, st.st_gid); + return chmod_and_chown_unsafe(to, st.st_mode & 07777, st.st_uid, st.st_gid); } static int rename_and_apply_smack(const char *temp_path, const char *dest_path) { @@ -389,7 +389,7 @@ static int write_temporary_passwd(const char *passwd_path, FILE **tmpfile, char original = fopen(passwd_path, "re"); if (original) { - r = sync_rights(original, passwd); + r = sync_rights(original, passwd_tmp); if (r < 0) return r; @@ -491,7 +491,7 @@ static int write_temporary_shadow(const char *shadow_path, FILE **tmpfile, char original = fopen(shadow_path, "re"); if (original) { - r = sync_rights(original, shadow); + r = sync_rights(original, shadow_tmp); if (r < 0) return r; @@ -588,7 +588,7 @@ static int write_temporary_group(const char *group_path, FILE **tmpfile, char ** original = fopen(group_path, "re"); if (original) { - r = sync_rights(original, group); + r = sync_rights(original, group_tmp); if (r < 0) return r; @@ -687,7 +687,7 @@ static int write_temporary_gshadow(const char * gshadow_path, FILE **tmpfile, ch if (original) { struct sgrp *sg; - r = sync_rights(original, gshadow); + r = sync_rights(original, gshadow_tmp); if (r < 0) return r;