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