admin: validate feature name in vm_feature_remove

Feature name validation was missing in vm_feature_remove, while
vm_feature_set already validated it. This inconsistency allowed
path traversal characters in feature names when removing features.

Also use ProtocolError instead of QubesValueError for invalid
feature names in vm_feature_set, consistent with PR #751.

Fixes: QubesOS/qubes-issues#7186

Author: Tarek

qubes/api/admin.py

@@ -1162,8 +1162,12 @@ 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 to prevent path traversal and ensure
+        # consistency with the Admin API validation in vm_feature_set
+        if re.match(r"\A[a-zA-Z0-9_.-]+\Z", self.arg) is None:
+            raise qubes.exc.ProtocolError(
+                "feature name contains illegal characters"
+            )
         self.fire_event_for_permission()
         try:
             del self.dest.features[self.arg]
@@ -1176,7 +1180,7 @@ 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(
+            raise qubes.exc.ProtocolError(
                 "feature name contains illegal characters"
             )
         if re.match(r"\A[\x20-\x7E]*\Z", untrusted_value) is None:

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]