[apparmor] [PATCH] apparmor: implement profile-based query interface in apparmorfs
Tyler Hicks
tyhicks at canonical.com
Tue Mar 5 21:01:49 UTC 2013
On 2013-03-05 12:52:08, John Johansen wrote:
> 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
Yes.
>
> > +
> > +/**
> > + * 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
Not only that, but I should be checking if it returns zero, not
non-zero. Otherwise, testing would have caught the problem that you
pointed out. It should be:
if (count > QUERY_PROFILE_LEN && !memcmp(buf, QUERY_PROFILE, QUERY_PROFILE_LEN))
Thanks!
>
> > + 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
Sure! I just reused the access name that smack and selinux use. Are you
happy enough with .access or is there something that you feel fits
better into the apparmorfs naming scheme?
Tyler
>
> > #ifdef CONFIG_SECURITY_APPARMOR_COMPAT_24
> > AA_FS_FILE_FOPS("profiles", 0640, &aa_fs_profiles_fops),
> > #endif
> >
>
> thanks Tyler
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130305/0af31851/attachment-0001.pgp>
More information about the AppArmor
mailing list