From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 0BE4B5A004F for ; Thu, 20 Jun 2024 13:31:01 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1718883061; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=UKIFqXHuAI2ytvl/EdsH1DNmCAZmuQGzbrRLNzQk/sk=; b=BUIC5exTkIrcjkqC5xGqetwuAtjcUcbhu1XUg9Q2Eyv9AaHj5Tr7dfPgNYE5RufVnFavdB mf9KVmvrLyRcfg2Z4MY2EqR/DPLkKMz9d7kDL39RVGP8kSpXs5uAXx1xPjqZ4oqDp2eA1S B+ORKcnEUxC1XD+nYtjz4lra2m9kPXE= Received: from mx-prod-mc-04.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-340-yzEZkKcRMemjDL_CQZ0-pg-1; Thu, 20 Jun 2024 07:30:59 -0400 X-MC-Unique: yzEZkKcRMemjDL_CQZ0-pg-1 Received: from mx-prod-int-02.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-02.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.15]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-04.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 4B6AD19560B6; Thu, 20 Jun 2024 11:30:57 +0000 (UTC) Received: from localhost (unknown [10.39.195.98]) by mx-prod-int-02.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id B671F1956087; Thu, 20 Jun 2024 11:30:55 +0000 (UTC) Date: Thu, 20 Jun 2024 12:30:54 +0100 From: "Richard W.M. Jones" To: David Gibson Subject: Re: [PATCH 7/8] conf, passt, tap: Open socket and PID files before switching UID/GID Message-ID: <20240620113054.GB1450@redhat.com> References: <20240522205911.261325-1-sbrivio@redhat.com> <20240522205911.261325-8-sbrivio@redhat.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Scanned-By: MIMEDefang 3.0 on 10.30.177.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Message-ID-Hash: 5LAAXX5ULWJD2ACNYGHJW6VALGKDX4BT X-Message-ID-Hash: 5LAAXX5ULWJD2ACNYGHJW6VALGKDX4BT X-MailFrom: rjones@redhat.com 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: Stefano Brivio , passt-dev@passt.top, 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: 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 > > 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. 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