After installing clicks to /custom/click, /usr/share/click/preinstalled version are still preferred

Bug #1371574 reported by Chris Wayne on 2014-09-19
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
apparmor (Ubuntu)
Medium
Jamie Strandboge
apparmor (Ubuntu RTM)
Critical
Jamie Strandboge
click-apparmor (Ubuntu)
Critical
Jamie Strandboge
click-apparmor (Ubuntu RTM)
Critical
Jamie Strandboge

Bug Description

This occurs while moving click apps from the rootfs into the custom tarball. Because of this some apps fail to launch.

Test case:
Install an app on a device and verify the apparmor policy for it in /var/lib/apparmor/profiles/click_<app_id> has:
@{CLICK_DIR}="{/custom/click,/opt/click.ubuntu.com,/usr/share/click/preinstalled}"

ProblemType: Bug
DistroRelease: Ubuntu 14.10
Package: click 0.4.32.1
ProcVersionSignature: Ubuntu 3.16.0-10.15-generic 3.16.1
Uname: Linux 3.16.0-10-generic x86_64
NonfreeKernelModules: fglrx
ApportVersion: 2.14.7-0ubuntu2
Architecture: amd64
CurrentDesktop: Unity
Date: Fri Sep 19 07:53:58 2014
InstallationDate: Installed on 2013-04-26 (510 days ago)
InstallationMedia: Ubuntu 13.04 "Raring Ringtail" - Release amd64 (20130424)
SourcePackage: click
UpgradeStatus: No upgrade log present (probably fresh install)

Chris Wayne (cwayne18) wrote :
Colin Watson (cjwatson) on 2014-09-19
Changed in click (Ubuntu):
assignee: nobody → Colin Watson (cjwatson)
importance: Undecided → Critical
status: New → In Progress
tags: added: rtm14
Colin Watson (cjwatson) wrote :

This is quite an interesting problem. I think I've tracked it down to Hook.install_link, which calls DB.get_path to work out where a package lives so that it can set the symlink to point to the right place, and that walks the database from the bottom up. On one level this is clearly wrong; but we have to be careful here, particularly in the area of multi-user support. Consider this situation:

  Initial state:
    /usr/share/click/preinstalled: org.example.foo 1.0, --all-users
    /opt/click.ubuntu.com: org.example.foo 1.1, --user=cjwatson

    This is clear: user cjwatson has upgraded that package from the store, but we are to leave the package at version 1.0 for all other users. There's no conflict as we can simply run system hooks for both org.example.foo 1.0 and org.example.foo 1.1, creating AppArmor profiles for both.

  After system image upgrade:
    /usr/share/click/preinstalled: org.example.foo 1.1, --all-users
    /opt/click.ubuntu.com: org.example.foo 1.1, --user=cjwatson

    Now what do we do? If user cjwatson runs org.example.foo then they will get the version in /opt/click.ubuntu.com, but user cwayne will get the version in /usr/share/click/preinstalled. It's not possible to simultaneously generate both AppArmor profiles; we have to pick one.

I think the proper answer here is probably to resolve versions before running system hooks. In general only the topmost database layer is available for per-user registrations, so it should always be possible to find a topmost path that is common to all users. We can then automatically unregister any conflicting versions above that and use the topmost path for system hooks.

Colin Watson (cjwatson) wrote :

I've made some progress on this. It is all however excruciatingly delicate, and this is not the first time that I have had to fix bugs of this general nature. I'm beginning to wonder if I should be taking a different approach. The whole problem here is that system hooks may only be attached to by a single instance of any given package version; this leads to the requirement that the AppArmor profile must point to just the right instance, and we often run into trouble (of the "launching application results in blank screen" kind) when trying to move packages around between databases in various ways. Even with regression tests, I'm worried that if I whack this mole it's still going to pop up somewhere else. It's not clear that all the requirements are truly soluble given the current design.

A more comprehensive answer to this category of problems, then, would involve a slight tweak to the design of system hooks, though fortunately an entirely backward-compatible one:

 * We add a new substitution to the hook Pattern field that expresses which database a package is in (the practicalities seem to suggest that a mangled version of its path would be easiest, so ${db-path} expanding to "usr_share_click_preinstalled" etc.).
 * When this substitution is present, "click hook" would start running system hooks for all instances of every package version.
 * click-apparmor could then add that to its .hook file once it's ready, perhaps becoming "Pattern: /var/lib/apparmor/clicks/${db-path}/${id}.json", or a flat structure if that's easier. It would need to adjust its handling of /var/lib/apparmor/{clicks,profiles}/ to cope with this, of course, and the first system image upgrade with the new version of click might have to rebuild all profiles on startup unless it's very clever, but that's comparatively minor.
 * Now that profiles depend on the database path, aa-exec-click would need to change its lookup to depend on the click database containing the requested executable. Some care would be needed to make sure this doesn't slow down application startup and make Ricardo cry; ubuntu-app-launch's desktop hook could perhaps help with this, although that may not be necessary.

