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

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?


> -- e.g. group-writable paths under the home.
> - keep the real uid at 0 and change euid/egid only, so it stays
> reversible; setegid() before seteuid() on the way down, restore in
> reverse.
> - effective ids are process-wide, so this assumes the SSH command is
> not doing its I/O from more than one thread concurrently -- worth a
> quick check against the agent's dispatch model.
>

No issue there. All QGA commands are synchronous.


>
> The passwd lookup already runs before any of this, so it can stay
> outside the dropped region.
>
> Happy to re-run my PoC against the reworked version once it's ready and
> confirm the window is closed.
>
> Regards,
> Valentino Paulon
>


Best Regards,
Kostiantyn Kostiuk.


>
> On Tue, Jul 14, 2026 02:30 PM, "Daniel P. Berrangé" <[email protected]>
> wrote:
>
>> On Tue, Jul 14, 2026 at 02:28:35PM +0300, Kostiantyn Kostiuk wrote:
>> > On Tue, Jul 14, 2026 at 1:37 PM Daniel P. Berrangé <[email protected]
>> >
>> > wrote:
>> >
>> > > On Thu, Jul 09, 2026 at 01:57:07PM +0300, Kostiantyn Kostiuk wrote:
>> > > > Before this commit, when qmp_guest_ssh_add_authorized_keys adds an
>> > > > SSH key for an existing local user, the agent (running as root)
>> decides
>> > > > whether to create the user's .ssh directory with a symlink-following
>> > > > directory test, and then writes and chowns the authorized_keys file.
>> > > > A local unprivileged user who owns their home directory can
>> pre-stage
>> > > > their .ssh directory (or the authorized_keys file) as a symbolic
>> link
>> > > > so that, when the host or operator triggers a key add for that user,
>> > > > the root agent follows the link and transfers ownership of an
>> arbitrary
>> > > > root-owned file or directory to the unprivileged user, who can then
>> > > rewrite
>> > > > it to obtain root
>> > > >
>> > > > Fixes: CVE-2026-12080
>> > > > Fixes: https://gitlab.com/qemu-project/qemu/-/work_items/3929
>> > > >
>> > > > Reported-by: Valentino Paulon <[email protected]>
>> > > > Signed-off-by: Kostiantyn Kostiuk <[email protected]>
>> > > > ---
>> > > >  qga/commands-posix-ssh.c | 53
>> ++++++++++++++++++++++++++++++++++------
>> > > >  1 file changed, 45 insertions(+), 8 deletions(-)
>> > > >
>> > > > diff --git a/qga/commands-posix-ssh.c b/qga/commands-posix-ssh.c
>> > > > index 661972e34e..4e717d8ae8 100644
>> > > > --- a/qga/commands-posix-ssh.c
>> > > > +++ b/qga/commands-posix-ssh.c
>> > > > @@ -66,7 +66,7 @@ mkdir_for_user(const char *path, const struct
>> passwd
>> > > *p,
>> > > >          return false;
>> > > >      }
>> > > >
>> > > > -    if (chown(path, p->pw_uid, p->pw_gid) == -1) {
>> > > > +    if (lchown(path, p->pw_uid, p->pw_gid) == -1) {
>> > > >          error_setg_errno(errp, errno,
>> > > >                           "failed to set ownership of directory
>> '%s'",
>> > > >                           path);
>> > > > @@ -96,7 +96,7 @@ write_authkeys(const char *path, const GStrv keys,
>> > > >          return false;
>> > > >      }
>> > > >
>> > > > -    if (chown(path, p->pw_uid, p->pw_gid) == -1) {
>> > > > +    if (lchown(path, p->pw_uid, p->pw_gid) == -1) {
>> > > >          error_setg_errno(errp, errno,
>> > > >                           "failed to set ownership of directory
>> '%s'",
>> > > >                           path);
>> > > > @@ -123,6 +123,7 @@ qmp_guest_ssh_add_authorized_keys(const char
>> > > *username, strList *keys,
>> > > >      g_auto(GStrv) authkeys = NULL;
>> > > >      strList *k;
>> > > >      size_t nkeys, nauthkeys;
>> > > > +    int fd;
>> > > >
>> > > >      reset = has_reset && reset;
>> > > >
>> > > > @@ -138,15 +139,25 @@ qmp_guest_ssh_add_authorized_keys(const char
>> > > *username, strList *keys,
>> > > >      ssh_path = g_build_filename(p->pw_dir, ".ssh", NULL);
>> > > >      authkeys_path = g_build_filename(ssh_path, "authorized_keys",
>> NULL);
>> > > >
>> > > > +    fd = open(ssh_path, O_DIRECTORY | O_NOFOLLOW);
>> > > > +    if (fd == -1) {
>> > > > +        if (errno != ENOENT) {
>> > > > +            error_setg_errno(errp, errno, "failed to open directory
>> > > '%s'", ssh_path);
>> > > > +            return;
>> > > > +        }
>> > > > +    }
>> > >
>> > > IIUC, you're trying to protect against the possbility that
>> /home/fred/.ssh
>> > > is
>> > > a symlink to some other privileged directory.
>> > >
>> > > >      if (!reset) {
>> > > >          authkeys = read_authkeys(authkeys_path, NULL);
>> > > >      }
>> > > >      if (authkeys == NULL) {
>> > > > -        if (!g_file_test(ssh_path, G_FILE_TEST_IS_DIR) &&
>> > > > -            !mkdir_for_user(ssh_path, p, 0700, errp)) {
>> > > > +        if (fd == -1 && !mkdir_for_user(ssh_path, p, 0700, errp)) {
>> > > >              return;
>> > > >          }
>> > > >      }
>> > > > +    if (fd >= 0) {
>> > > > +        close(fd);
>> > > > +    }
>> > >
>> > > Does holding open an FD on a directory prevent that directory being
>> > > altered ?  Even if it prevents it being deleted, surely there's still
>> > > a race where the dir could be renamed, andd .ssh turned back into a
>> > > symlink ?
>> > >
>> >
>> > Yes, you are right; race is possible
>> >
>> >
>> > >
>> > > Rather than do these checks and switch chown->lchown, I wonder if we
>> > > are better off having the agent simply change its effective UID/GID
>> > > while it updates the SSH key files ? That way the agent would be
>> > > confined just like the user would be and we don't need to implement
>> > > special cases, nor would we have to think about race conditions.
>> > >
>> > >
>> > 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.
>>
>>
>> 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 :|
>>
>>