[apparmor] [PATCH v2] apparmor: implement profile-based query interface in apparmorfs
Tyler Hicks
tyhicks at canonical.com
Thu Mar 7 00:39:00 UTC 2013
On 2013-03-06 16:10:53, Seth Arnold wrote:
> 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'
Ack
>
> > 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.
Oh? I must have misunderstood the quiet flag. I thought quiet overrode
audit and deny overrode allow.
This section of the wiki doesn't help to clarify anything:
http://wiki.apparmor.net/index.php/AppArmor_Core_Policy_Reference#Rule_qualifiers
The quiet subsection doesn't convey confidence and the deny section is a
little strange. It says, "Deny rules also specifies noaudit," but
nowhere else on that wiki page is "noaudit" mentioned. I'm confused. :)
> What use is there for 'deny' in this interface? Is a 'loud explicit
> deny' something we're intending to add?
This was something that JJ requested while we were designing the
interface. IIRC, he said that deny is currently only honored at the
parser level but that we may start tracking it in the kernel one day so
he'd like to include a deny mask in this interface.
>
> > 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>
> > ---
> >
> > * 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.
You're right. I even had "NUL" there at one point and changed it for
some reason. I'll switch it back.
>
>
> > + * 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.
You'll have to take this one up with linux-fsdevel. count is the
conventional name of the third parameter of write()-related functions.
But, I suppose I can cave and change it if you have a better suggestion.
:)
>
> > + 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 :)
Thanks for the review!
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/20130306/76ca5b0f/attachment.pgp>
More information about the AppArmor
mailing list