From: Mike Yuan Date: Wed, 21 Feb 2024 05:45:01 +0000 (+0800) Subject: fd-util: introduce fd_verify_safe_flags X-Git-Tag: v256-rc1~778^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=14f38d179db689116dbfd13ae3e62ad3bde04e8f;p=thirdparty%2Fsystemd.git fd-util: introduce fd_verify_safe_flags As per https://github.com/systemd/systemd/pull/31419#discussion_r1496921074 --- diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index fa78284100f..371547facb9 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 044811443b6..f549831090a 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 bfe23ceb126..a6f26fea66d 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 648d2254fd2..579a03811b9 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. */