From 8039442a401b6c5cc587f3cb81fcf62211991e60 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 13 Jan 2024 21:38:15 -0500 Subject: [PATCH] Support using UUIDs instead of VM names This supports using UUIDs (instead of VM names) in the Admin API. It also provides the UUIDs to qrexec-policy-daemon, which will use them to support uuid: syntax as an alternative way of naming a VM. All admin APIs now support UUIDs as source and destination. Specific APIs changed include: - admin.vm.CreateDisposable: if the payload is "uuid" used, the VM UUID (with "uuid:" prefix) is returned instead of the name. - admin.vm.List: the UUID is included in the returned list. - internal.GetSystemInfo: the UUID is returned in the "uuid" key of each VM's JSON object. - A new stubdom_uuid property contains the UUID of the stubdomain. Fixes: QubesOS/qubes-issues#8862 --- qubes/api/__init__.py | 59 +++++++++++++++++++++++++++---------- qubes/api/admin.py | 17 +++++++---- qubes/api/internal.py | 1 + qubes/app.py | 3 +- qubes/exc.py | 8 +++++ qubes/tests/api_admin.py | 57 +++++++++++++++++++++++++++++++---- qubes/tests/api_internal.py | 7 +++++ qubes/vm/adminvm.py | 3 +- qubes/vm/qubesvm.py | 18 +++++++++-- 9 files changed, 142 insertions(+), 31 deletions(-) diff --git a/qubes/api/__init__.py b/qubes/api/__init__.py index 28b0d99d4..341107d76 100644 --- a/qubes/api/__init__.py +++ b/qubes/api/__init__.py @@ -24,10 +24,13 @@ import functools import io import os +import re import shutil import socket import struct import traceback +from typing import Union, Any +import uuid import qubes.exc from qubes.exc import ProtocolError, PermissionDenied @@ -88,6 +91,32 @@ def apply_filters(iterable, filters): iterable = filter(selector, iterable) return iterable +# This regex allows incorrect-length UUIDs, +# but there is an explicit length check to catch that. +_uuid_regex = re.compile(rb"\Auuid:[0-9a-f]{8}(?:-[0-9a-f]{4}){3}-[0-9a-f]*") +def decode_vm( + untrusted_input: bytes, domains: qubes.app.VMCollection +) -> qubes.vm.qubesvm.QubesVM: + lookup: Union[uuid.UUID, str] + vm = untrusted_input.decode("ascii", "strict") + if untrusted_input.startswith(b"uuid:"): + if (len(untrusted_input) != 41 or + not _uuid_regex.match(untrusted_input)): + raise qubes.exc.QubesVMInvalidUUIDError(vm[5:]) + lookup = uuid.UUID(vm[5:]) + else: + # throws if name is invalid + qubes.vm.validate_name(None, None, vm) + lookup = vm + try: + return domains[lookup] + except KeyError: + # normally this should filtered out by qrexec policy, but there are + # two cases it might not be: + # 1. The call comes from dom0, which bypasses qrexec policy + # 2. Domain was removed between checking the policy and here + # we inform the client accordingly + raise qubes.exc.QubesVMNotFoundError(vm) class AbstractQubesAPI: '''Common code for Qubes Management Protocol handling @@ -108,25 +137,23 @@ class AbstractQubesAPI: #: the preferred socket location (to be overridden in child's class) SOCKNAME = None - def __init__(self, app, src, method_name, dest, arg, send_event=None): + app: qubes.Qubes + src: qubes.vm.qubesvm.QubesVM + def __init__(self, + app: qubes.Qubes, + src: bytes, + method_name: bytes, + dest: bytes, + arg: qubes.Qubes, + send_event: Any = None) -> None: #: :py:class:`qubes.Qubes` object self.app = app - try: - vm = src.decode('ascii') - #: source qube - self.src = self.app.domains[vm] - - vm = dest.decode('ascii') - #: destination qube - self.dest = self.app.domains[vm] - except KeyError: - # normally this should be filtered out by qrexec policy, but there - # are two cases it might not be: - # 1. The call comes from dom0, which bypasses qrexec policy - # 2. Domain was removed between checking the policy and here - # we inform the client accordingly - raise qubes.exc.QubesVMNotFoundError(vm) + #: source qube + self.src = decode_vm(src, app.domains) + + #: destination qube + self.dest = decode_vm(dest, app.domains) #: argument self.arg = arg.decode('ascii') diff --git a/qubes/api/admin.py b/qubes/api/admin.py index 39b9311e4..87fa473c7 100644 --- a/qubes/api/admin.py +++ b/qubes/api/admin.py @@ -121,10 +121,11 @@ async def vm_list(self): else: domains = self.fire_event_for_filter([self.dest]) - return ''.join('{} class={} state={}\n'.format( + return ''.join('{} class={} state={} uuid={}\n'.format( vm.name, vm.__class__.__name__, - vm.get_power_state()) + vm.get_power_state(), + vm.uuid) for vm in sorted(domains)) @qubes.api.method('admin.vm.property.List', no_payload=True, @@ -1138,10 +1139,14 @@ async def _vm_create(self, vm_type, allow_pool=False, raise self.app.save() - @qubes.api.method('admin.vm.CreateDisposable', no_payload=True, + @qubes.api.method('admin.vm.CreateDisposable', scope='global', write=True) - async def create_disposable(self): + async def create_disposable(self, untrusted_payload): self.enforce(not self.arg) + if untrusted_payload not in (b"", b"uuid"): + raise qubes.exc.QubesValueError( + "Invalid payload for admin.vm.CreateDisposable: " + "expected the empty string or 'uuid'") if self.dest.name == 'dom0': dispvm_template = self.src.default_dispvm @@ -1155,7 +1160,9 @@ async def create_disposable(self): dispvm.tags.add('created-by-' + str(self.src)) dispvm.tags.add('disp-created-by-' + str(self.src)) - return dispvm.name + return (("uuid:" + str(dispvm.uuid)) + if untrusted_payload + else dispvm.name) @qubes.api.method( 'admin.vm.Remove', no_payload=True, scope='global', write=True) diff --git a/qubes/api/internal.py b/qubes/api/internal.py index 3812764d1..587a61b0c 100644 --- a/qubes/api/internal.py +++ b/qubes/api/internal.py @@ -43,6 +43,7 @@ def get_system_info(app): 'guivm': (domain.guivm.name if getattr(domain, 'guivm', None) else None), 'power_state': domain.get_power_state(), + 'uuid': str(domain.uuid), } for domain in app.domains }} return system_info diff --git a/qubes/app.py b/qubes/app.py index df0b98071..d7a9600b0 100644 --- a/qubes/app.py +++ b/qubes/app.py @@ -516,6 +516,7 @@ def __getitem__(self, key): if isinstance(key, uuid.UUID): for vm in self: + assert isinstance(vm.uuid, uuid.UUID) if vm.uuid == key: return vm raise KeyError(key) @@ -540,7 +541,7 @@ def __delitem__(self, key): self.app.fire_event('domain-delete', vm=vm) def __contains__(self, key): - return any((key in (vm, vm.qid, vm.name)) + return any((key in (vm, vm.qid, vm.name, vm.uuid)) for vm in self) def __len__(self): diff --git a/qubes/exc.py b/qubes/exc.py index 45a67fab0..55b04dce8 100644 --- a/qubes/exc.py +++ b/qubes/exc.py @@ -37,6 +37,14 @@ def __str__(self): # KeyError overrides __str__ method return QubesException.__str__(self) +class QubesVMInvalidUUIDError(QubesException): + """Domain UUID is invalid""" + # pylint: disable = super-init-not-called + def __init__(self, uuid: str) -> None: + # QubesVMNotFoundError overrides __init__ method + # pylint: disable = non-parent-init-called + QubesException.__init__(self, f"VM UUID is not valid: {uuid!r}") + self.vmname = uuid class QubesVMError(QubesException): '''Some problem with domain state.''' diff --git a/qubes/tests/api_admin.py b/qubes/tests/api_admin.py index 6b752b392..b7a91b515 100644 --- a/qubes/tests/api_admin.py +++ b/qubes/tests/api_admin.py @@ -23,9 +23,11 @@ import asyncio import operator import os +import re import shutil import tempfile import unittest.mock +import uuid import libvirt import copy @@ -46,6 +48,8 @@ 'pool', 'vid', 'size', 'usage', 'rw', 'source', 'path', 'save_on_stop', 'snap_on_start', 'revisions_to_keep', 'ephemeral'] +_uuid_regex = re.compile( + r"\Auuid:[0-9a-f]{8}(?:-[0-9a-f]{4}){3}-[0-9a-f]{12}\Z") class AdminAPITestCase(qubes.tests.QubesTestCase): def setUp(self): @@ -78,6 +82,7 @@ def setUp(self): app.save = unittest.mock.Mock() self.vm = app.add_new_vm('AppVM', label='red', name='test-vm1', template='test-template') + self.uuid = b"uuid:" + str(self.vm.uuid).encode("ascii", "strict") self.app = app libvirt_attrs = { 'libvirt_conn.lookupByUUID.return_value.isActive.return_value': @@ -130,14 +135,33 @@ class TC_00_VMs(AdminAPITestCase): def test_000_vm_list(self): value = self.call_mgmt_func(b'admin.vm.List', b'dom0') self.assertEqual(value, - 'dom0 class=AdminVM state=Running\n' - 'test-template class=TemplateVM state=Halted\n' - 'test-vm1 class=AppVM state=Halted\n') + 'dom0 class=AdminVM state=Running ' + 'uuid=00000000-0000-0000-0000-000000000000\n' + 'test-template class=TemplateVM state=Halted ' + f'uuid={self.template.uuid}\n' + f'test-vm1 class=AppVM state=Halted uuid={self.vm.uuid}\n') def test_001_vm_list_single(self): value = self.call_mgmt_func(b'admin.vm.List', b'test-vm1') self.assertEqual(value, - 'test-vm1 class=AppVM state=Halted\n') + f"test-vm1 class=AppVM state=Halted uuid={self.vm.uuid}\n") + + def test_001_vm_list_single_uuid(self): + value = self.call_mgmt_func(b'admin.vm.List', self.uuid) + self.assertEqual(value, + f"test-vm1 class=AppVM state=Halted uuid={self.vm.uuid}\n") + + def test_001_vm_list_single_invalid_name(self): + with self.assertRaisesRegex(qubes.exc.QubesValueError, + r"\AVM name contains illegal characters\Z"): + self.call_mgmt_func(b'admin.vm.CreateDisposable', b'.test-vm1') + self.assertFalse(self.app.save.called) + + def test_001_vm_list_single_invalid_uuid(self): + with self.assertRaisesRegex(qubes.exc.QubesVMInvalidUUIDError, + r"\AVM UUID is not valid: ''\Z"): + self.call_mgmt_func(b'admin.vm.CreateDisposable', b"uuid:") + self.assertFalse(self.app.save.called) def test_002_vm_list_filter(self): with tempfile.TemporaryDirectory() as tmpdir: @@ -154,8 +178,9 @@ def test_002_vm_list_filter(self): value = loop.run_until_complete( mgmt_obj.execute(untrusted_payload=b'')) self.assertEqual(value, - 'dom0 class=AdminVM state=Running\n' - 'test-vm1 class=AppVM state=Halted\n') + 'dom0 class=AdminVM state=Running ' + 'uuid=00000000-0000-0000-0000-000000000000\n' + f"test-vm1 class=AppVM state=Halted uuid={self.vm.uuid}\n") def test_010_vm_property_list(self): # this test is kind of stupid, but at least check if appropriate @@ -2816,6 +2841,26 @@ def test_640_vm_create_disposable(self, mock_storage): mock_storage.assert_called_once_with() self.assertTrue(self.app.save.called) + @unittest.mock.patch('qubes.storage.Storage.create') + def test_640_vm_create_disposable_uuid(self, mock_storage): + mock_storage.side_effect = self.dummy_coro + self.vm.template_for_dispvms = True + retval = self.call_mgmt_func(b'admin.vm.CreateDisposable', + b'test-vm1', payload=b'uuid') + self.assertRegex(retval, _uuid_regex) + found = False + for i in self.app.domains: + print(i.uuid) + if i.uuid == uuid.UUID(retval): + found = True + self.assertIsInstance(i.uuid, uuid.UUID) + self.assertTrue(found) + self.assertIn(uuid.UUID(retval), self.app.domains) + dispvm = self.app.domains[uuid.UUID(retval)] + self.assertEqual(dispvm.template, self.vm) + mock_storage.assert_called_once_with() + self.assertTrue(self.app.save.called) + @unittest.mock.patch('qubes.storage.Storage.create') def test_641_vm_create_disposable_default(self, mock_storage): mock_storage.side_effect = self.dummy_coro diff --git a/qubes/tests/api_internal.py b/qubes/tests/api_internal.py index 653f20fdb..4e686f438 100644 --- a/qubes/tests/api_internal.py +++ b/qubes/tests/api_internal.py @@ -23,6 +23,7 @@ import qubes.vm.adminvm from unittest import mock import json +import uuid def mock_coro(f): async def coro_f(*args, **kwargs): @@ -30,6 +31,8 @@ async def coro_f(*args, **kwargs): return coro_f +TEST_UUID = uuid.UUID("50c7dad4-5f1e-4586-9f6a-bf10a86ba6f0") + class TC_00_API_Misc(qubes.tests.QubesTestCase): def setUp(self): super().setUp() @@ -150,6 +153,7 @@ def test_010_get_system_info(self): self.dom0.template_for_dispvms = False self.dom0.label.icon = 'icon-dom0' self.dom0.get_power_state.return_value = 'Running' + self.dom0.uuid = uuid.UUID("00000000-0000-0000-0000-000000000000") del self.dom0.guivm vm = mock.NonCallableMock(spec=qubes.vm.qubesvm.QubesVM) @@ -160,6 +164,7 @@ def test_010_get_system_info(self): vm.label.icon = 'icon-vm' vm.guivm = vm vm.get_power_state.return_value = 'Halted' + vm.uuid = TEST_UUID self.domains['vm'] = vm ret = json.loads(self.call_mgmt_func(b'internal.GetSystemInfo')) @@ -173,6 +178,7 @@ def test_010_get_system_info(self): 'icon': 'icon-dom0', 'guivm': None, 'power_state': 'Running', + 'uuid': "00000000-0000-0000-0000-000000000000", }, 'vm': { 'tags': ['tag3', 'tag4'], @@ -182,6 +188,7 @@ def test_010_get_system_info(self): 'icon': 'icon-vm', 'guivm': 'vm', 'power_state': 'Halted', + "uuid": str(TEST_UUID), } } }) diff --git a/qubes/vm/adminvm.py b/qubes/vm/adminvm.py index 6b67addbe..6a9bb67be 100644 --- a/qubes/vm/adminvm.py +++ b/qubes/vm/adminvm.py @@ -25,6 +25,7 @@ import grp import subprocess import libvirt +import uuid import qubes import qubes.exc @@ -45,7 +46,7 @@ class AdminVM(BaseVM): default=0, type=int, setter=qubes.property.forbidden) uuid = qubes.property('uuid', - default='00000000-0000-0000-0000-000000000000', + default=uuid.UUID('00000000-0000-0000-0000-000000000000'), setter=qubes.property.forbidden) default_dispvm = qubes.VMProperty('default_dispvm', diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py index 04889dcc5..be5bae68b 100644 --- a/qubes/vm/qubesvm.py +++ b/qubes/vm/qubesvm.py @@ -61,6 +61,7 @@ MEM_OVERHEAD_BASE = (3 + 1) * 1024 * 1024 MEM_OVERHEAD_PER_VCPU = 3 * 1024 * 1024 / 2 +_vm_uuid_re = re.compile(rb"\A/vm/[0-9a-f]{8}(?:-[0-9a-f]{4}){4}[0-9a-f]{8}\Z") def _setter_kernel(self, prop, value): """ Helper for setting the domain kernel and running sanity checks on it. @@ -801,6 +802,17 @@ def xid(self): e.get_error_code())) raise + @qubes.stateless_property + def stubdom_uuid(self): + stubdom_xid = self.stubdom_xid + if stubdom_xid == -1: + return "" + stubdom_uuid = self.app.vmm.xs.read( + '', '/local/domain/{}/vm'.format( + stubdom_xid)) + assert _vm_uuid_re.match(stubdom_uuid), "Invalid UUID in XenStore" + return stubdom_uuid[4:].decode("ascii", "strict") + @qubes.stateless_property def stubdom_xid(self): if not self.is_running(): @@ -1807,9 +1819,11 @@ async def start_qrexec_daemon(self, stubdom=False): self.log.debug('Starting the qrexec daemon') if stubdom: - qrexec_args = [str(self.stubdom_xid), self.name + '-dm', 'root'] + qrexec_args = ["-u", self.stubdom_uuid, "--", + str(self.stubdom_xid), self.name + '-dm', 'root'] else: - qrexec_args = [str(self.xid), self.name, self.default_user] + qrexec_args = ["-u", str(self.uuid), "--", + str(self.xid), self.name, self.default_user] if not self.debug: qrexec_args.insert(0, "-q")