Re: [PATCH v3 1/1] Introducing domain transition mechanism into Smack.

[Casey Schaufler]

On 9/29/2015 7:41 AM, Zbigniew Jasinski wrote:
> This feature consists of two, new kernel interfaces:
>
> - <smack_fs>/relabel-possible - for setting transition privilege
>
> This flag can be set only by process to itself and only with CAP_MAC_ADMIN
> capability. With this flag on, process can change it's label without
> CAP_MAC_ADMIN but only once. After label changing flag is unset.
>
> - <smack_fs>/relabel-list - for transition-labels list
>
> This list is used to control smack label transition mechanism.
> Process can transit to new label only if label is on the list.
> Only process with CAP_MAC_ADMIN capability can add label to this list.
>
> Changes in v2:
> * use list_for_each_entry instead of _rcu during label write
> * added missing description in security/Smack.txt
>
> Changes in v3:
> * squashed into one commit
>
> Signed-off-by: Zbigniew Jasinski <z.jasinski@xxxxxxxxxxx>
> Signed-off-by: Rafal Krypa <r.krypa@xxxxxxxxxxx>
> ---
>  Documentation/security/Smack.txt |  13 +++
>  security/smack/smack.h           |   9 ++
>  security/smack/smack_lsm.c       |  27 ++++-
>  security/smack/smackfs.c         | 221 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 268 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/security/Smack.txt b/Documentation/security/Smack.txt
> index 5e6d07f..0ffd194 100644
> --- a/Documentation/security/Smack.txt
> +++ b/Documentation/security/Smack.txt
> @@ -255,6 +255,19 @@ unconfined
>  	the access permitted if it wouldn't be otherwise. Note that this
>  	is dangerous and can ruin the proper labeling of your system.
>  	It should never be used in production.
> +relabel-possible
> +	This interface is used to set relabel-possible flag. A process
> +	with this flag is able to change its label without CAP_MAC_ADMIN,
> +	but only once. After label transition this flag is zeroed.
> +	0 - default: process is not allowed to label transition
> +	1 - process is allowed to one-time label tranistion
> +relabel-list
> +	This interface contains a list of labels, in which process can
> +	transition to. The format accepted on write is:
> +                "%s"
> +	for adding label, and:
> +		"-%s"
> +	for removing label from list.

This interface has a significant drawback in that you can't
limit the labels that are permitted for relabeling differently
for multiple processes. The problem with that is that you have to
make every label you might want to give anyone available to all
of the relabel-possible processes. That could be dangerous if you're
not careful.

I suggest that you drop the relabel-possible interface and
attach the relabel-list to the task instead of maintaining
a global list. Yes, it's more work, but the result is safer.

>  If you are using the smackload utility
>  you can add access rules in /etc/smack/accesses. They take the form:
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index fff0c61..db4a1a3 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -115,6 +115,7 @@ struct task_smack {
>  	struct smack_known	*smk_forked;	/* label when forked */
>  	struct list_head	smk_rules;	/* per task access rules */
>  	struct mutex		smk_rules_lock;	/* lock for the rules */
> +	int	smk_relabel;	/* task can change its label */
>  };

Change this to
	struct list_head smk_relabel;

In smack_task_alloc() initialize it to an empty list.

>  
>  #define	SMK_INODE_INSTANT	0x01	/* inode is instantiated */
> @@ -192,6 +193,12 @@ enum {
>  	Opt_fstransmute = 5,
>  };
>  
> +struct smack_relabel {
> +	struct rcu_head         rcu;
> +	struct list_head	list;
> +	struct smack_known	*smk_label;
> +};
> +
>  /*
>   * Mount options
>   */
> @@ -332,6 +339,8 @@ extern struct list_head smk_net6addr_list;
>  extern struct mutex     smack_onlycap_lock;
>  extern struct list_head smack_onlycap_list;
>  
> +extern struct list_head smack_relabel_list;
> +

