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