[PATCH 0/2] Introduce --log-fd option

[PATCH 0/2] Introduce --log-fd option

[Michal Privoznik]

The idea behind this is so that libvirt can create the log file, set all
DAC/SELinux labels and just pass pre-opened FD to passt.

You can view my WIP patches for libvirt here:

https://gitlab.com/MichalPrivoznik/libvirt/-/commit/5bbe40087d6888a7c4031a2224731523e117c072

Michal Privoznik (2):
  util: Introduce set_cloexec()
  conf, log: Introduce --log-fd option

 conf.c  | 29 +++++++++++++++++++++++++----
 log.c   | 17 ++++++++++++-----
 log.h   |  2 +-
 passt.1 |  5 +++++
 util.c  | 17 +++++++++++++++++
 util.h  |  1 +
 6 files changed, 61 insertions(+), 10 deletions(-)

-- 
2.39.3

[PATCH 1/2] util: Introduce set_cloexec()

[Michal Privoznik]

This function is going to be used on FDs inherited from parent.
Parent can't set O_CLOEXEC obviously, so we have to set it
ourselves.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 util.c | 17 +++++++++++++++++
 util.h |  1 +
 2 files changed, 18 insertions(+)

diff --git a/util.c b/util.c
index 1d00404..9817c74 100644
--- a/util.c
+++ b/util.c
@@ -526,6 +526,23 @@ int write_file(const char *path, const char *buf)
 	return len == 0 ? 0 : -1;
 }
 
+/**
+ * set_cloexec() - Set Close-on-exec flag on given FD
+ * @fd:		FD to set the flag on
+ *
+ * Return: 0 on success, -1 on any error
+ */
+int set_cloexec(int fd)
+{
+	int fflags;
+	if ((fflags = fcntl(fd, F_GETFD)) < 0)
+		return -1;
+	fflags |= FD_CLOEXEC;
+	if ((fcntl(fd, F_SETFD, fflags)) < 0)
+		return -1;
+	return 0;
+}
+
 #ifdef __ia64__
 /* Needed by do_clone() below: glibc doesn't export the prototype of __clone2(),
  * use the description from clone(2).
diff --git a/util.h b/util.h
index 26892aa..3db901d 100644
--- a/util.h
+++ b/util.h
@@ -222,5 +222,6 @@ void write_pidfile(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);
+int set_cloexec(int fd);
 
 #endif /* UTIL_H */
-- 
@@ -222,5 +222,6 @@ void write_pidfile(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);
+int set_cloexec(int fd);
 
 #endif /* UTIL_H */
-- 
2.39.3

Re: [PATCH 1/2] util: Introduce set_cloexec()

[Stefano Brivio]

On Tue,  6 Jun 2023 13:41:29 +0200
Michal Privoznik <mprivozn@redhat.com> wrote:

> This function is going to be used on FDs inherited from parent.
> Parent can't set O_CLOEXEC obviously, so we have to set it
> ourselves.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  util.c | 17 +++++++++++++++++
>  util.h |  1 +
>  2 files changed, 18 insertions(+)
> 
> diff --git a/util.c b/util.c
> index 1d00404..9817c74 100644
> --- a/util.c
> +++ b/util.c
> @@ -526,6 +526,23 @@ int write_file(const char *path, const char *buf)
>  	return len == 0 ? 0 : -1;
>  }
>  
> +/**
> + * set_cloexec() - Set Close-on-exec flag on given FD
> + * @fd:		FD to set the flag on

Nits: "close-on-exec", "file descriptor".

> + *
> + * Return: 0 on success, -1 on any error

I'd rather return errno as set by fcntl(), for consistency.

> + */
> +int set_cloexec(int fd)
> +{
> +	int fflags;
> +	if ((fflags = fcntl(fd, F_GETFD)) < 0)
> +		return -1;
> +	fflags |= FD_CLOEXEC;
> +	if ((fcntl(fd, F_SETFD, fflags)) < 0)
> +		return -1;
> +	return 0;
> +}

...but in general, I'm not convinced this is really needed. O_CLOEXEC
has been the only file _descriptor_ flag for 16 years, and I have some
doubts another one will ever come up (and at that point, we wouldn't
probably want to have it set by default anyway).

I'd just set it with a direct call. This is not a strong preference
though.

-- 
Stefano

[PATCH 2/2] conf, log: Introduce --log-fd option

[Michal Privoznik]

So far, users/mgmt apps can use --log-file to specify log output.
This has a downside though - they have to set up permissions so
that passt/pasta can open the file. But we can do a bit better:
allow mgmt apps pass an pre-opened FD and write all log messages
into it.

This then allows the log file to be readable by very few users
(root:root even) or reside in a path which would be otherwise
inaccessible.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 conf.c  | 29 +++++++++++++++++++++++++----
 log.c   | 17 ++++++++++++-----
 log.h   |  2 +-
 passt.1 |  5 +++++
 4 files changed, 43 insertions(+), 10 deletions(-)

diff --git a/conf.c b/conf.c
index ffff235..59d0a8e 100644
--- a/conf.c
+++ b/conf.c
@@ -744,6 +744,7 @@ static void usage(const char *name)
 	info(   "  -e, --stderr		Log to stderr too");
 	info(   "    default: log to system logger only if started from a TTY");
 	info(   "  -l, --log-file PATH	Log (only) to given file");
+	info(	"  --log-fd FD		Log (only) to given file descriptor");
 	info(   "  --log-size BYTES	Maximum size of log file");
 	info(   "    default: 1 MiB");
 	info(   "  --runas UID|UID:GID 	Run as given UID, GID, which can be");
