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

[John Johansen]

On 03/05/2013 09:05 PM, 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? Is there a chance path names could leak via this
> interface? (i.e. query for a path only readable by root, etc?) On the other
> hand, everything in /etc/apparmor.d/ is readable (not that it needs to be).
> Could it leak dynamic profiles? The list of profiles is 0640...
> 
yes it can leak profile names via probing
  ie. does profile X allow blah, oh it returned ENOENT, so that profile doesn't
exist

however you can prob other ways by say launching an app it seeing if its confined

If your confined you will be restricted and may not have access to the interface,
unconfined there is no restriction currently.

I am not sure what the correct perms is here and I raised that question in the
introspection patch that adds the profile tree, and nobody has answered.

I am unsure whether the correct solution for .access is to limit it to cap_mac_admin
Should a task be able to query its own permissions, as long as its given access
to the interface? Maybe, a task certainly could do probing instead and see what
permission is returned, of course there is always the threat of the task being
killed for that.

For the profile interface, who should get to see the list of profiles?
The admin sure, but what of the user? Should he get to see which of his tasks
are confined like is currently possible with ps -Z, is that different than
knowing the list of loaded profiles (yes).

Anyways for the dev tree I am happy to take the current patch but yeah we need
to figure out what permissions and additional checks these new files should have.