sd-varlink: hook up fd passing control with SO_PASSRIGHTS

This is a tricky one, because we effectively turn fd passing input
toggle into a tristate: unset, disabled, and enabled; whereas unset
and disabled were identical previously. *Unset* state silently
ignores SCM_RIGHTS passed by invoking recv() instead of recvmsg(),
and for disabled we now disable SO_PASSRIGHTS completely.

The plot thickens when it comes to the server, since we want to
turn off the SO_PASSRIGHTS already on the listening socket so that
there's no race between accept() and recvmsg() wrt SO_PASSRIGHTS state.
However, if we do this unconditionally, the existing use case of
creating a custom connection callback and enabling fd passing there
would be broken.

Hence, let's introduce a new flag,
SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT, which when set
ties the enablement of fd passing to SO_PASSRIGHTS in server,
and set it for all our varlink servers.

Refer to the previous commit for the rationale behind return value
change in sd_varlink_set_allow_fd_passing_input().

diff --git a/src/libsystemd/sd-varlink/sd-varlink.c b/src/libsystemd/sd-varlink/sd-varlink.c
index bf764bb97f1652d0aacd496bdfef58fc17f1eff2..1e70ce41a7c75d136f930effb80f6d205b7d5e20 100644 (file)
--- a/src/libsystemd/sd-varlink/sd-varlink.c
+++ b/src/libsystemd/sd-varlink/sd-varlink.c
@@ -139,12 +139,14 @@ static int varlink_new(sd_varlink **ret) {
 
                 .ucred = UCRED_INVALID,
 
+                .peer_pidfd = -EBADF,
+
                 .timestamp = USEC_INFINITY,
                 .timeout = VARLINK_DEFAULT_TIMEOUT_USEC,
 
-                .af = -1,
+                .allow_fd_passing_input = -1,
 
-                .peer_pidfd = -EBADF,
+                .af = -1,
         };
 
         *ret = v;