The result of this would be much easier to deal with in click, and would be free of corner cases where there isn't obviously a single correct answer.

Jamie Strandboge (jdstrand) wrote :
Download full text (6.3 KiB)

This is a long reply, but stick with me until the end-- it should be worth it. :)

This would complicate things a bit in the following ways (which may reinforce some of your points but listing them for the alternate perspective if nothing else). This bit I am most concerned about is that we have multiple profiles for a particular app version since each profile will have a different CLICK_DIR. Eg, from comment #2, cjwatson may end up with version 1.1 in /opt/click.ubuntu.com and everyone else 1.1 in /usr/share/click/preinstalled. This means:

1. we have extra profiles to compile which will take more time when compiles are required. On the one hand, we can expect this number to be relatively low, but I've heard custom tarballs having as many as 30 or more clicks. Compiling profiles takes time, and while there are some additional gains to be made, we are likely going to focus on precompiling profiles in the short-medium term (see below)

2. these extra profiles need to be loaded into the kernel. With the numbers we are talking about, we probably won't be over 3M for these extra profiles unless the device has an extreme number of preinstalled/custom clicks, but I thought the point worth making (profiles are typically (far) under 50K each). It also takes time to load profiles into the kernel, but most of this pain should be addressed by recent changes to profile loading (we can load ~40 cached profiles per second on mako, for example). With the numbers we are talking about, this shouldn't be more than 2 seconds.

3. precompiling profiles is affected, but not appreciably more complicated. Right now we precompile profiles for preinstalled apps and custom apps. Preinstalled apps have the cache files in the rootfs and custom in the custom tarball. Scripts and boot jobs would have to be updated for both, but there is a 1 to 1 mapping (ie, custom always deals with custom and preinstalled with rootfs) so this change should be easy enough

4. app launch can be complicated since it would have to figure out which profile to use. This could probably be done without startup impact

5. developers are familiar with current filenames and the APP_ID concept. Adding complexity based on db path could confuse existing users and increase the learning curve for new users.

6. Most importantly, the APP_ID concept becomes considerably more complicated. Right now, the APP_ID flows though click, apparmor, lifecycle, and trusted helpers. Click currently works with click-apparmor to generate profile names that are the APP_ID. AppArmor doesn't care about the filenames of the profiles, and uses the APP_ID as the profile name. App launch (UAL and aa-exec-click, the latter not used in unity8) uses the APP_ID to change profile (application lifecycle also keeps track of the APP_ID for tracking processes of course, though I don't think it looks at the label of the process to do this). Trusted helpers will look at the AppArmor profile name to make decisions (eg trust-store lookups, online accounts access, etc). Right now, the APP_ID is a thread that all of these disparate parts can trust. The current proposal would necessitate the profile name to change to differentiate between pro...

Read more...

OK. I'm convinced by your arguments about app IDs; I didn't realise
that the profile name needed to be known outside just aa-exec-click
(which does have the application path in hand as well as the app ID).

I like your proposal of putting all the possible database names in
CLICK_DIR. Would it be reasonable to just add all the valid click
databases there, regardless of whether a given package happens to be
unpacked there at the moment? In that case, the necessary API for that
already exists, albeit a little obscurely:

  >>> from gi.repository import Click
  >>> db = Click.DB()
  >>> db.read()
  >>> [db.get(i).props.root for i in range(db.props.size)]
  ['/usr/share/click/preinstalled', '/custom/click', '/opt/click.ubuntu.com']

It might then be unnecessary to change click at all, which is obviously
fantastic from my point of view. ;-) But this does genuinely seem like
a better technical solution, and would let us stop playing whack-a-mole
with the precise ordering of various database operations. Given that
click-apparmor is the only system-level hook in the phone stack right
now, we could mandate this kind of thing as required behaviour for
future system-level hooks.

