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

["Michal Prívozník" <mprivozn@redhat.com>]

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