[libvirt] [SECURITY] PATCH: Fix missing read-only access checks (CVE-2008-5086)

[Daniel P. Berrange]

On Thu, Dec 18, 2008 at 09:23:27AM +1100, James Morris wrote:
> On Wed, 17 Dec 2008, Daniel P. Berrange wrote:
> 
> > The following methods in libvirt.c are missing a check against the
> > read-only connection flag:
> > 
> >     virDomainMigrate
> >     virDomainMigratePrepare
> >     virDomainMigratePerform
> >     virDomainMigrateFinish
> >     virDomainMigratePrepare2
> >     virDomainMigrateFinish2
> >     virDomainBlockPeek
> >     virDomainMemoryPeek
> >     virDomainSetAutostart
> >     virNetworkSetAutostart
> >     virConnectFindStoragePoolSources
> >     virStoragePoolSetAutostart
> > 
> > If using PolicyKit auth, the default policy will allow any local user 
> > to make a read-only connection to the libvirtd daemon without needing
> > authentication.
> > 
> > If not using PolicyKit, the default libvirtd.conf configuration settings
> > will allow an unprivileged user to make a read-only connection to the
> > libvirtd daemon without needing authentication. 
> 
> Dan (Walsh or otherwise),
> 
> Any idea if the standard SELinux policy helps here?

Nope, doesn't do anything in this context, because there is no policy
controlling what actions you can invoke. The limited policy that exists
thus far only serves to protect the host OS from the guest OS, not
protect the guest OS' from malicious admins using the host mgmt APIs.

> We'll need to assess policy for these sockets for sVirt, in any case.

It is not really the socket policy that's the problem - the policy is
working exactly as intended - which was to allow users ability to monitor
system status. eg in same way a user can use 'top' to monitor status of
all processes in an OS, this read-only socket allows you to use 'virt-top'
to monitor all VMs on a local host.

The issues which contributed to this vulnerability are:

 1. Inadequate code review of public APIs - many people reviewed the
    patches adding this public APIs and none of us noticed the missing
    read-only flag check

 2. Bad code structure/model for permissions check, such that a missing
    permission check results in an allow-by-default scenario.
    
With the code review issue, the problem is that of the hundreds of
patches sent to the list, only a very few deal with new public facing
APIs. It is only in the public facing APIs that the permissions checks
are relevant, so reviewers are not used to checking for this as a general
rule. To address this I suggest we create a list of mandatory review
points for all new public APIs. Anyone posting a new public API should
post the list of review points and how their patch complies. Reviewers
then have a clear explicit list of things to validate.

For the second problem, we need to refactor the code such that every
single public API includes a permissions check in its entry point
and centralize this check one place, and structure it such that a
mistake results in a deny-unless-readonly policy by default.

In the a past a number of people have suggested that we include some
form of fine grained access control checks - perhaps RBAC, on a 
per-API basis, or even per-API, per-Object instead of just a global
read-write vs read-only checks.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|