conf, passt, tap: Open socket and PID files before switching UID/GID

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>
Acked-by: Richard W.M. Jones <rjones@redhat.com>

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;
@@ -1114,6 +1115,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
  * @argc:	Argument count
@@ -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 */