Use ProtocolError on invalid requests #751

ben-grande · 2025-11-11T09:14:07Z

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

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...

codecov · 2025-11-14T15:26:53Z

Codecov Report

❌ Patch coverage is 85.37415% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.17%. Comparing base (bb52c10) to head (1a16799).

Files with missing lines	Patch %	Lines
qubes/api/admin.py	85.85%	28 Missing ⚠️
qubes/api/__init__.py	79.16%	10 Missing ⚠️
qubes/utils.py	76.47%	4 Missing ⚠️
qubes/exc.py	94.11%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #751   +/-   ##
=======================================
  Coverage   70.17%   70.17%           
=======================================
  Files          61       61           
  Lines       13977    14073   +96     
=======================================
+ Hits         9808     9876   +68     
- Misses       4169     4197   +28

Flag	Coverage Δ
unittests	`70.17% <85.37%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

DemiMarie · 2026-01-14T19:25:47Z

qubes/exc.py

+    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


Suggested change

way that a correctly behaving client can get ProtocolError is qubesd is

way that a correctly behaving client can get ProtocolError is qubesd is if

DemiMarie · 2026-01-14T19:26:51Z

qubes/exc.py

+
+    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.


Suggested change

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.

Is this deleting the paragraph? Github interface doesn't make it obvious.

I tried to make sure that only sanitized and already trusted information, as well as non private information that the caller doesn't know, is sent in the exception message. There are various cases where this could go wrong, especially in the part about revealing more information to the caller than what it should have, but a proper error message should be done any way to avoid hard to debug errors such as Got empty response from qubesd.

DemiMarie · 2026-01-14T19:28:44Z

qubes/exc.py

+class QubesVolumeRevisionNotFoundError(KeyError):
+    """Specified revision not found in qube volume."""
+
+
+class QubesPoolNotFoundError(KeyError):
+    """Pool does not exist."""
+
+
+class QubesVolumeNotFoundError(KeyError):
+    """Pool does not exist."""
+
+


Catching all of these requires catching KeyError, which seems a bit weird to me. Would StoragePoolException be a better superclass?

And then make class StoragePoolException(QubesException, KeyError)? If yes, I agree.

Would StoragePoolException be a better superclass?

No because it is a superclass of QubesException. Search for comments above each of these exceptions in qubes/api/*, I mention that it may review existence of revisions, volumes and pools.

The issue is that we are doing checks to pass sanitized data to fire_event_for_permission for the qubes.ext.admin, this can revealinformation about the system that the admin-extension could want to block. The current state is that Qrexec policy may not be restrictive enough, and the admin-extension is something to be a more precise ACL, as it is already in qubesd, and not on qrexec level.

DemiMarie · 2026-01-14T19:30:00Z

qubes/api/admin.py

+                self.enforce(
+                    b"\n" not in passphrase,
+                    reason="Passphrase must not contain newline character",
+                )


Should there also be a check for NUL, or does qrexec do this implicitly?

Tested, it allows null, QubesVM.run_service, which run_service_for_stdio uses, has filter_esc=False as default. If set to True, it substitutes by _ which would become an invalid password if user has other prohibited characters in the UTF-8 set. This is a hard one if we want to allow UTF-8 characters in passphrases... switching it off now might break existing workflows, maybe allow the safe to print set you did on qrexec?

scrypt allows NUL and escape character as a passphrase. Tested with reading the passphrase from a file, to not have to deal with bash's handling. scrypt does prohibits \n though.

The qubes.backup.Backup.backup_do, encodes the passphrase in UTF-8, I don't know if conversion from higher sets to a lower set always works. Later, on launch_scrypt, does write the passphrase to the pty.

ben-grande · 2026-01-26T17:35:02Z

PipelineRetryFailed

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

ben-grande · 2026-02-04T11:28:43Z

This PR got bigger than what I anticipated. I will try to restrict myself to:

Choosing ProtocolError and PermissionDenied correctly
Returning exceptions to the clients so problems can be debugged
Minimizing information leak of object existence before admin-permission (hard to do a on most cases, because some information needs to be sanitized before passing to admin_for_permission()).

For future PRs, somethings to tackle would be improving API security: QubesOS/qubes-doc@753fa4e, which, if no GSoC applicant takes it, might be assigned directly to me. I think this becomes more important when the non-dom0 GUI domain is streamlined.

Now the client is able to understand what failed. Fixes: QubesOS/qubes-issues#10345

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().

ben-grande · 2026-02-10T15:37:31Z

Demi did a PR in jan/2025 that I didn't see before... #645, will take a look at it and add things that are relevant.

ben-grande force-pushed the exc-proto-permission branch 2 times, most recently from a8cd75b to 479ac3d Compare November 14, 2025 15:12

ben-grande mentioned this pull request Nov 14, 2025

Start GUI session in VM before GUI daemon connects QubesOS/qubes-issues#9940

Open

ben-grande force-pushed the exc-proto-permission branch from 479ac3d to 6be57f3 Compare January 14, 2026 16:48

DemiMarie reviewed Jan 14, 2026

View reviewed changes

ben-grande force-pushed the exc-proto-permission branch 2 times, most recently from 5858529 to 9c79152 Compare January 23, 2026 15:35

ben-grande force-pushed the exc-proto-permission branch 2 times, most recently from 4ccd46d to cadacc5 Compare January 28, 2026 08:51

ben-grande added 2 commits February 4, 2026 11:42

Clean return logic on transport success or failure

b5df7c3

Skip logging unsanitized value

9dd8297

ben-grande force-pushed the exc-proto-permission branch from cadacc5 to 389be8e Compare February 4, 2026 10:42

ben-grande added 3 commits February 4, 2026 12:30

Return programming exception to caller

3d8e297

Now the client is able to understand what failed. Fixes: QubesOS/qubes-issues#10345

Log PermissionDenied and return exc to caller

1b49482

ben-grande force-pushed the exc-proto-permission branch from 389be8e to 1a16799 Compare February 4, 2026 11:38

ben-grande marked this pull request as ready for review February 5, 2026 15:03

ben-grande requested a review from marmarek February 10, 2026 15:34

ben-grande mentioned this pull request Feb 10, 2026

Better error messages #645

Closed

	way that a correctly behaving client can get ProtocolError is qubesd is
	way that a correctly behaving client can get ProtocolError is qubesd is if

Uh oh!

Use ProtocolError on invalid requests #751

Are you sure you want to change the base?

Use ProtocolError on invalid requests #751

Uh oh!

Conversation

ben-grande commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-grande Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-grande commented Jan 26, 2026

Uh oh!

ben-grande commented Feb 4, 2026

Uh oh!

ben-grande commented Feb 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ben-grande commented Nov 11, 2025 •

edited

Loading

codecov bot commented Nov 14, 2025 •

edited

Loading

ben-grande Jan 22, 2026 •

edited

Loading