security: Add support for SUSE edk2 firmware paths

[PATCH] security: Add support for SUSE edk2 firmware paths

Posted by Jim Fehlig 1 year, 2 months ago

SUSE installs edk2 firmwares for both x86_64 and aarch64 in /usr/share/qemu.
Add support for this path in virt-aa-helper and allow locking files within
the path in the libvirt qemu abstraction.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---

FYI, I'm fine maintaining this patch downstream if such distro-specific
change is unwanted upstream. I've already maintained the virt-aa-helper
hunk for several years.

 src/security/apparmor/libvirt-qemu | 2 +-
 src/security/virt-aa-helper.c      | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu
index d0289b8943..9af1333b22 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -91,7 +91,7 @@
   /usr/share/proll/** r,
   /usr/share/qemu-efi/** r,
   /usr/share/qemu-kvm/** r,
-  /usr/share/qemu/** r,
+  /usr/share/qemu/** rk,
   /usr/share/seabios/** r,
   /usr/share/sgabios/** r,
   /usr/share/slof/** r,
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index f6c9703db6..d65d459850 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -481,6 +481,7 @@ valid_path(const char *path, const bool readonly)
         "/usr/share/AAVMF/",                 /* for AAVMF images */
         "/usr/share/qemu-efi/",              /* for AAVMF images */
         "/usr/share/qemu-efi-aarch64/",      /* for AAVMF images */
+        "/usr/share/qemu/",                  /* SUSE path for OVMF and AAVMF images */
         "/usr/lib/u-boot/",                  /* u-boot loaders for qemu */
         "/usr/lib/riscv64-linux-gnu/opensbi" /* RISC-V SBI implementation */
     };
-- 
2.39.2

Re: [PATCH] security: Add support for SUSE edk2 firmware paths

Posted by Andrea Bolognani 1 year, 2 months ago

On Thu, Feb 23, 2023 at 11:13:28AM -0700, Jim Fehlig wrote:
> +++ b/src/security/apparmor/libvirt-qemu
> @@ -91,7 +91,7 @@
>    /usr/share/proll/** r,
>    /usr/share/qemu-efi/** r,
>    /usr/share/qemu-kvm/** r,
> -  /usr/share/qemu/** r,
> +  /usr/share/qemu/** rk,
>    /usr/share/seabios/** r,
>    /usr/share/sgabios/** r,
>    /usr/share/slof/** r,
> +++ b/src/security/virt-aa-helper.c
> @@ -481,6 +481,7 @@ valid_path(const char *path, const bool readonly)
>          "/usr/share/AAVMF/",                 /* for AAVMF images */
>          "/usr/share/qemu-efi/",              /* for AAVMF images */
>          "/usr/share/qemu-efi-aarch64/",      /* for AAVMF images */
> +        "/usr/share/qemu/",                  /* SUSE path for OVMF and AAVMF images */
>          "/usr/lib/u-boot/",                  /* u-boot loaders for qemu */
>          "/usr/lib/riscv64-linux-gnu/opensbi" /* RISC-V SBI implementation */

Having these files in /usr/share/qemu directly looks... Kinda
sketchy? That directory should belong to the QEMU package. Compare
with how all the other paths listed here point to directories that
are specific to the firmware at hand.

I don't think this really opens up any attack vectors, so

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

but perhaps it would be a good idea to consider migrating edk2 images
to their own directory long term?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] security: Add support for SUSE edk2 firmware paths

Posted by Jim Fehlig 1 year, 2 months ago

On 3/2/23 07:43, Andrea Bolognani wrote:
> On Thu, Feb 23, 2023 at 11:13:28AM -0700, Jim Fehlig wrote:
>> +++ b/src/security/apparmor/libvirt-qemu
>> @@ -91,7 +91,7 @@
>>     /usr/share/proll/** r,
>>     /usr/share/qemu-efi/** r,
>>     /usr/share/qemu-kvm/** r,
>> -  /usr/share/qemu/** r,
>> +  /usr/share/qemu/** rk,
>>     /usr/share/seabios/** r,
>>     /usr/share/sgabios/** r,
>>     /usr/share/slof/** r,
>> +++ b/src/security/virt-aa-helper.c
>> @@ -481,6 +481,7 @@ valid_path(const char *path, const bool readonly)
>>           "/usr/share/AAVMF/",                 /* for AAVMF images */
>>           "/usr/share/qemu-efi/",              /* for AAVMF images */
>>           "/usr/share/qemu-efi-aarch64/",      /* for AAVMF images */
>> +        "/usr/share/qemu/",                  /* SUSE path for OVMF and AAVMF images */
>>           "/usr/lib/u-boot/",                  /* u-boot loaders for qemu */
>>           "/usr/lib/riscv64-linux-gnu/opensbi" /* RISC-V SBI implementation */
> 
> Having these files in /usr/share/qemu directly looks... Kinda
> sketchy? That directory should belong to the QEMU package. Compare
> with how all the other paths listed here point to directories that
> are specific to the firmware at hand.
> 
> I don't think this really opens up any attack vectors, so

Agree. FYI, I don't know the history behind choosing that location. Probably 
because it contained other firmware files.

>    Reviewed-by: Andrea Bolognani <abologna@redhat.com>
> 
> but perhaps it would be a good idea to consider migrating edk2 images
> to their own directory long term?

I think so. A task for the future, when we have a dedicated edk2 maintainer.

Regards,
Jim

Re: [PATCH] security: Add support for SUSE edk2 firmware paths

Posted by Michal Prívozník 1 year, 2 months ago

On 2/23/23 19:13, Jim Fehlig wrote:
> SUSE installs edk2 firmwares for both x86_64 and aarch64 in /usr/share/qemu.
> Add support for this path in virt-aa-helper and allow locking files within
> the path in the libvirt qemu abstraction.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
> 
> FYI, I'm fine maintaining this patch downstream if such distro-specific
> change is unwanted upstream. I've already maintained the virt-aa-helper
> hunk for several years.

I think it makes sense to have it upstream. I too try to upstream gentoo
downstream patches (unless they are very gentoo specific).

> 
>  src/security/apparmor/libvirt-qemu | 2 +-
>  src/security/virt-aa-helper.c      | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal