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