[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