From: Adrian Vovk <adrianvovk@gmail.com>
Date: Tue, 20 Feb 2024 19:54:21 +0000 (-0500)
Subject: fd-util: Add helpers to check if FD flags are safe
X-Git-Tag: v256-rc1~795^2
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F31419%2Fhead;p=thirdparty%2Fsystemd.git

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
---

diff --git a/src/basic/fd-util.h b/src/basic/fd-util.h
index 3691f33ffdb..044811443b6 100644
--- a/src/basic/fd-util.h
+++ b/src/basic/fd-util.h
@@ -8,6 +8,7 @@
 #include <sys/socket.h>
 
 #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 bc6e8e37c5b..bfe23ceb126 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 75d9082abfc..648d2254fd2 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;
         }