From 14f38d179db689116dbfd13ae3e62ad3bde04e8f Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 21 Feb 2024 13:45:01 +0800 Subject: [PATCH 1/2] fd-util: introduce fd_verify_safe_flags As per https://github.com/systemd/systemd/pull/31419#discussion_r1496921074 --- src/basic/fd-util.c | 30 ++++++++++++++++++++++++++++++ src/basic/fd-util.h | 17 +++-------------- src/home/homed-bus.c | 18 +++++++----------- src/journal/journald-native.c | 19 +++++++------------ 4 files changed, 47 insertions(+), 37 deletions(-) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index fa78284100..371547facb 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -913,6 +913,36 @@ int fd_is_opath(int fd) { return FLAGS_SET(r, O_PATH); } +int fd_verify_safe_flags(int fd) { + int flags, unexpected_flags; + + /* Check if an extrinsic fd is safe to work on (by a privileged service). This ensures that clients + * can't trick a privileged service into giving access to a file the client doesn't already have + * access to (especially via something like O_PATH). + * + * O_NOFOLLOW: For some reason the kernel will return this flag from fcntl; it doesn't go away + * immediately after open(). It should have no effect whatsoever to an already-opened FD, + * and since we refuse O_PATH it should be safe. + * + * RAW_O_LARGEFILE: glibc secretly sets this and neglects to hide it from us if we call fcntl. + * See comment in missing_fcntl.h for more details about this. + */ + + assert(fd >= 0); + + flags = fcntl(fd, F_GETFL); + if (flags < 0) + return -errno; + + unexpected_flags = flags & ~(O_ACCMODE|O_NOFOLLOW|RAW_O_LARGEFILE); + if (unexpected_flags != 0) + return log_debug_errno(SYNTHETIC_ERRNO(EREMOTEIO), + "Unexpected flags set for extrinsic fd: 0%o", + (unsigned) unexpected_flags); + + return 0; +} + int read_nr_open(void) { _cleanup_free_ char *nr_open = NULL; int r; diff --git a/src/basic/fd-util.h b/src/basic/fd-util.h index 044811443b..f549831090 100644 --- a/src/basic/fd-util.h +++ b/src/basic/fd-util.h @@ -22,20 +22,6 @@ #define EBADF_PAIR { -EBADF, -EBADF } #define EBADF_TRIPLET { -EBADF, -EBADF, -EBADF } -/* Flags that are safe to have set on an FD given to a privileged service to operate on. - * This ensures that clients can't trick a privileged service into giving access to a file the client - * doesn't already have access to (especially via something like O_PATH). - * - * O_NOFOLLOW: For some reason the kernel will return this flag from fcntl; it doesn't go away immediately - * after open(). It should have no effect whatsoever to an already-opened FD, but if it does - * it's decreasing the risk to a privileged service since it disables symlink following. - * - * RAW_O_LARGEFILE: glibc secretly sets this and neglects to hide it from us if we call fcntl. See comment - * in missing_fcntl.h for more details about this. - */ -#define SAFE_FD_FLAGS (O_ACCMODE|O_NOFOLLOW|RAW_O_LARGEFILE) -#define UNSAFE_FD_FLAGS(flags) ((unsigned)(flags) & ~SAFE_FD_FLAGS) - int close_nointr(int fd); int safe_close(int fd); void safe_close_pair(int p[static 2]); @@ -126,7 +112,10 @@ static inline int make_null_stdio(void) { int fd_reopen(int fd, int flags); int fd_reopen_condition(int fd, int flags, int mask, int *ret_new_fd); + int fd_is_opath(int fd); +int fd_verify_safe_flags(int fd); + int read_nr_open(void); int fd_get_diskseq(int fd, uint64_t *ret); diff --git a/src/home/homed-bus.c b/src/home/homed-bus.c index bfe23ceb12..a6f26fea66 100644 --- a/src/home/homed-bus.c +++ b/src/home/homed-bus.c @@ -89,7 +89,7 @@ int bus_message_read_blobs(sd_bus_message *m, Hashmap **ret, sd_bus_error *error _cleanup_free_ char *filename = NULL; _cleanup_close_ int fd = -EBADF; const char *_filename = NULL; - int _fd, flags; + int _fd; r = sd_bus_message_read(m, "{sh}", &_filename, &_fd); if (r < 0) @@ -111,18 +111,14 @@ int bus_message_read_blobs(sd_bus_message *m, Hashmap **ret, sd_bus_error *error r = fd_verify_regular(fd); if (r < 0) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "FD for %s is not a regular file", filename); + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "FD for '%s' is not a regular file", filename); - flags = fcntl(fd, F_GETFL); - if (flags < 0) - return -errno; - - /* Refuse fds w/ unexpected flags set. In particular, we don't want to permit O_PATH FDs, since - * those don't actually guarantee that the client has access to the file. */ - if (UNSAFE_FD_FLAGS(flags) != 0) + r = fd_verify_safe_flags(fd); + if (r == -EREMOTEIO) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, - "FD for %s has unexpected flags set: 0%o", - filename, UNSAFE_FD_FLAGS(flags)); + "FD for '%s' has unexpected flags set", filename); + if (r < 0) + return r; r = hashmap_put(blobs, filename, FD_TO_PTR(fd)); if (r < 0) diff --git a/src/journal/journald-native.c b/src/journal/journald-native.c index 648d2254fd..579a03811b 100644 --- a/src/journal/journald-native.c +++ b/src/journal/journald-native.c @@ -356,18 +356,13 @@ void server_process_native_file( if (st.st_size <= 0) return; - int flags = fcntl(fd, F_GETFL); - if (flags < 0) { - log_ratelimit_error_errno(errno, JOURNAL_LOG_RATELIMIT, "Failed to get flags of passed file, ignoring: %m"); - return; - } - - if (UNSAFE_FD_FLAGS(flags) != 0) { - log_ratelimit_error(JOURNAL_LOG_RATELIMIT, - "Unexpected flags of passed memory fd (0%o), ignoring message: %m", - UNSAFE_FD_FLAGS(flags)); - return; - } + r = fd_verify_safe_flags(fd); + if (r == -EREMOTEIO) + return (void) log_ratelimit_error_errno(r, JOURNAL_LOG_RATELIMIT, + "Unexpected flags of passed memory fd, ignoring message."); + if (r < 0) + return (void) log_ratelimit_error_errno(r, JOURNAL_LOG_RATELIMIT, + "Failed to get flags of passed file: %m"); /* If it's a memfd, check if it is sealed. If so, we can just mmap it and use it, and do not need to * copy the data out. */ From f5cbe313743c7f4051233d23fdc739698fc4875e Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 21 Feb 2024 14:03:55 +0800 Subject: [PATCH 2/2] journal-native: ignore server_process_native_file error on caller's side Also, stop saying ", ignoring". It is unclear whether the message or the error is ignored. "ignoring message" or "refusing" is OK. --- src/journal/fuzz-journald-native-fd.c | 4 +- src/journal/journald-native.c | 154 ++++++++++++-------------- src/journal/journald-native.h | 2 +- src/journal/journald-server.c | 2 +- 4 files changed, 72 insertions(+), 90 deletions(-) diff --git a/src/journal/fuzz-journald-native-fd.c b/src/journal/fuzz-journald-native-fd.c index 26395dfbcb..07a8eb55e1 100644 --- a/src/journal/fuzz-journald-native-fd.c +++ b/src/journal/fuzz-journald-native-fd.c @@ -30,13 +30,13 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { .uid = geteuid(), .gid = getegid(), }; - server_process_native_file(s, sealed_fd, &ucred, tv, label, label_len); + (void) server_process_native_file(s, sealed_fd, &ucred, tv, label, label_len); unsealed_fd = mkostemp_safe(name); assert_se(unsealed_fd >= 0); assert_se(write(unsealed_fd, data, size) == (ssize_t) size); assert_se(lseek(unsealed_fd, 0, SEEK_SET) == 0); - server_process_native_file(s, unsealed_fd, &ucred, tv, label, label_len); + (void) server_process_native_file(s, unsealed_fd, &ucred, tv, label, label_len); return 0; } diff --git a/src/journal/journald-native.c b/src/journal/journald-native.c index 579a03811b..d6bceb0e4a 100644 --- a/src/journal/journald-native.c +++ b/src/journal/journald-native.c @@ -326,7 +326,7 @@ void server_process_native_message( } while (r == 0); } -void server_process_native_file( +int server_process_native_file( Server *s, int fd, const struct ucred *ucred, @@ -342,27 +342,25 @@ void server_process_native_file( assert(s); assert(fd >= 0); - if (fstat(fd, &st) < 0) { - log_ratelimit_error_errno(errno, JOURNAL_LOG_RATELIMIT, "Failed to stat passed file, ignoring: %m"); - return; - } + if (fstat(fd, &st) < 0) + return log_ratelimit_error_errno(errno, JOURNAL_LOG_RATELIMIT, + "Failed to stat passed file: %m"); r = stat_verify_regular(&st); - if (r < 0) { - log_ratelimit_error_errno(r, JOURNAL_LOG_RATELIMIT, "File passed is not regular, ignoring: %m"); - return; - } + if (r < 0) + return log_ratelimit_error_errno(r, JOURNAL_LOG_RATELIMIT, + "File passed is not regular, ignoring message: %m"); if (st.st_size <= 0) - return; + return 0; r = fd_verify_safe_flags(fd); if (r == -EREMOTEIO) - return (void) log_ratelimit_error_errno(r, JOURNAL_LOG_RATELIMIT, - "Unexpected flags of passed memory fd, ignoring message."); + return log_ratelimit_error_errno(r, JOURNAL_LOG_RATELIMIT, + "Unexpected flags of passed memory fd, ignoring message."); if (r < 0) - return (void) log_ratelimit_error_errno(r, JOURNAL_LOG_RATELIMIT, - "Failed to get flags of passed file: %m"); + return log_ratelimit_error_errno(r, JOURNAL_LOG_RATELIMIT, + "Failed to get flags of passed file: %m"); /* If it's a memfd, check if it is sealed. If so, we can just mmap it and use it, and do not need to * copy the data out. */ @@ -376,34 +374,26 @@ void server_process_native_file( * path. */ r = fd_get_path(fd, &k); - if (r < 0) { - log_ratelimit_error_errno(r, JOURNAL_LOG_RATELIMIT, - "readlink(/proc/self/fd/%i) failed: %m", fd); - return; - } + if (r < 0) + return log_ratelimit_error_errno(r, JOURNAL_LOG_RATELIMIT, + "Failed to get path of passed fd: %m"); e = PATH_STARTSWITH_SET(k, "/dev/shm/", "/tmp/", "/var/tmp/"); - if (!e) { - log_ratelimit_error(JOURNAL_LOG_RATELIMIT, - "Received file outside of allowed directories. Refusing."); - return; - } + if (!e) + return log_ratelimit_error_errno(SYNTHETIC_ERRNO(EPERM), JOURNAL_LOG_RATELIMIT, + "Received file outside of allowed directories, refusing."); - if (!filename_is_valid(e)) { - log_ratelimit_error(JOURNAL_LOG_RATELIMIT, - "Received file in subdirectory of allowed directories. Refusing."); - return; - } + if (!filename_is_valid(e)) + return log_ratelimit_error_errno(SYNTHETIC_ERRNO(EPERM), JOURNAL_LOG_RATELIMIT, + "Received file in subdirectory of allowed directories, refusing."); } /* When !sealed, set a lower memory limit. We have to read the file, effectively doubling memory * use. */ - if (st.st_size > ENTRY_SIZE_MAX / (sealed ? 1 : 2)) { - log_ratelimit_error(JOURNAL_LOG_RATELIMIT, - "File passed too large (%"PRIu64" bytes). Ignoring.", - (uint64_t) st.st_size); - return; - } + if (st.st_size > ENTRY_SIZE_MAX / (sealed ? 1 : 2)) + return log_ratelimit_error_errno(SYNTHETIC_ERRNO(EFBIG), JOURNAL_LOG_RATELIMIT, + "File passed too large (%"PRIu64" bytes), refusing.", + (uint64_t) st.st_size); if (sealed) { void *p; @@ -414,62 +404,54 @@ void server_process_native_file( ps = PAGE_ALIGN(st.st_size); assert(ps < SIZE_MAX); p = mmap(NULL, ps, PROT_READ, MAP_PRIVATE, fd, 0); - if (p == MAP_FAILED) { - log_ratelimit_error_errno(errno, JOURNAL_LOG_RATELIMIT, - "Failed to map memfd, ignoring: %m"); - return; - } + if (p == MAP_FAILED) + return log_ratelimit_error_errno(errno, JOURNAL_LOG_RATELIMIT, + "Failed to map memfd: %m"); server_process_native_message(s, p, st.st_size, ucred, tv, label, label_len); assert_se(munmap(p, ps) >= 0); - } else { - _cleanup_free_ void *p = NULL; - struct statvfs vfs; - ssize_t n; - if (fstatvfs(fd, &vfs) < 0) { - log_ratelimit_error_errno(errno, JOURNAL_LOG_RATELIMIT, - "Failed to stat file system of passed file, not processing it: %m"); - return; - } - - /* Refuse operating on file systems that have mandatory locking enabled, see: - * - * https://github.com/systemd/systemd/issues/1822 - */ - if (vfs.f_flag & ST_MANDLOCK) { - log_ratelimit_error(JOURNAL_LOG_RATELIMIT, - "Received file descriptor from file system with mandatory locking enabled, not processing it."); - return; - } - - /* Make the fd non-blocking. On regular files this has the effect of bypassing mandatory - * locking. Of course, this should normally not be necessary given the check above, but let's - * better be safe than sorry, after all NFS is pretty confusing regarding file system flags, - * and we better don't trust it, and so is SMB. */ - r = fd_nonblock(fd, true); - if (r < 0) { - log_ratelimit_error_errno(r, JOURNAL_LOG_RATELIMIT, - "Failed to make fd non-blocking, not processing it: %m"); - return; - } - - /* The file is not sealed, we can't map the file here, since clients might then truncate it - * and trigger a SIGBUS for us. So let's stupidly read it. */ - - p = malloc(st.st_size); - if (!p) { - log_oom(); - return; - } - - n = pread(fd, p, st.st_size, 0); - if (n < 0) - log_ratelimit_error_errno(errno, JOURNAL_LOG_RATELIMIT, - "Failed to read file, ignoring: %m"); - else if (n > 0) - server_process_native_message(s, p, n, ucred, tv, label, label_len); + return 0; } + + _cleanup_free_ void *p = NULL; + struct statvfs vfs; + ssize_t n; + + if (fstatvfs(fd, &vfs) < 0) + return log_ratelimit_error_errno(errno, JOURNAL_LOG_RATELIMIT, + "Failed to stat file system of passed file: %m"); + + /* Refuse operating on file systems that have mandatory locking enabled. + * See also: https://github.com/systemd/systemd/issues/1822 */ + if (FLAGS_SET(vfs.f_flag, ST_MANDLOCK)) + return log_ratelimit_error_errno(SYNTHETIC_ERRNO(EPERM), JOURNAL_LOG_RATELIMIT, + "Received file descriptor from file system with mandatory locking enabled, not processing it."); + + /* Make the fd non-blocking. On regular files this has the effect of bypassing mandatory + * locking. Of course, this should normally not be necessary given the check above, but let's + * better be safe than sorry, after all NFS is pretty confusing regarding file system flags, + * and we better don't trust it, and so is SMB. */ + r = fd_nonblock(fd, true); + if (r < 0) + return log_ratelimit_error_errno(r, JOURNAL_LOG_RATELIMIT, + "Failed to make fd non-blocking: %m"); + + /* The file is not sealed, we can't map the file here, since clients might then truncate it + * and trigger a SIGBUS for us. So let's stupidly read it. */ + + p = malloc(st.st_size); + if (!p) + return log_oom(); + + n = pread(fd, p, st.st_size, 0); + if (n < 0) + return log_ratelimit_error_errno(errno, JOURNAL_LOG_RATELIMIT, + "Failed to read file: %m"); + if (n > 0) + server_process_native_message(s, p, n, ucred, tv, label, label_len); + + return 0; } int server_open_native_socket(Server *s, const char *native_socket) { diff --git a/src/journal/journald-native.h b/src/journal/journald-native.h index 7bbaaed181..10db267966 100644 --- a/src/journal/journald-native.h +++ b/src/journal/journald-native.h @@ -12,7 +12,7 @@ void server_process_native_message( const char *label, size_t label_len); -void server_process_native_file( +int server_process_native_file( Server *s, int fd, const struct ucred *ucred, diff --git a/src/journal/journald-server.c b/src/journal/journald-server.c index a8c186dc20..2c6f6e301d 100644 --- a/src/journal/journald-server.c +++ b/src/journal/journald-server.c @@ -1538,7 +1538,7 @@ int server_process_datagram( if (n > 0 && n_fds == 0) server_process_native_message(s, s->buffer, n, ucred, tv, label, label_len); else if (n == 0 && n_fds == 1) - server_process_native_file(s, fds[0], ucred, tv, label, label_len); + (void) server_process_native_file(s, fds[0], ucred, tv, label, label_len); else if (n_fds > 0) log_ratelimit_warning(JOURNAL_LOG_RATELIMIT, "Got too many file descriptors via native socket. Ignoring.");