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

On Mon, Aug 10, 2026 at 5:17 PM Peter Maydell <[email protected]>
wrote:

> On Mon, 10 Aug 2026 at 15:07, 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.
> >
> > v2: https://patchew.org/QEMU/[email protected]
> > v3 -> v2:
> > Deduplicate code.
> > Fail daemon when can't rollback effective user/group ID.
> >
> > Reported-by: Valentino Paulon <[email protected]>
> > Signed-off-by: Kostiantyn Kostiuk <[email protected]>
> > ---
> >  qga/commands-posix-ssh.c | 66 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >
> > diff --git a/qga/commands-posix-ssh.c b/qga/commands-posix-ssh.c
> > index 661972e34e..070cdffcd0 100644
> > --- a/qga/commands-posix-ssh.c
> > +++ b/qga/commands-posix-ssh.c
> > @@ -8,11 +8,29 @@
> >  #include <glib/gstdio.h>
> >  #include <locale.h>
> >  #include <pwd.h>
> > +#include <grp.h>
> >
> >  #include "commands-common-ssh.h"
> >  #include "qapi/error.h"
> >  #include "qga-qapi-commands.h"
> >
> > +typedef struct EffectiveUserInfo {
> > +    uid_t uid;
> > +    gid_t gid;
> > +} EffectiveUserInfo;
> > +
> > +typedef EffectiveUserInfo *PEffectiveUserInfo;
> > +
> > +static void rollback_effective_info(PEffectiveUserInfo info)
> > +{
> > +    if (info) {
> > +        assert(seteuid(info->uid) == 0);
> > +        assert(setegid(info->gid) == 0);
>
> A brief comment here about why we're happy to assert() here would
> be helpful.
>

Done


>
> > +    }
> > +}
> > +
> > +G_DEFINE_AUTO_CLEANUP_FREE_FUNC(PEffectiveUserInfo,
> rollback_effective_info, NULL);
>
> Don't we also need to g_free(info) in the cleanup function?
>

You are right


>
> thanks
> -- PMM
>
>