Merge remote-tracking branch 'origin/pr/595'

* origin/pr/595:
  Refuse names that would not be accessible in /run/qubes-service anyway
  Verify if service names are not too long
  Revert "Validate service names"

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,30 @@ 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 '/' 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')
+
     @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"""
@@ -88,28 +118,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":

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',

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={