On 03/06/2013 04:39 PM, Tyler Hicks wrote:
> 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. :)
>
Alright let me clear something up. What is stored in policy is not the same
as what the language.
deny in the language is an explicit deny and also implies quiet. These are
carried separately within the parser for good reasons.
The quiet flag (currently unsupported) will allow setting the quiet field
independent of deny.
the noaudit part needs to be fixed, as that part was writen long ago
when it was first being speced. An for what ever reason the term quiet
wasn't being used.
The reason deny implies quiet is the original use case for deny was a way
for the policy to store decisions that had already been made in the
profile learning tools. After a first pass at this it was noted that
the logs where still being filled with entries that where known about
and not wanted. Because these messages where not exceptional, in fact
expected otherwise the policy wouldn't store them in this use case,
it was decided that deny should also quiet the messages.
The use case of explicit deny followed, as well as wanting to audit some
of these. If I could undo the conflating of audit/allow, and quiet/deny
I probably would but we have stuck with the original for backwards
compatibility.
>> 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.
>
right, though with it being an extensible text string, I am ambivalent about
it being in atm.
<< snip >>
>>> +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.
> :)
heh I don't care either way
>
>>
>>> + 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
>
>
>
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
