Re: [PATCH] Security/sysfs: Enable security xattrs to be set on sysfs files, directories, and symlinks.

[Casey Schaufler <casey@schaufler-ca.com>]

Stephen Smalley wrote:
> On Fri, 2009-08-14 at 08:20 -0400, Stephen Smalley wrote:
>   
>> ...
>>> + */
>>> +static DEFINE_MUTEX(sysfs_xattr_lock);
>>> +
>>> +static struct sysfs_xattr *new_xattr(const char *name, const void *value,
>>> +					size_t size)
>>> +{
>>> +	struct sysfs_xattr *nxattr;
>>> +	void *nvalue;
>>> +	char *nname;
>>> +
>>> +	nxattr = kzalloc(sizeof(*nxattr), GFP_KERNEL);
>>> +	if (!nxattr)
>>> +		return NULL;
>>> +	nvalue = kzalloc(size, GFP_KERNEL);
>>> +	if (!nvalue) {
>>> +		kfree(nxattr);
>>> +		return NULL;
>>> +	}
>>> +	nname = kzalloc(strlen(name) + 1, GFP_KERNEL);
>>> +	if (!nname) {
>>> +		kfree(nxattr);
>>> +		kfree(nvalue);
>>> +		return NULL;
>>> +	}
>>> +	memcpy(nvalue, value, size);
>>> +	strcpy(nname, name);
>>> +	nxattr->sx_name = nname;
>>> +	nxattr->sx_value = nvalue;
>>> +	nxattr->sx_size = size;
>>>       
>> Storing the name/value pairs here is redundant - the security module
>> already has to store the value in some form (potentially smaller, like a
>> secid + struct in the SELinux case).  This wastes memory.
>>     
>
> Sorry - to clarify, I understand that we have to store a representation
> of the security attribute in the backing data structure so that it can
> be restored later, but that representation should come from the security
> module rather than being the original (name, value, size) triple.  Which
> is what David's patch does - he obtains a secid from the security module
> for storage in the wrapped iattr structure.
>   

Sorry, but I disagree with your assertion. An LSM can do what
it likes with the xattr, but the value sent from userland is
what should be stored.

>>> +
>>> +	return nxattr;
>>> +}
>>> +
>>> +int sysfs_setxattr(struct dentry *dentry, const char *name,
>>> +			const void *value, size_t size, int flags)
>>> +{
>>> +	struct sysfs_dirent *sd = dentry->d_fsdata;
>>> +	struct list_head *xlist;
>>> +	struct sysfs_xattr *nxattr;
>>> +	void *nvalue;
>>> +	int rc = 0;
>>> +
>>> +	/*
>>> +	 * Only support the security namespace.
>>> +	 * Only allow privileged processes to set them.
>>> +	 * It has to be OK with the LSM, if any, as well.
>>> +	 */
>>> +	if (strncmp(name, XATTR_SECURITY_PREFIX,
>>> +			sizeof XATTR_SECURITY_PREFIX - 1))
>>> +		return -ENOTSUPP;
>>> +
>>> +	if (!capable(CAP_SYS_ADMIN))
>>> +		return -EPERM;
>>>       
>> SELinux does not require CAP_SYS_ADMIN to set its attributes, so this
>> breaks its security model.
>>     
>
> And you don't need to apply any permission check here, as it gets
> covered by the security_inode_setxattr() hook in vfs_setxattr() prior to
> invoking i_op->setxattr.
>   

David seemed to think it necessary in an earlier review. I will
have another look.