Open andrewdavidwong opened 2 years ago
Continuing from https://github.com/QubesOS/qubes-issues/issues/8227#issuecomment-1828309184 (by @ben-grande):
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=
.
No, it additionally requires other "allow" rule to be present.
Generally, the current behavior of target=
rule is meant o limit leaking target name (or possibility to enumerate qubes) to the source qube.
Consider the following example:
some.service * source @default target=destination
With current implementation, only the call with @default
target will get redirected to destination. Specifying any call target explicitly (being actual destination
or something else) will get denied. With your change, all calls (also with @default
target) will get denied, as collect_targets_for_ask
will return an empty list.
But then, if you add an allow rule:
some.service * source destination allow
some.service * source @default allow target=destination
the source qube can make a call to destination
explicitly and confirm its existence this way.
Note also, in this (and few other) examples, the order of allow/deny rules rarely matter if you override the target, as the original target will usually be different than the overridden one. For example, with your change:
some.service * source dest-1 deny
some.service * source dest-2 allow target=dest-1
is the same as
some.service * source dest-2 allow target=dest-1
some.service * source dest-1 deny
because dest-2
!= dest-1
, so collect_targets_for_ask
will consider both rules.
All that said, even with your change, it's still possible to prevent source qube learning real target name, by using different rule(s):
some.service * source @anyvm allow target=destination
This, instead of refusing other calls (requested to actual destination
or elsewhere), will allow them but redirect. This may be a bit confusing when configuring your system (you think you make a call to target1, but actually it gets redirected to target2), but OTOH arguably the current behavior of target=
is even more confusing.
On 23-11-27 11:01:13, Marek Marczykowski-Górecki wrote:
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=
.No, it additionally requires other "allow" rule to be present.
Generally, the current behavior of
target=
rule is meant o limit leaking taret name (or possibility to enumerate qubes) to the source qube. Consider the following example:some.service * source @default target=destination
With current implementation, only the call with @.
target will get redirected to destination. Specifying any call target explicitly (being actual
destination` or something else) will get denied. With your change, all calls (also with @.target) will get denied, as
collect_targets_for_ask` will return an empty list.
It will return an empty list only
if deny
rule is placed before the
ask
or allow
rule. That is the behavior I am proposing.
When dev
calls:
qrexec-client-vm -- @default qubes.GitInit+dir
The call will be allowed (with interaction):
qubes.GitInit * dev @default ask default_target=sys-git target=sys-git
qubes.GitInit * dev @anyvm deny
which is the intended behavior.
The call will be allowed (with interaction):
qubes.GitInit * dev @anyvm deny
qubes.GitInit * dev @default ask default_target=sys-git target=sys-git
and this is the weird case we are discussing.
The call will be allowed (with interaction):
qubes.GitInit * dev @default ask default_target=sys-git target=sys-git
qubes.GitInit * dev @anyvm deny
because the ask comes before the deny.
The call will be rejected:
qubes.GitInit * dev @anyvm deny
qubes.GitInit * dev @default ask default_target=sys-git target=sys-git
because sys-git matches in previous rule @.***` that was denied to that service+arg.
But then, if you add an allow rule:
some.service * source destination allow some.service * source @default allow target=destination
the source qube can make a call to
destination
explicitly and confirm its existence this way.Note also, in this (and few other) examples, the order of allow/deny rules rarely matter if you override the target, as the original target will usually be different than the overridden one. For example, with your change:
some.service * source dest-1 deny some.service * source dest-2 allow target=dest-1
is the same as
some.service * source dest-2 allow target=dest-1 some.service * source dest-1 deny
because
dest-2
!=dest-1
, socollect_targets_for_ask
will consider both rules.
And this behavior I don't want, the deny rule precedence not being enforced when using redirects.
User sets in 30-user.policy:
some.service * source dest-1 deny
AdminVM, 3rd-party packagers and QubesOS sets in 80-package.policy:
some.service * source @default allow target=dest-1
And I do not like that later rules override previous rules. This behavior does not allow users to fully control their deny rules, as they can be overriden at a later time. Isn't this the exception to the rule? Overriding user configuration? I believe that following this norm and allowing redirects only if there are no previous deny rule to that qube is more easy to grasp and does not break current qubes policies at 90-*.policy.
Side note that the PR is not finished for some edge cases, but all that can be worked on if you agree that the current behavior should change and is against the norm that previous rules can enforce later rules.
All that said, even with your change, it's still possible to prevent source qube learning real target name, by using different rule(s):
some.service * source @anyvm allow target=destination
I do not use direct qube names, always tokens.
This, instead of refusing other calls (requested to actual
destination
or elsewhere), will allow them but redirect. This may be a bit confusing when configuring your system (you think you make a call to target1, but actually it gets redirected to target2), but OTOH arguably the current behavior oftarget=
is even more confusing.
I don't find the behavior of target=
confusing and I don't think the
issues were duplicate.
I understand the currenct behavior is to always override the destination distegarding of its value and redirect to the target you want.
I am however trying to limit the target=
to a value that has no
preceding deny
rule, making this call be denied:
some.service * source destination deny
some.service * source @default allow target=destination
But not this call when the destination qube value is the token @.***`.
some.service * source @default allow target=destination
some.service * source destination deny
-- Benjamin Grande
Email parsing mangled formatting and masked out some keywords...
The call will be allowed (with interaction):
qubes.GitInit * dev @AnyVM deny qubes.GitInit * dev @default ask default_target=sys-git target=sys-git
No, it won't. @anyvm
matches also @default
, so it will never reach the ask
action.
But I think you meant this example instead:
qubes.GitInit * dev sys-git deny
qubes.GitInit * dev @default ask default_target=sys-git target=sys-git
With your patch indeed this will be denied (which wasn't the case before your patch).
Note to self: the patch indeed considers only earlier rules, not later ones (even though target column may not match) only because collect_targets_for_ask
prefers target=
field if present over the target column.
I don't find the behavior of
target=
confusing and I don't think the issues were duplicate.
The change you propose fixes part of this issue, the difference in allowed targets between
qubes.GitInit * dev sys-git deny
# with or without default_target=
qubes.GitInit * dev @default ask target=sys-git
and
qubes.GitInit * dev sys-git deny
qubes.GitInit * dev @default ask default_target=sys-git
Before your change, the first will have one possible target, while the latter will have possible targets list empty (effectively denying the call). After your change, both will have empty targets list.
But indeed, the difference remains without the earlier deny
rule. So it isn't fully the same issue, but I'd call it still close enough, especially since that's the difference that was considered to be changed.
So, generally I agree with the proposed change. But we need some safe approach to deploy it, because it is a semantic change of existing rules. The qrexec-legacy-convert
tool uses qrexec-policy-graph
before and after to verify if semantic remained the same (BTW, thanks for helping with improving this tool!), but that was about changing rule files, not changing meaning of rules without changing rules themselves.
The change should only result in some actions to be denied that were previously allowed, so it is a "safer" direction, but still it may break user setups in unintended ways. This also means it is too late to deploy it into R4.2, I think.
The change should only result in some actions to be denied that were previously allowed, so it is a "safer" direction, but still it may break user setups in unintended ways. This also means it is too late to deploy it into R4.2, I think.
With the default rules this change is a no-op, so it is definitely safe to deploy in that case. What about:
So, generally I agree with the proposed change. But we need some safe approach to deploy it, because it is a semantic change of existing rules. The qrexec-legacy-convert tool uses qrexec-policy-graph before and after to verify if semantic remained the same (BTW, thanks for helping with improving this tool!),
Thanks for reviewing!
but that was about changing rule files, not changing meaning of rules without changing rules themselves.
As far as I can see, this migration would be difficult because it needs to change the rules order. The destination
can have tokens such as tags which the policy doesn't know which qube has that. Placing all deny rules at the bottom of the file would facilitate not having to find the correct line number to but the rule below, but this would mess even further with user setups by leaving ask and allow at the top and deny at the bottom, but without semantic changes.
But.... unman and andrew and me were expecting for this to be already the behavior and Qubes default policies also put deny rules after ask and allow that have redirects.
The change should only result in some actions to be denied that were previously allowed, so it is a "safer" direction, but still it may break user setups in unintended ways. This also means it is too late to deploy it into R4.2, I think.
I understand it is not safe to assume how the user has configured their own system... and maybe I am overcomplicating how difficult it is to do this migration. I don't think it is too late for R4.2, maybe for the first release yes, but for a point release it might be okay depending on how much you consider this a breaking change.
The change should only result in some actions to be denied that were previously allowed, so it is a "safer" direction, but still it may break user setups in unintended ways. This also means it is too late to deploy it into R4.2, I think.
With the default rules this change is a no-op, so it is definitely safe to deploy in that case. What about:
- Making the change conditional on some setting.
- Enabling the setting by default if it would be a no-op.
- Allowing users to opt-in manually if this was not done automatically.
If it is worth it, !compat-4.1
can be used, which can be set on 35-compat.policy
or another file if that one can not be replaced.
How to file a helpful issue
Qubes OS release
4.1.1
Description
This issue is branched from #7720. It concerns the parts in bold below.
@marmarek wrote in https://github.com/QubesOS/qubes-issues/issues/7720#issuecomment-1230206595:
Therefore, this issue is for deciding how to handle the semantics of
target=
anddefault_target=
with the aim of reducing potential user confusion regarding the difference between them.Related issues
The other points mentioned in the quoted text above are handled in other issues: