Re: [PATCH v2] qga: Change effective user/group ID in guest-ssh-* commands

On Mon, Aug 10, 2026 at 02:01:53PM +0100, Peter Maydell wrote:
> On Mon, 10 Aug 2026 at 11:47, Kostiantyn Kostiuk <[email protected]> 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
> >
> > v1: https://patchew.org/QEMU/[email protected]/
> > v2 -> v1:
> > Change effective user/group ID instead of checking for symlinks and
> > changing ownership of the file.
> >
> > Reported-by: Valentino Paulon <[email protected]>
> > Signed-off-by: Kostiantyn Kostiuk <[email protected]>
> 
> 
> 
> > @@ -135,6 +139,29 @@ qmp_guest_ssh_add_authorized_keys(const char *username, strList *keys,
> >          return;
> >      }
> >
> > +    euid = geteuid();
> > +    egid = getegid();
> > +#ifndef QGA_BUILD_UNIT_TEST
> > +    /* The initgroups requires CAP_SETGID. During build time unit tests, we can't do this. */
> > +    if (initgroups(p->pw_name, p->pw_gid) == -1) {
> > +        error_setg_errno(errp, errno, "failed to set group for user '%s'",
> > +                         p->pw_name);
> > +        return;
> > +    }
> > +#endif
> > +    if (setegid(p->pw_gid) == -1) {
> > +        error_setg_errno(errp, errno, "failed to set effective group ID for user '%s'",
> > +                         p->pw_name);
> > +        return;
> > +    }
> > +    if (seteuid(p->pw_uid) == -1) {
> > +        error_setg_errno(errp, errno, "failed to set effective user ID for user '%s'",
> > +                         p->pw_name);
> > +        /* Ignore errors, we can't do anything in this case */
> > +        unused_egid = setegid(egid);
> > +        return;
> > +    }
> > +
> 
> This identical bit of logic appears in three different places in this
> patch, plus we need to do the "clean it up" logic at every place we
> could return from each of the three functions. (Incidentally I think
> you've missed some of those in the remove function.)
> 
> I think it would be helpful to at least abstract out the duplicate logic
> for setup and cleanup. Maybe we should also consider using the glib
> autoptr-cleanup macros so that a set_privs_to_user() function can
> return a struct that has the old euid/egid, and then the cleanup of
> that struct enforces the "return to those IDs on function exit".

Perhaps do it as a callback, so all privs handling is isolated in
a single helper

 int run_as_user(uid_t uid, gid_t gid, int(*callback)(void *opaque));




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 :|