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/5bbe40087d6888a7c4031a2… 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
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(a)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 */
--
2.39.3
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(a)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
--
2.39.3
Hi, On Tue, 6 Jun 2023 13:41:28 +0200 Michal Privoznik <mprivozn(a)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/5bbe40087d6888a7c4031a2…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
On Tue, 6 Jun 2023 13:41:29 +0200 Michal Privoznik <mprivozn(a)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(a)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 onNits: "close-on-exec", "file descriptor".+ * + * Return: 0 on success, -1 on any errorI'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
On Tue, 6 Jun 2023 13:41:30 +0200 Michal Privoznik <mprivozn(a)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(a)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
On 6/6/23 22:58, Stefano Brivio wrote:Hi, On Tue, 6 Jun 2023 13:41:28 +0200 Michal Privoznik <mprivozn(a)redhat.com> wrote: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.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/5bbe40087d6888a7c4031a2…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).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
On Wed, 7 Jun 2023 13:38:12 +0200 Michal Prívozník <mprivozn(a)redhat.com> wrote:On 6/6/23 22:58, Stefano Brivio wrote: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:Hi, On Tue, 6 Jun 2023 13:41:28 +0200 Michal Privoznik <mprivozn(a)redhat.com> wrote:Well, is it any different to when libvirt would create the file, set perms and then let passt open() it?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/5bbe40087d6888a7c4031a2…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.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.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.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.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).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'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.Why? I think this also reflects on usability, and might have some weight on the previous point.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.Either way, do you plan to take care of those? I can, but not right away. -- StefanoBy 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.
On 6/7/23 16:42, Stefano Brivio wrote:On Wed, 7 Jun 2023 13:38:12 +0200 Michal Prívozník <mprivozn(a)redhat.com> wrote: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).On 6/6/23 22:58, Stefano Brivio wrote: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:Hi, On Tue, 6 Jun 2023 13:41:28 +0200 Michal Privoznik <mprivozn(a)redhat.com> wrote:Well, is it any different to when libvirt would create the file, set perms and then let passt open() it?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/5bbe40087d6888a7c4031a2…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.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.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.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.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).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'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.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.Why? I think this also reflects on usability, and might have some weight on the previous point.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.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! MichalEither way, do you plan to take care of those? I can, but not right away.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.
On Thu, 8 Jun 2023 13:51:44 +0200 Michal Prívozník <mprivozn(a)redhat.com> wrote:On 6/7/23 16:42, Stefano Brivio wrote: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.On Wed, 7 Jun 2023 13:38:12 +0200 Michal Prívozník <mprivozn(a)redhat.com> wrote: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.On 6/6/23 22:58, Stefano Brivio wrote: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:Hi, On Tue, 6 Jun 2023 13:41:28 +0200 Michal Privoznik <mprivozn(a)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/5bbe40087d6888a7c4031a2… 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,...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.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.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.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).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'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.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).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).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).Why? I think this also reflects on usability, and might have some weight on the previous point.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.IOW, it is tangent.Sorry for the extra work, but yes, I think forcing passt to open() the file is quite relevant.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.Either way, do you plan to take care of those? I can, but not right away.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.Thanks anyway!And thanks for the tentative patches! -- Stefano