Skip to content

Commit 71b5ab2

Browse files
committed
Merge remote-tracking branch 'origin/pr/751'
* origin/pr/751: Document the API Disable libvirtaio debug log and add allow revert Optionally log API calls made by dom0 Add custom exception to assignment mode and backup Fix assignment mode docstring Do not raise absent object before admin-permission Log and return prohibitions and client errors Skip logging unsanitized value Clean return logic on transport success or failure Prohibit empty tags and labels Use correct internal exception Pull request description: 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. ProtocolError must be used only for client *programming* errors (bugs), not for problems that the client could not have detected before making the request. PermissionDenied must be used only for service authorization denials. 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-api:" events. If we are already leaking existence of a property, it makes sense to provide a useful exception class for it. Fixes: QubesOS/qubes-issues#10345 Fixes: QubesOS/qubes-issues#10689 --- It is a draft, just posting here for CI. It is more like an idea than a finished version. Logging messages should be okay now, it is only from trusted input. From some open TODOs: ``` % rg 'TODO: ben' qubes/api/admin.py 460: # TODO: ben: info-leak: revision existence 522: # TODO: ben: info-leak: pool existence 525: # TODO: ben: info-leak: volume existence 799: # TODO: ben: info-leak: pool existence 841: # TODO: ben: info-leak: pool existence 849: # TODO: ben: dangerous logging any ASCII value? 949: # TODO: ben: info-leak: label existence 963: # TODO: ben: info-leak: label existence 975: # TODO: ben: info-leak: label existence 1008: # TODO: ben: info-leak: label existence 1309: # TODO: ben: info-leak: template existence 1341: # TODO: ben: info-leak: label existence 1392: # TODO: ben: info-disclosure: learns if qube exists ``` It seems that most checks that are done before `admin-permission` events may leak some data but not sure of its sensitiveness. - `revision` is a date - `volumes` are always the same - `pool` and `labels` have a default that most people use - `template` and `qube` existence, not so nice But without these checks, there is no good error message...
2 parents 624a7eb + deeff5d commit 71b5ab2

18 files changed

Lines changed: 1858 additions & 511 deletions

