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

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

On Mon, Aug 10, 2026 at 01:46:59PM +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
> 
> v1: https://patchew.org/QEMU/20260709105707.91209-1-kkostiuk@redhat.com/
> v2 -> v1:
> Change effective user/group ID instead of checking for symlinks and
> changing ownership of the file.
> 
> Reported-by: Valentino Paulon <valentino.paulon88@gmail.com>
> Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> ---
>  qga/commands-posix-ssh.c | 94 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
> 
> diff --git a/qga/commands-posix-ssh.c b/qga/commands-posix-ssh.c
> index 661972e34e..9a3507fda4 100644
> --- a/qga/commands-posix-ssh.c
> +++ b/qga/commands-posix-ssh.c
> @@ -8,6 +8,7 @@
>  #include <glib/gstdio.h>
>  #include <locale.h>
>  #include <pwd.h>
> +#include <grp.h>
> 
>  #include "commands-common-ssh.h"
>  #include "qapi/error.h"
> @@ -123,6 +124,9 @@ qmp_guest_ssh_add_authorized_keys(const char *username, strList *keys,
>      g_auto(GStrv) authkeys = NULL;
>      strList *k;
>      size_t nkeys, nauthkeys;
> +    uid_t euid, egid;
> +    __attribute__((unused)) uid_t unused_euid;
> +    __attribute__((unused)) uid_t unused_egid;
> 
>      reset = has_reset && reset;
> 
> @@ -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;
> +    }
> +
>      ssh_path = g_build_filename(p->pw_dir, ".ssh", NULL);
>      authkeys_path = g_build_filename(ssh_path, "authorized_keys", NULL);
> 
> @@ -144,6 +171,9 @@ qmp_guest_ssh_add_authorized_keys(const char *username, strList *keys,
>      if (authkeys == NULL) {
>          if (!g_file_test(ssh_path, G_FILE_TEST_IS_DIR) &&
>              !mkdir_for_user(ssh_path, p, 0700, errp)) {
> +            /* Ignore errors, we can't do anything in this case */
> +            unused_euid = seteuid(euid);
> +            unused_egid = setegid(egid);

IMHO we should

      assert(seteuid(euid) == 0)

given the code flow, there is no way in which this should ever fail in
normal operation. So if it fails, aborting the daemon feels right, as
this is a sign of a major disaster. Systemd would auto-restart it to
recover

With that, you don't need the "unused" annotations

If we really strongly don't want to abort though, we could just
fork but not exec, just to run the privileged operation, and thus
the temporary throwaway process would exit and not need to change
UID back to the orignial.

>              return;
>          }
>      }
> @@ -160,6 +190,9 @@ qmp_guest_ssh_add_authorized_keys(const char *username, strList *keys,
>      }
> 
>      write_authkeys(authkeys_path, authkeys, p, errp);
> +    /* Ignore errors, we can't do anything in this case */
> +    unused_euid = seteuid(euid);
> +    unused_egid = setegid(egid);
>  }
> 
>  void
> @@ -172,6 +205,9 @@ qmp_guest_ssh_remove_authorized_keys(const char *username, strList *keys,
>      g_auto(GStrv) authkeys = NULL;
>      GStrv a;
>      size_t nkeys = 0;
> +    uid_t euid, egid;
> +    __attribute__((unused)) uid_t unused_euid;
> +    __attribute__((unused)) uid_t unused_egid;
> 
>      if (!check_openssh_pub_keys(keys, NULL, errp)) {
>          return;
> @@ -182,6 +218,29 @@ qmp_guest_ssh_remove_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;
> +    }
> +
>      authkeys_path = g_build_filename(p->pw_dir, ".ssh",
>                                       "authorized_keys", NULL);
>      if (!g_file_test(authkeys_path, G_FILE_TEST_EXISTS)) {
> @@ -209,6 +268,9 @@ qmp_guest_ssh_remove_authorized_keys(const char *username, strList *keys,
>      }
> 
>      write_authkeys(authkeys_path, new_keys, p, errp);
> +    /* Ignore errors, we can't do anything in this case */
> +    unused_euid = seteuid(euid);
> +    unused_egid = setegid(egid);
>  }
> 
>  GuestAuthorizedKeys *
> @@ -219,16 +281,45 @@ qmp_guest_ssh_get_authorized_keys(const char *username, Error **errp)
>      g_auto(GStrv) authkeys = NULL;
>      g_autoptr(GuestAuthorizedKeys) ret = NULL;
>      int i;
> +    uid_t euid, egid;
> +    __attribute__((unused)) uid_t unused_euid;
> +    __attribute__((unused)) uid_t unused_egid;
> 
>      p = get_passwd_entry(username, errp);
>      if (p == NULL) {
>          return NULL;
>      }
> 
> +    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 NULL;
> +    }
> +#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 NULL;
> +    }
> +    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 NULL;
> +    }
> +
>      authkeys_path = g_build_filename(p->pw_dir, ".ssh",
>                                       "authorized_keys", NULL);
>      authkeys = read_authkeys(authkeys_path, errp);
>      if (authkeys == NULL) {
> +        /* Ignore errors, we can't do anything in this case */
> +        unused_euid = seteuid(euid);
> +        unused_egid = setegid(egid);
>          return NULL;
>      }
> 
> @@ -242,6 +333,9 @@ qmp_guest_ssh_get_authorized_keys(const char *username, Error **errp)
>          QAPI_LIST_PREPEND(ret->keys, g_strdup(authkeys[i]));
>      }
> 
> +    /* Ignore errors, we can't do anything in this case */
> +    unused_euid = seteuid(euid);
> +    unused_egid = setegid(egid);
>      return g_steal_pointer(&ret);
>  }
> 
> --
> 2.55.0
> 

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