From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id B2FF55A004C for ; Wed, 29 May 2024 04:35:38 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1716950133; bh=qWJnffrE9wutE3w0gEm4gLITb10bT8UYit7EbfXuw9U=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=bCrKSNtpmDMK+igxydWen1Sx0Xijr/qPUrfrHv0/ozbCLTA+N3zywnDbRLz5ec582 nC5YI3WxsPmtpTWtB5PHyuH9SEZ1Zlra65d3pC49dIfb4meTj6nZuk/3/rUHHZnOPD 6gaif3mG5MlctMYesKkjTc2eH6pW5Hqj7WYpn2J8wzYq0ZFgJEBHlGP713Znu2xWWm Ao+ebdWQopGdcHQYMf79bJaJv56nM6OkGpG6PwQ+Vz6VdLUCJd76OZAitZAlw1QUrV RMRGyfX6fe4N90loFkQIgfHKPfT+dDGQaNWn/xSU5JbDiqG7H90mqTiEdOvwZhjmVn AUdpfUAqDXKQQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Vptln5Xy5z4x1T; Wed, 29 May 2024 12:35:33 +1000 (AEST) Date: Wed, 29 May 2024 12:35:24 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 7/8] conf, passt, tap: Open socket and PID files before switching UID/GID Message-ID: References: <20240522205911.261325-1-sbrivio@redhat.com> <20240522205911.261325-8-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="luDw/cryFwoMHYcA" Content-Disposition: inline In-Reply-To: <20240522205911.261325-8-sbrivio@redhat.com> Message-ID-Hash: QUXBPITTMUGICUV6ELHQHK4TRCG3ZXXQ X-Message-ID-Hash: QUXBPITTMUGICUV6ELHQHK4TRCG3ZXXQ X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, "'Richard W . M . Jones'" , Minxi Hou X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --luDw/cryFwoMHYcA Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > Reported-by: Minxi Hou > Reported-by: Richard W.M. Jones > Signed-off-by: Stefano Brivio 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(-) >=20 > 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; > =20 > /* ...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) > } > } > =20 > +/** > + * conf_open_files() - Open files as requested by configuration > + * @c: Execution context > + */ > +static void conf_open_files(struct ctx *c) > +{ > + if (c->mode =3D=3D MODE_PASST && c->fd_tap =3D=3D -1) > + c->fd_tap_listen =3D tap_sock_unix_open(c->sock_path); > + > + c->pidfile_fd =3D 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 !=3D argc) > die("Extra non-option argument: %s", argv[optind]); > =20 > + conf_open_files(c); /* Before any possible setuid() / setgid() */ > + > isolate_user(uid, gid, !netns_only, userns, c->mode); > =20 > 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 =3D -1, pidfile_fd; > struct epoll_event events[EPOLL_EVENTS]; > char *log_name, argv0[PATH_MAX], *name; > + int nfds, i, devnull_fd =3D -1; > struct ctx c =3D { 0 }; > struct rlimit limit; > struct timespec now; > @@ -211,7 +211,7 @@ int main(int argc, char **argv) > =20 > isolate_initial(); > =20 > - c.pasta_netns_fd =3D c.fd_tap =3D -1; > + c.pasta_netns_fd =3D c.fd_tap =3D c.pidfile_fd =3D -1; > =20 > sigemptyset(&sa.sa_mask); > sa.sa_flags =3D 0; > @@ -299,8 +299,6 @@ int main(int argc, char **argv) > } > } > =20 > - pidfile_fd =3D pidfile_open(c.pid_file); > - > if (isolate_prefork(&c)) > die("Failed to sandbox process, exiting"); > =20 > @@ -308,9 +306,9 @@ int main(int argc, char **argv) > __openlog(log_name, 0, LOG_DAEMON); > =20 > 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()); > =20 > 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 go= ne > * @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; > =20 > 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 =3D socket(AF_UNIX, SOCK_STREAM, 0); > struct sockaddr_un addr =3D { > @@ -1144,7 +1144,7 @@ static int tap_sock_unix_open(char *sock_path) > if (i =3D=3D UNIX_SOCK_MAX) > die("UNIX socket bind: %s", strerror(errno)); > =20 > - 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); > =20 > @@ -1167,7 +1167,7 @@ static void tap_sock_unix_init(struct ctx *c) > ev.data.u64 =3D ref.u64; > epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev); > =20 > - info("You can now start qemu (>=3D 7.2, with commit 13c6be96618c):"); > + info("\nYou can now start qemu (>=3D 7.2, with commit 13c6be96618c):"); > info(" kvm ... -device virtio-net-pci,netdev=3Ds -netdev stream,id= =3Ds,server=3Doff,addr.type=3Dunix,addr.path=3D%s", > c->sock_path); > info("or qrap, for earlier qemu versions:"); > @@ -1318,7 +1318,6 @@ void tap_sock_init(struct ctx *c) > } > =20 > if (c->mode =3D=3D MODE_PASST) { > - c->fd_tap_listen =3D tap_sock_unix_open(c->sock_path); > tap_sock_unix_init(c); > =20 > /* 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); > =20 > #endif /* TAP_H */ --=20 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 --luDw/cryFwoMHYcA Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZWlF8ACgkQzQJF27ox 2Gft6Q/6AocJX4dr1P1LygY4E5XrZ+GiJvUuuZZYPEU8WJhJrKE/9SAyt62kMyOR PL7aUrbvtB+QRxrRRKeCUDoIoUroWuhR1N3smj+/TLi5aNl3f2DSsKRrKQd0MQVK I2QAIFpqR/mQzhpdnai6MPHCAQzSQ6clH17YAqzEadvusybkyfYKCloZKI5br9f/ QdmRJYYIEmpzirZmTCKAZEfMhlTtZc3+S1bI4u+sT6LJRhOSi+IKU+PD9WhPKkt9 9P0wwH9gspnUoS8ZVVCsZGPLX5CyTnKHRfU/6vFfQVUBzNj2cJv/nap0IDpiNdAk Mfldd+9VZ5WzWWizCONcvlzw6A7Pdlw4Vt3EK74PmFqdtDgrN3Su1SSuj84E+eE2 MvhKzDA5N81RaypP12fHYeKutUvmLtPbapJGfG3edoJvt2eJoPbUSDB8Qhlmu3/y 3675R/llvgq9nTfBONoG4T8dyUs1uqvmObtGn9I2ZoxrkvAWFiNw7hTuTeBhZS8t KK1UvgDSVu318EKm6U5LpDFMxq8YBJD7+Lxuy/bD7NqFn8jK0AbR9Cx4iH6e3qo3 r6mpHUiXFssTez4ZraZHp0Z9+z2CuGINNnT8kyq3jg5i4aWrr5ePPEX3YYegxJPn 4jNB2BFHPwCxZIcfrHJVRIgS0V0i6jTPcaHthTuDeJu/F90eGmQ= =3CJO -----END PGP SIGNATURE----- --luDw/cryFwoMHYcA--