From fd74c6f3f83185f95dfb464db0f0a73ba69ec841 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Thu, 29 Nov 2018 11:08:52 +0100 Subject: [PATCH 1/5] fs-util: add new CHASE_WARN flag to chase_symlinks() This flag can be used to make chase_symlinks() emit a warning when it encounters an error. Such flag can be useful for generating a comprehensive and detailed warning since chase_symlinks() can generate a warning with a full context. For now only warnings for unsafe transitions are produced. --- src/basic/fs-util.c | 21 ++++++++++++++++++--- src/basic/fs-util.h | 1 + 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 55651baa80..e45ad06491 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -15,6 +15,7 @@ #include "fd-util.h" #include "fileio.h" #include "fs-util.h" +#include "locale-util.h" #include "log.h" #include "macro.h" #include "missing.h" @@ -644,6 +645,20 @@ static bool safe_transition(const struct stat *a, const struct stat *b) { return a->st_uid == b->st_uid; /* Otherwise we need to stay within the same UID */ } +static int log_unsafe_transition(int a, int b, const char *path, unsigned flags) { + _cleanup_free_ char *n1 = NULL, *n2 = NULL; + + if (!FLAGS_SET(flags, CHASE_WARN)) + return -EPERM; + + (void) fd_get_path(a, &n1); + (void) fd_get_path(b, &n2); + + return log_warning_errno(SYNTHETIC_ERRNO(EPERM), + "Detected unsafe path transition %s %s %s during canonicalization of %s.", + n1, special_glyph(ARROW), n2, path); +} + int chase_symlinks(const char *path, const char *original_root, unsigned flags, char **ret) { _cleanup_free_ char *buffer = NULL, *done = NULL, *root = NULL; _cleanup_close_ int fd = -1; @@ -819,7 +834,7 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, return -errno; if (!safe_transition(&previous_stat, &st)) - return -EPERM; + return log_unsafe_transition(fd, fd_parent, path, flags); previous_stat = st; } @@ -860,7 +875,7 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, return -errno; if ((flags & CHASE_SAFE) && !safe_transition(&previous_stat, &st)) - return -EPERM; + return log_unsafe_transition(fd, child, path, flags); previous_stat = st; @@ -899,7 +914,7 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, return -errno; if (!safe_transition(&previous_stat, &st)) - return -EPERM; + return log_unsafe_transition(child, fd, path, flags); previous_stat = st; } diff --git a/src/basic/fs-util.h b/src/basic/fs-util.h index 955b146a6a..7ad030be5d 100644 --- a/src/basic/fs-util.h +++ b/src/basic/fs-util.h @@ -74,6 +74,7 @@ enum { CHASE_TRAIL_SLASH = 1 << 5, /* If set, any trailing slash will be preserved */ CHASE_STEP = 1 << 6, /* If set, just execute a single step of the normalization */ CHASE_NOFOLLOW = 1 << 7, /* Only valid with CHASE_OPEN: when the path's right-most component refers to symlink return O_PATH fd of the symlink, rather than following it. */ + CHASE_WARN = 1 << 8, /* Emit an appropriate warning when an error is encountered */ }; /* How many iterations to execute before returning -ELOOP */ From 36c97decbe46a62b6976b8ba7c59e8552f14f935 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Fri, 30 Nov 2018 15:13:44 +0100 Subject: [PATCH 2/5] fs-util: make chase_symlink() returns -ENOLINK when unsafe transitions are met We previously returned -EPERM but it can be returned for various other reasons too. Let's use -ENOLINK instead as this value shouldn't be used currently. This allows users of CHASE_SAFE to detect without any ambiguities when unsafe transitions are encountered by chase_symlinks(). All current users of CHASE_SAFE that explicitly reacted on -EPERM have been converted to react on -ENOLINK. --- src/basic/fs-util.c | 8 ++++++-- src/core/service.c | 4 ++-- src/test/test-fs-util.c | 4 ++-- src/tmpfiles/tmpfiles.c | 4 ++-- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index e45ad06491..f91d338507 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -649,12 +649,12 @@ static int log_unsafe_transition(int a, int b, const char *path, unsigned flags) _cleanup_free_ char *n1 = NULL, *n2 = NULL; if (!FLAGS_SET(flags, CHASE_WARN)) - return -EPERM; + return -ENOLINK; (void) fd_get_path(a, &n1); (void) fd_get_path(b, &n2); - return log_warning_errno(SYNTHETIC_ERRNO(EPERM), + return log_warning_errno(SYNTHETIC_ERRNO(ENOLINK), "Detected unsafe path transition %s %s %s during canonicalization of %s.", n1, special_glyph(ARROW), n2, path); } @@ -719,6 +719,10 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, * path is fully normalized, and == 0 for each normalization step. This may be combined with * CHASE_NONEXISTENT, in which case 1 is returned when a component is not found. * + * 4. With CHASE_SAFE: in this case the path must not contain unsafe transitions, i.e. transitions from + * unprivileged to privileged files or directories. In such cases the return value is -ENOLINK. If + * CHASE_WARN is also set a warning describing the unsafe transition is emitted. + * * */ /* A root directory of "/" or "" is identical to none */ diff --git a/src/core/service.c b/src/core/service.c index 964a7fd057..1fafb33f23 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -934,8 +934,8 @@ static int service_load_pid_file(Service *s, bool may_warn) { prio = may_warn ? LOG_INFO : LOG_DEBUG; fd = chase_symlinks(s->pid_file, NULL, CHASE_OPEN|CHASE_SAFE, NULL); - if (fd == -EPERM) { - log_unit_full(UNIT(s), LOG_DEBUG, fd, "Permission denied while opening PID file or potentially unsafe symlink chain, will now retry with relaxed checks: %s", s->pid_file); + if (fd == -ENOLINK) { + log_unit_full(UNIT(s), LOG_DEBUG, fd, "Potentially unsafe symlink chain, will now retry with relaxed checks: %s", s->pid_file); questionable_pid_file = true; diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index d5f652c938..26980d939f 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -249,11 +249,11 @@ static void test_chase_symlinks(void) { assert_se(chase_symlinks(q, NULL, CHASE_SAFE, NULL) >= 0); assert_se(chown(q, 0, 0) >= 0); - assert_se(chase_symlinks(q, NULL, CHASE_SAFE, NULL) == -EPERM); + assert_se(chase_symlinks(q, NULL, CHASE_SAFE, NULL) == -ENOLINK); assert_se(rmdir(q) >= 0); assert_se(symlink("/etc/passwd", q) >= 0); - assert_se(chase_symlinks(q, NULL, CHASE_SAFE, NULL) == -EPERM); + assert_se(chase_symlinks(q, NULL, CHASE_SAFE, NULL) == -ENOLINK); assert_se(chown(p, 0, 0) >= 0); assert_se(chase_symlinks(q, NULL, CHASE_SAFE, NULL) >= 0); diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index eeeb1d1850..1f2caf5f73 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -862,7 +862,7 @@ static int path_open_parent_safe(const char *path) { return log_oom(); fd = chase_symlinks(dn, NULL, CHASE_OPEN|CHASE_SAFE, NULL); - if (fd == -EPERM) + if (fd == -ENOLINK) return log_error_errno(fd, "Unsafe symlinks encountered in %s, refusing.", path); if (fd < 0) return log_error_errno(fd, "Failed to validate path %s: %m", path); @@ -885,7 +885,7 @@ static int path_open_safe(const char *path) { path); fd = chase_symlinks(path, NULL, CHASE_OPEN|CHASE_SAFE|CHASE_NOFOLLOW, NULL); - if (fd == -EPERM) + if (fd == -ENOLINK) return log_error_errno(fd, "Unsafe symlinks encountered in %s, refusing.", path); if (fd < 0) return log_error_errno(fd, "Failed to validate path %s: %m", path); From 7f0704da9454d36d19920e033ddadf06c9c6441e Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Wed, 28 Nov 2018 16:09:16 +0100 Subject: [PATCH 3/5] tmpfiles: use CHASE_WARN in addition to CHASE_SAFE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit and let's emit a more comprehensive warning when an unsafe transition is encountered. Before this patch: Unsafe symlinks encountered in /run/nrpe, refusing. After: Detected unsafe path transition / → /run during canonicalization of /run/nrpe. --- src/tmpfiles/tmpfiles.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 1f2caf5f73..d4e4f0c535 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -861,10 +861,8 @@ static int path_open_parent_safe(const char *path) { if (!dn) return log_oom(); - fd = chase_symlinks(dn, NULL, CHASE_OPEN|CHASE_SAFE, NULL); - if (fd == -ENOLINK) - return log_error_errno(fd, "Unsafe symlinks encountered in %s, refusing.", path); - if (fd < 0) + fd = chase_symlinks(dn, NULL, CHASE_OPEN|CHASE_SAFE|CHASE_WARN, NULL); + if (fd < 0 && fd != -ENOLINK) return log_error_errno(fd, "Failed to validate path %s: %m", path); return fd; @@ -884,10 +882,8 @@ static int path_open_safe(const char *path) { "Failed to open invalid path '%s'.", path); - fd = chase_symlinks(path, NULL, CHASE_OPEN|CHASE_SAFE|CHASE_NOFOLLOW, NULL); - if (fd == -ENOLINK) - return log_error_errno(fd, "Unsafe symlinks encountered in %s, refusing.", path); - if (fd < 0) + fd = chase_symlinks(path, NULL, CHASE_OPEN|CHASE_SAFE|CHASE_WARN|CHASE_NOFOLLOW, NULL); + if (fd < 0 && fd != -ENOLINK) return log_error_errno(fd, "Failed to validate path %s: %m", path); return fd; From b85ee2ec95808db6981d2659610ed1713738df11 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Thu, 29 Nov 2018 11:21:12 +0100 Subject: [PATCH 4/5] fs-util: rename safe_transition() into unsafe_transition() We're always interested into finding unsafe transitions so let's make the helper return true when it finds such transitions so we don't need to negate its results. No functional changes. --- src/basic/fs-util.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index f91d338507..59383c52d2 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -634,15 +634,15 @@ int inotify_add_watch_fd(int fd, int what, uint32_t mask) { return r; } -static bool safe_transition(const struct stat *a, const struct stat *b) { +static bool unsafe_transition(const struct stat *a, const struct stat *b) { /* Returns true if the transition from a to b is safe, i.e. that we never transition from unprivileged to * privileged files or directories. Why bother? So that unprivileged code can't symlink to privileged files * making us believe we read something safe even though it isn't safe in the specific context we open it in. */ if (a->st_uid == 0) /* Transitioning from privileged to unprivileged is always fine */ - return true; + return false; - return a->st_uid == b->st_uid; /* Otherwise we need to stay within the same UID */ + return a->st_uid != b->st_uid; /* Otherwise we need to stay within the same UID */ } static int log_unsafe_transition(int a, int b, const char *path, unsigned flags) { @@ -837,7 +837,7 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, if (fstat(fd_parent, &st) < 0) return -errno; - if (!safe_transition(&previous_stat, &st)) + if (unsafe_transition(&previous_stat, &st)) return log_unsafe_transition(fd, fd_parent, path, flags); previous_stat = st; @@ -878,7 +878,7 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, if (fstat(child, &st) < 0) return -errno; if ((flags & CHASE_SAFE) && - !safe_transition(&previous_stat, &st)) + unsafe_transition(&previous_stat, &st)) return log_unsafe_transition(fd, child, path, flags); previous_stat = st; @@ -917,7 +917,7 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, if (fstat(fd, &st) < 0) return -errno; - if (!safe_transition(&previous_stat, &st)) + if (unsafe_transition(&previous_stat, &st)) return log_unsafe_transition(child, fd, path, flags); previous_stat = st; From 145b8d0f68485e54cea3beade7166ade8216b9d2 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Fri, 30 Nov 2018 15:43:13 +0100 Subject: [PATCH 5/5] fs-util: make CHASE_WARN effective with CHASE_NO_AUTOFS This has the side effect to upgrade the log level at which the log is emitted from debug to warning. This might be better since after all we didn't apply a tmpfiles.d/ rule and that actually might end up being problematic eventually. --- src/basic/fs-util.c | 19 ++++++++++++++++++- src/tmpfiles/tmpfiles.c | 7 ++++--- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 59383c52d2..ac97c803d0 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -659,6 +659,19 @@ static int log_unsafe_transition(int a, int b, const char *path, unsigned flags) n1, special_glyph(ARROW), n2, path); } +static int log_autofs_mount_point(int fd, const char *path, unsigned flags) { + _cleanup_free_ char *n1 = NULL; + + if (!FLAGS_SET(flags, CHASE_WARN)) + return -EREMOTE; + + (void) fd_get_path(fd, &n1); + + return log_warning_errno(SYNTHETIC_ERRNO(EREMOTE), + "Detected autofs mount point %s during canonicalization of %s.", + n1, path); +} + int chase_symlinks(const char *path, const char *original_root, unsigned flags, char **ret) { _cleanup_free_ char *buffer = NULL, *done = NULL, *root = NULL; _cleanup_close_ int fd = -1; @@ -723,6 +736,10 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, * unprivileged to privileged files or directories. In such cases the return value is -ENOLINK. If * CHASE_WARN is also set a warning describing the unsafe transition is emitted. * + * 5. With CHASE_NO_AUTOFS: in this case if an autofs mount point is encountered, the path normalization is + * aborted and -EREMOTE is returned. If CHASE_WARN is also set a warning showing the path of the mount point + * is emitted. + * * */ /* A root directory of "/" or "" is identical to none */ @@ -885,7 +902,7 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, if ((flags & CHASE_NO_AUTOFS) && fd_is_fs_type(child, AUTOFS_SUPER_MAGIC) > 0) - return -EREMOTE; + return log_autofs_mount_point(child, path, flags); if (S_ISLNK(st.st_mode) && !((flags & CHASE_NOFOLLOW) && isempty(todo))) { char *joined; diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index d4e4f0c535..810b03567e 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -2261,11 +2261,12 @@ static int process_item(Item *i, OperationMask operation) { i->done |= operation; - r = chase_symlinks(i->path, NULL, CHASE_NO_AUTOFS, NULL); + r = chase_symlinks(i->path, NULL, CHASE_NO_AUTOFS|CHASE_WARN, NULL); if (r == -EREMOTE) { - log_debug_errno(r, "Item '%s' is below autofs, skipping.", i->path); + log_notice_errno(r, "Skipping %s", i->path); return 0; - } else if (r < 0) + } + if (r < 0) log_debug_errno(r, "Failed to determine whether '%s' is below autofs, ignoring: %m", i->path); r = FLAGS_SET(operation, OPERATION_CREATE) ? create_item(i) : 0;