PATCH: Actually make /local/domain/$DOMID readonly to the guest

PATCH: Actually make /local/domain/$DOMID readonly to the guest

[Daniel P. Berrange]

A few months back change 18564 fixed a permissions flaw in xenstore
which allowed guests to over-write critical data used by the host

    changeset:   18564:60937c4c5a67
    user:        Keir Fraser <keir.fraser@citrix.com>
    date:        Thu Oct 02 10:37:28 2008 +0100
    files:       tools/python/xen/xend/XendDomainInfo.py tools/python/xen/xend/image.py
    description:
    xend: Make only selected subdirs of /local/domain/<domid> writable by the guest.

    This protects critical data like
    /local/domain/<domid>/console/{tty,limit}. It also means we can trust
    .../vm, and hence do not need /vm_path. Various parts of the previous
    two changesets disappear.

    Signed-off-by: Keir Fraser <keir.fraser@citrix.com>


One of the parts in this patch was

@@ -1269,8 +1301,11 @@ class XendDomainInfo:
     def _recreateDomFunc(self, t):
         t.remove()
         t.mkdir()
-        t.set_permissions({'dom' : self.domid})
+        t.set_permissions({'dom' : self.domid, 'read' : True})
         t.write('vm', self.vmpath)
+        for i in [ 'device', 'control', 'error' ]:
+            t.mkdir(i)
+            t.set_permissions(i, {'dom' : self.domid})
 
Most people reading this chunk would assume that the call

     t.set_permissions({'dom' : self.domid, 'read' : True})

would make the xenstore path in question read only to the guest listed.
Unfortunately the 'set_permissions' call turns out to be bizarre

The first permission record listed does not set permissions at all. It
in fact sets the 'owner' of the path. The owner is given full access to
do whatever they like in that path, regardless of what permissions
bits are set.

To actually restrict the permissions on a path for a guest, you have
to first give a different guest permissions on the path so it becomes
the 'owner'.

The upshot is that changeset  18564:60937c4c5a67 didn't do anything to
protect xenstore data from the guest as we thought it did :-(

The increment fix is to explicitly make Dom0 the owner of all the 
/local/domain/$DOMID  paths, and then give the DomU read permission

So we need the following additional patch applied:



diff -r a76b4e00e186 tools/python/xen/xend/XendDomainInfo.py
--- a/tools/python/xen/xend/XendDomainInfo.py	Tue Dec 16 13:14:25 2008 +0000
+++ b/tools/python/xen/xend/XendDomainInfo.py	Thu Dec 18 10:20:42 2008 -0500
@@ -1359,7 +1359,18 @@ class XendDomainInfo:
     def _recreateDomFunc(self, t):
         t.remove()
         t.mkdir()
-        t.set_permissions({'dom' : self.domid, 'read' : True})
+        #
+        # The first permission record on any node indicates
+        # the domain owner. Thus any permissions associated
+        # with the first record are ignored, since the owner
+        # is allowed todo anything.
+        #
+        # Thus to make the node read-only to the guest
+        # we must explicitly give Dom0 permission making
+        # it the owner, even though Dom0 already has all the
+        # permissions inherited from the parent.
+        #
+        t.set_permissions({'dom' : 0 }, {'dom' : self.domid, 'read' : True})
         t.write('vm', self.vmpath)
         for i in [ 'device', 'control', 'error', 'memory' ]:
             t.mkdir(i)

Explicitly give Dom0 permissions on the /local/domain/$DOMID so it
becomes the owner of the path. The guest is then granted read perm
on the path.

   Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

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

Re: PATCH: Actually make /local/domain/$DOMID readonly to the guest

[Keir Fraser]

On 18/12/2008 15:53, "Daniel P. Berrange" <berrange@redhat.com> wrote:

> Explicitly give Dom0 permissions on the /local/domain/$DOMID so it
> becomes the owner of the path. The guest is then granted read perm
> on the path.

Thanks Daniel, that's a nasty one!

However there are other places in xend that commit the same error, and this
interface weakness would doubtless bite us again in future. Hence the patch
I actually committed (c/s 18933) actually takes a different strategy: in the
bowels of the xend xenstore C package I check to see if the caller is try to
change permissions of the node owner, and if so I fudge in dom0 as the owner
instead. A bit grim, but I think probably a safer bet in this instance.

What do you think of it? If it seems okay I will backport and will have to
do new RCs of 3.2.3 and 3.3.1.

 Thanks again,
 Keir

Re: PATCH: Actually make /local/domain/$DOMID readonly to the guest

[Daniel P. Berrange]

On Thu, Dec 18, 2008 at 05:21:10PM +0000, Keir Fraser wrote:
> On 18/12/2008 15:53, "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > Explicitly give Dom0 permissions on the /local/domain/$DOMID so it
> > becomes the owner of the path. The guest is then granted read perm
> > on the path.
> 
> Thanks Daniel, that's a nasty one!
> 
> However there are other places in xend that commit the same error, and this
> interface weakness would doubtless bite us again in future. Hence the patch
> I actually committed (c/s 18933) actually takes a different strategy: in the
> bowels of the xend xenstore C package I check to see if the caller is try to
> change permissions of the node owner, and if so I fudge in dom0 as the owner
> instead. A bit grim, but I think probably a safer bet in this instance.

I think that looks correct to me. The easy way to test is to try and
write to '/local/domain/$DOMID/console/tty' from within the guest and
see if it succeeds or not 


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

Re: PATCH: Actually make /local/domain/$DOMID readonly to the guest

[Keir Fraser]

On 18/12/2008 17:49, "Daniel P. Berrange" <berrange@redhat.com> wrote:

>> However there are other places in xend that commit the same error, and this
>> interface weakness would doubtless bite us again in future. Hence the patch
>> I actually committed (c/s 18933) actually takes a different strategy: in the
>> bowels of the xend xenstore C package I check to see if the caller is try to
>> change permissions of the node owner, and if so I fudge in dom0 as the owner
>> instead. A bit grim, but I think probably a safer bet in this instance.
> 
> I think that looks correct to me. The easy way to test is to try and
> write to '/local/domain/$DOMID/console/tty' from within the guest and
> see if it succeeds or not

Yes, I actually tested that, and it was no longer writeable. Same for a few
susceptible nodes under /vm too.

 -- Keir