From 9e3db91f2f7663860657afbf2076ecebeeb26c93 Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Tue, 20 Feb 2024 14:24:01 -0500 Subject: [PATCH 1/2] missing_fcntl: Fix RAW_O_LARGEFILE This value is actually arch-specific, so this commit defines it for all the arches that set it to some custom value Fixes https://github.com/systemd/systemd/issues/31417 --- src/basic/missing_fcntl.h | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/basic/missing_fcntl.h b/src/basic/missing_fcntl.h index 24b2dc3119..3c85befda9 100644 --- a/src/basic/missing_fcntl.h +++ b/src/basic/missing_fcntl.h @@ -69,9 +69,26 @@ /* So O_LARGEFILE is generally implied by glibc, and defined to zero hence, because we only build in LFS * mode. However, when invoking fcntl(F_GETFL) the flag is ORed into the result anyway — glibc does not mask - * it away. Which sucks. Let's define the actual value here, so that we can mask it ourselves. */ + * it away. Which sucks. Let's define the actual value here, so that we can mask it ourselves. + * + * The precise definition is arch specific, so we use the values defined in the kernel (note that some + * are hexa and others are octal; duplicated as-is from the kernel definitions): + * - alpha, arm, arm64, m68k, mips, parisc, powerpc, sparc: each has a specific value; + * - others: they use the "generic" value (defined in include/uapi/asm-generic/fcntl.h) */ #if O_LARGEFILE != 0 #define RAW_O_LARGEFILE O_LARGEFILE #else -#define RAW_O_LARGEFILE 0100000 +#if defined(__alpha__) || defined(__arm__) || defined(__aarch64__) || defined(__m68k__) +#define RAW_O_LARGEFILE 0400000 +#elif defined(__mips__) +#define RAW_O_LARGEFILE 0x2000 +#elif defined(__parisc__) || defined(__hppa__) +#define RAW_O_LARGEFILE 000004000 +#elif defined(__powerpc__) +#define RAW_O_LARGEFILE 0200000 +#elif defined(__sparc__) +#define RAW_O_LARGEFILE 0x40000 +#else +#define RAW_O_LARGEFILE 00100000 +#endif #endif From e4d0606c2b44079d885ce03522e558964769f143 Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Tue, 20 Feb 2024 14:54:21 -0500 Subject: [PATCH 2/2] fd-util: Add helpers to check if FD flags are safe Adds a SAFE_FD_FLAGS define to list out all the safe FD flags, and also an UNSAFE_FD_FLAGS() macro to strip out the safe flags and leave only the unsafe flags. This can be used to quickly check if any unsafe flags are set and print them for diagnostic purposes --- src/basic/fd-util.h | 15 +++++++++++++++ src/home/homed-bus.c | 7 ++++--- src/journal/journald-native.c | 7 ++++--- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/basic/fd-util.h b/src/basic/fd-util.h index 3691f33ffd..044811443b 100644 --- a/src/basic/fd-util.h +++ b/src/basic/fd-util.h @@ -8,6 +8,7 @@ #include #include "macro.h" +#include "missing_fcntl.h" #include "stdio-util.h" /* maximum length of fdname */ @@ -21,6 +22,20 @@ #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]); diff --git a/src/home/homed-bus.c b/src/home/homed-bus.c index bc6e8e37c5..bfe23ceb12 100644 --- a/src/home/homed-bus.c +++ b/src/home/homed-bus.c @@ -3,7 +3,6 @@ #include "fd-util.h" #include "home-util.h" #include "homed-bus.h" -#include "missing_fcntl.h" #include "stat-util.h" #include "strv.h" @@ -120,8 +119,10 @@ int bus_message_read_blobs(sd_bus_message *m, Hashmap **ret, sd_bus_error *error /* 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 ((flags & ~(O_ACCMODE|RAW_O_LARGEFILE)) != 0) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "FD for %s has unexpected flags set", filename); + if (UNSAFE_FD_FLAGS(flags) != 0) + 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)); 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 75d9082abf..648d2254fd 100644 --- a/src/journal/journald-native.c +++ b/src/journal/journald-native.c @@ -22,7 +22,6 @@ #include "journald-wall.h" #include "memfd-util.h" #include "memory-util.h" -#include "missing_fcntl.h" #include "parse-util.h" #include "path-util.h" #include "process-util.h" @@ -363,8 +362,10 @@ void server_process_native_file( return; } - if ((flags & ~(O_ACCMODE|RAW_O_LARGEFILE)) != 0) { - log_ratelimit_error(JOURNAL_LOG_RATELIMIT, "Unexpected flags of passed memory fd, ignoring message: %m"); + 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; }