On Thu, Aug 08, 2024 at 05:42:49AM +0200, Stefano Brivio wrote:

If a parent accidentally or due to implementation reasons leaks any open file, we don't want to have access to them, except for the file passed via --fd, if any.

This is the case for Podman when Podman's parent leaks files into Podman: it's not practical for Podman to close unrelated files before starting pasta, as reported by Paul.

Use close_range(2) to close all open files except for standard streams and the one from --fd.

Given that parts of conf() depend on other files to be already opened, such as the epoll file descriptor, we can't easily defer this to a more convenient point, where --fd was already parsed. Introduce a minimal, duplicate version of --fd parsing to keep this simple.

As we need to check that the passed --fd option doesn't exceed INT_MAX, because we'll parse it with strtol() but file descriptor indices are signed ints (regardless of the arguments close_range() take), extend the existing check in the actual --fd parsing in conf(), also rejecting file descriptors numbers that match standard streams, while at it.

Suggested-by: Paul Holzinger Signed-off-by: Stefano Brivio

Reviewed-by: David Gibson

--- v7: (yes, seriously) don't close STDERR_FILENO in the general case, start from STDERR_FILENO + 1

v6: (seriously?) fix STDERR_FILENO comparison in conf()

v5: Reject any --fd matching standard streams

v4: c->fd_tap, as used in conf(), is an int: don't assign to it directly from strtol(), or we won't catch overflows

v3: Handle --fd 3 case, and don't overflow if the --fd number exceeds UINT_MAX: add an explicit check to ensure it's less than INT_MAX

v2: Move call to close_open_files() to isolate_initial()

conf.c | 8 ++++++-- isolation.c | 12 +++++++++--- isolation.h | 2 +- passt.c | 2 +- util.c | 41 +++++++++++++++++++++++++++++++++++++++++ util.h | 1 + 6 files changed, 59 insertions(+), 7 deletions(-)

diff --git a/conf.c b/conf.c index 14d8ece..2f5d649 100644 --- a/conf.c +++ b/conf.c @@ -1245,6 +1245,7 @@ void conf(struct ctx *c, int argc, char **argv) const char *optstring; size_t logsize = 0; char *runas = NULL; + long fd_tap_opt; int name, ret; uid_t uid; gid_t gid; @@ -1260,6 +1261,7 @@ void conf(struct ctx *c, int argc, char **argv) c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; c->udp.fwd_in.mode = c->udp.fwd_out.mode = FWD_UNSET;

+ optind = 1; do { name = getopt_long(argc, argv, optstring, options, NULL);

@@ -1424,11 +1426,13 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'F': errno = 0; - c->fd_tap = strtol(optarg, NULL, 0); + fd_tap_opt = strtol(optarg, NULL, 0);

- if (c->fd_tap < 0 || errno) + if (errno || + fd_tap_opt <= STDERR_FILENO || fd_tap_opt > INT_MAX) die("Invalid --fd: %s", optarg);

+ c->fd_tap = fd_tap_opt; c->one_off = true; *c->sock_path = 0; break; diff --git a/isolation.c b/isolation.c index 4956d7e..45fba1e 100644 --- a/isolation.c +++ b/isolation.c @@ -29,7 +29,8 @@ * * Executed immediately after startup, drops capabilities we don't * need at any point during execution (or which we gain back when we - * need by joining other namespaces). + * need by joining other namespaces), and closes any leaked file we + * might have inherited from the parent process. * * 2. isolate_user() * ================= @@ -166,14 +167,17 @@ static void clamp_caps(void) }

/** - * isolate_initial() - Early, config independent self isolation + * isolate_initial() - Early, mostly config independent self isolation + * @argc: Argument count + * @argv: Command line options: only --fd (if present) is relevant here * * Should: * - drop unneeded capabilities + * - close all open files except for standard streams and the one from --fd * Musn't: * - remove filesytem access (we need to access files during setup) */ -void isolate_initial(void) +void isolate_initial(int argc, char **argv) { uint64_t keep;

@@ -207,6 +211,8 @@ void isolate_initial(void) keep |= BIT(CAP_SETFCAP) | BIT(CAP_SYS_PTRACE);

drop_caps_ep_except(keep); + + close_open_files(argc, argv); }

/** diff --git a/isolation.h b/isolation.h index 846b2af..80bb68d 100644 --- a/isolation.h +++ b/isolation.h @@ -7,7 +7,7 @@ #ifndef ISOLATION_H #define ISOLATION_H

-void isolate_initial(void); +void isolate_initial(int argc, char **argv); void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns, enum passt_modes mode); int isolate_prefork(const struct ctx *c); diff --git a/passt.c b/passt.c index ea5bece..4b3c306 100644 --- a/passt.c +++ b/passt.c @@ -211,7 +211,7 @@ int main(int argc, char **argv)

arch_avx2_exec(argv);

- isolate_initial(); + isolate_initial(argc, argv);

c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1;

diff --git a/util.c b/util.c index 07fb21c..7761bd3 100644 --- a/util.c +++ b/util.c @@ -26,6 +26,7 @@ #include #include #include +#include

#include "util.h" #include "iov.h" @@ -694,3 +695,43 @@ const char *str_ee_origin(const struct sock_extended_err *ee)

return "<invalid>"; } + +/** + * close_open_files() - Close leaked files, but not --fd, stdin, stdout, stderr + * @argc: Argument count + * @argv: Command line options, as we need to skip any file given via --fd + */ +void close_open_files(int argc, char **argv) +{ + const struct option optfd[] = { { "fd", required_argument, NULL, 'F' }, + { 0 }, + }; + long fd = -1; + int name, rc; + + do { + name = getopt_long(argc, argv, ":F", optfd, NULL); + + if (name == 'F') { + errno = 0; + fd = strtol(optarg, NULL, 0); + + if (errno || fd <= STDERR_FILENO || fd > INT_MAX) + die("Invalid --fd: %s", optarg); + } + } while (name != -1); + + if (fd == -1) { + rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE); + } else if (fd == STDERR_FILENO + 1) { /* Still a single range */ + rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE); + } else { + rc = close_range(STDERR_FILENO + 1, fd - 1, + CLOSE_RANGE_UNSHARE); + if (!rc) + rc = close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE); + } + + if (rc) + die_perror("Failed to close files leaked by parent"); +} diff --git a/util.h b/util.h index 83d2b53..cb4d181 100644 --- a/util.h +++ b/util.h @@ -183,6 +183,7 @@ int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); +void close_open_files(int argc, char **argv);

/** * af_name() - Return name of an address family

-- 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