1. Patchew
  2. QEMU
  3. View series

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

Kostiantyn Kostiuk posted 1 patch 5 days, 17 hours ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260709105707.91209-1-kkostiuk@redhat.com
Maintainers: Michael Roth <michael.roth@amd.com>, Kostiantyn Kostiuk <kkostiuk@redhat.com>
qga/commands-posix-ssh.c | 53 ++++++++++++++++++++++++++++++++++------
1 file changed, 45 insertions(+), 8 deletions(-)
[PATCH] qga: Do not follow symlink in guest-ssh-* commands
Posted by Kostiantyn Kostiuk 5 days, 17 hours ago 
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
Posted by Daniel P. Berrangé 17 hours ago 
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
Posted by Kostiantyn Kostiuk 16 hours ago 
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 :|
>
>
Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands
Posted by Daniel P. Berrangé 16 hours ago 
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
Posted by Valentino Paulon 11 hours ago 
> > 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 :|
>
>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.