@@ -1181,6 +1182,7 @@ void conf(struct ctx *c, int argc, char **argv)
 		{"config-net",	no_argument,		NULL,		17 },
 		{"no-copy-routes", no_argument,		NULL,		18 },
 		{"no-copy-addrs", no_argument,		NULL,		19 },
+		{"log-fd", required_argument,		NULL,		20 },
 		{ 0 },
 	};
 	struct get_bound_ports_ns_arg ns_ports_arg = { .c = c };
@@ -1192,7 +1194,7 @@ void conf(struct ctx *c, int argc, char **argv)
 	struct in_addr *dns4 = c->ip4.dns;
 	unsigned int ifi4 = 0, ifi6 = 0;
 	const char *optstring;
-	int name, ret, b, i;
+	int name, ret, b, i, logfd = -1;
 	size_t logsize = 0;
 	uid_t uid;
 	gid_t gid;
@@ -1358,6 +1360,22 @@ void conf(struct ctx *c, int argc, char **argv)
 
 			warn("--no-copy-addrs will be dropped soon");
 			c->no_copy_addrs = 1;
+			break;
+		case 20:
+			if (logfile)
+				die("Either --log-file or --log-fd");
+
+			if (c->force_stderr)
+				die("Can't log to both stderr and file");
+
+			if (logfd >= 0)
+				die("Multiple --log-fd options given");
+
+			errno = 0;
+			logfd = strtol(optarg, NULL, 0);
+			if (logfd < 0 || errno)
+				die("Invalid --log-fd: %s", optarg);
+
 			break;
 		case 'd':
 			if (c->debug)
@@ -1369,7 +1387,7 @@ void conf(struct ctx *c, int argc, char **argv)
 			c->debug = 1;
 			break;
 		case 'e':
-			if (logfile)
+			if (logfile || logfd >= 0)
 				die("Can't log to both file and stderr");
 
 			if (c->force_stderr)
@@ -1381,6 +1399,9 @@ void conf(struct ctx *c, int argc, char **argv)
 			if (c->force_stderr)
 				die("Can't log to both stderr and file");
 
+			if (logfd >= 0)
+				die("Either --log-file or --log-fd");
+
 			if (logfile)
 				die("Multiple --log-file options given");
 
@@ -1659,9 +1680,9 @@ void conf(struct ctx *c, int argc, char **argv)
 
 	conf_ugid(runas, &uid, &gid);
 
-	if (logfile) {
+	if (logfile || logfd >= 0) {
 		logfile_init(c->mode == MODE_PASST ? "passt" : "pasta",
-			     logfile, logsize);
+			     logfile, logfd, logsize);
 	}
 
 	nl_sock_init(c, false);
diff --git a/log.c b/log.c
index 3a3d101..28929a7 100644
--- a/log.c
+++ b/log.c
@@ -174,9 +174,10 @@ void passt_vsyslog(int pri, const char *format, va_list ap)
  * logfile_init() - Open log file and write header with PID, version, path
  * @name:	Identifier for header: passt or pasta
  * @path:	Path to log file
+ * @logfd:	Pre-opened log file descriptor (if >= 0)
  * @size:	Maximum size of log file: log_cut_size is calculatd here
  */
-void logfile_init(const char *name, const char *path, size_t size)
+void logfile_init(const char *name, const char *path, int logfd, size_t size)
 {
 	char nl = '\n', exe[PATH_MAX] = { 0 };
 	int n;
@@ -186,10 +187,16 @@ void logfile_init(const char *name, const char *path, size_t size)
 		exit(EXIT_FAILURE);
 	}
 
-	log_file = open(path, O_CREAT | O_TRUNC | O_APPEND | O_RDWR | O_CLOEXEC,
-			S_IRUSR | S_IWUSR);
-	if (log_file == -1)
-		die("Couldn't open log file %s: %s", path, strerror(errno));
+	if (logfd >= 0) {
+		if (set_cloexec(logfd) < 0)
+			die("Could not set CLOEXEC flag on logfd", strerror(errno));
+		log_file = logfd;
+	} else {
+		log_file = open(path, O_CREAT | O_TRUNC | O_APPEND | O_RDWR | O_CLOEXEC,
+				S_IRUSR | S_IWUSR);
+		if (log_file == -1)
+			die("Couldn't open log file %s: %s", path, strerror(errno));
+	}
 
 	log_size = size ? size : LOGFILE_SIZE_DEFAULT;
 
diff --git a/log.h b/log.h
index 3aab29d..d96359a 100644
--- a/log.h
+++ b/log.h
@@ -30,7 +30,7 @@ void trace_init(int enable);
 	} while (0)
 
 void __openlog(const char *ident, int option, int facility);
-void logfile_init(const char *name, const char *path, size_t size);
+void logfile_init(const char *name, const char *path, int logfd, size_t size);
 void passt_vsyslog(int pri, const char *format, va_list ap);
 void logfile_write(int pri, const char *format, va_list ap);
 void __setlogmask(int mask);
diff --git a/passt.1 b/passt.1
index 1ad4276..ee5abbb 100644
--- a/passt.1
+++ b/passt.1
@@ -101,6 +101,11 @@ terminal, and to both system logger and standard error otherwise.
 .BR \-l ", " \-\-log-file " " \fIPATH\fR
 Log to file \fIPATH\fR, not to standard error, and not to the system logger.
 
+.TP
+.BR \-l ", " \-\-log-fd " " \fIFD\fR
+Log to pre-opened file descriptor \fIFD\fR, not to standard error, and not to
+the system logger.
+
 .TP
 .BR \-\-log-size " " \fISIZE\fR
 Limit log file size to \fISIZE\fR bytes. When the log file is full, make room
