MIME-version: 1.0
Content-type: text/plain; charset=UTF-8
Content-transfer-encoding: 8BIT
Message-id: <55E3F0A2.2070102@samsung.com>
Date: Mon, 31 Aug 2015 15:13:54 +0900
From: jonghwa3.lee@samsung.com
User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101
 Thunderbird/31.6.0
To: Casey Schaufler <casey@schaufler-ca.com>, linux-kernel@vger.kernel.org,
        linux-security-module@vger.kernel.org
Cc: james.l.morris@oracle.com, serge@hallyn.com, sangbae90.lee@samsung.com,
        inki.dae@samsung.com
Subject: Re: [PATCH] security: smack: Add support automatic Smack labeling
References: <1440640704-21730-1-git-send-email-jonghwa3.lee@samsung.com>
 <1440640704-21730-2-git-send-email-jonghwa3.lee@samsung.com>
 <55E09B2E.3040805@schaufler-ca.com>
In-reply-to: <55E09B2E.3040805@schaufler-ca.com>
DLP-Filter: Pass
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 14392
Lines: 442

On 2015년 08월 29일 02:32, Casey Schaufler wrote:
> On 8/26/2015 6:58 PM, Jonghwa Lee wrote:
>> Current Smack object's label is always given by userspace.
>> So there might be a certain gap between the time of file creation
>> and the time of applying actual label. And because of the time gap,
>> it results unintended Smack denial time to time.
>>
>> If accessing a file occurs ahead of labeling, Smack module will check
>> the authority with uninitialized label and will prohibit the access
>> as a result. It shouldn't be happened labeling is done in proper time.
> The Smack label is assigned when the inode is created.
> The value will depend on the filesystem type, the state
> of transmute attributes, and the Smack label of the
> creating process. But it will always be assigned.
>
>> For the case that system can't gueratee that Smack labeling is done
>> before any other accesses to newly created file, this patch introdues
>> new interface called 'Automatic Smack labling'.
> The system guarantees that the Smack labeling is done
> before any other access can be attempted in all cases.
> Can you describe a situation where this does not occur?
>
Hi, Casey,

First of all, I`m thank you for review my humble patch.
I already explained the situation in cover letter but it seemed not enough.
(RFC cover : https://lkml.org/lkml/2015/8/26/782)

Let me explain it in detail. Here is the case,

A rule is defined for a process, 'process A',  in smack rule table.

...
Process A    device::A    arwx-
...

The object 'device::A' will be used to a device node that 'process A' will access.
However when the target device node is created  it's labeled with default label
which is inherited from any of filesystem, ancestor,  or creating process.
Let's say the default object label for devtmpfs is '_' which allows only read and
write access. So we need the specific labeling by the authorized process as like
udevd for the devtmpfs.

In normal, smack label and access control follow the sequences,

1. Kernel module driver loaded
2. New device node is created  (/dev/aaa ,  '_')
3. Udevd gets uevent and appies udev rule (/dev/aaa, 'device::A')
4. 'Process A' accesses the device node ('Process A' ---> 'device::A', MAY_WRITE)
5. Access is permitted.

However, when labeling isn't done in proper time, result will be different,

1. Kernel module driver loaded
2. New device node is created  (/dev/aaa ,  '_')
3. 'Process A' accesses the device node ('Process A' ---> '_', MAY_WRITE)
4. Access is prohibited

Can this situation be handled in current Smack subsystem?
If so, could you give me an idea how to handle it.


Best regards,
Jonghwa Lee

>> The label can be defined before file creation with absolute path.
> You could in a pathname based system like AppArmor or TOMOYO,
> but Smack is an object based system. You can't count on the
> pathname to identify the object you care about. There are too
> many facilities that can manipulate the filesystem namespace.
> You have to deal with chroot and filesystem namespaces just to
> start.
>
> That, and the performance impact would be prohibitive.
>
>> Then, when target file is created in generic manner, the pre-defined
>> label is applied automatically at once.
>>
>> Pre-defined label format follows below form.
>>
>>    <File path> <Smack Label>
>>
>> And it can add new entry to autolabel table via 'autolabel' in smackfs.
>>
>>    echo '<File path> <Smack Label>' > /sys/fs/smackfs/autolabel
>>
>> To view entries of autolabel table,
>>
>>    $cat /sys/fs/smackfs/autolabel
>>    /dev/device00 Label:A <enabled>
>>    /run/userfile0 Label:B <disabled>
>>
>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> Nacked-by: Casey Schaufler <casey@schaufler-ca.com>
>
>> ---
>>  security/smack/Kconfig     |   11 ++++
>>  security/smack/smack.h     |   23 ++++++++
>>  security/smack/smack_lsm.c |   66 +++++++++++++++++++++
>>  security/smack/smackfs.c   |  136 ++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 236 insertions(+)
>>
>> diff --git a/security/smack/Kconfig b/security/smack/Kconfig
>> index 271adae..002aa01 100644
>> --- a/security/smack/Kconfig
>> +++ b/security/smack/Kconfig
>> @@ -30,6 +30,17 @@ config SECURITY_SMACK_BRINGUP
>>  	  "permissive" mode of other systems.
>>  	  If you are unsure how to answer this question, answer N.
>>  
>> +config SECURITY_SMACK_AUTOLABEL
>> +	bool "Enable Automatic Smack Labeling"
>> +	depends on SECURITY_SMACK
>> +	default n
>> +	help
>> +	  To remove out the gap between file creation and actual labeling,
>> +	  this option gives the additional interface for automatic labeling.
>> +	  With this option enabled, a file will posseses the assigned label
>> +	  in autolabel table at creation.
>> +	  If you are unsure how to answer this question, answer N.
>> +
>>  config SECURITY_SMACK_NETFILTER
>>  	bool "Packet marking using secmarks for netfilter"
>>  	depends on SECURITY_SMACK
>> diff --git a/security/smack/smack.h b/security/smack/smack.h
>> index fff0c61..8b7ee83 100644
>> --- a/security/smack/smack.h
>> +++ b/security/smack/smack.h
>> @@ -24,6 +24,7 @@
>>  #include <linux/list.h>
>>  #include <linux/rculist.h>
>>  #include <linux/lsm_audit.h>
>> +#include <linux/namei.h>
>>  
>>  /*
>>   * Use IPv6 port labeling if IPv6 is enabled and secmarks
>> @@ -106,6 +107,9 @@ struct inode_smack {
>>  	struct smack_known	*smk_inode;	/* label of the fso */
>>  	struct smack_known	*smk_task;	/* label of the task */
>>  	struct smack_known	*smk_mmap;	/* label of the mmap domain */
>> +#ifdef CONFIG_SECURITY_SMACK_AUTOLABEL
>> +	struct smack_auto_label *smk_auto;
>> +#endif
>>  	struct mutex		smk_lock;	/* initialization lock */
>>  	int			smk_flags;	/* smack inode flags */
>>  };
>> @@ -480,4 +484,23 @@ static inline void smk_ad_setfield_u_net_sk(struct smk_audit_info *a,
>>  }
>>  #endif
>>  
>> +#ifdef CONFIG_SECURITY_SMACK_AUTOLABEL
>> +struct smack_auto_label {
>> +	bool disabled;
>> +	char *fpath;
>> +	struct list_head entry;
>> +	struct smack_known *label;
>> +	struct inode *inode;
>> +};
>> +
>> +extern struct list_head smack_autolabel_list;
>> +extern struct mutex smack_autolabel_lock;
>> +
>> +extern struct smack_known *smk_inode_bind_autolabel(struct inode *);
>> +extern void smk_inode_unbind_autolabel(struct inode *);
>> +#else
>> +#define smk_inode_bind_autolabel(inode)	NULL
>> +#define smk_inode_unbind_autolabel(inode)	do { } while (0)
>> +#endif /* CONFIG_SECURITY_SMACK_AUTOLABEL */
>> +
>>  #endif  /* _SECURITY_SMACK_H */
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 996c889..851e8b4 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -249,6 +249,65 @@ static int smk_bu_credfile(const struct cred *cred, struct file *file,
>>  #define smk_bu_credfile(cred, file, mode, RC) (RC)
>>  #endif
>>  
>> +#ifdef CONFIG_SECURITY_SMACK_AUTOLABEL
>> +/**
>> + * smk_inode_bind_autolabel - Check assigned label for this inode
>> + * @ip : the object of inode
>> + *
>> + * Return pre-assigned smack_known when exists, NULL if not.
>> + */
>> +struct smack_known *smk_inode_bind_autolabel(struct inode *ip)
>> +{
>> +	struct smack_auto_label *smk_auto;
>> +	struct inode_smack *isp;
>> +	struct smack_known *smk = NULL;
>> +	struct path p;
>> +
>> +	if (list_empty(&smack_autolabel_list))
>> +		goto out;
> Poor coding style. You can safely return NULL. There is no reason
> to goto out.
>
>> +
>> +	mutex_lock(&smack_autolabel_lock);
>> +	list_for_each_entry(smk_auto, &smack_autolabel_list, entry) {
>> +		if (smk_auto->disabled)
>> +			continue;
>> +
>> +		if (!smk_auto->inode) {
>> +			if (kern_path(smk_auto->fpath, 0, &p))
> Do you have any idea of the impact this would have on
> system performance?
>
>> +				continue;
>> +			else
>> +				smk_auto->inode = p.dentry->d_inode;
>> +		}
>> +
>> +		if (smk_auto->inode == ip) {
>> +			isp = ip->i_security;
>> +			isp->smk_auto = smk_auto;
>> +			smk = smk_auto->label;
>> +			smk_auto->disabled = true;
>> +			break;
>> +		}
>> +	}
>> +	mutex_unlock(&smack_autolabel_lock);
>> +out:
>> +	return smk;
>> +}
>> +
>> +/**
>> + * smk_inode_bind_autolabel - Re-activate autolabel for use in future
>> + * @ip : the object of inode
>> + */
>> +void smk_inode_unbind_autolabel(struct inode *ip)
>> +{
>> +	struct inode_smack *isp = ip->i_security;
>> +	struct smack_auto_label *smk_auto = isp->smk_auto;
>> +
>> +	if (smk_auto) {
>> +		smk_auto->inode = NULL;
>> +		smk_auto->disabled = false;
>> +		isp->smk_auto = NULL;
>> +	}
>> +}
>> +#endif /* CONFIG_SECURITY_SMACK_AUTOLABEL */
>> +
>>  /**
>>   * smk_fetch - Fetch the smack label from a file.
>>   * @name: type of the label (attribute)
>> @@ -1089,6 +1148,8 @@ static int smack_inode_unlink(struct inode *dir, struct dentry *dentry)
>>  		smk_ad_setfield_u_fs_inode(&ad, dir);
>>  		rc = smk_curacc(smk_of_inode(dir), MAY_WRITE, &ad);
>>  		rc = smk_bu_inode(dir, MAY_WRITE, rc);
>> +		if (rc == 0)
>> +			smk_inode_unbind_autolabel(ip);
>>  	}
>>  	return rc;
>>  }
>> @@ -1122,6 +1183,8 @@ static int smack_inode_rmdir(struct inode *dir, struct dentry *dentry)
>>  		smk_ad_setfield_u_fs_inode(&ad, dir);
>>  		rc = smk_curacc(smk_of_inode(dir), MAY_WRITE, &ad);
>>  		rc = smk_bu_inode(dir, MAY_WRITE, rc);
>> +		if (rc == 0)
>> +			smk_inode_unbind_autolabel(d_backing_inode(dentry));
>>  	}
>>  
>>  	return rc;
>> @@ -3445,6 +3508,9 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
>>  		if (!IS_ERR_OR_NULL(skp))
>>  			final = skp;
>>  
>> +		skp = smk_inode_bind_autolabel(inode);
>> +		if (skp != NULL)
>> +			final = skp;
>>  		/*
>>  		 * Transmuting directory
>>  		 */
>> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
>> index c20b154..28380ba 100644
>> --- a/security/smack/smackfs.c
>> +++ b/security/smack/smackfs.c
>> @@ -61,6 +61,9 @@ enum smk_inos {
>>  #if IS_ENABLED(CONFIG_IPV6)
>>  	SMK_NET6ADDR	= 23,	/* single label IPv6 hosts */
>>  #endif /* CONFIG_IPV6 */
>> +#ifdef CONFIG_SECURITY_SMACK_AUTOLABEL
>> +	SMK_AUTOLABEL	= 24,
>> +#endif
>>  };
>>  
>>  /*
>> @@ -746,6 +749,135 @@ static void smk_cipso_doi(void)
>>  	}
>>  }
>>  
>> +#ifdef CONFIG_SECURITY_SMACK_AUTOLABEL
>> +LIST_HEAD(smack_autolabel_list);
>> +DEFINE_MUTEX(smack_autolabel_lock);
>> +static int smk_add_autolabel(const char *fpath, const char *label)
>> +{
>> +	struct smack_auto_label *smk_auto;
>> +	struct smack_known *smk;
>> +	struct path p;
>> +
>> +	/*
>> +	 * Check whether given file path already exists in entry
>> +	 */
>> +	mutex_lock(&smack_autolabel_lock);
>> +	list_for_each_entry(smk_auto, &smack_autolabel_list, entry) {
>> +		if (!strcmp(smk_auto->fpath, fpath)) {
>> +			smk = smk_import_entry(label, 0);
>> +			if (!IS_ERR(smk))
>> +				smk_auto->label = smk;
>> +			mutex_unlock(&smack_autolabel_lock);
>> +			return 0;
>> +		}
>> +	}
>> +	mutex_unlock(&smack_autolabel_lock);
>> +
>> +	/*
>> +	 * Allocation for new autolabel candidate
>> +	 */
>> +	smk_auto = kzalloc(sizeof(*smk_auto), GFP_KERNEL);
>> +	if (smk_auto == NULL)
>> +		return -ENOMEM;
>> +
>> +	smk_auto->fpath = kzalloc(strlen(fpath), GFP_KERNEL);
>> +	if (smk_auto->fpath == NULL) {
>> +		kfree(smk_auto);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	strcpy(smk_auto->fpath, fpath);
>> +	smk_auto->label = smk_import_entry(label, 0);
>> +	if (IS_ERR(smk_auto->label)) {
>> +		kfree(smk_auto->fpath);
>> +		kfree(smk_auto);
>> +		return PTR_ERR(smk_auto->label);
>> +	}
>> +
>> +	mutex_lock(&smack_autolabel_lock);
>> +	list_add(&smk_auto->entry, &smack_autolabel_list);
>> +	mutex_unlock(&smack_autolabel_lock);
>> +
>> +	/*
>> +	 * If path is already existed, bind autolabel to it.
>> +	 */
>> +	if (!kern_path(fpath, 0, &p))
>> +		smk_inode_bind_autolabel(p.dentry->d_inode);
>> +
>> +	return 0;
>> +}
>> +
>> +static void *autolabel_seq_start(struct seq_file *s, loff_t *pos)
>> +{
>> +	return smk_seq_start(s, pos, &smack_autolabel_list);
>> +}
>> +
>> +static void *autolabel_seq_next(struct seq_file *s, void *v, loff_t *pos)
>> +{
>> +	return smk_seq_next(s, v, pos, &smack_autolabel_list);
>> +}
>> +
>> +static int autolabel_seq_show(struct seq_file *s, void *v)
>> +{
>> +	struct list_head *list = v;
>> +	struct smack_auto_label *smk_auto =
>> +		 list_entry(list, struct smack_auto_label, entry);
>> +
>> +	seq_printf(s, "%s %s %s\n",
>> +		smk_auto->fpath, smk_auto->label->smk_known,
>> +		smk_auto->disabled ? "(disabled)" : "(enabled)");
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct seq_operations autolabel_seq_ops = {
>> +	.start	= autolabel_seq_start,
>> +	.next	= autolabel_seq_next,
>> +	.show	= autolabel_seq_show,
>> +	.stop	= smk_seq_stop,
>> +};
>> +
>> +static int smk_open_autolabel(struct inode *inode, struct file *file)
>> +{
>> +	return seq_open(file, &autolabel_seq_ops);
>> +}
>> +
>> +static ssize_t smk_write_autolabel(struct file *file, const char __user *buf,
>> +				size_t count, loff_t *ppos)
>> +{
>> +	char *data;
>> +	char *tok[2];
>> +	int ret;
>> +
>> +	data = kzalloc(count + 1, GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	if (copy_from_user(data, buf, count))
>> +		return -EFAULT;
>> +
>> +	if (data[0] != '/')
>> +		return -EINVAL;
>> +
>> +	tok[0] = strsep(&data, " ");
>> +	tok[1] = data;
>> +
>> +	ret = smk_add_autolabel(tok[0], tok[1]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return count;
>> +}
>> +
>> +static const struct file_operations smk_autolabel_ops = {
>> +	.open		= smk_open_autolabel,
>> +	.read		= seq_read,
>> +	.llseek		= seq_lseek,
>> +	.write		= smk_write_autolabel,
>> +	.release	= seq_release,
>> +};
>> +#endif /* CONFIG_SECURITY_SMACK_AUTOLABEL */
>> +
>>  /**
>>   * smk_unlbl_ambient - initialize the unlabeled domain
>>   * @oldambient: previous domain string
>> @@ -2824,6 +2956,10 @@ static int smk_fill_super(struct super_block *sb, void *data, int silent)
>>  		[SMK_NET6ADDR] = {
>>  			"ipv6host", &smk_net6addr_ops, S_IRUGO|S_IWUSR},
>>  #endif /* CONFIG_IPV6 */
>> +#ifdef CONFIG_SECURITY_SMACK_AUTOLABEL
>> +		[SMK_AUTOLABEL] = {
>> +			"autolabel", &smk_autolabel_ops, S_IRUGO|S_IWUSR},
>> +#endif
>>  		/* last one */
>>  			{""}
>>  	};
>

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