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:
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?
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.
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