-- 
@@ -101,6 +101,11 @@ terminal, and to both system logger and standard error otherwise.
 .BR \-l ", " \-\-log-file " " \fIPATH\fR
 Log to file \fIPATH\fR, not to standard error, and not to the system logger.
 
+.TP
+.BR \-l ", " \-\-log-fd " " \fIFD\fR
+Log to pre-opened file descriptor \fIFD\fR, not to standard error, and not to
+the system logger.
+
 .TP
 .BR \-\-log-size " " \fISIZE\fR
 Limit log file size to \fISIZE\fR bytes. When the log file is full, make room
-- 
2.39.3

Re: [PATCH 2/2] conf, log: Introduce --log-fd option

[Stefano Brivio]

On Tue,  6 Jun 2023 13:41:30 +0200
Michal Privoznik <mprivozn@redhat.com> wrote:

> So far, users/mgmt apps can use --log-file to specify log output.
> This has a downside though - they have to set up permissions so
> that passt/pasta can open the file. But we can do a bit better:
> allow mgmt apps pass an pre-opened FD and write all log messages
> into it.
> 
> This then allows the log file to be readable by very few users
> (root:root even) or reside in a path which would be otherwise
> inaccessible.

Right... that's exactly the part that worries me (even though I
understand and appreciate the compatibility argument).

> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  conf.c  | 29 +++++++++++++++++++++++++----
>  log.c   | 17 ++++++++++++-----
>  log.h   |  2 +-
>  passt.1 |  5 +++++
>  4 files changed, 43 insertions(+), 10 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index ffff235..59d0a8e 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -744,6 +744,7 @@ static void usage(const char *name)
>  	info(   "  -e, --stderr		Log to stderr too");
>  	info(   "    default: log to system logger only if started from a TTY");
>  	info(   "  -l, --log-file PATH	Log (only) to given file");
> +	info(	"  --log-fd FD		Log (only) to given file descriptor");
>  	info(   "  --log-size BYTES	Maximum size of log file");
>  	info(   "    default: 1 MiB");
>  	info(   "  --runas UID|UID:GID 	Run as given UID, GID, which can be");
> @@ -1181,6 +1182,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  		{"config-net",	no_argument,		NULL,		17 },
>  		{"no-copy-routes", no_argument,		NULL,		18 },
>  		{"no-copy-addrs", no_argument,		NULL,		19 },
> +		{"log-fd", required_argument,		NULL,		20 },
>  		{ 0 },
>  	};
>  	struct get_bound_ports_ns_arg ns_ports_arg = { .c = c };
> @@ -1192,7 +1194,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  	struct in_addr *dns4 = c->ip4.dns;
>  	unsigned int ifi4 = 0, ifi6 = 0;
>  	const char *optstring;
> -	int name, ret, b, i;
> +	int name, ret, b, i, logfd = -1;

We use a reverse christmas tree convention for ordering local
variables (like Linux kernel network code does), rationale:
consistency, plus:
  https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyramids/

