unloadProfiles from interfaces/apparmor/apparmor.go does not unload any profiles

Bug #1818241 reported by Jamie Strandboge on 2019-03-01
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
snapd
High
Unassigned

Bug Description

unloadProfiles() is currently called with a list of profile names and it invokes apparmor_parser --remove on those names. Unfortunately, apparmor_parser --remove requires full paths to the profiles, not just a profile name, so the profiles were not removed. While we could prepend dirs.SnapAppArmorDir to the profile names, by the time unloadProfiles() is called it is too late because the profiles on disk have already been removed. This was never caught before because apparmor_parser less than 2.13 would return 0 when given profiles that didn't exist and so the error check would never fire.

That said, this code is somewhat misguided for the current implementation of snapd since 'snap remove' does not guarantee there are no processes running for the removed snap. While unloadProfiles() could be fixed to unload the profiles, this could result in a snap process being moved to unconfined upon 'snap remove'. If this is going to be fixed, we must guarantee all processes are killed as part of 'snap remove' and only then remove the profiles via sysfs. It is arguably poor UX to kill all processes on 'snap remove' so simply removing the cache files may be better (this is simply what distro packaging does; I'll let someone else decide on the UX aspect; but please keep me involved if changing the behavior).

https://github.com/snapcore/snapd/pull/6549 removes the attempt and adds the following comment:

// UnloadProfiles is meant to remove the named profiles from the running
// kernel and then remove any cache files. Importantly, we can only unload
// profiles when we are sure there are no lingering processes from the snap
// (ie, forcibly stop all running processes from the snap). Otherwise, any
// running processes will become unconfined. Since we don't have this guarantee
// yet, leave the profiles loaded in the kernel but remove the cache files from
// the system so the policy is gone on the next reboot.

Changed in snapd:
status: New → Triaged
Zygmunt Krynicki (zyga) on 2019-10-08
Changed in snapd:
importance: Undecided → High
Zygmunt Krynicki (zyga) on 2019-10-29
Changed in snapd:
status: Triaged → Confirmed
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers