apparmor may fail to load some profiles if one is corrupted

Bug #1377338 reported by Jamie Strandboge on 2014-10-03
10
This bug affects 2 people
Affects Status Importance Assigned to Milestone
AppArmor
Medium
Unassigned
apparmor (Ubuntu)
Medium
Steve Beattie
apparmor (Ubuntu RTM)
High
Jamie Strandboge
click-apparmor (Ubuntu)
Critical
Jamie Strandboge
click-apparmor (Ubuntu RTM)
Critical
Jamie Strandboge

Bug Description

Steps to reproduce (on the emulator):
1. sudo sh -c 'echo foo > /var/lib/apparmor/profiles/click_com.ubuntu.music_music_1.3.638'
2. sudo start apparmor ACTION=teardown
3. sudo start apparmor
start: Job failed to start
4. sudo aa-status|egrep '^ '|grep -v '('| sort -u > /tmp/aa-status.music_bad
5. sudo rm -f /var/lib/apparmor/profiles/click_com.ubuntu.music_music_1.3.638
6. sudo aa-clickhook # regenerates the missing profile to had a good one
7. sudo start apparmor ACTION=teardown
8. sudo start apparmor
9. sudo aa-status|egrep '^ '|grep -v '('| sort -u > /tmp/aa-status.music_good
10. diff -Naur /tmp/aa-status.music_bad /tmp/aa-status.music_good
--- /tmp/aa-status.music_bad 2014-10-03 22:47:52.890906744 +0000
+++ /tmp/aa-status.music_good 2014-10-03 22:49:54.372739381 +0000
@@ -13,6 +13,10 @@
    com.ubuntu.developer.webapps.webapp-twitter_webapp-twitter_1.0.18//oxide_helper
    com.ubuntu.developer.webapps.webapp-twitter_webapp-twitter-helper_1.0.18
    com.ubuntu.dropping-letters_dropping-letters_0.1.2.2.66
+ com.ubuntu.music_music_1.3.638
+ com.ubuntu.shorts_shorts_0.2.330
+ com.ubuntu.sudoku_sudoku_1.1.292
+ com.ubuntu.weather_weather_1.1.374
    lxc-container-default
    lxc-container-default-with-mounting
    lxc-container-default-with-nesting

Expected results: only com.ubuntu.music_music_1.3.638 should be missing.

Changed in apparmor (Ubuntu):
importance: Undecided → Critical
description: updated
Changed in apparmor (Ubuntu):
status: New → In Progress
assignee: nobody → Steve Beattie (sbeattie)
tags: added: rtm14 touch-2014-10-09
Changed in apparmor (Ubuntu RTM):
status: New → In Progress
importance: Undecided → Critical
assignee: nobody → Jamie Strandboge (jdstrand)
Jamie Strandboge (jdstrand) wrote :

The cause of the corruption is believed to be an interaction between the click-system-hooks and the apparmor upstart jobs. click-apparmor will be adjusted to use a blocking lockfile to avoid the corruption. As such, the apparmor task priority can be reduced.

After discussing with the apparmor team, fixing the parser bug can (and should be done) but it more involved that the cache bug and we can't fix it in time for rtm. If the lockfile doesn't fully address this issue, we can go back to using '-n1' with xargs unconditionally in /lib/apparmor/functions.

Changed in click-apparmor (Ubuntu):
status: New → In Progress
importance: Undecided → Critical
assignee: nobody → Jamie Strandboge (jdstrand)
Changed in click-apparmor (Ubuntu RTM):
status: New → In Progress
importance: Undecided → Critical
assignee: nobody → Jamie Strandboge (jdstrand)
Changed in apparmor (Ubuntu RTM):
importance: Critical → Medium
Changed in apparmor (Ubuntu):
importance: Critical → Medium
Changed in apparmor (Ubuntu RTM):
status: In Progress → Triaged
Changed in apparmor (Ubuntu):
status: In Progress → Triaged
no longer affects: apparmor (Ubuntu RTM)
Jamie Strandboge (jdstrand) wrote :

Upon further investigation, python3-apparmor-click and python3-apparmor-easyprof both use shutil.move() to put a temp file into place. shutil.move() will use os.rename() if the files reside on the same file, but will use shutil.copy2() followed by an unlink otherwise. Since the tempfile.mkstemp() in both cases does not specify to use a different temp directory (ie, dir=None), these files will be created in /tmp, which is a tmpfs on devices (verified on mako), therefore the shutil.move() is not atomic. This confirms that utilizing a blocking lock file will prevent at least some forms of races and corruption. We could adjust the mkstemp() call to use the same filesystem, however, that would result in unexpected behavior when two aa-clickhooks are run at the same time (ie, both would think they did everything correctly but each could have missed something).

Jamie Strandboge (jdstrand) wrote :

For rtm I can add a workaround to /lib/apparmor/functions to fallback to using -n1 if tha parser fails on the profile set. This is a minimal change and retains the performance improvements of not using -n1 in the normal case of things being ok. However, we want to remove this and rely on the parser handling this correctly going forward.

Changed in apparmor (Ubuntu RTM):
assignee: nobody → Jamie Strandboge (jdstrand)
importance: Undecided → High
status: New → In Progress
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package click-apparmor - 0.2.11.1

---------------
click-apparmor (0.2.11.1) utopic; urgency=medium

  * aa-clickhook: don't remove the lock file so we can properly handle 3 or
    more processes contending for the lock

click-apparmor (0.2.11) utopic; urgency=medium

  * apparmor/click.py: be more careful with out_fn assignment in
    output_policy()
  * aa-clickhook: implement blocking lockfile to make sure this script does
    not run concurrently with itself (LP: #1377338)
 -- Jamie Strandboge <email address hidden> Tue, 07 Oct 2014 09:32:53 -0500

Changed in click-apparmor (Ubuntu RTM):
status: In Progress → Fix Released
Changed in click-apparmor (Ubuntu):
status: In Progress → Fix Released
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package apparmor - 2.8.96~2652-0ubuntu5.1

---------------
apparmor (2.8.96~2652-0ubuntu5.1) 14.09; urgency=medium

  * debian/apparmor.{upstart,init}: check if click-apparmor md5sums changed so
    we regenerate the policy if it changes too (LP: #1371574)
  * debian/lib/apparmor/functions: fall back to using -n1 if the parser failed
    to load a profile set. This should be removed when the parser properly
    handles profile sets with corrupted profiles (LP: #1377338).
 -- Jamie Strandboge <email address hidden> Tue, 07 Oct 2014 09:24:45 -0500

Changed in apparmor (Ubuntu RTM):
status: In Progress → Fix Released
tags: added: aa-parser
Jamie Strandboge (jdstrand) wrote :

14.10 had workaround in place in 2.8.98-0ubuntu2

Changed in apparmor:
status: New → Triaged
importance: Undecided → Medium
Changed in apparmor (Ubuntu):
status: Triaged → Fix Released
intrigeri (intrigeri) wrote :

Along with LP: #1488179, this is one source of ugliness in current Debian/Ubuntu initscript, that makes it harder than needed to port it to systemd.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers