Re: [libvirt] [PATCH v2 4/5] apparmor: allow qemu-smb access in /tmp
On Wed, Aug 15, 2018 at 7:35 PM Jamie Strandboge
wrote:
> On Tue, 2018-08-14 at 08:18 +0200, Christian Ehrhardt wrote:
> > The samba feature of qemu will place the samba config file in
> > /tmp/qemu-smb..
> >
> > But at least it has a predictable path identifying qemu-smb feature
> > itself by an infix in the path.
> >
> > This is a compromise of security and usability as the "owner"
> > restriction
> > will not protect guests among each other.
> > Therefore the rule added makes the feature usable, but does not allow
> > cross guest protection.
> >
> > Core issue is, that it is currently impossible to predict the PID
> > which would
> > follow "qemu-smb-", but long term, once the samba feature would be
> > exposed in
> > guest XML we'd prefer a virt-aa-helper based solution that can render
> > the
> > samba rule on demand and with a custom PID into the per guest
> > profile.
> >
> > But the same is true for manual user overrides for this feature as
> > well,
> > they can neither predict the PID, nor have a local include per-guest.
> > Thereby
> > punting this to the user to add the rule later will not make it
> > safer, but
> > only less usable.
>
> While the facts are true, there is something omitted and I come to a
> different conclusion. It is true that the pid is unpredictable, but
> with the 'owner /{,var/}tmp/**/ r,' rule, it is easy for a confined
> process to find the directories. Also, the rule 'owner /tmp/qemu-
> smb.*/{,**} rw,' (below) allows writing /tmp/qemu-smb.*/ so symlink
> attacks are possible by a rogue VM against other VMs. Rogue VMs could
> also use the directory in coordinated file sharing (but I mention this
> only for completeness, not as an objection, since there are other rules
> in the policy that 'overlap' and allow this sort of thing).
>
> It is definitely a usability improvement to include the rule, but I
> disagree that it isn't safer without it-- your addition applies to all
> libvirt/apparmor users, many of whom will not use the smb feature that
> isn't supported by the domain xml. Now, you could argue that users that
> never use the smb feature aren't affected by the proposed global rule
> (which is true), but omitting this patch, users can add the glob rule
> to the per-VM site policy in /etc/apparmor.d/libvirt/libvirt- for
> only the VMs they need it. This might be useful if they, for example,
> have one trusted VM that uses the smb feature and other untrusted VMs
> that do not. With the global rule, the untrusted VMs have access by
> default.
>
> These concerns are somewhat contrived and I'm ambivalent about the
> addition, but it bothers me that we are adding a rule for a use case
> that isn't directly supported by libvirt. +0 as is.
>
Thanks for laying out the security concerns on this in detail.
I agree, this might be too much for a general rule and has to stay out of
the abstraction.
We'd have to wait for the feature to be supported by libvirt to fully
support it.
Even then we might not have the pid at the time the rule is created since
qemu was not yet spawned at that point.
> If you can demonstrate that qemu guards against symlink attacks on the
> dir and is otherwise safe from attack (eg, open(..., O_CREAT|O_EXCL)
> followed by unlink(...)), etc, etc then I could be persuaded to give a
> +1.
>
I can't, therefore I'll abandon this change for now.
>
> > Signed-off-by: Christian Ehrhardt
> > ---
> > examples/apparmor/libvirt-qemu | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/examples/apparmor/libvirt-qemu
> > b/examples/apparmor/libvirt-qemu
> > index 6971d3db03..350b13b824 100644
> > --- a/examples/apparmor/libvirt-qemu
> > +++ b/examples/apparmor/libvirt-qemu
> > @@ -191,6 +191,11 @@
> ># want more unique paths per rule.
> >/{,var/}tmp/ r,
> >owner /{,var/}tmp/**/ r,
> > + # allow qemu smb feature specific path with write access
> > + # TODO: This is a compromise between security and usability - once
> > e.g. samba
> > + # would be expressed in libvirt XML it should be added on demand
> > via
> > + # virt-aa-helper instead.
> > + owner /tmp/qemu-smb.*/{,**} rw,
> >
> ># for file-posix getting limits since 9103f1ce
> >/sys/devices/**/block/*/queue/max_segments r,
> --
> Jamie Strandboge | http://www.canonical.com
--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/5] apparmor: allow qemu-smb access in /tmp
On Tue, 2018-08-14 at 08:18 +0200, Christian Ehrhardt wrote:
> The samba feature of qemu will place the samba config file in
> /tmp/qemu-smb..
>
> But at least it has a predictable path identifying qemu-smb feature
> itself by an infix in the path.
>
> This is a compromise of security and usability as the "owner"
> restriction
> will not protect guests among each other.
> Therefore the rule added makes the feature usable, but does not allow
> cross guest protection.
>
> Core issue is, that it is currently impossible to predict the PID
> which would
> follow "qemu-smb-", but long term, once the samba feature would be
> exposed in
> guest XML we'd prefer a virt-aa-helper based solution that can render
> the
> samba rule on demand and with a custom PID into the per guest
> profile.
>
> But the same is true for manual user overrides for this feature as
> well,
> they can neither predict the PID, nor have a local include per-guest.
> Thereby
> punting this to the user to add the rule later will not make it
> safer, but
> only less usable.
While the facts are true, there is something omitted and I come to a
different conclusion. It is true that the pid is unpredictable, but
with the 'owner /{,var/}tmp/**/ r,' rule, it is easy for a confined
process to find the directories. Also, the rule 'owner /tmp/qemu-
smb.*/{,**} rw,' (below) allows writing /tmp/qemu-smb.*/ so symlink
attacks are possible by a rogue VM against other VMs. Rogue VMs could
also use the directory in coordinated file sharing (but I mention this
only for completeness, not as an objection, since there are other rules
in the policy that 'overlap' and allow this sort of thing).
It is definitely a usability improvement to include the rule, but I
disagree that it isn't safer without it-- your addition applies to all
libvirt/apparmor users, many of whom will not use the smb feature that
isn't supported by the domain xml. Now, you could argue that users that
never use the smb feature aren't affected by the proposed global rule
(which is true), but omitting this patch, users can add the glob rule
to the per-VM site policy in /etc/apparmor.d/libvirt/libvirt- for
only the VMs they need it. This might be useful if they, for example,
have one trusted VM that uses the smb feature and other untrusted VMs
that do not. With the global rule, the untrusted VMs have access by
default.
These concerns are somewhat contrived and I'm ambivalent about the
addition, but it bothers me that we are adding a rule for a use case
that isn't directly supported by libvirt. +0 as is.
If you can demonstrate that qemu guards against symlink attacks on the
dir and is otherwise safe from attack (eg, open(..., O_CREAT|O_EXCL)
followed by unlink(...)), etc, etc then I could be persuaded to give a
+1.
> Signed-off-by: Christian Ehrhardt
> ---
> examples/apparmor/libvirt-qemu | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index 6971d3db03..350b13b824 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -191,6 +191,11 @@
># want more unique paths per rule.
>/{,var/}tmp/ r,
>owner /{,var/}tmp/**/ r,
> + # allow qemu smb feature specific path with write access
> + # TODO: This is a compromise between security and usability - once
> e.g. samba
> + # would be expressed in libvirt XML it should be added on demand
> via
> + # virt-aa-helper instead.
> + owner /tmp/qemu-smb.*/{,**} rw,
>
># for file-posix getting limits since 9103f1ce
>/sys/devices/**/block/*/queue/max_segments r,
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 4/5] apparmor: allow qemu-smb access in /tmp
The samba feature of qemu will place the samba config file in
/tmp/qemu-smb..
But at least it has a predictable path identifying qemu-smb feature
itself by an infix in the path.
This is a compromise of security and usability as the "owner" restriction
will not protect guests among each other.
Therefore the rule added makes the feature usable, but does not allow
cross guest protection.
Core issue is, that it is currently impossible to predict the PID which would
follow "qemu-smb-", but long term, once the samba feature would be exposed in
guest XML we'd prefer a virt-aa-helper based solution that can render the
samba rule on demand and with a custom PID into the per guest profile.
But the same is true for manual user overrides for this feature as well,
they can neither predict the PID, nor have a local include per-guest. Thereby
punting this to the user to add the rule later will not make it safer, but
only less usable.
Signed-off-by: Christian Ehrhardt
---
examples/apparmor/libvirt-qemu | 5 +
1 file changed, 5 insertions(+)
diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index 6971d3db03..350b13b824 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -191,6 +191,11 @@
# want more unique paths per rule.
/{,var/}tmp/ r,
owner /{,var/}tmp/**/ r,
+ # allow qemu smb feature specific path with write access
+ # TODO: This is a compromise between security and usability - once e.g. samba
+ # would be expressed in libvirt XML it should be added on demand via
+ # virt-aa-helper instead.
+ owner /tmp/qemu-smb.*/{,**} rw,
# for file-posix getting limits since 9103f1ce
/sys/devices/**/block/*/queue/max_segments r,
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
