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

Bug #1620635 reported by Florian Boucault
20
This bug affects 3 people
Affects Status Importance Assigned to Milestone
AppArmor
Triaged
Medium
Unassigned
Snappy
Won't Fix
Undecided
Unassigned
apparmor (Ubuntu)
Triaged
Medium
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

Revision history for this message
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
Revision history for this message
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
Revision history for this message
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)
tags: added: aa-feature aa-kernel
Revision history for this message
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
Revision history for this message
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)
tags: added: snap-desktop-issue
Revision history for this message
Bill Filler (bfiller) wrote :

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

Revision history for this message
Paweł Stołowski (stolowski) wrote :

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

Revision history for this message
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).

Revision history for this message
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.

Revision history for this message
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).

Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Retriaging these down to Medium. People worked around this in different ways and High was obviously inflated since it isn't fixed yet (I just verified with 5.0.0-25.26-generic and apparmor 2.13.2-9ubuntu6.1).

Changed in apparmor:
importance: High → Medium
Changed in apparmor (Ubuntu):
importance: High → Medium
Revision history for this message
Alfred E. Neumayer (beidl) wrote :

We're hitting this bug in UBports Ubuntu Touch on the Sony Xperia X (4.4 kernel, xenial AppArmor) as in #6 and #12, apps using content-hub to request files from a confined application fail receiving the file due to aa_query_label returning a denial before trying transfering a file.

Revision history for this message
John Johansen (jjohansen) wrote :

How is content hub looking up the confinement (label) of the task. Are you using pids, looking through /proc/<pid>/, using aa_gettaskcon?

This will help with creating an interface wrapper for query_label so we can pass the needed information to the kernel.

Revision history for this message
John Johansen (jjohansen) wrote :

Alfred,

which version of apparmor userspace is Ubuntu touch using? You can use

apparmor_parser -V

to find out

Revision history for this message
Alfred E. Neumayer (beidl) wrote :

Nothing special from what I could gather, just:

-) Calling an internal utility function for getting the allow status:
https://github.com/ubports/content-hub/blob/xenial/src/com/ubuntu/content/detail/transfer.cpp#L187
-) Which eventually lands here:
https://github.com/ubports/content-hub/blob/xenial/src/com/ubuntu/content/utils.cpp#L354

No special pid lookups or anything.

Revision history for this message
Alfred E. Neumayer (beidl) wrote :

AppArmor userspace in use: AppArmor parser version 2.10.95

Revision history for this message
Alfred E. Neumayer (beidl) wrote :
Revision history for this message
John Johansen (jjohansen) wrote :

To answer the question posed on IRC.

I do not know at this time if any fix to this will be SRUed to Xenial.

A proper generic fix will require a new userspace api. The owner conditional can not be properly generically answered without subject context. This api can be fixed for the inquiring tasks subject querying against the the object, but the the generic case of querying where an external helper task H needs to query whether task A with profile P can access file F can not be fixed with the current api.

Fixing the query using the subjects task is possible to SRU Xenial. The generic fix of a new API will not be SRUed.

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

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.