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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 14 Aug 2009 18:33:16 -0700
From:	Casey Schaufler <casey@...aufler-ca.com>
To:	Stephen Smalley <sds@...ho.nsa.gov>
CC:	"David P. Quigley" <dpquigl@...ho.nsa.gov>, jmorris@...ei.org,
	Greg Kroah-Hartman <greg@...ah.com>, ebiederm@...ssion.com,
	linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org, selinux@...ho.nsa.gov
Subject: Re: [PATCH] Security/sysfs: Enable security xattrs to be set on sysfs
 files, directories, and symlinks.

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/