ntp doesn't unload its apparmor profile on purge

Hi Simon,
thank you for your report - it indeed should unload the profile.

I wonder thou as it uses:
  dh_apparmor --profile-name=usr.sbin.ntpd -pntp

Which I thought should handle load and unload in the generated sections.
Commenting on that once I prepped my text ...

This is the section it created on postrm:

# Automatically added by dh_apparmor
if [ "$1" = "purge" ] && ! [ -e "/etc/apparmor.d/usr.sbin.ntpd" ] ; then
    rm -f "/etc/apparmor.d/disable/usr.sbin.ntpd" || true
    rm -f "/etc/apparmor.d/force-complain/usr.sbin.ntpd" || true
    rm -f "/etc/apparmor.d/local/usr.sbin.ntpd" || true
    rmdir /etc/apparmor.d/disable 2>/dev/null || true
    rmdir /etc/apparmor.d/local 2>/dev/null || true
    rmdir /etc/apparmor.d 2>/dev/null || true
fi
# End automatically added section

Which does not reload apparmor or unload the profile indeed.
It only makes sure no customizing stays around.

The apparmor bits are in Debian and I'd prefer to fix it there for ntp.
But OTOH this might be something much more generic - shouldn't dh_apparmor unload it just as it loads it?

From postinst of dh_apparmor:
        # Reload the profile, including any abstraction updates
        if aa_is_enabled; then
            apparmor_parser -r -T -W "$APP_PROFILE" || true
        fi

So if dh_apparmor generates the snippet to load correctly, shouldn't it (not only on purge but on remove as well) also unload it?

I'm not a 1005 sure, but for now adding an apparmor task (where dh-apparmor is from) for the experts to weight in.

On 2017-05-12 01:48 AM, ChristianEhrhardt wrote:
> shouldn't dh_apparmor unload it just as it loads it?

Exactly, I would have assumed that it was dh_apparmor's job. Curious to
hear from the Apparmor folks. Thanks for looking into this.

Simon

You are technically correct that the still-loaded profile doesn't match a clean uninstall. However, I have a different opinion on this and thing keeping the profile loaded is the better choice.

Unloading a profile means removing the confinement from running processes. So if a process is still running and (Hi Murphy!) does something bad after being uninstalled and becoming unconfined, you are screwed up.

If the profile stays loaded, still running processes stay confined. The disadvantages are a) you waste some bytes in the RAM and b) if you install a different package shipping a binary with the same path, but without an AppArmor profile, it will suffer from the still-loaded profile.

Both ways are not perfect, but I really prefer keeping the profile loaded because it does less harm.

For comparison: Does the uninstall script also run "killall -9 ntp"? If so, feel free to unload the profile ;-)

Christian is right and this is precisely why dh_apparmor intentionally does not unload the profile. Marking the apparmor task as Won't Fix since this has been discussed several times in the past (if apparmor upstream wants to revisit, we can open the bug).

The ntp package is still in a position to unload the profile if it desires, so leaving its task open, but I believe this would be a mistake and if done in Ubuntu, I would file a bug requesting the previous behavior.

I don't particularly care for the openntpd kludge, but you can unload a profile that was deleted from disk with:

sudo sh -c 'echo -n /usr/sbin/ntpd > /sys/kernel/security/apparmor/.remove'

(note, the '-n' with echo is important).

On 2017-05-12 02:15 PM, Christian Boltz wrote:
> You are technically correct that the still-loaded profile doesn't
> match a clean uninstall. However, I have a different opinion on this
> and thing keeping the profile loaded is the better choice.
>
> Unloading a profile means removing the confinement from running
> processes. So if a process is still running and (Hi Murphy!) does
> something bad after being uninstalled and becoming unconfined, you
> are screwed up.

If purging a package doesn't kill the running process, that's a
packaging bug, not something Apparmor should try to paper over, IMHO.

> If the profile stays loaded, still running processes stay confined.
> The disadvantages are a) you waste some bytes in the RAM and b) if
> you install a different package shipping a binary with the same path,
> but without an AppArmor profile, it will suffer from the
> still-loaded profile.

Asking someone to know about that:

  echo -n "<profile_name>" > /sys/kernel/security/apparmor/.remove

Is asking too much IMHO and increases the friction between sysadmins and
Apparmor in general. The colleague that experience the issue was about
to do an Apparmor teardown to get going...

> Both ways are not perfect, but I really prefer keeping the profile
> loaded because it does less harm.
>
>
> For comparison: Does the uninstall script also run "killall -9 ntp"?
> If so, feel free to unload the profile ;-)

I still think that having dh_apparmor do the unload is the best way :)

On Fri, May 12, 2017 at 06:56:35PM -0000, Simon Déziel wrote:
> If purging a package doesn't kill the running process, that's a
> packaging bug, not something Apparmor should try to paper over, IMHO.

Yikes, package pre/post inst/rm scripts already do way too many things.
Deciding what processes to kill is way too much. No thanks. :)

Thanks

On 2017-05-12 03:34 PM, Seth Arnold wrote:
> On Fri, May 12, 2017 at 06:56:35PM -0000, Simon Déziel wrote:
>> If purging a package doesn't kill the running process, that's a
>> packaging bug, not something Apparmor should try to paper over, IMHO.
>
> Yikes, package pre/post inst/rm scripts already do way too many things.
> Deciding what processes to kill is way too much. No thanks. :)

Sorry, I meant it's the service's job to properly/forcefully stop a
daemon. I agree that killing processes in postrm is dangerous.

> Sorry, I meant it's the service's job to properly/forcefully stop a
> daemon. I agree that killing processes in postrm is dangerous.

I agree that kill -9 isn't the way to go (it was meant as a rhetoric question), but there are still valid reasons why a daemon doesn't get stopped in postrm:
- the daemon could be started manually (for example in debug mode, which needs an additional cmdline parameter) and is therefore "invisible" to "systemctl stop ntp"
- what about non-daemons, for example firefox? I seriously hope package uninstall doesn't even try to stop or kill it ;-)

"Asking someone to know about that:

  echo -n "<profile_name>" > /sys/kernel/security/apparmor/.remove

Is asking too much IMHO and increases the friction between sysadmins and
Apparmor in general."

Of course. I listed this as something that could be considered for the openntpd/ntpd case, not for a sysadmin. This is a corner case that should be coordinated between these packages. In all cases except this rare corner case, it is totally fine (and correct) to leave the profile loaded until next reboot and sysadmins don't have to care about this at all.

How about suggesting the following to openntpd in Debian then?
Simon would you be so kind and open a bug there if that would find a consensus?

diff --git a/debian/openntpd.preinst b/debian/openntpd.preinst
index 4cb3147..3e55947 100644
--- a/debian/openntpd.preinst
+++ b/debian/openntpd.preinst
@@ -7,6 +7,12 @@ if dpkg-maintscript-helper supports rm_conffile 2>/dev/null; then
     dpkg-maintscript-helper rm_conffile /etc/apparmor.d/usr.sbin.ntpd 1:5.7p4-1 -- "$@"
 fi

+# due to former installations of ntp the system could still have an apparmor
+# loaded at the shared binary path /usr/sbin/ntpd. There are various reasons
+# discussed that dh_appamor nor ntp can unload it. But it could block openntp
+# to work, so remove it unconditionally.
+echo -n /usr/sbin/ntpd > /sys/kernel/security/apparmor/.remove 2>/dev/null || /bin/true
+
 #DEBHELPER#

 exit 0

Thanks for the patch Christian, I relayed it in https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=882556

Nothing to do on NTP here, linked up the relayed Debian bug on openntpd - thanks Simon!