[PATCH] conf: Follow changes to the host nameserver used for DNS forwarding

[PATCH] conf: Follow changes to the host nameserver used for DNS forwarding

[Aditya Merchant]

We read the host's /etc/resolv.conf once, at start, and forward queries
sent to the --dns-forward address to the first nameserver listed there.
If the host then joins another network, connects to a VPN, or pasta
starts before the network is configured, that nameserver goes stale and
name resolution in the namespace stops working until pasta restarts.
With rootless Podman, that means stopping every container using the
rootless network namespace.

In pasta mode, if we forward DNS queries for an IP version, and the host
nameserver for it comes from resolv.conf (no --dns-host or --dns), watch
resolv.conf with inotify and read it again as it changes, so that new
DNS flows go to its current first nameserver. Watch the directory, not
the file, as NetworkManager and others replace resolv.conf by renaming
a new file over it. If resolv.conf is a symbolic link, watch the
directory of its target.

Once sandboxed, we can't open files by path, and keeping a descriptor
for the directory would undo the filesystem isolation: lookups of ".."
from it reach the rest of the host filesystem. Instead, right after
isolate_prefork() unshares our mount namespace, and before mounts become
unbindable, take a detached copy of the directory with open_tree() and
OPEN_TREE_CLONE: lookups from its root can't go above it, so the
directory holding resolv.conf is all we can still see. Where possible
(Linux 5.12 or later), make that copy read-only, too. Cloning needs
Linux 5.2 or later: if it fails, or if we can't read resolv.conf through
the copy, we just don't follow changes.

AppArmor resolves paths of files on a detached mount relative to their
filesystem, so allow the usual locations of resolv.conf under /run with
paths relative to /run, too.

Flows that already exist keep their target, so a client reusing the
same source port keeps reaching the old nameserver until its flow
expires.

Link: https://github.com/podman-container-tools/podman/issues/29130
Link: https://archives.passt.top/passt-dev/20230121104703.3ebcc753@elisabeth/
Signed-off-by: Aditya Merchant <adityamerchant08@gmail.com>
---

Notes:
    Tested on Kali (arm64, Linux 7.1) with rootless Podman 5.8.6, and with
    pasta alone, with the AppArmor profile enforcing: resolv.conf replaced
    by rename, rewritten in place, empty at start and filled in later, and
    a symbolic link into /run. With --dns-host, the address stays fixed, as
    before. I also checked that, from the copy, ".." doesn't lead anywhere,
    and that files can't be written or created. I didn't try kernels older
    than 5.12 or 5.2.
    
    I couldn't test SELinux: pasta.te doesn't allow 'watch' on etc_t or on
    the directories under /run holding resolv.conf, so on Fedora I'd expect
    inotify_add_watch() to be denied, and changes not to be followed (by
    default, Fedora points resolv.conf to systemd-resolved on 127.0.0.53,
    which doesn't change). I can add rules if you tell me which types or
    interfaces you'd prefer.
    
    There's no test under test/ yet, as the tests can't change the host's
    resolv.conf. I could try one using a separate mount namespace, if you
    think it's worth it.

 conf.c                              | 239 ++++++++++++++++++++++++++--
 conf.h                              |   2 +
 contrib/apparmor/abstractions/pasta |   5 +
 epoll_type.h                        |   2 +
 isolation.c                         |   6 +-
 isolation.h                         |   2 +-
 linux_dep.h                         |  30 ++++
 passt.1                             |   4 +-
 passt.c                             |   5 +
 passt.h                             |   9 ++
 10 files changed, 291 insertions(+), 13 deletions(-)

diff --git a/conf.c b/conf.c
index 149a4c7..63b4b9a 100644
--- a/conf.c
+++ b/conf.c
@@ -19,6 +19,9 @@
 #include <libgen.h>
 #include <string.h>
 #include <sched.h>
+#include <sys/inotify.h>
+#include <sys/mount.h>
+#include <sys/syscall.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <limits.h>
@@ -36,6 +39,7 @@
 #include <netinet/if_ether.h>
 
 #include "util.h"
+#include "linux_dep.h"
 #include "bitmap.h"
 #include "ip.h"
 #include "passt.h"
@@ -199,6 +203,31 @@ static void add_dns_resolv(struct ctx *c, const char *nameserver,
 		add_dns_resolv6(c, &ns6, idx6);
 }
 
+/**
+ * resolv_nameserver() - Get address from a nameserver line of resolv.conf
+ * @line:	Line from resolv.conf, terminated, might be modified
+ *
+ * Return: address as string, NULL if @line is not a nameserver line
+ */
+static const char *resolv_nameserver(char *line)
+{
+	const char *p;
+	char *end;
+
+	if (strstr(line, "nameserver ") != line)
+		return NULL;
+
+	p = strrchr(line, ' ');
+	if (!p)
+		return NULL;
+
+	end = strpbrk(line, "%\n");
+	if (end)
+		*end = 0;
+
+	return p + 1;
+}
+
 /**
  * get_dns() - Get nameserver addresses from local /etc/resolv.conf
  * @c:		Execution context
@@ -218,6 +247,14 @@ static void get_dns(struct ctx *c)
 	dnss_set = !!*s->n || c->no_dns_search;
 	dns_set = (dns4_set && dns6_set) || c->no_dns;
 
+	/* Unless given with --dns-host, or unless we don't read nameservers
+	 * at all, host nameservers we forward queries to come from resolv.conf
+	 */
+	c->ip4.dns_host_resolv = !c->no_dns && !dns4_set &&
+				 IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_host);
+	c->ip6.dns_host_resolv = !c->no_dns && !dns6_set &&
+				 IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_host);
+
 	if (dns_set && dnss_set)
 		return;
 
