Re: [PATCH 7/8] conf, passt, tap: Open socket and PID files before switching UID/GID

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6992 bytes --]

On Wed, May 22, 2024 at 10:59:10PM +0200, Stefano Brivio wrote:
> Otherwise, if the user runs us as root, and gives us paths that are
> only accessible by root, we'll fail to open them, which might in turn
> encourage users to change permissions or ownerships: definitely a bad
> idea in terms of security.
> 
> Reported-by: Minxi Hou <mhou@redhat.com>
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Looking at this I did notice a pre-existing, well, maybe not bug
exactly, but possibly surprising behaviour, which makes me a
bit more nervous now that we can invoke it as root.

tap_sock_unix_open() will silently truncate the given socket path to
the maximum length for a Unix socket.  Which means we could bind(),
but also unlink() a path that's not exactly the same as the one the
one the user requested.  I don't immediately see a way to exploit
that, but it's the sort of thing that makes me nervous.  I think we
should instead outright fail if the given socket path is too long.

> ---
>  conf.c  | 17 ++++++++++++++++-
>  passt.c | 10 ++++------
>  passt.h |  4 ++++
>  tap.c   |  7 +++----
>  tap.h   |  1 +
>  5 files changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 2e0d909..f62a5eb 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -38,6 +38,7 @@
>  #include "ip.h"
>  #include "passt.h"
>  #include "netlink.h"
> +#include "tap.h"
>  #include "udp.h"
>  #include "tcp.h"
>  #include "pasta.h"
> @@ -1093,7 +1094,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
>  		return;
>  
>  	/* ...otherwise use nobody:nobody */
> -	warn("Started as root. Changing to nobody...");
> +	warn("Started as root, will change to nobody.");
>  	{
>  #ifndef GLIBC_NO_STATIC_NSS
>  		const struct passwd *pw;
> @@ -1113,6 +1114,18 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
>  	}
>  }
>  
> +/**
> + * conf_open_files() - Open files as requested by configuration
> + * @c:		Execution context
> + */
> +static void conf_open_files(struct ctx *c)
> +{
> +	if (c->mode == MODE_PASST && c->fd_tap == -1)
> +		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
> +
> +	c->pidfile_fd = pidfile_open(c->pid_file);
> +}
> +
>  /**
>   * conf() - Process command-line arguments and set configuration
>   * @c:		Execution context
> @@ -1712,6 +1725,8 @@ void conf(struct ctx *c, int argc, char **argv)
>  	else if (optind != argc)
>  		die("Extra non-option argument: %s", argv[optind]);
>  
> +	conf_open_files(c);	/* Before any possible setuid() / setgid() */
> +
>  	isolate_user(uid, gid, !netns_only, userns, c->mode);
>  
>  	if (c->pasta_conf_ns)
> diff --git a/passt.c b/passt.c
> index e2446fc..a8c4cd3 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -199,9 +199,9 @@ void exit_handler(int signal)
>   */
>  int main(int argc, char **argv)
>  {
> -	int nfds, i, devnull_fd = -1, pidfile_fd;
>  	struct epoll_event events[EPOLL_EVENTS];
>  	char *log_name, argv0[PATH_MAX], *name;
> +	int nfds, i, devnull_fd = -1;
>  	struct ctx c = { 0 };
>  	struct rlimit limit;
>  	struct timespec now;
> @@ -211,7 +211,7 @@ int main(int argc, char **argv)
>  
>  	isolate_initial();
>  
> -	c.pasta_netns_fd = c.fd_tap = -1;
> +	c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1;
>  
>  	sigemptyset(&sa.sa_mask);
>  	sa.sa_flags = 0;
> @@ -299,8 +299,6 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> -	pidfile_fd = pidfile_open(c.pid_file);
> -
>  	if (isolate_prefork(&c))
>  		die("Failed to sandbox process, exiting");
>  
> @@ -308,9 +306,9 @@ int main(int argc, char **argv)
>  		__openlog(log_name, 0, LOG_DAEMON);
>  
>  	if (!c.foreground)
> -		__daemon(pidfile_fd, devnull_fd);
> +		__daemon(c.pidfile_fd, devnull_fd);
>  	else
> -		pidfile_write(pidfile_fd, getpid());
> +		pidfile_write(c.pidfile_fd, getpid());
>  
>  	if (pasta_child_pid)
>  		kill(pasta_child_pid, SIGUSR1);
> diff --git a/passt.h b/passt.h
> index bc58d64..3e50612 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -185,6 +185,7 @@ struct ip6_ctx {
>   * @sock_path:		Path for UNIX domain socket
>   * @pcap:		Path for packet capture file
>   * @pid_file:		Path to PID file, empty string if not configured
> + * @pidfile_fd:		File descriptor for PID file, -1 if none
>   * @pasta_netns_fd:	File descriptor for network namespace in pasta mode
>   * @no_netns_quit:	In pasta mode, don't exit if fs-bound namespace is gone
>   * @netns_base:		Base name for fs-bound namespace, if any, in pasta mode
> @@ -234,7 +235,10 @@ struct ctx {
>  	int nofile;
>  	char sock_path[UNIX_PATH_MAX];
>  	char pcap[PATH_MAX];
> +
>  	char pid_file[PATH_MAX];
> +	int pidfile_fd;
> +
>  	int one_off;
>  
>  	int pasta_netns_fd;
> diff --git a/tap.c b/tap.c
> index c9f580e..2ea0849 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1100,7 +1100,7 @@ restart:
>   *
>   * Return: socket descriptor on success, won't return on failure
>   */
> -static int tap_sock_unix_open(char *sock_path)
> +int tap_sock_unix_open(char *sock_path)
>  {
>  	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
>  	struct sockaddr_un addr = {
> @@ -1144,7 +1144,7 @@ static int tap_sock_unix_open(char *sock_path)
>  	if (i == UNIX_SOCK_MAX)
>  		die("UNIX socket bind: %s", strerror(errno));
>  
> -	info("UNIX domain socket bound at %s\n", addr.sun_path);
> +	info("UNIX domain socket bound at %s", addr.sun_path);
>  	if (!*sock_path)
>  		memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX);
>  
> @@ -1167,7 +1167,7 @@ static void tap_sock_unix_init(struct ctx *c)
>  	ev.data.u64 = ref.u64;
>  	epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev);
>  
> -	info("You can now start qemu (>= 7.2, with commit 13c6be96618c):");
> +	info("\nYou can now start qemu (>= 7.2, with commit 13c6be96618c):");
>  	info("    kvm ... -device virtio-net-pci,netdev=s -netdev stream,id=s,server=off,addr.type=unix,addr.path=%s",
>  	     c->sock_path);
>  	info("or qrap, for earlier qemu versions:");
> @@ -1318,7 +1318,6 @@ void tap_sock_init(struct ctx *c)
>  	}
>  
>  	if (c->mode == MODE_PASST) {
> -		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
>  		tap_sock_unix_init(c);
>  
>  		/* In passt mode, we don't know the guest's MAC address until it
> diff --git a/tap.h b/tap.h
> index d146d2f..2285a87 100644
> --- a/tap.h
> +++ b/tap.h
> @@ -68,6 +68,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t events,
>  		       const struct timespec *now);
>  void tap_handler_passt(struct ctx *c, uint32_t events,
>  		       const struct timespec *now);
> +int tap_sock_unix_open(char *sock_path);
>  void tap_sock_init(struct ctx *c);
>  
>  #endif /* TAP_H */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]