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

[John Johansen]

On 03/05/2013 09:52 PM, 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.
> 
well we can do something with in apparmor itself so, some additional check
beyond 0666. I agree unprivileged apps need to work with it, but we could
do things like limit queries against their own confinement, or subset
of their confinement, and/or add an apparmor "capability" controlling
the ability to query policy.

>> 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..)
> 
Like I said in my other reply it depends on what you mean by leak.
You can probe the interface, there are two ways to leak here, profile list,
and what a profile allows.

The profile list can be partially reconstructed by querying based on potential
profile names, bad names return ENOENT. However we already leak some of this
via ps -Z, at least from unprivileged unconfined users. Confined users need
accesss to the processes pid files, which is unlikely.

We can fix this leak by restricting what profiles can be queried if you
are confined. Maybe no one, maybe your self, maybe others if you have
cap MAC_ADMIN. We could make it part of policy or hard code. Its something
to think about.

Once you know about a profile and can query about it, the interface can leak
information, in a task can prob and find out if the profile grants access
to say dbus messages on address foo. Or paths once pathnames are hooked up

This leak is harder control, and I think the best solution is all or nothing
that is if you have rights to do access rights queries against the profile
then you can query which permissions the profile grants.

Again we need to design what those access rights look like.


>> 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.
> 
Well yes but we should probably be more careful with the kernel
interface in that if an admin decides to change /etc/apparmor.d/ access
the kernel shouldn't be leaking that info

>> Could it leak dynamic profiles? The list of profiles is 0640...
> 
We it is on the old interface but isn't on the new one yet. I asked
this as part of my query a couple submissions ago and got no reply.

There are cases where we will want to allow a user to access part
of the list, this is my biggest problem with the profile dirs as its
hard to virtualize the information where a single file makes this
easy.

> I don't immediately know this answer. Maybe JJ can chime in here.
> 
I am not sure of the answer but I think it is we need to grow some
policy around who can do queries that is separate from DAC as
DAC is insufficient for our needs.