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

["Daniel P. Berrangé" <berrange@redhat.com>]

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é <berrange@redhat.com>
> 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 <valentino.paulon88@gmail.com>
> > > Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> > > ---
> > >  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 :|