Improve qrexec policy syntax for $dispvm to be more expressive and less confusing

[rootkovska]

There are a few problems with the new syntax with regards to DispVMs (introduced in 4.0-rc1).

In scenarios where we will have more than one mgmt VM in the system (e.g. GUI domain and corporate mgmt VM), we would like to be able to state something like this via the policy (for whatever service):

# Allow any VM to use system-default DispVMs (but not per-AppVM's default, see below for discussion):
$anyvm $dispvm_system_default allow

# Allow personal VMs to start DispVMs created by the user via guidom:
$tag:created-by-guidom $dispvm_tagged:created-by-guidom allow

# Allow corpo-managed VMs to start DispVMs based on corpo-owned AppVMs:
$tag:created-by-mgmt-corpo $dispvm_tagged:created-by-mgmt-corpo allow

# Forbid any other combinations (redundant, but for completeness)
$anyvm $dispvm deny

This is because we would like to (generically) avoid a situation where one of the mgmt VMs (say the guidom) will request to start a DispVM based on some AppVM belonging to the "corpo world".

The above exemplary snippet contains a few proposed extensions to the policy:

1. The new keyword $dispvm_tagged:XYZ meaning: a DispVM created from an AppVM which has tag XYZ,

2. The change of the meaning of $dispvm. Currently it means "a DispVM based on the AppVM pointed by default_dispvm property", and I think this is confusing to those users who might interpret this as "any DispVM". The consequence of this mistaken interpretation might be fatal. Thus I postulate that we change the meaning of $dispvm to mean "any AppVM", and this is how it was used in the example above,

3. Adding explicit $dispvm_system_default, to mean system-default DispVM (as set/reported by qubes-prefs),

4. Adding explicit $dispvm_appvm_default, to mean per-AppVM default DispVM (as set/reported by qvm-prefs for the particular calling VM, defaults to system default).

The distinction between $dispvm_system_default and $dispvm_appvm_default is an important one, because otherwise users are likely to fall into the trap of thinking of $dispvm_default as "system default", so e.g. a benign DispVM based on some empty template, overlooking the possibility that a malcious mgmt VM might override the default_dispvm for its own VMs, as discussed above.

[woju]

Arguably this is relevant only when one has more than one MgmtVM. This is already complicated situation, since R4.0 out of the box supports only one MgmtVM, namely dom0 which also doubles as GuiVM. Situation when one needs another MgmtVM is an "enterprise" situation. With that in mind, I'd suggest at minimum to reschedule this for R4.1.

Ad rem: IMO this needlessly complicates policy syntax (and consequently parser). This can already be implemented using extensions by forbidding setting dispvm_allowed (or #3047) property to unwanted values.

Example extension (maybe add this to documentation?):

import qubes.api
import qubes.ext
class MyCorporateExtension(qubes.ext.Extension):
    @qubes.ext.handler('admin-permission:admin.vm.property.Set')
    def on_vm_property_set(self, subject, event, dest, arg, newvalue):
        if dest.name in ('secretvm1', 'secretvm2') \
                and arg == 'default_dispvm':
            try:
                newvalue = self.app.domains[newvalue]
            except KeyError:
                raise qubes.api.PermissionDenied()
            if 'managed-by-corpo' not in newvalue.tags:
                raise qubes.api.PermissionDenied()

[woju]

Arguably this is relevant only when one has more than one MgmtVM. This is already complicated situation, since R4.0 out of the box supports only one MgmtVM, namely dom0 which also doubles as GuiVM. Situation when one needs another MgmtVM is an "enterprise" situation. With that in mind, I'd suggest at minimum to reschedule this for R4.1.

Ad rem: IMO this needlessly complicates policy syntax (and consequently parser). This can already be implemented using extensions by forbidding setting dispvm_allowed (or #3047) property to unwanted values.

Example extension (maybe add this to documentation?):

import qubes.api
import qubes.ext
class MyCorporateExtension(qubes.ext.Extension):
    @qubes.ext.handler('admin-permission:admin.vm.property.Set')
    def on_vm_property_set(self, subject, event, dest, arg, newvalue):
        if dest.name in ('secretvm1', 'secretvm2') \
                and arg == 'default_dispvm':
            try:
                newvalue = self.app.domains[newvalue]
            except KeyError:
                raise qubes.api.PermissionDenied()
            if 'managed-by-corpo' not in newvalue.tags:
                raise qubes.api.PermissionDenied()

[rootkovska]

The problem with deferring this to a later time, e.g. 4.1, is that large part of the solution I proposed requires changing of the syntax for the policy definition, and this is something we should do now (i.e. still at an early rc1 stage, rather than later). How are we going to implement it actually (extension or core logic) is of secondary importance.

[rootkovska]

The problem with deferring this to a later time, e.g. 4.1, is that large part of the solution I proposed requires changing of the syntax for the policy definition, and this is something we should do now (i.e. still at an early rc1 stage, rather than later). How are we going to implement it actually (extension or core logic) is of secondary importance.

[marmarek]

3. Adding explicit $dispvm_system_default, to mean system-default DispVM (as set/reported by qubes-prefs),

4. Adding explicit $dispvm_appvm_default, to mean per-AppVM default DispVM (as set/reported by qvm-prefs for the particular calling VM, defaults to system default).

The distinction between $dispvm_system_default and $dispvm_appvm_default is an important one, because otherwise users are likely to fall into the trap of thinking of $dispvm_default as "system default", so e.g. a benign DispVM based on some empty template, overlooking the possibility that a malcious mgmt VM might override the default_dispvm for its own VMs, as discussed above.

There is no such thing as "system default" and "appvm default" DispVM. There is only one: "appvm default" (using your terminology). This property happen to default to global property, but that's a mere convenience for simplest use case (when user have just one DispVM template for everything). If you want to prevent usage of some particular DispVM by others it's better to express that explicitly in the policy (using "deny" rule, before others), instead of looking for all possible ways leading to that place (carefully manually evaluating all "allow" rules). Also, we have some preliminary version of a tool to help with policy validation.
If you still want to have some DispVM available for anyone, and state that in "allow" rule, better use that VM characteristic itself (tag for example), not the fact that it is set as default (either global one, or per-VM). Otherwise you'll fall into exactly the same "trap" when one (either user, or corporate admin) is allowed to change the global default.

1. The new keyword $dispvm_tagged:XYZ meaning: a DispVM created from an AppVM which has tag XYZ,

I agree this is useful, but I don't like the underscore here. Personally I like the initial version (from email thread): $dispvm:$tag:XYZ. I think it's more logical, even if that's the only special word allowed after $dispvm:.

2. The change of the meaning of $dispvm. Currently it means "a DispVM based on the AppVM pointed by default_dispvm property", and I think this is confusing to those users who might interpret this as "any DispVM". The consequence of this mistaken interpretation might be fatal. Thus I postulate that we change the meaning of $dispvm to mean "any AppVM", and this is how it was used in the example above,

Actually, the current approach is the safe one, and yours may lead to fatal mistakes. Allowing access to $dispvm allow only to one DispVM, dedicated to this VM (in most cases: global default), not all of them - including some corporate one, and the one with printing credentials. You may say that one may write $anyvm $dispvm deny rule to selectively deny DispVM usage (allowing everything else), but it's always better to selectively allow things, than to selectively deny them (when it comes to writing policy).

On the other hand, there is currently no syntax for "any DispVM" (but it is possible to express it using multiple rules). Maybe add $dispvm:$anyvm? But this may be confusing, because it will really mean "DispVM based on any AppVM with dispvm_allowed (or #3047) set to True", so not exactly the same as $anyvm in other context.

Related: what tags new DispVM should have? This is important for writing rules what DispVM can do.
I see a few sensible options:

1. Just one, started-by-some-vm - based on what VM have created it (or maybe even use created-by-some-vm?).
2. Same as AppVM the DispVM is based on (this is the current version) - based on assumption that AppVM being base for DispVM control how DispVM behave.
3. Something based on all tags of calling VM - either full copy, or the same with some prefix (caller-?). So DispVM started by VM with tag created-by-corpo-mgmt will get a tag caller-created-by-corpo-mgmt.

[marmarek]

3. Adding explicit $dispvm_system_default, to mean system-default DispVM (as set/reported by qubes-prefs),

4. Adding explicit $dispvm_appvm_default, to mean per-AppVM default DispVM (as set/reported by qvm-prefs for the particular calling VM, defaults to system default).

The distinction between $dispvm_system_default and $dispvm_appvm_default is an important one, because otherwise users are likely to fall into the trap of thinking of $dispvm_default as "system default", so e.g. a benign DispVM based on some empty template, overlooking the possibility that a malcious mgmt VM might override the default_dispvm for its own VMs, as discussed above.

There is no such thing as "system default" and "appvm default" DispVM. There is only one: "appvm default" (using your terminology). This property happen to default to global property, but that's a mere convenience for simplest use case (when user have just one DispVM template for everything). If you want to prevent usage of some particular DispVM by others it's better to express that explicitly in the policy (using "deny" rule, before others), instead of looking for all possible ways leading to that place (carefully manually evaluating all "allow" rules). Also, we have some preliminary version of a tool to help with policy validation.
If you still want to have some DispVM available for anyone, and state that in "allow" rule, better use that VM characteristic itself (tag for example), not the fact that it is set as default (either global one, or per-VM). Otherwise you'll fall into exactly the same "trap" when one (either user, or corporate admin) is allowed to change the global default.

1. The new keyword $dispvm_tagged:XYZ meaning: a DispVM created from an AppVM which has tag XYZ,

I agree this is useful, but I don't like the underscore here. Personally I like the initial version (from email thread): $dispvm:$tag:XYZ. I think it's more logical, even if that's the only special word allowed after $dispvm:.

2. The change of the meaning of $dispvm. Currently it means "a DispVM based on the AppVM pointed by default_dispvm property", and I think this is confusing to those users who might interpret this as "any DispVM". The consequence of this mistaken interpretation might be fatal. Thus I postulate that we change the meaning of $dispvm to mean "any AppVM", and this is how it was used in the example above,

Actually, the current approach is the safe one, and yours may lead to fatal mistakes. Allowing access to $dispvm allow only to one DispVM, dedicated to this VM (in most cases: global default), not all of them - including some corporate one, and the one with printing credentials. You may say that one may write $anyvm $dispvm deny rule to selectively deny DispVM usage (allowing everything else), but it's always better to selectively allow things, than to selectively deny them (when it comes to writing policy).

On the other hand, there is currently no syntax for "any DispVM" (but it is possible to express it using multiple rules). Maybe add $dispvm:$anyvm? But this may be confusing, because it will really mean "DispVM based on any AppVM with dispvm_allowed (or #3047) set to True", so not exactly the same as $anyvm in other context.

Related: what tags new DispVM should have? This is important for writing rules what DispVM can do.
I see a few sensible options:

1. Just one, started-by-some-vm - based on what VM have created it (or maybe even use created-by-some-vm?).
2. Same as AppVM the DispVM is based on (this is the current version) - based on assumption that AppVM being base for DispVM control how DispVM behave.
3. Something based on all tags of calling VM - either full copy, or the same with some prefix (caller-?). So DispVM started by VM with tag created-by-corpo-mgmt will get a tag caller-created-by-corpo-mgmt.

[rootkovska]

I suggest the best way to keep this discussion, is that everyone who proposes a different syntax, provides also their own version of the qrexec policy that solves the examples I've given above. We could then compare which solution offers the least-confusing expression of polices. Additionally, extra examples might be given, e.g. to illustrate how the proposed solution is better than other proposed solutions (or current implementation).

[rootkovska]

I suggest the best way to keep this discussion, is that everyone who proposes a different syntax, provides also their own version of the qrexec policy that solves the examples I've given above. We could then compare which solution offers the least-confusing expression of polices. Additionally, extra examples might be given, e.g. to illustrate how the proposed solution is better than other proposed solutions (or current implementation).

[marmarek]

The problem with deferring this to a later time, e.g. 4.1, is that large part of the solution I proposed requires changing of the syntax for the policy definition, and this is something we should do now (i.e. still at an early rc1 stage, rather than later).

This repeatedly come during rc1 stage. Some choices turns out to be not so good only at broader testing phase. In our current workflow, only bugfixes are allowed after releasing rc1. Specifically, new features or API changes are not allowed. While in practice we make exceptions to this rule, it is not the right thing to do. Maybe we need to bring alpha/beta stages back (or just one beta)?

Back to the topic:

1. Mark fedora-25-dvm with globally-available-dispvm tag (or any other sensible name).
2. Use this policy:

# Allow any VM to use standard DispVMs:
$anyvm $dispvm:$tag:globally-available-dispvm allow

# Allow personal VMs to start DispVMs created by the user via guidom:
$tag:created-by-guidom $dispvm:$tag:created-by-guidom allow

# Allow corpo-managed VMs to start DispVMs based on corpo-owned AppVMs:
$tag:created-by-mgmt-corpo $dispvm:$tag:created-by-mgmt-corpo allow

# Forbid any other combinations (redundant, but for completeness)
$anyvm $anyvm deny

The above will enforce the right policy, regardless of default_dispvm property (either AppVM or global).
Note that I haven't used here neither $dispvm:$anyvm, nor bare $dispvm.

[marmarek]

The problem with deferring this to a later time, e.g. 4.1, is that large part of the solution I proposed requires changing of the syntax for the policy definition, and this is something we should do now (i.e. still at an early rc1 stage, rather than later).

This repeatedly come during rc1 stage. Some choices turns out to be not so good only at broader testing phase. In our current workflow, only bugfixes are allowed after releasing rc1. Specifically, new features or API changes are not allowed. While in practice we make exceptions to this rule, it is not the right thing to do. Maybe we need to bring alpha/beta stages back (or just one beta)?

Back to the topic:

1. Mark fedora-25-dvm with globally-available-dispvm tag (or any other sensible name).
2. Use this policy:

# Allow any VM to use standard DispVMs:
$anyvm $dispvm:$tag:globally-available-dispvm allow

# Allow personal VMs to start DispVMs created by the user via guidom:
$tag:created-by-guidom $dispvm:$tag:created-by-guidom allow

# Allow corpo-managed VMs to start DispVMs based on corpo-owned AppVMs:
$tag:created-by-mgmt-corpo $dispvm:$tag:created-by-mgmt-corpo allow

# Forbid any other combinations (redundant, but for completeness)
$anyvm $anyvm deny

The above will enforce the right policy, regardless of default_dispvm property (either AppVM or global).
Note that I haven't used here neither $dispvm:$anyvm, nor bare $dispvm.

[rootkovska]

So, we agree that we want to add ability for DispVM specification via tags and that we want to do that before rc2.

Even thought I originally proposed the $dispvm:$tag syntax (in the original email thread), I was somehow convinced by Wojtek that it would be trickier in implementation than the "flat" syntax of $dispvm_tagged (or $dispvm$tag maybe?) But I won't fight too hard for this ;)

I agree it's desirable to try to use tags instead of what I proposed above as $dispvm_{system,appvm}_default. So, we need to agree on the policy on tags inheritance for the DispVMs. From what Marek proposed, I'm inclined to say #2 (inherit from the AppVM template), as this is the simplest and most static case. Also this is what Marek used in the snipped above.

[rootkovska]

So, we agree that we want to add ability for DispVM specification via tags and that we want to do that before rc2.

Even thought I originally proposed the $dispvm:$tag syntax (in the original email thread), I was somehow convinced by Wojtek that it would be trickier in implementation than the "flat" syntax of $dispvm_tagged (or $dispvm$tag maybe?) But I won't fight too hard for this ;)

I agree it's desirable to try to use tags instead of what I proposed above as $dispvm_{system,appvm}_default. So, we need to agree on the policy on tags inheritance for the DispVMs. From what Marek proposed, I'm inclined to say #2 (inherit from the AppVM template), as this is the simplest and most static case. Also this is what Marek used in the snipped above.

[marmarek]

Even thought I originally proposed the $dispvm:$tag syntax (in the original email thread), I was somehow convinced by Wojtek that it would be trickier in implementation than the "flat" syntax of $dispvm_tagged (or $dispvm$tag maybe?) But I won't fight too hard for this ;)

I've just implemented $dispvm:$tag. The code treats $dispvm:$tag: as yet another prefix (there is no nested parsing of content of $dispvm: tag) - specifically to avoid what Wojtek feared. Changing this to $dispvm_tagged: would be a simple find&replace, but IMO $dispvm:$tag: is nicer logically and visually.

I've checked (using our test suite) that this change do not break any existing syntax. As it should be.

[marmarek]

Even thought I originally proposed the $dispvm:$tag syntax (in the original email thread), I was somehow convinced by Wojtek that it would be trickier in implementation than the "flat" syntax of $dispvm_tagged (or $dispvm$tag maybe?) But I won't fight too hard for this ;)

I've just implemented $dispvm:$tag. The code treats $dispvm:$tag: as yet another prefix (there is no nested parsing of content of $dispvm: tag) - specifically to avoid what Wojtek feared. Changing this to $dispvm_tagged: would be a simple find&replace, but IMO $dispvm:$tag: is nicer logically and visually.

I've checked (using our test suite) that this change do not break any existing syntax. As it should be.

[qubesos-bot]

Automated announcement from builder-github

The package qubes-core-dom0-4.0.7-1.fc25 has been pushed to the r4.0 testing repository for dom0.
To test this update, please install it with the following command:

sudo qubes-dom0-update --enablerepo=qubes-dom0-current-testing

Changes included in this update

[qubesos-bot]

Automated announcement from builder-github

The package qubes-core-dom0-4.0.7-1.fc25 has been pushed to the r4.0 testing repository for dom0.
To test this update, please install it with the following command:

sudo qubes-dom0-update --enablerepo=qubes-dom0-current-testing

Changes included in this update

[qubesos-bot]

Automated announcement from builder-github

The package qubes-core-dom0-4.0.11-1.fc25 has been pushed to the r4.0 stable repository for dom0.
To install this update, please use the standard update command:

sudo qubes-dom0-update

Or update dom0 via Qubes Manager.

Changes included in this update

[qubesos-bot]

Automated announcement from builder-github

The package qubes-core-dom0-4.0.11-1.fc25 has been pushed to the r4.0 stable repository for dom0.
To install this update, please use the standard update command:

sudo qubes-dom0-update

Or update dom0 via Qubes Manager.

Changes included in this update