>  	size_t logsize = 0;
>  	uid_t uid;
>  	gid_t gid;
> @@ -1358,6 +1360,22 @@ void conf(struct ctx *c, int argc, char **argv)
>  
>  			warn("--no-copy-addrs will be dropped soon");
>  			c->no_copy_addrs = 1;
> +			break;
> +		case 20:
> +			if (logfile)
> +				die("Either --log-file or --log-fd");
> +
> +			if (c->force_stderr)
> +				die("Can't log to both stderr and file");
> +
> +			if (logfd >= 0)
> +				die("Multiple --log-fd options given");
> +
> +			errno = 0;
> +			logfd = strtol(optarg, NULL, 0);
> +			if (logfd < 0 || errno)
> +				die("Invalid --log-fd: %s", optarg);
> +
>  			break;
>  		case 'd':
>  			if (c->debug)
> @@ -1369,7 +1387,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  			c->debug = 1;
>  			break;
>  		case 'e':
> -			if (logfile)
> +			if (logfile || logfd >= 0)
>  				die("Can't log to both file and stderr");
>  
>  			if (c->force_stderr)
> @@ -1381,6 +1399,9 @@ void conf(struct ctx *c, int argc, char **argv)
>  			if (c->force_stderr)
>  				die("Can't log to both stderr and file");
>  
> +			if (logfd >= 0)
> +				die("Either --log-file or --log-fd");
> +
>  			if (logfile)
>  				die("Multiple --log-file options given");
>  
> @@ -1659,9 +1680,9 @@ void conf(struct ctx *c, int argc, char **argv)
>  
>  	conf_ugid(runas, &uid, &gid);
>  
> -	if (logfile) {
> +	if (logfile || logfd >= 0) {
>  		logfile_init(c->mode == MODE_PASST ? "passt" : "pasta",
> -			     logfile, logsize);
> +			     logfile, logfd, logsize);
>  	}
>  
>  	nl_sock_init(c, false);
> diff --git a/log.c b/log.c
> index 3a3d101..28929a7 100644
> --- a/log.c
> +++ b/log.c
> @@ -174,9 +174,10 @@ void passt_vsyslog(int pri, const char *format, va_list ap)
>   * logfile_init() - Open log file and write header with PID, version, path
>   * @name:	Identifier for header: passt or pasta
>   * @path:	Path to log file
> + * @logfd:	Pre-opened log file descriptor (if >= 0)
>   * @size:	Maximum size of log file: log_cut_size is calculatd here
>   */
> -void logfile_init(const char *name, const char *path, size_t size)
> +void logfile_init(const char *name, const char *path, int logfd, size_t size)
>  {
>  	char nl = '\n', exe[PATH_MAX] = { 0 };
>  	int n;
> @@ -186,10 +187,16 @@ void logfile_init(const char *name, const char *path, size_t size)
>  		exit(EXIT_FAILURE);
>  	}
>  
> -	log_file = open(path, O_CREAT | O_TRUNC | O_APPEND | O_RDWR | O_CLOEXEC,
> -			S_IRUSR | S_IWUSR);
> -	if (log_file == -1)
> -		die("Couldn't open log file %s: %s", path, strerror(errno));
> +	if (logfd >= 0) {

We should probably ftruncate() the referenced file to zero bytes, here,
because otherwise we're not guaranteeing the rotation with fallocate()
later.

> +		if (set_cloexec(logfd) < 0)
> +			die("Could not set CLOEXEC flag on logfd", strerror(errno));

Using errno from set_cloexec() is a bit fragile, in case somebody ever
adds something else to that function.

By the way, this,

> +		log_file = logfd;
> +	} else {
> +		log_file = open(path, O_CREAT | O_TRUNC | O_APPEND | O_RDWR | O_CLOEXEC,
> +				S_IRUSR | S_IWUSR);

this,

> +		if (log_file == -1)
> +			die("Couldn't open log file %s: %s", path, strerror(errno));

and this now exceed 80 columns.

-- 
Stefano

Re: [PATCH 0/2] Introduce --log-fd option

[Stefano Brivio]

Hi,

On Tue,  6 Jun 2023 13:41:28 +0200
Michal Privoznik <mprivozn@redhat.com> wrote:

> The idea behind this is so that libvirt can create the log file, set all
> DAC/SELinux labels and just pass pre-opened FD to passt.
> 
> You can view my WIP patches for libvirt here:
> 
> https://gitlab.com/MichalPrivoznik/libvirt/-/commit/5bbe40087d6888a7c4031a2224731523e117c072

Thanks for the implementation... and also for taking care of the
details!

I'm not really enthusiastic about the security implications of this
approach, but _if it's the only reasonable way to solve this_, I won't
certainly stand in the way of progress. The series looks mostly good to
me, I have only a few nits, reported in the single replies.

This adds a further interfacing mode between passt and the parent
process, which makes me a bit uncomfortable in general.

Specifically, if the parent process runs as root, this gives a rogue
passt process a way to write potentially unlimited amounts of data to
essentially any place (minus _some_ checks implemented by Linux Security
Modules). And a rogue passt process doesn't necessarily imply a rogue
parent, so this is additional surface.

Oh, and by the way of LSMs, we kind of bypass stuff like this.

For a non-rogue passt process, if the parent runs as root, I don't see
any additional attack scenario -- the parent could do anything it wants
anyway. But if the parent runs as regular user, there are a few
additional ways to cause passt to misbehave by passing in a file
descriptor that doesn't correspond to a file, or that's opened by other
processes, without any mediation by the filesystem (which is generally
speaking not under control of unprivileged users).

I'd rather much prefer the more common approach of defaulting, or
suggesting to the user, to write to a temporary filesystem, available
under most distributions under /run or /var/run. Is that really
unfeasible?

I'm thinking that libvirt already needs a specific directory for passt
to use (socket and PID files). What if logFile, other than an absolute
path, supported a relative path? This logic might perhaps apply to
other helpers or external programs too.

By the way, passt ships with AppArmor and SELinux example policies,
which are also included in packages. They would need at least a quick
review, probably some edits, and some basic tests. Thinking of those, a
relative path would also simplify things.

-- 
Stefano

Re: [PATCH 0/2] Introduce --log-fd option

[Michal Prívozník]

On 6/6/23 22:58, Stefano Brivio wrote:
> Hi,
> 
> On Tue,  6 Jun 2023 13:41:28 +0200
> Michal Privoznik <mprivozn@redhat.com> wrote:
> 
>> The idea behind this is so that libvirt can create the log file, set all
>> DAC/SELinux labels and just pass pre-opened FD to passt.
>>
>> You can view my WIP patches for libvirt here:
>>
>> https://gitlab.com/MichalPrivoznik/libvirt/-/commit/5bbe40087d6888a7c4031a2224731523e117c072
> 
> Thanks for the implementation... and also for taking care of the
> details!
> 
> I'm not really enthusiastic about the security implications of this
> approach, but _if it's the only reasonable way to solve this_, I won't
> certainly stand in the way of progress. The series looks mostly good to
> me, I have only a few nits, reported in the single replies.
> 
> This adds a further interfacing mode between passt and the parent
> process, which makes me a bit uncomfortable in general.
> 
> Specifically, if the parent process runs as root, this gives a rogue
> passt process a way to write potentially unlimited amounts of data to
> essentially any place (minus _some_ checks implemented by Linux Security
> Modules). And a rogue passt process doesn't necessarily imply a rogue
> parent, so this is additional surface.

Well, is it any different to when libvirt would create the file, set
perms and then let passt open() it? In fact, I find passing FD safer
because libvirt doesn't need to set up perms/owner and can leave the log
file be owned by root:root with 0600 mode, or any other user that
libvirtd runs under.

> 
> Oh, and by the way of LSMs, we kind of bypass stuff like this.
> 
> For a non-rogue passt process, if the parent runs as root, I don't see
> any additional attack scenario -- the parent could do anything it wants
> anyway. But if the parent runs as regular user, there are a few
> additional ways to cause passt to misbehave by passing in a file
> descriptor that doesn't correspond to a file, or that's opened by other
> processes, without any mediation by the filesystem (which is generally
> speaking not under control of unprivileged users).

So an attacker can cause libvirtd to pass an FD that doesn't belong to
the log file opened by libvirtd? Interesting, I though that's
impossible. I mean, it's sort of a goal we're working towards with QEMU
- libvirt opens FDs and then just pass them to QEMU. So if it's really
unsafe, we should re-evaluate our goal.

> 
> I'd rather much prefer the more common approach of defaulting, or
> suggesting to the user, to write to a temporary filesystem, available
> under most distributions under /run or /var/run. Is that really
> unfeasible?

It if feasible. I just thought that when users want their logs to reside
under some special path that libvirtd has access to, but not passt then
we might use FD passing.

> 
> I'm thinking that libvirt already needs a specific directory for passt
> to use (socket and PID files). What if logFile, other than an absolute
> path, supported a relative path? This logic might perhaps apply to
> other helpers or external programs too.

That's tangent to this problem.

> 
> By the way, passt ships with AppArmor and SELinux example policies,
> which are also included in packages. They would need at least a quick
> review, probably some edits, and some basic tests. Thinking of those, a
> relative path would also simplify things.
> 

Ah, completely forgot about those. Yeah, they might need tweaking even
if we decide to go this route.

Michal

Re: [PATCH 0/2] Introduce --log-fd option

[Stefano Brivio]

On Wed, 7 Jun 2023 13:38:12 +0200
Michal Prívozník <mprivozn@redhat.com> wrote:

> On 6/6/23 22:58, Stefano Brivio wrote:
> > Hi,
> > 
> > On Tue,  6 Jun 2023 13:41:28 +0200
> > Michal Privoznik <mprivozn@redhat.com> wrote:
> >   
> >> The idea behind this is so that libvirt can create the log file, set all
> >> DAC/SELinux labels and just pass pre-opened FD to passt.
> >>
> >> You can view my WIP patches for libvirt here:
> >>
> >> https://gitlab.com/MichalPrivoznik/libvirt/-/commit/5bbe40087d6888a7c4031a2224731523e117c072  
> > 
> > Thanks for the implementation... and also for taking care of the
> > details!
> > 
> > I'm not really enthusiastic about the security implications of this
> > approach, but _if it's the only reasonable way to solve this_, I won't
> > certainly stand in the way of progress. The series looks mostly good to
> > me, I have only a few nits, reported in the single replies.
> > 
> > This adds a further interfacing mode between passt and the parent
> > process, which makes me a bit uncomfortable in general.
> > 
> > Specifically, if the parent process runs as root, this gives a rogue
> > passt process a way to write potentially unlimited amounts of data to
> > essentially any place (minus _some_ checks implemented by Linux Security
> > Modules). And a rogue passt process doesn't necessarily imply a rogue
> > parent, so this is additional surface.  
> 
> Well, is it any different to when libvirt would create the file, set
> perms and then let passt open() it?

Yes -- I think it would be appropriate that the file is not opened as
root, because access control (also on behalf of LSMs) logically happens
on open(). It's passt using that file, so it should own it and be
accounted for it (also in terms of disk quota), not libvirt. On the
other hand:

> In fact, I find passing FD safer because libvirt doesn't need to set
> up perms/owner and can leave the log file be owned by root:root with
> 0600 mode,

...also true, even though:

> or any other user that libvirtd runs under.

in that case, the file would be already, naturally, owned by that
(non-root) user and created with 0600 permissions anyway.

The issue here in some sense is that libvirtd is commonly (?) running
as root. To keep it simple, in the Podman integration (for pasta), we
only enabled pasta (same binary as passt) to be used if Podman also
runs "rootless". But, oops:
  https://github.com/containers/podman/issues/17840

This makes me realise another point though: if libvirtd runs as root,
at least in the current integration, or at least by default (?), passt
will run with the same user as QEMU (usually "qemu") -- not "nobody".
And at this point I'm not entirely sure that having a log file owned by
root is much preferable to having it owned by that "qemu" user.

> > Oh, and by the way of LSMs, we kind of bypass stuff like this.
> > 
> > For a non-rogue passt process, if the parent runs as root, I don't see
> > any additional attack scenario -- the parent could do anything it wants
> > anyway. But if the parent runs as regular user, there are a few
> > additional ways to cause passt to misbehave by passing in a file
> > descriptor that doesn't correspond to a file, or that's opened by other
> > processes, without any mediation by the filesystem (which is generally
> > speaking not under control of unprivileged users).  
> 
> So an attacker can cause libvirtd to pass an FD that doesn't belong to
> the log file opened by libvirtd?

No, no, by "[not] a file" I really meant [not] a regular file (say, a
TTY), and not necessarily libvirtd -- or a rogue libvirtd. All I'm
saying is that if you have control over the log file descriptor,
outside passt, as regular user, you might also get control over the
behaviour of passt, without any mediation by the filesystem.

> Interesting, I though that's impossible.

Well, strictly speaking it's not, just use a tracer, but then at that
point one would gain nothing additional from it.

> I mean, it's sort of a goal we're working towards with QEMU
> - libvirt opens FDs and then just pass them to QEMU. So if it's really
> unsafe, we should re-evaluate our goal.

To me that also poses the problem that if a LSM policy or VFS-based
access control forbids QEMU to access a resource, you are effectively
bypassing that -- and you're also bypassing whatever flag QEMU would
normally use on open(). But I don't know in which cases this is being
done and if it's actually a problem (or a bigger problem than the
solution it offers).

