On Tue, Mar 05, 2013 at 06:38:35PM -0800, Tyler Hicks wrote: This looks really good. A few nitpicks inline..
> 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',
^^^^^^^ should be 'query'
> 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
I may have tuned out a discussion on IRC about the 'deny' flags -- at
least it feels like a conversation I've ignored :) -- but the profiles
currently communicate 'deny' through the 'quiet' flags. What use is there
for 'deny' in this interface? Is a 'loud explicit deny' something we're
intending to add?
> 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 <tyhi...@canonical.com>
> ---
>
> * v2 includes fixes for JJ's review comments:
> - Rename QUERY_PROFILE and QUERY_PROFILE_LEN to QUERY_CMD_PROFILE and
> QUERY_CMD_PROFILE_LEN
> - Fix conditional around memcmp() in aa_write_access()
> + memcmp() should compare QUERY_CMD_PROFILE_LEN bytes, not count bytes
> + memcmp() returns 0 when successful, so check for !memcmp(,,)
> - Rename apparmorfs/access to apparmorfs/.access to match apparmorfs
> naming conventions
>
> security/apparmor/apparmorfs.c | 125
> +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 125 insertions(+)
>
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 3d8619d..4a0c128 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -188,6 +188,130 @@ 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.
Perhaps a bit nit-picky, but "NUL" is for the '\0' ascii string
terminator, "NULL" is for pointers.
> + * 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_CMD_PROFILE "profile\0"
> +#define QUERY_CMD_PROFILE_LEN 8
> +
> +/**
> + * 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)
> +{
Again, nit-picking :) but the name 'count' doesn't sound right; it's
being used as a buffer length indicator, not an interation counter.
> + 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_CMD_PROFILE_LEN &&
> + !memcmp(buf, QUERY_CMD_PROFILE, QUERY_CMD_PROFILE_LEN)) {
> + len = query_profile(buf, SIMPLE_TRANSACTION_LIMIT,
> + buf + QUERY_CMD_PROFILE_LEN,
> + count - QUERY_CMD_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 +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
Thanks :)
signature.asc
Description: Digital signature
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
