On Tue, 2025-01-14 at 12:13 -0600, Andrea Bolognani wrote:
> On Wed, Jan 08, 2025 at 11:06:54AM -0700, Jim Fehlig wrote:
> > On 1/8/25 06:50, Georgia Garcia wrote:
> > > On Tue, 2025-01-07 at 17:29 -0700, Jim Fehlig wrote:
> > > > On 1/7/25 08:23, Georgia Garcia wrote:
> > > > > Some rules are generated dynamically during boot and added to the
> > > > > AppArmor policy. An example of that is macvtap devices that call the
> > > > > AppArmorSetFDLabel hook to add a rule for the tap device path.
> > > > >
> > > > > Since this information is dynamic, it is not available in the xml
> > > > > config, therefore whenever a "Restore" hook is called, the entire
> > > > > profile is regenerated by virt-aa-helper based only the information
> > > > > from the VM definition, so the dynamic/runtime information is lost.
> > > >
> > > > Have you considered, or experimented with, adding a "remove file" option to the
> > > > "replace" mode of virt-aa-helper? Figuring out the short name of the option
> > > > might be the most difficult part :-P.
> > >
> > > I didn't experiment with it because I thought it was a change too
> > > drastic to change the behavior of "Restore" to instead of regenerate
> > > the policy based on the xml, to read the policy, string match the drive
> > > (for example) being removed, remove that entry and rewrite the policy
> > > file. By maintaining current behavior for the most part I would also
> > > lower the risk of regressions. It might be possible but I'd have to
> > > look into more detail into all the "Restore" hooks to say for certain.
> >
> > Given the current behavior of Restore, I share your fear of regressions.
> >
> > > > > This patch stores the dynamically generated rules in a new file called
> > > > > libvirt-uuid.runtime_files which is included by the AppArmor
> > > > > policy. This file should exist while the domain is running and should
> > > > > be reloaded automatically whenever there's a restore operation. These
> > > > > rules only make sense when the VM is running, so the file is removed
> > > > > when the VM is shutdown.
> > > >
> > > > I'm not super excited about this approach, but I'm also no apparmor expert.
> > > > Perhaps my preference for a '--remove-file' option to supplement '--add-file' is
> > > > not really possible or realistic with the current apparmor integration.
> > > >
> > > > Andrea also has some experience with apparmor and its libvirt support. He may
> > > > have better advice on fixing this issue.
> > >
> > > Since there aren't hooks for removing permissions for files that were
> > > created by FD (domainSetSecurityImageFDLabel /
> > > domainSetSecurityTapFDLabel) I figured that separating them in a
> > > different file was the best approach but I'm open to changing it if
> > > it's more appropriate. Any feedback is welcome!
> >
> > I'm not against the approach, and indeed it may be the safest way to go.
> > Before I invest time reviewing and testing this patch, let's see if others
> > have comments/suggestions.
> >
> > PS: Add Andrea to cc this time :-)
>
> Thanks for looping me in.
>
> I'm far from an expert when it comes to AppArmor, but after giving
> the patch a closer look I share some of Jim's concerns.
>
> While the current approach of throwing away all information that is
> not recorded in the XML is obviously problematic, it seems that what
> you're implementing here is a workaround for a somewhat narrow
> failure scenario that doesn't fully address the underlying
> limitations.
>
> Going by the example presented in [1], IIUC your change would make it
> so the lines needed for macvtap use, specifically
>
> "/dev/net/tun" rwk,
> "/dev/tap82" rwk,
>
> would be written to .runtime_files instead of .files. That's good
> enough to safeguard them from disappearing when disks are unplugged,
> but what if the macvtap interface itself is? Wouldn't those lines
> linger around despite being no longer needed?
Yes, they would, and that is the current behavior - if you remove only
the macvtap, it will not be removed from .files
That's the current limitation because there are no security hooks
called when macvtap devices are unplugged. I thought it would be better
to be over-permissive (fd permissions linger throughout the runtime of
the vm) than over-restrictive to fix the issue given what's available
in the security side of libvirt.
>
> I've also noticed that the lines
>
> "/var/lib/libvirt/qemu/domain-33-tap/{,**}" rwk,
> "/run/libvirt/qemu/channel/33-tap/{,**}" rwk,
> "/var/lib/libvirt/qemu/domain-33-tap/master-key.aes" rwk,
These rules were also created from a file descriptor like macvtap
devices, therefore were deleted when the profile was created based on
the xml on a restore.
>
> have disappeared, and the lines
>
> "/dev/pts/0" rw,
> "/dev/pts/0" rw,
>
> (duplicated?) have appeared. That doesn't seem right, and I wouldn't
> be surprised if this change could lead to further issues.
>
> So I think we really need a --remove-file option that can be used to
> carefully undo the changes applied by an earlier use of --add-file.
>
> Unfortunately this will likely involve a far more significant rework
> of the AppArmor driver, and we will certainly have to be careful
> about not introducing regressions in the process, but I'm really not
> a fan of half measures unless the trade-off is overwhelmingly stacked
> in their favor...
>
As I said earlier, it would also involve the addition of at least one
security hook, impacting all security drivers. But yes, this change
would basically involve rewriting the entire AppArmor driver and a part
of virt-aa-helper. While I'm not against it, unfortunately I will not
be able to dedicate the amount of time needed for such a significant
change.
Thank you,
Georgia
> To reiterate, this opinion is based on vague at best familiarity with
> the AppArmor driver. Don't hesitate to point out why I'm wrong :)
>
>
> [1] https://gitlab.com/libvirt/libvirt/-/issues/692
