From 879ee9e7d6cea9deb817826c5e6a299660554828 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 20 Feb 2020 05:38:50 +0100 Subject: [PATCH 1/5] api/internal: extract get_system_info() function This will be useful in other places too. QubesOS/qubes-issues#5099 --- qubes/api/internal.py | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/qubes/api/internal.py b/qubes/api/internal.py index 1f618fbb..19659a17 100644 --- a/qubes/api/internal.py +++ b/qubes/api/internal.py @@ -30,6 +30,23 @@ import qubes.vm.adminvm import qubes.vm.dispvm +def get_system_info(app): + system_info = {'domains': { + domain.name: { + 'tags': list(domain.tags), + 'type': domain.__class__.__name__, + 'template_for_dispvms': + getattr(domain, 'template_for_dispvms', False), + 'default_dispvm': (domain.default_dispvm.name if + getattr(domain, 'default_dispvm', None) else None), + 'icon': str(domain.label.icon), + 'guivm': (domain.guivm.name if getattr(domain, 'guivm', None) + else None), + } for domain in app.domains + }} + return system_info + + class QubesInternalAPI(qubes.api.AbstractQubesAPI): ''' Communication interface for dom0 components, by design the input here is trusted.''' @@ -42,19 +59,7 @@ class QubesInternalAPI(qubes.api.AbstractQubesAPI): self.enforce(self.dest.name == 'dom0') self.enforce(not self.arg) - system_info = {'domains': { - domain.name: { - 'tags': list(domain.tags), - 'type': domain.__class__.__name__, - 'template_for_dispvms': - getattr(domain, 'template_for_dispvms', False), - 'default_dispvm': (domain.default_dispvm.name if - getattr(domain, 'default_dispvm', None) else None), - 'icon': str(domain.label.icon), - 'guivm': (domain.guivm.name if getattr(domain, 'guivm', None) - else None), - } for domain in self.app.domains - }} + system_info = get_system_info(self.app) return json.dumps(system_info) From 3f96c72ee360f8fca3df0a76efc74bfbea1693f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 21 Feb 2020 03:54:14 +0100 Subject: [PATCH 2/5] api/admin: (ext/admin) limit listing VMs based on qrexec policy Various Admin API calls, when directed at dom0, retrieve global system view instead of a specific VM. This applies to admin.vm.List (called at dom0 retrieve full VM list) and admin.Events (called at dom0 listen for events of all the VMs). This makes it tricky to configure a management VM with access to limited set of VMs only, because many tools require ability to list VMs, and that would return full list. Fix this issue by adding a filter to admin.vm.List and admin.Events calls (using event handlers in AdminExtension) that filters the output using qrexec policy. This version evaluates policy for each VM or event (but loads only once). If the performance will be an issue, it can be optimized later. Fixes QubesOS/qubes-issues#5509 --- qubes/ext/admin.py | 101 +++++++++++++++++++++++++++++++++++++++ qubes/tests/api_admin.py | 59 +++++++++++++++++++++++ 2 files changed, 160 insertions(+) diff --git a/qubes/ext/admin.py b/qubes/ext/admin.py index 5f928821..3f26f568 100644 --- a/qubes/ext/admin.py +++ b/qubes/ext/admin.py @@ -18,10 +18,28 @@ # License along with this library; if not, see . import qubes.api +import qubes.api.internal import qubes.ext import qubes.vm.adminvm +from qrexec.policy import utils, parser + + +class JustEvaluateAskResolution(parser.AskResolution): + def execute(self, caller_ident): + pass + + +class JustEvaluateAllowResolution(parser.AllowResolution): + def execute(self, caller_ident): + pass + class AdminExtension(qubes.ext.Extension): + def __init__(self): + super(AdminExtension, self).__init__() + self.policy_cache = utils.PolicyCache(lazy_load=True) + self.policy_cache.initialize_watcher() + # pylint: disable=too-few-public-methods @qubes.ext.handler( 'admin-permission:admin.vm.tag.Set', @@ -36,3 +54,86 @@ class AdminExtension(qubes.ext.Extension): __name__, type(self).__name__)) # TODO create that tag here (need to figure out how to pass mgmtvm name) + + @qubes.ext.handler('admin-permission:admin.vm.List') + def admin_vm_list(self, vm, event, arg, **kwargs): + '''When called with target 'dom0' (aka "get full list"), exclude domains + that the caller don't have permission to list + ''' + # pylint: disable=unused-argument + + if vm.klass == 'AdminVM': + # dom0 can always list everything + return None + + policy = self.policy_cache.get_policy() + system_info = qubes.api.internal.get_system_info(vm.app) + + def filter_vms(dest_vm): + request = parser.Request( + 'admin.vm.List', + '+' + arg, + vm.name, + dest_vm.name, + system_info=system_info, + ask_resolution_type=JustEvaluateAskResolution, + allow_resolution_type=JustEvaluateAllowResolution) + try: + resolution = policy.evaluate(request) + # do not consider 'ask' as allow here, + # this needs to be not interactive + return isinstance(resolution, parser.AllowResolution) + except parser.AccessDenied: + return False + + return (filter_vms,) + + @qubes.ext.handler('admin-permission:admin.Events') + def admin_events(self, vm, event, arg, **kwargs): + '''When called with target 'dom0' (aka "get all events"), + exclude domains that the caller don't have permission to receive + events about + ''' + # pylint: disable=unused-argument + + if vm.klass == 'AdminVM': + # dom0 can always list everything + return None + + def filter_events(event): + subject, event, kwargs = event + try: + dest = subject.name + except AttributeError: + # domain-add and similar events fired on the Qubes() object + if 'vm' in kwargs: + dest = kwargs['vm'].name + else: + dest = '@adminvm' + + policy = self.policy_cache.get_policy() + # TODO: cache system_info (based on last qubes.xml write time?) + system_info = qubes.api.internal.get_system_info(vm.app) + request = parser.Request( + 'admin.Events', + '+' + event.replace(':', '_'), + vm.name, + dest, + system_info=system_info, + ask_resolution_type=JustEvaluateAskResolution, + allow_resolution_type=JustEvaluateAllowResolution) + try: + resolution = policy.evaluate(request) + # do not consider 'ask' as allow here, + # this needs to be not interactive + return isinstance(resolution, parser.AllowResolution) + except parser.AccessDenied: + return False + + return (filter_events,) + + @qubes.ext.handler('qubes-close', system=True) + def on_qubes_close(self, app, event, **kwargs): + """Unregister policy file watches on app.close().""" + # pylint: disable=unused-argument + self.policy_cache.cleanup() diff --git a/qubes/tests/api_admin.py b/qubes/tests/api_admin.py index ab8b7373..0e7a1aa5 100644 --- a/qubes/tests/api_admin.py +++ b/qubes/tests/api_admin.py @@ -30,6 +30,8 @@ import unittest.mock import libvirt import copy +import pathlib + import qubes import qubes.devices import qubes.firewall @@ -135,6 +137,24 @@ class TC_00_VMs(AdminAPITestCase): self.assertEqual(value, 'test-vm1 class=AppVM state=Halted\n') + def test_002_vm_list_filter(self): + with tempfile.TemporaryDirectory() as tmpdir: + tmpdir = pathlib.Path(tmpdir) + with unittest.mock.patch( + 'qubes.ext.admin.AdminExtension._instance.policy_cache.path', + pathlib.Path(tmpdir)): + with (tmpdir / 'admin.policy').open('w') as f: + f.write('admin.vm.List * @anyvm @adminvm allow\n') + f.write('admin.vm.List * @anyvm test-vm1 allow') + mgmt_obj = qubes.api.admin.QubesAdminAPI(self.app, b'test-vm1', + b'admin.vm.List', b'dom0', b'') + loop = asyncio.get_event_loop() + 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') + def test_010_vm_property_list(self): # this test is kind of stupid, but at least check if appropriate # admin-permission event is fired @@ -1112,6 +1132,45 @@ netvm default=True type=vm \n''' unittest.mock.call(vm2, 'test-event2', arg1='abc'), ]) + def test_272_events_filter(self): + with tempfile.TemporaryDirectory() as tmpdir: + tmpdir = pathlib.Path(tmpdir) + with unittest.mock.patch( + 'qubes.ext.admin.AdminExtension._instance.policy_cache.path', + pathlib.Path(tmpdir)): + with (tmpdir / 'admin.policy').open('w') as f: + f.write('admin.Events * @anyvm @adminvm allow\n') + f.write('admin.Events * @anyvm test-vm1 allow') + + send_event = unittest.mock.Mock(spec=[]) + mgmt_obj = qubes.api.admin.QubesAdminAPI(self.app, b'test-vm1', + b'admin.Events', + b'dom0', b'', send_event=send_event) + + @asyncio.coroutine + def fire_event(): + # add VM _after_ starting admin.Events call + vm = self.app.add_new_vm('AppVM', label='red', + name='test-vm2', + template='test-template') + vm.fire_event('test-event2', arg1='abc') + self.vm.fire_event('test-event', arg1='abc') + mgmt_obj.cancel() + return vm + + loop = asyncio.get_event_loop() + execute_task = asyncio.ensure_future( + mgmt_obj.execute(untrusted_payload=b'')) + event_task = asyncio.ensure_future(fire_event()) + loop.run_until_complete(execute_task) + vm2 = event_task.result() + self.assertIsNone(execute_task.result()) + self.assertEqual(send_event.mock_calls, + [ + unittest.mock.call(self.app, 'connection-established'), + unittest.mock.call(self.vm, 'test-event', arg1='abc'), + ]) + def test_280_feature_list(self): self.vm.features['test-feature'] = 'some-value' value = self.call_mgmt_func(b'admin.vm.feature.List', b'test-vm1') From a90e7e365e33781cb4e48bf9229498b7becd54af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 21 Feb 2020 05:39:15 +0100 Subject: [PATCH 3/5] travis: include core-qrexec in tests --- .travis.yml | 5 +++-- ci/requirements.txt | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index e0a7c20c..a280033e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -11,9 +11,10 @@ install: - sudo apt-get -y install python3-gi gir1.2-gtk-3.0 - pip install --quiet -r ci/requirements.txt - git clone https://github.com/"${TRAVIS_REPO_SLUG%%/*}"/qubes-builder ~/qubes-builder + - git clone https://github.com/"${TRAVIS_REPO_SLUG%%/*}"/qubes-core-qrexec ~/qubes-core-qrexec script: - - PYTHONPATH=test-packages pylint qubes - - ./run-tests + - PYTHONPATH=test-packages:~/qubes-core-qrexec pylint qubes + - PYTHONPATH=test-packages:~/qubes-core-qrexec ./run-tests - ~/qubes-builder/scripts/travis-build env: - DIST_DOM0=fc31 USE_QUBES_REPO_VERSION=4.1 USE_QUBES_REPO_TESTING=1 diff --git a/ci/requirements.txt b/ci/requirements.txt index 311d186f..c40d5ffc 100644 --- a/ci/requirements.txt +++ b/ci/requirements.txt @@ -8,3 +8,4 @@ lxml pylint sphinx PyYAML +pyinotify From 0341cc52582f8ca7f5166ed1840ee8b6a4b1f20e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Tue, 10 Mar 2020 01:43:42 +0100 Subject: [PATCH 4/5] tests: teardown fixes Add few missing app.close() calls on test teardown. Fix socket cleanup in TC_00_QubesDaemonProtocol() - not only close the FD, but also unregister it from asyncio event loop. --- qubes/tests/api.py | 8 ++++++-- qubes/tests/app.py | 6 ++++++ qubes/tests/storage_lvm.py | 2 ++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/qubes/tests/api.py b/qubes/tests/api.py index 3c3e1e1a..41b1c2d5 100644 --- a/qubes/tests/api.py +++ b/qubes/tests/api.py @@ -109,8 +109,12 @@ class TC_00_QubesDaemonProtocol(qubes.tests.QubesTestCase): connect_coro) def tearDown(self): - self.sock_server.close() - self.sock_client.close() + self.writer.close() + try: + self.loop.run_until_complete(self.writer.wait_closed()) + except AttributeError: # old python in travis + pass + self.transport.close() super(TC_00_QubesDaemonProtocol, self).tearDown() def test_000_message_ok(self): diff --git a/qubes/tests/app.py b/qubes/tests/app.py index f4c202ea..a9c47ce0 100644 --- a/qubes/tests/app.py +++ b/qubes/tests/app.py @@ -480,6 +480,9 @@ class TC_89_QubesEmpty(qubes.tests.QubesTestCase): with self.assertRaises(AttributeError): self.app.default_fw_netvm + self.app.close() + del self.app + with self.subTest('loop'): with open('/tmp/qubestest.xml', 'w') as xml_file: xml_file.write(xml_template.format( @@ -502,6 +505,9 @@ class TC_89_QubesEmpty(qubes.tests.QubesTestCase): with self.assertRaises(AttributeError): self.app.default_fw_netvm + self.app.close() + del self.app + class TC_90_Qubes(qubes.tests.QubesTestCase): def tearDown(self): diff --git a/qubes/tests/storage_lvm.py b/qubes/tests/storage_lvm.py index 5646ac9f..8d297c6f 100644 --- a/qubes/tests/storage_lvm.py +++ b/qubes/tests/storage_lvm.py @@ -131,6 +131,7 @@ class TC_00_ThinPool(ThinPoolBase): def tearDown(self): super(TC_00_ThinPool, self).tearDown() os.unlink(self.app.store) + self.app.close() del self.app for attr in dir(self): if isinstance(getattr(self, attr), qubes.vm.BaseVM): @@ -1141,6 +1142,7 @@ class TC_02_StorageHelpers(ThinPoolBase): self.thin_dir.cleanup() super(TC_02_StorageHelpers, self).tearDown() os.unlink(self.app.store) + self.app.close() del self.app for attr in dir(self): if isinstance(getattr(self, attr), qubes.vm.BaseVM): From b11d6e058b659d6aa958a8bd2a453ca6f3296f5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Tue, 31 Mar 2020 01:57:22 +0200 Subject: [PATCH 5/5] ext/admin: workaround for extension's __init__() called multiple times ... during tests. qubes.ext.Extension class is a weird thing that tries to make each extension a singleton. But this unfortunately have a side effect that __init__() is called separately for each "instance" (created in Qubes()'s __init__()), even though this is really the same object. During normal execution this isn't an issue, because there is just one Qubes() object instance. But during tests, multiple objects are created. In this particular case, it caused PolicyCache() to be created twice and the second one overriden the first one - without properly cleaning it up. This leaks a file descriptor (inotify one). The fact that cleanup() was called twice too didn't helped, because it was really called on the same object, the one requiring cleanup was already gone. Workaround this by checking if policy_cache field is initialize and avoid re-initialize it. Also, on Qubes() object cleanup remove that field, so it can be properly initialized on the next test iteration. --- qubes/ext/admin.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/qubes/ext/admin.py b/qubes/ext/admin.py index 3f26f568..d16e05a1 100644 --- a/qubes/ext/admin.py +++ b/qubes/ext/admin.py @@ -37,8 +37,11 @@ class JustEvaluateAllowResolution(parser.AllowResolution): class AdminExtension(qubes.ext.Extension): def __init__(self): super(AdminExtension, self).__init__() - self.policy_cache = utils.PolicyCache(lazy_load=True) - self.policy_cache.initialize_watcher() + # during tests, __init__() of the extension can be called multiple + # times, because there are multiple Qubes() object instances + if not hasattr(self, 'policy_cache'): + self.policy_cache = utils.PolicyCache(lazy_load=True) + self.policy_cache.initialize_watcher() # pylint: disable=too-few-public-methods @qubes.ext.handler( @@ -136,4 +139,6 @@ class AdminExtension(qubes.ext.Extension): def on_qubes_close(self, app, event, **kwargs): """Unregister policy file watches on app.close().""" # pylint: disable=unused-argument - self.policy_cache.cleanup() + if hasattr(self, 'policy_cache'): + self.policy_cache.cleanup() + del self.policy_cache