[libvirt] Various apparmor related changes (part 1)
Over the years there have been a bunch of changes to the apparmor profiles and/or virt-aa-helper which have been carried in Debian/Ubuntu but never made it upstream. In an attempt to clean this up and generally improve the apparmor based environments, we (Christian and I) went over the changes, cleaned out cruft as much as possible and would be sending out hunks of changes to this list for upstream inclusion. I hope doing multiple but smaller rounds of submissions will make it simpler to get those reviewed and hopefully accepted. This first batch contains a mix of changes from Debian and Ubuntu. Thanks, Stefan
From: Serge Hallyn <serge.hallyn@ubuntu.com> Just because a disk element only requests read access doesn't mean there may not be another readwrite request. Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bugs/1554031 Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- src/security/virt-aa-helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5f5d1cd..d976a00 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -888,11 +888,11 @@ add_file_path(virDomainDiskDefPtr disk, if (depth == 0) { if (disk->src->readonly) - ret = vah_add_file(buf, path, "r"); + ret = vah_add_file(buf, path, "R"); else ret = vah_add_file(buf, path, "rw"); } else { - ret = vah_add_file(buf, path, "r"); + ret = vah_add_file(buf, path, "R"); } if (ret != 0) -- 2.7.4
On Mon, May 15, 2017 at 03:23:10PM +0200, Stefan Bader wrote:
From: Serge Hallyn <serge.hallyn@ubuntu.com>
Just because a disk element only requests read access doesn't mean there may not be another readwrite request.
Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bugs/1554031
The URL is wrong (drop the "ubuntu" part. From the bug report this looks like a workaround: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1554031/comments/8 or am I misreading this? Shouldn't we only change to 'w' on blockcommit? Cheers -- Guido
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- src/security/virt-aa-helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5f5d1cd..d976a00 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -888,11 +888,11 @@ add_file_path(virDomainDiskDefPtr disk,
if (depth == 0) { if (disk->src->readonly) - ret = vah_add_file(buf, path, "r"); + ret = vah_add_file(buf, path, "R"); else ret = vah_add_file(buf, path, "rw"); } else { - ret = vah_add_file(buf, path, "r"); + ret = vah_add_file(buf, path, "R"); }
if (ret != 0) -- 2.7.4
On 15.05.2017 17:48, Guido Günther wrote:
On Mon, May 15, 2017 at 03:23:10PM +0200, Stefan Bader wrote:
From: Serge Hallyn <serge.hallyn@ubuntu.com>
Just because a disk element only requests read access doesn't mean there may not be another readwrite request.
Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bugs/1554031
The URL is wrong (drop the "ubuntu" part. From the bug report this looks like a workaround:
Darn, thanks for catching this.
https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1554031/comments/8
or am I misreading this? Shouldn't we only change to 'w' on blockcommit?
As I understand it, the 'r' would implicitly add a write deny rule, so it is not possible to override that. With the 'R' notation only a rule allowing read is added. Which allows to change to change to 'w' on blockcommit. -Stefan
Cheers -- Guido
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- src/security/virt-aa-helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5f5d1cd..d976a00 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -888,11 +888,11 @@ add_file_path(virDomainDiskDefPtr disk,
if (depth == 0) { if (disk->src->readonly) - ret = vah_add_file(buf, path, "r"); + ret = vah_add_file(buf, path, "R"); else ret = vah_add_file(buf, path, "rw"); } else { - ret = vah_add_file(buf, path, "r"); + ret = vah_add_file(buf, path, "R"); }
if (ret != 0) -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, May 15, 2017 at 06:07:12PM +0200, Stefan Bader wrote:
On 15.05.2017 17:48, Guido Günther wrote:
On Mon, May 15, 2017 at 03:23:10PM +0200, Stefan Bader wrote:
From: Serge Hallyn <serge.hallyn@ubuntu.com>
Just because a disk element only requests read access doesn't mean there may not be another readwrite request.
Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bugs/1554031
The URL is wrong (drop the "ubuntu" part. From the bug report this looks like a workaround:
Darn, thanks for catching this.
https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1554031/comments/8
or am I misreading this? Shouldn't we only change to 'w' on blockcommit?
As I understand it, the 'r' would implicitly add a write deny rule, so it is not possible to override that. With the 'R' notation only a rule allowing read is added. Which allows to change to change to 'w' on blockcommit.
But if we aim for a profile replace on blockcommit [1] the would't matter since the whole profile would get replaced, wouldn't it? Cheers, -- Guido [1] I have not checked that that profile replace currenlty happens with libvirt I'm trying to figure out how we want it to work and then fix the right place.
-Stefan
Cheers -- Guido
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- src/security/virt-aa-helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5f5d1cd..d976a00 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -888,11 +888,11 @@ add_file_path(virDomainDiskDefPtr disk,
if (depth == 0) { if (disk->src->readonly) - ret = vah_add_file(buf, path, "r"); + ret = vah_add_file(buf, path, "R"); else ret = vah_add_file(buf, path, "rw"); } else { - ret = vah_add_file(buf, path, "r"); + ret = vah_add_file(buf, path, "R"); }
if (ret != 0) -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, May 19, 2017 at 10:03 AM, Guido Günther <agx@sigxcpu.org> wrote:
But if we aim for a profile replace on blockcommit [1] the would't matter since the whole profile would get replaced, wouldn't it?
Since this is based on [1][2] looping in Cédric here to share some old explaiantions. See especially [1] for some reasoning for 'R' in general. [1]: http://libvirt.org/git/?p=libvirt.git;a=commit;h=c726af2d5a2248f0dad01201b2f... [2]: http://libvirt.org/git/?p=libvirt.git;a=commit;h=cedd2ab28262db62976b351dbf2... -- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd
Hi Christian, On Fri, May 19, 2017 at 11:18:18AM +0200, Christian Ehrhardt wrote:
On Fri, May 19, 2017 at 10:03 AM, Guido Günther <agx@sigxcpu.org> wrote:
But if we aim for a profile replace on blockcommit [1] the would't matter since the whole profile would get replaced, wouldn't it?
Since this is based on [1][2] looping in Cédric here to share some old explaiantions. See especially [1] for some reasoning for 'R' in general.
[1]: http://libvirt.org/git/?p=libvirt.git;a=commit;h=c726af2d5a2248f0dad01201b2f... [2]: http://libvirt.org/git/?p=libvirt.git;a=commit;h=cedd2ab28262db62976b351dbf2...
Thanks for digging these out again. I know the reason for 'R' but it seems for block commit we want s.th. different namely replacing the profile with a 'w' when replacing the profile. Cheers, -- Guido
Hi Christian, On Fri, 2017-05-19 at 11:18 +0200, Christian Ehrhardt wrote:
On Fri, May 19, 2017 at 10:03 AM, Guido Günther <agx@sigxcpu.org> wrote:
But if we aim for a profile replace on blockcommit [1] the would't matter since the whole profile would get replaced, wouldn't it?
Since this is based on [1][2] looping in Cédric here to share some old explaiantions. See especially [1] for some reasoning for 'R' in general.
[1]: http://libvirt.org/git/?p=libvirt.git;a=commit;h=c726af2d5a2248f0dad01201b2f... [2]: http://libvirt.org/git/?p=libvirt.git;a=commit;h=cedd2ab28262db62976b351dbf2...
Sadly the bug report isn't public since it has been reported again SLES. But here is the description of the bug that motivated that fix: ------------------ %< ------------------ Steps to reproduce: * run virt-sandbox /bin/sh as root Expected result: Run a shell in a qemu domain, apparmor enforced Actual result: Domain fails to start After some more debugging it happens that the problem is caused by <filesystem type='mount' accessmode='passthrough'> <source dir='/'/> <target dir='host_root'/> <readonly/> </filesystem> Since commit http://libvirt.org/git/?p=libvirt.git;a=commit;h=d0d4b8ad76d3e8a859ee90701a2..., virt-aa-helper generates a "deny /** w" rule in such cases that takes precedence over the allow rules. This has several effects: * It hides the DENIED/ALLOWED apparmor log entries * It prevents qemu to write to the log file, /dev/ptmx and other important files to run the domain. To see the rules, add the audit flag to /etc/apparmor.d/libvirt/TEMPLATE.qemu file and rerun virt-sandbox. ------------------ %< ------------------ Hi hope this will answer your questions -- Cedric
On Fri, May 19, 2017 at 11:18:18AM +0200, Christian Ehrhardt wrote:
On Fri, May 19, 2017 at 10:03 AM, Guido Günther <agx@sigxcpu.org> wrote:
But if we aim for a profile replace on blockcommit [1] the would't matter since the whole profile would get replaced, wouldn't it?
Since this is based on [1][2] looping in Cédric here to share some old explaiantions. See especially [1] for some reasoning for 'R' in general.
[1]: http://libvirt.org/git/?p=libvirt.git;a=commit;h=c726af2d5a2248f0dad01201b2f... [2]: http://libvirt.org/git/?p=libvirt.git;a=commit;h=cedd2ab28262db62976b351dbf2...
Let me try to explain why I don't consider this to be a proper fix: virsh blockcommit is invoked this leads to: 1.) qemuDomainBlockCommit -> 2.) qemuDomainDiskChainElementPrepare -> 3.) qemuSecuritySetImageLabel -> 4.) AppArmorSetSecurityImageLabel (triggers profile reload only) -> 5.) virt-aa-helper does the profile reload -> 6.) failure since the image has an explicit deny rule The path in question tries to fix this at 5.) by not adding a deny write rule at all but the place to fix this is 4.) since AppArmorSetSecurityImageLabel does not take the virStorageSourcePtr src element into account to create a virDomainDefPtr based on def that marks the image in question as 'rw' but "only" reloads the profile. Cheers, -- Guido
On 19.05.2017 13:13, Guido Günther wrote:
On Fri, May 19, 2017 at 11:18:18AM +0200, Christian Ehrhardt wrote:
On Fri, May 19, 2017 at 10:03 AM, Guido Günther <agx@sigxcpu.org> wrote:
But if we aim for a profile replace on blockcommit [1] the would't matter since the whole profile would get replaced, wouldn't it?
Since this is based on [1][2] looping in Cédric here to share some old explaiantions. See especially [1] for some reasoning for 'R' in general.
[1]: http://libvirt.org/git/?p=libvirt.git;a=commit;h=c726af2d5a2248f0dad01201b2f... [2]: http://libvirt.org/git/?p=libvirt.git;a=commit;h=cedd2ab28262db62976b351dbf2...
Let me try to explain why I don't consider this to be a proper fix:
virsh blockcommit is invoked this leads to:
1.) qemuDomainBlockCommit -> 2.) qemuDomainDiskChainElementPrepare -> 3.) qemuSecuritySetImageLabel -> 4.) AppArmorSetSecurityImageLabel (triggers profile reload only) -> 5.) virt-aa-helper does the profile reload -> 6.) failure since the image has an explicit deny rule
The path in question tries to fix this at 5.) by not adding a deny write rule at all but the place to fix this is 4.) since AppArmorSetSecurityImageLabel does not take the virStorageSourcePtr src element into account to create a virDomainDefPtr based on def that marks the image in question as 'rw' but "only" reloads the profile.
Ok, I think we got the drift but need more time to ponder about that. Christian created a tracking bug for us and I move it to the needs-more-thinking section. -Stefan
Cheers, -- Guido
From: Simon McVittie <smcv@debian.org> The split firmware and variables files introduced by https://bugs.debian.org/764918 are in a different directory for some reason. Let the virtual machine read both. Extended by Christian Ehrhardt to generalize FW test (simplifies additional testing on firmware files in future). Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/libvirt-qemu | 1 + src/security/virt-aa-helper.c | 1 + tests/virt-aa-helper-test | 24 ++++++++++++++++-------- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index a9020aa..e0988bb 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -70,6 +70,7 @@ /usr/share/vgabios/** r, /usr/share/seabios/** r, /usr/share/ovmf/** r, + /usr/share/OVMF/** r, # access PKI infrastructure /etc/pki/libvirt-vnc/** r, diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index d976a00..dd166c2 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -512,6 +512,7 @@ valid_path(const char *path, const bool readonly) "/vmlinuz", "/initrd", "/initrd.img", + "/usr/share/OVMF/", /* for OVMF images */ "/usr/share/ovmf/" /* for OVMF images */ }; /* override the above with these */ diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index 68e9399..73f3080 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -145,6 +145,20 @@ testme() { fi } +testfw() { + title="$1" + fwpath="$2" + + if [ -f "$fwpath" ]; then + sed -e "s,###UUID###,$uuid,g" \ + -e "s,###DISK###,$disk1,g" \ + -e "s,</os>,<loader readonly='yes' type='pflash'>$fwpath</loader></os>,g" "$template_xml" > "$test_xml" + testme "0" "$title" "-r -u $valid_uuid" "$test_xml" + else + echo "Skipping FW $title test. Could not find $fwpath" + fi +} + # Expected failures echo "Expected failures:" >$output testme "1" "invalid arg" "-z" @@ -291,14 +305,8 @@ sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</os>,<kernel>$tm touch "$tmpdir/kernel" testme "0" "kernel" "-r -u $valid_uuid" "$test_xml" -if [ -f /usr/share/ovmf/OVMF.fd ]; then - sed -e "s,###UUID###,$uuid,g" \ - -e "s,###DISK###,$disk1,g" \ - -e "s,</os>,<loader readonly='yes' type='pflash'>/usr/share/ovmf/OVMF.fd</loader></os>,g" "$template_xml" > "$test_xml" - testme "0" "ovmf" "-r -u $valid_uuid" "$test_xml" -else - echo "Skipping OVMF test. Could not find /usr/share/ovmf/OVMF.fd" -fi +testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd" +testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd" sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</os>,<initrd>$tmpdir/initrd</initrd></os>,g" "$template_xml" > "$test_xml" touch "$tmpdir/initrd" -- 2.7.4
On Mon, May 15, 2017 at 03:23:11PM +0200, Stefan Bader wrote:
From: Simon McVittie <smcv@debian.org>
The split firmware and variables files introduced by https://bugs.debian.org/764918 are in a different directory for some reason. Let the virtual machine read both.
Extended by Christian Ehrhardt to generalize FW test (simplifies additional testing on firmware files in future).
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/libvirt-qemu | 1 + src/security/virt-aa-helper.c | 1 + tests/virt-aa-helper-test | 24 ++++++++++++++++-------- 3 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index a9020aa..e0988bb 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -70,6 +70,7 @@ /usr/share/vgabios/** r, /usr/share/seabios/** r, /usr/share/ovmf/** r, + /usr/share/OVMF/** r,
# access PKI infrastructure /etc/pki/libvirt-vnc/** r, diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index d976a00..dd166c2 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -512,6 +512,7 @@ valid_path(const char *path, const bool readonly) "/vmlinuz", "/initrd", "/initrd.img", + "/usr/share/OVMF/", /* for OVMF images */ "/usr/share/ovmf/" /* for OVMF images */ }; /* override the above with these */ diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index 68e9399..73f3080 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -145,6 +145,20 @@ testme() { fi }
+testfw() { + title="$1" + fwpath="$2" + + if [ -f "$fwpath" ]; then + sed -e "s,###UUID###,$uuid,g" \ + -e "s,###DISK###,$disk1,g" \ + -e "s,</os>,<loader readonly='yes' type='pflash'>$fwpath</loader></os>,g" "$template_xml" > "$test_xml" + testme "0" "$title" "-r -u $valid_uuid" "$test_xml" + else + echo "Skipping FW $title test. Could not find $fwpath" + fi +} + # Expected failures echo "Expected failures:" >$output testme "1" "invalid arg" "-z" @@ -291,14 +305,8 @@ sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</os>,<kernel>$tm touch "$tmpdir/kernel" testme "0" "kernel" "-r -u $valid_uuid" "$test_xml"
-if [ -f /usr/share/ovmf/OVMF.fd ]; then - sed -e "s,###UUID###,$uuid,g" \ - -e "s,###DISK###,$disk1,g" \ - -e "s,</os>,<loader readonly='yes' type='pflash'>/usr/share/ovmf/OVMF.fd</loader></os>,g" "$template_xml" > "$test_xml" - testme "0" "ovmf" "-r -u $valid_uuid" "$test_xml" -else - echo "Skipping OVMF test. Could not find /usr/share/ovmf/OVMF.fd" -fi +testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd" +testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd"
sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</os>,<initrd>$tmpdir/initrd</initrd></os>,g" "$template_xml" > "$test_xml" touch "$tmpdir/initrd" -- 2.7.4
ACK. -- Guido
From: William Grant <wgrant@ubuntu.com> Allow access to aarch64 UEFI images. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/libvirt-qemu | 2 ++ src/security/virt-aa-helper.c | 4 +++- tests/virt-aa-helper-test | 2 ++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index e0988bb..89466c9 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -71,6 +71,8 @@ /usr/share/seabios/** r, /usr/share/ovmf/** r, /usr/share/OVMF/** r, + /usr/share/AAVMF/** r, + /usr/share/qemu-efi/** r, # access PKI infrastructure /etc/pki/libvirt-vnc/** r, diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index dd166c2..a2d5c21 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -513,7 +513,9 @@ valid_path(const char *path, const bool readonly) "/initrd", "/initrd.img", "/usr/share/OVMF/", /* for OVMF images */ - "/usr/share/ovmf/" /* for OVMF images */ + "/usr/share/ovmf/", /* for OVMF images */ + "/usr/share/AAVMF/", /* for AAVMF images */ + "/usr/share/qemu-efi/" /* for AAVMF images */ }; /* override the above with these */ const char * const override[] = { diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index 73f3080..51072f6 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -307,6 +307,8 @@ testme "0" "kernel" "-r -u $valid_uuid" "$test_xml" testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd" testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd" +testfw "AAVMF" "/usr/share/AAVMF/AAVMF_CODE.fd" +testfw "qemu-efi" "/usr/share/qemu-efi/QEMU_EFI.fd" sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</os>,<initrd>$tmpdir/initrd</initrd></os>,g" "$template_xml" > "$test_xml" touch "$tmpdir/initrd" -- 2.7.4
On Mon, May 15, 2017 at 03:23:12PM +0200, Stefan Bader wrote:
From: William Grant <wgrant@ubuntu.com>
Allow access to aarch64 UEFI images.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/libvirt-qemu | 2 ++ src/security/virt-aa-helper.c | 4 +++- tests/virt-aa-helper-test | 2 ++ 3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index e0988bb..89466c9 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -71,6 +71,8 @@ /usr/share/seabios/** r, /usr/share/ovmf/** r, /usr/share/OVMF/** r, + /usr/share/AAVMF/** r, + /usr/share/qemu-efi/** r,
# access PKI infrastructure /etc/pki/libvirt-vnc/** r, diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index dd166c2..a2d5c21 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -513,7 +513,9 @@ valid_path(const char *path, const bool readonly) "/initrd", "/initrd.img", "/usr/share/OVMF/", /* for OVMF images */ - "/usr/share/ovmf/" /* for OVMF images */ + "/usr/share/ovmf/", /* for OVMF images */ + "/usr/share/AAVMF/", /* for AAVMF images */ + "/usr/share/qemu-efi/" /* for AAVMF images */ }; /* override the above with these */ const char * const override[] = { diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index 73f3080..51072f6 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -307,6 +307,8 @@ testme "0" "kernel" "-r -u $valid_uuid" "$test_xml"
testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd" testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd" +testfw "AAVMF" "/usr/share/AAVMF/AAVMF_CODE.fd" +testfw "qemu-efi" "/usr/share/qemu-efi/QEMU_EFI.fd"
sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</os>,<initrd>$tmpdir/initrd</initrd></os>,g" "$template_xml" > "$test_xml" touch "$tmpdir/initrd" -- 2.7.4
ACK. -- Guido
From: Felix Geyer <fgeyer@debian.org> Allow access to libnl-3 config files Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 2 ++ 1 file changed, 2 insertions(+) diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index 4a8f197..ee53c2c 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -16,6 +16,8 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { owner @{PROC}/[0-9]*/status r, @{PROC}/filesystems r, + /etc/libnl-3/classid r, + # for hostdev /sys/devices/ r, /sys/devices/** r, -- 2.7.4
On Mon, May 15, 2017 at 03:23:13PM +0200, Stefan Bader wrote:
From: Felix Geyer <fgeyer@debian.org>
Allow access to libnl-3 config files
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index 4a8f197..ee53c2c 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -16,6 +16,8 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { owner @{PROC}/[0-9]*/status r, @{PROC}/filesystems r,
+ /etc/libnl-3/classid r, + # for hostdev /sys/devices/ r, /sys/devices/** r, --
ACK (shipped in a similar form in Debian already). Cheers, -- Guido
From: Felix Geyer <fgeyer@debian.org> Add explicit denies for disk devices to avoid cluttering dmesg with (acceptable) denials. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index ee53c2c..7804b72 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -21,6 +21,11 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { # for hostdev /sys/devices/ r, /sys/devices/** r, + deny /dev/sd* r, + deny /dev/vd* r, + deny /dev/dm-* r, + deny /dev/mapper/ r, + deny /dev/mapper/* r, /usr/{lib,lib64}/libvirt/virt-aa-helper mr, /{usr/,}sbin/apparmor_parser Ux, -- 2.7.4
On Mon, May 15, 2017 at 03:23:14PM +0200, Stefan Bader wrote:
From: Felix Geyer <fgeyer@debian.org>
Add explicit denies for disk devices to avoid cluttering dmesg with (acceptable) denials.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index ee53c2c..7804b72 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -21,6 +21,11 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { # for hostdev /sys/devices/ r, /sys/devices/** r, + deny /dev/sd* r, + deny /dev/vd* r, + deny /dev/dm-* r, + deny /dev/mapper/ r, + deny /dev/mapper/* r,
/usr/{lib,lib64}/libvirt/virt-aa-helper mr, /{usr/,}sbin/apparmor_parser Ux, -- 2.7.4
ACK (shipped in a similar form in Debian already). -- Guido
From: Christian Ehrhardt <christian.ehrhardt@canonical.com> This adds further explicit denies for host devices to silence (acceptable) denial warnings. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index 7804b72..012080c 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -24,6 +24,10 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { deny /dev/sd* r, deny /dev/vd* r, deny /dev/dm-* r, + deny /dev/drbd[0-9]* r, + deny /dev/dasd* r, + deny /dev/nvme* r, + deny /dev/zd[0-9]* r, deny /dev/mapper/ r, deny /dev/mapper/* r, -- 2.7.4
On Mon, May 15, 2017 at 03:23:15PM +0200, Stefan Bader wrote:
From: Christian Ehrhardt <christian.ehrhardt@canonical.com>
This adds further explicit denies for host devices to silence (acceptable) denial warnings.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index 7804b72..012080c 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -24,6 +24,10 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { deny /dev/sd* r, deny /dev/vd* r, deny /dev/dm-* r, + deny /dev/drbd[0-9]* r, + deny /dev/dasd* r, + deny /dev/nvme* r, + deny /dev/zd[0-9]* r, deny /dev/mapper/ r, deny /dev/mapper/* r,
This could IMHO be squashed into the previous patch since it just extends the list. Cheers, -- Guido
On 15.05.2017 17:56, Guido Günther wrote:
On Mon, May 15, 2017 at 03:23:15PM +0200, Stefan Bader wrote:
From: Christian Ehrhardt <christian.ehrhardt@canonical.com>
This adds further explicit denies for host devices to silence (acceptable) denial warnings.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index 7804b72..012080c 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -24,6 +24,10 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { deny /dev/sd* r, deny /dev/vd* r, deny /dev/dm-* r, + deny /dev/drbd[0-9]* r, + deny /dev/dasd* r, + deny /dev/nvme* r, + deny /dev/zd[0-9]* r, deny /dev/mapper/ r, deny /dev/mapper/* r,
This could IMHO be squashed into the previous patch since it just extends the list.
I agree. another case of being not certain how to proceed with changes from different origins. -Stefan
Cheers, -- Guido
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
From: Felix Geyer <fgeyer@debian.org> Local overrides is a feature Debian/Ubuntu libvirt provided for a while. This allows the user to have a non-conffile that he can use to extend the package delivered rules with extra content matching his special case. This change adds the include directives to the apparmor profiles for virt-aa-helper and libvirtd. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 3 +++ examples/apparmor/usr.sbin.libvirtd | 3 +++ 2 files changed, 6 insertions(+) diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index 012080c..93ba74e 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -56,4 +56,7 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { /**.vmdk r, /**.[iI][sS][oO] r, /**/disk{,.*} r, + + # Site-specific additions and overrides. See local/README for details. + #include <local/usr.lib.libvirt.virt-aa-helper> } diff --git a/examples/apparmor/usr.sbin.libvirtd b/examples/apparmor/usr.sbin.libvirtd index 353b039..c37d5ee 100644 --- a/examples/apparmor/usr.sbin.libvirtd +++ b/examples/apparmor/usr.sbin.libvirtd @@ -85,4 +85,7 @@ /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix, } + + # Site-specific additions and overrides. See local/README for details. + #include <local/usr.sbin.libvirtd> } -- 2.7.4
On Mon, 2017-05-15 at 15:23 +0200, Stefan Bader wrote:
From: Felix Geyer <fgeyer@debian.org>
Local overrides is a feature Debian/Ubuntu libvirt provided for a while. This allows the user to have a non-conffile that he can use to extend the package delivered rules with extra content matching his special case.
This change adds the include directives to the apparmor profiles for virt-aa-helper and libvirtd.
I'm fine with this change but it is important to understand that /etc/apparmor.d/local/usr.sbin.libvirtd must exist otherwise the profile will fail to load. In Debian/Ubuntu we use dh_apparmor which takes care of this for us. If this is upstreamed, then wherever install of the profile happens or is documented, then the local changes file needs to also be installed/documented. Other non-deb distributions might not like this extra file, so it is possible this may be a Debian and its derivatives thing....
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 3 +++ examples/apparmor/usr.sbin.libvirtd | 3 +++ 2 files changed, 6 insertions(+)
diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index 012080c..93ba74e 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -56,4 +56,7 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa- helper { /**.vmdk r, /**.[iI][sS][oO] r, /**/disk{,.*} r, + + # Site-specific additions and overrides. See local/README for details. + #include <local/usr.lib.libvirt.virt-aa-helper> } diff --git a/examples/apparmor/usr.sbin.libvirtd b/examples/apparmor/usr.sbin.libvirtd index 353b039..c37d5ee 100644 --- a/examples/apparmor/usr.sbin.libvirtd +++ b/examples/apparmor/usr.sbin.libvirtd @@ -85,4 +85,7 @@ /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix, } + + # Site-specific additions and overrides. See local/README for details. + #include <local/usr.sbin.libvirtd> } -- Jamie Strandboge | http://www.canonical.com
On Mon, 2017-05-15 at 09:28 -0500, Jamie Strandboge wrote:
On Mon, 2017-05-15 at 15:23 +0200, Stefan Bader wrote:
From: Felix Geyer <fgeyer@debian.org>
Local overrides is a feature Debian/Ubuntu libvirt provided for a while. This allows the user to have a non-conffile that he can use to extend the package delivered rules with extra content matching his special case.
This change adds the include directives to the apparmor profiles for virt-aa-helper and libvirtd.
I'm fine with this change but it is important to understand that /etc/apparmor.d/local/usr.sbin.libvirtd must exist otherwise the profile will fail to load. In Debian/Ubuntu we use dh_apparmor which takes care of this for us. If this is upstreamed, then wherever install of the profile happens or is documented, then the local changes file needs to also be installed/documented. Other non-deb distributions might not like this extra file, so it is possible this may be a Debian and its derivatives thing....
Oh heh, I see you adjusted the Makefile.am for this in 08. Thanks! -- Jamie Strandboge | http://www.canonical.com
On 15.05.2017 16:30, Jamie Strandboge wrote:
On Mon, 2017-05-15 at 09:28 -0500, Jamie Strandboge wrote:
On Mon, 2017-05-15 at 15:23 +0200, Stefan Bader wrote:
From: Felix Geyer <fgeyer@debian.org>
Local overrides is a feature Debian/Ubuntu libvirt provided for a while. This allows the user to have a non-conffile that he can use to extend the package delivered rules with extra content matching his special case.
This change adds the include directives to the apparmor profiles for virt-aa-helper and libvirtd.
I'm fine with this change but it is important to understand that /etc/apparmor.d/local/usr.sbin.libvirtd must exist otherwise the profile will fail to load. In Debian/Ubuntu we use dh_apparmor which takes care of this for us. If this is upstreamed, then wherever install of the profile happens or is documented, then the local changes file needs to also be installed/documented. Other non-deb distributions might not like this extra file, so it is possible this may be a Debian and its derivatives thing....
Oh heh, I see you adjusted the Makefile.am for this in 08. Thanks!
Yeah, I guess it could make sense to merge those two changes into one. I was just hesitating initially as the first part came via Debian and the latter is and extension I did. Admittedly it is not completely consistent as I did merge for other things.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Local overrides is a feature Debian/Ubuntu libvirt provided for a while. This allows the user to have a non-conffile that he can use to extend the package delivered rules with extra content matching his special case. This change provides override templates which the user can extend and modifies the makefile template to include those when installing the apparmor profiles. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/Makefile.am | 14 ++++++++++++++ examples/apparmor/local-usr.lib.libvirt.virt-aa-helper | 2 ++ examples/apparmor/local-usr.sbin.libvirtd | 2 ++ 3 files changed, 18 insertions(+) create mode 100644 examples/apparmor/local-usr.lib.libvirt.virt-aa-helper create mode 100644 examples/apparmor/local-usr.sbin.libvirtd diff --git a/examples/Makefile.am b/examples/Makefile.am index 2956e14..16c7bf6 100644 --- a/examples/Makefile.am +++ b/examples/Makefile.am @@ -25,6 +25,8 @@ EXTRA_DIST = \ apparmor/libvirt-lxc \ apparmor/usr.lib.libvirt.virt-aa-helper \ apparmor/usr.sbin.libvirtd \ + apparmor/local-usr.sbin.libvirtd \ + apparmor/local-usr.lib.libvirt.virt-aa-helper \ lxcconvert/virt-lxc-convert \ polkit/libvirt-acl.rules \ $(wildcard $(srcdir)/systemtap/*.stp) \ @@ -74,6 +76,18 @@ apparmor_DATA = \ apparmor/usr.sbin.libvirtd \ $(NULL) +localdir = $(apparmordir)/local +local_DATA = \ + apparmor/local-usr.sbin.libvirtd \ + apparmor/local-usr.lib.libvirt.virt-aa-helper \ + $(NULL) + +install-data-hook: + mv $(DESTDIR)$(localdir)/local-usr.sbin.libvirtd \ + $(DESTDIR)$(localdir)/usr.sbin.libvirtd + mv $(DESTDIR)$(localdir)/local-usr.lib.libvirt.virt-aa-helper \ + $(DESTDIR)$(localdir)/usr.lib.libvirt.virt-aa-helper + abstractionsdir = $(apparmordir)/abstractions abstractions_DATA = \ apparmor/libvirt-qemu \ diff --git a/examples/apparmor/local-usr.lib.libvirt.virt-aa-helper b/examples/apparmor/local-usr.lib.libvirt.virt-aa-helper new file mode 100644 index 0000000..82c9c39 --- /dev/null +++ b/examples/apparmor/local-usr.lib.libvirt.virt-aa-helper @@ -0,0 +1,2 @@ +# Site-specific additions and overrides for usr.lib.libvirt.virt-aa-helper. +# For more details, please see /etc/apparmor.d/local/README. diff --git a/examples/apparmor/local-usr.sbin.libvirtd b/examples/apparmor/local-usr.sbin.libvirtd new file mode 100644 index 0000000..6e19f20 --- /dev/null +++ b/examples/apparmor/local-usr.sbin.libvirtd @@ -0,0 +1,2 @@ +# Site-specific additions and overrides for usr.sbin.libvirtd. +# For more details, please see /etc/apparmor.d/local/README. -- 2.7.4
On Mon, May 15, 2017 at 03:23:17PM +0200, Stefan Bader wrote:
Local overrides is a feature Debian/Ubuntu libvirt provided for a while. This allows the user to have a non-conffile that he can use to extend the package delivered rules with extra content matching his special case.
This change provides override templates which the user can extend and modifies the makefile template to include those when installing the apparmor profiles.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/Makefile.am | 14 ++++++++++++++ examples/apparmor/local-usr.lib.libvirt.virt-aa-helper | 2 ++ examples/apparmor/local-usr.sbin.libvirtd | 2 ++ 3 files changed, 18 insertions(+) create mode 100644 examples/apparmor/local-usr.lib.libvirt.virt-aa-helper create mode 100644 examples/apparmor/local-usr.sbin.libvirtd
diff --git a/examples/Makefile.am b/examples/Makefile.am index 2956e14..16c7bf6 100644 --- a/examples/Makefile.am +++ b/examples/Makefile.am @@ -25,6 +25,8 @@ EXTRA_DIST = \ apparmor/libvirt-lxc \ apparmor/usr.lib.libvirt.virt-aa-helper \ apparmor/usr.sbin.libvirtd \ + apparmor/local-usr.sbin.libvirtd \ + apparmor/local-usr.lib.libvirt.virt-aa-helper \ lxcconvert/virt-lxc-convert \ polkit/libvirt-acl.rules \ $(wildcard $(srcdir)/systemtap/*.stp) \ @@ -74,6 +76,18 @@ apparmor_DATA = \ apparmor/usr.sbin.libvirtd \ $(NULL)
+localdir = $(apparmordir)/local +local_DATA = \ + apparmor/local-usr.sbin.libvirtd \ + apparmor/local-usr.lib.libvirt.virt-aa-helper \ + $(NULL) + +install-data-hook: + mv $(DESTDIR)$(localdir)/local-usr.sbin.libvirtd \ + $(DESTDIR)$(localdir)/usr.sbin.libvirtd + mv $(DESTDIR)$(localdir)/local-usr.lib.libvirt.virt-aa-helper \ + $(DESTDIR)$(localdir)/usr.lib.libvirt.virt-aa-helper + abstractionsdir = $(apparmordir)/abstractions abstractions_DATA = \ apparmor/libvirt-qemu \ diff --git a/examples/apparmor/local-usr.lib.libvirt.virt-aa-helper b/examples/apparmor/local-usr.lib.libvirt.virt-aa-helper new file mode 100644 index 0000000..82c9c39 --- /dev/null +++ b/examples/apparmor/local-usr.lib.libvirt.virt-aa-helper @@ -0,0 +1,2 @@ +# Site-specific additions and overrides for usr.lib.libvirt.virt-aa-helper. +# For more details, please see /etc/apparmor.d/local/README. diff --git a/examples/apparmor/local-usr.sbin.libvirtd b/examples/apparmor/local-usr.sbin.libvirtd new file mode 100644 index 0000000..6e19f20 --- /dev/null +++ b/examples/apparmor/local-usr.sbin.libvirtd @@ -0,0 +1,2 @@ +# Site-specific additions and overrides for usr.sbin.libvirtd. +# For more details, please see /etc/apparmor.d/local/README.
I wonder if this is too much distro speifics? (We're shipping the same in Debian). It should in any case be squashed into the previous commit. Cheers, -- Guido
From: Serge Hallyn <serge.hallyn@ubuntu.com> Add fowner and fsetid to libvirt-qemu profile and add link to 9p file options in virt-aa-helper. Bug-Ubuntu: https://bugs.launchpad.net/bugs/1378434 Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/libvirt-qemu | 4 ++++ src/security/virt-aa-helper.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 89466c9..f04ce04 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -13,6 +13,10 @@ capability setgid, capability setuid, + # for 9p + capability fsetid, + capability fowner, + network inet stream, network inet6 stream, diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a2d5c21..667241b 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1108,7 +1108,7 @@ get_files(vahControl * ctl) /* We don't need to add deny rw rules for readonly mounts, * this can only lead to troubles when mounting / readonly. */ - if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rw", true) != 0) + if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rwl", true) != 0) goto cleanup; } } -- 2.7.4
On Mon, May 15, 2017 at 03:23:18PM +0200, Stefan Bader wrote:
From: Serge Hallyn <serge.hallyn@ubuntu.com>
Add fowner and fsetid to libvirt-qemu profile and add link to 9p file options in virt-aa-helper.
Bug-Ubuntu: https://bugs.launchpad.net/bugs/1378434
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/libvirt-qemu | 4 ++++ src/security/virt-aa-helper.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 89466c9..f04ce04 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -13,6 +13,10 @@ capability setgid, capability setuid,
+ # for 9p + capability fsetid, + capability fowner, + network inet stream, network inet6 stream,
I would put this into a separate patch.
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a2d5c21..667241b 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1108,7 +1108,7 @@ get_files(vahControl * ctl) /* We don't need to add deny rw rules for readonly mounts, * this can only lead to troubles when mounting / readonly. */ - if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rw", true) != 0) + if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rwl", true) != 0)
Given the recent QEMU 9pfs CVS that allowed to access paths outside src.path I would feel better if the rule produces s.th. like link subset src.path/** -> src.path/**, instead of allowing links to /**. Cheers, -- Guido
goto cleanup; } } -- 2.7.4
On 15.05.2017 18:13, Guido Günther wrote:
On Mon, May 15, 2017 at 03:23:18PM +0200, Stefan Bader wrote:
From: Serge Hallyn <serge.hallyn@ubuntu.com>
Add fowner and fsetid to libvirt-qemu profile and add link to 9p file options in virt-aa-helper.
Bug-Ubuntu: https://bugs.launchpad.net/bugs/1378434
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/libvirt-qemu | 4 ++++ src/security/virt-aa-helper.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 89466c9..f04ce04 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -13,6 +13,10 @@ capability setgid, capability setuid,
+ # for 9p + capability fsetid, + capability fowner, + network inet stream, network inet6 stream,
I would put this into a separate patch.
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a2d5c21..667241b 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1108,7 +1108,7 @@ get_files(vahControl * ctl) /* We don't need to add deny rw rules for readonly mounts, * this can only lead to troubles when mounting / readonly. */ - if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rw", true) != 0) + if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rwl", true) != 0)
Given the recent QEMU 9pfs CVS that allowed to access paths outside src.path I would feel better if the rule produces s.th. like
link subset src.path/** -> src.path/**,
instead of allowing links to /**.
I had hoped to gain additional feedback from other people. But will start an updated submission tomorrow. Splitting this one back into the two halves as suggested and merging the other (5+6 and 7+8) together. -Stefan
Cheers, -- Guido
goto cleanup; } } -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
From: Serge Hallyn <serge.hallyn@ubuntu.com> Updates profile to allow running on ppc64el. Bug-Ubuntu: https://bugs.launchpad.net/bugs/1374554 Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- examples/apparmor/libvirt-qemu | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index f04ce04..2791dfc 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -77,6 +77,7 @@ /usr/share/OVMF/** r, /usr/share/AAVMF/** r, /usr/share/qemu-efi/** r, + /usr/share/slof/** r, # access PKI infrastructure /etc/pki/libvirt-vnc/** r, @@ -102,6 +103,7 @@ /usr/bin/qemu-system-or32 rmix, /usr/bin/qemu-system-ppc rmix, /usr/bin/qemu-system-ppc64 rmix, + /usr/bin/qemu-system-ppc64le rmix, /usr/bin/qemu-system-ppcemb rmix, /usr/bin/qemu-system-s390x rmix, /usr/bin/qemu-system-sh4 rmix, @@ -158,3 +160,8 @@ /etc/udev/udev.conf r, /sys/bus/ r, /sys/class/ r, + + # for ppc device-tree access + @{PROC}/device-tree/ r, + @{PROC}/device-tree/** r, + /sys/firmware/devicetree/** r, -- 2.7.4
participants (5)
-
Cedric Bosdonnat -
Christian Ehrhardt -
Guido Günther -
Jamie Strandboge -
Stefan Bader