* [PATCH] qga: Do not follow symlink in guest-ssh-* commands @ 2026-07-09 10:57 Kostiantyn Kostiuk 2026-07-14 10:37 ` Daniel P. Berrangé 0 siblings, 1 reply; 5+ messages in thread From: Kostiantyn Kostiuk @ 2026-07-09 10:57 UTC (permalink / raw) To: qemu-devel Cc: Valentino Paulon, Daniel P . Berrangé, Yan Vugenfirer, Kostiantyn Kostiuk, Michael Roth 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands 2026-07-09 10:57 [PATCH] qga: Do not follow symlink in guest-ssh-* commands Kostiantyn Kostiuk @ 2026-07-14 10:37 ` Daniel P. Berrangé 2026-07-14 11:28 ` Kostiantyn Kostiuk 0 siblings, 1 reply; 5+ messages in thread From: Daniel P. Berrangé @ 2026-07-14 10:37 UTC (permalink / raw) To: Kostiantyn Kostiuk Cc: qemu-devel, Valentino Paulon, Yan Vugenfirer, Michael Roth 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 :| ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands 2026-07-14 10:37 ` Daniel P. Berrangé @ 2026-07-14 11:28 ` Kostiantyn Kostiuk 2026-07-14 11:30 ` Daniel P. Berrangé 0 siblings, 1 reply; 5+ messages in thread From: Kostiantyn Kostiuk @ 2026-07-14 11:28 UTC (permalink / raw) To: Daniel P. Berrangé Cc: qemu-devel, Valentino Paulon, Yan Vugenfirer, Michael Roth [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands 2026-07-14 11:28 ` Kostiantyn Kostiuk @ 2026-07-14 11:30 ` Daniel P. Berrangé 2026-07-14 16:10 ` Valentino Paulon 0 siblings, 1 reply; 5+ messages in thread From: Daniel P. Berrangé @ 2026-07-14 11:30 UTC (permalink / raw) To: Kostiantyn Kostiuk Cc: qemu-devel, Valentino Paulon, Yan Vugenfirer, Michael Roth 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 :| ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands 2026-07-14 11:30 ` Daniel P. Berrangé @ 2026-07-14 16:10 ` Valentino Paulon 0 siblings, 0 replies; 5+ messages in thread From: Valentino Paulon @ 2026-07-14 16:10 UTC (permalink / raw) To: qemu-devel, berrange, kkostiuk; +Cc: yvugenfi, michael.roth [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-07-14 20:11 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-09 10:57 [PATCH] qga: Do not follow symlink in guest-ssh-* commands Kostiantyn Kostiuk 2026-07-14 10:37 ` Daniel P. Berrangé 2026-07-14 11:28 ` Kostiantyn Kostiuk 2026-07-14 11:30 ` Daniel P. Berrangé 2026-07-14 16:10 ` Valentino Paulon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.