Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect handling of qrexec policy rules #8227

Closed
unman opened this issue May 26, 2023 · 7 comments · May be fixed by QubesOS/qubes-core-qrexec#133
Closed

Incorrect handling of qrexec policy rules #8227

unman opened this issue May 26, 2023 · 7 comments · May be fixed by QubesOS/qubes-core-qrexec#133
Labels
affects-4.1 This issue affects Qubes OS 4.1. C: core P: critical Priority: critical. Between "major" and "blocker" in severity. R: duplicate Resolution: Another issue exists that is very similar to or subsumes this one. security This issue pertains to the security of Qubes OS. T: bug Type: bug report. A problem or defect resulting in unintended behavior in something that exists.

Comments

@unman
Copy link
Member

unman commented May 26, 2023

How to file a helpful issue

Qubes OS release

4.1

Brief summary

qrexec policy rules are not handled correctly, with effect that it is possible to qvm-copy to a prohibited qube.

Steps to reproduce

Create rules in /etc/qubes/policy.d/30-user.policy:

qubes.Filecopy  *  @anyvm  vault   deny
qubes.Filecopy  *  foo     @anyvm  allow target=vault

Attempt to qvm-copy file in foo

Expected behavior

Filecopy will be blocked by 1st rule

Actual behavior

File is copied to vault, although precedent rule explicitly blocks transfers.

@unman unman added P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. T: bug Type: bug report. A problem or defect resulting in unintended behavior in something that exists. labels May 26, 2023
@andrewdavidwong
Copy link
Member

I can also reproduce this.

@andrewdavidwong andrewdavidwong added P: critical Priority: critical. Between "major" and "blocker" in severity. security This issue pertains to the security of Qubes OS. needs diagnosis Requires technical diagnosis from developer. Replace with "diagnosed" or remove if otherwise closed. C: core and removed P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. labels May 26, 2023
@andrewdavidwong andrewdavidwong added this to the Release 4.1 updates milestone May 26, 2023
@andrewdavidwong andrewdavidwong changed the title Incorrect handling qrexec policy rules Incorrect handling of qrexec policy rules May 26, 2023
@andrewdavidwong andrewdavidwong added the affects-4.1 This issue affects Qubes OS 4.1. label Aug 8, 2023
@andrewdavidwong andrewdavidwong removed this from the Release 4.1 updates milestone Aug 13, 2023
@ben-grande
Copy link

Diagnosis: Redirect (target= parameter, but also default_target=) does not check against a list of targets that were not denied, instead it redirects to any target that is available in your system.

I think the error was introduced because the target= parameter is supposed to override the target disregarding the IntendedTarget value dest:

qubes.Filecopy * foo dest allow target=vault

That is why the above works.

But I don't think it is correct for the redirect target to dismiss previous deny rules.

The function collect_targets_for_ask() seems to build the correct list when the rule has no redirect. It could be useful to compare the set of available targets that are not denied by policy against the redirect target.

I tried to fix this issue but couldn't. Would appreciate guidance.

ben-grande added a commit to ben-grande/qubes-core-qrexec that referenced this issue Nov 27, 2023
Limits the parameters target= and default_target= to targets that have
no previous deny rule. The previous rule file name and line number is
not known and a general information is reported.

Reported-by: unman <unman@thirdeyesecurity.org>
Fixes: QubesOS/qubes-issues#8227
@ben-grande
Copy link

PR submitted :)

@marmarek
Copy link
Member

Diagnosis: Redirect (target= parameter, but also default_target=) does not check against a list of targets that were not denied, instead it redirects to any target that is available in your system.

This is intentional for target= - see https://www.qubes-os.org/doc/qrexec/#policy-files:

Note that if the request is redirected (target= parameter), policy action remains the same – even if there is another rule which would otherwise deny such request.

But default_target should not have this side effect, it's only about pre-selecting target from otherwise allowed ones.

@ben-grande
Copy link

This is intentional for target= - see https://www.qubes-os.org/doc/qrexec/#policy-files:

Note that if the request is redirected (target= parameter), policy action remains the same – even if there is another rule which would otherwise deny such request.

I understand it is intentional. It is okay to override the destination on the same line by using target=, but is it okay to ignore previous deny rules to that destination?

My PR aboves consider previous deny rules and if encounters none, uses the redirect target=.

Shouldn't this intentional behavior change? When using Admin Management VMs or qubes 90-*.policy, should their rule (later sourced/read) always override the user rule (previous sourced/read)?

@marmarek
Copy link
Member

Duplicate of #7723

@marmarek marmarek marked this as a duplicate of #7723 Nov 27, 2023
@marmarek marmarek added the R: duplicate Resolution: Another issue exists that is very similar to or subsumes this one. label Nov 27, 2023
Copy link

This issue has been closed as a "duplicate." This means that another issue exists that is very similar to or subsumes this one. If any useful information on this issue is not already present on the other issue, please add it in a comment on the other issue. Here are some common cases of duplicate issues:

  • The other issue is closed. The other issue being closed does not prevent this issue from duplicating it. We will examine the closed issue and, if appropriate, reopen it.
  • The other issue is for a different Qubes release. We usually maintain only one issue for all affected Qubes releases.
  • The other issue is very old. The mere age of an issue is not, by itself, a relevant factor when determining duplicates.

By default, the newer issue will be closed in favor of the older issue. However, we make exceptions when we determine that it would be significantly more useful to keep the newer issue open instead of the older one.

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 "duplicate" is not accurate, please leave a comment below, and we will review this issue again. For more information, see How issues get closed.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 27, 2023
@andrewdavidwong andrewdavidwong removed the needs diagnosis Requires technical diagnosis from developer. Replace with "diagnosed" or remove if otherwise closed. label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-4.1 This issue affects Qubes OS 4.1. C: core P: critical Priority: critical. Between "major" and "blocker" in severity. R: duplicate Resolution: Another issue exists that is very similar to or subsumes this one. security This issue pertains to the security of Qubes OS. T: bug Type: bug report. A problem or defect resulting in unintended behavior in something that exists.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants