[PATCH] qga: Do not follow symlink in guest-ssh-* commands

[PATCH] qga: Do not follow symlink in guest-ssh-* commands

[Kostiantyn Kostiuk]

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 <valentino.paulon88@gmail.com>
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
---
 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;
+        }
+    }
+
     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);
+    }
 
     nauthkeys = authkeys ? g_strv_length(authkeys) : 0;
     authkeys = g_realloc_n(authkeys, nauthkeys + nkeys + 1, sizeof(char *));
@@ -167,11 +178,13 @@ qmp_guest_ssh_remove_authorized_keys(const char *username, strList *keys,
                                      Error **errp)
 {
     g_autofree struct passwd *p = NULL;
+    g_autofree char *ssh_path = NULL;
     g_autofree char *authkeys_path = NULL;
     g_autofree GStrv new_keys = NULL; /* do not own the strings */
     g_auto(GStrv) authkeys = NULL;
     GStrv a;
     size_t nkeys = 0;
+    int fd;
 
     if (!check_openssh_pub_keys(keys, NULL, errp)) {
         return;
@@ -182,8 +195,19 @@ qmp_guest_ssh_remove_authorized_keys(const char *username, strList *keys,
         return;
     }
 
-    authkeys_path = g_build_filename(p->pw_dir, ".ssh",
-                                     "authorized_keys", NULL);
+    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;
+        }
+    } else {
+        close(fd);
+    }
+
     if (!g_file_test(authkeys_path, G_FILE_TEST_EXISTS)) {
         return;
     }