> > I'd rather much prefer the more common approach of defaulting, or
> > suggesting to the user, to write to a temporary filesystem, available
> > under most distributions under /run or /var/run. Is that really
> > unfeasible?  
> 
> It if feasible. I just thought that when users want their logs to reside
> under some special path that libvirtd has access to, but not passt then
> we might use FD passing.

On the other hand (and sure, from a user perspective this is different)
we don't allow passt to save its PID file or create its socket in
whatever place. But I see the usability point you're making and I think
it has its value too.

Long story short, if you think (you know better) that users would
commonly run libvirtd as root and request that passt writes its log
file to an arbitrary location, even if we offer a different default
(including a relative path) or somehow recommend something else, I think
we should go ahead with this. Otherwise I'm mildly against it.

> > I'm thinking that libvirt already needs a specific directory for passt
> > to use (socket and PID files). What if logFile, other than an absolute
> > path, supported a relative path? This logic might perhaps apply to
> > other helpers or external programs too.  
> 
> That's tangent to this problem.

Why? I think this also reflects on usability, and might have some weight
on the previous point.

> > By the way, passt ships with AppArmor and SELinux example policies,
> > which are also included in packages. They would need at least a quick
> > review, probably some edits, and some basic tests. Thinking of those, a
> > relative path would also simplify things.
> 
> Ah, completely forgot about those. Yeah, they might need tweaking even
> if we decide to go this route.