@@ -226,16 +263,8 @@ static void get_dns(struct ctx *c)
 
 	lineread_init(&resolvconf, fd);
 	while ((line_len = lineread_get(&resolvconf, &line)) > 0) {
-		if (!dns_set && strstr(line, "nameserver ") == line) {
-			p = strrchr(line, ' ');
-			if (!p)
-				continue;
-
-			end = strpbrk(line, "%\n");
-			if (end)
-				*end = 0;
-
-			add_dns_resolv(c, p + 1,
+		if (!dns_set && (p = resolv_nameserver(line))) {
+			add_dns_resolv(c, p,
 				       dns4_set ? NULL : &dns4_idx,
 				       dns6_set ? NULL : &dns6_idx);
 		} else if (!dnss_set && strstr(line, "search ") == line &&
@@ -280,6 +309,196 @@ out:
 	}
 }
 
+/**
+ * conf_resolv_update() - Update host nameservers from host's resolv.conf
+ * @c:		Execution context
+ *
+ * Return: 0 on success, negative error code if the file can't be opened
+ *
+ * #syscalls:pasta openat
+ */
+static int conf_resolv_update(struct ctx *c)
+{
+	bool found4 = false, found6 = false;
+	char addr[INET6_ADDRSTRLEN];
+	struct lineread resolvconf;
+	struct in6_addr ns6;
+	struct in_addr ns4;
+	char *line;
+	int fd;
+
+	fd = openat(c->resolv_dir_fd, c->resolv_base, O_RDONLY | O_CLOEXEC);
+	if (fd < 0)
+		return -errno;
+
+	lineread_init(&resolvconf, fd);
+	while (lineread_get(&resolvconf, &line) > 0) {
+		const char *p = resolv_nameserver(line);
+
+		if (!p)
+			continue;
+
+		if (!found4)
+			found4 = inet_pton(AF_INET, p, &ns4) == 1;
+		if (!found6)
+			found6 = inet_pton(AF_INET6, p, &ns6) == 1;
+	}
+	close(fd);
+
+	if (c->ip4.dns_host_resolv && found4 &&
+	    !IN4_ARE_ADDR_EQUAL(&ns4, &c->ip4.dns_host)) {
+		c->ip4.dns_host = ns4;
+		info("Forwarding DNS queries to new host nameserver %s",
+		     inet_ntop(AF_INET, &ns4, addr, sizeof(addr)));
+	}
+
+	if (c->ip6.dns_host_resolv && found6 &&
+	    !IN6_ARE_ADDR_EQUAL(&ns6, &c->ip6.dns_host)) {
+		c->ip6.dns_host = ns6;
+		info("Forwarding DNS queries to new host nameserver %s",
+		     inet_ntop(AF_INET6, &ns6, addr, sizeof(addr)));
+	}
+
+	return 0;
+}
+
+/**
+ * conf_resolv_watch() - Watch host's resolv.conf to follow nameserver changes
+ * @c:		Execution context
+ *
+ * The host nameserver we forward DNS queries to can change at any time, for
+ * example as the host joins a different network or connects to a VPN. Watch
+ * the directory containing resolv.conf with inotify, as the file is usually
+ * replaced by renaming a new one over it, not rewritten in place.
+ *
+ * Once sandboxed, we can't open resolv.conf by path, and a descriptor for the
+ * directory itself would let lookups of ".." reach the whole filesystem. Keep
+ * a detached, read-only copy of the directory instead: lookups can't go above
+ * its root. Cloning needs our own mount namespace, before its mounts are made
+ * unbindable: see isolate_prefork().
+ */
+void conf_resolv_watch(struct ctx *c)
+{
+	struct mount_attr attr = {
+		.attr_set	= MOUNT_ATTR_RDONLY | MOUNT_ATTR_NOSUID |
+				  MOUNT_ATTR_NODEV | MOUNT_ATTR_NOEXEC,
+		.propagation	= MS_PRIVATE,
+	};
+	union epoll_ref ref = { .type = EPOLL_TYPE_RESOLV_INOTIFY };
+	char dir[PATH_MAX], *base;
+	int fd, dir_fd, rc;
+
+	if (c->mode != MODE_PASTA)
+		return;
+
+	if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match))
+		c->ip4.dns_host_resolv = false;
+	if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_match))
+		c->ip6.dns_host_resolv = false;
+
+	if (!c->ip4.dns_host_resolv && !c->ip6.dns_host_resolv)
+		return;
+
+	/* With systemd-resolved, resolvconf, or NetworkManager in some
+	 * configurations, resolv.conf is a symbolic link: watch its target
+	 */
+	if (!realpath("/etc/resolv.conf", dir)) {
+		debug_perror("Not following changes to /etc/resolv.conf");
+		return;
+	}
+
+	/* Absolute path from realpath(): split it into directory and file */
+	base = strrchr(dir, '/');
+	if (!base || snprintf_check(c->resolv_base, sizeof(c->resolv_base),
+				    "%s", base + 1))
+		return;
+	*(base == dir ? base + 1 : base) = 0;
+
+	/* AT_RECURSIVE: mounts we inherited in our user namespace are locked,
+	 * and the kernel refuses to clone a subtree without its submounts
+	 */
+	dir_fd = syscall(SYS_open_tree, AT_FDCWD, dir,
+			 OPEN_TREE_CLONE | OPEN_TREE_CLOEXEC | AT_RECURSIVE);
+	if (dir_fd < 0) {
+		debug_perror("Can't clone %s, not following changes", dir);
+		return;
+	}
+
+	/* Best effort, needs Linux 5.12: we only need to read from the copy */
+	if (syscall(SYS_mount_setattr, dir_fd, "", AT_EMPTY_PATH | AT_RECURSIVE,
+		    &attr, sizeof(attr)))
+		debug_perror("Can't make copy of %s read-only", dir);
+
+	if ((fd = inotify_init1(IN_NONBLOCK | IN_CLOEXEC)) < 0) {
+		debug_perror("Can't follow changes to resolv.conf");
+		goto fail;
+	}
+
+	if (inotify_add_watch(fd, dir, IN_CLOSE_WRITE | IN_MOVED_TO) < 0) {
+		debug_perror("Can't watch %s for changes", dir);
+		goto fail;
+	}
+
+	/* With the watch in place, read the file again: it might have changed
+	 * since get_dns() read it. This also checks that we can read it through
+	 * the copy, which security policies might not allow.
+	 */
+	c->resolv_dir_fd = dir_fd;
+	if ((rc = conf_resolv_update(c))) {
+		debug("Can't read %s/%s through a copy: %s", dir, c->resolv_base,
+		      strerror_(-rc));
+		goto fail;
+	}
+
+	if (fd > FD_REF_MAX) {
+		debug("resolv.conf watch file number %i too big", fd);
+		goto fail;
+	}
+
+	ref.fd = fd;
+	if (epoll_add(c->epollfd, EPOLLIN, ref))
+		goto fail;
+
+	return;
+
+fail:
+	c->resolv_dir_fd = -1;
+	if (fd >= 0)
+		close(fd);
+	close(dir_fd);
+}
+
+/**
+ * conf_resolv_handler() - Handle inotify events for host's resolv.conf
+ * @c:		Execution context
+ * @fd:		inotify file descriptor watching directory of resolv.conf
+ */
+void conf_resolv_handler(struct ctx *c, int fd)
+{
+	char buf[sizeof(struct inotify_event) + NAME_MAX + 1]
+		__attribute__ ((aligned(__alignof__(struct inotify_event))));
+	const struct inotify_event *ev;
+	bool changed = false;
+	ssize_t n;
+	char *p;
+	int rc;
+
+	if ((n = read(fd, buf, sizeof(buf))) < (ssize_t)sizeof(*ev))
+		return;
+
+	for (p = buf; p < buf + n; p += sizeof(*ev) + ev->len) {
+		ev = (const struct inotify_event *)p;
+
+		if ((ev->mask & IN_Q_OVERFLOW) ||
+		    (ev->len && !strcmp(ev->name, c->resolv_base)))
+			changed = true;
+	}
+
+	if (changed && (rc = conf_resolv_update(c)))
+		warn("Can't read %s to update nameservers: %s",
+		     c->resolv_base, strerror_(-rc));
+}
+
 /**
  * conf_netns_opt() - Parse --netns option
  * @netns:	buffer of size PATH_MAX, updated with netns path
diff --git a/conf.h b/conf.h
index 19bf9bc..15eb226 100644
--- a/conf.h
+++ b/conf.h
@@ -11,5 +11,7 @@ int conf_tap_fd(int argc, char **argv);
 void conf(struct ctx *c, int argc, char **argv);
 void conf_listen_handler(struct ctx *c, uint32_t events);
 void conf_handler(struct ctx *c, uint32_t events);
+void conf_resolv_watch(struct ctx *c);
+void conf_resolv_handler(struct ctx *c, int fd);
 
 #endif /* CONF_H */
