qga/commands-posix-ssh.c | 94 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+)
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);
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.0On Mon, 10 Aug 2026 at 11:47, Kostiantyn Kostiuk <kkostiuk@redhat.com> 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>
> @@ -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".
thanks
-- PMM
On Mon, Aug 10, 2026 at 02:01:53PM +0100, Peter Maydell wrote:
> On Mon, 10 Aug 2026 at 11:47, Kostiantyn Kostiuk <kkostiuk@redhat.com> 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>
>
>
>
> > @@ -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 :|
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 :|
© 2016 - 2026 Red Hat, Inc.