If libguestfs tools run as root, with the 'direct' backend (without libvirt), we'll start as root as well. As guest images might be owned by root, there are valid reasons to use libguestfs tools as root, so be nice to them: open socket and PID files *before* switching to nobody, so that we can still access their paths. Stefano Brivio (8): conf: Don't lecture user about starting us as root tap: Move all-ones initialisation of mac_guest to tap_sock_init() passt, tap: Don't use -1 as uninitialised value for fd_tap_listen tap: Split tap_sock_unix_init() into opening and listening parts util: Rename write_pidfile() to pidfile_write() passt, util: Move opening of PID file to its own function conf, passt, tap: Open socket and PID files before switching UID/GID conf, passt.h: Rename pid_file in struct ctx to pidfile conf.c | 23 +++++++++++++++++++---- passt.c | 17 ++++------------- passt.h | 8 ++++++-- tap.c | 57 +++++++++++++++++++++++++++++++++++---------------------- tap.h | 1 + util.c | 28 +++++++++++++++++++++++++--- util.h | 3 ++- 7 files changed, 92 insertions(+), 45 deletions(-) -- 2.43.0
libguestfs tools have a good reason to run as root: if the guest image
is owned by root, it would be counterproductive to encourage users to
invoke them as non-root, as it would require changing permissions or
ownership of the image file.
And if they run as root, we'll start as root, too. Warn users we'll
switch to 'nobody', but don't tell them what to do.
Reported-by: Richard W.M. Jones <rjones(a)redhat.com>
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/conf.c b/conf.c
index 21d46fe..2e0d909 100644
--- a/conf.c
+++ b/conf.c
@@ -1093,7 +1093,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
return;
/* ...otherwise use nobody:nobody */
- warn("Don't run as root. Changing to nobody...");
+ warn("Started as root. Changing to nobody...");
{
#ifndef GLIBC_NO_STATIC_NSS
const struct passwd *pw;
--
2.43.0
On Wed, May 22, 2024 at 10:59:04PM +0200, Stefano Brivio wrote:libguestfs tools have a good reason to run as root: if the guest image is owned by root, it would be counterproductive to encourage users to invoke them as non-root, as it would require changing permissions or ownership of the image file. And if they run as root, we'll start as root, too. Warn users we'll switch to 'nobody', but don't tell them what to do. Reported-by: Richard W.M. Jones <rjones(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>--- conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conf.c b/conf.c index 21d46fe..2e0d909 100644 --- a/conf.c +++ b/conf.c @@ -1093,7 +1093,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid) return; /* ...otherwise use nobody:nobody */ - warn("Don't run as root. Changing to nobody..."); + warn("Started as root. Changing to nobody..."); { #ifndef GLIBC_NO_STATIC_NSS const struct passwd *pw;-- 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
On Wed, May 22, 2024 at 10:59:04PM +0200, Stefano Brivio wrote:libguestfs tools have a good reason to run as root: if the guest image is owned by root, it would be counterproductive to encourage users to invoke them as non-root, as it would require changing permissions or ownership of the image file. And if they run as root, we'll start as root, too. Warn users we'll switch to 'nobody', but don't tell them what to do. Reported-by: Richard W.M. Jones <rjones(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conf.c b/conf.c index 21d46fe..2e0d909 100644 --- a/conf.c +++ b/conf.c @@ -1093,7 +1093,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid) return; /* ...otherwise use nobody:nobody */ - warn("Don't run as root. Changing to nobody..."); + warn("Started as root. Changing to nobody..."); { #ifndef GLIBC_NO_STATIC_NSS const struct passwd *pw;Makes sense: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com> Rich. -- 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
It has nothing to do with tap_sock_unix_init(). It used to be there as
that function could be called multiple times per passt instance, but
it's not the case anymore.
This also takes care of the fact that, with --fd, we wouldn't set the
initial MAC address, so we would need to wait for the guest to send us
an ARP packet before we could exchange data.
Fixes: 6b4e68383c66 ("passt, tap: Add --fd option")
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
tap.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/tap.c b/tap.c
index 91fd2e2..177fe26 100644
--- a/tap.c
+++ b/tap.c
@@ -1111,12 +1111,6 @@ static void tap_sock_unix_init(struct ctx *c)
if (fd < 0)
die("UNIX socket: %s", strerror(errno));
- /* In passt mode, we don't know the guest's MAC until it sends
- * us packets. Use the broadcast address so our first packets
- * will reach it.
- */
- memset(&c->mac_guest, 0xff, sizeof(c->mac_guest));
-
for (i = 1; i < UNIX_SOCK_MAX; i++) {
char *path = addr.sun_path;
int ex, ret;
@@ -1312,6 +1306,12 @@ void tap_sock_init(struct ctx *c)
if (c->mode == MODE_PASST) {
if (c->fd_tap_listen == -1)
tap_sock_unix_init(c);
+
+ /* In passt mode, we don't know the guest's MAC address until it
+ * sends us packets. Use the broadcast address so that our
+ * first packets will reach it.
+ */
+ memset(&c->mac_guest, 0xff, sizeof(c->mac_guest));
} else {
tap_sock_tun_init(c);
}
--
2.43.0
On Wed, May 22, 2024 at 10:59:05PM +0200, Stefano Brivio wrote:It has nothing to do with tap_sock_unix_init(). It used to be there as that function could be called multiple times per passt instance, but it's not the case anymore. This also takes care of the fact that, with --fd, we wouldn't set the initial MAC address, so we would need to wait for the guest to send us an ARP packet before we could exchange data. Fixes: 6b4e68383c66 ("passt, tap: Add --fd option") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>--- tap.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tap.c b/tap.c index 91fd2e2..177fe26 100644 --- a/tap.c +++ b/tap.c @@ -1111,12 +1111,6 @@ static void tap_sock_unix_init(struct ctx *c) if (fd < 0) die("UNIX socket: %s", strerror(errno)); - /* In passt mode, we don't know the guest's MAC until it sends - * us packets. Use the broadcast address so our first packets - * will reach it. - */ - memset(&c->mac_guest, 0xff, sizeof(c->mac_guest)); - for (i = 1; i < UNIX_SOCK_MAX; i++) { char *path = addr.sun_path; int ex, ret; @@ -1312,6 +1306,12 @@ void tap_sock_init(struct ctx *c) if (c->mode == MODE_PASST) { if (c->fd_tap_listen == -1) tap_sock_unix_init(c); + + /* In passt mode, we don't know the guest's MAC address until it + * sends us packets. Use the broadcast address so that our + * first packets will reach it. + */ + memset(&c->mac_guest, 0xff, sizeof(c->mac_guest)); } else { tap_sock_tun_init(c); }-- 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
On Wed, May 22, 2024 at 10:59:05PM +0200, Stefano Brivio wrote:It has nothing to do with tap_sock_unix_init(). It used to be there as that function could be called multiple times per passt instance, but it's not the case anymore. This also takes care of the fact that, with --fd, we wouldn't set the initial MAC address, so we would need to wait for the guest to send us an ARP packet before we could exchange data. Fixes: 6b4e68383c66 ("passt, tap: Add --fd option") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tap.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tap.c b/tap.c index 91fd2e2..177fe26 100644 --- a/tap.c +++ b/tap.c @@ -1111,12 +1111,6 @@ static void tap_sock_unix_init(struct ctx *c) if (fd < 0) die("UNIX socket: %s", strerror(errno)); - /* In passt mode, we don't know the guest's MAC until it sends - * us packets. Use the broadcast address so our first packets - * will reach it. - */ - memset(&c->mac_guest, 0xff, sizeof(c->mac_guest)); - for (i = 1; i < UNIX_SOCK_MAX; i++) { char *path = addr.sun_path; int ex, ret; @@ -1312,6 +1306,12 @@ void tap_sock_init(struct ctx *c) if (c->mode == MODE_PASST) { if (c->fd_tap_listen == -1) tap_sock_unix_init(c); + + /* In passt mode, we don't know the guest's MAC address until it + * sends us packets. Use the broadcast address so that our + * first packets will reach it. + */ + memset(&c->mac_guest, 0xff, sizeof(c->mac_guest)); } else { tap_sock_tun_init(c); }Reading tap.c, the effect of this is that memset will also be called when c->fd_tap_listen is set (the --fd option). As c cannot be NULL and c->mac_guest exists, this seems safe. Acked-by: Richard W.M. Jones <rjones(a)redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On Thu, May 23, 2024 at 10:59:32AM +0100, Richard W.M. Jones wrote:On Wed, May 22, 2024 at 10:59:05PM +0200, Stefano Brivio wrote:Reading the next patch, I see that fd_tap_listen *isn't* the --fd option ... However that doesn't change the analysis.It has nothing to do with tap_sock_unix_init(). It used to be there as that function could be called multiple times per passt instance, but it's not the case anymore. This also takes care of the fact that, with --fd, we wouldn't set the initial MAC address, so we would need to wait for the guest to send us an ARP packet before we could exchange data. Fixes: 6b4e68383c66 ("passt, tap: Add --fd option") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tap.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tap.c b/tap.c index 91fd2e2..177fe26 100644 --- a/tap.c +++ b/tap.c @@ -1111,12 +1111,6 @@ static void tap_sock_unix_init(struct ctx *c) if (fd < 0) die("UNIX socket: %s", strerror(errno)); - /* In passt mode, we don't know the guest's MAC until it sends - * us packets. Use the broadcast address so our first packets - * will reach it. - */ - memset(&c->mac_guest, 0xff, sizeof(c->mac_guest)); - for (i = 1; i < UNIX_SOCK_MAX; i++) { char *path = addr.sun_path; int ex, ret; @@ -1312,6 +1306,12 @@ void tap_sock_init(struct ctx *c) if (c->mode == MODE_PASST) { if (c->fd_tap_listen == -1) tap_sock_unix_init(c); + + /* In passt mode, we don't know the guest's MAC address until it + * sends us packets. Use the broadcast address so that our + * first packets will reach it. + */ + memset(&c->mac_guest, 0xff, sizeof(c->mac_guest)); } else { tap_sock_tun_init(c); }Reading tap.c, the effect of this is that memset will also be called when c->fd_tap_listen is set (the --fd option). As c cannot be NULLand c->mac_guest exists, this seems safe. Acked-by: Richard W.M. Jones <rjones(a)redhat.com>Rich. -- 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
This is a remnant from the time we kept access to the original
filesystem and we could reinitialise the listening AF_UNIX socket.
Since commit 0515adceaa8f ("passt, pasta: Namespace-based sandboxing,
defer seccomp policy application"), however, we can't re-bind the
listening socket once we're up and running.
Drop the -1 initalisation and the corresponding check.
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
passt.c | 2 +-
tap.c | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/passt.c b/passt.c
index 771b8a7..1df1dc4 100644
--- a/passt.c
+++ b/passt.c
@@ -211,7 +211,7 @@ int main(int argc, char **argv)
isolate_initial();
- c.pasta_netns_fd = c.fd_tap = c.fd_tap_listen = -1;
+ c.pasta_netns_fd = c.fd_tap = -1;
sigemptyset(&sa.sa_mask);
sa.sa_flags = 0;
diff --git a/tap.c b/tap.c
index 177fe26..cb6df5a 100644
--- a/tap.c
+++ b/tap.c
@@ -1304,8 +1304,7 @@ void tap_sock_init(struct ctx *c)
}
if (c->mode == MODE_PASST) {
- if (c->fd_tap_listen == -1)
- tap_sock_unix_init(c);
+ tap_sock_unix_init(c);
/* In passt mode, we don't know the guest's MAC address until it
* sends us packets. Use the broadcast address so that our
--
2.43.0
On Wed, May 22, 2024 at 10:59:06PM +0200, Stefano Brivio wrote:This is a remnant from the time we kept access to the original filesystem and we could reinitialise the listening AF_UNIX socket. Since commit 0515adceaa8f ("passt, pasta: Namespace-based sandboxing, defer seccomp policy application"), however, we can't re-bind the listening socket once we're up and running. Drop the -1 initalisation and the corresponding check. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>--- passt.c | 2 +- tap.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/passt.c b/passt.c index 771b8a7..1df1dc4 100644 --- a/passt.c +++ b/passt.c @@ -211,7 +211,7 @@ int main(int argc, char **argv) isolate_initial(); - c.pasta_netns_fd = c.fd_tap = c.fd_tap_listen = -1; + c.pasta_netns_fd = c.fd_tap = -1; sigemptyset(&sa.sa_mask); sa.sa_flags = 0; diff --git a/tap.c b/tap.c index 177fe26..cb6df5a 100644 --- a/tap.c +++ b/tap.c @@ -1304,8 +1304,7 @@ void tap_sock_init(struct ctx *c) } if (c->mode == MODE_PASST) { - if (c->fd_tap_listen == -1) - tap_sock_unix_init(c); + tap_sock_unix_init(c); /* In passt mode, we don't know the guest's MAC address until it * sends us packets. Use the broadcast address so that our-- 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
We'll need to open and bind the socket a while before listening to it,
so split that into two different functions. No functional changes
intended.
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
tap.c | 39 +++++++++++++++++++++++++++------------
1 file changed, 27 insertions(+), 12 deletions(-)
diff --git a/tap.c b/tap.c
index cb6df5a..c9f580e 100644
--- a/tap.c
+++ b/tap.c
@@ -1095,14 +1095,14 @@ restart:
}
/**
- * tap_sock_unix_init() - Create and bind AF_UNIX socket, listen for connection
- * @c: Execution context
+ * tap_sock_unix_open() - Create and bind AF_UNIX socket
+ * @sock_path: Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix)
+ *
+ * Return: socket descriptor on success, won't return on failure
*/
-static void tap_sock_unix_init(struct ctx *c)
+static int tap_sock_unix_open(char *sock_path)
{
int fd = socket(AF_UNIX, SOCK_STREAM, 0);
- union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN };
- struct epoll_event ev = { 0 };
struct sockaddr_un addr = {
.sun_family = AF_UNIX,
};
@@ -1115,8 +1115,8 @@ static void tap_sock_unix_init(struct ctx *c)
char *path = addr.sun_path;
int ex, ret;
- if (*c->sock_path)
- memcpy(path, c->sock_path, UNIX_PATH_MAX);
+ if (*sock_path)
+ memcpy(path, sock_path, UNIX_PATH_MAX);
else
snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
@@ -1127,7 +1127,7 @@ static void tap_sock_unix_init(struct ctx *c)
ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr));
if (!ret || (errno != ENOENT && errno != ECONNREFUSED &&
errno != EACCES)) {
- if (*c->sock_path)
+ if (*sock_path)
die("Socket path %s already in use", path);
close(ex);
@@ -1137,7 +1137,7 @@ static void tap_sock_unix_init(struct ctx *c)
unlink(path);
if (!bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) ||
- *c->sock_path)
+ *sock_path)
break;
}
@@ -1145,17 +1145,31 @@ static void tap_sock_unix_init(struct ctx *c)
die("UNIX socket bind: %s", strerror(errno));
info("UNIX domain socket bound at %s\n", addr.sun_path);
+ if (!*sock_path)
+ memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX);
+
+ return fd;
+}
+
+/**
+ * tap_sock_unix_init() - Start listening for connections on AF_UNIX socket
+ * @c: Execution context
+ */
+static void tap_sock_unix_init(struct ctx *c)
+{
+ union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN };
+ struct epoll_event ev = { 0 };
- listen(fd, 0);
+ listen(c->fd_tap_listen, 0);
- ref.fd = c->fd_tap_listen = fd;
+ ref.fd = c->fd_tap_listen;
ev.events = EPOLLIN | EPOLLET;
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(" kvm ... -device virtio-net-pci,netdev=s -netdev
stream,id=s,server=off,addr.type=unix,addr.path=%s",
- addr.sun_path);
+ c->sock_path);
info("or qrap, for earlier qemu versions:");
info(" ./qrap 5 kvm ... -net socket,fd=5 -net nic,model=virtio");
}
@@ -1304,6 +1318,7 @@ 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
--
2.43.0
On Wed, May 22, 2024 at 10:59:07PM +0200, Stefano Brivio wrote:We'll need to open and bind the socket a while before listening to it, so split that into two different functions. No functional changes intended. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tap.c | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/tap.c b/tap.c index cb6df5a..c9f580e 100644 --- a/tap.c +++ b/tap.c @@ -1095,14 +1095,14 @@ restart: } /** - * tap_sock_unix_init() - Create and bind AF_UNIX socket, listen for connection - * @c: Execution context + * tap_sock_unix_open() - Create and bind AF_UNIX socket + * @sock_path: Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix) + * + * Return: socket descriptor on success, won't return on failure */ -static void tap_sock_unix_init(struct ctx *c) +static int tap_sock_unix_open(char *sock_path) { int fd = socket(AF_UNIX, SOCK_STREAM, 0); - union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN }; - struct epoll_event ev = { 0 }; struct sockaddr_un addr = { .sun_family = AF_UNIX, }; @@ -1115,8 +1115,8 @@ static void tap_sock_unix_init(struct ctx *c) char *path = addr.sun_path; int ex, ret; - if (*c->sock_path) - memcpy(path, c->sock_path, UNIX_PATH_MAX); + if (*sock_path) + memcpy(path, sock_path, UNIX_PATH_MAX); else snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i); @@ -1127,7 +1127,7 @@ static void tap_sock_unix_init(struct ctx *c) ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr)); if (!ret || (errno != ENOENT && errno != ECONNREFUSED && errno != EACCES)) { - if (*c->sock_path) + if (*sock_path) die("Socket path %s already in use", path); close(ex); @@ -1137,7 +1137,7 @@ static void tap_sock_unix_init(struct ctx *c) unlink(path); if (!bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) || - *c->sock_path) + *sock_path) break; } @@ -1145,17 +1145,31 @@ static void tap_sock_unix_init(struct ctx *c) die("UNIX socket bind: %s", strerror(errno)); info("UNIX domain socket bound at %s\n", addr.sun_path); + if (!*sock_path) + memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX); + + return fd; +} + +/** + * tap_sock_unix_init() - Start listening for connections on AF_UNIX socket + * @c: Execution context + */ +static void tap_sock_unix_init(struct ctx *c) +{ + union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN }; + struct epoll_event ev = { 0 }; - listen(fd, 0); + listen(c->fd_tap_listen, 0); - ref.fd = c->fd_tap_listen = fd; + ref.fd = c->fd_tap_listen; ev.events = EPOLLIN | EPOLLET; 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(" kvm ... -device virtio-net-pci,netdev=s -netdev stream,id=s,server=off,addr.type=unix,addr.path=%s", - addr.sun_path); + c->sock_path); info("or qrap, for earlier qemu versions:"); info(" ./qrap 5 kvm ... -net socket,fd=5 -net nic,model=virtio"); } @@ -1304,6 +1318,7 @@ 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 --Looks like a neutral factoring, so: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On Wed, May 22, 2024 at 10:59:07PM +0200, Stefano Brivio wrote:We'll need to open and bind the socket a while before listening to it, so split that into two different functions. No functional changes intended. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>--- tap.c | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/tap.c b/tap.c index cb6df5a..c9f580e 100644 --- a/tap.c +++ b/tap.c @@ -1095,14 +1095,14 @@ restart: } /** - * tap_sock_unix_init() - Create and bind AF_UNIX socket, listen for connection - * @c: Execution context + * tap_sock_unix_open() - Create and bind AF_UNIX socket + * @sock_path: Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix) + * + * Return: socket descriptor on success, won't return on failure */ -static void tap_sock_unix_init(struct ctx *c) +static int tap_sock_unix_open(char *sock_path) { int fd = socket(AF_UNIX, SOCK_STREAM, 0); - union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN }; - struct epoll_event ev = { 0 }; struct sockaddr_un addr = { .sun_family = AF_UNIX, }; @@ -1115,8 +1115,8 @@ static void tap_sock_unix_init(struct ctx *c) char *path = addr.sun_path; int ex, ret; - if (*c->sock_path) - memcpy(path, c->sock_path, UNIX_PATH_MAX); + if (*sock_path) + memcpy(path, sock_path, UNIX_PATH_MAX); else snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i); @@ -1127,7 +1127,7 @@ static void tap_sock_unix_init(struct ctx *c) ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr)); if (!ret || (errno != ENOENT && errno != ECONNREFUSED && errno != EACCES)) { - if (*c->sock_path) + if (*sock_path) die("Socket path %s already in use", path); close(ex); @@ -1137,7 +1137,7 @@ static void tap_sock_unix_init(struct ctx *c) unlink(path); if (!bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) || - *c->sock_path) + *sock_path) break; } @@ -1145,17 +1145,31 @@ static void tap_sock_unix_init(struct ctx *c) die("UNIX socket bind: %s", strerror(errno)); info("UNIX domain socket bound at %s\n", addr.sun_path); + if (!*sock_path) + memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX); + + return fd; +} + +/** + * tap_sock_unix_init() - Start listening for connections on AF_UNIX socket + * @c: Execution context + */ +static void tap_sock_unix_init(struct ctx *c) +{ + union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN }; + struct epoll_event ev = { 0 }; - listen(fd, 0); + listen(c->fd_tap_listen, 0); - ref.fd = c->fd_tap_listen = fd; + ref.fd = c->fd_tap_listen; ev.events = EPOLLIN | EPOLLET; 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(" kvm ... -device virtio-net-pci,netdev=s -netdev stream,id=s,server=off,addr.type=unix,addr.path=%s", - addr.sun_path); + c->sock_path); info("or qrap, for earlier qemu versions:"); info(" ./qrap 5 kvm ... -net socket,fd=5 -net nic,model=virtio"); } @@ -1304,6 +1318,7 @@ 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-- 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
As I'm adding pidfile_open() in the next patch. The function comment
didn't match, by the way.
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
passt.c | 2 +-
util.c | 6 +++---
util.h | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/passt.c b/passt.c
index 1df1dc4..fb9773d 100644
--- a/passt.c
+++ b/passt.c
@@ -317,7 +317,7 @@ int main(int argc, char **argv)
if (!c.foreground)
__daemon(pidfile_fd, devnull_fd);
else
- write_pidfile(pidfile_fd, getpid());
+ pidfile_write(pidfile_fd, getpid());
if (pasta_child_pid)
kill(pasta_child_pid, SIGUSR1);
diff --git a/util.c b/util.c
index 849fa7f..18c04ba 100644
--- a/util.c
+++ b/util.c
@@ -380,11 +380,11 @@ int open_in_ns(const struct ctx *c, const char *path, int flags)
}
/**
- * pid_file() - Write PID to file, if requested to do so, and close it
+ * pidfile_write() - Write PID to file, if requested to do so, and close it
* @fd: Open PID file descriptor, closed on exit, -1 to skip writing it
* @pid: PID value to write
*/
-void write_pidfile(int fd, pid_t pid)
+void pidfile_write(int fd, pid_t pid)
{
char pid_buf[12];
int n;
@@ -419,7 +419,7 @@ int __daemon(int pidfile_fd, int devnull_fd)
}
if (pid) {
- write_pidfile(pidfile_fd, pid);
+ pidfile_write(pidfile_fd, pid);
exit(EXIT_SUCCESS);
}
diff --git a/util.h b/util.h
index 264423b..f811975 100644
--- a/util.h
+++ b/util.h
@@ -156,7 +156,7 @@ char *line_read(char *buf, size_t len, int fd);
void ns_enter(const struct ctx *c);
bool ns_is_init(void);
int open_in_ns(const struct ctx *c, const char *path, int flags);
-void write_pidfile(int fd, pid_t pid);
+void pidfile_write(int fd, pid_t pid);
int __daemon(int pidfile_fd, int devnull_fd);
int fls(unsigned long x);
int write_file(const char *path, const char *buf);
--
2.43.0
On Wed, May 22, 2024 at 10:59:08PM +0200, Stefano Brivio wrote:As I'm adding pidfile_open() in the next patch. The function comment didn't match, by the way. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- passt.c | 2 +- util.c | 6 +++--- util.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/passt.c b/passt.c index 1df1dc4..fb9773d 100644 --- a/passt.c +++ b/passt.c @@ -317,7 +317,7 @@ int main(int argc, char **argv) if (!c.foreground) __daemon(pidfile_fd, devnull_fd); else - write_pidfile(pidfile_fd, getpid()); + pidfile_write(pidfile_fd, getpid()); if (pasta_child_pid) kill(pasta_child_pid, SIGUSR1); diff --git a/util.c b/util.c index 849fa7f..18c04ba 100644 --- a/util.c +++ b/util.c @@ -380,11 +380,11 @@ int open_in_ns(const struct ctx *c, const char *path, int flags) } /** - * pid_file() - Write PID to file, if requested to do so, and close it + * pidfile_write() - Write PID to file, if requested to do so, and close it * @fd: Open PID file descriptor, closed on exit, -1 to skip writing it * @pid: PID value to write */ -void write_pidfile(int fd, pid_t pid) +void pidfile_write(int fd, pid_t pid) { char pid_buf[12]; int n; @@ -419,7 +419,7 @@ int __daemon(int pidfile_fd, int devnull_fd) } if (pid) { - write_pidfile(pidfile_fd, pid); + pidfile_write(pidfile_fd, pid); exit(EXIT_SUCCESS); } diff --git a/util.h b/util.h index 264423b..f811975 100644 --- a/util.h +++ b/util.h @@ -156,7 +156,7 @@ char *line_read(char *buf, size_t len, int fd); void ns_enter(const struct ctx *c); bool ns_is_init(void); int open_in_ns(const struct ctx *c, const char *path, int flags); -void write_pidfile(int fd, pid_t pid); +void pidfile_write(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf);Neutral refactoring, so: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com> However the same function name appears (just as a comment) in contrib/apparmor/usr.bin.passt so I don't know if you would want to update that? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On Thu, 23 May 2024 11:06:15 +0100 "Richard W.M. Jones" <rjones(a)redhat.com> wrote:On Wed, May 22, 2024 at 10:59:08PM +0200, Stefano Brivio wrote:Right, nice catch, thanks for grepping! I'll send a separate patch for it. -- StefanoAs I'm adding pidfile_open() in the next patch. The function comment didn't match, by the way. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- passt.c | 2 +- util.c | 6 +++--- util.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/passt.c b/passt.c index 1df1dc4..fb9773d 100644 --- a/passt.c +++ b/passt.c @@ -317,7 +317,7 @@ int main(int argc, char **argv) if (!c.foreground) __daemon(pidfile_fd, devnull_fd); else - write_pidfile(pidfile_fd, getpid()); + pidfile_write(pidfile_fd, getpid()); if (pasta_child_pid) kill(pasta_child_pid, SIGUSR1); diff --git a/util.c b/util.c index 849fa7f..18c04ba 100644 --- a/util.c +++ b/util.c @@ -380,11 +380,11 @@ int open_in_ns(const struct ctx *c, const char *path, int flags) } /** - * pid_file() - Write PID to file, if requested to do so, and close it + * pidfile_write() - Write PID to file, if requested to do so, and close it * @fd: Open PID file descriptor, closed on exit, -1 to skip writing it * @pid: PID value to write */ -void write_pidfile(int fd, pid_t pid) +void pidfile_write(int fd, pid_t pid) { char pid_buf[12]; int n; @@ -419,7 +419,7 @@ int __daemon(int pidfile_fd, int devnull_fd) } if (pid) { - write_pidfile(pidfile_fd, pid); + pidfile_write(pidfile_fd, pid); exit(EXIT_SUCCESS); } diff --git a/util.h b/util.h index 264423b..f811975 100644 --- a/util.h +++ b/util.h @@ -156,7 +156,7 @@ char *line_read(char *buf, size_t len, int fd); void ns_enter(const struct ctx *c); bool ns_is_init(void); int open_in_ns(const struct ctx *c, const char *path, int flags); -void write_pidfile(int fd, pid_t pid); +void pidfile_write(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf);Neutral refactoring, so: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com> However the same function name appears (just as a comment) in contrib/apparmor/usr.bin.passt so I don't know if you would want to update that?
We won't call it from main() any longer: move it.
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
passt.c | 11 ++---------
util.c | 22 ++++++++++++++++++++++
util.h | 1 +
3 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/passt.c b/passt.c
index fb9773d..e2446fc 100644
--- a/passt.c
+++ b/passt.c
@@ -199,7 +199,7 @@ void exit_handler(int signal)
*/
int main(int argc, char **argv)
{
- int nfds, i, devnull_fd = -1, pidfile_fd = -1;
+ int nfds, i, devnull_fd = -1, pidfile_fd;
struct epoll_event events[EPOLL_EVENTS];
char *log_name, argv0[PATH_MAX], *name;
struct ctx c = { 0 };
@@ -299,14 +299,7 @@ int main(int argc, char **argv)
}
}
- if (*c.pid_file) {
- if ((pidfile_fd = open(c.pid_file,
- O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC,
- S_IRUSR | S_IWUSR)) < 0) {
- perror("PID file open");
- exit(EXIT_FAILURE);
- }
- }
+ pidfile_fd = pidfile_open(c.pid_file);
if (isolate_prefork(&c))
die("Failed to sandbox process, exiting");
diff --git a/util.c b/util.c
index 18c04ba..cf5a62b 100644
--- a/util.c
+++ b/util.c
@@ -402,6 +402,28 @@ void pidfile_write(int fd, pid_t pid)
close(fd);
}
+/**
+ * pidfile_open() - Open PID file if needed
+ * @path: Path for PID file, empty string if no PID file is requested
+ *
+ * Return: descriptor for PID file, -1 if path is NULL, won't return on failure
+ */
+int pidfile_open(const char *path)
+{
+ int fd;
+
+ if (!*path)
+ return -1;
+
+ if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC,
+ S_IRUSR | S_IWUSR)) < 0) {
+ perror("PID file open");
+ exit(EXIT_FAILURE);
+ }
+
+ return fd;
+}
+
/**
* __daemon() - daemon()-like function writing PID file before parent exits
* @pidfile_fd: Open PID file descriptor
diff --git a/util.h b/util.h
index f811975..8a430ca 100644
--- a/util.h
+++ b/util.h
@@ -156,6 +156,7 @@ char *line_read(char *buf, size_t len, int fd);
void ns_enter(const struct ctx *c);
bool ns_is_init(void);
int open_in_ns(const struct ctx *c, const char *path, int flags);
+int pidfile_open(const char *path);
void pidfile_write(int fd, pid_t pid);
int __daemon(int pidfile_fd, int devnull_fd);
int fls(unsigned long x);
--
2.43.0
On Wed, May 22, 2024 at 10:59:09PM +0200, Stefano Brivio wrote:We won't call it from main() any longer: move it. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- passt.c | 11 ++--------- util.c | 22 ++++++++++++++++++++++ util.h | 1 + 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/passt.c b/passt.c index fb9773d..e2446fc 100644 --- a/passt.c +++ b/passt.c @@ -199,7 +199,7 @@ void exit_handler(int signal) */ int main(int argc, char **argv) { - int nfds, i, devnull_fd = -1, pidfile_fd = -1; + int nfds, i, devnull_fd = -1, pidfile_fd; struct epoll_event events[EPOLL_EVENTS]; char *log_name, argv0[PATH_MAX], *name; struct ctx c = { 0 }; @@ -299,14 +299,7 @@ int main(int argc, char **argv) } } - if (*c.pid_file) { - if ((pidfile_fd = open(c.pid_file, - O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC, - S_IRUSR | S_IWUSR)) < 0) { - perror("PID file open"); - exit(EXIT_FAILURE); - } - } + pidfile_fd = pidfile_open(c.pid_file); if (isolate_prefork(&c)) die("Failed to sandbox process, exiting"); diff --git a/util.c b/util.c index 18c04ba..cf5a62b 100644 --- a/util.c +++ b/util.c @@ -402,6 +402,28 @@ void pidfile_write(int fd, pid_t pid) close(fd); } +/** + * pidfile_open() - Open PID file if needed + * @path: Path for PID file, empty string if no PID file is requested + * + * Return: descriptor for PID file, -1 if path is NULL, won't return on failure + */ +int pidfile_open(const char *path) +{ + int fd; + + if (!*path) + return -1; + + if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC, + S_IRUSR | S_IWUSR)) < 0) { + perror("PID file open"); + exit(EXIT_FAILURE); + } + + return fd; +} + /** * __daemon() - daemon()-like function writing PID file before parent exits * @pidfile_fd: Open PID file descriptor diff --git a/util.h b/util.h index f811975..8a430ca 100644 --- a/util.h +++ b/util.h @@ -156,6 +156,7 @@ char *line_read(char *buf, size_t len, int fd); void ns_enter(const struct ctx *c); bool ns_is_init(void); int open_in_ns(const struct ctx *c, const char *path, int flags); +int pidfile_open(const char *path); void pidfile_write(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x);Trivial refactoring: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On Wed, May 22, 2024 at 10:59:09PM +0200, Stefano Brivio wrote:We won't call it from main() any longer: move it. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>--- passt.c | 11 ++--------- util.c | 22 ++++++++++++++++++++++ util.h | 1 + 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/passt.c b/passt.c index fb9773d..e2446fc 100644 --- a/passt.c +++ b/passt.c @@ -199,7 +199,7 @@ void exit_handler(int signal) */ int main(int argc, char **argv) { - int nfds, i, devnull_fd = -1, pidfile_fd = -1; + int nfds, i, devnull_fd = -1, pidfile_fd; struct epoll_event events[EPOLL_EVENTS]; char *log_name, argv0[PATH_MAX], *name; struct ctx c = { 0 }; @@ -299,14 +299,7 @@ int main(int argc, char **argv) } } - if (*c.pid_file) { - if ((pidfile_fd = open(c.pid_file, - O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC, - S_IRUSR | S_IWUSR)) < 0) { - perror("PID file open"); - exit(EXIT_FAILURE); - } - } + pidfile_fd = pidfile_open(c.pid_file); if (isolate_prefork(&c)) die("Failed to sandbox process, exiting"); diff --git a/util.c b/util.c index 18c04ba..cf5a62b 100644 --- a/util.c +++ b/util.c @@ -402,6 +402,28 @@ void pidfile_write(int fd, pid_t pid) close(fd); } +/** + * pidfile_open() - Open PID file if needed + * @path: Path for PID file, empty string if no PID file is requested + * + * Return: descriptor for PID file, -1 if path is NULL, won't return on failure + */ +int pidfile_open(const char *path) +{ + int fd; + + if (!*path) + return -1; + + if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC, + S_IRUSR | S_IWUSR)) < 0) { + perror("PID file open"); + exit(EXIT_FAILURE); + } + + return fd; +} + /** * __daemon() - daemon()-like function writing PID file before parent exits * @pidfile_fd: Open PID file descriptor diff --git a/util.h b/util.h index f811975..8a430ca 100644 --- a/util.h +++ b/util.h @@ -156,6 +156,7 @@ char *line_read(char *buf, size_t len, int fd); void ns_enter(const struct ctx *c); bool ns_is_init(void); int open_in_ns(const struct ctx *c, const char *path, int flags); +int pidfile_open(const char *path); void pidfile_write(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x);-- 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
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(a)redhat.com>
Reported-by: Richard W.M. Jones <rjones(a)redhat.com>
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
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 */
--
2.43.0
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(a)redhat.com> Reported-by: Richard W.M. Jones <rjones(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- 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 */Seems sensible. The code to open the pidfile definitely moves to the new function and the new function is called just before passt isolates itself in the new namespace. Acked-by: Richard W.M. Jones <rjones(a)redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
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(a)redhat.com> Reported-by: Richard W.M. Jones <rjones(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)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.--- 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
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:Yes, agreed. It seems as if the latest passt code still does this. Do you want me to open a bug about it? Rich.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(a)redhat.com> Reported-by: Richard W.M. Jones <rjones(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)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.-- 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--- 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
On Thu, 20 Jun 2024 12:30:54 +0100 "Richard W.M. Jones" <rjones(a)redhat.com> wrote:On Wed, May 29, 2024 at 12:35:24PM +1000, David Gibson wrote:Yes, please, that, or a patch :) -- StefanoOn Wed, May 22, 2024 at 10:59:10PM +0200, Stefano Brivio wrote:Yes, agreed. It seems as if the latest passt code still does this. Do you want me to open a bug about it?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(a)redhat.com> Reported-by: Richard W.M. Jones <rjones(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)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.
On Thu, Jun 20, 2024 at 02:12:53PM +0200, Stefano Brivio wrote:On Thu, 20 Jun 2024 12:30:54 +0100 "Richard W.M. Jones" <rjones(a)redhat.com> wrote:While I was testing this, I found we do seem to check it: https://passt.top/passt/tree/conf.c#n1446 Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.orgOn Wed, May 29, 2024 at 12:35:24PM +1000, David Gibson wrote:Yes, please, that, or a patch :)On Wed, May 22, 2024 at 10:59:10PM +0200, Stefano Brivio wrote:Yes, agreed. It seems as if the latest passt code still does this. Do you want me to open a bug about it?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(a)redhat.com> Reported-by: Richard W.M. Jones <rjones(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)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.
On Thu, 20 Jun 2024 13:47:31 +0100 "Richard W.M. Jones" <rjones(a)redhat.com> wrote:On Thu, Jun 20, 2024 at 02:12:53PM +0200, Stefano Brivio wrote:Oh, I thought David was referring to the loop in tap_sock_unix_open(), where we try paths in the form "/tmp/passt_%i.socket". But even there, we can't exceed UNIX_PATH_MAX. One minor issue remains, though: in conf(), we refuse paths that are longer than UNIX_SOCK_MAX (100). That's the maximum index for the "/tmp/passt_%i.socket", it happens to be a sane value, but we should use UNIX_PATH_MAX (108) instead. I'll fix it, but wait for David's feedback first. -- StefanoOn Thu, 20 Jun 2024 12:30:54 +0100 "Richard W.M. Jones" <rjones(a)redhat.com> wrote:While I was testing this, I found we do seem to check it: https://passt.top/passt/tree/conf.c#n1446On Wed, May 29, 2024 at 12:35:24PM +1000, David Gibson wrote:Yes, please, that, or a patch :)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(a)redhat.com> > Reported-by: Richard W.M. Jones <rjones(a)redhat.com> > Signed-off-by: Stefano Brivio <sbrivio(a)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?
On Thu, Jun 20, 2024 at 04:22:12PM +0200, Stefano Brivio wrote:On Thu, 20 Jun 2024 13:47:31 +0100 "Richard W.M. Jones" <rjones(a)redhat.com> wrote:Actually I was referring to the memcpy() from sock_path to path in tap_sock_unix_open(). I hadn't thought to check if we'd previously verified the length of sock_path. So, yeah we seem to be ok here (although it's not obvious that's the case from within the code of tap_sock_unix_open()).On Thu, Jun 20, 2024 at 02:12:53PM +0200, Stefano Brivio wrote:Oh, I thought David was referring to the loop in tap_sock_unix_open(), where we try paths in the form "/tmp/passt_%i.socket". But even there, we can't exceed UNIX_PATH_MAX.On Thu, 20 Jun 2024 12:30:54 +0100 "Richard W.M. Jones" <rjones(a)redhat.com> wrote:While I was testing this, I found we do seem to check it: https://passt.top/passt/tree/conf.c#n1446On 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(a)redhat.com> > > Reported-by: Richard W.M. Jones <rjones(a)redhat.com> > > Signed-off-by: Stefano Brivio <sbrivio(a)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?Yes, please, that, or a patch :)One minor issue remains, though: in conf(), we refuse paths that are longer than UNIX_SOCK_MAX (100). That's the maximum index for the "/tmp/passt_%i.socket", it happens to be a sane value, but we should use UNIX_PATH_MAX (108) instead. I'll fix it, but wait for David's feedback first.Well, apart from that, yes. -- David Gibson (he or they) | 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
We have pidfile_fd now, pid_file_fd would be quite ugly.
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
conf.c | 8 ++++----
passt.h | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/conf.c b/conf.c
index f62a5eb..50383a3 100644
--- a/conf.c
+++ b/conf.c
@@ -1123,7 +1123,7 @@ 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);
+ c->pidfile_fd = pidfile_open(c->pidfile);
}
/**
@@ -1456,12 +1456,12 @@ void conf(struct ctx *c, int argc, char **argv)
break;
case 'P':
- if (*c->pid_file)
+ if (*c->pidfile)
die("Multiple --pid options given");
- ret = snprintf(c->pid_file, sizeof(c->pid_file), "%s",
+ ret = snprintf(c->pidfile, sizeof(c->pidfile), "%s",
optarg);
- if (ret <= 0 || ret >= (int)sizeof(c->pid_file))
+ if (ret <= 0 || ret >= (int)sizeof(c->pidfile))
die("Invalid PID file: %s", optarg);
break;
diff --git a/passt.h b/passt.h
index 3e50612..46d073a 100644
--- a/passt.h
+++ b/passt.h
@@ -184,7 +184,7 @@ struct ip6_ctx {
* @nofile: Maximum number of open files (ulimit -n)
* @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: 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
@@ -236,7 +236,7 @@ struct ctx {
char sock_path[UNIX_PATH_MAX];
char pcap[PATH_MAX];
- char pid_file[PATH_MAX];
+ char pidfile[PATH_MAX];
int pidfile_fd;
int one_off;
--
2.43.0
On Wed, May 22, 2024 at 10:59:11PM +0200, Stefano Brivio wrote:We have pidfile_fd now, pid_file_fd would be quite ugly. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- conf.c | 8 ++++---- passt.h | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/conf.c b/conf.c index f62a5eb..50383a3 100644 --- a/conf.c +++ b/conf.c @@ -1123,7 +1123,7 @@ 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); + c->pidfile_fd = pidfile_open(c->pidfile); } /** @@ -1456,12 +1456,12 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'P': - if (*c->pid_file) + if (*c->pidfile) die("Multiple --pid options given"); - ret = snprintf(c->pid_file, sizeof(c->pid_file), "%s", + ret = snprintf(c->pidfile, sizeof(c->pidfile), "%s", optarg); - if (ret <= 0 || ret >= (int)sizeof(c->pid_file)) + if (ret <= 0 || ret >= (int)sizeof(c->pidfile)) die("Invalid PID file: %s", optarg); break; diff --git a/passt.h b/passt.h index 3e50612..46d073a 100644 --- a/passt.h +++ b/passt.h @@ -184,7 +184,7 @@ struct ip6_ctx { * @nofile: Maximum number of open files (ulimit -n) * @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: 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 @@ -236,7 +236,7 @@ struct ctx { char sock_path[UNIX_PATH_MAX]; char pcap[PATH_MAX]; - char pid_file[PATH_MAX]; + char pidfile[PATH_MAX]; int pidfile_fd; int one_off;Looks like trivial refactor so: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On Wed, May 22, 2024 at 10:59:11PM +0200, Stefano Brivio wrote:We have pidfile_fd now, pid_file_fd would be quite ugly. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au> -- 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