libapparmor's aa_query_label() always returns allowed = 0 for file rules containing the "owner" conditional

Bug #1620635 reported by Florian Boucault on 2016-09-06
14
This bug affects 2 people
Affects Status Importance Assigned to Milestone
AppArmor
High
Unassigned
Snappy
Undecided
Unassigned
apparmor (Ubuntu)
High
Unassigned

Bug Description

Steps to reproduce:
1. Download and compile the following sample C app that calls aa_query_label

wget https://launchpadlibrarian.net/207629699/query_file.c
gcc -o query_file query_file.c -l apparmor

2. Install a snap that uses the home interface, for example demo-wget:

snap install demo-wget

3. Create a file in your home:

touch /home/USERNAME/testfile

4. Ask apparmor if demo-wget can read that file with query_file:

./query_file snap.demo-wget.wget /home/USERNAME/testfile

Expected result:

output of ./query_file command is
read '/home/kaleo/toto' allowed

Current result:

output of ./query_file command is
read '/home/kaleo/toto' denied

Related branches

Tyler Hicks (tyhicks) wrote :

I think that the problem here stems from the fact that the home interface's
rules use the "owner" prefix:

 # Allow read access to toplevel $HOME for the user
 owner @{HOME}/ r,

 # Allow read/write access to all non-hidden files that aren't in ~/snap/
 owner @{HOME}/[^s.]** rwk,
 owner @{HOME}/s[^n]** rwk,
 owner @{HOME}/sn[^a]** rwk,
 owner @{HOME}/sna[^p]** rwk,
 # allow creating a few files not caught above
 owner @{HOME}/{s,sn,sna}{,/} rwk,

The kernel tracks these owner permissions differently than permissions that are
not tied to the owner. You can see that the allow vector is not 0x00 with strace:

 $ strace -s1024 ./query_file snap.demo-wget.wget /home/tyhicks/testfile
 ...
 read(3, "allow 0x00000200\ndeny 0x00000000\naudit 0x00000000\nquiet 0x00000000\n", 67) = 67
 ...

The allow vector is non-zero (0x00000200) but it isn't AA_MAY_READ
(0x00000011). Instead, I think AA_MAY_READ is being left shifted by the kernel
to indicate that the "owner" prefix was present on the rule. I'll need to
verify that and then discuss among the upstream developers what to do about
this in the libapparmor query interface.

Additionally, the query_file.c test program has a bug. It redefines
AA_MAY_READ, AA_MAY_WRITE, and AA_CLASS_FILE. It gets the definitions of
AA_MAY_READ and AA_MAY_WRITE wrong. Please just use the definitions provided by
<sys/apparmor.h>.

After making that change, you can remove the "owner" prefix from the rules that
grant access to $HOME in the snap.demo-wget.wget profile, reload the profile,
and then test program will work as expected. This confirms that the "owner" prefix causes the unexpected test program results.

Changed in apparmor:
status: New → Confirmed
Changed in apparmor (Ubuntu):
status: New → Confirmed
Tyler Hicks (tyhicks) wrote :

Marking the Snappy task as "Wont't Fix" for now. This theoretically could be fixed in snapd's home interface by dropping the "owner" prefix but I don't think that's the correct fix for this bug. Either libapparmor or the kernel need to handle the owner conditional better or the calling application should do another query for owned files.

Changed in snappy:
status: New → Won't Fix
summary: - libapparmor's aa_query_label() always returns allowed = 0 for snaps
+ libapparmor's aa_query_label() always returns allowed = 0 for file rules
+ containing the "owner" conditional
Tyler Hicks (tyhicks) wrote :

After thinking this through some more and discussing with John Johansen, the current query interface is not sufficient to support querying of permissions granted by owner file rules. The reason is that, when dealing with owner file rules, the decision to allow or not depends on two objects. The first is the file itself and the second is the UID associated with the process accessing the file. The current query interface only knows about the file and the UID associated with the process doing the *query*. The process doing the query is almost never the same as the process attempting to access the file.

Changed in apparmor:
status: Confirmed → Triaged
Tyler Hicks (tyhicks) on 2016-09-06
tags: added: aa-feature aa-kernel
Tyler Hicks (tyhicks) wrote :

Important is high as we'll need a fix soon in order for thumbnailer-service to run as a snap.

Changed in apparmor:
importance: Undecided → High
Changed in apparmor (Ubuntu):
importance: Undecided → Critical
importance: Critical → High
status: Confirmed → Triaged
Tyler Hicks (tyhicks) wrote :

Triaging this bug lead me to discover bug #1620791. This bug will need to be fixed before, or at the same time as, bug #1620791 is fixed.

Bill Filler (bfiller) on 2016-10-05
tags: added: snap-desktop-issue
Bill Filler (bfiller) wrote :

this is the bug that causes the thumbnails to be blank for camera-app and gallery-app snaps

Paweł Stołowski (stolowski) wrote :

This also affects scopes, we get empty art for thumbnailers uri such as image://thumbnailer/file:///snap/unity8-session/...

Paweł Stołowski (stolowski) wrote :

Please ignore my last comment about this affecting scopes... It was a different root cause afterwards (missing mime database and gdk-pixbuf loaders/cache - https://code.launchpad.net/~stolowski/unity8-session-snap/thumbnailer-fixes/+merge/310756 should fix it).

James Henstridge (jamesh) wrote :

I had a go writing a custom interface to allow thumbnailer to access the private files of another snap here:

https://github.com/snapcore/snapd/pull/2783

Unfortunately access to ~/snap/$name is also guarded by the "owner" modifier, so it suffers from the same problems as checking for access granted by the home interface. So this will be a problem on systems built on core as well as classic desktops.

John Johansen (jjohansen) wrote :

James, I can give you access to a custom kernel and library that provides a fix for the apparmor end if you would like. The issue is that these are not in the distro yet, and have not been backported to earlier releases (yet).

James Henstridge (jamesh) wrote :

John: that would be useful. Our code already tracks the peer's UID, so it will hopefully be quite easy to hook up what ever you've come up with.

Andrew Hayzen (ahayzen) wrote :

FWIW, this bug was stopping the webbrowser from being able to export a PDF to the printing 'app'.

This was due to content-hub using libapparmor to check if the app was able to read the path, and the webbrowser-app using an apparmor manifest which generated a rule with owner in it.

It would be good to get this fixed to prevent other apps using content-hub failing to import/export.

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

Other bug subscribers