AppArmor does not load all valid profiles if broken profile symlink exists

[Vincas Dargis]

[ Impact ]

The apparmor_parser tool in Bionic does not handle return error codes for certain conditions (most notably, handling file access issues), and this leads to inappropriate error handling in any shells executing the tool.

[ Test Plan ]

One simple test case is to have apparmor_parser use a flag that requires a filename input that does not exist. E.g.:

$ ll /etc/apparmor.d/test.rule
ls: cannot access '/etc/apparmor.d/test.rule': No such file or directory

If we try to remove a defitintion using the non-existing file:

$ sudo apparmor_parser -R /etc/apparmor.d/test.rule
File /etc/apparmor.d/test.rule not found, skipping...

Now, at that same prompt:

$ echo $?
0

This operation should not return zero, as the action clearly failed.

[ Where problems could occur ]

Issues could occurr on scripts that use apparmor_parser and read (for unbeknownst reasos) the zero return code and, once the fix is landed, start seeing the "real" error code in the calling prompt/session.

It is wise to mention that this has already landed in AppArmor versions in Focal on, with no regressions detected ever since.

[Original Description]
Debian bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=921866

AppArmor does not load all (just some) profiles if `/etc/apparmor.d/`
contains broken symlink to previously existing local profile.

Steps to reproduce:

sudo ln -s /foo/bar/nonexistent /etc/apparmor.d/usr.bin.foo
sudo aa-teardown # or reboot, systemctl restart is not enough
sudo systemctl restart apparmor
sudo aa-status

This is `aa-status` after creating broken symlink:
```
$ sudo aa-status
apparmor module is loaded.
4 profiles are loaded.
2 profiles are in enforce mode.
   /usr/bin/freshclam
   libreoffice-xpdfimport
2 profiles are in complain mode.
   mdnsd
   smbd
1 processes have profiles defined.
0 processes are in enforce mode.
0 processes are in complain mode.
1 processes are unconfined but have a profile defined.
   /usr/bin/freshclam (558)
```

And this is how it looks without broken symlink:

```
apparmor module is loaded.
53 profiles are loaded.
37 profiles are in enforce mode.
   /usr/bin/freshclam
   /usr/bin/man
   /usr/bin/pidgin
   /usr/bin/pidgin//sanitized_helper
   /usr/bin/totem
   /usr/bin/totem-audio-preview
   /usr/bin/totem-video-thumbnailer
   /usr/bin/totem//sanitized_helper
   /usr/lib/cups/backend/cups-pdf
   /usr/local/bin/netest.sh
   /usr/sbin/apt-cacher-ng
   /usr/sbin/cups-browsed
   /usr/sbin/cupsd
   /usr/sbin/cupsd//third_party
   /usr/sbin/ejabberdctl
   /usr/sbin/ejabberdctl//su
   /usr/sbin/haveged
   /usr/sbin/mysqld-akonadi
   /usr/sbin/mysqld-akonadi///usr/sbin/mysqld
   /usr/sbin/sshd
   /usr/sbin/sshd//passwd
   apache2
   apache2//DEFAULT_URI
   apache2//HANDLING_UNTRUSTED_INPUT
   dhclient
   libreoffice-oopslash
   libreoffice-senddoc
   libreoffice-soffice
   libreoffice-soffice//gpg
   libreoffice-xpdfimport
   man_filter
   man_groff
   thunderbird
   thunderbird//browser_java
   thunderbird//browser_openjdk
   thunderbird//gpg
   thunderbird//sanitized_helper
16 profiles are in complain mode.
   /usr/bin/irssi
   /usr/sbin/dnsmasq
   /usr/sbin/dnsmasq//libvirt_leaseshelper
   avahi-daemon
   identd
   klogd
   mdnsd
   nmbd
   nscd
   ping
   smbd
   smbldap-useradd
   smbldap-useradd///etc/init.d/nscd
   syslog-ng
   syslogd
   traceroute
5 processes have profiles defined.
0 processes are in enforce mode.
0 processes are in complain mode.
5 processes are unconfined but have a profile defined.
   /usr/bin/freshclam (558)
   /usr/sbin/cups-browsed (608)
   /usr/sbin/cupsd (566)
   /usr/sbin/haveged (508)
   /usr/sbin/sshd (736)

```

