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