lkml.org 
[lkml]   [2009]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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.




    \
     
     \ /
      Last update: 2009-08-15 03:37    [W:0.027 / U:0.324 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site