diff --git a/contrib/apparmor/abstractions/pasta b/contrib/apparmor/abstractions/pasta
index 89fa427..8711f9f 100644
--- a/contrib/apparmor/abstractions/pasta
+++ b/contrib/apparmor/abstractions/pasta
@@ -29,6 +29,11 @@
 
   @{run}/user/@{uid}/**			rw,	# pasta_open_ns()
 
+  # Host's resolv.conf in /run, read via a detached copy of its directory,
+  # with paths relative to /run: conf_resolv_watch(), conf_resolv_update()
+  /{NetworkManager,connman,netconfig,resolvconf}/resolv.conf	r,
+  /systemd/resolve/{,stub-}resolv.conf	r,
+
   @{PROC}/[0-9]*/ns/			r,	# pasta_netns_quit_init(),
   @{PROC}/[0-9]*/ns/net			r,	# pasta_wait_for_ns(),
   @{PROC}/[0-9]*/ns/user		r,	# conf_pasta_ns()
diff --git a/epoll_type.h b/epoll_type.h
index 061325a..a42944f 100644
--- a/epoll_type.h
+++ b/epoll_type.h
@@ -50,6 +50,8 @@ enum epoll_type {
 	EPOLL_TYPE_CONF_LISTEN,
 	/* Configuration socket */
 	EPOLL_TYPE_CONF,
+	/* inotify fd watching host's resolv.conf (pasta) */
+	EPOLL_TYPE_RESOLV_INOTIFY,
 
 	EPOLL_NUM_TYPES,
 };
