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

[Stefano Brivio <sbrivio@redhat.com>]

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