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

[John Johansen]

On 03/04/2013 06:23 PM, 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>

comments inlined

> ---
>  security/apparmor/apparmorfs.c | 124 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 124 insertions(+)
> 
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 3d8619d..9f23347 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -188,6 +188,129 @@ static const struct file_operations aa_fs_profile_remove = {
>  	.llseek = default_llseek,
>  };
>  
> +/**
> + * query_profile - queries a profile and writes permissions to buf
> + * @buf: the resulting permissions string is stored here (NOT NULL)
> + * @buf_len: size of buf
> + * @query: binary query string to match against the dfa
> + * @query_len: size of query
> + *
> + * The buffers pointed to by buf and query may overlap. The query buffer is
> + * parsed before buf is written to.
> + *
> + * The query should look like "PROFILE_NAME\0DFA_STRING" where PROFILE_NAME is
> + * the name of the profile, in the current namespace, that is to be queried and
> + * DFA_STRING is a binary string to match against the profile's DFA.
> + *
> + * PROFILE_NAME must be NULL terminated. DFA_STRING may contain NULL characters
> + * but must *not* be NULL terminated.
> + *
> + * Returns: number of characters written to buf or -errno on failure
> + */
> +static ssize_t query_profile(char *buf, size_t buf_len,
> +			     char *query, size_t query_len)
> +{
> +	struct aa_profile *profile;
> +	char *profile_name;
> +	size_t profile_name_len;
> +	u32 allow, audit, quiet;
> +
> +	if (!query_len)
> +		return -EINVAL;
> +
> +	profile_name = query;
> +	profile_name_len = strnlen(query, query_len);
> +	if (!profile_name_len || profile_name_len == query_len)
> +		return -EINVAL;
> +
> +	profile = aa_lookup_profile(aa_current_profile()->ns, profile_name);
> +	if (!profile)
> +		return -ENOENT;
> +
> +	if (unconfined(profile)) {
> +		allow = 0xFFFFFFFF;
> +		audit = 0x00000000;
> +		quiet = 0x00000000;
> +	} else if (profile->policy.dfa) {
> +		/**
> +		 * The extra byte is to account for the null byte between the
> +		 * profile name and dfa string. profile_name_len is greater
> +		 * than zero and less than query_len, so a byte can be safely
> +		 * added or subtracted.
> +		 */
> +		char *dfa = profile_name + profile_name_len + 1;
> +		size_t dfa_len = query_len - profile_name_len - 1;
> +		unsigned int state;
> +
> +		state = aa_dfa_match_len(profile->policy.dfa,
> +					 profile->policy.start[0],
> +					 dfa, dfa_len);
> +		allow = dfa_user_allow(profile->policy.dfa, state);
> +		audit = dfa_user_audit(profile->policy.dfa, state);
> +		quiet = dfa_user_quiet(profile->policy.dfa, state);
> +	} else {
> +		aa_put_profile(profile);
> +		return -EINVAL;
> +	}
> +	aa_put_profile(profile);
> +
> +	return scnprintf(buf, buf_len,
> +			 "allow %#08x\ndeny %#08x\naudit %#08x\nquiet %#08x\n",
> +			 allow, 0, audit, quiet);
> +}
> +
> +#define QUERY_PROFILE		"profile\0"
> +#define QUERY_PROFILE_LEN	8

can we make this QUERY_CMD_PROFILE, its not a big deal just more intuitive to me

> +
> +/**
> + * aa_write_access - generic permissions query
> + * @file: pointer to open apparmorfs/access file
> + * @ubuf: user buffer containing the complete query string (NOT NULL)
> + * @count: size of ubuf
> + * @ppos: position in the file (MUST BE ZERO)
> + *
> + * Allows for one permission query per open(), write(), and read() sequence.
> + * The only query currently supported is a profile-based query. For this query
> + * ubuf must begin with "profile\0", followed by the profile query specific
> + * format described in the query_profile() function documentation.
> + *
> + * 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;
> +
> +	buf = simple_transaction_get(file, ubuf, count);
> +	if (IS_ERR(buf))
> +		return PTR_ERR(buf);
> +
> +	if (count > QUERY_PROFILE_LEN && memcmp(buf, QUERY_PROFILE, count)) {
err shouldn't this memcmp be using QUERY_PROFILE_LEN not count

> +		len = query_profile(buf, SIMPLE_TRANSACTION_LIMIT,
> +				    buf + QUERY_PROFILE_LEN,
> +				    count - QUERY_PROFILE_LEN);
> +	} else
> +		len = -EINVAL;
> +
> +	if (len < 0)
> +		return len;
> +
> +	simple_transaction_set(file, len);
> +
> +	return count;
> +}
> +
> +static const struct file_operations aa_fs_access = {
> +	.write		= aa_write_access,
> +	.read		= simple_transaction_read,
> +	.release	= simple_transaction_release,
> +	.llseek		= generic_file_llseek,
> +};
> +
>  static int aa_fs_seq_show(struct seq_file *seq, void *v)
>  {
>  	struct aa_fs_entry *fs_file = seq->private;
> @@ -787,6 +910,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),

how about .access so that its hidden by default as its not really meant
for command line access

>  #ifdef CONFIG_SECURITY_APPARMOR_COMPAT_24
>  	AA_FS_FILE_FOPS("profiles", 0640, &aa_fs_profiles_fops),
>  #endif
> 

thanks Tyler