| 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
| ||
|
Message-ID: <CA+RmS-_oMDkmtwYeq48HDMU0HE8hwe2w2cR5U77s5yeirCvw-Q@mail.gmail.com>
Date: Mon, 28 Nov 2016 14:04:15 -0600
From: David Graziano <david.graziano@...kwellcollins.com>
To: Paul Moore <paul@...l-moore.com>
Cc: golbi@....uni.torun.pl, Michal Wronski <michal.wronski@...il.com>,
linux-kernel@...r.kernel.org,
linux-security-module@...r.kernel.org,
Stephen Smalley <sds@...ho.nsa.gov>,
Seth Forshee <seth.forshee@...onical.com>,
ebiederm@...ssion.com, selinux@...ho.nsa.gov
Subject: Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support
On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore <paul@...l-moore.com> wrote:
> On Wed, Nov 9, 2016 at 11:25 AM, David Graziano
> <david.graziano@...kwellcollins.com> wrote:
>> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore <paul@...l-moore.com> wrote:
>>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>>> <david.graziano@...kwellcollins.com> wrote:
>>>> This patch adds support for generic extended attributes within the
>>>> POSIX message queues filesystem and setting them by consulting the LSM.
>>>> This is needed so that the security.selinux extended attribute can be
>>>> set via a SELinux named type transition on file inodes created within
>>>> the filesystem. The implementation and LSM call back function are based
>>>> off tmpfs/shmem.
>>>>
>>>> Signed-off-by: David Graziano <david.graziano@...kwellcollins.com>
>>>> ---
>>>> ipc/mqueue.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 46 insertions(+)
>>>
>>> Hi David,
>>>
>>> At first glance this looks reasonable to me, I just have a two
>>> questions/comments:
>>>
>>> * Can you explain your current need for this functionality? For
>>> example, what are you trying to do that is made easier by allowing
>>> greater message queue labeling flexibility? This helps put things in
>>> context and helps people review and comment on your patch.
>>>
>>> * How have you tested this? While this patch is not SELinux specific,
>>> I think adding a test to the selinux-testsuite[1] would be worthwhile.
>>> The other LSM maintainers may suggest something similar if they have
>>> an established public testsuite.
>>>
>>> [1] https://github.com/SELinuxProject/selinux-testsuite
>>
>> Hi Paul,
>>
>> I needed to write a selinux policy for a set of custom applications that use
>> POSIX message queues for their IPC. The queues are created by one
>> application and we needed a way for selinux to enforce which of the other
>> apps are able to read/write to each individual queue. Uniquely labeling them
>> based on the app that created them and the file name seemed to be our best
>> solution since it’s an embedded system and we don’t have restorecond to
>> handle any relabeling.
>
> In the future putting things like the above in the patch description
> can be helpful. In other words, instead of simply saying this allows
> you to better control the labels assigned to message queues, you could
> expand upon it by saying that this patch allows you to better control
> which applications have access to a given queue. Yes, I realize that
> is implied by better control over the labels, but being explicit is
> rarely a bad thing when it comes to patch descriptions.
>
> I've never rejected a patch for a description that was too lengthy,
> but I have rejected patches that need better descriptions ;)
>
>> To test this patch I used both a selinux enabled, buildroot based qemu target
>> with a customized selinux policy and test C app to create the mqueues. I also
>> tested with our real apps and selinux policy on our target hardware. I can
>> certainly look at adding a test to the selinux-testsuite if that would
>> be helpful.
>
> Please do. I've been requiring tests for all new SELinux
> functionality lately; this isn't strictly a SELinux patch but I think
> it is a good practice regardless.
Sorry for the delay. I have created a pull request within the
selinux-testsuite github
project with a set of mqueue tests.
>
>>>> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
>>>> index 0b13ace..512a546 100644
>>>> --- a/ipc/mqueue.c
>>>> +++ b/ipc/mqueue.c
>>>> @@ -35,6 +35,7 @@
>>>> #include <linux/ipc_namespace.h>
>>>> #include <linux/user_namespace.h>
>>>> #include <linux/slab.h>
>>>> +#include <linux/xattr.h>
>>>>
>>>> #include <net/sock.h>
>>>> #include "util.h"
>>>> @@ -70,6 +71,7 @@ struct mqueue_inode_info {
>>>> struct rb_root msg_tree;
>>>> struct posix_msg_tree_node *node_cache;
>>>> struct mq_attr attr;
>>>> + struct simple_xattrs xattrs; /* list of xattrs */
>>>>
>>>> struct sigevent notify;
>>>> struct pid *notify_owner;
>>>> @@ -254,6 +256,7 @@ static struct inode *mqueue_get_inode(struct super_block *sb,
>>>> info->attr.mq_maxmsg = attr->mq_maxmsg;
>>>> info->attr.mq_msgsize = attr->mq_msgsize;
>>>> }
>>>> + simple_xattrs_init(&info->xattrs);
>>>> /*
>>>> * We used to allocate a static array of pointers and account
>>>> * the size of that array as well as one msg_msg struct per
>>>> @@ -413,6 +416,41 @@ static void mqueue_evict_inode(struct inode *inode)
>>>> put_ipc_ns(ipc_ns);
>>>> }
>>>>
>>>> +/*
>>>> + * Callback for security_inode_init_security() for acquiring xattrs.
>>>> + */
>>>> +static int mqueue_initxattrs(struct inode *inode,
>>>> + const struct xattr *xattr_array,
>>>> + void *fs_info)
>>>> +{
>>>> + struct mqueue_inode_info *info = MQUEUE_I(inode);
>>>> + const struct xattr *xattr;
>>>> + struct simple_xattr *new_xattr;
>>>> + size_t len;
>>>> +
>>>> + for (xattr = xattr_array; xattr->name != NULL; xattr++) {
>>>> + new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
>>>> + if (!new_xattr)
>>>> + return -ENOMEM;
>>>> + len = strlen(xattr->name) + 1;
>>>> + new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,
>>>> + GFP_KERNEL);
>>>> + if (!new_xattr->name) {
>>>> + kfree(new_xattr);
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + memcpy(new_xattr->name, XATTR_SECURITY_PREFIX,
>>>> + XATTR_SECURITY_PREFIX_LEN);
>>>> + memcpy(new_xattr->name + XATTR_SECURITY_PREFIX_LEN,
>>>> + xattr->name, len);
>>>> +
>>>> + simple_xattr_list_add(&info->xattrs, new_xattr);
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int mqueue_create(struct inode *dir, struct dentry *dentry,
>>>> umode_t mode, bool excl)
>>>> {
>>>> @@ -443,6 +481,14 @@ static int mqueue_create(struct inode *dir, struct dentry *dentry,
>>>> ipc_ns->mq_queues_count--;
>>>> goto out_unlock;
>>>> }
>>>> + error = security_inode_init_security(inode, dir,
>>>> + &dentry->d_name,
>>>> + mqueue_initxattrs, NULL);
>>>> + if (error && error != -EOPNOTSUPP) {
>>>> + spin_lock(&mq_lock);
>>>> + ipc_ns->mq_queues_count--;
>>>> + goto out_unlock;
>>>> + }
>>>>
>>>> put_ipc_ns(ipc_ns);
>>>> dir->i_size += DIRENT_SIZE;
>>>> --
>>>> 1.9.1
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
>>>> the body of a message to majordomo@...r.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>> --
>>> paul moore
>>> www.paul-moore.com
>
>
>
> --
> paul moore
> www.paul-moore.com
Powered by blists - more mailing lists