@@ -849,7 +851,7 @@ static int varlink_read(sd_varlink *v) {
         p = v->input_buffer + v->input_buffer_index + v->input_buffer_size;
         rs = MALLOC_SIZEOF_SAFE(v->input_buffer) - (v->input_buffer_index + v->input_buffer_size);
 
-        if (v->allow_fd_passing_input) {
+        if (v->allow_fd_passing_input > 0) {
                 iov = IOVEC_MAKE(p, rs);
 
                 /* Allocate the fd buffer on the heap, since we need a lot of space potentially */
@@ -893,14 +895,14 @@ static int varlink_read(sd_varlink *v) {
                 return n;
         if (n == 0) { /* EOF */
 
-                if (v->allow_fd_passing_input)
+                if (v->allow_fd_passing_input > 0)
                         cmsg_close_all(&mh);
 
                 v->read_disconnected = true;
                 return 1;
         }
 
-        if (v->allow_fd_passing_input) {
+        if (v->allow_fd_passing_input > 0) {
                 struct cmsghdr *cmsg;
 
                 cmsg = cmsg_find(&mh, SOL_SOCKET, SCM_RIGHTS, (socklen_t) -1);
@@ -3159,7 +3161,7 @@ _public_ int sd_varlink_peek_fd(sd_varlink *v, size_t i) {
         /* Returns one of the file descriptors that were received along with the current message. This does
          * not duplicate the fd nor invalidate it, it hence remains in our possession. */
 
-        if (!v->allow_fd_passing_input)
+        if (v->allow_fd_passing_input <= 0)
                 return -EPERM;
 
         if (i >= v->n_input_fds)
@@ -3185,7 +3187,7 @@ _public_ int sd_varlink_take_fd(sd_varlink *v, size_t i) {
          * we'll invalidate the reference to it under our possession. If called twice in a row will return
          * -EBADF */
 
-        if (!v->allow_fd_passing_input)
+        if (v->allow_fd_passing_input <= 0)
                 return -EPERM;
 
         if (i >= v->n_input_fds)
@@ -3197,7 +3199,7 @@ _public_ int sd_varlink_take_fd(sd_varlink *v, size_t i) {
 _public_ int sd_varlink_get_n_fds(sd_varlink *v) {
         assert_return(v, -EINVAL);
 
-        if (!v->allow_fd_passing_input)
+        if (v->allow_fd_passing_input <= 0)
                 return -EPERM;
 
         return (int) v->n_input_fds;
@@ -3247,20 +3249,29 @@ _public_ int sd_varlink_set_allow_fd_passing_input(sd_varlink *v, int b) {
 
         assert_return(v, -EINVAL);
 
-        if (v->allow_fd_passing_input == !!b)
+        if (v->allow_fd_passing_input >= 0 && (v->allow_fd_passing_input > 0) == !!b)
                 return 0;
 
-        if (!b) {
-                v->allow_fd_passing_input = false;
-                return 1;
-        }
-
         r = verify_unix_socket(v);
-        if (r < 0)
+        if (r < 0) {
+                assert(v->allow_fd_passing_input <= 0);
+
+                if (!b) {
+                        v->allow_fd_passing_input = false;
+                        return 0;
+                }
+
                 return r;
+        }
 
-        v->allow_fd_passing_input = true;
-        return 0;
+        if (!v->server || FLAGS_SET(v->server->flags, SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT)) {
+                r = setsockopt_int(v->input_fd, SOL_SOCKET, SO_PASSRIGHTS, !!b);
+                if (r < 0 && !ERRNO_IS_NEG_NOT_SUPPORTED(r))
+                        log_debug_errno(r, "Failed to set SO_PASSRIGHTS socket option: %m");
+        }
+
+        v->allow_fd_passing_input = !!b;
+        return 1;
 }
 
 _public_ int sd_varlink_set_allow_fd_passing_output(sd_varlink *v, int b) {
@@ -3297,7 +3308,8 @@ _public_ int sd_varlink_server_new(sd_varlink_server **ret, sd_varlink_server_fl
                                  SD_VARLINK_SERVER_INHERIT_USERDATA|
                                  SD_VARLINK_SERVER_INPUT_SENSITIVE|
                                  SD_VARLINK_SERVER_ALLOW_FD_PASSING_INPUT|
-                                 SD_VARLINK_SERVER_ALLOW_FD_PASSING_OUTPUT)) == 0, -EINVAL);
+                                 SD_VARLINK_SERVER_ALLOW_FD_PASSING_OUTPUT|
+                                 SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT)) == 0, -EINVAL);
 
         s = new(sd_varlink_server, 1);
         if (!s)
@@ -3493,6 +3505,12 @@ _public_ int sd_varlink_server_add_connection_pair(
         if (r < 0)
                 return r;
 
+        /* Link up the server and the connection, and take reference in both directions. Note that the
+         * reference on the connection is left dangling. It will be dropped when the connection is closed,
+         * which happens in varlink_close(), including in the event loop quit callback. */
+        v->server = sd_varlink_server_ref(server);
+        sd_varlink_ref(v);
+
         v->input_fd = input_fd;
         v->output_fd = output_fd;
         if (server->flags & SD_VARLINK_SERVER_INHERIT_USERDATA)
@@ -3510,12 +3528,6 @@ _public_ int sd_varlink_server_add_connection_pair(
         (void) sd_varlink_set_allow_fd_passing_input(v, FLAGS_SET(server->flags, SD_VARLINK_SERVER_ALLOW_FD_PASSING_INPUT));
         (void) sd_varlink_set_allow_fd_passing_output(v, FLAGS_SET(server->flags, SD_VARLINK_SERVER_ALLOW_FD_PASSING_OUTPUT));
 
-        /* Link up the server and the connection, and take reference in both directions. Note that the
-         * reference on the connection is left dangling. It will be dropped when the connection is closed,
-         * which happens in varlink_close(), including in the event loop quit callback. */
-        v->server = sd_varlink_server_ref(server);
-        sd_varlink_ref(v);
-
         varlink_set_state(v, VARLINK_IDLE_SERVER);
 
         if (server->event) {
@@ -3632,6 +3644,13 @@ _public_ int sd_varlink_server_listen_fd(sd_varlink_server *s, int fd) {
         if (r < 0)
                 return r;
 
+        /* If fd passing is disabled on server, and SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT flag is set,
+         * turn off SO_PASSRIGHTS immediately on listening socket. The conditionalization behind a flag
+         * is needed to retain backwards compat, where implementations would register a connection callback
+         * to enable fd passing after accept(), which might race with clients wrt SO_PASSRIGHTS state. */
+        if (FLAGS_SET(s->flags, SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT))
+                (void) setsockopt_int(fd, SOL_SOCKET, SO_PASSRIGHTS, FLAGS_SET(s->flags, SD_VARLINK_SERVER_ALLOW_FD_PASSING_INPUT));
+
         r = varlink_server_create_listen_fd_socket(s, fd, &ss);
         if (r < 0)
                 return r;
@@ -3673,6 +3692,10 @@ _public_ int sd_varlink_server_listen_address(sd_varlink_server *s, const char *
 
         fd = fd_move_above_stdio(fd);
 
+        /* See the comment in sd_varlink_server_listen_fd() */
+        if (FLAGS_SET(s->flags, SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT))
+                (void) setsockopt_int(fd, SOL_SOCKET, SO_PASSRIGHTS, FLAGS_SET(s->flags, SD_VARLINK_SERVER_ALLOW_FD_PASSING_INPUT));
+
         (void) sockaddr_un_unlink(&sockaddr.un);
 
         WITH_UMASK(~m & 0777)

diff --git a/src/libsystemd/sd-varlink/varlink-internal.h b/src/libsystemd/sd-varlink/varlink-internal.h
index a163f81061d707427442668d38bdf059c385a978..ef848243738c7ee36029ba8190acf42e89533ff1 100644 (file)
--- a/src/libsystemd/sd-varlink/varlink-internal.h
+++ b/src/libsystemd/sd-varlink/varlink-internal.h
@@ -166,12 +166,12 @@ struct sd_varlink {
         bool prefer_write:1;
         bool got_pollhup:1;
 
-        bool allow_fd_passing_input:1;
-        bool allow_fd_passing_output:1;
-
         bool output_buffer_sensitive:1; /* whether to erase the output buffer after writing it to the socket */
         bool input_sensitive:1; /* Whether incoming messages might be sensitive */
 
+        bool allow_fd_passing_output;
+        int allow_fd_passing_input;
+
         int af; /* address family if socket; AF_UNSPEC if not socket; negative if not known */
 
         usec_t timestamp;

diff --git a/src/libsystemd/sd-varlink/varlink-util.c b/src/libsystemd/sd-varlink/varlink-util.c
index cc18656d13815dd16861606024be7cb4a0314ad6..be7df731437510937fc08d8cb0fc2400b13bfbeb 100644 (file)
--- a/src/libsystemd/sd-varlink/varlink-util.c
+++ b/src/libsystemd/sd-varlink/varlink-util.c
@@ -174,7 +174,7 @@ int varlink_server_new(
         _cleanup_(sd_varlink_server_unrefp) sd_varlink_server *s = NULL;
         int r;
 
-        r = sd_varlink_server_new(&s, flags);
+        r = sd_varlink_server_new(&s, flags|SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT);
         if (r < 0)
                 return log_debug_errno(r, "Failed to allocate varlink server object: %m");
 

diff --git a/src/systemd/sd-varlink.h b/src/systemd/sd-varlink.h
index 67d9cc3d9487e30841312d7cd1acb145127663ff..9d3dc48a44476341037c1118c1b2e449915521b4 100644 (file)
--- a/src/systemd/sd-varlink.h
+++ b/src/systemd/sd-varlink.h
@@ -68,6 +68,7 @@ __extension__ typedef enum _SD_ENUM_TYPE_S64(sd_varlink_server_flags_t) {
         SD_VARLINK_SERVER_INPUT_SENSITIVE         = 1 << 4, /* Automatically mark all connection input as sensitive */
         SD_VARLINK_SERVER_ALLOW_FD_PASSING_INPUT  = 1 << 5, /* Allow receiving fds over all connections */
         SD_VARLINK_SERVER_ALLOW_FD_PASSING_OUTPUT = 1 << 6, /* Allow sending fds over all connections */
+        SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT = 1 << 7, /* Reject input messages with fds if fd passing is disabled (needs kernel v6.16+) */
         _SD_ENUM_FORCE_S64(SD_VARLINK_SERVER)
 } sd_varlink_server_flags_t;