doc/qubes-api.rst

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
:py:mod:`qubes.api` -- API
2+
=================================
3+
4+
API sanitization is very important to maintain the AdminVM and everything it
5+
controls, secure.
6+
7+
**Simple rules**:
8+
9+
* Handle exceptions gracefully. Unhandled exceptions have the traceback only in
10+
the AdminVM, not allowing clients to know what happened. More information
11+
about exceptions can be found at :py:class:`qubes.exc`.
12+
* Failure to serve a request must raise exceptions that can be understood by the
13+
client. Each stage below allows a different class of exception to be shown.
14+
Be careful to not reveal more information than what the client is allowed to
15+
know at each stage.
16+
17+
**Stages**:
18+
19+
.# Sanitize data
20+
21+
* Must never print, log or store untrusted data. Be careful when throwing an
22+
exception.
23+
* Qrexec sanitizes the argument, but if you will use the argument to
24+
something that you know has an even stricter syntax, it must be sanitized
25+
also.
26+
* The payload is received raw, in bytes. It must always be sanitized before
27+
being passed to functions that expect it to be already trusted.
28+
* When sanitizing data, if it may reveal information from the system, such as
29+
object existence, then, throw :py:class:`qubes.exc.PermissionDenied` to
30+
avoid leaking object existence or delay reveal till after
31+
`admin-permission`.
32+
33+
.# Fire permission event
34+
35+
* Must only pass sanitized information.
36+
* The most commonly used is
37+
:py:meth:`qubes.api.AbstractQubesAPI.fire_event_for_permission`, which
38+
fires event `admin-permission` directly, used by
39+
:py:class:`qubes.api.admin.AdminExtension`. For more complex cases,
40+
involving global information such as fetching objects from different
41+
destinations, :py:meth:`qubes.api.AbstractQubesAPI.fire_event_for_filter`
42+
is more appropriate, as it fires `admin-permission` for each operation
43+
required.
44+
45+
.# Action
46+
47+
* The client is fully authorized at this stage, it passed Qrexec policy
48+
evaluation and Qubesd `admin-permission`. The server may be aware that some
49+
resource could not be served before the `admin-permisison`, but it was not
50+
allowed to reveal at that stage, now it can reveal what and why it failed.
51+
A custom exception derived from :py:class:`qubes.exc.QubesException` can be
52+
used to allow the client to handle it gracefully.
53+
* Act.
54+
55+
.. code-block:: python
56+
57+
@qubes.api.method(
58+
"dest.feat.Set", # RPC name
59+
wants_arg=True, # Argument must be provided
60+
wants_payload=None, # Payload can be provided
61+
dest_adminvm=False, # Target must not be AdminVM
62+
scope="global", # Applies to the whole system
63+
read=True, # Will read system information
64+
write=True, # Will write information to the system
65+
)
66+
async def dest_feat_set(self, untrusted_payload):
67+
"""
68+
Set destination feature
69+
70+
name: self.arg
71+
value: untrusted_payload
72+
"""
73+
# Qrexec sanitizes self.arg, but our feature name can only be made of
74+
# letters. The client should have know to not make such a request in the
75+
# first case, therefore it throws qubes.exc.ProtocolError.
76+
allowed_chars = string.ascii_letters
77+
self.enforce(
78+
all(c in allowed_chars for c in self.arg),
79+
reason="Feature name must be in safe set: " + allowed_chars,
80+
)
81+
82+
# Payload is in bytes and we receive it without being sanitized
83+
# previously. We only want to allow values to be in ASCII.
84+
try:
85+
untrusted_value = untrusted_payload.decode("ascii", errors="strict")
86+
except UnicodeDecodeError:
87+
raise qubes.exc.ProtocolError("Value contains non-ASCII characters")
88+
# Delete untrusted payload prevent using it.
89+
del untrusted_payload
90+
91+
# Second sanitization of the value
92+
if re.match(r"\A[\x20-\x7E]*\Z", untrusted_value) is None:
93+
raise qubes.exc.ProtocolError(
94+
f"{self.arg} value contains illegal characters"
95+
)
96+
# Delete untrusted value prevent using it.
97+
value = untrusted_value
98+
del untrusted_value
99+
100+
# In this case, we just want to allow setting value to features that are
101+
# already set. We want to hide hide "absent" feature from "prohibited"
102+
# feature (see "admin-permission" below), therefore, it throws
103+
# qubes.exc.PermissionDenied.
104+
self.enforce_arg(
105+
wants=self.dest.features.keys(),
106+
short_reason="destination features",
107+
)
108+
109+
# Event "admin-permission" is used to prohibit certain API calls from
110+
# qubesd when qrexec cannot possibly be that restrictive, as it doesn't
111+
# have full knowledge of the system nor the policy is expressive enough.
112+
# Only trusted data should be passed to this method.
113+
# Throws qubes.exc.PermissionDenied if call is prohibited.
114+
self.fire_event_for_permission(value=value)
115+
116+
# The server is in a bad mood today. Let the user know we will not
117+
# serve them today.
118+
if True:
119+
raise qubes.exc.QubesException(
120+
"Not in a good mood today, feature '%r' doesn't look nice" %
121+
self.arg
122+
)
123+
124+
# All validation has passed, we can return the requested data.
125+
self.dest.features[self.arg] = value
126+
self.app.save()
127+
128+
Inheritance diagram
129+
-------------------
130+
131+
.. inheritance-diagram:: qubes.api
132+
133+
Module contents
134+
---------------
135+
136+
.. autoclass:: qubes.api.admin
137+
.. autoclass:: qubes.api.internal
138+
.. autoclass:: qubes.api.misc
139+
140+
.. vim: ts=3 sw=3 et tw=80

doc/qubes-exc.rst

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ call for effect). Instead consider writing in negative form, implying expected
4343
state: "Domain is not running" instead of "Domain is paused" (yeah, what's wrong
4444
with that?).
4545

46-
Also avoid implying the personhood of the computer, including adressing user in
46+
Also avoid implying the personhood of the computer, including addressing user in
4747
second person. For example, write "Sending message failed" instead of "I failed
4848
to send the message".
4949

qubes-rpc-policy/generate-admin-policy

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -43,19 +43,19 @@ parser.add_argument('--exclude', action='store', nargs='*',
4343
parser.add_argument('service', nargs='*', action='store',
4444
help='Generate policy for those services (default: all)')
4545

46-
def write_default_policy(args, apiname, clasifiers, f):
46+
def write_default_policy(args, apiname, classifiers, f):
4747
''' Write single default policy for given API call '''
48-
assert 'scope' in clasifiers, \
48+
assert 'scope' in classifiers, \
4949
'Method {} lack scope classifier'.format(apiname)
50-
assert any(attr in clasifiers for attr in ('read', 'write', 'execute')), \
50+
assert any(attr in classifiers for attr in ('read', 'write', 'execute')), \
5151
'Method {} lack read/write/execute classifier'.format(apiname)
52-
assert clasifiers['scope'] in ('local', 'global'), \
53-
'Method {} have invalid scope: {}'.format(apiname, clasifiers['scope'])
52+
assert classifiers['scope'] in ('local', 'global'), \
53+
'Method {} have invalid scope: {}'.format(apiname, classifiers['scope'])
5454

5555
file_to_include = 'admin-{scope}-{rwx}'.format(
56-
scope=clasifiers['scope'],
57-
rwx=('rwx' if clasifiers.get('write', False) or
58-
clasifiers.get('execute', False)
56+
scope=classifiers['scope'],
57+
rwx=('rwx' if classifiers.get('write', False) or
58+
classifiers.get('execute', False)
5959
else 'ro'))
6060

6161
if args.verbose:

0 commit comments

Comments
 (0)