Re: [PATCH] security: AppArmor allow write when os loader readonly=no

[Christian Ehrhardt <christian.ehrhardt@xxxxxxxxxxxxx>]

On Tue, Jul 2, 2024 at 5:02 PM Andrea Bolognani <abologna@xxxxxxxxxx> wrote:

On Fri, Jun 07, 2024 at 09:39:30AM GMT, Andrea Bolognani wrote:
> On Tue, Jun 04, 2024 at 02:20:14PM GMT, Christian Ehrhardt wrote:
> > On Tue, Jun 4, 2024 at 1:17 PM Miroslav Los via Devel <devel@xxxxxxxxxxxxxxxxx> wrote:
> > > -    if (ctl->def->os.loader && ctl->def->os.loader->path)
> > > -        if (vah_add_file(&buf, ctl->def->os.loader->path, "rk") != 0)
> > > +    if (ctl->def->os.loader && ctl->def->os.loader->path) {
> > > +        bool readonly = false;
> > > +        virTristateBoolToBool(ctl->def->os.loader->readonly, &readonly);
> > > +        if (vah_add_file(&buf,
> > > +                         ctl->def->os.loader->path,
> > > +                         readonly ? "rk" : "rwk") != 0)
> >
> > Not tested, but the approach looks totally reasonable and in line with
> > the usual virt-aa-helper handling of such cases.
> >
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@xxxxxxxxxxxxx>
>
> It looks reasonable to me too, but I'd like to see someone other than
> the author take it for a spin. Christian, can you please give it a
> shot? Once we have your Tested-by, I'll happily throw in my
> Reviewed-by and push the patch.

I can currently only easily test that as a patch on top of Ubuntu.
There I'm still on 10.0 planning for the merge of 10.6.
I can apply and build the patch there fine there though.

With that I was trying to look into starting a domain with loader + readonly
set to yes/no. When setting readable="no" I also remove the nvram entry which
is then no more needed and would cause conflicts.

# Before the patch

readonly
root@o:~# virsh dumpxml testdomain | grep OVMF; uuid=$(virsh domuuid testdomain); cat /etc/apparmor.d/libvirt/libvirt-${uuid}.files | grep OVM
    <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE_4M.fd</loader>
    <nvram template='/usr/share/OVMF/OVMF_VARS_4M.fd'>/var/lib/libvirt/nvram/testdomain_VARS.fd</nvram>
  "/usr/share/OVMF/OVMF_CODE_4M.fd" rk,
  deny "/usr/share/OVMF/OVMF_CODE_4M.fd" w,

writable
  <loader readonly='no' type='pflash'>/usr/share/OVMF/OVMF_CODE_4M.fd</loader>
  <!-- <nvram template='/usr/share/OVMF/OVMF_VARS_4M.fd'>/var/lib/libvirt/nvram/testdomain_VARS.fd</nvram> -->
=>
error: internal error: process exited while connecting to monitor: 2024-07-05T10:09:54.914293Z qemu-system-x86_64: Could not open '/usr/share/OVMF/OVMF_CODE_4M.fd': Permission denied
As expected as it would be blocked by appamor


# After the patch
readonly
root@o:~# virsh dumpxml testdomain | grep OVMF; uuid=$(virsh domuuid testdomain); cat /etc/apparmor.d/libvirt/libvirt-${uuid}.files | grep OVM
    <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE_4M.fd</loader>
    <nvram template='/usr/share/OVMF/OVMF_VARS_4M.fd'>/var/lib/libvirt/nvram/testdomain_VARS.fd</nvram>
  "/usr/share/OVMF/OVMF_CODE_4M.fd" rk,
  deny "/usr/share/OVMF/OVMF_CODE_4M.fd" w,

writable
  <loader readonly='no' type='pflash'>/usr/share/OVMF/OVMF_CODE_4M.fd</loader>
  <!-- <nvram template='/usr/share/OVMF/OVMF_VARS_4M.fd'>/var/lib/libvirt/nvram/testdomain_VARS.fd</nvram> -->
=>
error: internal error: cannot load AppArmor profile 'libvirt-0801d8c1-d29a-41b4-afc3-aff78bfe0bb6'

So something is broken in the generated profile now, that is ... not good.
Sadly if it breaks on loading it cleans up and does not leave it behind.
Regenerating the content it would have created along the way ...

$ virsh dumpxml testdomain > verify.xml
/usr/lib/libvirt/virt-aa-helper -r -d -u 'libvirt-0801d8c1-d29a-41b4-afc3-aff78bfe0bb6'  < verify.xml
virt-aa-helper: error: /usr/share/OVMF/OVMF_CODE_4M.fd
virt-aa-helper: error: skipped restricted file
virt-aa-helper: error: invalid VM definition

That is due to the check for valid_path having /usr/share/OVMF/ in
restricted_rw and therefore blocking this. Now the question is,
was this meant to allow using the system files RW - then I think
it is rightfully blocked. Instead I'm going to assume this was meant for separate files.

# TEST ONLY - DO NOT DO THIS
$ cp /usr/share/OVMF/OVMF_CODE_4M.fd /opt/OVMF_CODE_4M.fd
$ chmod 666 /opt/OVMF_CODE_4M.fd

root@o:~# virsh dumpxml testdomain | grep OVMF; uuid=$(virsh domuuid testdomain); cat /etc/apparmor.d/libvirt/libvirt-${uuid}.files | grep OVM
    <loader readonly='no' type='pflash'>/opt/OVMF_CODE_4M.fd</loader>
  "/opt/OVMF_CODE_4M.fd" rwk,

So we can see the code achieves what it is intended to do while not
regressing the former flow.

But it can not be used on the normal paths, that is probably good to avoid
accidentally overwriting these.
But I think along the patch we should hear a little bit more about the use case
to ensure that it has a practical use?

@Miroslav: how exactly would you make use of that now?

Valid once the question above about the use case is answered: Tested-by: Christian Ehrhardt <christian.ehrhardt@xxxxxxxxxxxxx>

 

Unfortunately this has missed 10.5.0 now. Can we get it tested and
merged? Thanks!

--
Andrea Bolognani / Red Hat / Virtualization

--

Christian Ehrhardt
Director of Engineering, Ubuntu Server
Canonical Ltd