[apparmor] [PATCH 25/32] apparmor: Add interface files for profiles and namespaces
John Johansen
john.johansen at canonical.com
Fri Feb 1 01:48:58 UTC 2013
On 01/31/2013 04:18 PM, Seth Arnold wrote:
> On Wed, Jan 16, 2013 at 01:28:54PM -0800, John Johansen wrote:
>> Add basic interface files to access namespace and profile information.
>> The interface files are created when a profile is loaded and removed
>> when the profile or namespace is removed.
>>
>> Signed-off-by: John Johansen <john.johansen at canonical.com>
>
>> /**
>> + * aa_mangle_name - mangle a profile name to std profile layout form
>> + * @name: profile name to mangle (NOT NULL)
>> + * @target: buffer to store mangled name, same length as @name (MAYBE NULL)
>> + *
>> + * Returns: length of mangled name
>> + */
>> +static int mangle_name(char *name, char *target)
>> +{
>> + char *t = target;
>> +
>> + while (*name == '/' || *name == '.')
>> + name++;
>> +
>> + if (target) {
>> + for (; *name; name++) {
>> + if (strchr("\"\'(){}[]", *name))
>> + continue;
>> + else if (*name == '/')
>> + *(t)++ = '.';
>> + else if (isspace(*name))
>> + *(t)++ = '_';
>> + else if (strchr("*?^$\\", *name))
>> + *(t)++ = 'X';
>> + else if (isgraph(*name))
>> + *(t)++ = *name;
>> + }
>> +
>> + *t = 0;
>> + } else {
>> + int len = 0;
>> + for (; *name; name++) {
>> + if (strchr("\"\'(){}[]", *name))
>> + continue;
>> + len++;
>> + }
>> +
>> + return len;
>> + }
>> +
>> + return t - target;
>> +}
>
> This returns a different length with target is NULL than when target
> is non-NULL -- the isgraph() catch-all at the end of the first branch
> doesn't have an analogue in the else branch.
>
yep, thanks
>> +/**
>> * aa_simple_write_to_buffer - common routine for getting policy from user
>> * @op: operation doing the user buffer copy
>> * @userbuf: user buffer to copy data from (NOT NULL)
>> @@ -182,8 +226,252 @@ const struct file_operations aa_fs_seq_file_ops = {
>> .release = single_release,
>> };
>>
>> -/** Base file system setup **/
>> +static int aa_fs_seq_profile_open(struct inode *inode, struct file *file,
>> + int (*show)(struct seq_file *, void *))
>> +{
>> + struct aa_replacedby *r = aa_get_replacedby(inode->i_private);
>> + int error = single_open(file, show, r);
>> +
>> + if (error)
>> + aa_put_replacedby(r);
>> +
>> + return error;
>> +}
>> +
>> +static int aa_fs_seq_profile_release(struct inode *inode, struct file *file)
>> +{
>> + aa_put_replacedby(inode->i_private);
>> + return single_release(inode, file);
>> +}
>
> A comment on lwn suggests being careful with _release() -- if _open()
> fails, _release() is still called, potentially with invalid data. Might
> this case lead to aa_put_replacedby() being called twice when it was
> only aa_get_replacedby() once?
hrmmm, good question I need to trace everything real carefully again but
it does look like we are doing an extra put if open fails.
>
>> +/* requires lock be held */
>> +int __aa_fs_profile_mkdir(struct aa_profile *profile, struct dentry *parent)
>> +{
>> + struct aa_profile *child;
>> + struct dentry *dent = NULL, *dir;
>> + int error;
>> +
>> + if (!parent) {
>> + dent = prof_dir(profile->parent);
>> + /* adding to parent that previously didn't have children */
>> + dent = securityfs_create_dir("profiles", dent);
>> + if (IS_ERR(dent))
>> + goto fail;
>> + prof_child_dir(profile->parent) = parent = dent;
>> + }
>> +
>> + if (!profile->dirname) {
>> + int len, id_len;
>> + len = mangle_name(profile->base.name, NULL);
>> + id_len = snprintf(NULL, 0, ".%ld", profile->ns->uniq_id);
>> +
>> + profile->dirname = kmalloc(len + id_len + 1, GFP_KERNEL);
>> + if (!profile->dirname)
>> + goto fail;
>> +
>> + mangle_name(profile->base.name, profile->dirname);
>> + sprintf(profile->dirname + len, ".%ld", profile->ns->uniq_id++);
>> + }
>
> .. and here's a case where the length difference could be trouble.
> Either len needs to be re-set with the final mangle_name() call or
> mangle_name needs to be fixed up -- this runs the risk of exposing
> kernel data to userspace. (Not much, mind, but not good for reliability
> either. :)
>
right the code assumes mangle name is consistent in the size returned
>> --- a/security/apparmor/audit.c
>> +++ b/security/apparmor/audit.c
>> +const char *const aa_profile_mode_names[] = {
>> + "enforce",
>> + "complain",
>> + "kill"
>> +};
>
> Does this need updating to include "unconfined"?
>
yes, there is actually already a trailing patch that hasn't been pushed yet
to fix this
>
> The rest of the patch looked speculative enough that I didn't review
> further.
>
> Thanks
>
>
>
More information about the AppArmor
mailing list