[apparmor] [PATCH v2] apparmor: implement profile-based query interface in apparmorfs

[Kees Cook]

On Tue, Mar 05, 2013 at 09:52:18PM -0800, Tyler Hicks wrote:
> On 2013-03-05 21:05:57, Kees Cook wrote:
> > Hi,
> > 
> > On Tue, Mar 05, 2013 at 06:38:35PM -0800, Tyler Hicks wrote:
> > > Allow userspace applications to query for allowed, denied, audit, and
> > > quiet permissions using a profile name and a DFA match string. Userspace
> > > applications that wish to enforce access controls defined in the
> > > system's AppArmor policy can use this interface to perform access
> > > control lookups.
> > > 
> > > This patch adds a new file, called .access, to the apparmorfs root
> > > directory. The semantics of the .access file should be hidden behind a
> > > libapparmor interface, but the process for doing a query looks like
> > > this:
> > > 
> > > open("/sys/kernel/security/apparmor/.access", O_RDWR) = 3
> > > write(3, "profile\0/usr/bin/app\0 system\0org"..., 98) = 98
> > > read(3, "allow 0x000002\ndeny 0x000000\naud"..., 1024) = 59
> > > close(3) = 0
> > > 
> > > The write() buffer contains the prefix specific to the current type of
> > > current ("profile\0" in this case), the profile name followed by a '\0',
> > > and the binary DFA match string. The read() buffer contains the query
> > > results. Here's an example of the query results:
> > > 
> > > allow 0x000002
> > > deny 0x000000
> > > audit 0x000000
> > > quiet 0x000000
> > > 
> > > The returned masks can be compared to the permission mask of interest.
> > > In the above example, the permission represented by 0x000002 would be
> > > allowed and the action would not be audited. The permission represented
> > > by 0x000001 would not be allowed and an AVC audit message would need to
> > > be generated.
> > > 
> > > Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> > > [...]
> > > + * Returns: number of bytes written or -errno on failure
> > > + */
> > > +static ssize_t aa_write_access(struct file *file, const char __user *ubuf,
> > > +			       size_t count, loff_t *ppos)
> > > +{
> > > +	char *buf;
> > > +	ssize_t len;
> > > +
> > > +	if (*ppos)
> > > +		return -ESPIPE;
> > > [...]
> > > @@ -787,6 +911,7 @@ static struct aa_fs_entry aa_fs_entry_apparmor[] = {
> > >  	AA_FS_FILE_FOPS(".load", 0640, &aa_fs_profile_load),
> > >  	AA_FS_FILE_FOPS(".replace", 0640, &aa_fs_profile_replace),
> > >  	AA_FS_FILE_FOPS(".remove", 0640, &aa_fs_profile_remove),
> > > +	AA_FS_FILE_FOPS(".access", 0666, &aa_fs_access),
> > >  #ifdef CONFIG_SECURITY_APPARMOR_COMPAT_24
> > >  	AA_FS_FILE_FOPS("profiles", 0640, &aa_fs_profiles_fops),
> > >  #endif
> > 
> > This is a mode-666 file with no check for CAP_MAC_ADMIN. Is there a reason
> > this is so open?
> 
> Unprivileged user processes need to use this query interface. For
> example, the session dbus-daemon must be able to perform a query, so it
> needs read and write permissions.
> 
> I was initially uneasy about giving it 0666 perms, but I don't see a way
> around it.

Yeah, that's my memory of this discussion back in the day. It's a pretty
cool interface anyway, but I figured I should ask just to be sure. :)

> > Is there a chance path names could leak via this
> > interface? (i.e. query for a path only readable by root, etc?)
> 
> I don't think so. It currently only works based on profile names, so the
> aa_file_rules DFA is not currently queried. (I'm not sure if that will
> be added longterm or not..)
> 
> > On the other hand, everything in /etc/apparmor.d/ is readable (not
> > that it needs to be).
> 
> This was my main concern. A user process could quietly probe the
> security policy. But, as you pointed out, the policy is not currently
> treated as a secret.

Yeah. I think this is okay; e.g. what profiles currently allow reading
/etc/apparmor.d? :) I doubt they allow writing to this file either.

> > Could it leak dynamic profiles? The list of profiles is 0640...
> 
> I don't immediately know this answer. Maybe JJ can chime in here.

Yeah, though I can't think of a reason that profile names are secret beyond
something like process hiding ("Is someone running a confined libvirt
process?"). It seems like those things are already visible.

Anyway, I didn't mean to block the patch going in. I'm fine with it, but I
just wanted to double-check the state of these questions. :)

-Kees

-- 
Kees Cook