@@ -215,18 +239,31 @@ GuestAuthorizedKeys *
 qmp_guest_ssh_get_authorized_keys(const char *username, Error **errp)
 {
     g_autofree struct passwd *p = NULL;
+    g_autofree char *ssh_path = NULL;
     g_autofree char *authkeys_path = NULL;
     g_auto(GStrv) authkeys = NULL;
     g_autoptr(GuestAuthorizedKeys) ret = NULL;
     int i;
+    int fd;
 
     p = get_passwd_entry(username, errp);
     if (p == NULL) {
         return NULL;
     }
 
-    authkeys_path = g_build_filename(p->pw_dir, ".ssh",
-                                     "authorized_keys", NULL);
+    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 NULL;
+        }
+    } else {
+        close(fd);
+    }
+
     authkeys = read_authkeys(authkeys_path, errp);
     if (authkeys == NULL) {
         return NULL;
-- 
2.53.0

Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands

[Daniel P. Berrangé]

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 <valentino.paulon88@gmail.com>
> Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> ---
>  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 ?

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.

>  
>      nauthkeys = authkeys ? g_strv_length(authkeys) : 0;
>      authkeys = g_realloc_n(authkeys, nauthkeys + nkeys + 1, sizeof(char *));
> @@ -167,11 +178,13 @@ qmp_guest_ssh_remove_authorized_keys(const char *username, strList *keys,
>                                       Error **errp)
>  {
>      g_autofree struct passwd *p = NULL;
> +    g_autofree char *ssh_path = NULL;
>      g_autofree char *authkeys_path = NULL;
>      g_autofree GStrv new_keys = NULL; /* do not own the strings */
>      g_auto(GStrv) authkeys = NULL;
>      GStrv a;
>      size_t nkeys = 0;
> +    int fd;
>  
>      if (!check_openssh_pub_keys(keys, NULL, errp)) {
>          return;
> @@ -182,8 +195,19 @@ qmp_guest_ssh_remove_authorized_keys(const char *username, strList *keys,
>          return;
>      }
>  
> -    authkeys_path = g_build_filename(p->pw_dir, ".ssh",
> -                                     "authorized_keys", NULL);
> +    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;
> +        }
> +    } else {
> +        close(fd);
> +    }
> +
>      if (!g_file_test(authkeys_path, G_FILE_TEST_EXISTS)) {
>          return;
>      }
> @@ -215,18 +239,31 @@ GuestAuthorizedKeys *
>  qmp_guest_ssh_get_authorized_keys(const char *username, Error **errp)
>  {
>      g_autofree struct passwd *p = NULL;
> +    g_autofree char *ssh_path = NULL;
>      g_autofree char *authkeys_path = NULL;
>      g_auto(GStrv) authkeys = NULL;
>      g_autoptr(GuestAuthorizedKeys) ret = NULL;
>      int i;
> +    int fd;
>  
>      p = get_passwd_entry(username, errp);
>      if (p == NULL) {
>          return NULL;
>      }
>  
> -    authkeys_path = g_build_filename(p->pw_dir, ".ssh",
> -                                     "authorized_keys", NULL);
> +    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 NULL;
> +        }
> +    } else {
> +        close(fd);
> +    }
> +
>      authkeys = read_authkeys(authkeys_path, errp);
>      if (authkeys == NULL) {
>          return NULL;
> -- 
> 2.53.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 :|

Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands

[Kostiantyn Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 7446 bytes --]

On Tue, Jul 14, 2026 at 1:37 PM Daniel P. Berrangé <berrange@redhat.com>
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 <valentino.paulon88@gmail.com>
> > Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> > ---
> >  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?

Best Regards,
Kostiantyn Kostiuk.



> >
> >      nauthkeys = authkeys ? g_strv_length(authkeys) : 0;
> >      authkeys = g_realloc_n(authkeys, nauthkeys + nkeys + 1, sizeof(char
> *));
> > @@ -167,11 +178,13 @@ qmp_guest_ssh_remove_authorized_keys(const char
> *username, strList *keys,
> >                                       Error **errp)
> >  {
> >      g_autofree struct passwd *p = NULL;
> > +    g_autofree char *ssh_path = NULL;
> >      g_autofree char *authkeys_path = NULL;
> >      g_autofree GStrv new_keys = NULL; /* do not own the strings */
> >      g_auto(GStrv) authkeys = NULL;
> >      GStrv a;
> >      size_t nkeys = 0;
> > +    int fd;
> >
> >      if (!check_openssh_pub_keys(keys, NULL, errp)) {
> >          return;
> > @@ -182,8 +195,19 @@ qmp_guest_ssh_remove_authorized_keys(const char
> *username, strList *keys,
> >          return;
> >      }
> >
> > -    authkeys_path = g_build_filename(p->pw_dir, ".ssh",
> > -                                     "authorized_keys", NULL);
> > +    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;
> > +        }
> > +    } else {
> > +        close(fd);
> > +    }
> > +
> >      if (!g_file_test(authkeys_path, G_FILE_TEST_EXISTS)) {
> >          return;
> >      }
> > @@ -215,18 +239,31 @@ GuestAuthorizedKeys *
> >  qmp_guest_ssh_get_authorized_keys(const char *username, Error **errp)
> >  {
> >      g_autofree struct passwd *p = NULL;
> > +    g_autofree char *ssh_path = NULL;
> >      g_autofree char *authkeys_path = NULL;
> >      g_auto(GStrv) authkeys = NULL;
> >      g_autoptr(GuestAuthorizedKeys) ret = NULL;
> >      int i;
> > +    int fd;
> >
> >      p = get_passwd_entry(username, errp);
> >      if (p == NULL) {
> >          return NULL;
> >      }
> >
> > -    authkeys_path = g_build_filename(p->pw_dir, ".ssh",
> > -                                     "authorized_keys", NULL);
> > +    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 NULL;
> > +        }
> > +    } else {
> > +        close(fd);
> > +    }
> > +
> >      authkeys = read_authkeys(authkeys_path, errp);
> >      if (authkeys == NULL) {
> >          return NULL;
> > --
> > 2.53.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 :|
>
>

[-- Attachment #2: Type: text/html, Size: 10514 bytes --]

Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands

[Daniel P. Berrangé]

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é <berrange@redhat.com>
> 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 <valentino.paulon88@gmail.com>
> > > Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> > > ---
> > >  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 :|

Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands

[Valentino Paulon]

[-- Attachment #1: Type: text/plain, Size: 6708 bytes --]

> > 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é" <berrange@redhat.com>
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é <berrange@redhat.com>
> > 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 <valentino.paulon88@gmail.com>
> > > > Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> > > > ---
> > > >  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 :|
>
>

[-- Attachment #2: Type: text/html, Size: 9354 bytes --]

Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands

[Kostiantyn Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 7487 bytes --]

Hi Valentino,

Sorry for the long delay.
I have a question for you

On Tue, Jul 14, 2026 at 7:10 PM Valentino Paulon <
valentino.paulon88@gmail.com> 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é" <berrange@redhat.com>
> 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é <berrange@redhat.com
>> >
>> > 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 <valentino.paulon88@gmail.com>
>> > > > Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
>> > > > ---
>> > > >  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 :|
>>
>>

[-- Attachment #2: Type: text/html, Size: 11201 bytes --]

Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands

[Daniel P. Berrangé]

On Mon, Aug 10, 2026 at 11:06:27AM +0300, Kostiantyn Kostiuk wrote:
> Hi Valentino,
> 
> Sorry for the long delay.
> I have a question for you
> 
> On Tue, Jul 14, 2026 at 7:10 PM Valentino Paulon <
> valentino.paulon88@gmail.com> 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?

In any practical sense it is a system daemon given the set of commands
it is exposing. Thus, IMHO, deploying as an unprivileged user should
be considered a broken deployment. We could make that explicit by
refusing to launch.

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

Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands

[Kostiantyn Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 2312 bytes --]

On Mon, Aug 10, 2026 at 11:20 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, Aug 10, 2026 at 11:06:27AM +0300, Kostiantyn Kostiuk wrote:
> > Hi Valentino,
> >
> > Sorry for the long delay.
> > I have a question for you
> >
> > On Tue, Jul 14, 2026 at 7:10 PM Valentino Paulon <
> > valentino.paulon88@gmail.com> 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?
>
> In any practical sense it is a system daemon given the set of commands
> it is exposing. Thus, IMHO, deploying as an unprivileged user should
> be considered a broken deployment. We could make that explicit by
> refusing to launch.
>

Makes sense, but we have one more issue with QGA unit tests in this case.
They are called by an unprivileged user (build user), and initgroups broke
it.
Is it possible to enter the namespace automatically by the test engine?


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

[-- Attachment #2: Type: text/html, Size: 3998 bytes --]

Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands

[Daniel P. Berrangé]

On Mon, Aug 10, 2026 at 11:37:34AM +0300, Kostiantyn Kostiuk wrote:
> On Mon, Aug 10, 2026 at 11:20 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Mon, Aug 10, 2026 at 11:06:27AM +0300, Kostiantyn Kostiuk wrote:
> > > Hi Valentino,
> > >
> > > Sorry for the long delay.
> > > I have a question for you
> > >
> > > On Tue, Jul 14, 2026 at 7:10 PM Valentino Paulon <
> > > valentino.paulon88@gmail.com> 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?
> >
> > In any practical sense it is a system daemon given the set of commands
> > it is exposing. Thus, IMHO, deploying as an unprivileged user should
> > be considered a broken deployment. We could make that explicit by
> > refusing to launch.
> >
> 
> Makes sense, but we have one more issue with QGA unit tests in this case.
> They are called by an unprivileged user (build user), and initgroups broke
> it.
> Is it possible to enter the namespace automatically by the test engine?

We can't assume that is possible todo in all build/test environments.

Doing things as different user accounts feels pretty dubious for the
scope of a unit test too - that is really functional or integration
testing.

Can we somehow restructure the tests and/or agent code to avoid the
changing groups stuff in unit tests.

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

Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands

[Kostiantyn Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 3222 bytes --]

On Mon, Aug 10, 2026 at 11:55 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, Aug 10, 2026 at 11:37:34AM +0300, Kostiantyn Kostiuk wrote:
> > On Mon, Aug 10, 2026 at 11:20 AM Daniel P. Berrangé <berrange@redhat.com
> >
> > wrote:
> >
> > > On Mon, Aug 10, 2026 at 11:06:27AM +0300, Kostiantyn Kostiuk wrote:
> > > > Hi Valentino,
> > > >
> > > > Sorry for the long delay.
> > > > I have a question for you
> > > >
> > > > On Tue, Jul 14, 2026 at 7:10 PM Valentino Paulon <
> > > > valentino.paulon88@gmail.com> 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?
> > >
> > > In any practical sense it is a system daemon given the set of commands
> > > it is exposing. Thus, IMHO, deploying as an unprivileged user should
> > > be considered a broken deployment. We could make that explicit by
> > > refusing to launch.
> > >
> >
> > Makes sense, but we have one more issue with QGA unit tests in this case.
> > They are called by an unprivileged user (build user), and initgroups
> broke
> > it.
> > Is it possible to enter the namespace automatically by the test engine?
>
> We can't assume that is possible todo in all build/test environments.
>
> Doing things as different user accounts feels pretty dubious for the
> scope of a unit test too - that is really functional or integration
> testing.
>
> Can we somehow restructure the tests and/or agent code to avoid the
> changing groups stuff in unit tests.
>

There are two possible options:
1. add #ifdef QGA_BUILD_UNIT_TEST into qmp_guest_ssh_* functions
2. check the current uid and skip initgroups if it is zero (my initial
question)


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

[-- Attachment #2: Type: text/html, Size: 5058 bytes --]

Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands

[Daniel P. Berrangé]

On Mon, Aug 10, 2026 at 11:58:44AM +0300, Kostiantyn Kostiuk wrote:
> On Mon, Aug 10, 2026 at 11:55 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Mon, Aug 10, 2026 at 11:37:34AM +0300, Kostiantyn Kostiuk wrote:
> > > On Mon, Aug 10, 2026 at 11:20 AM Daniel P. Berrangé <berrange@redhat.com
> > >
> > > wrote:
> > >
> > > > On Mon, Aug 10, 2026 at 11:06:27AM +0300, Kostiantyn Kostiuk wrote:
> > > > > Hi Valentino,
> > > > >
> > > > > Sorry for the long delay.
> > > > > I have a question for you
> > > > >
> > > > > On Tue, Jul 14, 2026 at 7:10 PM Valentino Paulon <
> > > > > valentino.paulon88@gmail.com> 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?
> > > >
> > > > In any practical sense it is a system daemon given the set of commands
> > > > it is exposing. Thus, IMHO, deploying as an unprivileged user should
> > > > be considered a broken deployment. We could make that explicit by
> > > > refusing to launch.
> > > >
> > >
> > > Makes sense, but we have one more issue with QGA unit tests in this case.
> > > They are called by an unprivileged user (build user), and initgroups
> > broke
> > > it.
> > > Is it possible to enter the namespace automatically by the test engine?
> >
> > We can't assume that is possible todo in all build/test environments.
> >
> > Doing things as different user accounts feels pretty dubious for the
> > scope of a unit test too - that is really functional or integration
> > testing.
> >
> > Can we somehow restructure the tests and/or agent code to avoid the
> > changing groups stuff in unit tests.
> >
> 
> There are two possible options:
> 1. add #ifdef QGA_BUILD_UNIT_TEST into qmp_guest_ssh_* functions
> 2. check the current uid and skip initgroups if it is zero (my initial
> question)

Surely you mean "if it is non-zero" here, as it is with uid!=0 that
the UID changes will fail.

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

Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands

[Kostiantyn Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 3919 bytes --]

On Mon, Aug 10, 2026 at 12:28 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, Aug 10, 2026 at 11:58:44AM +0300, Kostiantyn Kostiuk wrote:
> > On Mon, Aug 10, 2026 at 11:55 AM Daniel P. Berrangé <berrange@redhat.com
> >
> > wrote:
> >
> > > On Mon, Aug 10, 2026 at 11:37:34AM +0300, Kostiantyn Kostiuk wrote:
> > > > On Mon, Aug 10, 2026 at 11:20 AM Daniel P. Berrangé <
> berrange@redhat.com
> > > >
> > > > wrote:
> > > >
> > > > > On Mon, Aug 10, 2026 at 11:06:27AM +0300, Kostiantyn Kostiuk wrote:
> > > > > > Hi Valentino,
> > > > > >
> > > > > > Sorry for the long delay.
> > > > > > I have a question for you
> > > > > >
> > > > > > On Tue, Jul 14, 2026 at 7:10 PM Valentino Paulon <
> > > > > > valentino.paulon88@gmail.com> 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?
> > > > >
> > > > > In any practical sense it is a system daemon given the set of
> commands
> > > > > it is exposing. Thus, IMHO, deploying as an unprivileged user
> should
> > > > > be considered a broken deployment. We could make that explicit by
> > > > > refusing to launch.
> > > > >
> > > >
> > > > Makes sense, but we have one more issue with QGA unit tests in this
> case.
> > > > They are called by an unprivileged user (build user), and initgroups
> > > broke
> > > > it.
> > > > Is it possible to enter the namespace automatically by the test
> engine?
> > >
> > > We can't assume that is possible todo in all build/test environments.
> > >
> > > Doing things as different user accounts feels pretty dubious for the
> > > scope of a unit test too - that is really functional or integration
> > > testing.
> > >
> > > Can we somehow restructure the tests and/or agent code to avoid the
> > > changing groups stuff in unit tests.
> > >
> >
> > There are two possible options:
> > 1. add #ifdef QGA_BUILD_UNIT_TEST into qmp_guest_ssh_* functions
> > 2. check the current uid and skip initgroups if it is zero (my initial
> > question)
>
> Surely you mean "if it is non-zero" here, as it is with uid!=0 that
> the UID changes will fail.
>

yes. So what sounds better: "ifdef" or "uid!=0"?

Best Regards,
Kostiantyn Kostiuk.



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

[-- Attachment #2: Type: text/html, Size: 6307 bytes --]

Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands

[Daniel P. Berrangé]

On Mon, Aug 10, 2026 at 12:33:39PM +0300, Kostiantyn Kostiuk wrote:
> On Mon, Aug 10, 2026 at 12:28 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Mon, Aug 10, 2026 at 11:58:44AM +0300, Kostiantyn Kostiuk wrote:
> > > On Mon, Aug 10, 2026 at 11:55 AM Daniel P. Berrangé <berrange@redhat.com
> > >
> > > wrote:
> > >
> > > > On Mon, Aug 10, 2026 at 11:37:34AM +0300, Kostiantyn Kostiuk wrote:
> > > > > On Mon, Aug 10, 2026 at 11:20 AM Daniel P. Berrangé <
> > berrange@redhat.com
> > > > >
> > > > > wrote:
> > > > >
> > > > > > On Mon, Aug 10, 2026 at 11:06:27AM +0300, Kostiantyn Kostiuk wrote:
> > > > > > > Hi Valentino,
> > > > > > >
> > > > > > > Sorry for the long delay.
> > > > > > > I have a question for you
> > > > > > >
> > > > > > > On Tue, Jul 14, 2026 at 7:10 PM Valentino Paulon <
> > > > > > > valentino.paulon88@gmail.com> 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?
> > > > > >
> > > > > > In any practical sense it is a system daemon given the set of
> > commands
> > > > > > it is exposing. Thus, IMHO, deploying as an unprivileged user
> > should
> > > > > > be considered a broken deployment. We could make that explicit by
> > > > > > refusing to launch.
> > > > > >
> > > > >
> > > > > Makes sense, but we have one more issue with QGA unit tests in this
> > case.
> > > > > They are called by an unprivileged user (build user), and initgroups
> > > > broke
> > > > > it.
> > > > > Is it possible to enter the namespace automatically by the test
> > engine?
> > > >
> > > > We can't assume that is possible todo in all build/test environments.
> > > >
> > > > Doing things as different user accounts feels pretty dubious for the
> > > > scope of a unit test too - that is really functional or integration
> > > > testing.
> > > >
> > > > Can we somehow restructure the tests and/or agent code to avoid the
> > > > changing groups stuff in unit tests.
> > > >
> > >
> > > There are two possible options:
> > > 1. add #ifdef QGA_BUILD_UNIT_TEST into qmp_guest_ssh_* functions
> > > 2. check the current uid and skip initgroups if it is zero (my initial
> > > question)
> >
> > Surely you mean "if it is non-zero" here, as it is with uid!=0 that
> > the UID changes will fail.
> >
> 
> yes. So what sounds better: "ifdef" or "uid!=0"?

I'd be inclined to #ifdef so we know it doesn't affect production
deployments, only tests.


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

Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands

[Kostiantyn Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 4645 bytes --]

Version 2 with setegid/seteuid logic pushed
Please review and test

On Mon, Aug 10, 2026 at 12:58 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, Aug 10, 2026 at 12:33:39PM +0300, Kostiantyn Kostiuk wrote:
> > On Mon, Aug 10, 2026 at 12:28 PM Daniel P. Berrangé <berrange@redhat.com
> >
> > wrote:
> >
> > > On Mon, Aug 10, 2026 at 11:58:44AM +0300, Kostiantyn Kostiuk wrote:
> > > > On Mon, Aug 10, 2026 at 11:55 AM Daniel P. Berrangé <
> berrange@redhat.com
> > > >
> > > > wrote:
> > > >
> > > > > On Mon, Aug 10, 2026 at 11:37:34AM +0300, Kostiantyn Kostiuk wrote:
> > > > > > On Mon, Aug 10, 2026 at 11:20 AM Daniel P. Berrangé <
> > > berrange@redhat.com
> > > > > >
> > > > > > wrote:
> > > > > >
> > > > > > > On Mon, Aug 10, 2026 at 11:06:27AM +0300, Kostiantyn Kostiuk
> wrote:
> > > > > > > > Hi Valentino,
> > > > > > > >
> > > > > > > > Sorry for the long delay.
> > > > > > > > I have a question for you
> > > > > > > >
> > > > > > > > On Tue, Jul 14, 2026 at 7:10 PM Valentino Paulon <
> > > > > > > > valentino.paulon88@gmail.com> 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?
> > > > > > >
> > > > > > > In any practical sense it is a system daemon given the set of
> > > commands
> > > > > > > it is exposing. Thus, IMHO, deploying as an unprivileged user
> > > should
> > > > > > > be considered a broken deployment. We could make that explicit
> by
> > > > > > > refusing to launch.
> > > > > > >
> > > > > >
> > > > > > Makes sense, but we have one more issue with QGA unit tests in
> this
> > > case.
> > > > > > They are called by an unprivileged user (build user), and
> initgroups
> > > > > broke
> > > > > > it.
> > > > > > Is it possible to enter the namespace automatically by the test
> > > engine?
> > > > >
> > > > > We can't assume that is possible todo in all build/test
> environments.
> > > > >
> > > > > Doing things as different user accounts feels pretty dubious for
> the
> > > > > scope of a unit test too - that is really functional or integration
> > > > > testing.
> > > > >
> > > > > Can we somehow restructure the tests and/or agent code to avoid the
> > > > > changing groups stuff in unit tests.
> > > > >
> > > >
> > > > There are two possible options:
> > > > 1. add #ifdef QGA_BUILD_UNIT_TEST into qmp_guest_ssh_* functions
> > > > 2. check the current uid and skip initgroups if it is zero (my
> initial
> > > > question)
> > >
> > > Surely you mean "if it is non-zero" here, as it is with uid!=0 that
> > > the UID changes will fail.
> > >
> >
> > yes. So what sounds better: "ifdef" or "uid!=0"?
>
> I'd be inclined to #ifdef so we know it doesn't affect production
> deployments, only tests.
>
>
> 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 :|
>
>

[-- Attachment #2: Type: text/html, Size: 7346 bytes --]