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

["Richard W.M. Jones" <rjones@redhat.com>]

On Wed, May 29, 2024 at 12:35:24PM +1000, David Gibson wrote:
> 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.

Yes, agreed.

It seems as if the latest passt code still does this.  Do you want me
to open a bug about it?

Rich.

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



-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit