Skip to content

Commit

Permalink
Limit parser target redirect to allowed list
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ben-grande committed Nov 27, 2023
1 parent 9670cb5 commit ebc6a3c
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 3 deletions.
25 changes: 22 additions & 3 deletions qrexec/policy/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -1013,6 +1013,17 @@ def evaluate(self, request: Request) -> AllowResolution:
elif target == "@adminvm":
target = "dom0"

available_targets = list(
self.rule.policy.collect_targets_for_ask(request)
)
if target not in available_targets:
raise AccessDenied(
"policy define 'allow' action at {}:{} but target {} was "
"denied in a previous rule".format(
self.rule.filepath, self.rule.lineno, target
)
)

if not self.autostart and not self.allow_no_autostart(
target, request.system_info
):
Expand Down Expand Up @@ -1076,12 +1087,20 @@ def evaluate(self, request: Request) -> AskResolution:
assert self.rule.is_match(request)

targets_for_ask: Iterable[str]
available_targets = list(
self.rule.policy.collect_targets_for_ask(request)
)
if self.target is not None:
if self.target not in available_targets:
raise AccessDenied(
"policy define 'ask' action at {}:{} but target {} was "
"denied in a previous rule".format(
self.rule.filepath, self.rule.lineno, self.target
)
)
targets_for_ask = [self.target]
else:
targets_for_ask = list(
self.rule.policy.collect_targets_for_ask(request)
)
targets_for_ask = available_targets

if not self.autostart:
targets_for_ask = [
Expand Down
10 changes: 10 additions & 0 deletions qrexec/tests/policy_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -1177,6 +1177,10 @@ def test_020_collect_targets_for_ask(self):
* * test-vm1 @anyvm ask
* * test-vm2 @tag:tag1 deny
* * test-vm2 @tag:tag2 allow
* * test-vm3 @tag:tag1 deny
* * test-vm3 @tag:tag1 allow target=test-vm1
* * test-vm3 @tag:tag2 deny
* * test-vm3 @tag:tag2 ask default_target=test-vm2 target=test-vm2
* * test-no-dvm @type:AppVM deny
* * @type:AppVM @default allow target=test-vm3
* * @tag:tag1 @type:AppVM allow
Expand Down Expand Up @@ -1207,6 +1211,12 @@ def test_020_collect_targets_for_ask(self):
self.assertCountEqual(
policy.collect_targets_for_ask(_req("test-vm3", "@default")), []
)
self.assertCountEqual(
policy.collect_targets_for_ask(_req("test-vm3", "test-vm1")), [],
)
self.assertCountEqual(
policy.collect_targets_for_ask(_req("test-vm3", "test-vm2")), [],
)
self.assertCountEqual(
policy.collect_targets_for_ask(_req("test-standalone", "@default")),
[
Expand Down

0 comments on commit ebc6a3c

Please sign in to comment.