Invalid target domains in qrexec cause confusing error

[DemiMarie]

How to file a helpful issue

Qubes OS release

R4.2

Brief summary

If I run qrexec-client-vm --filter-escape-chars-stdout --filter-escape-chars-stderr \# a, I get the expected Request refused. However, the notification is “Denied a from SOURCEVM to _”, which isn’t what the user typed. If there was a VM named _, this might actually result in a service call to _.

If the target VM is merely nonexistent (qrexec-client-vm --filter-escape-chars-stdout --filter-escape-chars-stderr none a, the notification is the expected “Denied a from SOURCEVM to none”. If the target VM is syntactically incorrect even after invalid characters are replaced with _ (such as qrexec-client-vm --filter-escape-chars-stdout --filter-escape-chars-stderr @none a, there is no notification.

Steps to reproduce

See above.

Expected behavior

I expect that invalid target domains are rejected in qrexec-daemon, before being passed to the policy engine.

Actual behavior

Invalid target domains are mangled by qrexec-daemon.

[marmarek]

I expect that invalid target domains are rejected in qrexec-daemon, before being passed to the policy engine.

It might be a good idea if that was the case from the beginning. But changing it now may break some uses (especially when mangling happens inside the argument). We can reconsider it for R5.0, but not earlier.

[DemiMarie]

I expect that invalid target domains are rejected in qrexec-daemon, before being passed to the policy engine.

It might be a good idea if that was the case from the beginning. But changing it now may break some uses (especially when mangling happens inside the argument). We can reconsider it for R5.0, but not earlier.

Would you mind elaborating on “especially when mangling happens inside the argument”? This issue is about target VM names, not service names or arguments, so I’m not sure how this is relevant.

[marmarek]

I've seen the PR changed sanitize_name function to refuse instead of replace chars, but indeed you also change where it is used. If requests that were allowed before are still allowed (given any policy) and requests that were denied are still denied (given any policy) but just with a better error message, then it's okay.

But do pay close attention to @default and @anyvm targets in the policy, which may apply to targets that are not existing VMs too.

[marmarek]

For example qvm-copy-to-vm '!' some-file does trigger target prompt (due to default policy for @anyvm target), and it should continue to do so.

[DemiMarie]

I've seen the PR changed sanitize_name function to refuse instead of replace chars, but indeed you also change where it is used. If requests that were allowed before are still allowed (given any policy) and requests that were denied are still denied (given any policy) but just with a better error message, then it's okay.

After https://github.com/QubesOS/qubes-core-qrexec/pull/146/commits/2f74ec911797db62858c79804d6aff93bb01e83d, sanitize_name() is only used for service names and arguments (not for target names), so https://github.com/QubesOS/qubes-core-qrexec/pull/146/commits/c99d561261be542df3d682a2cac67ea5c69c4bb3 inlines it into its only caller without changing any observable behavior.

But do pay close attention to @default and @anyvm targets in the policy, which may apply to targets that are not existing VMs too.

The only case where behavior is changed is where the caller passes something that could not possibly be a valid VM name. Specifically, consider qrexec-client-vm 'a#b' a+b (note the pound sign):

• Previously, this would be interpreted as a call to a VM named a_b (note the underscore) with a+b as the argument.
• With QubesOS/qubes-core-qrexec#146, this is rejected immediately (before the policy engine acts on it).

My reasoning is that passing a string that cannot possibly be a valid VM name is probably a bug in the caller, not intentional.

[DemiMarie]

For example qvm-copy-to-vm '!' some-file does trigger target prompt (due to default policy for @anyvm target), and it should continue to do so.

I see. In that case I’m going to close this (and QubesOS/qubes-core-qrexec#146). To give a better error message would require passing the unsanitized name into the Python code, and I’d prefer to not do that for security reasons.

[github-actions[bot]]

This issue has been closed as "declined." This means that the issue describes a legitimate bug (in the case of bug reports) or proposal (in the case of enhancements and tasks), and it is actionable, at least in principle. Nonetheless, it has been decided that no action will be taken on this issue. Here are some examples of reasons why an issue may be declined:

• No solution can be found.
• The proposed action is not possible.
• The proposed action would weaken security to an unacceptable degree.
• The proposed action would be too costly (in time, money, or other resources) relative to the benefits it would provide.
• The proposed action would make some things better while making other things worse, and the trade-off is not worthwhile.

These are just general examples. If the specific reason for this particular issue being declined has not already been provided, please feel free to leave a comment below asking for an explanation.

We respect the time and effort you have taken to file this issue, and we understand that this outcome may be unsatisfying. Please accept our sincere apologies and know that we greatly value your participation and membership in the Qubes community.

If anyone reading this believes that this issue was closed in error or that the resolution of "declined" is not accurate, please leave a comment below saying so, and the Qubes team will review this issue again. For more information, see How issues get closed.

[DemiMarie]

For example qvm-copy-to-vm '!' some-file does trigger target prompt (due to default policy for @anyvm target), and it should continue to do so.

Better solution: replace ! with @default to match what the policy engine already does internally.

[marmarek]

Better solution: replace ! with @default to match what the policy engine already does internally.

That's completely not the point! Sure, there are several way to express given intention. The point is that the solution you propose changes behavior (breaks something that was working configuration before).

[marmarek]

If you meant that qrexec-daemon should replace "invalid" targets with @default instead of "_" chars, then it simply breaks different setups (granted, less likely, but still). For example a call qrexec-client-vm 'some#vm' ..., where some_vm qube exists will be interpreted differently with the change you propose.

[DemiMarie]

If you meant that qrexec-daemon should replace "invalid" targets with @default instead of "_" chars, then it simply breaks different setups (granted, less likely, but still). For example a call qrexec-client-vm 'some#vm' ..., where some_vm qube exists will be interpreted differently with the change you propose.

I see! I had assumed that calling some_vm when some#vm was provided is a bug. Are you saying that it is intended behavior, or at least should not be changed until R5.0?

[marmarek]

This might be considered a bug, but behavior changes in qrexec (and also in other already widely used features) should be done carefully. The mangling of forbidden characters in the target is there since basically forever, so changing it needs to wait until R5.0.