Support policy query interface for file

This ability was introduced in the utopic kernel.

Attached is a example program that builds a file query string.

to build
  gcc -o query_file query_file.c -l apparmor

to use
  query_file <profile_name> file1 file2 file3 ...

eg.
  > ./query_file firefox /tmp /tmp/
  read '/tmp' denied
  read '/tmp/' allowed

Note: specifying a profile name that doesn't exist will result in an error like

./query_file badprofile /tmp /tmp/
read '/tmp' error: No such file or directory
read '/tmp/' error: No such file or directory

the apparmor query interface will not tell you if the file being queried does not exist, it is the profile being queried that does not exist

updated query_file.c example to fix a stupid bug

Adding media-hub, mediascanner2 and thumbnailer to this bug since there is now a way to query apparmor for file access instead of having to hardcode APP_IDs (see the attached files). This query interface will improve going forward, but this should be able to clean up the code for various trusted helpers. Please add any that I may have missed. :)

This technique looks quite promising. I have a few questions though:

1. if I do the aa_query_label() check followed by an open() call to read it, am I open to the same race conditions as if I was relying on access() to check permissions?

2. if the given path is a symlink, am I checking for permission to read the symlink or the destination of the symlink, or both?

If this lets us replace the FD passing hack, I'd love to use it. I'm just wondering how to safely use it in a race free manner.

1. Yes and no. Ideally we would have a aa_query function that accepted an open file descriptor. That isn't available right now but should be down the road.

access(2) is more racey, IMO, because unprivileged attackers can modify file permissions and fool programs doing the access() -> open() dance. Passing a path to aa_query_label() is not entirely susceptible to the TOCTOU conditions known with access(2) because only a privileged user can load AppArmor profiles.

2. symlinks are a problem with this interface since the kernel isn't actually walking the path. Instead, the kernel is just using the path string passed to it in order to look up the set of allowed permissions.

This means that you'll need to fully resolve a symlink target and pass it to aa_query_label(). That also opens up the chance of another race condition. If the symlink file is user controllable, an attacker might make the symlink point to an innocent path, let a trusted helper resolve the innocent path and call aa_query_label(), and then win the race to adjust the symlink to point to a malicious path before the trusted helper calls open().

If the paths that you're performing queries on are not user controllable, I think you should be safe using aa_query_label(). If they are user controllable, you'll probably need to wait until we have an interface that accepts an open fd.

It is worth noting that the upcoming apparmor 2.10 release will have helper functions (aa_query_file_path and aa_query_file_path_len) that make it easier to query permissions for a file path.

  http://bazaar.launchpad.net/~apparmor-dev/apparmor/master/revision/3081

So I gave (2) by creating a symlink in a folder that a particular profile was could access to a file in folder it didn't have access to. The query_file utility attached to this bug said I was allowed access to the symlink.

So I think we need a bit more guidance on how to use this interface safely. I guess a call to realpath() could help with the symlink issue, but the issue of races if we're separating the access check from the use of the resource. Do we just decide that this isn't a problem worth solving, or is there some other way to use this API that I'm not seeing?

Okay, we've been experimenting with this in the thumbnailer, and will look to roll it out in the next landing. The first branch adds code that calls GetConnectionCredentials() to determine the peer's AppArmor label, while the second one adds aa_query_label based security checks based on the label. We were already canonicalising the path name with boost::filesystem::canonical(), so should be safe for the symlink issue.

I managed to get the format of the query message wrong when integrating the code first time, so I've attached a version of the query_file() method using std::string to build the message, which is a bit easier to understand.

Re: your symlink question. AppArmor is returning permissions regarding reading the symlink it self, which is a precursor to traversing the symlink to the file it is pointing at.

Fix committed into lp:thumbnailer/devel at revision 218, scheduled for release in thumbnailer, milestone Unknown

Fix committed into lp:thumbnailer/devel at revision 219, scheduled for release in thumbnailer, milestone Unknown

One thing that comes to mind is that any check that doesn't actually carry out the intended action (such as opening a file) is subject to race conditions. Ideallly, what I would like to say is "open this file for me as if I had the following privileges". As is, I think all I can say is "would I be allowed to open this file with the following privileges?" If the answer is "yes", by the time I actually go an open the file, it may not be the same file anymore. This seems exactly analogous to access(2).

It is analogous to access, however the set of races is smaller. Only the privileged MAC admin user can change the policy, where with access a user may change a files permissions. If you are using this to test whether you can open a file, in hopes that open() won't deny it, then yes this is similar to access, in that permissions can change. If this is being used by a trusted helper to determine check permissions that it enforces then it is different in that it is the trusted helper who ends up enforcing permissions. So it will depend on how/what you are using the interface for. With a split between kernel policy and user space decisions there will always be some potential for races; that even exists in the kernel as opening a file does not guarantee the rights to continue to access the file, those rights can be revoked by a policy replacement and subsequent writes or reads could fail.

With that said, yes we recognize the need for an fd based query, and other improvements to help expand what can be done safely from userspace

We're in the process of trying to land these changes for thumbnailer, and have been noticing problems with the music-app: we are getting denials from aa_query_label for files under ~/Music. For example:

    $ ./query_file com.ubuntu.music_music_2.1.867 /home/phablet/Music/10-amarillo.mp3
    read '/home/phablet/Music/10-amarillo.mp3' denied

However, the profile seems to be able to read files in that location anyway:

    $ aa-exec -p com.ubuntu.music_music_2.1.867 cat /home/phablet/Music/10-amarillo.mp3 >/dev/null

It seems the aa_query_label checks are working for ~/.local/share/$PACKAGE directories though, so it is working at some level:

    $ ./query_file com.ubuntu.music_music_2.1.867 /home/phablet/.local/share/com.ubuntu.music/foo
    read '/home/phablet/.local/share/com.ubuntu.music/foo' allowed
    $ ./query_file com.ubuntu.music_music_2.1.867 /home/phablet/.local/share/com.ubuntu.gallery/foo
    read '/home/phablet/.local/share/com.ubuntu.gallery/foo' denied

Is there something special about the way ~/Music access is enabled in the policy? I've been trying this out with devel-proposed (wily) image 233 on a Nexus 4 if that matters.

What is the return code for the failure, and is there a message logged in dmesg?

Marking this as critical because it's a showstopper bug: with this bug present, the music app shows nothing but "no artwork" thumbnails.

We considered skipping the security check in the thumbnailer to work around this, but that's not an option: without the security check, any app can go and ship out the contents of the photo roll...

It seems this was a transcription problem when I converted the code to C++, so never mind.

Foot in mouth. Bug in our code. Have unmarked this from critical.

Status changed to 'Confirmed' because the bug affects multiple users.