[libvirt] [PATCH 4/4] apparmor: let AppArmorSetSecurityImageLabel append rules

[Jamie Strandboge]

On Wed, 16 Oct 2019, Christian Ehrhardt wrote:

> There are currently broken use cases, e.g. snapshotting more than one disk at
> once like:
>  $ virsh snapshot-create-as --domain eoan --disk-only --atomic
>    --diskspec vda,snapshot=no  --diskspec vdb,snapshot=no
>    --diskspec vdc,file=/test/disk1.snapshot1.qcow,snapshot=external
>    --diskspec vdd,file=/test/disk2.snapshot1.qcow,snapshot=external
> The command above will iterate from qemuDomainSnapshotCreateDiskActive and
> eventually add /test/disk1.snapshot1.qcow first (appears in the rules)
> to then later add /test/disk2.snapshot1.qcow and while doing so throwing
> away the former rule causing it to fail.
> 
> All other calls to (re)load_profile already use append=true when adding
> rules append=false is only used when restoring rules [1].
> 
> Fix this by letting AppArmorSetSecurityImageLabel use append=true as well.
> 
> Bugs:
> https://bugs.launchpad.net/libvirt/+bug/1845506
> https://bugzilla.redhat.com/show_bug.cgi?id=1746684
> 
> [1]: https://bugs.launchpad.net/libvirt/+bug/1845506/comments/13
> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt at canonical.com>
> ---
>  src/security/security_apparmor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> index 320d69e52a..4dd7ba20b4 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -812,7 +812,7 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
>          return -1;
>      }
>  
> -    return reload_profile(mgr, def, src->path, false);
> +    return reload_profile(mgr, def, src->path, true);

src/security/security_manager.c shows that
virSecurityManagerSetImageLabel() is called to label 'a storage image
with the configured security label'. Based on your report, it seems that
in addition to the underlying disk (vda in this case), it is also called
for each additional disk (ie, vdc and vdd). While your patch to append
makes sense for this scenario, what happens if you update the vm
definition to use 'vde' instead of 'vda', is the vda access removed and
vde added with vdc and vdd remaining? What if we remove vda and replace
it with only vde, are vda, vdc and vdd removed? I fear that making this
change will result in scenarios where the rule is (correctly) added, but
previous rules are not removed.

Can you comment on if this is working correctly? Is it possible to have
tests that demonstrate everything is working as intended?

-- 
Jamie Strandboge             | http://www.canonical.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191119/417ee68d/attachment-0001.sig>