admin: validate feature name in vm_feature_remove

Feature name validation was missing in vm_feature_remove while
vm_feature_set already validated it, causing inconsistent behavior
between dom0 and management qube.

Add shared _validate_feature_name() function to avoid future
discrepancies. Use ProtocolError consistent with PR #751.

Also validate arg in AbstractQubesAPI.__init__() consistent with
sanitize_name() in qrexec-daemon.c.

Fixes: QubesOS/qubes-issues#7186
Related: #751

Author: Tarek

qubes/api/__init__.py

@@ -166,6 +166,10 @@ def __init__(
 
         #: argument
         self.arg = arg.decode("ascii")
+        # Validate arg: must only contain characters allowed by qrexec
+        # See sanitize_name() in qrexec-daemon.c
+        if self.arg and not re.match(r"\A[\w.-]+\Z", self.arg):
+            raise ProtocolError(f"arg not allowed in qrexec: {self.arg!r}")
 
         #: name of the method
         self.method = method_name.decode("ascii")
@@ -232,7 +236,7 @@ def fire_event_for_permission(self, **kwargs):
             pre_event=True,
             dest=self.dest,
             arg=self.arg,
-            **kwargs
+            **kwargs,
         )
 
     def fire_event_for_filter(self, iterable, **kwargs):

qubes/api/admin.py

@@ -100,6 +100,18 @@ def on_domain_delete(self, subject, event, vm):
         vm.remove_handler("*", self.vm_handler)
 
 
+def _validate_feature_name(name):
+    """Validate feature name against allowed character set.
+
+    Allowed characters: alphanumeric, dot, underscore, hyphen.
+    Raises ProtocolError if name is invalid.
+    """
+    if not re.match(r"\A[a-zA-Z0-9_.-]+\Z", name):
+        raise qubes.exc.ProtocolError(
+            "feature name contains illegal characters"
+        )
+
+
 class QubesAdminAPI(qubes.api.AbstractQubesAPI):
     """Implementation of Qubes Management API calls
 
@@ -1162,8 +1174,7 @@ async def vm_feature_checkwithtpladminvm(self):
         "admin.vm.feature.Remove", no_payload=True, scope="local", write=True
     )
     async def vm_feature_remove(self):
-        # validation of self.arg done by qrexec-policy is enough
-
+        _validate_feature_name(self.arg)
         self.fire_event_for_permission()
         try:
             del self.dest.features[self.arg]
@@ -1175,10 +1186,7 @@ async def vm_feature_remove(self):
     async def vm_feature_set(self, untrusted_payload):
         untrusted_value = untrusted_payload.decode("ascii", errors="strict")
         del untrusted_payload
-        if re.match(r"\A[a-zA-Z0-9_.-]+\Z", self.arg) is None:
-            raise qubes.exc.QubesValueError(
-                "feature name contains illegal characters"
-            )
+        _validate_feature_name(self.arg)
         if re.match(r"\A[\x20-\x7E]*\Z", untrusted_value) is None:
             raise qubes.exc.QubesValueError(
                 f"{self.arg} value contains illegal characters"
@@ -2021,7 +2029,7 @@ def _send_stats_single(
         the next iteration)
         """
 
-        (info_time, info) = self.app.host.get_vm_stats(
+        info_time, info = self.app.host.get_vm_stats(
             info_time, info, only_vm=only_vm
         )
         for vm_id, vm_info in info.items():

qubes/tests/api_admin.py

@@ -1694,6 +1694,27 @@ def test_301_feature_remove_none(self):
             )
         self.assertFalse(self.app.save.called)
 
+    def test_302_feature_remove_invalid_name(self):
+        # Invalid feature names must raise ProtocolError before any action
+        with self.assertRaises(qubes.exc.ProtocolError):
+            self.call_mgmt_func(
+                b"admin.vm.feature.Remove",
+                b"test-vm1",
+                b"../../../root/gotcha",
+            )
+        self.assertFalse(self.app.save.called)
+
+    def test_302b_feature_set_invalid_name(self):
+        # Invalid feature names must raise ProtocolError in vm_feature_set
+        with self.assertRaises(qubes.exc.ProtocolError):
+            self.call_mgmt_func(
+                b"admin.vm.feature.Set",
+                b"test-vm1",
+                b"../../../root/gotcha",
+                b"some-value",
+            )
+        self.assertFalse(self.app.save.called)
+
     def test_303_feature_prohibited(self):
         del self.app.domains[0].fire_event
         feature = qubes.ext.admin.PROHIBITED_FEATURES[0]