[PATCH] [RFC] security: smack: Add support automatic Smack labeling

[PATCH] [RFC] security: smack: Add support automatic Smack labeling

[Jonghwa Lee]

Smack labeling is always done in user space, not in kernel space.
Because kernel can't know which node or preocess should has the certain label
and it shouldn't be neither. Therefore there is a distinct time gap between
file creation and the labeling which might be about few miliseconds or even
indefinite period. This unavoidable and imprecise time gap produces unintended
Smack denial time to time. If it guerantees that labeling is done before any
other access, the time gap doesn't make matter. However if not, the system
will suffer from false alarm.

I've already exeperienced the situation with specific cases.
Mostly, it happens with devtmpfs where udevd applies xattr with defined udev
rules. When kernel module is loaded, device node is newly created and it sends
uevent and notification to processes in wait. However, somtimes uevent may be
handled lately then other processes's access. So, Smack module checks the
authority with uninitialized label, and prohibits the access even the access
authority exists in Smack rule.

At the first time of encounting the problem, I tries to fix the system and user
process to prevent the accesses in ahead of labeling but it wasn't the solution.
Performance is degraded because of compulsory delay.

Another candidate of solution is given label from kernel side.
However, kernel should not be aware of the label, the string, and the
relationship between specific node and label. So I left it in user space.

Label is still given from user space. Kernel only checks there is a pre-assigned
labeling rule for the node. If so, the file will acquire the label as default at
the creation. A file is created with pre-assigend label as like it's done
automatically. So it's called 'Auto Smack labeling'

Pre-assigned label can be given via additional file, 'autolabel' in smackfs.

   echo '<File path> <Smack Label>' > /sys/fs/smackfs/autolabel

The label only activates when the file isn't created yet. If file already
exists, then just add the label relationship to table and deactivate it.
When the label is applied, it'll also be deactivated. The label turns to be
enabled again only when the file is removed.

This is a candidate of solution for the specific problem, and might be buggy or
hamful for system-wide security. So I gently request your opinion for more clear
and wise solution for the case that I faced.

Jonghwa Lee (1):
  security: smack: Add support automatic Smack labeling

 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(+)

-- 
1.7.9.5

[PATCH] security: smack: Add support automatic Smack labeling

[Jonghwa Lee]

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.

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 label can be defined before file creation with absolute path.
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>
---
 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;
+
+	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))
+				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 */
 			{""}
 	};
-- 
1.7.9.5

Re: [PATCH] security: smack: Add support automatic Smack labeling

[Casey Schaufler]

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?

> 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 */
>  			{""}
>  	};

Re: [PATCH] security: smack: Add support automatic Smack labeling

[jonghwa3.lee]

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 */
>>  			{""}
>>  	};
>

Re: [PATCH] security: smack: Add support automatic Smack labeling

[Lukasz Pawelczyk]

On pon, 2015-08-31 at 15:13 +0900, jonghwa3.lee@samsung.com wrote:
> 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.

This doesn't seem to be a Smack problem. This isn't even a kernel
problem. It's userspace race. You should wait for a proper udev event
that notifies after all udev rules are applied.

I think there are 2 udev events. One that notifies that a device has
been added. Second that notifies where all the rules for the device has
been applied. You need to use the second one.



-- 
Lukasz Pawelczyk
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH] security: smack: Add support automatic Smack labeling

[jonghwa3.lee]

On 2015년 08월 31일 22:59, Lukasz Pawelczyk wrote:
> On pon, 2015-08-31 at 15:13 +0900, jonghwa3.lee@samsung.com wrote:
>> 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.
> This doesn't seem to be a Smack problem. This isn't even a kernel
> problem. It's userspace race. You should wait for a proper udev event
> that notifies after all udev rules are applied.
>
> I think there are 2 udev events. One that notifies that a device has
> been added. Second that notifies where all the rules for the device has
> been applied. You need to use the second one.
>
>
>
Yes you're right, it's not a problem of neither Smack nor kernel. However it will
help to resolve the problem if there is a proper way to label handled by kernel
at least for files created by kernel.(e.g. device node)

I'm not sure whether there is a uevent for completion of applying rule.
 (I couldn't catch any of such uevent with udevadm.)
However even there is, I think kernel side labeling has obvious advantages.

The every new files need proper labeling before working under Smack.
The files created by user application can be labeled by the same process at once.
So it doesn't need to consider delayed labeling before access.
However, the files created by kernel has to wait user space's control to be used.
The timing of user space's labeling is not precised. It can be delayed indefinitely.
Yes it's userspace race, but if kernel help it, It can prevent such issues.

My proposal might be not quite fancy. (It could degrade system's performance
severely as like Casey pointed out) But I'd like to ask whether this attempt is
useless.
Do you think that is just userspace's affair and couldn't be solved with
kernel's help?
I gently ask your comments. 

Thanks,
Jonghwa

Re: [PATCH] security: smack: Add support automatic Smack labeling

[Casey Schaufler]

On 9/1/2015 1:01 AM, jonghwa3.lee@samsung.com wrote:
> On 2015년 08월 31일 22:59, Lukasz Pawelczyk wrote:
>> On pon, 2015-08-31 at 15:13 +0900, jonghwa3.lee@samsung.com wrote:
>>> 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.
>> This doesn't seem to be a Smack problem. This isn't even a kernel
>> problem. It's userspace race. You should wait for a proper udev event
>> that notifies after all udev rules are applied.
>>
>> I think there are 2 udev events. One that notifies that a device has
>> been added. Second that notifies where all the rules for the device has
>> been applied. You need to use the second one.
>>
>>
>>
> Yes you're right, it's not a problem of neither Smack nor kernel. However it will
> help to resolve the problem if there is a proper way to label handled by kernel
> at least for files created by kernel.(e.g. device node)
>
> I'm not sure whether there is a uevent for completion of applying rule.
>  (I couldn't catch any of such uevent with udevadm.)
> However even there is, I think kernel side labeling has obvious advantages.
>
> The every new files need proper labeling before working under Smack.
> The files created by user application can be labeled by the same process at once.
> So it doesn't need to consider delayed labeling before access.
> However, the files created by kernel has to wait user space's control to be used.
> The timing of user space's labeling is not precised. It can be delayed indefinitely.
> Yes it's userspace race, but if kernel help it, It can prevent such issues.
>
> My proposal might be not quite fancy. (It could degrade system's performance
> severely as like Casey pointed out) But I'd like to ask whether this attempt is
> useless.

The problem is that you're fixing the problem in the wrong place.
I see that there is an issue, but there are several ways you could
address it in udev. You could change udev so that instead of creating
the device and changing it's Smack label you could create the device
with a "-not-yet" suffix, change it's Smack label, then rename it to
the proper name. There are other approaches as well.

> Do you think that is just userspace's affair and couldn't be solved with
> kernel's help?

It *can* be fixed with the kernel's help, but it shouldn't.

> I gently ask your comments. 
>
> Thanks,
> Jonghwa
>
> --
> 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/
>