The apparmor-abstractions package does:

%posttrans abstractions
rm -f /var/cache/apparmor/* 2>/dev/null
systemctl is-active -q apparmor && systemctl reload apparmor ||:

Maybe we should have a macro for this as well. 
I do wonder, however, if `systemctl reload apparmor` is sufficiently atomic to prevent attacks while reloading occurs.

Maybe the @security team could help out here.

(In reply to Egbert Eich from comment #0)
> The package apparmor-rpm-macros contains a single macro `%apparmor_reload`
> with no usage description. 
> Such description may be given in a comment in the macro file itself.

Good idea, I'll add a comment.


> The usefulness of this macro is rather limited: it only allows to add/update
> profiles. 
> Updated/added abstractions cannot be processed by this as it would require
> to reload all profiles that use them. Such updates may require a full reload
> of all profiles.

Right.


(In reply to Egbert Eich from comment #1)
> The apparmor-abstractions package does:
> 
> %posttrans abstractions
> rm -f /var/cache/apparmor/* 2>/dev/null

Note that this "rm" only deletes very old caches from old AppArmor versions (I'd have to check which version exactly). Newer AppArmor versions use subdirectories with a "feature hash" as name - which means that you'll get a new subdirectory if the kernel or AppArmor userspace get support for (for example) new rule types.

> systemctl is-active -q apparmor && systemctl reload apparmor ||:
> 
> Maybe we should have a macro for this as well. 

Nowadays you could even use the usual macro to restart apparmor.service. As workaround for bug 853019, ExecStop=/bin/true was introduced (see systemctl cat apparmor.service for details), which also means "systemctl restart apparmor" is not a problem anymore.

> I do wonder, however, if `systemctl reload apparmor` is sufficiently atomic
> to prevent attacks while reloading occurs.
> 
> Maybe the @security team could help out here.

Maybe I can save the security team some work and answer that ;-)

Short answer: Yes, it is sufficiently atomic.

Long answer:

systemctl reload apparmor basically translates to "apparmor_parser -r /etc/apparmor.d" (+ some checks if AppArmor is enabled in the kernel etc.) which means all profiles get reloaded with one apparmor_parser call. This is not completely atomic (apparmor_parser loads/replaces one profile after the other into the kernel - ideally it would replace all profiles at once).

However, confined processes will never run unconfined during reload. Reloading a profile means running processes get moved from the old profile to the new one. So in worst case, half of your processes might still run under the old profiles, while the other half already uses the new profiles (depending on which profiles were already reloaded).


Unlikely corner case ahead... ;-)

In theory, and if you _really_ like corner cases, you could think of a race condition to abuse the fact that profiles get reloaded one after the other (especially if exec rules change, and the profiles for the old/new exec target differs a lot). Of course this won't allow a process to become unconfined, it would only be possible to use an existing execute rule to execute another program, and the attack would only be useful if the profile of this other program got way more permissions than before.

In practise (and assuming realistic profile changes) I don't think that you have to worry about this corner case.


One more note for completeness: If you _unload_ a profile (using apparmor_parser -R or aa-teardown), then the running processes will obviously become unconfined. Even if you load the profiles again, they'll stay unconfined until you restart the affected processes. So - don't do that ;-)


@security team: I'll keep the needinfo to give you a chance to respond. Feel free to just drop the needinfo flag if my answer is good enough ;-)

(In reply to Christian Boltz from comment #2)
> (In reply to Egbert Eich from comment #0)
> > The package apparmor-rpm-macros contains a single macro `%apparmor_reload`
> > with no usage description. 
> > Such description may be given in a comment in the macro file itself.
> 
> Good idea, I'll add a comment.
> 
> 
> > The usefulness of this macro is rather limited: it only allows to add/update
> > profiles. 
> > Updated/added abstractions cannot be processed by this as it would require
> > to reload all profiles that use them. Such updates may require a full reload
> > of all profiles.
> 
> Right.
> 
> 
> (In reply to Egbert Eich from comment #1)
> > The apparmor-abstractions package does:
> > 
> > %posttrans abstractions
> > rm -f /var/cache/apparmor/* 2>/dev/null
> 
> Note that this "rm" only deletes very old caches from old AppArmor versions
> (I'd have to check which version exactly). Newer AppArmor versions use
> subdirectories with a "feature hash" as name - which means that you'll get a
> new subdirectory if the kernel or AppArmor userspace get support for (for
> example) new rule types.

Is it still useful to delete? If so, there should be an `rm -rf /var/cache/apparmor/* 2> /dev/null`
To make this consistent (and match the apparmor version) this would really 
call for another rpm macro. 

> 
> > systemctl is-active -q apparmor && systemctl reload apparmor ||:
> > 
> > Maybe we should have a macro for this as well. 
> 
> Nowadays you could even use the usual macro to restart apparmor.service. As
> workaround for bug 853019, ExecStop=/bin/true was introduced (see systemctl
> cat apparmor.service for details), which also means "systemctl restart
> apparmor" is not a problem anymore.

Yeah, I've seen this. The unfortunate side effect is that `rcapparmor stop`
does not do what one expects and apparmor keeps lingering on even if stopped.
You'd have to do aa_teardown to get rid of it.

> > I do wonder, however, if `systemctl reload apparmor` is sufficiently atomic
> > to prevent attacks while reloading occurs.
> > 
> > Maybe the @security team could help out here.
> 
> Maybe I can save the security team some work and answer that ;-)
> 
> Short answer: Yes, it is sufficiently atomic.
> 
> Long answer:
> 
> systemctl reload apparmor basically translates to "apparmor_parser -r
> /etc/apparmor.d" (+ some checks if AppArmor is enabled in the kernel etc.)
> which means all profiles get reloaded with one apparmor_parser call. This is
> not completely atomic (apparmor_parser loads/replaces one profile after the
> other into the kernel - ideally it would replace all profiles at once).
> 
> However, confined processes will never run unconfined during reload.
> Reloading a profile means running processes get moved from the old profile
> to the new one. So in worst case, half of your processes might still run
> under the old profiles, while the other half already uses the new profiles
> (depending on which profiles were already reloaded).

Ok, I got that. What I gather from boo#853019 and here is:
1. When loading updated profiles existing ones are swapped sufficiently
   atomically for updated ones so running application are always subject
   to either the old set or the new set.
2. Clearing out old profiles (like with aa_teardown) will cause running programs
   to run unconfined even after the new profiles are loaded(??)
In this case, adding a profile that previously did not exist will cause a running program for which a profile was added to continue to run unconfined.

> Unlikely corner case ahead... ;-)
> 
> In theory, and if you _really_ like corner cases, you could think of a race
> condition to abuse the fact that profiles get reloaded one after the other
> (especially if exec rules change, and the profiles for the old/new exec
> target differs a lot). Of course this won't allow a process to become
> unconfined, it would only be possible to use an existing execute rule to
> execute another program, and the attack would only be useful if the profile
> of this other program got way more permissions than before.

Right. This would matter only if an update (of possible multiple packages)
would change things around in place. 
With non-transactional updates there is always a window where the system is somewhat inconsistent.

> In practise (and assuming realistic profile changes) I don't think that you
> have to worry about this corner case.
> 
> 
> One more note for completeness: If you _unload_ a profile (using
> apparmor_parser -R or aa-teardown), then the running processes will
> obviously become unconfined. Even if you load the profiles again, they'll
> stay unconfined until you restart the affected processes. So - don't do that
> ;-)

Yes. I understand this - it obviously was the concern in boo#853019

> 
> 
> @security team: I'll keep the needinfo to give you a chance to respond. Feel
> free to just drop the needinfo flag if my answer is good enough ;-)

Thanks! 
Let's see ...

i think christian covered it well.