From 0843cf914472b296d2d32dc2f5bc734a074ae005 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Tue, 31 Dec 2024 01:23:53 -0500 Subject: [PATCH 01/11] Use correct internal exception The exceptions ProtocolError and PermissionDenied are both raised by qubesd to indicate various problems with requests. However, both cause clients to print the extremely unhelpful "Got empty response from qubesd" message. Use ProtocolError on invalid requests. Use PermissionDenied when the client doesn't have permission to know something, such as a missing object, specified argument should be a pool but said pool doesn't exist. Therefore, a API handler can be arranged as: - Validation: ProtocolError otherwise - fire_event_for_permission(): PermissionDenied if unauthorized - Action Ideally, validation that may leak existence of a system property should be done after asking for administrative permission, but then it would not be possible to pass only safe values to "admin-permission:" event. If we are already leaking existence of a property, it makes sense to provide a useful exception class for it. Strict validation to API method parameters makes it possible to enforce presence, absence (or both states) for argument and payload. Don't leave things for chance, require passing explicitly if argument and payload should be passed or not, and if destination should be AdminVM. Passing "**classifiers" means it doesn't check if the argument passed is a valid classifier or valid value for the classifier. Decided to be explicit on the allowed classifiers with sane defaults of not setting the scope at all and only having read capability. Fixes: https://github.com/QubesOS/qubes-issues/issues/10345 --- doc/qubes-exc.rst | 2 +- qubes-rpc-policy/generate-admin-policy | 16 +- qubes/api/__init__.py | 215 +++-- qubes/api/admin.py | 1042 +++++++++++++++++------- qubes/api/internal.py | 55 +- qubes/api/misc.py | 42 +- qubes/exc.py | 103 ++- qubes/tests/api.py | 85 ++ qubes/tests/api_admin.py | 383 +++++++-- qubes/tests/api_misc.py | 11 +- qubes/tests/vm/dispvm.py | 2 +- qubes/tests/vm/qubesvm.py | 9 + qubes/utils.py | 40 + qubes/vm/__init__.py | 3 +- 14 files changed, 1541 insertions(+), 467 deletions(-) diff --git a/doc/qubes-exc.rst b/doc/qubes-exc.rst index f3a644b14..0b1c45e40 100644 --- a/doc/qubes-exc.rst +++ b/doc/qubes-exc.rst @@ -43,7 +43,7 @@ call for effect). Instead consider writing in negative form, implying expected state: "Domain is not running" instead of "Domain is paused" (yeah, what's wrong with that?). -Also avoid implying the personhood of the computer, including adressing user in +Also avoid implying the personhood of the computer, including addressing user in second person. For example, write "Sending message failed" instead of "I failed to send the message". diff --git a/qubes-rpc-policy/generate-admin-policy b/qubes-rpc-policy/generate-admin-policy index a724a375c..31ff0b196 100755 --- a/qubes-rpc-policy/generate-admin-policy +++ b/qubes-rpc-policy/generate-admin-policy @@ -43,19 +43,19 @@ parser.add_argument('--exclude', action='store', nargs='*', parser.add_argument('service', nargs='*', action='store', help='Generate policy for those services (default: all)') -def write_default_policy(args, apiname, clasifiers, f): +def write_default_policy(args, apiname, classifiers, f): ''' Write single default policy for given API call ''' - assert 'scope' in clasifiers, \ + assert 'scope' in classifiers, \ 'Method {} lack scope classifier'.format(apiname) - assert any(attr in clasifiers for attr in ('read', 'write', 'execute')), \ + assert any(attr in classifiers for attr in ('read', 'write', 'execute')), \ 'Method {} lack read/write/execute classifier'.format(apiname) - assert clasifiers['scope'] in ('local', 'global'), \ - 'Method {} have invalid scope: {}'.format(apiname, clasifiers['scope']) + assert classifiers['scope'] in ('local', 'global'), \ + 'Method {} have invalid scope: {}'.format(apiname, classifiers['scope']) file_to_include = 'admin-{scope}-{rwx}'.format( - scope=clasifiers['scope'], - rwx=('rwx' if clasifiers.get('write', False) or - clasifiers.get('execute', False) + scope=classifiers['scope'], + rwx=('rwx' if classifiers.get('write', False) or + classifiers.get('execute', False) else 'ro')) if args.verbose: diff --git a/qubes/api/__init__.py b/qubes/api/__init__.py index 764c98679..04bf7d00c 100644 --- a/qubes/api/__init__.py +++ b/qubes/api/__init__.py @@ -27,16 +27,28 @@ import re import shutil import socket +import string import struct import traceback -from typing import Union, Any +from typing import Union, Any, Literal, Generator import uuid import qubes.exc from qubes.exc import ProtocolError, PermissionDenied -def method(name, *, no_payload=False, endpoints=None, **classifiers): +def method( + name, + *, + dest_adminvm: bool | None, + wants_arg: bool | None, + wants_payload: bool | None, + scope: Literal["local", "global"] | None = None, + read: bool = True, + write: bool = False, + execute: bool = False, + endpoints: Generator[str, None, None] | None = None, +): """Decorator factory for methods intended to appear in API. The decorated method can be called from public API using a child of @@ -44,10 +56,27 @@ def method(name, *, no_payload=False, endpoints=None, **classifiers): called using remote management interface. :param str name: qrexec rpc method name - :param bool no_payload: if :py:obj:`True`, will barf on non-empty payload; \ - also will not pass payload at all to the method + :param bool dest_adminvm: if :py:obj:`True`, will barf if destination is \ + not AdminVM; If :py:obj:`False`, will barf if destination is AdminVM. \ + If :py:obj:`None`, checks are disabled. + :param bool wants_arg: if :py:obj:`False`, will barf on non-empty \ + argument; also will not pass argument at all to the method. If \ + :py:obj:`True`, will barf on empty argument. If :py:obj:`None`, \ + checks are disabled. + :param bool wants_payload: if :py:obj:`False`, will barf on non-empty \ + payload; also will not pass payload at all to the method. If \ + :py:obj:`True`, will barf on empty payload. If :py:obj:`None`, \ + checks are disabled. :param iterable endpoints: if specified, method serve multiple API calls generated by replacing `{endpoint}` with each value in this iterable + :param bool scope: if `global` or `local`, will include the service to the \ + global section of included policy directories. + :param bool read: if :py:obj:`True` will add the service to the included \ + policy directories with read capabilities. + :param bool write: if :py:obj:`True` will add the service to the included \ + policy directories with write capabilities. + :param bool read: if :py:obj:`True` will add the service to the included \ + policy directories with execute capabilities. The expected function method should have one argument (other than usual *self*), ``untrusted_payload``, which will contain the payload. @@ -55,22 +84,27 @@ def method(name, *, no_payload=False, endpoints=None, **classifiers): .. warning:: This argument has to be named such, to remind the programmer that the content of this variable is indeed untrusted. - - If *no_payload* is true, then the method is called with no arguments. """ def decorator(func): - if no_payload: - # the following assignment is needed for how closures work in Python - _func = func - - @functools.wraps(_func) - def wrapper(self, untrusted_payload, **kwargs): - if untrusted_payload != b"": - raise ProtocolError("unexpected payload") - return _func(self, **kwargs) - - func = wrapper + if wants_arg is not None and not isinstance(wants_arg, bool): + raise ValueError("Invalid value for 'wants_arg'") + if wants_payload is not None and not isinstance(wants_payload, bool): + raise ValueError("Invalid value for 'wants_payload'") + if dest_adminvm is not None and not isinstance(dest_adminvm, bool): + raise ValueError("Invalid value for 'dest_adminvm'") + if not isinstance(read, bool): + raise ValueError("Invalid value for 'read'") + if not isinstance(write, bool): + raise ValueError("Invalid value for 'write'") + if not isinstance(execute, bool): + raise ValueError("Invalid value for 'execute'") + if scope not in [None, "local", "global"]: + raise ValueError("Invalid value for 'scope'") + + func.api_wants_arg = wants_arg + func.api_wants_payload = wants_payload + func.api_dest_adminvm = dest_adminvm # pylint: disable=protected-access if endpoints is None: @@ -81,8 +115,12 @@ def wrapper(self, untrusted_payload, **kwargs): for endpoint in endpoints ) - func.classifiers = classifiers - + func.classifiers = { + "read": read, + "write": write, + "execute": execute, + "scope": scope, + } return func return decorator @@ -189,6 +227,9 @@ def __init__( self._handler = candidates[0] self._running_handler = None + # pylint: disable=invalid-name + self.EXC_ARG_NOT_IN_DEST_VOLUMES = "{} volumes".format(self.dest.name) + @classmethod def list_methods(cls, select_method=None): for attr in dir(cls): @@ -215,9 +256,30 @@ def execute(self, *, untrusted_payload): kwargs = {} if endpoint is not None: kwargs["endpoint"] = endpoint - self._running_handler = asyncio.ensure_future( - handler(self, untrusted_payload=untrusted_payload, **kwargs) - ) + + if handler.api_wants_payload is True: + if untrusted_payload == b"": + raise ProtocolError("expected payload") + kwargs["untrusted_payload"] = untrusted_payload + if handler.api_wants_payload is False and untrusted_payload != b"": + raise ProtocolError("unexpected payload") + if handler.api_wants_payload is None: + kwargs["untrusted_payload"] = untrusted_payload + del untrusted_payload + + if handler.api_wants_arg is True and not self.arg: + raise ProtocolError("expected an argument") + if handler.api_wants_arg is False: + if self.arg: + raise ProtocolError("unexpected argument") + del self.arg + + if handler.api_dest_adminvm is True and self.dest.klass != "AdminVM": + raise ProtocolError("expected destination to be AdminVM") + if handler.api_dest_adminvm is False and self.dest.klass == "AdminVM": + raise ProtocolError("expected destination to not be AdminVM") + + self._running_handler = asyncio.ensure_future(handler(self, **kwargs)) return self._running_handler def cancel(self): @@ -231,35 +293,61 @@ def fire_event_for_permission(self, **kwargs): "admin-permission:" + self.method, pre_event=True, dest=self.dest, - arg=self.arg, - **kwargs + arg=getattr(self, "arg", ""), + **kwargs, ) def fire_event_for_filter(self, iterable, **kwargs): """Fire an event on the source qube to filter for permission""" return apply_filters(iterable, self.fire_event_for_permission(**kwargs)) + def enforce_arg( + self, + wants: Union[list, tuple], + short_reason: str = "", + ) -> None: + """Enforce argument to be absent or be in iterable.""" + assert isinstance(self.arg, str) + assert short_reason + if self.arg not in wants: + raise PermissionDenied("Argument not in {!r}".format(short_reason)) + @staticmethod - def enforce(predicate): - """An assert replacement, but works even with optimisations.""" + def enforce(predicate, reason: str = "") -> None: + """If predicate is false, raise an exception to terminate handling + the request. + + This will raise :py:class:`ProtocolError` if the predicate is false. + See the documentation of that class for details. + + :param str reason: Exception motive. + """ if not predicate: - raise PermissionDenied() + raise ProtocolError(reason) - def validate_size( - self, untrusted_size: bytes, allow_negative: bool = False + def validate_number( + self, untrusted_number: bytes, name: str, allow_negative: bool = False ) -> int: - self.enforce(isinstance(untrusted_size, bytes)) + allowed_chars = string.ascii_letters + string.digits + "-_. " + assert all(c in allowed_chars for c in name) + if not isinstance(untrusted_number, bytes): + raise ProtocolError( + "Expected {!r} to be of type bytes, got {!r}".format( + name, type(untrusted_number).__name__ + ) + ) coefficient = 1 - if allow_negative and untrusted_size.startswith(b"-"): + if allow_negative and untrusted_number.startswith(b"-"): coefficient = -1 - untrusted_size = untrusted_size[1:] - if not untrusted_size.isdigit(): - raise qubes.exc.ProtocolError("Size must be ASCII digits (only)") - if len(untrusted_size) >= 20: - raise qubes.exc.ProtocolError("Sizes limited to 19 decimal digits") - if untrusted_size[0] == 48 and untrusted_size != b"0": - raise qubes.exc.ProtocolError("Spurious leading zeros not allowed") - return int(untrusted_size) * coefficient + untrusted_number = untrusted_number[1:] + name_cap = name.capitalize() + if not untrusted_number.isdigit(): + raise ProtocolError(name_cap + " contains non ASCII digits") + if len(untrusted_number) >= 20: + raise ProtocolError(name_cap + " is bigger than 19 decimal digits") + if untrusted_number[0] == 48 and untrusted_number != b"0": + raise ProtocolError(name_cap + " contains spurious leading zeros") + return int(untrusted_number) * coefficient class QubesDaemonProtocol(asyncio.Protocol): @@ -332,6 +420,9 @@ def eof_received(self): return True async def respond(self, src, meth, dest, arg, *, untrusted_payload): + exc_fmt = ( + "failed call %r+%r (%r → %r) with payload of %d bytes due to %r" + ) try: self.mgmt = self.handler( self.app, src, meth, dest, arg, self.send_event @@ -343,62 +434,46 @@ async def respond(self, src, meth, dest, arg, *, untrusted_payload): if self.transport is None: return - # except clauses will fall through to transport.abort() below - - except PermissionDenied: + except (PermissionDenied, ProtocolError) as untrusted_exc: + exc_name = untrusted_exc.__class__.__name__ + del untrusted_exc self.app.log.warning( - "permission denied for call %s+%s (%s → %s) " - "with payload of %d bytes", + exc_fmt, meth, arg, src, dest, len(untrusted_payload), + exc_name, ) - except ProtocolError: - self.app.log.warning( - "protocol error for call %s+%s (%s → %s) " - "with payload of %d bytes", - meth, - arg, - src, - dest, - len(untrusted_payload), - ) - - except qubes.exc.QubesException as err: - msg = ( - "%r while calling " - "src=%r meth=%r dest=%r arg=%r len(untrusted_payload)=%d" - ) - + except qubes.exc.QubesException as exc: if self.debug: self.app.log.debug( - msg, - err, - src, + exc_fmt, meth, - dest, arg, + src, + dest, len(untrusted_payload), + exc, exc_info=1, ) if self.transport is not None: - self.send_exception(err) + self.send_exception(exc) self.transport.write_eof() self.transport.close() return except Exception: # pylint: disable=broad-except self.app.log.exception( - "unhandled exception while calling " - "src=%r meth=%r dest=%r arg=%r len(untrusted_payload)=%d", - src, + exc_fmt, meth, - dest, arg, + src, + dest, len(untrusted_payload), + "unhandled exception", ) else: diff --git a/qubes/api/admin.py b/qubes/api/admin.py index 2b86b4cee..0c001bc58 100644 --- a/qubes/api/admin.py +++ b/qubes/api/admin.py @@ -112,13 +112,15 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI): SOCKNAME = "/var/run/qubesd.sock" @qubes.api.method( - "admin.vmclass.List", no_payload=True, scope="global", read=True + "admin.vmclass.List", + wants_payload=False, + wants_arg=False, + dest_adminvm=True, + scope="global", + read=True, ) async def vmclass_list(self): """List all VM classes""" - self.enforce(not self.arg) - self.enforce(self.dest.name == "dom0") - entrypoints = self.fire_event_for_filter( importlib.metadata.entry_points(group=qubes.vm.VM_ENTRY_POINT) ) @@ -126,12 +128,15 @@ async def vmclass_list(self): return "".join("{}\n".format(ep.name) for ep in entrypoints) @qubes.api.method( - "admin.vm.List", no_payload=True, scope="global", read=True + "admin.vm.List", + wants_payload=False, + wants_arg=False, + dest_adminvm=None, + scope="global", + read=True, ) async def vm_list(self): """List all the domains""" - self.enforce(not self.arg) - if self.dest.name == "dom0": domains = self.fire_event_for_filter(self.app.domains) else: @@ -145,40 +150,56 @@ async def vm_list(self): ) @qubes.api.method( - "admin.vm.property.List", no_payload=True, scope="local", read=True + "admin.vm.property.List", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="local", + read=True, ) async def vm_property_list(self): """List all properties on a qube""" return self._property_list(self.dest) @qubes.api.method( - "admin.property.List", no_payload=True, scope="global", read=True + "admin.property.List", + wants_arg=False, + wants_payload=False, + dest_adminvm=True, + scope="global", + read=True, ) async def property_list(self): """List all global properties""" - self.enforce(self.dest.name == "dom0") return self._property_list(self.app) def _property_list(self, dest): - self.enforce(not self.arg) - properties = self.fire_event_for_filter(dest.property_list()) return "".join("{}\n".format(prop.__name__) for prop in properties) @qubes.api.method( - "admin.vm.property.Get", no_payload=True, scope="local", read=True + "admin.vm.property.Get", + wants_arg=True, + wants_payload=False, + dest_adminvm=None, + scope="local", + read=True, ) async def vm_property_get(self): """Get a value of one property""" return self._property_get(self.dest) @qubes.api.method( - "admin.property.Get", no_payload=True, scope="global", read=True + "admin.property.Get", + wants_arg=True, + wants_payload=False, + dest_adminvm=True, + scope="global", + read=True, ) async def property_get(self): """Get a value of one global property""" - self.enforce(self.dest.name == "dom0") return self._property_get(self.app) def _property_get(self, dest): @@ -215,23 +236,30 @@ def _serialize_property(dest, prop): ) @qubes.api.method( - "admin.vm.property.GetAll", no_payload=True, scope="local", read=True + "admin.vm.property.GetAll", + wants_payload=False, + wants_arg=False, + dest_adminvm=None, + scope="local", + read=True, ) async def vm_property_get_all(self): """Get values of all VM properties""" return self._property_get_all(self.dest) @qubes.api.method( - "admin.property.GetAll", no_payload=True, scope="global", read=True + "admin.property.GetAll", + wants_arg=False, + wants_payload=False, + dest_adminvm=True, + scope="global", + read=True, ) async def property_get_all(self): """Get value all global properties""" - self.enforce(self.dest.name == "dom0") return self._property_get_all(self.app) def _property_get_all(self, dest): - self.enforce(not self.arg) - properties = dest.property_list() properties = self.fire_event_for_filter(properties) @@ -248,7 +276,9 @@ def _property_get_all(self, dest): @qubes.api.method( "admin.vm.property.GetDefault", - no_payload=True, + wants_arg=True, + wants_payload=False, + dest_adminvm=None, scope="local", read=True, ) @@ -258,12 +288,16 @@ async def vm_property_get_default(self): return self._property_get_default(self.dest) @qubes.api.method( - "admin.property.GetDefault", no_payload=True, scope="global", read=True + "admin.property.GetDefault", + wants_arg=True, + wants_payload=False, + dest_adminvm=True, + scope="global", + read=True, ) async def property_get_default(self): """Get default value of a global property (what value will be set if this property is set to default).""" - self.enforce(self.dest.name == "dom0") return self._property_get_default(self.app) def _property_get_default(self, dest): @@ -293,17 +327,30 @@ def _property_get_default(self, dest): property_type, str(value) if value is not None else "" ) - @qubes.api.method("admin.vm.property.Set", scope="local", write=True) + @qubes.api.method( + "admin.vm.property.Set", + wants_arg=True, + wants_payload=None, + dest_adminvm=None, + scope="local", + write=True, + ) async def vm_property_set(self, untrusted_payload): """Set property value""" return self._property_set( self.dest, untrusted_payload=untrusted_payload ) - @qubes.api.method("admin.property.Set", scope="global", write=True) + @qubes.api.method( + "admin.property.Set", + wants_arg=True, + wants_payload=None, + dest_adminvm=True, + scope="global", + write=True, + ) async def property_set(self, untrusted_payload): """Set property value""" - self.enforce(self.dest.name == "dom0") return self._property_set(self.app, untrusted_payload=untrusted_payload) def _property_set(self, dest, untrusted_payload): @@ -319,18 +366,27 @@ def _property_set(self, dest, untrusted_payload): self.app.save() @qubes.api.method( - "admin.vm.property.Help", no_payload=True, scope="local", read=True + "admin.vm.property.Help", + wants_arg=True, + wants_payload=False, + dest_adminvm=None, + scope="local", + read=True, ) async def vm_property_help(self): """Get help for one property""" return self._property_help(self.dest) @qubes.api.method( - "admin.property.Help", no_payload=True, scope="global", read=True + "admin.property.Help", + wants_arg=True, + wants_payload=False, + dest_adminvm=True, + scope="global", + read=True, ) async def property_help(self): """Get help for one property""" - self.enforce(self.dest.name == "dom0") return self._property_help(self.app) def _property_help(self, dest): @@ -347,18 +403,27 @@ def _property_help(self, dest): return qubes.utils.format_doc(doc) @qubes.api.method( - "admin.vm.property.Reset", no_payload=True, scope="local", write=True + "admin.vm.property.Reset", + wants_arg=True, + wants_payload=False, + dest_adminvm=None, + scope="local", + write=True, ) async def vm_property_reset(self): """Reset a property to a default value""" return self._property_reset(self.dest) @qubes.api.method( - "admin.property.Reset", no_payload=True, scope="global", write=True + "admin.property.Reset", + wants_arg=True, + wants_payload=False, + dest_adminvm=True, + scope="global", + write=True, ) async def property_reset(self): """Reset a property to a default value""" - self.enforce(self.dest.name == "dom0") return self._property_reset(self.app) def _property_reset(self, dest): @@ -371,11 +436,14 @@ def _property_reset(self, dest): self.app.save() @qubes.api.method( - "admin.vm.volume.List", no_payload=True, scope="local", read=True + "admin.vm.volume.List", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="local", + read=True, ) async def vm_volume_list(self): - self.enforce(not self.arg) - volume_names = ( self.fire_event_for_filter(self.dest.volumes.keys()) if isinstance(self.dest, qubes.vm.qubesvm.QubesVM) @@ -384,10 +452,18 @@ async def vm_volume_list(self): return "".join("{}\n".format(name) for name in volume_names) @qubes.api.method( - "admin.vm.volume.Info", no_payload=True, scope="local", read=True + "admin.vm.volume.Info", + wants_arg=True, + wants_payload=False, + dest_adminvm=None, + scope="local", + read=True, ) async def vm_volume_info(self): - self.enforce(self.arg in self.dest.volumes.keys()) + self.enforce_arg( + wants=self.dest.volumes.keys(), + short_reason=self.EXC_ARG_NOT_IN_DEST_VOLUMES, + ) self.fire_event_for_permission() @@ -426,12 +502,17 @@ def _serialize(value): @qubes.api.method( "admin.vm.volume.ListSnapshots", - no_payload=True, + wants_arg=True, + wants_payload=False, + dest_adminvm=None, scope="local", read=True, ) async def vm_volume_listsnapshots(self): - self.enforce(self.arg in self.dest.volumes.keys()) + self.enforce_arg( + wants=self.dest.volumes.keys(), + short_reason=self.EXC_ARG_NOT_IN_DEST_VOLUMES, + ) volume = self.dest.volumes[self.arg] id_to_timestamp = volume.revisions @@ -440,15 +521,27 @@ async def vm_volume_listsnapshots(self): return "".join("{}\n".format(revision) for revision in revisions) - @qubes.api.method("admin.vm.volume.Revert", scope="local", write=True) + @qubes.api.method( + "admin.vm.volume.Revert", + wants_arg=True, + wants_payload=True, + dest_adminvm=None, + scope="local", + write=True, + ) async def vm_volume_revert(self, untrusted_payload): - self.enforce(self.arg in self.dest.volumes.keys()) + self.enforce_arg( + wants=self.dest.volumes.keys(), + short_reason=self.EXC_ARG_NOT_IN_DEST_VOLUMES, + ) untrusted_revision = untrusted_payload.decode("ascii").strip() del untrusted_payload volume = self.dest.volumes[self.arg] snapshots = volume.revisions - self.enforce(untrusted_revision in snapshots) + # TODO: ben: info-leak: revision existence + if untrusted_revision not in snapshots: + raise qubes.exc.QubesVolumeRevisionNotFoundError() revision = untrusted_revision self.fire_event_for_permission(volume=volume, revision=revision) @@ -458,10 +551,18 @@ async def vm_volume_revert(self, untrusted_payload): # write=True because this allow to clone VM - and most likely modify that # one - still having the same data @qubes.api.method( - "admin.vm.volume.CloneFrom", no_payload=True, scope="local", write=True + "admin.vm.volume.CloneFrom", + wants_arg=True, + wants_payload=False, + dest_adminvm=None, + scope="local", + write=True, ) async def vm_volume_clone_from(self): - self.enforce(self.arg in self.dest.volumes.keys()) + self.enforce_arg( + wants=self.dest.volumes.keys(), + short_reason=self.EXC_ARG_NOT_IN_DEST_VOLUMES, + ) volume = self.dest.volumes[self.arg] @@ -473,19 +574,38 @@ async def vm_volume_clone_from(self): self.app.api_admin_pending_clone = {} # don't handle collisions any better - if someone is so much out of # luck, can try again anyway - self.enforce(token not in self.app.api_admin_pending_clone) + if token in self.app.api_admin_pending_clone: + raise qubes.exc.QubesVolumeCopyInUseError( + vm=self.dest, volume=self.arg + ) self.app.api_admin_pending_clone[token] = volume return token - @qubes.api.method("admin.vm.volume.CloneTo", scope="local", write=True) + @qubes.api.method( + "admin.vm.volume.CloneTo", + wants_arg=True, + wants_payload=True, + dest_adminvm=None, + scope="local", + write=True, + ) async def vm_volume_clone_to(self, untrusted_payload): - self.enforce(self.arg in self.dest.volumes.keys()) - untrusted_token = untrusted_payload.decode("ascii").strip() - del untrusted_payload - self.enforce( - untrusted_token in getattr(self.app, "api_admin_pending_clone", {}) + self.enforce_arg( + wants=self.dest.volumes.keys(), + short_reason=self.EXC_ARG_NOT_IN_DEST_VOLUMES, ) + try: + untrusted_token = untrusted_payload.decode("ascii").strip() + except UnicodeDecodeError: + raise qubes.exc.ProtocolError("Token contains non-ASCII characters") + + del untrusted_payload + if untrusted_token not in getattr( + self.app, "api_admin_pending_clone", {} + ): + raise qubes.exc.QubesVolumeCopyTokenNotFoundError() + token = untrusted_token del untrusted_token @@ -493,8 +613,12 @@ async def vm_volume_clone_to(self, untrusted_payload): del self.app.api_admin_pending_clone[token] # make sure the volume still exists, but invalidate token anyway - self.enforce(str(src_volume.pool) in self.app.pools) - self.enforce(src_volume in self.app.pools[str(src_volume.pool)].volumes) + # TODO: ben: info-leak: pool existence + if str(src_volume.pool) not in self.app.pools: + raise qubes.exc.QubesPoolNotFoundError() + # TODO: ben: info-leak: volume existence + if src_volume not in self.app.pools[str(src_volume.pool)].volumes: + raise qubes.exc.QubesVolumeNotFoundError() dst_volume = self.dest.volumes[self.arg] @@ -506,10 +630,22 @@ async def vm_volume_clone_to(self, untrusted_payload): ) self.app.save() - @qubes.api.method("admin.vm.volume.Resize", scope="local", write=True) + @qubes.api.method( + "admin.vm.volume.Resize", + wants_arg=True, + wants_payload=True, + dest_adminvm=None, + scope="local", + write=True, + ) async def vm_volume_resize(self, untrusted_payload): - self.enforce(self.arg in self.dest.volumes.keys()) - size = self.validate_size(untrusted_payload.strip()) + self.enforce_arg( + wants=self.dest.volumes.keys(), + short_reason=self.EXC_ARG_NOT_IN_DEST_VOLUMES, + ) + size = self.validate_number( + untrusted_payload.strip(), name="volume size" + ) self.fire_event_for_permission(size=size) try: await self.dest.storage.resize(self.arg, size) @@ -517,10 +653,18 @@ async def vm_volume_resize(self, untrusted_payload): self.app.save() @qubes.api.method( - "admin.vm.volume.Clear", no_payload=True, scope="local", write=True + "admin.vm.volume.Clear", + wants_arg=True, + wants_payload=False, + dest_adminvm=None, + scope="local", + write=True, ) async def vm_volume_clear(self): - self.enforce(self.arg in self.dest.volumes.keys()) + self.enforce_arg( + wants=self.dest.volumes.keys(), + short_reason=self.EXC_ARG_NOT_IN_DEST_VOLUMES, + ) self.fire_event_for_permission() @@ -546,24 +690,44 @@ async def vm_volume_clear(self): self.app.save() @qubes.api.method( - "admin.vm.volume.Set.revisions_to_keep", scope="local", write=True + "admin.vm.volume.Set.revisions_to_keep", + wants_arg=True, + wants_payload=True, + dest_adminvm=None, + scope="local", + write=True, ) async def vm_volume_set_revisions_to_keep(self, untrusted_payload): - self.enforce(self.arg in self.dest.volumes.keys()) - newvalue = self.validate_size(untrusted_payload, allow_negative=True) + self.enforce_arg( + wants=self.dest.volumes.keys(), + short_reason=self.EXC_ARG_NOT_IN_DEST_VOLUMES, + ) + newvalue = self.validate_number( + untrusted_payload, name="revisions to keep", allow_negative=True + ) self.fire_event_for_permission(newvalue=newvalue) self.dest.storage.set_revisions_to_keep(self.arg, newvalue) self.app.save() - @qubes.api.method("admin.vm.volume.Set.rw", scope="local", write=True) + @qubes.api.method( + "admin.vm.volume.Set.rw", + wants_arg=True, + wants_payload=True, + dest_adminvm=None, + scope="local", + write=True, + ) async def vm_volume_set_rw(self, untrusted_payload): - self.enforce(self.arg in self.dest.volumes.keys()) + self.enforce_arg( + wants=self.dest.volumes.keys(), + short_reason=self.EXC_ARG_NOT_IN_DEST_VOLUMES, + ) try: newvalue = qubes.property.bool( None, None, untrusted_payload.decode("ascii") ) except (UnicodeDecodeError, ValueError): - raise qubes.exc.ProtocolError("Invalid value") + raise qubes.exc.ProtocolError("Payload is not boolean ASCII") del untrusted_payload self.fire_event_for_permission(newvalue=newvalue) @@ -575,16 +739,24 @@ async def vm_volume_set_rw(self, untrusted_payload): self.app.save() @qubes.api.method( - "admin.vm.volume.Set.ephemeral", scope="local", write=True + "admin.vm.volume.Set.ephemeral", + wants_arg=True, + wants_payload=True, + dest_adminvm=None, + scope="local", + write=True, ) async def vm_volume_set_ephemeral(self, untrusted_payload): - self.enforce(self.arg in self.dest.volumes.keys()) + self.enforce_arg( + wants=self.dest.volumes.keys(), + short_reason=self.EXC_ARG_NOT_IN_DEST_VOLUMES, + ) try: newvalue = qubes.property.bool( None, None, untrusted_payload.decode("ascii") ) except (UnicodeDecodeError, ValueError): - raise qubes.exc.ProtocolError("Invalid value") + raise qubes.exc.ProtocolError("Payload is not boolean ASCII") del untrusted_payload self.fire_event_for_permission(newvalue=newvalue) @@ -596,11 +768,14 @@ async def vm_volume_set_ephemeral(self, untrusted_payload): self.app.save() @qubes.api.method( - "admin.vm.tag.List", no_payload=True, scope="local", read=True + "admin.vm.tag.List", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="local", + read=True, ) async def vm_tag_list(self): - self.enforce(not self.arg) - tags = self.dest.tags tags = self.fire_event_for_filter(tags) @@ -608,7 +783,12 @@ async def vm_tag_list(self): return "".join("{}\n".format(tag) for tag in sorted(tags)) @qubes.api.method( - "admin.vm.tag.Get", no_payload=True, scope="local", read=True + "admin.vm.tag.Get", + wants_arg=True, + wants_payload=False, + dest_adminvm=None, + scope="local", + read=True, ) async def vm_tag_get(self): qubes.vm.Tags.validate_tag(self.arg) @@ -618,7 +798,12 @@ async def vm_tag_get(self): return "1" if self.arg in self.dest.tags else "0" @qubes.api.method( - "admin.vm.tag.Set", no_payload=True, scope="local", write=True + "admin.vm.tag.Set", + wants_arg=True, + wants_payload=False, + dest_adminvm=None, + scope="local", + write=True, ) async def vm_tag_set(self): qubes.vm.Tags.validate_tag(self.arg) @@ -629,7 +814,12 @@ async def vm_tag_set(self): self.app.save() @qubes.api.method( - "admin.vm.tag.Remove", no_payload=True, scope="local", write=True + "admin.vm.tag.Remove", + wants_arg=True, + wants_payload=False, + dest_adminvm=None, + scope="local", + write=True, ) async def vm_tag_remove(self): qubes.vm.Tags.validate_tag(self.arg) @@ -643,11 +833,14 @@ async def vm_tag_remove(self): self.app.save() @qubes.api.method( - "admin.vm.Console", no_payload=True, scope="local", write=True + "admin.vm.Console", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="local", + write=True, ) async def vm_console(self): - self.enforce(not self.arg) - self.fire_event_for_permission() if not self.dest.is_running(): @@ -660,23 +853,27 @@ async def vm_console(self): return ttypath @qubes.api.method( - "admin.pool.List", no_payload=True, scope="global", read=True + "admin.pool.List", + wants_arg=False, + wants_payload=False, + dest_adminvm=True, + scope="global", + read=True, ) async def pool_list(self): - self.enforce(not self.arg) - self.enforce(self.dest.name == "dom0") - pools = self.fire_event_for_filter(self.app.pools) return "".join("{}\n".format(pool) for pool in pools) @qubes.api.method( - "admin.pool.ListDrivers", no_payload=True, scope="global", read=True + "admin.pool.ListDrivers", + wants_arg=False, + wants_payload=False, + dest_adminvm=True, + scope="global", + read=True, ) async def pool_listdrivers(self): - self.enforce(self.dest.name == "dom0") - self.enforce(not self.arg) - drivers = self.fire_event_for_filter(qubes.storage.pool_drivers()) return "".join( @@ -687,11 +884,15 @@ async def pool_listdrivers(self): ) @qubes.api.method( - "admin.pool.Info", no_payload=True, scope="global", read=True + "admin.pool.Info", + wants_arg=True, + wants_payload=False, + dest_adminvm=True, + scope="global", + read=True, ) async def pool_info(self): - self.enforce(self.dest.name == "dom0") - self.enforce(self.arg in self.app.pools.keys()) + self.enforce_arg(wants=self.app.pools.keys(), short_reason="pools") pool = self.app.pools[self.arg] @@ -723,11 +924,15 @@ async def pool_info(self): ) @qubes.api.method( - "admin.pool.UsageDetails", no_payload=True, scope="global", read=True + "admin.pool.UsageDetails", + wants_arg=True, + wants_payload=False, + dest_adminvm=True, + scope="global", + read=True, ) async def pool_usage(self): - self.enforce(self.dest.name == "dom0") - self.enforce(self.arg in self.app.pools.keys()) + self.enforce_arg(wants=self.app.pools.keys(), short_reason="pools") pool = self.app.pools[self.arg] @@ -742,17 +947,33 @@ async def pool_usage(self): return usage - @qubes.api.method("admin.pool.Add", scope="global", write=True) + @qubes.api.method( + "admin.pool.Add", + wants_arg=True, + wants_payload=True, + dest_adminvm=True, + scope="global", + write=True, + ) async def pool_add(self, untrusted_payload): - self.enforce(self.dest.name == "dom0") - if self.arg not in qubes.storage.pool_drivers(): - raise qubes.exc.QubesException( - "unexpected driver name: " + self.arg - ) + # TODO: ben: info-leak: pool existence + self.enforce_arg( + wants=qubes.storage.pool_drivers(), short_reason="available drivers" + ) - untrusted_pool_config = untrusted_payload.decode("ascii").splitlines() + try: + untrusted_pool_config = untrusted_payload.decode( + "ascii" + ).splitlines() + except UnicodeDecodeError: + raise qubes.exc.ProtocolError( + "Pool parameters contains non-ASCII characters" + ) del untrusted_payload - self.enforce(all(("=" in line) for line in untrusted_pool_config)) + self.enforce( + all(("=" in line) for line in untrusted_pool_config), + reason="Options must be separated by equal sign", + ) # pairs of (option, value) untrusted_pool_config = [ line.split("=", 1) for line in untrusted_pool_config @@ -760,26 +981,36 @@ async def pool_add(self, untrusted_payload): # reject duplicated options self.enforce( len(set(x[0] for x in untrusted_pool_config)) - == len([x[0] for x in untrusted_pool_config]) + == len([x[0] for x in untrusted_pool_config]), + reason="Options must be unique", ) # and convert to dict untrusted_pool_config = dict(untrusted_pool_config) - self.enforce("name" in untrusted_pool_config) + self.enforce( + "name" in untrusted_pool_config, + reason="Pool configuration must contain key: name", + ) untrusted_pool_name = untrusted_pool_config.pop("name") allowed_chars = string.ascii_letters + string.digits + "-_." - self.enforce(all(c in allowed_chars for c in untrusted_pool_name)) + self.enforce( + all(c in allowed_chars for c in untrusted_pool_name), + reason="Pool name must be in safe set: " + allowed_chars, + ) pool_name = untrusted_pool_name - self.enforce(pool_name not in self.app.pools) + # TODO: ben: info-leak: pool existence + if pool_name in self.app.pools: + raise qubes.exc.QubesPoolInUseError(pool_name=pool_name) driver_parameters = qubes.storage.driver_parameters(self.arg) dp_names = driver_parameters.keys() unexpected_parameters = [ key for key in untrusted_pool_config if key not in driver_parameters ] + # TODO: ben: dangerous logging any ASCII value? if unexpected_parameters: - raise qubes.exc.QubesException( - "unexpected driver options: " + " ".join(unexpected_parameters) + raise qubes.exc.ProtocolError( + "Unexpected driver options: " + " ".join(unexpected_parameters) ) required_parameters_unmet = [ key @@ -800,11 +1031,15 @@ async def pool_add(self, untrusted_payload): self.app.save() @qubes.api.method( - "admin.pool.Remove", no_payload=True, scope="global", write=True + "admin.pool.Remove", + wants_arg=True, + wants_payload=False, + dest_adminvm=True, + scope="global", + write=True, ) async def pool_remove(self): - self.enforce(self.dest.name == "dom0") - self.enforce(self.arg in self.app.pools.keys()) + self.enforce_arg(wants=self.app.pools.keys(), short_reason="pools") self.fire_event_for_permission() @@ -812,11 +1047,15 @@ async def pool_remove(self): self.app.save() @qubes.api.method( - "admin.pool.volume.List", no_payload=True, scope="global", read=True + "admin.pool.volume.List", + wants_arg=True, + wants_payload=False, + dest_adminvm=True, + scope="global", + read=True, ) async def pool_volume_list(self): - self.enforce(self.dest.name == "dom0") - self.enforce(self.arg in self.app.pools.keys()) + self.enforce_arg(wants=self.app.pools.keys(), short_reason="pools") pool = self.app.pools[self.arg] @@ -824,16 +1063,21 @@ async def pool_volume_list(self): return "".join("{}\n".format(name) for name in volume_names) @qubes.api.method( - "admin.pool.Set.revisions_to_keep", scope="global", write=True + "admin.pool.Set.revisions_to_keep", + wants_arg=True, + wants_payload=True, + dest_adminvm=True, + scope="global", + write=True, ) async def pool_set_revisions_to_keep(self, untrusted_payload): - self.enforce(self.dest.name == "dom0") - self.enforce(self.arg in self.app.pools.keys()) + self.enforce_arg(wants=self.app.pools.keys(), short_reason="pools") pool = self.app.pools[self.arg] - newvalue = self.validate_size(untrusted_payload, allow_negative=True) + newvalue = self.validate_number( + untrusted_payload, name="revisions to keep", allow_negative=True + ) - if newvalue < -1: - raise qubes.api.ProtocolError("Invalid value for revisions_to_keep") + self.enforce(newvalue >= -1, reason="Value can't be lower than -1") self.fire_event_for_permission(newvalue=newvalue) @@ -841,18 +1085,22 @@ async def pool_set_revisions_to_keep(self, untrusted_payload): self.app.save() @qubes.api.method( - "admin.pool.Set.ephemeral_volatile", scope="global", write=True + "admin.pool.Set.ephemeral_volatile", + wants_arg=True, + wants_payload=True, + dest_adminvm=True, + scope="global", + write=True, ) async def pool_set_ephemeral(self, untrusted_payload): - self.enforce(self.dest.name == "dom0") - self.enforce(self.arg in self.app.pools.keys()) + self.enforce_arg(wants=self.app.pools.keys(), short_reason="pools") pool = self.app.pools[self.arg] try: newvalue = qubes.property.bool( None, None, untrusted_payload.decode("ascii", "strict") ) except (UnicodeDecodeError, ValueError): - raise qubes.exc.ProtocolError("Invalid value") + raise qubes.exc.ProtocolError("Payload is not boolean ASCII") del untrusted_payload self.fire_event_for_permission(newvalue=newvalue) @@ -861,70 +1109,81 @@ async def pool_set_ephemeral(self, untrusted_payload): self.app.save() @qubes.api.method( - "admin.label.List", no_payload=True, scope="global", read=True + "admin.label.List", + wants_arg=False, + wants_payload=False, + dest_adminvm=True, + scope="global", + read=True, ) async def label_list(self): - self.enforce(self.dest.name == "dom0") - self.enforce(not self.arg) - labels = self.fire_event_for_filter(self.app.labels.values()) return "".join("{}\n".format(label.name) for label in labels) @qubes.api.method( - "admin.label.Get", no_payload=True, scope="global", read=True + "admin.label.Get", + wants_arg=True, + wants_payload=False, + dest_adminvm=True, + scope="global", + read=True, ) async def label_get(self): - self.enforce(self.dest.name == "dom0") + qubes.utils.validate_label_name(untrusted_label=self.arg) - try: - label = self.app.get_label(self.arg) - except KeyError: - raise qubes.exc.QubesValueError + # TODO: ben: info-leak: label existence + label = self.app.get_label(self.arg) self.fire_event_for_permission(label=label) return label.color @qubes.api.method( - "admin.label.Index", no_payload=True, scope="global", read=True + "admin.label.Index", + wants_arg=True, + wants_payload=False, + dest_adminvm=True, + scope="global", + read=True, ) async def label_index(self): - self.enforce(self.dest.name == "dom0") + qubes.utils.validate_label_name(untrusted_label=self.arg) - try: - label = self.app.get_label(self.arg) - except KeyError: - raise qubes.exc.QubesValueError + # TODO: ben: info-leak: label existence + label = self.app.get_label(self.arg) self.fire_event_for_permission(label=label) return str(label.index) - @qubes.api.method("admin.label.Create", scope="global", write=True) + @qubes.api.method( + "admin.label.Create", + wants_arg=True, + wants_payload=True, + dest_adminvm=True, + scope="global", + write=True, + ) async def label_create(self, untrusted_payload): - self.enforce(self.dest.name == "dom0") + qubes.utils.validate_label_name(untrusted_label=self.arg, creation=True) - # don't confuse label name with label index - self.enforce(not self.arg.isdigit()) - allowed_chars = string.ascii_letters + string.digits + "-_." - self.enforce(all(c in allowed_chars for c in self.arg)) + # TODO: ben: info-leak: label existence try: self.app.get_label(self.arg) except KeyError: # ok, no such label yet pass else: - raise qubes.exc.QubesValueError("label already exists") + raise qubes.exc.QubesLabelInUseError(label=self.arg) - untrusted_payload = untrusted_payload.decode("ascii", "strict").strip() - self.enforce(len(untrusted_payload) == 8) - self.enforce(untrusted_payload.startswith("0x")) - # besides prefix, only hex digits are allowed - self.enforce(all(x in string.hexdigits for x in untrusted_payload[2:])) + qubes.utils.validate_label_value( + untrusted_label_value=untrusted_payload + ) - # SEE: #2732 + # TODO: avoid creating too-similar qube labels: #2732 color = untrusted_payload + del untrusted_payload self.fire_event_for_permission(color=color) @@ -938,33 +1197,42 @@ async def label_create(self, untrusted_payload): self.app.save() @qubes.api.method( - "admin.label.Remove", no_payload=True, scope="global", write=True + "admin.label.Remove", + wants_arg=True, + wants_payload=False, + dest_adminvm=True, + scope="global", + write=True, ) async def label_remove(self): - self.enforce(self.dest.name == "dom0") + qubes.utils.validate_label_name(untrusted_label=self.arg) - try: - label = self.app.get_label(self.arg) - except KeyError: - raise qubes.exc.QubesValueError - # don't allow removing default labels - self.enforce(label.index > qubes.config.max_default_label) + # TODO: ben: info-leak: label existence + label = self.app.get_label(self.arg) + self.enforce( + label.index > qubes.config.max_default_label, + reason="Can only remove custom labels", + ) + + self.fire_event_for_permission(label=label) # FIXME: this should be in app.add_label() for vm in self.app.domains: if vm.label == label: - raise qubes.exc.QubesException("label still in use") - - self.fire_event_for_permission(label=label) + raise qubes.exc.QubesLabelInUseError(label=label) del self.app.labels[label.index] self.app.save() @qubes.api.method( - "admin.vm.Start", no_payload=True, scope="local", execute=True + "admin.vm.Start", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="local", + execute=True, ) async def vm_start(self): - self.enforce(not self.arg) self.fire_event_for_permission() try: await self.dest.start() @@ -977,65 +1245,97 @@ async def vm_start(self): ) @qubes.api.method( - "admin.vm.Shutdown", no_payload=True, scope="local", execute=True + "admin.vm.Shutdown", + wants_arg=None, + wants_payload=False, + dest_adminvm=None, + scope="local", + execute=True, ) async def vm_shutdown(self): if self.arg: args = self.arg.split("+") else: args = [] - self.enforce(all(arg in ("force", "wait") for arg in args)) + allowed_args = ("force", "wait") + self.enforce( + all(arg in allowed_args for arg in args), + reason="Argument must match: " + ", ".join(allowed_args), + ) force = "force" in args wait = "wait" in args self.fire_event_for_permission(force=force, wait=wait) await self.dest.shutdown(force=force, wait=wait) @qubes.api.method( - "admin.vm.Pause", no_payload=True, scope="local", execute=True + "admin.vm.Pause", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="local", + execute=True, ) async def vm_pause(self): - self.enforce(not self.arg) self.fire_event_for_permission() await self.dest.pause() @qubes.api.method( - "admin.vm.Unpause", no_payload=True, scope="local", execute=True + "admin.vm.Unpause", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="local", + execute=True, ) async def vm_unpause(self): - self.enforce(not self.arg) self.fire_event_for_permission() await self.dest.unpause() @qubes.api.method( - "admin.vm.Suspend", no_payload=True, scope="local", execute=True + "admin.vm.Suspend", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="local", + execute=True, ) async def vm_suspend(self): - self.enforce(not self.arg) self.fire_event_for_permission() await self.dest.suspend() @qubes.api.method( - "admin.vm.Resume", no_payload=True, scope="local", execute=True + "admin.vm.Resume", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="local", + execute=True, ) async def vm_resume(self): - self.enforce(not self.arg) self.fire_event_for_permission() await self.dest.resume() @qubes.api.method( - "admin.vm.Kill", no_payload=True, scope="local", execute=True + "admin.vm.Kill", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="local", + execute=True, ) async def vm_kill(self): - self.enforce(not self.arg) self.fire_event_for_permission() await self.dest.kill() @qubes.api.method( - "admin.Events", no_payload=True, scope="global", read=True + "admin.Events", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="global", + read=True, ) async def events(self): - self.enforce(not self.arg) - # run until client connection is terminated self.cancellable = True wait_for_cancel = asyncio.get_event_loop().create_future() @@ -1074,15 +1374,24 @@ async def events(self): self.dest.remove_handler("*", dispatcher.vm_handler) @qubes.api.method( - "admin.vm.feature.List", no_payload=True, scope="local", read=True + "admin.vm.feature.List", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="local", + read=True, ) async def vm_feature_list(self): - self.enforce(not self.arg) features = self.fire_event_for_filter(self.dest.features.keys()) return "".join("{}\n".format(feature) for feature in features) @qubes.api.method( - "admin.vm.feature.Get", no_payload=True, scope="local", read=True + "admin.vm.feature.Get", + wants_arg=True, + wants_payload=False, + dest_adminvm=None, + scope="local", + read=True, ) async def vm_feature_get(self): # validation of self.arg done by qrexec-policy is enough @@ -1096,7 +1405,9 @@ async def vm_feature_get(self): @qubes.api.method( "admin.vm.feature.CheckWithTemplate", - no_payload=True, + wants_arg=True, + wants_payload=False, + dest_adminvm=None, scope="local", read=True, ) @@ -1112,7 +1423,9 @@ async def vm_feature_checkwithtemplate(self): @qubes.api.method( "admin.vm.feature.CheckWithNetvm", - no_payload=True, + wants_arg=True, + wants_payload=False, + dest_adminvm=None, scope="local", read=True, ) @@ -1128,7 +1441,9 @@ async def vm_feature_checkwithnetvm(self): @qubes.api.method( "admin.vm.feature.CheckWithAdminVM", - no_payload=True, + wants_arg=True, + wants_payload=False, + dest_adminvm=None, scope="local", read=True, ) @@ -1144,7 +1459,9 @@ async def vm_feature_checkwithadminvm(self): @qubes.api.method( "admin.vm.feature.CheckWithTemplateAndAdminVM", - no_payload=True, + wants_arg=True, + wants_payload=False, + dest_adminvm=None, scope="local", read=True, ) @@ -1159,7 +1476,12 @@ async def vm_feature_checkwithtpladminvm(self): return value @qubes.api.method( - "admin.vm.feature.Remove", no_payload=True, scope="local", write=True + "admin.vm.feature.Remove", + wants_arg=True, + wants_payload=False, + dest_adminvm=None, + scope="local", + write=True, ) async def vm_feature_remove(self): # validation of self.arg done by qrexec-policy is enough @@ -1171,16 +1493,23 @@ async def vm_feature_remove(self): raise qubes.exc.QubesFeatureNotFoundError(self.dest, self.arg) self.app.save() - @qubes.api.method("admin.vm.feature.Set", scope="local", write=True) + @qubes.api.method( + "admin.vm.feature.Set", + wants_arg=True, + wants_payload=None, + dest_adminvm=None, + scope="local", + write=True, + ) 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: - raise qubes.exc.QubesValueError( + raise qubes.exc.ProtocolError( f"{self.arg} value contains illegal characters" ) value = untrusted_value @@ -1198,6 +1527,9 @@ async def vm_feature_set(self, untrusted_payload): group=qubes.vm.VM_ENTRY_POINT ) ), + wants_arg=None, + wants_payload=None, + dest_adminvm=True, scope="global", write=True, ) @@ -1214,6 +1546,9 @@ def vm_create(self, endpoint, untrusted_payload=None): group=qubes.vm.VM_ENTRY_POINT ) ), + wants_arg=None, + wants_payload=None, + dest_adminvm=True, scope="global", write=True, ) @@ -1225,8 +1560,6 @@ def vm_create_in_pool(self, endpoint, untrusted_payload=None): async def _vm_create( self, vm_type, allow_pool=False, untrusted_payload=None ): - self.enforce(self.dest.name == "dom0") - kwargs = {} pool = None pools = {} @@ -1240,11 +1573,12 @@ async def _vm_create( # when given VM class do need a template if self.arg: if hasattr(vm_class, "template"): + # TODO: ben: info-leak: template existence if self.arg not in self.app.domains: raise qubes.exc.QubesVMNotFoundError(self.arg) kwargs["template"] = self.app.domains[self.arg] else: - raise qubes.exc.QubesValueError( + raise qubes.exc.ProtocolError( "{} cannot be based on template".format(vm_type) ) @@ -1252,55 +1586,72 @@ async def _vm_create( "ascii", errors="strict" ).split(" "): untrusted_key, untrusted_value = untrusted_param.split("=", 1) - if untrusted_key in kwargs: - raise qubes.exc.ProtocolError("duplicated parameters") + self.enforce( + untrusted_key not in kwargs, reason="Options must be unique" + ) if untrusted_key == "name": qubes.vm.validate_name(None, None, untrusted_value) kwargs["name"] = untrusted_value elif untrusted_key == "label": - # don't confuse label name with label index - self.enforce(not untrusted_value.isdigit()) - allowed_chars = string.ascii_letters + string.digits + "-_." - self.enforce(all(c in allowed_chars for c in untrusted_value)) - # this may raise QubesLabelNotFoundError + qubes.utils.validate_label_name( + untrusted_label=untrusted_value, creation=True + ) + # TODO: ben: info-leak: label existence kwargs["label"] = self.app.get_label(untrusted_value) elif untrusted_key == "pool" and allow_pool: - if pool is not None: - raise qubes.exc.ProtocolError("duplicated pool parameter") + self.enforce( + pool is None, reason="Option 'pool' must be unique" + ) pool = self.app.get_pool(untrusted_value) + elif untrusted_key.startswith("pool:") and allow_pool: untrusted_volume = untrusted_key.split(":", 1)[1] # kind of ugly, but actual list of volumes is available only # after creating a VM + allowed_volumes = ["root", "private", "volatile", "kernel"] self.enforce( - untrusted_volume - in ["root", "private", "volatile", "kernel"] + untrusted_volume in allowed_volumes, + reason="Volume must be one of: " + + ", ".join(allowed_volumes), ) volume = untrusted_volume - if volume in pools: - raise qubes.exc.ProtocolError( - "duplicated pool:{} parameter".format(volume) - ) + self.enforce( + volume not in pools, + reason="Option 'pool:{}' must be unique".format(volume), + ) pools[volume] = self.app.get_pool(untrusted_value) else: - raise qubes.exc.ProtocolError("Invalid param name") + if not allow_pool and ( + untrusted_key == "pool" or untrusted_key.startswith("pool:") + ): + raise qubes.exc.ProtocolError( + "Pool option specified but 'allow_pool' is False" + ) + allowed_options = ["name", "label", "pool", "pool:"] + raise qubes.exc.ProtocolError( + "Options does not match: " + ", ".join(allowed_options) + ) del untrusted_payload - if "name" not in kwargs or "label" not in kwargs: - raise qubes.exc.ProtocolError("Missing name or label") - - if pool and pools: - raise qubes.exc.ProtocolError( - "Only one of 'pool=' and 'pool:volume=' can be used" + for required_key in ["name", "label"]: + self.enforce( + required_key in kwargs, + reason="Creation requested without key: " + required_key, ) + self.enforce( + not (pool and pools), + reason="Conflicting options specified: 'pool=', 'pool:volume='", + ) + + # TODO: ben: info-leak: qube existence if kwargs["name"] in self.app.domains: - raise qubes.exc.QubesValueError( - "VM {} already exists".format(kwargs["name"]) + raise qubes.exc.QubesVMAlreadyExistsError( + "Qube already exists: {}".format(kwargs["name"]) ) self.fire_event_for_permission(pool=pool, pools=pools, **kwargs) @@ -1317,20 +1668,29 @@ async def _vm_create( raise self.app.save() - @qubes.api.method("admin.vm.CreateDisposable", scope="global", write=True) + @qubes.api.method( + "admin.vm.CreateDisposable", + wants_arg=None, + wants_payload=None, + dest_adminvm=None, + scope="global", + write=True, + ) async def create_disposable(self, untrusted_payload): """ Create a disposable. If the RPC argument is ``preload``, cleanse the preload list and start preloading fresh disposables. """ - self.enforce(self.arg in [None, "", "preload"]) + self.enforce_arg( + [None, "", "preload"], + short_reason="'', 'preload'", + ) preload = False if self.arg == "preload": preload = True if untrusted_payload not in (b"", b"uuid"): - raise qubes.exc.QubesValueError( - "Invalid payload for admin.vm.CreateDisposable: " - "expected the empty string or 'uuid'" + raise qubes.exc.ProtocolError( + "Invalid payload, expected empty string or 'uuid'" ) if self.dest.name == "dom0": @@ -1353,11 +1713,14 @@ async def create_disposable(self, untrusted_payload): ) @qubes.api.method( - "admin.vm.Remove", no_payload=True, scope="global", write=True + "admin.vm.Remove", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="global", + write=True, ) async def vm_remove(self): - self.enforce(not self.arg) - self.fire_event_for_permission() if self.dest.name == "dom0": @@ -1386,13 +1749,15 @@ async def vm_remove(self): self.app.save() @qubes.api.method( - "admin.deviceclass.List", no_payload=True, scope="global", read=True + "admin.deviceclass.List", + wants_arg=False, + wants_payload=False, + dest_adminvm=True, + scope="global", + read=True, ) async def deviceclass_list(self): """List all DEVICES classes""" - self.enforce(not self.arg) - self.enforce(self.dest.name == "dom0") - entrypoints = self.fire_event_for_filter( importlib.metadata.entry_points(group="qubes.devices") ) @@ -1405,7 +1770,9 @@ async def deviceclass_list(self): ep.name for ep in importlib.metadata.entry_points(group="qubes.devices") ), - no_payload=True, + wants_arg=None, + wants_payload=False, + dest_adminvm=None, scope="local", read=True, ) @@ -1422,7 +1789,8 @@ async def vm_device_available(self, endpoint): devices = [dev for dev in devices if dev.port_id == self.arg] # no duplicated devices, but device may not exist, in which case # the list is empty - self.enforce(len(devices) <= 1) + if len(devices) > 1: + raise qubes.exc.DeviceNotFound() devices = self.fire_event_for_filter(devices, devclass=devclass) dev_info = { f"{dev.port_id}:{dev.device_id}": dev.serialize().decode() @@ -1438,7 +1806,9 @@ async def vm_device_available(self, endpoint): ep.name for ep in importlib.metadata.entry_points(group="qubes.devices") ), - no_payload=True, + wants_arg=None, + wants_payload=False, + dest_adminvm=None, scope="local", read=True, ) @@ -1463,7 +1833,8 @@ async def vm_device_list(self, endpoint): ] # no duplicated devices, but device may not exist, in which case # the list is empty - self.enforce(len(device_assignments) <= 1) + if len(device_assignments) > 1: + raise qubes.exc.DeviceNotFound() device_assignments = self.fire_event_for_filter( device_assignments, devclass=devclass ) @@ -1487,7 +1858,9 @@ async def vm_device_list(self, endpoint): ep.name for ep in importlib.metadata.entry_points(group="qubes.devices") ), - no_payload=True, + wants_arg=None, + wants_payload=False, + dest_adminvm=None, scope="local", read=True, ) @@ -1512,7 +1885,8 @@ async def vm_device_attached(self, endpoint): ] # no duplicated devices, but device may not exist, in which case # the list is empty - self.enforce(len(device_assignments) <= 1) + if len(device_assignments) > 1: + raise qubes.exc.DeviceNotFound() device_assignments = [ a.clone( device=self.app.domains[a.backend_name].devices[devclass][ @@ -1545,6 +1919,9 @@ async def vm_device_attached(self, endpoint): ep.name for ep in importlib.metadata.entry_points(group="qubes.devices") ), + wants_arg=True, + wants_payload=True, + dest_adminvm=None, scope="local", write=True, ) @@ -1591,7 +1968,9 @@ def load_device_info(self, devclass) -> VirtualDevice: ep.name for ep in importlib.metadata.entry_points(group="qubes.devices") ), - no_payload=True, + wants_arg=True, + wants_payload=False, + dest_adminvm=None, scope="local", write=True, ) @@ -1613,6 +1992,9 @@ async def vm_device_unassign(self, endpoint): ep.name for ep in importlib.metadata.entry_points(group="qubes.devices") ), + wants_arg=True, + wants_payload=None, + dest_adminvm=None, scope="local", execute=True, ) @@ -1637,7 +2019,9 @@ async def vm_device_attach(self, endpoint, untrusted_payload): ep.name for ep in importlib.metadata.entry_points(group="qubes.devices") ), - no_payload=True, + wants_arg=True, + wants_payload=False, + dest_adminvm=None, scope="local", execute=True, ) @@ -1658,6 +2042,9 @@ async def vm_device_detach(self, endpoint): ep.name for ep in importlib.metadata.entry_points(group="qubes.devices") ), + wants_arg=True, + wants_payload=True, + dest_adminvm=None, scope="local", write=True, ) @@ -1679,7 +2066,7 @@ async def vm_device_set_required(self, endpoint, untrusted_payload): try: mode = allowed_values[untrusted_payload] except KeyError: - raise qubes.exc.PermissionDenied() + raise qubes.exc.ProtocolError("Invalid assignment mode") dev = VirtualDevice.from_qarg(self.arg, devclass, self.app.domains) @@ -1690,7 +2077,9 @@ async def vm_device_set_required(self, endpoint, untrusted_payload): @qubes.api.method( "admin.vm.device.denied.List", - no_payload=True, + wants_arg=False, + wants_payload=False, + dest_adminvm=None, scope="local", read=True, ) @@ -1700,14 +2089,19 @@ async def vm_device_denied_list(self): Returns a newline-separated string. """ - self.enforce(not self.arg) - self.fire_event_for_permission() denied = self.dest.devices_denied return "\n".join(map(repr, DeviceInterface.from_str_bulk(denied))) - @qubes.api.method("admin.vm.device.denied.Add", scope="local", write=True) + @qubes.api.method( + "admin.vm.device.denied.Add", + wants_arg=False, + wants_payload=None, + dest_adminvm=None, + scope="local", + write=True, + ) async def vm_device_denied_add(self, untrusted_payload): """ Add device interface(s) to the denied list for the VM. @@ -1715,11 +2109,16 @@ async def vm_device_denied_add(self, untrusted_payload): Payload: Encoded device interface (can be repeated without any separator). """ - payload = untrusted_payload.decode("ascii", errors="strict") + try: + payload = untrusted_payload.decode("ascii", errors="strict") + except UnicodeDecodeError: + raise qubes.exc.ProtocolError( + "Device interface contains non-ASCII characters" + ) to_add = DeviceInterface.from_str_bulk(payload) if len(set(to_add)) != len(to_add): - raise qubes.exc.QubesValueError( + raise qubes.exc.ProtocolError( "Duplicated device interfaces in payload." ) @@ -1735,7 +2134,12 @@ async def vm_device_denied_add(self, untrusted_payload): self.app.save() @qubes.api.method( - "admin.vm.device.denied.Remove", scope="local", write=True + "admin.vm.device.denied.Remove", + wants_arg=False, + wants_payload=None, + dest_adminvm=None, + scope="local", + write=True, ) async def vm_device_denied_remove(self, untrusted_payload): """ @@ -1747,14 +2151,19 @@ async def vm_device_denied_remove(self, untrusted_payload): """ denied = DeviceInterface.from_str_bulk(self.dest.devices_denied) - payload = untrusted_payload.decode("ascii", errors="strict") + try: + payload = untrusted_payload.decode("ascii", errors="strict") + except UnicodeDecodeError: + raise qubes.exc.ProtocolError( + "Device interface contains non-ASCII characters" + ) if payload != "all": to_remove = DeviceInterface.from_str_bulk(payload) else: to_remove = denied.copy() if len(set(to_remove)) != len(to_remove): - raise qubes.exc.QubesValueError( + raise qubes.exc.ProtocolError( "Duplicated device interfaces in payload." ) @@ -1769,11 +2178,14 @@ async def vm_device_denied_remove(self, untrusted_payload): self.app.save() @qubes.api.method( - "admin.vm.firewall.Get", no_payload=True, scope="local", read=True + "admin.vm.firewall.Get", + wants_arg=False, + wants_payload=False, + dest_adminvm=False, + scope="local", + read=True, ) async def vm_firewall_get(self): - self.enforce(not self.arg) - self.fire_event_for_permission() return "".join( @@ -1782,17 +2194,26 @@ async def vm_firewall_get(self): if rule.api_rule is not None ) - @qubes.api.method("admin.vm.firewall.Set", scope="local", write=True) + @qubes.api.method( + "admin.vm.firewall.Set", + wants_arg=False, + wants_payload=True, + dest_adminvm=False, + scope="local", + write=True, + ) async def vm_firewall_set(self, untrusted_payload): - self.enforce(not self.arg) rules = [] - for untrusted_line in untrusted_payload.decode( - "ascii", errors="strict" - ).splitlines(): - rule = qubes.firewall.Rule.from_api_string( - untrusted_rule=untrusted_line - ) - rules.append(rule) + try: + for untrusted_line in untrusted_payload.decode( + "ascii", errors="strict" + ).splitlines(): + rule = qubes.firewall.Rule.from_api_string( + untrusted_rule=untrusted_line + ) + rules.append(rule) + except UnicodeDecodeError: + raise qubes.exc.ProtocolError("Rules contains non-ASCII characters") self.fire_event_for_permission(rules=rules) @@ -1801,13 +2222,13 @@ async def vm_firewall_set(self, untrusted_payload): @qubes.api.method( "admin.vm.firewall.Reload", - no_payload=True, + wants_arg=False, + wants_payload=False, + dest_adminvm=False, scope="local", execute=True, ) async def vm_firewall_reload(self): - self.enforce(not self.arg) - self.fire_event_for_permission() self.dest.fire_event("firewall-changed") @@ -1823,6 +2244,10 @@ async def _load_backup_profile(self, profile_name, skip_passphrase=False): profile_path = os.path.join( qubes.config.backup_profile_dir, profile_name + ".conf" ) + if not os.path.exists(profile_path): + raise qubes.exc.QubesBackupProfileNotFoundError( + "Backup profile does not exist: {}".format(profile_name) + ) with open(profile_path, encoding="utf-8") as profile_file: profile_data = yaml.safe_load(profile_file) @@ -1867,11 +2292,14 @@ async def _load_backup_profile(self, profile_name, skip_passphrase=False): ) try: passphrase, _ = await passphrase_vm.run_service_for_stdio( - "qubes.BackupPassphrase+" + self.arg + "qubes.BackupPassphrase+" + profile_name ) # make it foolproof against "echo passphrase" implementation passphrase = passphrase.strip() - self.enforce(b"\n" not in passphrase) + self.enforce( + b"\n" not in passphrase, + reason="Passphrase must not contain newline character", + ) except subprocess.CalledProcessError: raise qubes.exc.QubesException( "Failed to retrieve passphrase from '{}' VM".format( @@ -1927,26 +2355,21 @@ def _backup_progress_callback(self, profile_name, progress): @qubes.api.method( "admin.backup.Execute", - no_payload=True, + wants_arg=True, + wants_payload=False, + dest_adminvm=True, scope="global", read=True, execute=True, ) async def backup_execute(self): - self.enforce(self.dest.name == "dom0") - self.enforce(self.arg) - self.enforce("/" not in self.arg) + self.enforce( + "/" not in self.arg, + reason="Backup profile ID must not contain forward slash character", + ) self.fire_event_for_permission() - profile_path = os.path.join( - qubes.config.backup_profile_dir, self.arg + ".conf" - ) - if not os.path.exists(profile_path): - raise qubes.exc.PermissionDenied( - "Backup profile {} does not exist".format(self.arg) - ) - if not hasattr(self.app, "api_admin_running_backups"): self.app.api_admin_running_backups = {} @@ -1969,12 +2392,18 @@ async def backup_execute(self): del self.app.api_admin_running_backups[self.arg] @qubes.api.method( - "admin.backup.Cancel", no_payload=True, scope="global", execute=True + "admin.backup.Cancel", + wants_arg=True, + wants_payload=False, + dest_adminvm=True, + scope="global", + execute=True, ) async def backup_cancel(self): - self.enforce(self.dest.name == "dom0") - self.enforce(self.arg) - self.enforce("/" not in self.arg) + self.enforce( + "/" not in self.arg, + reason="Backup profile ID must not contain forward slash character", + ) self.fire_event_for_permission() @@ -1987,23 +2416,21 @@ async def backup_cancel(self): self.app.api_admin_running_backups[self.arg].cancel() @qubes.api.method( - "admin.backup.Info", no_payload=True, scope="local", read=True + "admin.backup.Info", + wants_arg=True, + wants_payload=False, + dest_adminvm=True, + scope="local", + read=True, ) async def backup_info(self): - self.enforce(self.dest.name == "dom0") - self.enforce(self.arg) - self.enforce("/" not in self.arg) + self.enforce( + "/" not in self.arg, + reason="Backup profile ID must not contain forward slash character", + ) self.fire_event_for_permission() - profile_path = os.path.join( - qubes.config.backup_profile_dir, self.arg + ".conf" - ) - if not os.path.exists(profile_path): - raise qubes.exc.PermissionDenied( - "Backup profile {} does not exist".format(self.arg) - ) - backup = await self._load_backup_profile(self.arg, skip_passphrase=True) return backup.get_backup_summary() @@ -2057,11 +2484,14 @@ def _send_stats_single( return info_time, info @qubes.api.method( - "admin.vm.Stats", no_payload=True, scope="global", read=True + "admin.vm.Stats", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="global", + read=True, ) async def vm_stats(self): - self.enforce(not self.arg) - # run until client connection is terminated self.cancellable = True @@ -2088,10 +2518,14 @@ async def vm_stats(self): pass @qubes.api.method( - "admin.vm.CurrentState", no_payload=True, scope="local", read=True + "admin.vm.CurrentState", + wants_arg=False, + wants_payload=False, + dest_adminvm=None, + scope="local", + read=True, ) async def vm_current_state(self): - self.enforce(not self.arg) self.fire_event_for_permission() state = { @@ -2103,19 +2537,29 @@ async def vm_current_state(self): return " ".join("{}={}".format(k, v) for k, v in state.items()) @qubes.api.method( - "admin.vm.notes.Get", no_payload=True, scope="local", read=True + "admin.vm.notes.Get", + wants_arg=False, + wants_payload=False, + dest_adminvm=False, + scope="local", + read=True, ) async def vm_notes_get(self): """Get qube notes""" - self.enforce(self.dest.name != "dom0") self.fire_event_for_permission() notes = self.dest.get_notes() return notes - @qubes.api.method("admin.vm.notes.Set", scope="local", write=True) + @qubes.api.method( + "admin.vm.notes.Set", + wants_arg=False, + wants_payload=True, + dest_adminvm=False, + scope="local", + write=True, + ) async def vm_notes_set(self, untrusted_payload): """Set qube notes""" - self.enforce(self.dest.name != "dom0") self.fire_event_for_permission() if len(untrusted_payload) > 256000: raise qubes.exc.ProtocolError( diff --git a/qubes/api/internal.py b/qubes/api/internal.py index e74462f75..3b7362c10 100644 --- a/qubes/api/internal.py +++ b/qubes/api/internal.py @@ -166,17 +166,23 @@ class QubesInternalAPI(qubes.api.AbstractQubesAPI): SOCKNAME = "/var/run/qubesd.internal.sock" - @qubes.api.method("internal.GetSystemInfo", no_payload=True) + @qubes.api.method( + "internal.GetSystemInfo", + dest_adminvm=True, + wants_arg=False, + wants_payload=False, + ) async def getsysteminfo(self): - self.enforce(self.dest.name == "dom0") - self.enforce(not self.arg) - system_info = SystemInfoCache.get_system_info(self.app) - return json.dumps(system_info) @qubes.api.method( - "internal.vm.volume.ImportBegin", scope="local", write=True + "internal.vm.volume.ImportBegin", + wants_arg=True, + wants_payload=None, + dest_adminvm=None, + scope="local", + write=True, ) async def vm_volume_import(self, untrusted_payload): """Begin importing volume data. Payload is either size of new data @@ -191,7 +197,10 @@ async def vm_volume_import(self, untrusted_payload): payload) and response from that call will be actually send to the caller. """ - self.enforce(self.arg in self.dest.volumes.keys()) + if self.arg not in self.dest.volumes.keys(): + raise qubes.exc.ProtocolError( + "Argument not in {}".format(self.EXC_ARG_NOT_IN_DEST_VOLUMES) + ) if untrusted_payload: original_method = "admin.vm.volume.ImportWithSize" @@ -208,12 +217,14 @@ async def vm_volume_import(self, untrusted_payload): raise qubes.exc.QubesVMNotHaltedError(self.dest) requested_size = ( - self.validate_size(untrusted_payload) if untrusted_payload else None + self.validate_number(untrusted_payload, name="size of new data") + if untrusted_payload + else None ) del untrusted_payload path = await self.dest.storage.import_data(self.arg, requested_size) - self.enforce(" " not in path) + self.enforce(" " not in path, reason="Path contains whitespace") if requested_size is None: size = self.dest.volumes[self.arg].size else: @@ -227,7 +238,12 @@ async def vm_volume_import(self, untrusted_payload): return "{} {}".format(size, path) - @qubes.api.method("internal.vm.volume.ImportEnd") + @qubes.api.method( + "internal.vm.volume.ImportEnd", + wants_arg=True, + wants_payload=True, + dest_adminvm=None, + ) async def vm_volume_import_end(self, untrusted_payload): """ This is second half of admin.vm.volume.Import handling. It is called @@ -236,7 +252,10 @@ async def vm_volume_import_end(self, untrusted_payload): The payload is either 'ok', or 'fail\n'. """ - self.enforce(self.arg in self.dest.volumes.keys()) + if self.arg not in self.dest.volumes.keys(): + raise qubes.exc.ProtocolError( + "Argument not in {}".format(self.EXC_ARG_NOT_IN_DEST_VOLUMES) + ) success = untrusted_payload == b"ok" try: @@ -260,7 +279,12 @@ async def vm_volume_import_end(self, untrusted_payload): "Data import failed: {}".format(error) ) - @qubes.api.method("internal.SuspendPre", no_payload=True) + @qubes.api.method( + "internal.SuspendPre", + wants_payload=False, + wants_arg=False, + dest_adminvm=True, + ) async def suspend_pre(self): """ Method called before host system goes to sleep. @@ -343,7 +367,12 @@ async def suspend_pre(self): "Failed to suspend some qubes: {}".format(failed) ) - @qubes.api.method("internal.SuspendPost", no_payload=True) + @qubes.api.method( + "internal.SuspendPost", + wants_payload=False, + wants_arg=False, + dest_adminvm=True, + ) async def suspend_post(self): """ Method called after host system wake up from sleep. diff --git a/qubes/api/misc.py b/qubes/api/misc.py index eddf21e7d..ecd81291d 100644 --- a/qubes/api/misc.py +++ b/qubes/api/misc.py @@ -32,7 +32,12 @@ class QubesMiscAPI(qubes.api.AbstractQubesAPI): SOCKNAME = "/var/run/qubesd.misc.sock" - @qubes.api.method("qubes.FeaturesRequest", no_payload=True) + @qubes.api.method( + "qubes.FeaturesRequest", + wants_payload=False, + wants_arg=False, + dest_adminvm=True, + ) async def qubes_features_request(self): """qubes.FeaturesRequest handler @@ -45,9 +50,6 @@ async def qubes_features_request(self): appropriate extensions. Requests not explicitly handled by some extension are ignored. """ - self.enforce(self.dest.name == "dom0") - self.enforce(not self.arg) - prefix = "/features-request/" keys = self.src.untrusted_qdb.list(prefix) @@ -61,21 +63,26 @@ async def qubes_features_request(self): safe_set = string.ascii_letters + string.digits + "-.,_= " for untrusted_key in untrusted_features: untrusted_value = untrusted_features[untrusted_key] - self.enforce(all((c in safe_set) for c in untrusted_value)) + self.enforce( + all((c in safe_set) for c in untrusted_value), + reason="Value outside safe set: " + safe_set, + ) await self.src.fire_event_async( "features-request", untrusted_features=untrusted_features ) self.app.save() - @qubes.api.method("qubes.NotifyTools", no_payload=True) + @qubes.api.method( + "qubes.NotifyTools", + wants_payload=False, + wants_arg=False, + dest_adminvm=True, + ) async def qubes_notify_tools(self): """ Legacy version of qubes.FeaturesRequest, used by Qubes Windows Tools """ - self.enforce(self.dest.name == "dom0") - self.enforce(not self.arg) - untrusted_features = {} safe_set = string.ascii_letters + string.digits expected_features = ( @@ -93,7 +100,10 @@ async def qubes_notify_tools(self): untrusted_value = untrusted_value.decode( "ascii", errors="strict" ) - self.enforce(all((c in safe_set) for c in untrusted_value)) + self.enforce( + all((c in safe_set) for c in untrusted_value), + reason="Value outside safe set: " + safe_set, + ) untrusted_features[feature] = untrusted_value del untrusted_value @@ -102,7 +112,12 @@ async def qubes_notify_tools(self): ) self.app.save() - @qubes.api.method("qubes.NotifyUpdates") + @qubes.api.method( + "qubes.NotifyUpdates", + wants_payload=True, + wants_arg=False, + dest_adminvm=True, + ) async def qubes_notify_updates(self, untrusted_payload): """ Receive VM notification about updates availability @@ -112,7 +127,10 @@ async def qubes_notify_updates(self, untrusted_payload): """ untrusted_update_count = untrusted_payload.strip() - self.enforce(untrusted_update_count.isdigit()) + self.enforce( + untrusted_update_count.isdigit(), + reason="Update count is not a digit", + ) # now sanitized update_count = int(untrusted_update_count) del untrusted_update_count diff --git a/qubes/exc.py b/qubes/exc.py index dcd145c00..f3e20994a 100644 --- a/qubes/exc.py +++ b/qubes/exc.py @@ -27,6 +27,40 @@ class QubesException(Exception): """Exception that can be shown to the user""" +class ProtocolError(AssertionError): + """Raised deliberately by handlers to indicate a malformed client request. + + Client programming errors. The client made a request that it should have + known better than to send in the first place. Includes things like passing + an argument or payload to a service documented to not take one. The only + way that a correctly behaving client can get ProtocolError is qubesd is + either buggy or too old. In HTTP, this would be 400 Bad Request. + + This does not provide any useful information to the client making the + request. Therefore, it should only be raised if there is a client + *programming* error, such as passing an argument to a request that does not + take an argument. It should not be used to reject requests that are valid, + but which qubesd is refusing to process. Instead, raise a subclass of + :py:class:`QubesException` with a useful error message. + """ + + +class PermissionDenied(PermissionError): + """Raised deliberately by handlers to inform the request is prohibited. + + The request is valid, but the client does not have permission to perform + the operation. Clients in dom0 should usually not get this error. In HTTP, + this would be 403 Forbidden. + + This does not provide any useful information to the client making the + request. Therefore, it should only be raised if there is a client + *programming* error, such as passing an argument to a request that does not + take an argument. It should not be used to reject requests that are valid, + but which qubesd is refusing to process. Instead, raise a subclass of + :py:class:`QubesException` with a useful error message. + """ + + class QubesVMNotFoundError(QubesException, KeyError): """Domain cannot be found in the system""" @@ -50,6 +84,10 @@ def __init__(self, uuid: str) -> None: self.vmname = uuid +class QubesVMAlreadyExistsError(QubesException): + """Domain was requested to be created, but it already exists.""" + + class QubesVMError(QubesException): """Some problem with domain state.""" @@ -167,9 +205,9 @@ def __init__(self, vm, msg=None): class QubesPoolInUseError(QubesException): """VM is in use, cannot remove.""" - def __init__(self, pool, msg=None): + def __init__(self, pool_name, msg=None): super().__init__( - msg or "Storage pool is in use: {!r}".format(pool.name) + msg or "Storage pool is in use: {!r}".format(pool_name) ) @@ -212,6 +250,10 @@ def __init__(self, msg=None): super().__init__(msg or "This feature is not available") +class QubesBackupProfileNotFoundError(QubesException): + """Requested backup profile does not exist.""" + + class BackupCancelledError(QubesException): """Thrown at user when backup was manually cancelled""" @@ -276,14 +318,6 @@ def __str__(self): return QubesException.__str__(self) -class ProtocolError(AssertionError): - """Raised when something is wrong with data received""" - - -class PermissionDenied(Exception): - """Raised deliberately by handlers when we decide not to cooperate""" - - class DeviceNotAssigned(QubesException, KeyError): """ Trying to unassign not assigned device. @@ -322,3 +356,52 @@ class UnexpectedDeviceProperty(QubesException, ValueError): class StoragePoolException(QubesException): """A general storage exception""" + + +class QubesVolumeCopyTokenNotFoundError(ProtocolError, KeyError): + """Domain volume copy did not specify a configured token.""" + + def __init__(self, msg=None): + super().__init__(msg or "Token to clone volume of qube was not found") + + +class QubesVolumeCopyInUseError(QubesException): + """Domain volume is already being cloned.""" + + def __init__(self, vm, volume, msg=None): + super().__init__( + vm, + msg + or "Domain volume is being cloned already: {!r}:{!r}".format( + vm.name, volume + ), + ) + + +class QubesVolumeRevisionNotFoundError(KeyError): + """Specified revision not found in qube volume.""" + + +class QubesPoolNotFoundError(KeyError): + """Pool does not exist.""" + + +class QubesVolumeNotFoundError(KeyError): + """Pool does not exist.""" + + +class QubesInvalidLabelError(ProtocolError): + """Domain label is invalid.""" + + +class QubesInvalidLabelValueError(ProtocolError): + """Domain label value is invalid.""" + + +class QubesLabelInUseError(QubesException): + """Cannot remove or add label as it is still in use.""" + + def __init__(self, label, msg=None): + super().__init__( + msg or "Label is still in use: {!r}".format(label), + ) diff --git a/qubes/tests/api.py b/qubes/tests/api.py index 228fde64f..4c2e420d9 100644 --- a/qubes/tests/api.py +++ b/qubes/tests/api.py @@ -201,3 +201,88 @@ def test_006_target_adminvm(self): response, b"0\0src: b'src', dest: b'dom0', arg: b'arg', payload: b'payload'", ) + + +class TC_10_QubesAPIValidation(qubes.tests.QubesTestCase): + + def test_000_method_param__wants_arg(self): + func = qubes.api.method( + "rpc", + wants_arg="test", + wants_payload=None, + dest_adminvm=None, + ) + with self.assertRaisesRegex(ValueError, "wants_arg"): + func("rpc") + + def test_000_method_param__wants_payload(self): + func = qubes.api.method( + "rpc", + wants_arg=None, + wants_payload="test", + dest_adminvm=None, + ) + with self.assertRaisesRegex(ValueError, "wants_payload"): + func("rpc") + + def test_000_method_param__dest_adminvm(self): + func = qubes.api.method( + "rpc", + wants_arg=None, + wants_payload=None, + dest_adminvm="test", + ) + with self.assertRaisesRegex(ValueError, "dest_adminvm"): + func("rpc") + + def test_000_method_param__read(self): + func = qubes.api.method( + "rpc", + wants_arg=None, + wants_payload=None, + dest_adminvm=None, + read="test", + ) + with self.assertRaisesRegex(ValueError, "read"): + func("rpc") + + def test_000_method_param__write(self): + func = qubes.api.method( + "rpc", + wants_arg=None, + wants_payload=None, + dest_adminvm=None, + write="test", + ) + with self.assertRaisesRegex(ValueError, "write"): + func("rpc") + + def test_000_method_param__execute(self): + func = qubes.api.method( + "rpc", + wants_arg=None, + wants_payload=None, + dest_adminvm=None, + execute="test", + ) + with self.assertRaisesRegex(ValueError, "execute"): + func("rpc") + + def test_000_method_param__scope(self): + func = qubes.api.method( + "rpc", + wants_arg=None, + wants_payload=None, + dest_adminvm=None, + scope="test", + ) + with self.assertRaisesRegex(ValueError, "scope"): + func("rpc") + + def test_030_validate_number(self): + with self.assertRaisesRegex( + qubes.exc.ProtocolError, "of type bytes, got 'int'" + ): + qubes.api.AbstractQubesAPI.validate_number( + "", untrusted_number=30, name="size" + ) diff --git a/qubes/tests/api_admin.py b/qubes/tests/api_admin.py index bc14f32fe..8816f4bbe 100644 --- a/qubes/tests/api_admin.py +++ b/qubes/tests/api_admin.py @@ -713,7 +713,7 @@ def test_100_vm_volume_snapshot_invalid_volume(self): }, } self.vm.volumes.configure_mock(**volumes_conf) - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.vm.volume.Snapshots", b"test-vm1", b"no-such-volume" ) @@ -728,7 +728,7 @@ def test_100_vm_volume_snapshot_invalid_revision(self): "keys.return_value": ["root", "private", "volatile", "kernel"] } self.vm.volumes.configure_mock(**volumes_conf) - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.vm.volume.Snapshots", b"test-vm1", @@ -783,7 +783,7 @@ def test_110_vm_volume_revert_invalid_rev(self): } self.vm.volumes.configure_mock(**volumes_conf) self.vm.storage = unittest.mock.Mock() - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.QubesVolumeRevisionNotFoundError): self.call_mgmt_func( b"admin.vm.volume.Revert", b"test-vm1", @@ -1053,6 +1053,32 @@ def test_160_pool_add(self, mock_parameters, mock_drivers): ) self.assertTrue(self.app.save.called) + @unittest.mock.patch("qubes.storage.pool_drivers") + @unittest.mock.patch("qubes.storage.driver_parameters") + def test_160_pool_add_wrong_payload(self, mock_parameters, mock_drivers): + self.app.pools = { + "file": unittest.mock.Mock(), + "lvm": unittest.mock.Mock(), + } + + mock_drivers.return_value = ["driver1", "driver2"] + mock_parameters.side_effect = lambda driver: { + "driver1": {"param1": True, "param2": True}, + "driver2": {"param3": True, "param4": True}, + }[driver] + + add_pool_mock, self.app.add_pool = self.coroutine_mock() + + with self.assertRaises(qubes.exc.ProtocolError): + self.call_mgmt_func( + b"admin.pool.Add", + b"dom0", + b"driver1", + "name=test-pool\nparam1=\u00f6\n".encode(), + ) + self.assertEqual(add_pool_mock.mock_calls, []) + self.assertFalse(self.app.save.called) + @unittest.mock.patch("qubes.storage.pool_drivers") @unittest.mock.patch("qubes.storage.driver_parameters") def test_160_pool_add_missing_param(self, mock_parameters, mock_drivers): @@ -1095,7 +1121,7 @@ def test_160_pool_add_invalid_driver(self, mock_parameters, mock_drivers): add_pool_mock, self.app.add_pool = self.coroutine_mock() - with self.assertRaises(qubes.exc.QubesException): + with self.assertRaises(qubes.exc.PermissionDenied): self.call_mgmt_func( b"admin.pool.Add", b"dom0", @@ -1123,7 +1149,7 @@ def test_160_pool_add_invalid_param(self, mock_parameters, mock_drivers): add_pool_mock, self.app.add_pool = self.coroutine_mock() - with self.assertRaises(qubes.exc.QubesException): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.pool.Add", b"dom0", @@ -1153,7 +1179,7 @@ def test_160_pool_add_missing_name(self, mock_parameters, mock_drivers): add_pool_mock, self.app.add_pool = self.coroutine_mock() - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.pool.Add", b"dom0", @@ -1181,7 +1207,7 @@ def test_160_pool_add_existing_pool(self, mock_parameters, mock_drivers): add_pool_mock, self.app.add_pool = self.coroutine_mock() - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.QubesPoolInUseError): self.call_mgmt_func( b"admin.pool.Add", b"dom0", @@ -1211,7 +1237,7 @@ def test_160_pool_add_invalid_config_format( add_pool_mock, self.app.add_pool = self.coroutine_mock() - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.pool.Add", b"dom0", @@ -1311,13 +1337,31 @@ def test_200_label_create_invalid_color(self): "keys.return_value": range(1, 9), } self.app.labels.configure_mock(**labels_config) - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaisesRegex( + qubes.exc.QubesInvalidLabelValueError, "encoded in ASCII" + ): self.call_mgmt_func( - b"admin.label.Create", b"dom0", b"cyan", b"abcd" + b"admin.label.Create", b"dom0", b"cyan", "\u00f6".encode() + ) + with self.assertRaisesRegex( + qubes.exc.QubesInvalidLabelValueError, "length of 8" + ): + self.call_mgmt_func( + b"admin.label.Create", b"dom0", b"cyan", b"0x00fffff" + ) + with self.assertRaisesRegex( + qubes.exc.QubesInvalidLabelValueError, "must start with: 0x" + ): + self.call_mgmt_func( + b"admin.label.Create", b"dom0", b"cyan", b"0X00ffff" + ) + with self.assertRaisesRegex( + qubes.exc.QubesInvalidLabelValueError, + "hexadecimal digits after prefix", + ): + self.call_mgmt_func( + b"admin.label.Create", b"dom0", b"cyan", b"0x00fffg" ) - self.assertEqual( - self.app.get_label.mock_calls, [unittest.mock.call("cyan")] - ) self.assertEqual(self.app.labels.mock_calls, []) self.assertFalse(self.app.save.called) @@ -1329,15 +1373,15 @@ def test_200_label_create_invalid_name(self): "keys.return_value": range(1, 9), } self.app.labels.configure_mock(**labels_config) - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.label.Create", b"dom0", b"01", b"0xff0000" ) - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.label.Create", b"dom0", b"../xxx", b"0xff0000" ) - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.label.Create", b"dom0", @@ -1351,7 +1395,7 @@ def test_200_label_create_invalid_name(self): def test_200_label_create_already_exists(self): self.app.get_label = unittest.mock.Mock(wraps=self.app.get_label) - with self.assertRaises(qubes.exc.QubesValueError): + with self.assertRaises(qubes.exc.QubesLabelInUseError): self.call_mgmt_func(b"admin.label.Create", b"dom0", b"red", b"abcd") self.assertEqual( self.app.get_label.mock_calls, [unittest.mock.call("red")] @@ -1373,8 +1417,19 @@ def test_210_label_remove(self): ) self.assertTrue(self.app.save.called) + def test_211_label_remove_in_use(self): + label = qubes.Label(9, "0x00ffff", "cyan") + self.app.labels[9] = label + self.app.get_label = unittest.mock.Mock(**{"return_value.index": 9}) + self.app.labels = unittest.mock.MagicMock(wraps=self.app.labels) + self.vm.label = "cyan" + with self.assertRaises(qubes.exc.QubesLabelInUseError): + self.call_mgmt_func(b"admin.label.Remove", b"dom0", b"cyan") + self.assertEqual(self.app.labels.mock_calls, []) + self.assertFalse(self.app.save.called) + def test_210_label_remove_invalid_label(self): - with self.assertRaises(qubes.exc.QubesValueError): + with self.assertRaises(qubes.exc.QubesLabelNotFoundError): self.call_mgmt_func( b"admin.label.Remove", b"dom0", b"no-such-label" ) @@ -1383,7 +1438,7 @@ def test_210_label_remove_invalid_label(self): def test_210_label_remove_default_label(self): self.app.labels = unittest.mock.MagicMock(wraps=self.app.labels) self.app.get_label = unittest.mock.Mock(**{"return_value.index": 6}) - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(b"admin.label.Remove", b"dom0", b"blue") self.assertEqual(self.app.labels.mock_calls, []) self.assertFalse(self.app.save.called) @@ -1391,7 +1446,7 @@ def test_210_label_remove_default_label(self): def test_210_label_remove_in_use(self): self.app.labels = unittest.mock.MagicMock(wraps=self.app.labels) self.app.get_label = unittest.mock.Mock(**{"return_value.index": 1}) - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(b"admin.label.Remove", b"dom0", b"red") self.assertEqual(self.app.labels.mock_calls, []) self.assertFalse(self.app.save.called) @@ -1473,7 +1528,7 @@ async def coroutine_mock(*args, **kwargs): return func_mock(*args, **kwargs) self.vm.shutdown = coroutine_mock - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(b"admin.vm.Shutdown", b"test-vm1", b"forcewait") func_mock.assert_not_called() @@ -1657,6 +1712,56 @@ async def fire_event(): ], ) + def test_273_events_single(self): + vm2 = self.app.add_new_vm( + "AppVM", + label="red", + name="test-vm2", + template="test-template", + ) + + with tempfile.TemporaryDirectory() as tmpdir: + tmpdir = pathlib.Path(tmpdir) + with unittest.mock.patch( + "qubes.ext.admin.AdminExtension._instance.policy_cache.paths", + [pathlib.Path(tmpdir)], + ): + with (tmpdir / "admin.policy").open("w") as file: + file.write("admin.Events * test-vm1 test-vm2 allow") + + send_event = unittest.mock.Mock(spec=[]) + mgmt_obj = qubes.api.admin.QubesAdminAPI( + self.app, + b"test-vm1", + b"admin.Events", + b"test-vm2", + b"", + send_event=send_event, + ) + + async def fire_event(): + vm2.fire_event("test-event2", arg1="abc") + self.vm.fire_event("test-event", arg1="abc") + mgmt_obj.cancel() + return vm2 + + loop = asyncio.get_event_loop() + execute_task = asyncio.ensure_future( + mgmt_obj.execute(untrusted_payload=b"") + ) + event_task = asyncio.ensure_future(fire_event()) + loop.run_until_complete(execute_task) + event_task.result() + self.assertIsNone(execute_task.result()) + + self.assertEqual( + send_event.mock_calls, + [ + unittest.mock.call(self.app, "connection-established"), + unittest.mock.call(vm2, "test-event2", arg1="abc"), + ], + ) + def test_280_feature_list(self): self.vm.features["test-feature"] = "some-value" value = self.call_mgmt_func(b"admin.vm.feature.List", b"test-vm1") @@ -1828,6 +1933,31 @@ def test_322_feature_set_invalid(self): self.assertNotIn("test-feature", self.vm.features) self.assertFalse(self.app.save.called) + def test_322_feature_set_invalid_key(self): + with self.assertRaisesRegex( + qubes.exc.ProtocolError, "expected an argument" + ): + self.call_mgmt_func( + b"admin.vm.feature.Set", + b"test-vm1", + b"", + b"value", + ) + self.assertNotIn("test-feature", self.vm.features) + self.assertFalse(self.app.save.called) + + with self.assertRaisesRegex( + qubes.exc.ProtocolError, "illegal characters" + ): + self.call_mgmt_func( + b"admin.vm.feature.Set", + b"test-vm1", + b" ", + b"", + ) + self.assertNotIn("test-feature", self.vm.features) + self.assertFalse(self.app.save.called) + def test_323_feature_set_service_too_long(self): with self.assertRaises(qubes.exc.QubesValueError): self.call_mgmt_func( @@ -1887,7 +2017,7 @@ def test_330_vm_create_standalone(self, storage_mock): @unittest.mock.patch("qubes.storage.Storage.create") def test_331_vm_create_standalone_spurious_template(self, storage_mock): storage_mock.side_effect = self.dummy_coro - with self.assertRaises(qubes.exc.QubesValueError): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.vm.Create.StandaloneVM", b"dom0", @@ -2014,7 +2144,7 @@ def test_336_vm_create_spurious_pool(self, storage_mock): @unittest.mock.patch("qubes.storage.Storage.create") def test_337_vm_create_duplicate_name(self, storage_mock): storage_mock.side_effect = self.dummy_coro - with self.assertRaises(qubes.exc.QubesException): + with self.assertRaises(qubes.exc.QubesVMAlreadyExistsError): self.call_mgmt_func( b"admin.vm.Create.AppVM", b"dom0", @@ -2140,7 +2270,7 @@ def test_343_vm_create_in_pool_invalid_pool2(self, storage_mock): @unittest.mock.patch("qubes.storage.Storage.create") def test_344_vm_create_in_pool_invalid_volume(self, storage_mock): storage_mock.side_effect = self.dummy_coro - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.vm.CreateInPool.AppVM", b"dom0", @@ -2200,6 +2330,41 @@ def test_410_property_get_str(self): ) self.assertEqual(value, "default=False type=str 1.0") + def test_411_property_get_all(self): + expected = """check_updates_vm default=True type=bool True +clockvm default=True type=vm +default_audiovm default=True type=vm dom0 +default_dispvm default=True type=vm +default_guivm default=True type=vm dom0 +default_kernel default=False type=str 1.0 +default_netvm default=False type=vm +default_pool default=False type=str varlibqubes +default_pool_kernel default=False type=str linux-kernel +default_pool_private default=True type=str varlibqubes +default_pool_root default=True type=str varlibqubes +default_pool_volatile default=True type=str varlibqubes +default_qrexec_timeout default=True type=int 60 +default_shutdown_timeout default=True type=int 60 +default_template default=False type=vm test-template +management_dispvm default=True type=vm +stats_interval default=True type=int 3 +updatevm default=True type=vm \n""" + value = self.call_mgmt_func(b"admin.property.GetAll", b"dom0", b"") + self.maxDiff = None + self.assertEqual(value, expected) + + def test_411_property_get_default_none(self): + value = self.call_mgmt_func( + b"admin.property.GetDefault", b"dom0", b"default_template" + ) + self.assertEqual(value, None) + + def test_411_property_get_default_bool(self): + value = self.call_mgmt_func( + b"admin.property.GetDefault", b"dom0", b"updatevm" + ) + self.assertEqual(value, "type=vm ") + def test_420_propert_set_str(self): # actual function tested for admin.vm.property.* already with unittest.mock.patch("qubes.property.__set__") as mock: @@ -3065,6 +3230,20 @@ def test_503_vm_remove_dom0(self, mock_rmtree, mock_remove): # Import tests # (internal methods, normally called from qubes-rpc script) + def test_509_vm_volume_import_begin_wrong_volume(self): + with self.assertRaises(qubes.exc.ProtocolError): + self.call_internal_mgmt_func( + b"internal.vm.volume.ImportBegin", b"test-vm1", b"inexistent" + ) + self.assertFalse(self.app.save.called) + + def test_509_vm_volume_import_end_wrong_volume(self): + with self.assertRaises(qubes.exc.ProtocolError): + self.call_internal_mgmt_func( + b"internal.vm.volume.ImportEnd", b"test-vm1", b"inexistent" + ) + self.assertFalse(self.app.save.called) + def test_510_vm_volume_import(self): value = self.call_internal_mgmt_func( b"internal.vm.volume.ImportBegin", b"test-vm1", b"private" @@ -3258,6 +3437,64 @@ def test_522_vm_volume_clone_invalid_volume2(self): ) self.assertFalse(self.app.save.called) + def test_522_vm_volume_clone_invalid_token(self): + self.setup_for_clone() + + self.vm.volumes["private"].is_running = lambda: False + self.vm.storage.get_volume = lambda x: x + self.vm2.storage.get_volume = lambda x: x + + token = self.call_mgmt_func( + b"admin.vm.volume.CloneFrom", b"test-vm1", b"private", b"" + ) + with self.assertRaises(qubes.exc.ProtocolError): + self.call_mgmt_func( + b"admin.vm.volume.CloneTo", + b"test-vm1", + b"private", + token.encode() + "\u00f6".encode(), + ) + self.assertNotIn( + "init_volume().import_volume", + map(operator.itemgetter(0), self.pool.mock_calls), + ) + self.assertFalse(self.app.save.called) + + def test_523_vm_volume_clone_wrong_pool(self): + self.setup_for_clone() + + self.vm.volumes["private"].is_running = lambda: False + self.vm.storage.get_volume = lambda x: x + self.vm2.storage.get_volume = lambda x: x + + token = self.call_mgmt_func( + b"admin.vm.volume.CloneFrom", b"test-vm1", b"private", b"" + ) + + self.app.pools = { + "pool1": unittest.mock.Mock( + config={"param1": "value1", "param2": "value2"}, + usage=102400, + size=204800, + volumes={ + "vol1": unittest.mock.Mock(), + "vol2": unittest.mock.Mock(), + }, + ) + } + with self.assertRaises(qubes.exc.QubesPoolNotFoundError): + self.call_mgmt_func( + b"admin.vm.volume.CloneTo", + b"test-vm1", + b"private", + token.encode(), + ) + self.assertNotIn( + "init_volume().import_volume", + map(operator.itemgetter(0), self.pool.mock_calls), + ) + self.assertFalse(self.app.save.called) + def test_523_vm_volume_clone_removed_volume(self): self.setup_for_clone() @@ -3276,7 +3513,7 @@ def get_volume(vid): return unittest.mock.DEFAULT self.pool.get_volume.side_effect = get_volume - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.QubesVolumeNotFoundError): self.call_mgmt_func( b"admin.vm.volume.CloneTo", b"test-vm1", @@ -3296,7 +3533,7 @@ def test_524_vm_volume_clone_invlid_token(self): self.vm.storage.get_volume = lambda x: x self.vm2.storage.get_volume = lambda x: x - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.vm.volume.CloneTo", b"test-vm1", @@ -3380,6 +3617,13 @@ def test_570_firewall_get(self): self.assertFalse(self.vm.firewall.save.called) self.assertFalse(self.app.save.called) + def test_570_firewall_get_invalid_dest(self): + self.vm.firewall.save = unittest.mock.Mock() + with self.assertRaisesRegex( + qubes.exc.ProtocolError, "destination to not be AdminVM" + ): + self.call_mgmt_func(b"admin.vm.firewall.Get", b"dom0", b"") + def test_571_firewall_get_non_default(self): self.vm.firewall.save = unittest.mock.Mock() self.vm.firewall.rules = [ @@ -3487,7 +3731,7 @@ def test_585_firewall_set_invalid(self): rules_txt = ( "action=accept dstports=1-1024 comment=ążźł\n" "action=drop\n" ) - with self.assertRaises(UnicodeDecodeError): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.vm.firewall.Set", b"test-vm1", b"", rules_txt.encode() ) @@ -3581,6 +3825,18 @@ def test_602_backup_info_profile_missing_destination_vm(self): b"admin.backup.Info", b"dom0", b"testprofile" ) + def test_603_backup_info_profile_missing(self): + with tempfile.TemporaryDirectory() as profile_dir: + with unittest.mock.patch( + "qubes.config.backup_profile_dir", profile_dir + ): + with self.assertRaises( + qubes.exc.QubesBackupProfileNotFoundError + ): + self.call_mgmt_func( + b"admin.backup.Info", b"dom0", b"testprofile" + ) + def test_610_backup_cancel_not_running(self): with self.assertRaises(qubes.exc.QubesException): self.call_mgmt_func(b"admin.backup.Cancel", b"dom0", b"testprofile") @@ -4003,6 +4259,17 @@ def test_642_vm_create_disposable_not_allowed(self, storage_mock): self.call_mgmt_func(b"admin.vm.CreateDisposable", b"test-vm1") self.assertFalse(self.app.save.called) + @unittest.mock.patch("qubes.storage.Storage.create") + def test_642_vm_create_disposable_wrong_arg(self, mock_storage): + mock_storage.side_effect = self.dummy_coro + self.vm.template_for_dispvms = True + self.app.default_dispvm = self.vm + with self.assertRaises(qubes.exc.PermissionDenied): + self.call_mgmt_func( + b"admin.vm.CreateDisposable", b"test-vm1", b"oops" + ) + self.assertFalse(self.app.save.called) + @unittest.mock.patch("qubes.vm.dispvm.DispVM.start") @unittest.mock.patch("qubes.storage.Storage.verify") @unittest.mock.patch("qubes.storage.Storage.create") @@ -4230,7 +4497,7 @@ def test_655_vm_device_set_mode_invalid_value(self): with unittest.mock.patch.object( qubes.vm.qubesvm.QubesVM, "is_halted", lambda _: False ): - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.vm.device.testclass.Set.assignment", b"test-vm1", @@ -4284,7 +4551,7 @@ def test_663_vm_device_denied_add_multiple(self): def test_664_vm_device_denied_add_repeated(self): self.vm.devices_denied = "b******p012345p53**2*" - with self.assertRaises(qubes.exc.QubesValueError): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.vm.device.denied.Add", b"test-vm1", @@ -4294,6 +4561,20 @@ def test_664_vm_device_denied_add_repeated(self): self.assertEqual(self.vm.devices_denied, "b******p012345p53**2*") self.assertFalse(self.app.save.called) + def test_664_vm_device_denied_add_invalid(self): + self.vm.devices_denied = "b******p012345p53**2*" + with self.assertRaisesRegex( + qubes.exc.ProtocolError, "non-ASCII characters" + ): + self.call_mgmt_func( + b"admin.vm.device.denied.Add", + b"test-vm1", + b"", + "u112233u112233\u00f6\n".encode(), + ) + self.assertEqual(self.vm.devices_denied, "b******p012345p53**2*") + self.assertFalse(self.app.save.called) + def test_665_vm_device_denied_add_present(self): self.vm.devices_denied = "b******p012345p53**2*" self.call_mgmt_func( @@ -4320,7 +4601,7 @@ def test_670_vm_device_denied_remove(self): def test_671_vm_device_denied_remove_repeated(self): self.vm.devices_denied = "b******p012345p53**2*" - with self.assertRaises(qubes.exc.QubesValueError): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.vm.device.denied.Remove", b"test-vm1", @@ -4330,6 +4611,20 @@ def test_671_vm_device_denied_remove_repeated(self): self.assertEqual(self.vm.devices_denied, "b******p012345p53**2*") self.assertFalse(self.app.save.called) + def test_671_vm_device_denied_remove_invalid(self): + self.vm.devices_denied = "b******p012345p53**2*" + with self.assertRaisesRegex( + qubes.exc.ProtocolError, "non-ASCII characters" + ): + self.call_mgmt_func( + b"admin.vm.device.denied.Remove", + b"test-vm1", + b"", + "u112233u112233\u00f6\n".encode(), + ) + self.assertEqual(self.vm.devices_denied, "b******p012345p53**2*") + self.assertFalse(self.app.save.called) + def test_672_vm_device_denied_remove_all(self): self.vm.devices_denied = "b******p012345p53**2*" self.call_mgmt_func( @@ -4841,17 +5136,16 @@ def test_991_vm_unexpected_argument(self): vm_mock.qid = self.vm.qid vm_mock.__lt__ = lambda x, y: x.qid < y.qid self.app.domains._dict[self.vm.qid] = vm_mock - exceptions = (qubes.exc.PermissionDenied, qubes.exc.ProtocolError) for method in methods_with_no_argument: # should reject argument regardless of having payload or not with self.subTest(method.decode("ascii")): - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(method, b"test-vm1", b"some-arg", b"") self.assertFalse(vm_mock.called) self.assertFalse(self.app.save.called) with self.subTest(method.decode("ascii") + "+payload"): - with self.assertRaises(exceptions): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( method, b"test-vm1", b"unexpected-arg", b"some-payload" ) @@ -4920,17 +5214,16 @@ def test_993_dom0_unexpected_argument(self): vm_mock.qid = self.vm.qid vm_mock.__lt__ = lambda x, y: x.qid < y.qid self.app.domains._dict[self.vm.qid] = vm_mock - exceptions = (qubes.exc.PermissionDenied, qubes.exc.ProtocolError) for method in methods_with_no_argument: # should reject argument regardless of having payload or not with self.subTest(method.decode("ascii")): - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(method, b"dom0", b"some-arg", b"") self.assertFalse(vm_mock.called) self.assertFalse(self.app.save.called) with self.subTest(method.decode("ascii") + "+payload"): - with self.assertRaises(exceptions): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( method, b"dom0", b"unexpected-arg", b"some-payload" ) @@ -4976,29 +5269,28 @@ def test_994_dom0_only_calls(self): vm_mock.qid = self.vm.qid vm_mock.__lt__ = lambda x, y: x.qid < y.qid self.app.domains._dict[self.vm.qid] = vm_mock - exceptions = (qubes.exc.PermissionDenied, qubes.exc.ProtocolError) for method in methods_for_dom0_only: # should reject call regardless of having payload or not with self.subTest(method.decode("ascii")): - with self.assertRaises(exceptions): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(method, b"test-vm1", b"", b"") self.assertFalse(vm_mock.called) self.assertFalse(self.app.save.called) with self.subTest(method.decode("ascii") + "+arg"): - with self.assertRaises(exceptions): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(method, b"test-vm1", b"some-arg", b"") self.assertFalse(vm_mock.called) self.assertFalse(self.app.save.called) with self.subTest(method.decode("ascii") + "+payload"): - with self.assertRaises(exceptions): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(method, b"test-vm1", b"", b"payload") self.assertFalse(vm_mock.called) self.assertFalse(self.app.save.called) with self.subTest(method.decode("ascii") + "+arg+payload"): - with self.assertRaises(exceptions): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( method, b"test-vm1", b"some-arg", b"some-payload" ) @@ -5061,30 +5353,29 @@ def test_995_vm_only_calls(self): vm_mock.qid = self.vm.qid vm_mock.__lt__ = lambda x, y: x.qid < y.qid self.app.domains._dict[self.vm.qid] = vm_mock - exceptions = (qubes.exc.PermissionDenied, qubes.exc.ProtocolError) for method in methods_for_vm_only: # should reject payload regardless of having argument or not # should reject call regardless of having payload or not with self.subTest(method.decode("ascii")): - with self.assertRaises(exceptions): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(method, b"dom0", b"", b"") self.assertFalse(vm_mock.called) self.assertFalse(self.app.save.called) with self.subTest(method.decode("ascii") + "+arg"): - with self.assertRaises(exceptions): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(method, b"dom0", b"some-arg", b"") self.assertFalse(vm_mock.called) self.assertFalse(self.app.save.called) with self.subTest(method.decode("ascii") + "+payload"): - with self.assertRaises(exceptions): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(method, b"dom0", b"", b"payload") self.assertFalse(vm_mock.called) self.assertFalse(self.app.save.called) with self.subTest(method.decode("ascii") + "+arg+payload"): - with self.assertRaises(exceptions): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( method, b"dom0", b"some-arg", b"some-payload" ) diff --git a/qubes/tests/api_misc.py b/qubes/tests/api_misc.py index fafeb4ecf..c3d29a2b0 100644 --- a/qubes/tests/api_misc.py +++ b/qubes/tests/api_misc.py @@ -39,6 +39,7 @@ def setUp(self): self.app = mock.NonCallableMock() self.dest = mock.NonCallableMock() self.dest.name = "dom0" + self.dest.klass = "AdminVM" self.app.configure_mock( domains={ "dom0": self.dest, @@ -117,7 +118,7 @@ def test_002_features_request_invalid1(self): "/features-request/feature1": b"test$chars", } self.configure_qdb(qdb_entries) - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(b"qubes.FeaturesRequest") self.assertEqual(self.app.mock_calls, []) self.assertEqual( @@ -217,7 +218,7 @@ def test_015_notify_tools_invalid_value_qrexec(self): "/qubes-tools/default-user": b"user", } self.configure_qdb(qdb_entries) - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(b"qubes.NotifyTools") self.assertEqual(self.app.mock_calls, []) self.assertEqual( @@ -236,7 +237,7 @@ def test_016_notify_tools_invalid_value_gui(self): "/qubes-tools/default-user": b"user", } self.configure_qdb(qdb_entries) - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(b"qubes.NotifyTools") self.assertEqual(self.app.mock_calls, []) self.assertEqual( @@ -296,7 +297,7 @@ def test_021_notify_updates_standalone2(self): def test_022_notify_updates_invalid(self): del self.src.template - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(b"qubes.NotifyUpdates", payload=b"") self.assertEqual(self.src.mock_calls, []) self.assertEqual(self.tpl.mock_calls, []) @@ -304,7 +305,7 @@ def test_022_notify_updates_invalid(self): def test_023_notify_updates_invalid2(self): del self.src.template - with self.assertRaises(qubes.exc.PermissionDenied): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(b"qubes.NotifyUpdates", payload=b"no updates") self.assertEqual(self.src.mock_calls, []) self.assertEqual(self.tpl.mock_calls, []) diff --git a/qubes/tests/vm/dispvm.py b/qubes/tests/vm/dispvm.py index 393dc16b9..138f9d7d0 100644 --- a/qubes/tests/vm/dispvm.py +++ b/qubes/tests/vm/dispvm.py @@ -467,7 +467,7 @@ def test_011_create_direct_reject(self): "__getitem__.side_effect": orig_getitem, } ) - with self.assertRaises(qubes.exc.QubesException): + with self.assertRaises(qubes.exc.QubesValueError): self.app.add_new_vm( qubes.vm.dispvm.DispVM, name="test-dispvm", diff --git a/qubes/tests/vm/qubesvm.py b/qubes/tests/vm/qubesvm.py index 492621639..4900f4ba7 100644 --- a/qubes/tests/vm/qubesvm.py +++ b/qubes/tests/vm/qubesvm.py @@ -64,6 +64,15 @@ def __init__(self): self.host = unittest.mock.Mock() self.host.memory_total = 4096 * 1024 + def get_label(self, label): + # pylint: disable=unused-argument + if label in self.labels: + return self.labels[label] + for l in self.labels.values(): + if l.name == label: + return l + raise qubes.exc.QubesLabelNotFoundError(label) + class TestFeatures(dict): def __init__(self, vm, **kwargs) -> None: diff --git a/qubes/utils.py b/qubes/utils.py index 49e0d0e89..545f11ba5 100644 --- a/qubes/utils.py +++ b/qubes/utils.py @@ -510,3 +510,43 @@ def is_pci_path(device_id: str): r"(-[0-9a-f]{2}_[0-9a-f]{2}\.[0-9a-f])*\Z" ) return bool(path_re.match(device_id)) + + +def validate_label_name(untrusted_label, creation: bool = False) -> None: + # don't confuse label name with label index + if creation and str(untrusted_label).isdigit(): + raise qubes.exc.QubesInvalidLabelError("Label must not be a digit") + + allowed_chars = string.ascii_letters + string.digits + "-_." + if not all(c in allowed_chars for c in untrusted_label): + raise qubes.exc.QubesInvalidLabelError( + "Label must be in safe set: " + allowed_chars + ) + + +def validate_label_value(untrusted_label_value) -> None: + try: + untrusted_value = untrusted_label_value.decode( + "ascii", "strict" + ).strip() + except UnicodeDecodeError: + raise qubes.exc.QubesInvalidLabelValueError( + "Label value must be encoded in ASCII" + ) + + if len(untrusted_value) != 8: + raise qubes.exc.QubesInvalidLabelValueError( + "Label value must have length of 8" + ) + + if not untrusted_value.startswith("0x"): + raise qubes.exc.QubesInvalidLabelValueError( + "Label value must start with: 0x" + ) + + # besides prefix, only hex digits are allowed + if not all(x in string.hexdigits for x in untrusted_value[2:]): + raise qubes.exc.QubesInvalidLabelValueError( + "Label value must only contain hexadecimal digits after prefix: " + + string.hexdigits + ) diff --git a/qubes/vm/__init__.py b/qubes/vm/__init__.py index de9431dc7..63d215f07 100644 --- a/qubes/vm/__init__.py +++ b/qubes/vm/__init__.py @@ -100,8 +100,7 @@ def setter_label(self, prop, value): if isinstance(value, qubes.Label): return value if isinstance(value, str) and value.startswith("label-"): - return self.app.labels[int(value.split("-", 1)[1])] - + value = int(value.split("-", 1)[1]) return self.app.get_label(value) From 4eb433c1e908e4ba717351d078defdabf306aff8 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Thu, 12 Mar 2026 11:57:39 +0100 Subject: [PATCH 02/11] Prohibit empty tags and labels --- qubes/api/admin.py | 8 +++++++- qubes/exc.py | 4 ++++ qubes/tests/api_admin.py | 34 +++++++++++++++++++++++++++++++--- qubes/utils.py | 6 ++++++ qubes/vm/__init__.py | 4 +++- 5 files changed, 51 insertions(+), 5 deletions(-) diff --git a/qubes/api/admin.py b/qubes/api/admin.py index 0c001bc58..db7930469 100644 --- a/qubes/api/admin.py +++ b/qubes/api/admin.py @@ -1502,7 +1502,13 @@ async def vm_feature_remove(self): write=True, ) async def vm_feature_set(self, untrusted_payload): - untrusted_value = untrusted_payload.decode("ascii", errors="strict") + try: + untrusted_value = untrusted_payload.decode("ascii", errors="strict") + except UnicodeDecodeError: + raise qubes.exc.ProtocolError( + "Feature value contains non-ASCII characters" + ) + del untrusted_payload if re.match(r"\A[a-zA-Z0-9_.-]+\Z", self.arg) is None: raise qubes.exc.ProtocolError( diff --git a/qubes/exc.py b/qubes/exc.py index f3e20994a..93df9e5c8 100644 --- a/qubes/exc.py +++ b/qubes/exc.py @@ -390,6 +390,10 @@ class QubesVolumeNotFoundError(KeyError): """Pool does not exist.""" +class QubesInvalidTagError(ProtocolError): + """Domain tag is invalid.""" + + class QubesInvalidLabelError(ProtocolError): """Domain label is invalid.""" diff --git a/qubes/tests/api_admin.py b/qubes/tests/api_admin.py index 8816f4bbe..d9a65f9d3 100644 --- a/qubes/tests/api_admin.py +++ b/qubes/tests/api_admin.py @@ -1293,6 +1293,11 @@ def test_190_label_get(self): ) self.assertFalse(self.app.save.called) + def test_191_label_get_invalid(self): + with self.assertRaises(qubes.exc.QubesInvalidLabelError): + self.call_mgmt_func(b"admin.label.Get", b"dom0", b" ") + self.assertFalse(self.app.save.called) + def test_195_label_index(self): self.app.get_label = unittest.mock.Mock() self.app.get_label.configure_mock(**{"return_value.index": 1}) @@ -1922,8 +1927,8 @@ def test_321_feature_set_empty(self): self.assertEqual(self.vm.features["test-feature"], "") self.assertTrue(self.app.save.called) - def test_322_feature_set_invalid(self): - with self.assertRaises(UnicodeDecodeError): + def test_322_feature_set_invalid_value(self): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func( b"admin.vm.feature.Set", b"test-vm1", @@ -1933,6 +1938,18 @@ def test_322_feature_set_invalid(self): self.assertNotIn("test-feature", self.vm.features) self.assertFalse(self.app.save.called) + with self.assertRaisesRegex( + qubes.exc.ProtocolError, "non-ASCII characters" + ): + self.call_mgmt_func( + b"admin.vm.feature.Set", + b"test-vm1", + b"test-feature", + "\u00f6".encode(), + ) + self.assertNotIn("test-feature", self.vm.features) + self.assertFalse(self.app.save.called) + def test_322_feature_set_invalid_key(self): with self.assertRaisesRegex( qubes.exc.ProtocolError, "expected an argument" @@ -3605,11 +3622,22 @@ def test_560_tag_set(self): self.assertTrue(self.app.save.called) def test_561_tag_set_invalid(self): - with self.assertRaises(ValueError): + with self.assertRaises(qubes.exc.ProtocolError): self.call_mgmt_func(b"admin.vm.tag.Set", b"test-vm1", b"+.some-tag") self.assertNotIn("+.some-tag", self.vm.tags) self.assertFalse(self.app.save.called) + def test_562_tag_set_invalid_empty(self): + with self.assertRaises(qubes.exc.ProtocolError): + self.call_mgmt_func(b"admin.vm.tag.Set", b"test-vm1", b"") + self.assertNotIn("", self.vm.tags) + self.assertFalse(self.app.save.called) + + with self.assertRaises(qubes.exc.QubesInvalidTagError): + self.call_mgmt_func(b"admin.vm.tag.Set", b"test-vm1", b"+") + self.assertNotIn("", self.vm.tags) + self.assertFalse(self.app.save.called) + def test_570_firewall_get(self): self.vm.firewall.save = unittest.mock.Mock() value = self.call_mgmt_func(b"admin.vm.firewall.Get", b"test-vm1", b"") diff --git a/qubes/utils.py b/qubes/utils.py index 545f11ba5..5a324f1df 100644 --- a/qubes/utils.py +++ b/qubes/utils.py @@ -513,6 +513,9 @@ def is_pci_path(device_id: str): def validate_label_name(untrusted_label, creation: bool = False) -> None: + if not untrusted_label: + raise qubes.exc.QubesInvalidLabelError("Label name must be set") + # don't confuse label name with label index if creation and str(untrusted_label).isdigit(): raise qubes.exc.QubesInvalidLabelError("Label must not be a digit") @@ -525,6 +528,9 @@ def validate_label_name(untrusted_label, creation: bool = False) -> None: def validate_label_value(untrusted_label_value) -> None: + if not untrusted_label_value: + raise qubes.exc.QubesInvalidLabelValueError("Label value must be set") + try: untrusted_value = untrusted_label_value.decode( "ascii", "strict" diff --git a/qubes/vm/__init__.py b/qubes/vm/__init__.py index 63d215f07..adfa97929 100644 --- a/qubes/vm/__init__.py +++ b/qubes/vm/__init__.py @@ -194,9 +194,11 @@ def remove(self, elem): @staticmethod def validate_tag(tag): + if not tag: + raise qubes.exc.QubesInvalidTagError("tag cannot be empty") safe_set = string.ascii_letters + string.digits + "-_" if not all((x in safe_set) for x in tag): - raise ValueError("disallowed characters") + raise qubes.exc.QubesInvalidTagError("disallowed characters") class BaseVM(qubes.PropertyHolder): From 0124bd1be741ddd6ce8757ffad1b31cb0ffd2dab Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Fri, 23 Jan 2026 09:10:08 +0100 Subject: [PATCH 03/11] Clean return logic on transport success or failure --- qubes/api/__init__.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/qubes/api/__init__.py b/qubes/api/__init__.py index 04bf7d00c..9e73959b9 100644 --- a/qubes/api/__init__.py +++ b/qubes/api/__init__.py @@ -423,6 +423,7 @@ async def respond(self, src, meth, dest, arg, *, untrusted_payload): exc_fmt = ( "failed call %r+%r (%r → %r) with payload of %d bytes due to %r" ) + success = False try: self.mgmt = self.handler( self.app, src, meth, dest, arg, self.send_event @@ -483,13 +484,12 @@ async def respond(self, src, meth, dest, arg, *, untrusted_payload): self.transport.write_eof() except NotImplementedError: pass - self.transport.close() - return - - # this is reached if from except: blocks; do not put it in finally:, - # because this will prevent the good case from sending the reply - if self.transport: - self.transport.abort() + success = True + finally: + if success: + self.transport.close() + elif self.transport: + self.transport.abort() def send_header(self, *args): self.transport.write(self.header.pack(*args)) From b5bdaba59d80fb3bfcd83ce975eb7c2b091494d0 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Fri, 23 Jan 2026 16:31:18 +0100 Subject: [PATCH 04/11] Skip logging unsanitized value --- qubes/api/admin.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/qubes/api/admin.py b/qubes/api/admin.py index db7930469..cf3006119 100644 --- a/qubes/api/admin.py +++ b/qubes/api/admin.py @@ -1007,11 +1007,8 @@ async def pool_add(self, untrusted_payload): unexpected_parameters = [ key for key in untrusted_pool_config if key not in driver_parameters ] - # TODO: ben: dangerous logging any ASCII value? if unexpected_parameters: - raise qubes.exc.ProtocolError( - "Unexpected driver options: " + " ".join(unexpected_parameters) - ) + raise qubes.exc.ProtocolError("Unexpected driver options found") required_parameters_unmet = [ key for key in dp_names From c04d5130fbe07e61e08679b5e3050db4c3a3b2cc Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Thu, 22 Jan 2026 16:17:30 +0100 Subject: [PATCH 05/11] Log and return prohibitions and client errors Now the client is able to understand what failed as well as relevant information being logged to dom0. - Only log safe data to dom0, to avoid compromise when reading from an application that interprets escape sequences; - Only return required data to the client that is relevant to the call, to avoid leaking more information than necessary. Fixes: https://github.com/QubesOS/qubes-issues/issues/10345 --- qubes/api/__init__.py | 9 +++++---- qubes/exc.py | 19 +++++-------------- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/qubes/api/__init__.py b/qubes/api/__init__.py index 9e73959b9..f81397e9f 100644 --- a/qubes/api/__init__.py +++ b/qubes/api/__init__.py @@ -435,9 +435,7 @@ async def respond(self, src, meth, dest, arg, *, untrusted_payload): if self.transport is None: return - except (PermissionDenied, ProtocolError) as untrusted_exc: - exc_name = untrusted_exc.__class__.__name__ - del untrusted_exc + except (PermissionDenied, ProtocolError) as exc: self.app.log.warning( exc_fmt, meth, @@ -445,8 +443,11 @@ async def respond(self, src, meth, dest, arg, *, untrusted_payload): src, dest, len(untrusted_payload), - exc_name, + exc, ) + if self.transport is not None: + self.send_exception(exc) + self.transport.write_eof() except qubes.exc.QubesException as exc: if self.debug: diff --git a/qubes/exc.py b/qubes/exc.py index 93df9e5c8..f35c3b8ea 100644 --- a/qubes/exc.py +++ b/qubes/exc.py @@ -36,11 +36,8 @@ class ProtocolError(AssertionError): way that a correctly behaving client can get ProtocolError is qubesd is either buggy or too old. In HTTP, this would be 400 Bad Request. - This does not provide any useful information to the client making the - request. Therefore, it should only be raised if there is a client - *programming* error, such as passing an argument to a request that does not - take an argument. It should not be used to reject requests that are valid, - but which qubesd is refusing to process. Instead, raise a subclass of + It should not be used to reject requests that are valid, but which qubesd + is refusing to process. Instead, raise a subclass of :py:class:`QubesException` with a useful error message. """ @@ -49,15 +46,9 @@ class PermissionDenied(PermissionError): """Raised deliberately by handlers to inform the request is prohibited. The request is valid, but the client does not have permission to perform - the operation. Clients in dom0 should usually not get this error. In HTTP, - this would be 403 Forbidden. - - This does not provide any useful information to the client making the - request. Therefore, it should only be raised if there is a client - *programming* error, such as passing an argument to a request that does not - take an argument. It should not be used to reject requests that are valid, - but which qubesd is refusing to process. Instead, raise a subclass of - :py:class:`QubesException` with a useful error message. + the operation. Clients in dom0 should usually not get this error. It must + only be raised by "admin-permission" events. In HTTP, this would be 403 + Forbidden. """ From ef21987f2e2a14481a3a5b6e10690d9c2df363fb Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Tue, 27 Jan 2026 12:01:36 +0100 Subject: [PATCH 06/11] Do not raise absent object before admin-permission If a required object is absent prior to admin-permission, it allows knowing if object exists before the admin extension can potentially hide it with PermissionDenied. - The comment that there was a leak on CloneTo and CloneFrom is not applicable, the token is already set to a volume, it is just checking if things are alright, volume pool still exists etc; - The existence leak is sometimes not possible to solved in the admin/api.py, because it would limit the sanitization made before the fire_event_for_permission(); - When the leak was avoided without compromising sanitization, it makes the logic a bit strange, some validation being done past fire_event_for_permission(). --- qubes/api/admin.py | 87 +++++++++++++++++++++++++--------------- qubes/tests/api_admin.py | 46 ++++++++++++++++----- 2 files changed, 91 insertions(+), 42 deletions(-) diff --git a/qubes/api/admin.py b/qubes/api/admin.py index cf3006119..f92d281ec 100644 --- a/qubes/api/admin.py +++ b/qubes/api/admin.py @@ -536,15 +536,22 @@ async def vm_volume_revert(self, untrusted_payload): ) untrusted_revision = untrusted_payload.decode("ascii").strip() del untrusted_payload + allowed_chars = string.ascii_letters + string.digits + ":-_." + self.enforce( + all(c in allowed_chars for c in untrusted_revision), + reason="Revision name must be in safe set: " + allowed_chars, + ) + revision = untrusted_revision + del untrusted_revision volume = self.dest.volumes[self.arg] + + self.fire_event_for_permission(volume=volume, revision=revision) + snapshots = volume.revisions - # TODO: ben: info-leak: revision existence - if untrusted_revision not in snapshots: + if revision not in snapshots: raise qubes.exc.QubesVolumeRevisionNotFoundError() - revision = untrusted_revision - self.fire_event_for_permission(volume=volume, revision=revision) await qubes.utils.coro_maybe(volume.revert(revision)) self.app.save() @@ -613,10 +620,8 @@ async def vm_volume_clone_to(self, untrusted_payload): del self.app.api_admin_pending_clone[token] # make sure the volume still exists, but invalidate token anyway - # TODO: ben: info-leak: pool existence if str(src_volume.pool) not in self.app.pools: raise qubes.exc.QubesPoolNotFoundError() - # TODO: ben: info-leak: volume existence if src_volume not in self.app.pools[str(src_volume.pool)].volumes: raise qubes.exc.QubesVolumeNotFoundError() @@ -956,7 +961,6 @@ async def pool_usage(self): write=True, ) async def pool_add(self, untrusted_payload): - # TODO: ben: info-leak: pool existence self.enforce_arg( wants=qubes.storage.pool_drivers(), short_reason="available drivers" ) @@ -998,9 +1002,9 @@ async def pool_add(self, untrusted_payload): reason="Pool name must be in safe set: " + allowed_chars, ) pool_name = untrusted_pool_name - # TODO: ben: info-leak: pool existence + invalid_pool = False if pool_name in self.app.pools: - raise qubes.exc.QubesPoolInUseError(pool_name=pool_name) + invalid_pool = True driver_parameters = qubes.storage.driver_parameters(self.arg) dp_names = driver_parameters.keys() @@ -1024,6 +1028,9 @@ async def pool_add(self, untrusted_payload): self.fire_event_for_permission(name=pool_name, pool_config=pool_config) + if invalid_pool: + raise qubes.exc.QubesPoolInUseError(pool_name=pool_name) + await self.app.add_pool(name=pool_name, driver=self.arg, **pool_config) self.app.save() @@ -1129,7 +1136,6 @@ async def label_list(self): async def label_get(self): qubes.utils.validate_label_name(untrusted_label=self.arg) - # TODO: ben: info-leak: label existence label = self.app.get_label(self.arg) self.fire_event_for_permission(label=label) @@ -1147,7 +1153,6 @@ async def label_get(self): async def label_index(self): qubes.utils.validate_label_name(untrusted_label=self.arg) - # TODO: ben: info-leak: label existence label = self.app.get_label(self.arg) self.fire_event_for_permission(label=label) @@ -1165,15 +1170,6 @@ async def label_index(self): async def label_create(self, untrusted_payload): qubes.utils.validate_label_name(untrusted_label=self.arg, creation=True) - # TODO: ben: info-leak: label existence - try: - self.app.get_label(self.arg) - except KeyError: - # ok, no such label yet - pass - else: - raise qubes.exc.QubesLabelInUseError(label=self.arg) - qubes.utils.validate_label_value( untrusted_label_value=untrusted_payload ) @@ -1184,6 +1180,14 @@ async def label_create(self, untrusted_payload): self.fire_event_for_permission(color=color) + try: + self.app.get_label(self.arg) + except KeyError: + # ok, no such label yet + pass + else: + raise qubes.exc.QubesLabelInUseError(label=self.arg) + # allocate new index, but make sure it's outside of default labels set new_index = ( max(qubes.config.max_default_label, *self.app.labels.keys()) + 1 @@ -1204,15 +1208,15 @@ async def label_create(self, untrusted_payload): async def label_remove(self): qubes.utils.validate_label_name(untrusted_label=self.arg) - # TODO: ben: info-leak: label existence label = self.app.get_label(self.arg) + + self.fire_event_for_permission(label=label) + self.enforce( label.index > qubes.config.max_default_label, reason="Can only remove custom labels", ) - self.fire_event_for_permission(label=label) - # FIXME: this should be in app.add_label() for vm in self.app.domains: if vm.label == label: @@ -1574,17 +1578,18 @@ async def _vm_create( # if argument is given, it needs to be a valid template, and only # when given VM class do need a template + invalid_template = False if self.arg: - if hasattr(vm_class, "template"): - # TODO: ben: info-leak: template existence - if self.arg not in self.app.domains: - raise qubes.exc.QubesVMNotFoundError(self.arg) - kwargs["template"] = self.app.domains[self.arg] - else: + if not hasattr(vm_class, "template"): raise qubes.exc.ProtocolError( "{} cannot be based on template".format(vm_type) ) + if self.arg in self.app.domains: + kwargs["template"] = self.app.domains[self.arg] + else: + invalid_template = True + invalid_label = False for untrusted_param in untrusted_payload.decode( "ascii", errors="strict" ).split(" "): @@ -1601,8 +1606,10 @@ async def _vm_create( qubes.utils.validate_label_name( untrusted_label=untrusted_value, creation=True ) - # TODO: ben: info-leak: label existence - kwargs["label"] = self.app.get_label(untrusted_value) + try: + kwargs["label"] = self.app.get_label(untrusted_value) + except qubes.exc.QubesLabelNotFoundError: + invalid_label = True elif untrusted_key == "pool" and allow_pool: self.enforce( @@ -1651,13 +1658,27 @@ async def _vm_create( reason="Conflicting options specified: 'pool=', 'pool:volume='", ) - # TODO: ben: info-leak: qube existence + invalid_name = False if kwargs["name"] in self.app.domains: + invalid_name = True + + self.fire_event_for_permission(pool=pool, pools=pools, **kwargs) + + # Avoids leaking qube existence before admin-permission. + if invalid_name: raise qubes.exc.QubesVMAlreadyExistsError( "Qube already exists: {}".format(kwargs["name"]) ) - self.fire_event_for_permission(pool=pool, pools=pools, **kwargs) + # Avoids leaking template existence before admin-permission. + if invalid_template: + raise qubes.exc.QubesVMNotFoundError(self.arg) + + # Avoids leaking label existence before admin-permission. + if invalid_label: + # If label is invalid, it came from payload and should not be + # logged. + raise qubes.exc.QubesLabelNotFoundError(label="untrusted label") vm = self.app.add_new_vm(vm_class, **kwargs) # TODO: move this to extension (in race-free fashion) diff --git a/qubes/tests/api_admin.py b/qubes/tests/api_admin.py index d9a65f9d3..38db1408a 100644 --- a/qubes/tests/api_admin.py +++ b/qubes/tests/api_admin.py @@ -790,13 +790,6 @@ def test_110_vm_volume_revert_invalid_rev(self): b"private", b"no-such-rev", ) - self.assertEqual( - self.vm.volumes.mock_calls, - [ - unittest.mock.call.keys(), - unittest.mock.call.__getattr__("__getitem__")("private"), - ], - ) self.assertFalse(self.vm.storage.called) def test_120_vm_volume_resize(self): @@ -1215,7 +1208,6 @@ def test_160_pool_add_existing_pool(self, mock_parameters, mock_drivers): b"name=file\nparam1=value\nparam2=some-value\n", ) self.assertEqual(mock_drivers.mock_calls, [unittest.mock.call()]) - self.assertEqual(mock_parameters.mock_calls, []) self.assertEqual(add_pool_mock.mock_calls, []) self.assertFalse(self.app.save.called) @@ -1401,7 +1393,9 @@ def test_200_label_create_invalid_name(self): def test_200_label_create_already_exists(self): self.app.get_label = unittest.mock.Mock(wraps=self.app.get_label) with self.assertRaises(qubes.exc.QubesLabelInUseError): - self.call_mgmt_func(b"admin.label.Create", b"dom0", b"red", b"abcd") + self.call_mgmt_func( + b"admin.label.Create", b"dom0", b"red", b"0xff0000" + ) self.assertEqual( self.app.get_label.mock_calls, [unittest.mock.call("red")] ) @@ -2079,6 +2073,26 @@ def test_332_vm_create_app(self, storage_mock): self.assertTrue(self.app.save.called) + @unittest.mock.patch("qubes.storage.Storage.create") + def test_333_vm_create_appvm_invalid_template(self, storage_mock): + storage_mock.side_effect = self.dummy_coro + with self.assertRaises(qubes.exc.QubesVMNotFoundError): + self.call_mgmt_func( + b"admin.vm.Create.AppVM", + b"dom0", + b"test-template-inexistent", + b"name=test-vm2 label=red", + ) + self.assertNotIn("test-vm2", self.app.domains) + self.assertEqual(storage_mock.mock_calls, []) + self.assertFalse( + os.path.exists( + os.path.join(self.test_base_dir, "appvms", "test-vm2") + ) + ) + self.assertNotIn("test-vm2", self.app.domains) + self.assertFalse(self.app.save.called) + @unittest.mock.patch("qubes.storage.Storage.create") def test_333_vm_create_app_default_template(self, storage_mock): storage_mock.side_effect = self.dummy_coro @@ -2186,6 +2200,20 @@ def test_338_vm_create_name_twice(self, storage_mock): self.assertNotIn("test-vm3", self.app.domains) self.assertFalse(self.app.save.called) + @unittest.mock.patch("qubes.storage.Storage.create") + def test_339_vm_create_spurious_params(self, storage_mock): + storage_mock.side_effect = self.dummy_coro + with self.assertRaisesRegex(qubes.exc.ProtocolError, "does not match"): + self.call_mgmt_func( + b"admin.vm.Create.AppVM", + b"dom0", + b"test-template", + b"name=test-vm2 label=red what=is", + ) + self.assertNotIn("test-vm2", self.app.domains) + self.assertNotIn("test-vm3", self.app.domains) + self.assertFalse(self.app.save.called) + @unittest.mock.patch("qubes.storage.Storage.create") def test_340_vm_create_in_pool_app(self, storage_mock): storage_mock.side_effect = self.dummy_coro From 48fdb45ddb3216dbdf121c53682b308b4cae1eee Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Tue, 17 Feb 2026 09:22:36 +0100 Subject: [PATCH 07/11] Fix assignment mode docstring --- qubes/api/admin.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/qubes/api/admin.py b/qubes/api/admin.py index f92d281ec..f95b9f607 100644 --- a/qubes/api/admin.py +++ b/qubes/api/admin.py @@ -2077,8 +2077,9 @@ async def vm_device_set_required(self, endpoint, untrusted_payload): Update `required` flag of an already assigned device. Payload: - `False` -> device will be auto-attached to qube - `True` -> device is required to start qube + `required` -> device is required to start qube + `ask-to-attach` -> TODO + `auto-attack` -> device will be auto-attached to qube """ devclass = endpoint From 0653fbb94f07a041126f60e0d470207be542c083 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Tue, 17 Feb 2026 09:30:52 +0100 Subject: [PATCH 08/11] Add custom exception to assignment mode and backup For: https://github.com/QubesOS/qubes-core-admin/pull/645 --- qubes/api/admin.py | 6 ++++-- qubes/exc.py | 11 +++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/qubes/api/admin.py b/qubes/api/admin.py index f95b9f607..fc0cb6add 100644 --- a/qubes/api/admin.py +++ b/qubes/api/admin.py @@ -2091,7 +2091,9 @@ async def vm_device_set_required(self, endpoint, untrusted_payload): try: mode = allowed_values[untrusted_payload] except KeyError: - raise qubes.exc.ProtocolError("Invalid assignment mode") + raise qubes.exc.QubesUnrecognizedDeviceAssignmentMode( + "Invalid assignment mode" + ) dev = VirtualDevice.from_qarg(self.arg, devclass, self.app.domains) @@ -2412,7 +2414,7 @@ async def backup_execute(self): try: await backup_task except asyncio.CancelledError: - raise qubes.exc.QubesException("Backup cancelled") + raise qubes.exc.BackupCancelledError() finally: del self.app.api_admin_running_backups[self.arg] diff --git a/qubes/exc.py b/qubes/exc.py index f35c3b8ea..c05346fc3 100644 --- a/qubes/exc.py +++ b/qubes/exc.py @@ -244,6 +244,11 @@ def __init__(self, msg=None): class QubesBackupProfileNotFoundError(QubesException): """Requested backup profile does not exist.""" + def __init__(self, msg=None, profile=None): + super().__init__( + msg or "Backup profile {!r} does not exist".format(profile) + ) + class BackupCancelledError(QubesException): """Thrown at user when backup was manually cancelled""" @@ -333,6 +338,12 @@ class DeviceAlreadyAssigned(QubesException, KeyError): """ +class QubesUnrecognizedDeviceAssignmentMode(ProtocolError): + """ + Device assignment is not as expected. + """ + + class UnrecognizedDevice(QubesException, ValueError): """ Device identity is not as expected. From 807a9a3074f388c10adda206fc1cb3406efad138 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Thu, 26 Feb 2026 17:53:20 +0100 Subject: [PATCH 09/11] Optionally log API calls made by dom0 Developers normally edit the code to log the calls or use a management domain such as a GUIVM to see logged calls. Add a command-line option and environment variable to enable logging dom0 calls also. Fixes: https://github.com/qubesos/qubes-issues/issues/10689 --- qubes/api/__init__.py | 15 ++++++++++++++- qubes/tools/qubesd.py | 8 ++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/qubes/api/__init__.py b/qubes/api/__init__.py index f81397e9f..491bd2f9b 100644 --- a/qubes/api/__init__.py +++ b/qubes/api/__init__.py @@ -354,7 +354,9 @@ class QubesDaemonProtocol(asyncio.Protocol): buffer_size = 65536 header = struct.Struct("Bx") - def __init__(self, handler, *args, app, debug=False, **kwargs): + def __init__( + self, handler, *args, app, debug=False, log_dom0_call=False, **kwargs + ): super().__init__(*args, **kwargs) self.handler = handler self.app = app @@ -362,6 +364,7 @@ def __init__(self, handler, *args, app, debug=False, **kwargs): self.len_untrusted_buffer = 0 self.transport = None self.debug = debug + self.log_dom0_call = log_dom0_call self.event_sent = False self.mgmt = None @@ -420,6 +423,16 @@ def eof_received(self): return True async def respond(self, src, meth, dest, arg, *, untrusted_payload): + if self.log_dom0_call: + call_fmt = "received call %r+%r (%r → %r) with payload of %d bytes" + self.app.log.debug( + call_fmt, + meth, + arg, + src, + dest, + len(untrusted_payload), + ) exc_fmt = ( "failed call %r+%r (%r → %r) with payload of %d bytes due to %r" ) diff --git a/qubes/tools/qubesd.py b/qubes/tools/qubesd.py index d0ff7f5df..856e5744c 100644 --- a/qubes/tools/qubesd.py +++ b/qubes/tools/qubesd.py @@ -38,6 +38,12 @@ def sighandler(loop, signame, servers, app): help="Enable verbose error logging (all exceptions with full " "tracebacks) and also send tracebacks to Admin API clients", ) +parser.add_argument( + "--log-dom0-call", + action="store_true", + default=False, + help="Enable logging API calls made by dom0", +) def main(args=None): @@ -64,6 +70,8 @@ def main(args=None): qubes.api.misc.QubesMiscAPI, app=args.app, debug=args.debug, + log_dom0_call=args.log_dom0_call + or os.environ.get("QUBES_LOG_DOM0_CALL"), ) ) From 83fa458bc232a3f574d529f12874864cc529f788 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Mon, 9 Mar 2026 17:42:03 +0100 Subject: [PATCH 10/11] Disable libvirtaio debug log and add allow revert The root level and handlers can have the log level debug, but libvirt is too verbose for that. Disabling it makes the output easier to read. Especially considering the previous commit that allows logging dom0 qrexec calls, when in debug mode. In the rare cases it's needed, there is an option to enable it. --- qubes/log.py | 4 +++- qubes/tools/__init__.py | 9 +++++++++ qubes/tools/qubesd.py | 9 ++++++--- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/qubes/log.py b/qubes/log.py index 8a2c8f97b..d3795af25 100644 --- a/qubes/log.py +++ b/qubes/log.py @@ -43,7 +43,7 @@ def formatMessage(self, record): return fmt % record.__dict__ -def enable(log_level: int = logging.INFO): +def enable(log_level: int = logging.INFO, enable_debug_libvirt: bool = False): """Enable global logging Use :py:mod:`logging` module from standard library to log messages. @@ -64,6 +64,8 @@ def enable(log_level: int = logging.INFO): if log_level == logging.DEBUG: for handler in logging.root.handlers: handler.setFormatter(Formatter(debug=True)) + if enable_debug_libvirt: + logging.getLogger("virEventAsyncIOImpl").setLevel(logging.INFO) logging.root.setLevel(log_level) diff --git a/qubes/tools/__init__.py b/qubes/tools/__init__.py index 8c13c8955..56c83da56 100644 --- a/qubes/tools/__init__.py +++ b/qubes/tools/__init__.py @@ -475,6 +475,15 @@ def set_qubes_verbosity(namespace): This is done by configuring global logging. :param argparse.Namespace args: args as parsed by parser """ + if namespace.debug: + qubes.log.enable( + log_level=logging.DEBUG, + enable_debug_libvirt=getattr( + namespace, "enable_debug_libvirt", False + ), + ) + return + verbose = namespace.verbose - namespace.quiet levels = [logging.NOTSET, logging.INFO, logging.DEBUG] log_level = levels[min(verbose, 2)] diff --git a/qubes/tools/qubesd.py b/qubes/tools/qubesd.py index 856e5744c..8ff3d5c09 100644 --- a/qubes/tools/qubesd.py +++ b/qubes/tools/qubesd.py @@ -38,6 +38,12 @@ def sighandler(loop, signame, servers, app): help="Enable verbose error logging (all exceptions with full " "tracebacks) and also send tracebacks to Admin API clients", ) +parser.add_argument( + "--enable-debug-libvirt", + action="store_true", + default=False, + help="Enables verbose logging of libvirtaio virEventAsyncIOImpl handler", +) parser.add_argument( "--log-dom0-call", action="store_true", @@ -60,9 +66,6 @@ def main(args=None): # Stop storage for domains not currently running loop.run_until_complete(args.app.stop_storage()) - if args.debug: - qubes.log.enable(log_level=10) - servers = loop.run_until_complete( qubes.api.create_servers( qubes.api.admin.QubesAdminAPI, From deeff5d90ff9fe1167651aa5efe1364bd5a2d1f2 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Fri, 13 Mar 2026 17:36:35 +0100 Subject: [PATCH 11/11] Document the API --- doc/qubes-api.rst | 140 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 140 insertions(+) create mode 100644 doc/qubes-api.rst diff --git a/doc/qubes-api.rst b/doc/qubes-api.rst new file mode 100644 index 000000000..a121cceb6 --- /dev/null +++ b/doc/qubes-api.rst @@ -0,0 +1,140 @@ +:py:mod:`qubes.api` -- API +================================= + +API sanitization is very important to maintain the AdminVM and everything it +controls, secure. + +**Simple rules**: + +* Handle exceptions gracefully. Unhandled exceptions have the traceback only in + the AdminVM, not allowing clients to know what happened. More information + about exceptions can be found at :py:class:`qubes.exc`. +* Failure to serve a request must raise exceptions that can be understood by the + client. Each stage below allows a different class of exception to be shown. + Be careful to not reveal more information than what the client is allowed to + know at each stage. + +**Stages**: + +.# Sanitize data + + * Must never print, log or store untrusted data. Be careful when throwing an + exception. + * Qrexec sanitizes the argument, but if you will use the argument to + something that you know has an even stricter syntax, it must be sanitized + also. + * The payload is received raw, in bytes. It must always be sanitized before + being passed to functions that expect it to be already trusted. + * When sanitizing data, if it may reveal information from the system, such as + object existence, then, throw :py:class:`qubes.exc.PermissionDenied` to + avoid leaking object existence or delay reveal till after + `admin-permission`. + +.# Fire permission event + + * Must only pass sanitized information. + * The most commonly used is + :py:meth:`qubes.api.AbstractQubesAPI.fire_event_for_permission`, which + fires event `admin-permission` directly, used by + :py:class:`qubes.api.admin.AdminExtension`. For more complex cases, + involving global information such as fetching objects from different + destinations, :py:meth:`qubes.api.AbstractQubesAPI.fire_event_for_filter` + is more appropriate, as it fires `admin-permission` for each operation + required. + +.# Action + + * The client is fully authorized at this stage, it passed Qrexec policy + evaluation and Qubesd `admin-permission`. The server may be aware that some + resource could not be served before the `admin-permisison`, but it was not + allowed to reveal at that stage, now it can reveal what and why it failed. + A custom exception derived from :py:class:`qubes.exc.QubesException` can be + used to allow the client to handle it gracefully. + * Act. + +.. code-block:: python + + @qubes.api.method( + "dest.feat.Set", # RPC name + wants_arg=True, # Argument must be provided + wants_payload=None, # Payload can be provided + dest_adminvm=False, # Target must not be AdminVM + scope="global", # Applies to the whole system + read=True, # Will read system information + write=True, # Will write information to the system + ) + async def dest_feat_set(self, untrusted_payload): + """ + Set destination feature + + name: self.arg + value: untrusted_payload + """ + # Qrexec sanitizes self.arg, but our feature name can only be made of + # letters. The client should have know to not make such a request in the + # first case, therefore it throws qubes.exc.ProtocolError. + allowed_chars = string.ascii_letters + self.enforce( + all(c in allowed_chars for c in self.arg), + reason="Feature name must be in safe set: " + allowed_chars, + ) + + # Payload is in bytes and we receive it without being sanitized + # previously. We only want to allow values to be in ASCII. + try: + untrusted_value = untrusted_payload.decode("ascii", errors="strict") + except UnicodeDecodeError: + raise qubes.exc.ProtocolError("Value contains non-ASCII characters") + # Delete untrusted payload prevent using it. + del untrusted_payload + + # Second sanitization of the value + if re.match(r"\A[\x20-\x7E]*\Z", untrusted_value) is None: + raise qubes.exc.ProtocolError( + f"{self.arg} value contains illegal characters" + ) + # Delete untrusted value prevent using it. + value = untrusted_value + del untrusted_value + + # In this case, we just want to allow setting value to features that are + # already set. We want to hide hide "absent" feature from "prohibited" + # feature (see "admin-permission" below), therefore, it throws + # qubes.exc.PermissionDenied. + self.enforce_arg( + wants=self.dest.features.keys(), + short_reason="destination features", + ) + + # Event "admin-permission" is used to prohibit certain API calls from + # qubesd when qrexec cannot possibly be that restrictive, as it doesn't + # have full knowledge of the system nor the policy is expressive enough. + # Only trusted data should be passed to this method. + # Throws qubes.exc.PermissionDenied if call is prohibited. + self.fire_event_for_permission(value=value) + + # The server is in a bad mood today. Let the user know we will not + # serve them today. + if True: + raise qubes.exc.QubesException( + "Not in a good mood today, feature '%r' doesn't look nice" % + self.arg + ) + + # All validation has passed, we can return the requested data. + self.dest.features[self.arg] = value + self.app.save() + +Inheritance diagram +------------------- + +.. inheritance-diagram:: qubes.api + +Module contents +--------------- + +.. autoclass:: qubes.api.admin +.. autoclass:: qubes.api.internal +.. autoclass:: qubes.api.misc + +.. vim: ts=3 sw=3 et tw=80