From 0ec452edaa6ba97889073b25e477ac38b4785947 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 31 May 2024 13:25:36 +0200 Subject: [PATCH 1/3] Revert "Validate service names" Conditions checked here are bogus (the only relevant one is length). Furthermore it's done in wrong place (should be pre- event). This reverts commit a3aea50671fe7b8263cd8a55cf9a8584d9887ad3. Fixes QubesOS/qubes-issues#9274 --- qubes/ext/services.py | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/qubes/ext/services.py b/qubes/ext/services.py index 82b225fd5..8e77620c3 100644 --- a/qubes/ext/services.py +++ b/qubes/ext/services.py @@ -88,28 +88,13 @@ def on_domain_feature_set(self, vm, event, feature, value, oldvalue=None): # balancing state anymore del vm.features['service.meminfo-writer'] + if not vm.is_running(): + return if not feature.startswith('service.'): return service = feature[len('service.'):] - # qubesdb keys are limited to 63 bytes, and "/qubes-service/" is - # 15 bytes. That leaves 48 for the service name. - if len(service) > 48: - raise qubes.exc.QubesValueError( - 'Service name must not exceed 48 bytes') - # The empty string is not a valid file name. - if not service: - raise qubes.exc.QubesValueError('Empty service name not allowed') - # Require service names to start with an ASCII letter. This implicitly - # rejects names which start with '-' (which could be interpreted as an - # option) or are '.' or '..'. - if not (('a' <= service[0] <= 'z') or ('A' <= service[0] <= 'Z')): - raise qubes.exc.QubesValueError( - 'Service name must start with an ASCII letter') - - if not vm.is_running(): - return # forcefully convert to '0' or '1' - vm.untrusted_qdb.write('/qubes-service/' + service, + vm.untrusted_qdb.write('/qubes-service/{}'.format(service), str(int(bool(value)))) if vm.name == "dom0": From cf8a863c161f345b51521a4b0bdb5f7907903d98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 31 May 2024 13:33:30 +0200 Subject: [PATCH 2/3] Verify if service names are not too long Reject them before actually setting them. But if there is already some too-long set, ignore it with a warning instead of breaking VM startup. And also remove test for too strict verification. --- qubes/ext/services.py | 21 +++++++++++++++++++++ qubes/tests/api_admin.py | 7 ------- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/qubes/ext/services.py b/qubes/ext/services.py index 8e77620c3..3b4c91694 100644 --- a/qubes/ext/services.py +++ b/qubes/ext/services.py @@ -62,6 +62,12 @@ def on_domain_qdb_create(self, vm, event): if not feature.startswith('service.'): continue service = feature[len('service.'):] + if not service: + vm.log.warning("Empty service name, ignoring: " + service) + continue + if len(service) > 48: + vm.log.warning("Too long service name, ignoring: " + service) + continue # forcefully convert to '0' or '1' vm.untrusted_qdb.write('/qubes-service/{}'.format(service), str(int(bool(value)))) @@ -70,6 +76,21 @@ def on_domain_qdb_create(self, vm, event): vm.untrusted_qdb.write('/qubes-service/meminfo-writer', '1' if vm.maxmem > 0 else '0') + @qubes.ext.handler('domain-feature-pre-set:*') + def on_domain_feature_pre_set(self, vm, event, feature, + value, oldvalue=None): + """Check if service name is compatible with QubesDB""" + # pylint: disable=unused-argument + if not feature.startswith('service.'): + return + service = feature[len('service.'):] + if not service: + raise qubes.exc.QubesValueError( + 'Service name cannot be empty') + if len(service) > 48: + raise qubes.exc.QubesValueError( + 'Service name must not exceed 48 bytes') + @qubes.ext.handler('domain-feature-set:*') def on_domain_feature_set(self, vm, event, feature, value, oldvalue=None): """Update /qubes-service/ QubesDB tree in runtime""" diff --git a/qubes/tests/api_admin.py b/qubes/tests/api_admin.py index 6faac2ff4..c24d29f12 100644 --- a/qubes/tests/api_admin.py +++ b/qubes/tests/api_admin.py @@ -1403,13 +1403,6 @@ def test_323_feature_set_service_too_long(self): self.assertNotIn('test-feature', self.vm.features) self.assertFalse(self.app.save.called) - def test_324_feature_set_service_bad_name(self): - with self.assertRaises(qubes.exc.QubesValueError): - self.call_mgmt_func(b'admin.vm.feature.Set', - b'test-vm1', b'service.0') - self.assertNotIn('test-feature', self.vm.features) - self.assertFalse(self.app.save.called) - def test_325_feature_set_service_empty_name(self): with self.assertRaises(qubes.exc.QubesValueError): self.call_mgmt_func(b'admin.vm.feature.Set', From 1ff10c9b6067aa2ab562f9d10b276860d5236aec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Wed, 12 Jun 2024 04:42:07 +0200 Subject: [PATCH 3/3] Refuse names that would not be accessible in /run/qubes-service anyway This applies to '.', '..' and anything with a slash. Technically those services can still be accessible via qubesdb, but that's not really intuitive and may look like a silent failure. But since technically those can be seen by a VM, do not exclude them from qubesdb, if somebody already set some of those names. Refuse only new values. QubesOS/qubes-issues#9274 --- qubes/ext/services.py | 9 +++++++++ qubes/tests/ext.py | 7 +++++++ 2 files changed, 16 insertions(+) diff --git a/qubes/ext/services.py b/qubes/ext/services.py index 3b4c91694..750ed5939 100644 --- a/qubes/ext/services.py +++ b/qubes/ext/services.py @@ -87,6 +87,15 @@ def on_domain_feature_pre_set(self, vm, event, feature, if not service: raise qubes.exc.QubesValueError( 'Service name cannot be empty') + + if '/' in service: + raise qubes.exc.QubesValueError( + 'Service name cannot contain a slash') + + if service in ('.', '..'): + raise qubes.exc.QubesValueError( + 'Service name cannot be "." or ".."') + if len(service) > 48: raise qubes.exc.QubesValueError( 'Service name must not exceed 48 bytes') diff --git a/qubes/tests/ext.py b/qubes/tests/ext.py index 49c37df63..7b39d8adb 100644 --- a/qubes/tests/ext.py +++ b/qubes/tests/ext.py @@ -487,6 +487,13 @@ def test_002_feature_delete(self): ('rm', ('/qubes-service/test3',), {}), ]) + def test_003_feature_set_invalid(self): + for val in ('', '.', 'a/b', 'aa' * 30): + with self.assertRaises(qubes.exc.QubesValueError): + self.ext.on_domain_feature_pre_set(self.vm, + 'feature-set:service.' + val, + 'service.' + val, '1') + def test_010_supported_services(self): self.ext.supported_services(self.vm, 'features-request', untrusted_features={