Re: [PATCH v3] passt, util: Close any open file that the parent might have leaked

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 7 Aug 2024 13:26:32 +0200
Paul Holzinger <pholzing@redhat.com> wrote:

> On 07/08/2024 13:11, 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(),
> > while at it.
> >
> > Suggested-by: Paul Holzinger <pholzing@redhat.com>
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> > 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      |  3 ++-
> >   isolation.c | 12 +++++++++---
> >   isolation.h |  2 +-
> >   passt.c     |  2 +-
> >   util.c      | 38 ++++++++++++++++++++++++++++++++++++++
> >   util.h      |  1 +
> >   6 files changed, 52 insertions(+), 6 deletions(-)
> >
> > diff --git a/conf.c b/conf.c
> > index 14d8ece..5422813 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -1260,6 +1260,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);
> >   
> > @@ -1426,7 +1427,7 @@ void conf(struct ctx *c, int argc, char **argv)
> >   			errno = 0;
> >   			c->fd_tap = strtol(optarg, NULL, 0);
> >   
> > -			if (c->fd_tap < 0 || errno)
> > +			if (c->fd_tap < 0 || errno || c->fd_tap > INT_MAX)
> >   				die("Invalid --fd: %s", optarg);
> >   
> >   			c->one_off = true;
> > 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..9c6be6a 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -26,6 +26,7 @@
> >   #include <errno.h>
> >   #include <stdbool.h>
> >   #include <linux/errqueue.h>
> > +#include <getopt.h>
> >   
> >   #include "util.h"
> >   #include "iov.h"
> > @@ -694,3 +695,40 @@ 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;
> > +
> > +	do {
> > +		name = getopt_long(argc, argv, ":F", optfd, NULL);
> > +
> > +		if (name == 'F') {
> > +			errno = 0;
> > +			fd = strtol(optarg, NULL, 0);
> > +
> > +			if (fd < 0 || errno || fd > INT_MAX)
> > +				die("Invalid --fd: %s", optarg);
> > +		}
> > +	} while (name != -1);
> > +
> > +	if (fd == -1 || fd == 3) {
> > +		unsigned int first = (fd == 3) ? 4 : 3;
> > +
> > +		if (close_range(first,	~0U,	CLOSE_RANGE_UNSHARE))
> > +			die_perror("Failed to close files leaked by parent");
> > +	} else {
> > +		if (close_range(3,	fd - 1,	CLOSE_RANGE_UNSHARE) ||
> > +		    close_range(fd + 1,	~0U,	CLOSE_RANGE_UNSHARE))
> > +			die_perror("Failed to close files leaked by parent");
> > +	}  
> Sorry that I didn't mentioned this before but doesn't this still fail 
> when given fd 0, 1 or 2? I guess it should check if (fd < 3) in the 
> die(Invalid --fd) case, unless someone sees a reason to allow passing 
> the fd on stdio fds?

Oops, I missed your comment as I was sending v4. Right, that would also
be problematic.

I think the only semi-reasonable thing somebody could try is to close
our standard input, because we don't use it at all, and use that as
--fd. But it wouldn't work anyway at the moment, in any case where we
call __daemon(), because we would happily close that file descriptor.
Same for standard output and standard error anyway.

So, yeah, let's add the check you proposed, and should somebody ever
come up with a '--fd 0' use case, we'll handle that. v5 coming.

-- 
Stefano