Journal does not produce any notice about failure (while restarting):

```
$ sudo journalctl -n0 -f -u apparmor
-- Logs begin at Sat 2019-02-09 17:25:42 EET. --
Feb 09 17:50:59 debian-sid systemd[1]: Stopping Load AppArmor
profiles...
Feb 09 17:50:59 debian-sid systemd[1]: apparmor.service: Succeeded.
Feb 09 17:50:59 debian-sid systemd[1]: Stopped Load AppArmor profiles.
Feb 09 17:50:59 debian-sid systemd[1]: Starting Load AppArmor
profiles...
Feb 09 17:50:59 debian-sid apparmor.systemd[6842]: Restarting AppArmor
Feb 09 17:50:59 debian-sid apparmor.systemd[6842]: Reloading AppArmor
profiles
Feb 09 17:50:59 debian-sid systemd[1]: Started Load AppArmor profiles.
```

`apparmor_parser` returns 0:

```
$ sudo /sbin/apparmor_parser --write-cache --verbose --replace --
/etc/apparmor.d && echo $?
Cached reload succeeded for
"/var/cache/apparmor/ea9ed67a.0/usr.lib.libreoffice.program.xpdfimport".
Cached reload succeeded for
"/var/cache/apparmor/ea9ed67a.0/usr.sbin.mdnsd".
Cached reload succeeded for
"/var/cache/apparmor/ea9ed67a.0/usr.bin.freshclam".
Cached reload succeeded for
"/var/cache/apparmor/ea9ed67a.0/usr.sbin.smbd".
0
```

[John Johansen (jjohansen)]

I tried this from the parser and it "works" in the sense that it continues to load profiles and longs an error message. However the parser does not return with an error.

Beyond the parser not returning an error it looks like this bug is in the initscript

[John Johansen (jjohansen)]

I spoke too soon, my test was flawed. The parser does indeed stop on first error.

[John Johansen (jjohansen)]

Okay, parser sorted. There are issues in the initscript

[John Johansen (jjohansen)]

so while there are somethings in the initscript to cleanup they aren't related to this bug.

please try https://gitlab.com/apparmor/apparmor/merge_requests/343

this works for me on buster.

[John Johansen (jjohansen)]

fixed in 125931950838 ("parser: Fix parser failing to handle errors when setting up work")

[Mauricio Faria de Oliveira (mfo)]

Marking as Fix Released.

$ sudo apt download apparmor
Get:1 https://esm.ubuntu.com/infra/ubuntu bionic-infra-updates/main amd64 apparmor amd64 2.12-4ubuntu5.3+esm1 [489 kB]
...

$ dpkg-deb -x apparmor_*.deb deb

$ gzip -dc deb/usr/share/doc/apparmor/changelog.Debian.gz | sed '/^ -- /q'
apparmor (2.12-4ubuntu5.3+esm1) bionic; urgency=medium

  [ Pedro Principeza ]
  * debian/patches/parser-fix-parser-failing-to-handle-errors-when-
    setting-up-work.patch: Fix parser failing to handle errors when
    setting up work (LP: #1815294)

  [ Steve Beattie ]
  * d/p/u/parser-fix-handling-of-failed-symlink-traversal.patch: report
    failure when a symlnk fails to resolve, also don't short circuit
    processing a directory when a symlink fails to resolve
  * d/p/u/parser-convert_error_tests_to_python_and_add_tests.patch:
    update error/warning tests to their modern python form and add tests
    that cover the parser failing to set an error code when passed files
    that do not exist (LP: #1815294)

 -- Pedro Principeza <email address hidden> Tue, 13 Jun 2023 14:19:14 +0000