From 0afee4b05e5cdeef6cc2415a235e7a50df9b78ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 1 Dec 2017 02:59:17 +0100 Subject: [PATCH] Improve checking for netvm loop There were many cases were the check was missing: - changing default_netvm - resetting netvm to default value - loading already broken qubes.xml Since it was possible to create broken qubes.xml using legal calls, do not reject loading such file, instead break the loop(s) by setting netvm to None when loop is detected. This will be also useful if still not all places are covered... Place the check in default_netvm setter. Skip it during qubes.xml loading (when events_enabled=False), but still keep it in setter, to _validate_ the value before any property-* event got fired. --- qubes/app.py | 22 ++++++++++++++++++++++ qubes/tests/app.py | 25 +++++++++++++++++++++++++ qubes/tests/vm/mix/net.py | 4 ++-- qubes/vm/mix/net.py | 29 +++++++++++++++++++++++++---- 4 files changed, 74 insertions(+), 6 deletions(-) diff --git a/qubes/app.py b/qubes/app.py index 58d2c3db..b78731eb 100644 --- a/qubes/app.py +++ b/qubes/app.py @@ -606,6 +606,27 @@ def _setter_pool(app, prop, value): raise qubes.exc.QubesPropertyValueError(app, prop, value, 'No such storage pool') +def _setter_default_netvm(app, prop, value): + # skip netvm loop check while loading qubes.xml, to avoid tricky loading + # order + if not app.events_enabled: + return value + + if value is None: + return value + # forbid setting to a value that would result in netvm loop + for vm in app.domains: + if not hasattr(vm, 'netvm'): + continue + if not vm.property_is_default('netvm'): + continue + if value == vm \ + or value in app.domains.get_vms_connected_to(vm): + raise qubes.exc.QubesPropertyValueError(app, prop, value, + 'Network loop on \'{!s}\''.format(vm)) + return value + + class Qubes(qubes.PropertyHolder): '''Main Qubes application @@ -664,6 +685,7 @@ class Qubes(qubes.PropertyHolder): default_netvm = qubes.VMProperty('default_netvm', load_stage=3, default=None, allow_none=True, + setter=_setter_default_netvm, doc='''Default NetVM for AppVMs. Initial state is `None`, which means that AppVMs are not connected to the Internet.''') default_fw_netvm = qubes.VMProperty('default_fw_netvm', load_stage=3, diff --git a/qubes/tests/app.py b/qubes/tests/app.py index 3b3b7944..69b494ee 100644 --- a/qubes/tests/app.py +++ b/qubes/tests/app.py @@ -327,6 +327,30 @@ class TC_90_Qubes(qubes.tests.QubesTestCase): self.assertIn('service.clocksync', self.template.features) self.assertTrue(self.template.features['service.clocksync']) + def test_110_netvm_loop(self): + '''Netvm loop through default_netvm''' + netvm = self.app.add_new_vm('AppVM', name='test-net', + template=self.template, label='red') + try: + self.app.default_netvm = None + netvm.netvm = qubes.property.DEFAULT + with self.assertRaises(ValueError): + self.app.default_netvm = netvm + finally: + del netvm + + def test_111_netvm_loop(self): + '''Netvm loop through default_netvm''' + netvm = self.app.add_new_vm('AppVM', name='test-net', + template=self.template, label='red') + try: + netvm.netvm = None + self.app.default_netvm = netvm + with self.assertRaises(ValueError): + netvm.netvm = qubes.property.DEFAULT + finally: + del netvm + def test_200_remove_template(self): appvm = self.app.add_new_vm('AppVM', name='test-vm', template=self.template, @@ -351,6 +375,7 @@ class TC_90_Qubes(qubes.tests.QubesTestCase): netvm = self.app.add_new_vm('AppVM', name='test-netvm', template=self.template, provides_network=True, label='red') + netvm.netvm = None self.app.default_netvm = netvm with mock.patch.object(self.app, 'vmm'): with self.assertRaises(qubes.exc.QubesVMInUseError): diff --git a/qubes/tests/vm/mix/net.py b/qubes/tests/vm/mix/net.py index 839561af..aa089eec 100644 --- a/qubes/tests/vm/mix/net.py +++ b/qubes/tests/vm/mix/net.py @@ -40,10 +40,10 @@ class TC_00_NetVMMixin( # testing properties used here self.netvm1 = qubes.vm.qubesvm.QubesVM(self.app, None, qid=2, name=qubes.tests.VMPREFIX + 'netvm1', - provides_network=True) + provides_network=True, netvm=None) self.netvm2 = qubes.vm.qubesvm.QubesVM(self.app, None, qid=3, name=qubes.tests.VMPREFIX + 'netvm2', - provides_network=True) + provides_network=True, netvm=None) self.nonetvm = qubes.vm.qubesvm.QubesVM(self.app, None, qid=4, name=qubes.tests.VMPREFIX + 'nonet') self.app.domains = qubes.app.VMCollection(self.app) diff --git a/qubes/vm/mix/net.py b/qubes/vm/mix/net.py index 7b09ff9f..f1911ae9 100644 --- a/qubes/vm/mix/net.py +++ b/qubes/vm/mix/net.py @@ -70,10 +70,13 @@ def _setter_netvm(self, prop, value): raise qubes.exc.QubesValueError( 'The {!s} qube does not provide network'.format(value)) - if value is self \ - or value in self.app.domains.get_vms_connected_to(self): - raise qubes.exc.QubesValueError( - 'Loops in network are unsupported') + # skip check for netvm loops during qubes.xml loading, to avoid tricky + # loading order + if self.events_enabled: + if value is self \ + or value in self.app.domains.get_vms_connected_to(self): + raise qubes.exc.QubesValueError( + 'Loops in network are unsupported') return value @@ -187,6 +190,22 @@ class NetVMMixin(qubes.events.Emitter): self._firewall = None super(NetVMMixin, self).__init__(*args, **kwargs) + @qubes.events.handler('domain-load') + def on_domain_load_netvm_loop_check(self, event): + # pylint: disable=unused-argument + # make sure there are no netvm loops - which could cause qubesd + # looping infinitely + if self is self.netvm: + self.log.error( + 'vm \'%s\' network-connected to itself, breaking the ' + 'connection', self.name) + self.netvm = None + elif self.netvm in self.app.domains.get_vms_connected_to(self): + self.log.error( + 'netvm loop detected on \'%s\', breaking the connection', + self.name) + self.netvm = None + @qubes.events.handler('domain-start') def on_domain_started(self, event, **kwargs): '''Connect this domain to its downstream domains. Also reload firewall @@ -323,6 +342,8 @@ class NetVMMixin(qubes.events.Emitter): # pylint: disable=unused-argument # we are changing to default netvm newvalue = type(self).netvm.get_default(self) + # check for netvm loop + _setter_netvm(self, type(self).netvm, newvalue) if newvalue == oldvalue: return self.fire_event('property-pre-set:netvm', pre_event=True,