Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands
Version 2 with setegid/seteuid logic pushed Please review and test On Mon, Aug 10, 2026 at 12:58 PM Daniel P. Berrangé 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é > > > 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é < > [email protected] > > > > > > > > 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é < > > > [email protected] > > > > > > > > > > > > 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 < > > > > > > > > [email protected]> 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
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é > 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é > > > > > 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é < > > [email protected] > > > > > > > > > > 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 < > > > > > > > [email protected]> 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
On Mon, Aug 10, 2026 at 12:28 PM Daniel P. Berrangé 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é > > > 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é < > [email protected] > > > > > > > > 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 < > > > > > > [email protected]> 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 :| > >
Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands
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é > 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é > > > > > 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 < > > > > > [email protected]> 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
On Mon, Aug 10, 2026 at 11:55 AM Daniel P. Berrangé 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é > > > 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 < > > > > [email protected]> 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 :| > >
Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands
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é > 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 < > > > [email protected]> 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
On Mon, Aug 10, 2026 at 11:20 AM Daniel P. Berrangé 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 < > > [email protected]> 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 :| > >
Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands
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 < > [email protected]> 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
Hi Valentino, Sorry for the long delay. I have a question for you On Tue, Jul 14, 2026 at 7:10 PM Valentino Paulon < [email protected]> 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é" > 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é > > >> > 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 >> > > > Signed-off-by: Kostiantyn Kostiuk >> > > > --- >> > > > 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 =
Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands
> > 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é"
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é
> > 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
> > > > Signed-off-by: Kostiantyn Kostiuk
> > > > ---
> > > > 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,
Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands
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é
> 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
> > > Signed-off-by: Kostiantyn Kostiuk
> > > ---
> > > 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
On Tue, Jul 14, 2026 at 1:37 PM Daniel P. Berrangé
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
> > Signed-off-by: Kostiantyn Kostiuk
> > ---
> > 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;
> > }
> >
> > -
Re: [PATCH] qga: Do not follow symlink in guest-ssh-* commands
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
> Signed-off-by: Kostiantyn Kostiuk
> ---
> 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,