Either way, do you plan to take care of those? I can, but not right
away.

-- 
Stefano

Re: [PATCH 0/2] Introduce --log-fd option

[Michal Prívozník]

On 6/7/23 16:42, Stefano Brivio wrote:
> On Wed, 7 Jun 2023 13:38:12 +0200
> Michal Prívozník <mprivozn@redhat.com> wrote:
> 
>> On 6/6/23 22:58, Stefano Brivio wrote:
>>> Hi,
>>>
>>> On Tue,  6 Jun 2023 13:41:28 +0200
>>> Michal Privoznik <mprivozn@redhat.com> wrote:
>>>   
>>>> The idea behind this is so that libvirt can create the log file, set all
>>>> DAC/SELinux labels and just pass pre-opened FD to passt.
>>>>
>>>> You can view my WIP patches for libvirt here:
>>>>
>>>> https://gitlab.com/MichalPrivoznik/libvirt/-/commit/5bbe40087d6888a7c4031a2224731523e117c072  
>>>
>>> Thanks for the implementation... and also for taking care of the
>>> details!
>>>
>>> I'm not really enthusiastic about the security implications of this
>>> approach, but _if it's the only reasonable way to solve this_, I won't
>>> certainly stand in the way of progress. The series looks mostly good to
>>> me, I have only a few nits, reported in the single replies.
>>>
>>> This adds a further interfacing mode between passt and the parent
>>> process, which makes me a bit uncomfortable in general.
>>>
>>> Specifically, if the parent process runs as root, this gives a rogue
>>> passt process a way to write potentially unlimited amounts of data to
>>> essentially any place (minus _some_ checks implemented by Linux Security
>>> Modules). And a rogue passt process doesn't necessarily imply a rogue
>>> parent, so this is additional surface.  
>>
>> Well, is it any different to when libvirt would create the file, set
>> perms and then let passt open() it?
> 
> Yes -- I think it would be appropriate that the file is not opened as
> root, because access control (also on behalf of LSMs) logically happens
> on open(). It's passt using that file, so it should own it and be
> accounted for it (also in terms of disk quota), not libvirt. On the
> other hand:
> 
>> In fact, I find passing FD safer because libvirt doesn't need to set
>> up perms/owner and can leave the log file be owned by root:root with
>> 0600 mode,
> 
> ...also true, even though:
> 
>> or any other user that libvirtd runs under.
> 
> in that case, the file would be already, naturally, owned by that
> (non-root) user and created with 0600 permissions anyway.
> 
> The issue here in some sense is that libvirtd is commonly (?) running
> as root. To keep it simple, in the Podman integration (for pasta), we
> only enabled pasta (same binary as passt) to be used if Podman also
> runs "rootless". But, oops:
>   https://github.com/containers/podman/issues/17840
> 
> This makes me realise another point though: if libvirtd runs as root,
> at least in the current integration, or at least by default (?), passt
> will run with the same user as QEMU (usually "qemu") -- not "nobody".
> And at this point I'm not entirely sure that having a log file owned by
> root is much preferable to having it owned by that "qemu" user.
> 
>>> Oh, and by the way of LSMs, we kind of bypass stuff like this.
>>>
>>> For a non-rogue passt process, if the parent runs as root, I don't see
>>> any additional attack scenario -- the parent could do anything it wants
>>> anyway. But if the parent runs as regular user, there are a few
>>> additional ways to cause passt to misbehave by passing in a file
>>> descriptor that doesn't correspond to a file, or that's opened by other
>>> processes, without any mediation by the filesystem (which is generally
>>> speaking not under control of unprivileged users).  
>>
>> So an attacker can cause libvirtd to pass an FD that doesn't belong to
>> the log file opened by libvirtd?
> 
> No, no, by "[not] a file" I really meant [not] a regular file (say, a
> TTY), and not necessarily libvirtd -- or a rogue libvirtd. All I'm
> saying is that if you have control over the log file descriptor,
> outside passt, as regular user, you might also get control over the
> behaviour of passt, without any mediation by the filesystem.
> 
>> Interesting, I though that's impossible.
> 
> Well, strictly speaking it's not, just use a tracer, but then at that
> point one would gain nothing additional from it.
> 
>> I mean, it's sort of a goal we're working towards with QEMU
>> - libvirt opens FDs and then just pass them to QEMU. So if it's really
>> unsafe, we should re-evaluate our goal.
> 
> To me that also poses the problem that if a LSM policy or VFS-based
> access control forbids QEMU to access a resource, you are effectively
> bypassing that -- and you're also bypassing whatever flag QEMU would
> normally use on open(). But I don't know in which cases this is being
> done and if it's actually a problem (or a bigger problem than the
> solution it offers).
> 
>>> I'd rather much prefer the more common approach of defaulting, or
>>> suggesting to the user, to write to a temporary filesystem, available
>>> under most distributions under /run or /var/run. Is that really
>>> unfeasible?  
>>
>> It if feasible. I just thought that when users want their logs to reside
>> under some special path that libvirtd has access to, but not passt then
>> we might use FD passing.
> 
> On the other hand (and sure, from a user perspective this is different)
> we don't allow passt to save its PID file or create its socket in
> whatever place. But I see the usability point you're making and I think
> it has its value too.
> 
> Long story short, if you think (you know better) that users would
> commonly run libvirtd as root and request that passt writes its log
> file to an arbitrary location, even if we offer a different default
> (including a relative path) or somehow recommend something else, I think
> we should go ahead with this. Otherwise I'm mildly against it.