No need for a global list.

>  #define SMACK_HASH_SLOTS 16
>  extern struct hlist_head smack_known_hash[SMACK_HASH_SLOTS];
>  
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 996c889..596f270 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -66,6 +66,8 @@ static const match_table_t smk_mount_tokens = {
>  	{Opt_error, NULL},
>  };
>  
> +LIST_HEAD(smack_relabel_list);
> +

No need for a global list

>  #ifdef CONFIG_SECURITY_SMACK_BRINGUP
>  static char *smk_bu_mess[] = {
>  	"Bringup Error",	/* Unused */
> @@ -325,6 +327,7 @@ static struct task_smack *new_task_smack(struct smack_known *task,
>  
>  	tsp->smk_task = task;
>  	tsp->smk_forked = forked;
> +	tsp->smk_relabel = 0;

Instead use:

	INIT_LIST_HEAD(&tsp->smk_relabel);

>  	INIT_LIST_HEAD(&tsp->smk_rules);
>  	mutex_init(&tsp->smk_rules_lock);
>  
> @@ -1953,6 +1956,7 @@ static int smack_cred_prepare(struct cred *new, const struct cred *old,
>  	if (rc != 0)
>  		return rc;
>  
> +	new_tsp->smk_relabel = old_tsp->smk_relabel;

You'll have to duplicate the list if there is one.

>  	new->security = new_tsp;
>  	return 0;
>  }
> @@ -3549,9 +3553,11 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>  static int smack_setprocattr(struct task_struct *p, char *name,
>  			     void *value, size_t size)
>  {
> -	struct task_smack *tsp;
> +	struct task_smack *tsp = current_security();
>  	struct cred *new;
>  	struct smack_known *skp;
> +	struct smack_relabel *srp;
> +	int rc;
>  
>  	/*
>  	 * Changing another process' Smack value is too dangerous
> @@ -3560,7 +3566,7 @@ static int smack_setprocattr(struct task_struct *p, char *name,
>  	if (p != current)
>  		return -EPERM;
>  
> -	if (!smack_privileged(CAP_MAC_ADMIN))
> +	if (!smack_privileged(CAP_MAC_ADMIN) && !tsp->smk_relabel)

	if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(tsp->smk_relabel))

>  		return -EPERM;
>  
>  	if (value == NULL || size == 0 || size >= SMK_LONGLABEL)
> @@ -3579,12 +3585,29 @@ static int smack_setprocattr(struct task_struct *p, char *name,
>  	if (skp == &smack_known_web)
>  		return -EPERM;
>  
> +	if (tsp->smk_relabel) {

	if (!list_empty(tsp->smk_relabel)) {

> +		rc = -EPERM;
> +		rcu_read_lock();
> +		list_for_each_entry_rcu(srp, &smack_relabel_list, list)
> +			if (srp->smk_label == skp) {
> +				rc = 0;
> +				break;
> +			}
> +		rcu_read_unlock();
> +		if (rc)
> +			return rc;
> +	}
> +
>  	new = prepare_creds();
>  	if (new == NULL)
>  		return -ENOMEM;
>  
>  	tsp = new->security;
>  	tsp->smk_task = skp;
> +	/*
> +	 * process can change its label only once
> +	 */
> +	tsp->smk_relabel = 0;

	smk_discard_relabel(tsp);

>  
>  	commit_creds(new);
>  	return size;
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index c20b154..11012f1 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -61,6 +61,8 @@ enum smk_inos {
>  #if IS_ENABLED(CONFIG_IPV6)
>  	SMK_NET6ADDR	= 23,	/* single label IPv6 hosts */
>  #endif /* CONFIG_IPV6 */
> +	SMK_RELABEL_POSSIBLE = 24, /* relabel possible without CAP_MAC_ADMIN */

Unnecessary.

> +	SMK_RELABEL_LIST = 25,
>  };
>  
>  /*
> @@ -72,6 +74,7 @@ static DEFINE_MUTEX(smk_net4addr_lock);
>  #if IS_ENABLED(CONFIG_IPV6)
>  static DEFINE_MUTEX(smk_net6addr_lock);
>  #endif /* CONFIG_IPV6 */
> +static DEFINE_MUTEX(smack_relabel_list_lock);
>  
>  /*
>   * This is the "ambient" label for network traffic.
> @@ -2700,6 +2703,218 @@ static const struct file_operations smk_syslog_ops = {
>  
>  
>  /**

The _relabel interfaces are unnecessary.

> + * smk_read_relabel - read() for smackfs/relabel
> + * @filp: file pointer, not actually used
> + * @buf: where to put the result
> + * @count: maximum to send along
> + * @ppos: where to start
> + *
> + * Returns number of bytes read or error code, as appropriate
> + */
> +static ssize_t smk_read_relabel(struct file *filp, char __user *buf,
> +				size_t count, loff_t *ppos)
> +{
> +	struct task_smack *tsp = current_security();
> +	char temp[32];
> +	ssize_t rc;
> +
> +	if (*ppos != 0)
> +		return 0;
> +
> +	sprintf(temp, "%d\n", tsp->smk_relabel);
> +	rc = simple_read_from_buffer(buf, count, ppos, temp, strlen(temp));
> +	return rc;
> +}
> +
> +/**
> + * smk_write_relabel - write() for /smack/relabel
> + * @file: file pointer, not actually used
> + * @buf: where to get the data from
> + * @count: bytes sent
> + * @ppos: where to start
> + *
> + * Returns number of bytes written or error code, as appropriate
> + */
> +static ssize_t smk_write_relabel(struct file *file, const char __user *buf,
> +				size_t count, loff_t *ppos)
> +{
> +	struct task_smack *tsp = current_security();
> +	struct cred *creds;
> +	int r;
> +
> +	if (!smack_privileged(CAP_MAC_ADMIN))
> +		return -EPERM;
> +
> +	if (count == 0 || count > 2)
> +		return -EINVAL;
> +
> +	if (kstrtoint_from_user(buf, count, 0, &r))
> +		return -EFAULT;
> +
> +	if (r != 0 && r != 1)
> +		return -EINVAL;
> +
> +	creds = prepare_creds();
> +	if (creds == NULL)
> +		return -ENOMEM;
> +
> +	tsp = creds->security;
> +	tsp->smk_relabel = r;
> +
> +	commit_creds(creds);
> +	return count;
> +}
> +
> +
> +static const struct file_operations smk_relabel_possible_ops = {
> +	.read		= smk_read_relabel,
> +	.write		= smk_write_relabel,
> +	.llseek		= default_llseek,
> +};
> +

relabel_list_ becomes task specific. You set it on yourself.

> +/*
> + * Seq_file read operations for /smack/relabel-list
> + */
> +
> +static void *relabel_list_seq_start(struct seq_file *s, loff_t *pos)
> +{

	struct task_smack *tsp = smk_of_current();

> +	return smk_seq_start(s, pos, &tsp->smk_relabel);
> +}
> +
> +static void *relabel_list_seq_next(struct seq_file *s, void *v, loff_t *pos)
> +{
> +	return smk_seq_next(s, v, pos, &smack_relabel_list);

...

> +}
> +
> +static int relabel_list_seq_show(struct seq_file *s, void *v)
> +{
> +	struct list_head *list = v;
> +	struct smack_relabel *srp =
> +		list_entry_rcu(list, struct smack_relabel, list);
> +
> +	seq_printf(s, "%s\n", srp->smk_label->smk_known);
> +
> +	return 0;
> +}

...

> +
> +static const struct seq_operations relabel_list_seq_ops = {
> +	.start = relabel_list_seq_start,
> +	.next  = relabel_list_seq_next,
> +	.show  = relabel_list_seq_show,
> +	.stop  = smk_seq_stop,
> +};
> +
> +/**
> + * smk_open_relabel_list - open() for /smack/relabel-list
> + * @inode: inode structure representing file
> + * @file: "relabel-list" file pointer
> + *
> + * For reading, use load2_seq_* seq_file reading operations.
> + */
> +static int smk_open_relabel_list(struct inode *inode, struct file *file)
> +{
> +	return seq_open(file, &relabel_list_seq_ops);
> +}
> +
> +/**
> + * smk_write_relabel_list - write() for /smack/relabel-list
> + * @file: file pointer, not actually used
> + * @buf: where to get the data from
> + * @count: bytes sent
> + * @ppos: where to start - must be 0
> + *
> + */
> +static ssize_t smk_write_relabel_list(struct file *file, const char __user *buf,
> +				size_t count, loff_t *ppos)
> +{
> +	struct smack_known *skp;
> +	struct smack_relabel *srp;
> +	int rc = count;
> +	int remove;
> +	char *data;
> +	char *label;
> +
> +	/*
> +	 * Must have privilege.
> +	 */
> +	if (!smack_privileged(CAP_MAC_ADMIN))
> +		return -EPERM;
> +
> +	/*
> +	 * Enough data must be present.
> +	 * One label per line.
> +	 */
> +	if (*ppos != 0 || count >= SMK_LONGLABEL)
> +		return -EINVAL;
> +
> +	data = kzalloc(count + 1, GFP_KERNEL);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(data, buf, count) != 0) {
> +		kfree(data);
> +		return -EFAULT;
> +	}
> +
> +	if (data[0] == '-') {
> +		remove = 1;
> +		label = smk_parse_smack(data + 1, count - 1);
> +		if (IS_ERR(label)) {
> +			kfree(data);
> +			return PTR_ERR(label);
> +		}
> +		skp = smk_find_entry(label);
> +		kfree(label);
> +	} else {
> +		remove = 0;
> +		skp = smk_import_entry(data, count);
> +	}
> +	kfree(data);
> +
> +	if (IS_ERR(skp))
> +		return PTR_ERR(skp);
> +
> +	mutex_lock(&smack_relabel_list_lock);
> +	list_for_each_entry(srp, &smack_relabel_list, list)

As above, tsp->smk_relabel instead of the global.

> +		if (srp->smk_label == skp) {
> +			if (remove) {
> +				list_del_rcu(&srp->list);
> +				mutex_unlock(&smack_relabel_list_lock);
> +				kfree_rcu(srp, rcu);
> +				return rc;
> +			} else
> +				goto out;
> +		}
> +
> +	/* Entry not found on smack_relabel_list */
> +	if (remove) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	srp = kzalloc(sizeof(*srp), GFP_KERNEL);
> +	if (srp == NULL) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	srp->smk_label = skp;
> +	list_add_rcu(&srp->list, &smack_relabel_list);
> +
> +out:
> +	mutex_unlock(&smack_relabel_list_lock);
> +	return rc;
> +}
> +
> +static const struct file_operations smk_relabel_list_ops = {
> +	.open           = smk_open_relabel_list,
> +	.read		= seq_read,
> +	.llseek         = seq_lseek,
> +	.write		= smk_write_relabel_list,
> +	.release        = seq_release,
> +};
> +
> +/**
>   * smk_read_ptrace - read() for /smack/ptrace
>   * @filp: file pointer, not actually used
>   * @buf: where to put the result
> @@ -2824,6 +3039,12 @@ 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 */
> +		[SMK_RELABEL_POSSIBLE] = {
> +			"relabel-possible", &smk_relabel_possible_ops,
> +				S_IRUGO|S_IWUGO},
> +		[SMK_RELABEL_LIST] = {
> +			"relabel-list", &smk_relabel_list_ops,
> +				S_IRUGO|S_IWUGO},
>  		/* last one */
>  			{""}
>  	};

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