diff --git a/isolation.c b/isolation.c
index a00475f..6c42db7 100644
--- a/isolation.c
+++ b/isolation.c
@@ -470,11 +470,12 @@ void isolate_user(const struct ctx *c, uid_t uid, gid_t gid, bool use_userns,
  * Should:
  *  - Move us to our own IPC and UTS namespaces
  *  - Move us to a mount namespace with only an empty directory
+ *  - Keep a detached copy of the directory of resolv.conf, if needed
  *  - Drop unneeded capabilities (in the new user namespace)
  * Mustn't:
  *  - Remove syscalls we need to daemonise
  */
-int isolate_prefork(const struct ctx *c)
+int isolate_prefork(struct ctx *c)
 {
 	int flags = CLONE_NEWIPC | CLONE_NEWNS | CLONE_NEWUTS;
 	uint64_t ns_caps = 0;
@@ -491,6 +492,9 @@ int isolate_prefork(const struct ctx *c)
 		return -errno;
 	}
 
+	/* This clones a mount: do it before they all become unbindable */
+	conf_resolv_watch(c);
+
 	if (mount("", "/", "", MS_UNBINDABLE | MS_REC, NULL)) {
 		err_perror("Failed to remount /");
 		return -errno;
diff --git a/isolation.h b/isolation.h
index ec47038..4b8f822 100644
--- a/isolation.h
+++ b/isolation.h
@@ -14,7 +14,7 @@ void isolate_initial(void);
 int isolate_fds(int argc, char **argv);
 void isolate_user(const struct ctx *c, uid_t uid, gid_t gid, bool use_userns,
 		  const char *userns);
-int isolate_prefork(const struct ctx *c);
+int isolate_prefork(struct ctx *c);
 void isolate_postfork(const struct ctx *c);
 
 #endif /* ISOLATION_H */
diff --git a/linux_dep.h b/linux_dep.h
index fa539ce..57ebc0d 100644
--- a/linux_dep.h
+++ b/linux_dep.h
@@ -138,6 +138,36 @@ struct tcp_info_linux {
 #define CLOSE_RANGE_UNSHARE	(1U << 1)
 #endif
 
+#include <sys/mount.h>
+
+/* Older C libraries, or kernel headers < 5.2 and < 5.12, need these */
+#ifndef SYS_open_tree
+#define SYS_open_tree		428
+#endif
+#ifndef OPEN_TREE_CLONE
+#define OPEN_TREE_CLONE		1
+#define OPEN_TREE_CLOEXEC	O_CLOEXEC
+#endif
+#ifndef AT_RECURSIVE
+#define AT_RECURSIVE		0x8000
+#endif
+#ifndef SYS_mount_setattr
+#define SYS_mount_setattr	442
+#endif
+#ifndef MOUNT_ATTR_RDONLY
+#define MOUNT_ATTR_RDONLY	0x00000001
+#define MOUNT_ATTR_NOSUID	0x00000002
+#define MOUNT_ATTR_NODEV	0x00000004
+#define MOUNT_ATTR_NOEXEC	0x00000008
+
+struct mount_attr {
+	uint64_t attr_set;
+	uint64_t attr_clr;
+	uint64_t propagation;
+	uint64_t userns_fd;
+};
+#endif
+
 #ifndef TCP_REPAIR_ON
 #define TCP_REPAIR_ON		1
 #define TCP_REPAIR_OFF		0
diff --git a/passt.1 b/passt.1
index 0b780fe..126e692 100644
--- a/passt.1
+++ b/passt.1
@@ -287,7 +287,9 @@ Configure the host nameserver which guest or namespace queries to the
 \fB\-\-dns-forward\fR address will be redirected to. This option can
 be specified zero to two times (once for IPv4, once for IPv6).
 By default, the first nameserver from the host's
-\fI/etc/resolv.conf\fR.
+\fI/etc/resolv.conf\fR. In pasta mode, this default follows changes to
+that file: new DNS flows go to its current first nameserver of the same IP
+version, while existing flows keep their target until they expire.
 
 .TP
 .BR \-S ", " \-\-search " " \fIlist
diff --git a/passt.c b/passt.c
index 5054551..3081166 100644
--- a/passt.c
+++ b/passt.c
@@ -69,6 +69,7 @@ struct ctx passt_ctx = {
 	.fd_control_listen	= -1,
 	.fd_repair_listen	= -1,
 	.pasta_netns_fd		= -1,
+	.resolv_dir_fd		= -1,
 	.device_state_fd	= -1,
 };
 
@@ -92,6 +93,7 @@ char *epoll_type_str[] = {
 	[EPOLL_TYPE_NL_NEIGH]		= "netlink neighbour notifier socket",
 	[EPOLL_TYPE_CONF_LISTEN]	= "configuration listening socket",
 	[EPOLL_TYPE_CONF]		= "configuration socket",
+	[EPOLL_TYPE_RESOLV_INOTIFY]	= "resolv.conf inotify watch",
 };
 static_assert(ARRAY_SIZE(epoll_type_str) == EPOLL_NUM_TYPES,
 	      "epoll_type_str[] doesn't match enum epoll_type");
@@ -302,6 +304,9 @@ static void passt_worker(void *opaque, int nfds, struct epoll_event *events)
 		case EPOLL_TYPE_CONF:
 			conf_handler(c, eventmask);
 			break;
+		case EPOLL_TYPE_RESOLV_INOTIFY:
+			conf_resolv_handler(c, ref.fd);
+			break;
 		default:
 			/* Can't happen */
 			assert(0);
diff --git a/passt.h b/passt.h
index afd8e9f..d807222 100644
--- a/passt.h
+++ b/passt.h
@@ -80,6 +80,7 @@ enum passt_modes {
  * @dns_host:		Use this DNS on the host for forwarding
  * @addr_out:		Optional source address for outbound traffic
  * @ifname_out:		Optional interface name to bind outbound sockets to
+ * @dns_host_resolv:	@dns_host comes from host's resolv.conf: follow changes
  * @no_copy_routes:	Don't copy all routes when configuring target namespace
  * @no_copy_addrs:	Don't copy all addresses when configuring namespace
  * @addr_fixed:		Address was given explicitly (-a): don't update
@@ -103,6 +104,7 @@ struct ip4_ctx {
 
 	char ifname_out[IFNAMSIZ];
 
+	bool dns_host_resolv;
 	bool no_copy_routes;
 	bool no_copy_addrs;
 	bool addr_fixed;
@@ -125,6 +127,7 @@ struct ip4_ctx {
  * @dns_host:		Use this DNS on the host for forwarding
  * @addr_out:		Optional source address for outbound traffic
  * @ifname_out:		Optional interface name to bind outbound sockets to
+ * @dns_host_resolv:	@dns_host comes from host's resolv.conf: follow changes
  * @no_copy_routes:	Don't copy all routes when configuring target namespace
  * @no_copy_addrs:	Don't copy all addresses when configuring namespace
  * @addr_fixed:		Address was given explicitly (-a): don't update
@@ -149,6 +152,7 @@ struct ip6_ctx {
 
 	char ifname_out[IFNAMSIZ];
 
+	bool dns_host_resolv;
 	bool no_copy_routes;
 	bool no_copy_addrs;
 	bool addr_fixed;
@@ -175,6 +179,8 @@ struct ip6_ctx {
  * @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
  * @netns_dir:		Directory of fs-bound namespace, if any, in pasta mode
+ * @resolv_dir_fd:	Directory of host's resolv.conf, if watched for changes
+ * @resolv_base:	Base name of host's resolv.conf in @resolv_dir_fd
  * @epollfd:		File descriptor for epoll instance
  * @fd_tap_listen:	File descriptor for listening AF_UNIX socket, if any
  * @fd_tap:		AF_UNIX socket, tuntap device, or pre-opened socket
@@ -252,6 +258,9 @@ struct ctx {
 	char netns_base[PATH_MAX];
 	char netns_dir[PATH_MAX];
 
+	int resolv_dir_fd;
+	char resolv_base[NAME_MAX + 1];
+
 	int epollfd;
 	int fd_tap_listen;
 	int fd_tap;
-- 
2.50.1 (Apple Git-155)