[apparmor] [PATCH 25/32] apparmor: Add interface files for profiles and namespaces

[Seth Arnold]

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.

> +/**
>   * 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?

> +/* 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. :)

> --- 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"?


The rest of the patch looked speculative enough that I didn't review
further.

Thanks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130131/af35697a/attachment.pgp>