Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands

Version 2 with setegid/seteuid logic pushed
Please review and test

On Mon, Aug 10, 2026 at 12:58 PM Daniel P. Berrangé <[email protected]>
wrote:

> On Mon, Aug 10, 2026 at 12:33:39PM +0300, Kostiantyn Kostiuk wrote:
> > On Mon, Aug 10, 2026 at 12:28 PM Daniel P. Berrangé <[email protected]
> >
> > wrote:
> >
> > > On Mon, Aug 10, 2026 at 11:58:44AM +0300, Kostiantyn Kostiuk wrote:
> > > > On Mon, Aug 10, 2026 at 11:55 AM Daniel P. Berrangé <
> [email protected]
> > > >
> > > > wrote:
> > > >
> > > > > On Mon, Aug 10, 2026 at 11:37:34AM +0300, Kostiantyn Kostiuk wrote:
> > > > > > On Mon, Aug 10, 2026 at 11:20 AM Daniel P. Berrangé <
> > > [email protected]
> > > > > >
> > > > > > wrote:
> > > > > >
> > > > > > > On Mon, Aug 10, 2026 at 11:06:27AM +0300, Kostiantyn Kostiuk
> wrote:
> > > > > > > > Hi Valentino,
> > > > > > > >
> > > > > > > > Sorry for the long delay.
> > > > > > > > I have a question for you
> > > > > > > >
> > > > > > > > On Tue, Jul 14, 2026 at 7:10 PM Valentino Paulon <
> > > > > > > > [email protected]> wrote:
> > > > > > > >
> > > > > > > > > > > So, you propose to call seteuid/setegid before any I/O
> > > > > operation?
> > > > > > > > > >
> > > > > > > > > > Yes, specifically for the SSH commands, because they're
> > > unusual
> > > > > in
> > > > > > > > > > that we're doing stuff on behalf of an unprivileged user.
> > > > > > > > >
> > > > > > > > > +1 on the euid/egid direction -- as the reporter that's the
> > > > > outcome I
> > > > > > > > > was hoping for. It confines every step (read, mkdir,
> create,
> > > > > rename,
> > > > > > > > > chown) to what the target user could already do himself,
> which
> > > > > removes
> > > > > > > > > the whole TOCTOU class instead of chasing one instance of
> it.
> > > > > > > > >
> > > > > > > > > A few implementation details that tend to bite with this
> > > pattern,
> > > > > in
> > > > > > > > > case they save a round-trip:
> > > > > > > > >
> > > > > > > > > - drop the supplementary groups too, not just egid:
> > > initgroups()
> > > > > (or
> > > > > > > > > setgroups() with the user's list) while still root,
> otherwise
> > > the
> > > > > > > > > agent's effective access won't match the user's in either
> > > direction
> > > > > > > > >
> > > > > > > >
> > > > > > > > Technically, there is no limitation to running QGA in
> > > unprivileged
> > > > > mode.
> > > > > > > > Normally, it is a system daemon, but this is not mandatory.
> > > > > > > > In this case, we can not call initgroups because the caller
> must
> > > > > > > > have CAP_SETGID.
> > > > > > > > We can call initgroups only when QGA runs as root. What do
> you
> > > think?
> > > > > > >
> > > > > > > In any practical sense it is a system daemon given the set of
> > > commands
> > > > > > > it is exposing. Thus, IMHO, deploying as an unprivileged user
> > > should
> > > > > > > be considered a broken deployment. We could make that explicit
> by
> > > > > > > refusing to launch.
> > > > > > >
> > > > > >
> > > > > > Makes sense, but we have one more issue with QGA unit tests in
> this
> > > case.
> > > > > > They are called by an unprivileged user (build user), and
> initgroups
> > > > > broke
> > > > > > it.
> > > > > > Is it possible to enter the namespace automatically by the test
> > > engine?
> > > > >
> > > > > We can't assume that is possible todo in all build/test
> environments.
> > > > >
> > > > > Doing things as different user accounts feels pretty dubious for
> the
> > > > > scope of a unit test too - that is really functional or integration
> > > > > testing.
> > > > >
> > > > > Can we somehow restructure the tests and/or agent code to avoid the
> > > > > changing groups stuff in unit tests.
> > > > >
> > > >
> > > > There are two possible options:
> > > > 1. add #ifdef QGA_BUILD_UNIT_TEST into qmp_guest_ssh_* functions
> > > > 2. check the current uid and skip initgroups if it is zero (my
> initial
> > > > question)
> > >
> > > Surely you mean "if it is non-zero" here, as it is with uid!=0 that
> > > the UID changes will fail.
> > >
> >
> > yes. So what sounds better: "ifdef" or "uid!=0"?
>
> I'd be inclined to #ifdef so we know it doesn't affect production
> deployments, only tests.
>
>
> With regards,
> Daniel
> --
> |: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
> |: https://libvirt.org          ~~          https://entangle-photo.org :|
> |: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|
>
>