Colin Watson (cjwatson) on 2014-10-03
affects: click (Ubuntu) → click-apparmor (Ubuntu)
Changed in click-apparmor (Ubuntu):
assignee: Colin Watson (cjwatson) → nobody
status: In Progress → Triaged
Changed in click-apparmor (Ubuntu):
assignee: nobody → Jamie Strandboge (jdstrand)
status: Triaged → In Progress
tags: added: touch-2014-10-09
Jamie Strandboge (jdstrand) wrote :

This will need an apparmor upstart job adjustment as will to regenerate policy on click-apparmor upgrades.

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

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

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

  * also remove cache files on uninstall (LP: #1375938)
  * obtain all available click database names from click and use all of them
    in CLICK_DIR. This ensures that the profile can be used regardless of
    where the click app installation directory is (LP: #1371574)
  * debian/postinst: update the cached .md5sums file on upgrade to avoid
    running on install and then again on first boot after upgrade. This change
    only affects apt upgrades and not system-image upgrades since system-image
    upgrades always use the existing .md5sums if they exist (see
    /etc/system-image/writable-paths).
 -- Jamie Strandboge <email address hidden> Mon, 06 Oct 2014 16:01:26 -0500

Changed in click-apparmor (Ubuntu):
status: In Progress → Fix Released
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
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
Jamie Strandboge (jdstrand) wrote :

Reducing the utopic apparmor task to 'Medium' since it makes it show up on the rtm report even though it is fixed in rtm. It will be fixed in the next utopic upload (this week)

Changed in apparmor (Ubuntu):
importance: Critical → Medium
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package apparmor - 2.8.98-0ubuntu2

---------------
apparmor (2.8.98-0ubuntu2) utopic; urgency=medium

  * Updated to apparmor 2.9.beta4 (aka apparmor 2.8.98)
    - fix logparsing memory leak (LP: #1340927)
    - incorporate fixes to regression testsuite to compensate for
      af_unix mediation, as well as extend test coverage
      (LP: #1375403, LP: #1375516)
    - fix libapparmor's log parsing code to accept additional rejection
      types (LP: #1375413)
    - fix X abstraction for changed lightdm xauthority file locations
      (LP: #1339727)
    - parser: disable downgrade and not enforced rule messages
      by default
    - fix error when using regex profile names in IPC rules
      (LP: #1373085)
    - updates and fixes to the python utilities
    - translation updates

  [ Steve Beattie ]
  * Removed upstreamed patches:
    drop-peer_addr-with-local-addr-in-base.patch,
    update_socketpair_tests_for_af_unix.patch,
    fix_socketpair_tests.patch, sanitized-helpers-updates.patch,
    01-tests-unix_socket_lists.patch,
    02-tests-accept_unix_rules_in_mkprofile.patch,
    03-tests-unix_sockets_v7_pathnames.patch,
    04-tests-migrate_from_poll_to_sockio_timeout.patch,
    05-tests-add_abstract_socket_tests.patch,
    06-tests-use_socketpair_and_none.patch,
    07-parser-fix_local_perms.patch,
    08-phpsysinfo-policy-updates.patch,
    09-apache2-policy-instructions.patch,
    10-lp1371771.patch, 11-lp1371765.patch,
    lp1169881.patch
  * refreshed etc-writable.patch and libapparmor-layout-deb.patch
  * debian/control: add breaks on python3-apparmor against older
    apparmor-utils that used to be where python bits lived
    (LP: #1373259)
  * debian/apport/source_apparmor.py:
   - fixes the apparmor apport hook so it does not raise an exception if
     a non-unicode character is found in /var/log/kern.log or in
     /var/log/syslog. This should work under python3 or python2.7
     (LP: #1304447)
   - adjusts the add_info() function to take the expected additional ui
     argument, though it has no need for it.
   - converts the log parsing code to use with statements so as not to
     leak open file descriptors
   - updates the set of packages to query to see if installed and if so,
     report the version of.
   - adjust import to make pyflakes job easier
   - minor pep8 cleanups

  [ Jamie Strandboge ]
  * add-chromium-browser.patch: don't allow writing to the oom score and
    adjust files since this allows chromium to change the values for any
    process matching our UID
  * debian/apparmor.upstart: check if click-apparmor md5sums changed so we
    regenerate the policy if it changes too (LP: #1371574)
  * debian/apparmor.init: make corresponding upstart change to initscript
  * 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)
  * debian/control: fix typo (LP: #1187447)
 -- Steve Beattie <email address hidden> Thu, 09 Oct 2014 22:39:32 -0700

Changed in apparmor (Ubuntu):
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers