[v1] Don't hold both monitor and agent jobs at the same time

[libvirt] [PATCH 0/8] Don't hold both monitor and agent jobs at the same time

Posted by Jonathon Jongsma 4 years, 4 months ago

We have to assume that the guest agent may be malicious, so we don't want to
allow any agent queries to block any other libvirt API. By holding a monitor
job and an agent job while we're querying the agent, any other threads will be
blocked from using the monitor while the agent is unresponsive. Because libvirt
waits forever for an agent response, this makes us vulnerable to a denial of
service from a malicious (or simply buggy) guest agent.

This series of patches attempts to remove any cases where we were holding both
jobs at the same time, removes a convenience function which allows us to grab
both jobs at once, and updates documentation regarding this issue.

Jonathon Jongsma (8):
  qemu: don't take agent and monitor job for shutdown
  qemu: don't hold a monitor and agent job for reboot
  qemu: don't hold both jobs for suspend
  qemu: don't hold monitor and agent job when setting time
  qemu: don't hold monitor job for fsinfo
  qemu: don't hold monitor job for GetGuestInfo()
  qemu: remove use of qemuDomainObjBeginJobWithAgent()
  qemu: remove qemuDomainObjBegin/EndJobWithAgent()

 src/qemu/THREADS.txt   |  58 +-----
 src/qemu/qemu_domain.c |  56 +-----
 src/qemu/qemu_domain.h |   7 -
 src/qemu/qemu_driver.c | 405 +++++++++++++++++++++++++----------------
 4 files changed, 258 insertions(+), 268 deletions(-)

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/8] Don't hold both monitor and agent jobs at the same time

Posted by Eric Blake 4 years, 4 months ago

On 12/5/19 10:08 AM, Jonathon Jongsma wrote:
> We have to assume that the guest agent may be malicious, so we don't want to
> allow any agent queries to block any other libvirt API. By holding a monitor
> job and an agent job while we're querying the agent, any other threads will be
> blocked from using the monitor while the agent is unresponsive. Because libvirt
> waits forever for an agent response, this makes us vulnerable to a denial of
> service from a malicious (or simply buggy) guest agent.
> 
> This series of patches attempts to remove any cases where we were holding both
> jobs at the same time, removes a convenience function which allows us to grab
> both jobs at once, and updates documentation regarding this issue.
> 

Are any of these worth CVEs?  Are any of the APIs usable on a read-only 
connection?  (My quick glance says no, it looks like they all require 
read-write).  What about ACLs?

> Jonathon Jongsma (8):
>    qemu: don't take agent and monitor job for shutdown

For example, virDomainShutdownFlags requires @acl 
domain:write:VIR_DOMAIN_SHUTDOWN_GUEST_AGENT (that is, if you request 
guest agent shutdown, you have to have the equivalent of domain:write 
ACL privilege which is effectively root permissions, precisely because 
the guest agent is untrusted).  While a DoS is bad, you already have 
enough permissions to shoot yourself in the foot in other ways, so it is 
not a privilege escalation and thus not a CVE.

>    qemu: don't hold a monitor and agent job for reboot
>    qemu: don't hold both jobs for suspend
>    qemu: don't hold monitor and agent job when setting time
>    qemu: don't hold monitor job for fsinfo
>    qemu: don't hold monitor job for GetGuestInfo()

But virDomainGetFSIinfo only requires @acl: domain:fs_freeze. Is it 
possible for a user to have domain:fs_freeze permission but not 
domain:write permission?  If so, we have a CVE because of the DoS, at 
least when ACLs are in effect.

>    qemu: remove use of qemuDomainObjBeginJobWithAgent()
>    qemu: remove qemuDomainObjBegin/EndJobWithAgent()
> 
>   src/qemu/THREADS.txt   |  58 +-----
>   src/qemu/qemu_domain.c |  56 +-----
>   src/qemu/qemu_domain.h |   7 -
>   src/qemu/qemu_driver.c | 405 +++++++++++++++++++++++++----------------
>   4 files changed, 258 insertions(+), 268 deletions(-)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/8] Don't hold both monitor and agent jobs at the same time

Posted by Michal Privoznik 4 years, 4 months ago

On 12/5/19 5:08 PM, Jonathon Jongsma wrote:
> We have to assume that the guest agent may be malicious, so we don't want to
> allow any agent queries to block any other libvirt API. By holding a monitor
> job and an agent job while we're querying the agent, any other threads will be
> blocked from using the monitor while the agent is unresponsive. Because libvirt
> waits forever for an agent response, this makes us vulnerable to a denial of
> service from a malicious (or simply buggy) guest agent.
> 
> This series of patches attempts to remove any cases where we were holding both
> jobs at the same time, removes a convenience function which allows us to grab
> both jobs at once, and updates documentation regarding this issue.
> 
> Jonathon Jongsma (8):
>    qemu: don't take agent and monitor job for shutdown
>    qemu: don't hold a monitor and agent job for reboot
>    qemu: don't hold both jobs for suspend
>    qemu: don't hold monitor and agent job when setting time
>    qemu: don't hold monitor job for fsinfo
>    qemu: don't hold monitor job for GetGuestInfo()
>    qemu: remove use of qemuDomainObjBeginJobWithAgent()
>    qemu: remove qemuDomainObjBegin/EndJobWithAgent()
> 
>   src/qemu/THREADS.txt   |  58 +-----
>   src/qemu/qemu_domain.c |  56 +-----
>   src/qemu/qemu_domain.h |   7 -
>   src/qemu/qemu_driver.c | 405 +++++++++++++++++++++++++----------------
>   4 files changed, 258 insertions(+), 268 deletions(-)
> 

ACK to all but 5/8 and 6/8. Also, I'm pushing patches 1-4 and 7. I'd 
push 8/8 also but we can't remove the function while it's still use :-D

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list