I don't think that passt advantage shows when running libvirtd as root.
There are other (better?) ways to provide network connectivity in that
case. The main advantage shows when running unprivileged in which case
the log file is going to be owned by current user (which is the same
user that QEMU will run under).

> 
>>> I'm thinking that libvirt already needs a specific directory for passt
>>> to use (socket and PID files). What if logFile, other than an absolute
>>> path, supported a relative path? This logic might perhaps apply to
>>> other helpers or external programs too.  
>>
>> That's tangent to this problem.
> 
> Why? I think this also reflects on usability, and might have some weight
> on the previous point.

Well, libvirt would firstly need to expose the domain specific path
somewhere, so that users know the prefix that the relative path refers
to. Secondly, libvirt must then NOT remove the temporary path, so that
the log file is preserved even after domain is shut off (e.g. somebody
might be interested in log messages that happen when QEMU disconnects).

IOW, it is tangent.

> 
>>> By the way, passt ships with AppArmor and SELinux example policies,
>>> which are also included in packages. They would need at least a quick
>>> review, probably some edits, and some basic tests. Thinking of those, a
>>> relative path would also simplify things.
>>
>> Ah, completely forgot about those. Yeah, they might need tweaking even
>> if we decide to go this route.
> 
> Either way, do you plan to take care of those? I can, but not right
> away.
> 

Nah, let's discard this. I didn't realize that you want the extra guard
of open(). So I'll just make libvirt create an empty file, set perms on
it and continue using --log-file.

Thanks anyway!

Michal

Re: [PATCH 0/2] Introduce --log-fd option

[Stefano Brivio]

On Thu, 8 Jun 2023 13:51:44 +0200
Michal Prívozník <mprivozn@redhat.com> wrote:

> On 6/7/23 16:42, Stefano Brivio wrote:
> > On Wed, 7 Jun 2023 13:38:12 +0200
> > Michal Prívozník <mprivozn@redhat.com> wrote:
> >   
> >> On 6/6/23 22:58, Stefano Brivio wrote:  
> >>> Hi,
> >>>
> >>> On Tue,  6 Jun 2023 13:41:28 +0200
> >>> Michal Privoznik <mprivozn@redhat.com> wrote:
> >>>     
> >>>> The idea behind this is so that libvirt can create the log file, set all
> >>>> DAC/SELinux labels and just pass pre-opened FD to passt.
> >>>>
> >>>> You can view my WIP patches for libvirt here:
> >>>>
> >>>> https://gitlab.com/MichalPrivoznik/libvirt/-/commit/5bbe40087d6888a7c4031a2224731523e117c072    
> >>>
> >>> Thanks for the implementation... and also for taking care of the
> >>> details!
> >>>
> >>> I'm not really enthusiastic about the security implications of this
> >>> approach, but _if it's the only reasonable way to solve this_, I won't
> >>> certainly stand in the way of progress. The series looks mostly good to
> >>> me, I have only a few nits, reported in the single replies.
> >>>
> >>> This adds a further interfacing mode between passt and the parent
> >>> process, which makes me a bit uncomfortable in general.
> >>>
> >>> Specifically, if the parent process runs as root, this gives a rogue
> >>> passt process a way to write potentially unlimited amounts of data to
> >>> essentially any place (minus _some_ checks implemented by Linux Security
> >>> Modules). And a rogue passt process doesn't necessarily imply a rogue
> >>> parent, so this is additional surface.    
> >>
> >> Well, is it any different to when libvirt would create the file, set
> >> perms and then let passt open() it?  
> > 
> > Yes -- I think it would be appropriate that the file is not opened as
> > root, because access control (also on behalf of LSMs) logically happens
> > on open(). It's passt using that file, so it should own it and be
> > accounted for it (also in terms of disk quota), not libvirt. On the
> > other hand:
> >   
> >> In fact, I find passing FD safer because libvirt doesn't need to set
> >> up perms/owner and can leave the log file be owned by root:root with
> >> 0600 mode,  
> > 
> > ...also true, even though:
> >   
> >> or any other user that libvirtd runs under.  
> > 
> > in that case, the file would be already, naturally, owned by that
> > (non-root) user and created with 0600 permissions anyway.
> > 
> > The issue here in some sense is that libvirtd is commonly (?) running
> > as root. To keep it simple, in the Podman integration (for pasta), we
> > only enabled pasta (same binary as passt) to be used if Podman also
> > runs "rootless". But, oops:
> >   https://github.com/containers/podman/issues/17840
> > 
> > This makes me realise another point though: if libvirtd runs as root,
> > at least in the current integration, or at least by default (?), passt
> > will run with the same user as QEMU (usually "qemu") -- not "nobody".
> > And at this point I'm not entirely sure that having a log file owned by
> > root is much preferable to having it owned by that "qemu" user.
> >   
> >>> Oh, and by the way of LSMs, we kind of bypass stuff like this.
> >>>
> >>> For a non-rogue passt process, if the parent runs as root, I don't see
> >>> any additional attack scenario -- the parent could do anything it wants
> >>> anyway. But if the parent runs as regular user, there are a few
> >>> additional ways to cause passt to misbehave by passing in a file
> >>> descriptor that doesn't correspond to a file, or that's opened by other
> >>> processes, without any mediation by the filesystem (which is generally
> >>> speaking not under control of unprivileged users).    
> >>
> >> So an attacker can cause libvirtd to pass an FD that doesn't belong to
> >> the log file opened by libvirtd?  
> > 
> > No, no, by "[not] a file" I really meant [not] a regular file (say, a
> > TTY), and not necessarily libvirtd -- or a rogue libvirtd. All I'm
> > saying is that if you have control over the log file descriptor,
> > outside passt, as regular user, you might also get control over the
> > behaviour of passt, without any mediation by the filesystem.
> >   
> >> Interesting, I though that's impossible.  
> > 
> > Well, strictly speaking it's not, just use a tracer, but then at that
> > point one would gain nothing additional from it.
> >   
> >> I mean, it's sort of a goal we're working towards with QEMU
> >> - libvirt opens FDs and then just pass them to QEMU. So if it's really
> >> unsafe, we should re-evaluate our goal.  
> > 
> > To me that also poses the problem that if a LSM policy or VFS-based
> > access control forbids QEMU to access a resource, you are effectively
> > bypassing that -- and you're also bypassing whatever flag QEMU would
> > normally use on open(). But I don't know in which cases this is being
> > done and if it's actually a problem (or a bigger problem than the
> > solution it offers).
> >   
> >>> I'd rather much prefer the more common approach of defaulting, or
> >>> suggesting to the user, to write to a temporary filesystem, available
> >>> under most distributions under /run or /var/run. Is that really
> >>> unfeasible?    
> >>
> >> It if feasible. I just thought that when users want their logs to reside
> >> under some special path that libvirtd has access to, but not passt then
> >> we might use FD passing.  
> > 
> > On the other hand (and sure, from a user perspective this is different)
> > we don't allow passt to save its PID file or create its socket in
> > whatever place. But I see the usability point you're making and I think
> > it has its value too.
> > 
> > Long story short, if you think (you know better) that users would
> > commonly run libvirtd as root and request that passt writes its log
> > file to an arbitrary location, even if we offer a different default
> > (including a relative path) or somehow recommend something else, I think
> > we should go ahead with this. Otherwise I'm mildly against it.  
> 
> I don't think that passt advantage shows when running libvirtd as root.
> There are other (better?) ways to provide network connectivity in that
> case.

Well, the default option is a bridge that offers quite little network
isolation. While libvirt's default firewalling rules should prevent any
spoofing, they are added on top, instead of being there by design. Same
with preventing arbitrary IP (not TCP, not UDP, not ICMP) traffic -- on
the other hand that might be a reason to use the bridge, if desired.

There are a few other advantages using passt: you don't need NAT, which
avoids the need for NDP/ARP proxying (a bit cumbersome with WiFi), and
it might be the only way to run some workloads.

Performance-wise, the default tap device gets a single queue, and with
a few filtering rules (for the bridged tap) passt actually gives better
throughput from some quick tests. We're also working on vhost-user
support, which should improve the situation further.

So, sure, there are valid reasons to use a bridged tap device, but they
don't necessarily apply whenever libvirtd (for other reasons) needs to
run as root.

> The main advantage shows when running unprivileged in which case
> the log file is going to be owned by current user (which is the same
> user that QEMU will run under).
> 
> >   
> >>> I'm thinking that libvirt already needs a specific directory for passt
> >>> to use (socket and PID files). What if logFile, other than an absolute
> >>> path, supported a relative path? This logic might perhaps apply to
> >>> other helpers or external programs too.    
> >>
> >> That's tangent to this problem.  
> > 
> > Why? I think this also reflects on usability, and might have some weight
> > on the previous point.  
> 
> Well, libvirt would firstly need to expose the domain specific path
> somewhere, so that users know the prefix that the relative path refers
> to. Secondly, libvirt must then NOT remove the temporary path, so that
> the log file is preserved even after domain is shut off (e.g. somebody
> might be interested in log messages that happen when QEMU disconnects).

Right. My only slightly informed opinion would suggest that this is
actually more elegant/fitting, but I don't know how complicated it is to
implement (especially the first point).

> IOW, it is tangent.
> 
> >>> By the way, passt ships with AppArmor and SELinux example policies,
> >>> which are also included in packages. They would need at least a quick
> >>> review, probably some edits, and some basic tests. Thinking of those, a
> >>> relative path would also simplify things.  
> >>
> >> Ah, completely forgot about those. Yeah, they might need tweaking even
> >> if we decide to go this route.  
> > 
> > Either way, do you plan to take care of those? I can, but not right
> > away.
> 
> Nah, let's discard this. I didn't realize that you want the extra guard
> of open(). So I'll just make libvirt create an empty file, set perms on
> it and continue using --log-file.

Sorry for the extra work, but yes, I think forcing passt to open() the
file is quite relevant.

> Thanks anyway!

And thanks for the tentative patches!

-- 
Stefano