[2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc

Commit Message

Stephen Smalley Dec. 16, 2016, 5:41 p.m. UTC

Processes can only alter their own security attributes via
/proc/pid/attr nodes.  This is presently enforced by each individual
security module and is also imposed by the Linux credentials
implementation, which only allows a task to alter its own credentials.
Move the check enforcing this restriction from the individual
security modules to proc_pid_attr_write() before calling the security hook,
and drop the unnecessary task argument to the security hook since it can
only ever be the current task.

Signed-off-by: Stephen Smalley <[e-mail]>
---
 fs/proc/base.c             | 13 +++++++++----
 include/linux/lsm_hooks.h  |  3 +--
 include/linux/security.h   |  4 ++--
 security/apparmor/lsm.c    |  7 ++-----
 security/security.c        |  4 ++--
 security/selinux/hooks.c   | 13 +------------
 security/smack/smack_lsm.c | 11 +----------
 7 files changed, 18 insertions(+), 37 deletions(-)

Comments

Casey Schaufler Dec. 16, 2016, 6:38 p.m. UTC | #1

On 12/16/2016 9:41 AM, Stephen Smalley wrote:
> Processes can only alter their own security attributes via
> /proc/pid/attr nodes.  This is presently enforced by each individual
> security module and is also imposed by the Linux credentials
> implementation, which only allows a task to alter its own credentials.
> Move the check enforcing this restriction from the individual
> security modules to proc_pid_attr_write() before calling the security hook,
> and drop the unnecessary task argument to the security hook since it can
> only ever be the current task.
>
> Signed-off-by: Stephen Smalley <[e-mail]>

Acked-by: Casey Schaufler <[e-mail]>


> ---
>  fs/proc/base.c             | 13 +++++++++----
>  include/linux/lsm_hooks.h  |  3 +--
>  include/linux/security.h   |  4 ++--
>  security/apparmor/lsm.c    |  7 ++-----
>  security/security.c        |  4 ++--
>  security/selinux/hooks.c   | 13 +------------
>  security/smack/smack_lsm.c | 11 +----------
>  7 files changed, 18 insertions(+), 37 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6eae4d0..7b228ea 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2485,6 +2485,12 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>  	length = -ESRCH;
>  	if (!task)
>  		goto out_no_task;
> +
> +	/* A task may only write its own attributes. */
> +	length = -EACCES;
> +	if (current != task)
> +		goto out;
> +
>  	if (count > PAGE_SIZE)
>  		count = PAGE_SIZE;
>  
> @@ -2500,14 +2506,13 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>  	}
>  
>  	/* Guard against adverse ptrace interaction */
> -	length = mutex_lock_interruptible(&task->signal->cred_guard_mutex);
> +	length = mutex_lock_interruptible(&current->signal->cred_guard_mutex);
>  	if (length < 0)
>  		goto out_free;
>  
> -	length = security_setprocattr(task,
> -				      (char*)file->f_path.dentry->d_name.name,
> +	length = security_setprocattr(file->f_path.dentry->d_name.name,
>  				      page, count);
> -	mutex_unlock(&task->signal->cred_guard_mutex);
> +	mutex_unlock(&current->signal->cred_guard_mutex);
>  out_free:
>  	kfree(page);
>  out:
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 558adfa..0dde959 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1547,8 +1547,7 @@ union security_list_options {
>  	void (*d_instantiate)(struct dentry *dentry, struct inode *inode);
>  
>  	int (*getprocattr)(struct task_struct *p, char *name, char **value);
> -	int (*setprocattr)(struct task_struct *p, char *name, void *value,
> -				size_t size);
> +	int (*setprocattr)(const char *name, void *value, size_t size);
>  	int (*ismaclabel)(const char *name);
>  	int (*secid_to_secctx)(u32 secid, char **secdata, u32 *seclen);
>  	int (*secctx_to_secid)(const char *secdata, u32 seclen, u32 *secid);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index c2125e9..f4ebac1 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -361,7 +361,7 @@ int security_sem_semop(struct sem_array *sma, struct sembuf *sops,
>  			unsigned nsops, int alter);
>  void security_d_instantiate(struct dentry *dentry, struct inode *inode);
>  int security_getprocattr(struct task_struct *p, char *name, char **value);
> -int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size);
> +int security_setprocattr(const char *name, void *value, size_t size);
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>  int security_ismaclabel(const char *name);
>  int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> @@ -1106,7 +1106,7 @@ static inline int security_getprocattr(struct task_struct *p, char *name, char *
>  	return -EINVAL;
>  }
>  
> -static inline int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size)
> +static inline int security_setprocattr(char *name, void *value, size_t size)
>  {
>  	return -EINVAL;
>  }
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 41b8cb1..8202e55 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -495,8 +495,8 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
>  	return error;
>  }
>  
> -static int apparmor_setprocattr(struct task_struct *task, char *name,
> -				void *value, size_t size)
> +static int apparmor_setprocattr(const char *name, void *value,
> +				size_t size)
>  {
>  	struct common_audit_data sa;
>  	struct apparmor_audit_data aad = {0,};
> @@ -506,9 +506,6 @@ static int apparmor_setprocattr(struct task_struct *task, char *name,
>  
>  	if (size == 0)
>  		return -EINVAL;
> -	/* task can only write its own attributes */
> -	if (current != task)
> -		return -EACCES;
>  
>  	/* AppArmor requires that the buffer must be null terminated atm */
>  	if (args[size - 1] != '\0') {
> diff --git a/security/security.c b/security/security.c
> index f825304..32052f5 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1170,9 +1170,9 @@ int security_getprocattr(struct task_struct *p, char *name, char **value)
>  	return call_int_hook(getprocattr, -EINVAL, p, name, value);
>  }
>  
> -int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size)
> +int security_setprocattr(const char *name, void *value, size_t size)
>  {
> -	return call_int_hook(setprocattr, -EINVAL, p, name, value, size);
> +	return call_int_hook(setprocattr, -EINVAL, name, value, size);
>  }
>  
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9992626..762276b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5859,8 +5859,7 @@ static int selinux_getprocattr(struct task_struct *p,
>  	return error;
>  }
>  
> -static int selinux_setprocattr(struct task_struct *p,
> -			       char *name, void *value, size_t size)
> +static int selinux_setprocattr(const char *name, void *value, size_t size)
>  {
>  	struct task_security_struct *tsec;
>  	struct cred *new;
> @@ -5868,16 +5867,6 @@ static int selinux_setprocattr(struct task_struct *p,
>  	int error;
>  	char *str = value;
>  
> -	if (current != p) {
> -		/*
> -		 * A task may only alter its own credentials.
> -		 * SELinux has always enforced this restriction,
> -		 * and it is now mandated by the Linux credentials
> -		 * infrastructure; see Documentation/security/credentials.txt.
> -		 */
> -		return -EACCES;
> -	}
> -
>  	/*
>  	 * Basic control over ability to set these attributes at all.
>  	 */
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 4d90257..9bde7f8 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3620,7 +3620,6 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>  
>  /**
>   * smack_setprocattr - Smack process attribute setting
> - * @p: the object task
>   * @name: the name of the attribute in /proc/.../attr
>   * @value: the value to set
>   * @size: the size of the value
> @@ -3630,8 +3629,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>   *
>   * Returns the length of the smack label or an error code
>   */
> -static int smack_setprocattr(struct task_struct *p, char *name,
> -			     void *value, size_t size)
> +static int smack_setprocattr(const char *name, void *value, size_t size)
>  {
>  	struct task_smack *tsp = current_security();
>  	struct cred *new;
> @@ -3639,13 +3637,6 @@ static int smack_setprocattr(struct task_struct *p, char *name,
>  	struct smack_known_list_elem *sklep;
>  	int rc;
>  
> -	/*
> -	 * Changing another process' Smack value is too dangerous
> -	 * and supports no sane use case.
> -	 */
> -	if (p != current)
> -		return -EPERM;
> -
>  	if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel))
>  		return -EPERM;
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

John Johansen Dec. 16, 2016, 7:06 p.m. UTC | #2

On 12/16/2016 09:41 AM, Stephen Smalley wrote:
> Processes can only alter their own security attributes via
> /proc/pid/attr nodes.  This is presently enforced by each individual
> security module and is also imposed by the Linux credentials
> implementation, which only allows a task to alter its own credentials.
> Move the check enforcing this restriction from the individual
> security modules to proc_pid_attr_write() before calling the security hook,
> and drop the unnecessary task argument to the security hook since it can
> only ever be the current task.
> 
> Signed-off-by: Stephen Smalley <[e-mail]>

Acked-by: John Johansen <[e-mail]>

> ---
>  fs/proc/base.c             | 13 +++++++++----
>  include/linux/lsm_hooks.h  |  3 +--
>  include/linux/security.h   |  4 ++--
>  security/apparmor/lsm.c    |  7 ++-----
>  security/security.c        |  4 ++--
>  security/selinux/hooks.c   | 13 +------------
>  security/smack/smack_lsm.c | 11 +----------
>  7 files changed, 18 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6eae4d0..7b228ea 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2485,6 +2485,12 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>  	length = -ESRCH;
>  	if (!task)
>  		goto out_no_task;
> +
> +	/* A task may only write its own attributes. */
> +	length = -EACCES;
> +	if (current != task)
> +		goto out;
> +
>  	if (count > PAGE_SIZE)
>  		count = PAGE_SIZE;
>  
> @@ -2500,14 +2506,13 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>  	}
>  
>  	/* Guard against adverse ptrace interaction */
> -	length = mutex_lock_interruptible(&task->signal->cred_guard_mutex);
> +	length = mutex_lock_interruptible(&current->signal->cred_guard_mutex);
>  	if (length < 0)
>  		goto out_free;
>  
> -	length = security_setprocattr(task,
> -				      (char*)file->f_path.dentry->d_name.name,
> +	length = security_setprocattr(file->f_path.dentry->d_name.name,
>  				      page, count);
> -	mutex_unlock(&task->signal->cred_guard_mutex);
> +	mutex_unlock(&current->signal->cred_guard_mutex);
>  out_free:
>  	kfree(page);
>  out:
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 558adfa..0dde959 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1547,8 +1547,7 @@ union security_list_options {
>  	void (*d_instantiate)(struct dentry *dentry, struct inode *inode);
>  
>  	int (*getprocattr)(struct task_struct *p, char *name, char **value);
> -	int (*setprocattr)(struct task_struct *p, char *name, void *value,
> -				size_t size);
> +	int (*setprocattr)(const char *name, void *value, size_t size);
>  	int (*ismaclabel)(const char *name);
>  	int (*secid_to_secctx)(u32 secid, char **secdata, u32 *seclen);
>  	int (*secctx_to_secid)(const char *secdata, u32 seclen, u32 *secid);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index c2125e9..f4ebac1 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -361,7 +361,7 @@ int security_sem_semop(struct sem_array *sma, struct sembuf *sops,
>  			unsigned nsops, int alter);
>  void security_d_instantiate(struct dentry *dentry, struct inode *inode);
>  int security_getprocattr(struct task_struct *p, char *name, char **value);
> -int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size);
> +int security_setprocattr(const char *name, void *value, size_t size);
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>  int security_ismaclabel(const char *name);
>  int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> @@ -1106,7 +1106,7 @@ static inline int security_getprocattr(struct task_struct *p, char *name, char *
>  	return -EINVAL;
>  }
>  
> -static inline int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size)
> +static inline int security_setprocattr(char *name, void *value, size_t size)
>  {
>  	return -EINVAL;
>  }
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 41b8cb1..8202e55 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -495,8 +495,8 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
>  	return error;
>  }
>  
> -static int apparmor_setprocattr(struct task_struct *task, char *name,
> -				void *value, size_t size)
> +static int apparmor_setprocattr(const char *name, void *value,
> +				size_t size)
>  {
>  	struct common_audit_data sa;
>  	struct apparmor_audit_data aad = {0,};
> @@ -506,9 +506,6 @@ static int apparmor_setprocattr(struct task_struct *task, char *name,
>  
>  	if (size == 0)
>  		return -EINVAL;
> -	/* task can only write its own attributes */
> -	if (current != task)
> -		return -EACCES;
>  
>  	/* AppArmor requires that the buffer must be null terminated atm */
>  	if (args[size - 1] != '\0') {
> diff --git a/security/security.c b/security/security.c
> index f825304..32052f5 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1170,9 +1170,9 @@ int security_getprocattr(struct task_struct *p, char *name, char **value)
>  	return call_int_hook(getprocattr, -EINVAL, p, name, value);
>  }
>  
> -int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size)
> +int security_setprocattr(const char *name, void *value, size_t size)
>  {
> -	return call_int_hook(setprocattr, -EINVAL, p, name, value, size);
> +	return call_int_hook(setprocattr, -EINVAL, name, value, size);
>  }
>  
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9992626..762276b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5859,8 +5859,7 @@ static int selinux_getprocattr(struct task_struct *p,
>  	return error;
>  }
>  
> -static int selinux_setprocattr(struct task_struct *p,
> -			       char *name, void *value, size_t size)
> +static int selinux_setprocattr(const char *name, void *value, size_t size)
>  {
>  	struct task_security_struct *tsec;
>  	struct cred *new;
> @@ -5868,16 +5867,6 @@ static int selinux_setprocattr(struct task_struct *p,
>  	int error;
>  	char *str = value;
>  
> -	if (current != p) {
> -		/*
> -		 * A task may only alter its own credentials.
> -		 * SELinux has always enforced this restriction,
> -		 * and it is now mandated by the Linux credentials
> -		 * infrastructure; see Documentation/security/credentials.txt.
> -		 */
> -		return -EACCES;
> -	}
> -
>  	/*
>  	 * Basic control over ability to set these attributes at all.
>  	 */
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 4d90257..9bde7f8 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3620,7 +3620,6 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>  
>  /**
>   * smack_setprocattr - Smack process attribute setting
> - * @p: the object task
>   * @name: the name of the attribute in /proc/.../attr
>   * @value: the value to set
>   * @size: the size of the value
> @@ -3630,8 +3629,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>   *
>   * Returns the length of the smack label or an error code
>   */
> -static int smack_setprocattr(struct task_struct *p, char *name,
> -			     void *value, size_t size)
> +static int smack_setprocattr(const char *name, void *value, size_t size)
>  {
>  	struct task_smack *tsp = current_security();
>  	struct cred *new;
> @@ -3639,13 +3637,6 @@ static int smack_setprocattr(struct task_struct *p, char *name,
>  	struct smack_known_list_elem *sklep;
>  	int rc;
>  
> -	/*
> -	 * Changing another process' Smack value is too dangerous
> -	 * and supports no sane use case.
> -	 */
> -	if (p != current)
> -		return -EPERM;
> -
>  	if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel))
>  		return -EPERM;
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paul Moore Dec. 16, 2016, 10:06 p.m. UTC | #3

On Fri, Dec 16, 2016 at 12:41 PM, Stephen Smalley <[e-mail]> wrote:
> Processes can only alter their own security attributes via
> /proc/pid/attr nodes.  This is presently enforced by each individual
> security module and is also imposed by the Linux credentials
> implementation, which only allows a task to alter its own credentials.
> Move the check enforcing this restriction from the individual
> security modules to proc_pid_attr_write() before calling the security hook,
> and drop the unnecessary task argument to the security hook since it can
> only ever be the current task.
>
> Signed-off-by: Stephen Smalley <[e-mail]>
> ---
>  fs/proc/base.c             | 13 +++++++++----
>  include/linux/lsm_hooks.h  |  3 +--
>  include/linux/security.h   |  4 ++--
>  security/apparmor/lsm.c    |  7 ++-----
>  security/security.c        |  4 ++--
>  security/selinux/hooks.c   | 13 +------------
>  security/smack/smack_lsm.c | 11 +----------
>  7 files changed, 18 insertions(+), 37 deletions(-)

Looks good to me.  I'm happy to pull this in via the SELinux tree
unless anyone else would rather take it?

Acked-by: Paul Moore <[e-mail]>

> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6eae4d0..7b228ea 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2485,6 +2485,12 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>         length = -ESRCH;
>         if (!task)
>                 goto out_no_task;
> +
> +       /* A task may only write its own attributes. */
> +       length = -EACCES;
> +       if (current != task)
> +               goto out;
> +
>         if (count > PAGE_SIZE)
>                 count = PAGE_SIZE;
>
> @@ -2500,14 +2506,13 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>         }
>
>         /* Guard against adverse ptrace interaction */
> -       length = mutex_lock_interruptible(&task->signal->cred_guard_mutex);
> +       length = mutex_lock_interruptible(&current->signal->cred_guard_mutex);
>         if (length < 0)
>                 goto out_free;
>
> -       length = security_setprocattr(task,
> -                                     (char*)file->f_path.dentry->d_name.name,
> +       length = security_setprocattr(file->f_path.dentry->d_name.name,
>                                       page, count);
> -       mutex_unlock(&task->signal->cred_guard_mutex);
> +       mutex_unlock(&current->signal->cred_guard_mutex);
>  out_free:
>         kfree(page);
>  out:
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 558adfa..0dde959 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1547,8 +1547,7 @@ union security_list_options {
>         void (*d_instantiate)(struct dentry *dentry, struct inode *inode);
>
>         int (*getprocattr)(struct task_struct *p, char *name, char **value);
> -       int (*setprocattr)(struct task_struct *p, char *name, void *value,
> -                               size_t size);
> +       int (*setprocattr)(const char *name, void *value, size_t size);
>         int (*ismaclabel)(const char *name);
>         int (*secid_to_secctx)(u32 secid, char **secdata, u32 *seclen);
>         int (*secctx_to_secid)(const char *secdata, u32 seclen, u32 *secid);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index c2125e9..f4ebac1 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -361,7 +361,7 @@ int security_sem_semop(struct sem_array *sma, struct sembuf *sops,
>                         unsigned nsops, int alter);
>  void security_d_instantiate(struct dentry *dentry, struct inode *inode);
>  int security_getprocattr(struct task_struct *p, char *name, char **value);
> -int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size);
> +int security_setprocattr(const char *name, void *value, size_t size);
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>  int security_ismaclabel(const char *name);
>  int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> @@ -1106,7 +1106,7 @@ static inline int security_getprocattr(struct task_struct *p, char *name, char *
>         return -EINVAL;
>  }
>
> -static inline int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size)
> +static inline int security_setprocattr(char *name, void *value, size_t size)
>  {
>         return -EINVAL;
>  }
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 41b8cb1..8202e55 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -495,8 +495,8 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
>         return error;
>  }
>
> -static int apparmor_setprocattr(struct task_struct *task, char *name,
> -                               void *value, size_t size)
> +static int apparmor_setprocattr(const char *name, void *value,
> +                               size_t size)
>  {
>         struct common_audit_data sa;
>         struct apparmor_audit_data aad = {0,};
> @@ -506,9 +506,6 @@ static int apparmor_setprocattr(struct task_struct *task, char *name,
>
>         if (size == 0)
>                 return -EINVAL;
> -       /* task can only write its own attributes */
> -       if (current != task)
> -               return -EACCES;
>
>         /* AppArmor requires that the buffer must be null terminated atm */
>         if (args[size - 1] != '\0') {
> diff --git a/security/security.c b/security/security.c
> index f825304..32052f5 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1170,9 +1170,9 @@ int security_getprocattr(struct task_struct *p, char *name, char **value)
>         return call_int_hook(getprocattr, -EINVAL, p, name, value);
>  }
>
> -int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size)
> +int security_setprocattr(const char *name, void *value, size_t size)
>  {
> -       return call_int_hook(setprocattr, -EINVAL, p, name, value, size);
> +       return call_int_hook(setprocattr, -EINVAL, name, value, size);
>  }
>
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9992626..762276b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5859,8 +5859,7 @@ static int selinux_getprocattr(struct task_struct *p,
>         return error;
>  }
>
> -static int selinux_setprocattr(struct task_struct *p,
> -                              char *name, void *value, size_t size)
> +static int selinux_setprocattr(const char *name, void *value, size_t size)
>  {
>         struct task_security_struct *tsec;
>         struct cred *new;
> @@ -5868,16 +5867,6 @@ static int selinux_setprocattr(struct task_struct *p,
>         int error;
>         char *str = value;
>
> -       if (current != p) {
> -               /*
> -                * A task may only alter its own credentials.
> -                * SELinux has always enforced this restriction,
> -                * and it is now mandated by the Linux credentials
> -                * infrastructure; see Documentation/security/credentials.txt.
> -                */
> -               return -EACCES;
> -       }
> -
>         /*
>          * Basic control over ability to set these attributes at all.
>          */
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 4d90257..9bde7f8 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3620,7 +3620,6 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>
>  /**
>   * smack_setprocattr - Smack process attribute setting
> - * @p: the object task
>   * @name: the name of the attribute in /proc/.../attr
>   * @value: the value to set
>   * @size: the size of the value
> @@ -3630,8 +3629,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>   *
>   * Returns the length of the smack label or an error code
>   */
> -static int smack_setprocattr(struct task_struct *p, char *name,
> -                            void *value, size_t size)
> +static int smack_setprocattr(const char *name, void *value, size_t size)
>  {
>         struct task_smack *tsp = current_security();
>         struct cred *new;
> @@ -3639,13 +3637,6 @@ static int smack_setprocattr(struct task_struct *p, char *name,
>         struct smack_known_list_elem *sklep;
>         int rc;
>
> -       /*
> -        * Changing another process' Smack value is too dangerous
> -        * and supports no sane use case.
> -        */
> -       if (p != current)
> -               return -EPERM;
> -
>         if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel))
>                 return -EPERM;
>
> --
> 2.7.4
>

Casey Schaufler Dec. 16, 2016, 10:13 p.m. UTC | #4

On 12/16/2016 2:06 PM, Paul Moore wrote:
> On Fri, Dec 16, 2016 at 12:41 PM, Stephen Smalley <[e-mail]> wrote:
>> Processes can only alter their own security attributes via
>> /proc/pid/attr nodes.  This is presently enforced by each individual
>> security module and is also imposed by the Linux credentials
>> implementation, which only allows a task to alter its own credentials.
>> Move the check enforcing this restriction from the individual
>> security modules to proc_pid_attr_write() before calling the security hook,
>> and drop the unnecessary task argument to the security hook since it can
>> only ever be the current task.
>>
>> Signed-off-by: Stephen Smalley <[e-mail]>
>> ---
>>  fs/proc/base.c             | 13 +++++++++----
>>  include/linux/lsm_hooks.h  |  3 +--
>>  include/linux/security.h   |  4 ++--
>>  security/apparmor/lsm.c    |  7 ++-----
>>  security/security.c        |  4 ++--
>>  security/selinux/hooks.c   | 13 +------------
>>  security/smack/smack_lsm.c | 11 +----------
>>  7 files changed, 18 insertions(+), 37 deletions(-)
> Looks good to me.  I'm happy to pull this in via the SELinux tree
> unless anyone else would rather take it?

That works for me. It does need to go in atomically.


>
> Acked-by: Paul Moore <[e-mail]>
>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index 6eae4d0..7b228ea 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -2485,6 +2485,12 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>>         length = -ESRCH;
>>         if (!task)
>>                 goto out_no_task;
>> +
>> +       /* A task may only write its own attributes. */
>> +       length = -EACCES;
>> +       if (current != task)
>> +               goto out;
>> +
>>         if (count > PAGE_SIZE)
>>                 count = PAGE_SIZE;
>>
>> @@ -2500,14 +2506,13 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>>         }
>>
>>         /* Guard against adverse ptrace interaction */
>> -       length = mutex_lock_interruptible(&task->signal->cred_guard_mutex);
>> +       length = mutex_lock_interruptible(&current->signal->cred_guard_mutex);
>>         if (length < 0)
>>                 goto out_free;
>>
>> -       length = security_setprocattr(task,
>> -                                     (char*)file->f_path.dentry->d_name.name,
>> +       length = security_setprocattr(file->f_path.dentry->d_name.name,
>>                                       page, count);
>> -       mutex_unlock(&task->signal->cred_guard_mutex);
>> +       mutex_unlock(&current->signal->cred_guard_mutex);
>>  out_free:
>>         kfree(page);
>>  out:
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index 558adfa..0dde959 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -1547,8 +1547,7 @@ union security_list_options {
>>         void (*d_instantiate)(struct dentry *dentry, struct inode *inode);
>>
>>         int (*getprocattr)(struct task_struct *p, char *name, char **value);
>> -       int (*setprocattr)(struct task_struct *p, char *name, void *value,
>> -                               size_t size);
>> +       int (*setprocattr)(const char *name, void *value, size_t size);
>>         int (*ismaclabel)(const char *name);
>>         int (*secid_to_secctx)(u32 secid, char **secdata, u32 *seclen);
>>         int (*secctx_to_secid)(const char *secdata, u32 seclen, u32 *secid);
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index c2125e9..f4ebac1 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -361,7 +361,7 @@ int security_sem_semop(struct sem_array *sma, struct sembuf *sops,
>>                         unsigned nsops, int alter);
>>  void security_d_instantiate(struct dentry *dentry, struct inode *inode);
>>  int security_getprocattr(struct task_struct *p, char *name, char **value);
>> -int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size);
>> +int security_setprocattr(const char *name, void *value, size_t size);
>>  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>>  int security_ismaclabel(const char *name);
>>  int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
>> @@ -1106,7 +1106,7 @@ static inline int security_getprocattr(struct task_struct *p, char *name, char *
>>         return -EINVAL;
>>  }
>>
>> -static inline int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size)
>> +static inline int security_setprocattr(char *name, void *value, size_t size)
>>  {
>>         return -EINVAL;
>>  }
>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>> index 41b8cb1..8202e55 100644
>> --- a/security/apparmor/lsm.c
>> +++ b/security/apparmor/lsm.c
>> @@ -495,8 +495,8 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
>>         return error;
>>  }
>>
>> -static int apparmor_setprocattr(struct task_struct *task, char *name,
>> -                               void *value, size_t size)
>> +static int apparmor_setprocattr(const char *name, void *value,
>> +                               size_t size)
>>  {
>>         struct common_audit_data sa;
>>         struct apparmor_audit_data aad = {0,};
>> @@ -506,9 +506,6 @@ static int apparmor_setprocattr(struct task_struct *task, char *name,
>>
>>         if (size == 0)
>>                 return -EINVAL;
>> -       /* task can only write its own attributes */
>> -       if (current != task)
>> -               return -EACCES;
>>
>>         /* AppArmor requires that the buffer must be null terminated atm */
>>         if (args[size - 1] != '\0') {
>> diff --git a/security/security.c b/security/security.c
>> index f825304..32052f5 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -1170,9 +1170,9 @@ int security_getprocattr(struct task_struct *p, char *name, char **value)
>>         return call_int_hook(getprocattr, -EINVAL, p, name, value);
>>  }
>>
>> -int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size)
>> +int security_setprocattr(const char *name, void *value, size_t size)
>>  {
>> -       return call_int_hook(setprocattr, -EINVAL, p, name, value, size);
>> +       return call_int_hook(setprocattr, -EINVAL, name, value, size);
>>  }
>>
>>  int security_netlink_send(struct sock *sk, struct sk_buff *skb)
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 9992626..762276b 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -5859,8 +5859,7 @@ static int selinux_getprocattr(struct task_struct *p,
>>         return error;
>>  }
>>
>> -static int selinux_setprocattr(struct task_struct *p,
>> -                              char *name, void *value, size_t size)
>> +static int selinux_setprocattr(const char *name, void *value, size_t size)
>>  {
>>         struct task_security_struct *tsec;
>>         struct cred *new;
>> @@ -5868,16 +5867,6 @@ static int selinux_setprocattr(struct task_struct *p,
>>         int error;
>>         char *str = value;
>>
>> -       if (current != p) {
>> -               /*
>> -                * A task may only alter its own credentials.
>> -                * SELinux has always enforced this restriction,
>> -                * and it is now mandated by the Linux credentials
>> -                * infrastructure; see Documentation/security/credentials.txt.
>> -                */
>> -               return -EACCES;
>> -       }
>> -
>>         /*
>>          * Basic control over ability to set these attributes at all.
>>          */
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 4d90257..9bde7f8 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -3620,7 +3620,6 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>>
>>  /**
>>   * smack_setprocattr - Smack process attribute setting
>> - * @p: the object task
>>   * @name: the name of the attribute in /proc/.../attr
>>   * @value: the value to set
>>   * @size: the size of the value
>> @@ -3630,8 +3629,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>>   *
>>   * Returns the length of the smack label or an error code
>>   */
>> -static int smack_setprocattr(struct task_struct *p, char *name,
>> -                            void *value, size_t size)
>> +static int smack_setprocattr(const char *name, void *value, size_t size)
>>  {
>>         struct task_smack *tsp = current_security();
>>         struct cred *new;
>> @@ -3639,13 +3637,6 @@ static int smack_setprocattr(struct task_struct *p, char *name,
>>         struct smack_known_list_elem *sklep;
>>         int rc;
>>
>> -       /*
>> -        * Changing another process' Smack value is too dangerous
>> -        * and supports no sane use case.
>> -        */
>> -       if (p != current)
>> -               return -EPERM;
>> -
>>         if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel))
>>                 return -EPERM;
>>
>> --
>> 2.7.4
>>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

José Bollo Dec. 19, 2016, 9:44 a.m. UTC | #5

Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen Smalley a écrit :
> Processes can only alter their own security attributes via
> /proc/pid/attr nodes.  This is presently enforced by each individual
> security module and is also imposed by the Linux credentials
> implementation, which only allows a task to alter its own
> credentials.
> Move the check enforcing this restriction from the individual
> security modules to proc_pid_attr_write() before calling the security
> hook,
> and drop the unnecessary task argument to the security hook since it
> can
> only ever be the current task.

Hi Stephen,

The module PTAGS that I made relies on the ability to write files in
/proc/pid/attr/...

Why did I used this files? Thaere are serious reasons:
 - follows the life cycle of the process
 - implements pid namespace

There is absolutely no way to get these features easyly outside that
implementation context of /proc/pid/attr/xxx.

For these reasons I vote against that change.

You will probably ask me where is PTAGS used and what is is made for.

So it is not used but the principle of its use is benefical to the
community. It allows a kind of user land implementation of
capabilities.

You probably know that the kernel can not do all things for a good
reason: it is not intended to do all things. So there is a need for
user land features. PTAGS is an intent to fulfill an empty gap.

I made a presentation at FOSDEM on the subject that may allows you to
understand the beginning of the issue that I'm writing about [2]. 

[1] https://lkml.org/lkml/2016/11/23/122
[2] https://archive.fosdem.org/2015/schedule/event/sec_enforcement/

> 
> Signed-off-by: Stephen Smalley <[e-mail]>
> ---
>  fs/proc/base.c             | 13 +++++++++----
>  include/linux/lsm_hooks.h  |  3 +--
>  include/linux/security.h   |  4 ++--
>  security/apparmor/lsm.c    |  7 ++-----
>  security/security.c        |  4 ++--
>  security/selinux/hooks.c   | 13 +------------
>  security/smack/smack_lsm.c | 11 +----------
>  7 files changed, 18 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6eae4d0..7b228ea 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2485,6 +2485,12 @@ static ssize_t proc_pid_attr_write(struct file
> * file, const char __user * buf,
>  	length = -ESRCH;
>  	if (!task)
>  		goto out_no_task;
> +
> +	/* A task may only write its own attributes. */
> +	length = -EACCES;
> +	if (current != task)
> +		goto out;
> +
>  	if (count > PAGE_SIZE)
>  		count = PAGE_SIZE;
>  
> @@ -2500,14 +2506,13 @@ static ssize_t proc_pid_attr_write(struct
> file * file, const char __user * buf,
>  	}
>  
>  	/* Guard against adverse ptrace interaction */
> -	length = mutex_lock_interruptible(&task->signal-
> >cred_guard_mutex);
> +	length = mutex_lock_interruptible(&current->signal-
> >cred_guard_mutex);
>  	if (length < 0)
>  		goto out_free;
>  
> -	length = security_setprocattr(task,
> -				      (char*)file->f_path.dentry-
> >d_name.name,
> +	length = security_setprocattr(file->f_path.dentry-
> >d_name.name,
>  				      page, count);
> -	mutex_unlock(&task->signal->cred_guard_mutex);
> +	mutex_unlock(&current->signal->cred_guard_mutex);
>  out_free:
>  	kfree(page);
>  out:
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 558adfa..0dde959 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1547,8 +1547,7 @@ union security_list_options {
>  	void (*d_instantiate)(struct dentry *dentry, struct inode
> *inode);
>  
>  	int (*getprocattr)(struct task_struct *p, char *name, char
> **value);
> -	int (*setprocattr)(struct task_struct *p, char *name, void
> *value,
> -				size_t size);
> +	int (*setprocattr)(const char *name, void *value, size_t
> size);
>  	int (*ismaclabel)(const char *name);
>  	int (*secid_to_secctx)(u32 secid, char **secdata, u32
> *seclen);
>  	int (*secctx_to_secid)(const char *secdata, u32 seclen, u32
> *secid);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index c2125e9..f4ebac1 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -361,7 +361,7 @@ int security_sem_semop(struct sem_array *sma,
> struct sembuf *sops,
>  			unsigned nsops, int alter);
>  void security_d_instantiate(struct dentry *dentry, struct inode
> *inode);
>  int security_getprocattr(struct task_struct *p, char *name, char
> **value);
> -int security_setprocattr(struct task_struct *p, char *name, void
> *value, size_t size);
> +int security_setprocattr(const char *name, void *value, size_t
> size);
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>  int security_ismaclabel(const char *name);
>  int security_secid_to_secctx(u32 secid, char **secdata, u32
> *seclen);
> @@ -1106,7 +1106,7 @@ static inline int security_getprocattr(struct
> task_struct *p, char *name, char *
>  	return -EINVAL;
>  }
>  
> -static inline int security_setprocattr(struct task_struct *p, char
> *name, void *value, size_t size)
> +static inline int security_setprocattr(char *name, void *value,
> size_t size)
>  {
>  	return -EINVAL;
>  }
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 41b8cb1..8202e55 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -495,8 +495,8 @@ static int apparmor_getprocattr(struct
> task_struct *task, char *name,
>  	return error;
>  }
>  
> -static int apparmor_setprocattr(struct task_struct *task, char
> *name,
> -				void *value, size_t size)
> +static int apparmor_setprocattr(const char *name, void *value,
> +				size_t size)
>  {
>  	struct common_audit_data sa;
>  	struct apparmor_audit_data aad = {0,};
> @@ -506,9 +506,6 @@ static int apparmor_setprocattr(struct
> task_struct *task, char *name,
>  
>  	if (size == 0)
>  		return -EINVAL;
> -	/* task can only write its own attributes */
> -	if (current != task)
> -		return -EACCES;
>  
>  	/* AppArmor requires that the buffer must be null terminated
> atm */
>  	if (args[size - 1] != '\0') {
> diff --git a/security/security.c b/security/security.c
> index f825304..32052f5 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1170,9 +1170,9 @@ int security_getprocattr(struct task_struct *p,
> char *name, char **value)
>  	return call_int_hook(getprocattr, -EINVAL, p, name, value);
>  }
>  
> -int security_setprocattr(struct task_struct *p, char *name, void
> *value, size_t size)
> +int security_setprocattr(const char *name, void *value, size_t size)
>  {
> -	return call_int_hook(setprocattr, -EINVAL, p, name, value,
> size);
> +	return call_int_hook(setprocattr, -EINVAL, name, value,
> size);
>  }
>  
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9992626..762276b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5859,8 +5859,7 @@ static int selinux_getprocattr(struct
> task_struct *p,
>  	return error;
>  }
>  
> -static int selinux_setprocattr(struct task_struct *p,
> -			       char *name, void *value, size_t size)
> +static int selinux_setprocattr(const char *name, void *value, size_t
> size)
>  {
>  	struct task_security_struct *tsec;
>  	struct cred *new;
> @@ -5868,16 +5867,6 @@ static int selinux_setprocattr(struct
> task_struct *p,
>  	int error;
>  	char *str = value;
>  
> -	if (current != p) {
> -		/*
> -		 * A task may only alter its own credentials.
> -		 * SELinux has always enforced this restriction,
> -		 * and it is now mandated by the Linux credentials
> -		 * infrastructure; see
> Documentation/security/credentials.txt.
> -		 */
> -		return -EACCES;
> -	}
> -
>  	/*
>  	 * Basic control over ability to set these attributes at
> all.
>  	 */
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 4d90257..9bde7f8 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3620,7 +3620,6 @@ static int smack_getprocattr(struct task_struct
> *p, char *name, char **value)
>  
>  /**
>   * smack_setprocattr - Smack process attribute setting
> - * @p: the object task
>   * @name: the name of the attribute in /proc/.../attr
>   * @value: the value to set
>   * @size: the size of the value
> @@ -3630,8 +3629,7 @@ static int smack_getprocattr(struct task_struct
> *p, char *name, char **value)
>   *
>   * Returns the length of the smack label or an error code
>   */
> -static int smack_setprocattr(struct task_struct *p, char *name,
> -			     void *value, size_t size)
> +static int smack_setprocattr(const char *name, void *value, size_t
> size)
>  {
>  	struct task_smack *tsp = current_security();
>  	struct cred *new;
> @@ -3639,13 +3637,6 @@ static int smack_setprocattr(struct
> task_struct *p, char *name,
>  	struct smack_known_list_elem *sklep;
>  	int rc;
>  
> -	/*
> -	 * Changing another process' Smack value is too dangerous
> -	 * and supports no sane use case.
> -	 */
> -	if (p != current)
> -		return -EPERM;
> -
>  	if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp-
> >smk_relabel))
>  		return -EPERM;
>  
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Smalley Dec. 19, 2016, 2:33 p.m. UTC | #6

On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
> Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen Smalley a écrit :
> > 
> > Processes can only alter their own security attributes via
> > /proc/pid/attr nodes.  This is presently enforced by each
> > individual
> > security module and is also imposed by the Linux credentials
> > implementation, which only allows a task to alter its own
> > credentials.
> > Move the check enforcing this restriction from the individual
> > security modules to proc_pid_attr_write() before calling the
> > security
> > hook,
> > and drop the unnecessary task argument to the security hook since
> > it
> > can
> > only ever be the current task.
> 
> Hi Stephen,
> 
> The module PTAGS that I made relies on the ability to write files in
> /proc/pid/attr/...
> 
> Why did I used this files? Thaere are serious reasons:
>  - follows the life cycle of the process
>  - implements pid namespace
> 
> There is absolutely no way to get these features easyly outside that
> implementation context of /proc/pid/attr/xxx.
> 
> For these reasons I vote against that change.

It is already prohibited by the credentials framework; see
Documentation/security/credentials.txt, under ALTERING CREDENTIALS.
We are not imposing a new restriction here, just recognizing that it
already exists (ever since Linux moved to using cred structures).  You
might not agree with that restriction, but we didn't invent it.

> You will probably ask me where is PTAGS used and what is is made for.
> 
> So it is not used but the principle of its use is benefical to the
> community. It allows a kind of user land implementation of
> capabilities.
> 
> You probably know that the kernel can not do all things for a good
> reason: it is not intended to do all things. So there is a need for
> user land features. PTAGS is an intent to fulfill an empty gap.
> 
> I made a presentation at FOSDEM on the subject that may allows you to
> understand the beginning of the issue that I'm writing about [2]. 
> 
> [1] https://lkml.org/lkml/2016/11/23/122
> [2] https://archive.fosdem.org/2015/schedule/event/sec_enforcement/
> 
> > 
> > 
> > Signed-off-by: Stephen Smalley <[e-mail]>
> > ---
> >  fs/proc/base.c             | 13 +++++++++----
> >  include/linux/lsm_hooks.h  |  3 +--
> >  include/linux/security.h   |  4 ++--
> >  security/apparmor/lsm.c    |  7 ++-----
> >  security/security.c        |  4 ++--
> >  security/selinux/hooks.c   | 13 +------------
> >  security/smack/smack_lsm.c | 11 +----------
> >  7 files changed, 18 insertions(+), 37 deletions(-)
> > 
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 6eae4d0..7b228ea 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -2485,6 +2485,12 @@ static ssize_t proc_pid_attr_write(struct
> > file
> > * file, const char __user * buf,
> >  	length = -ESRCH;
> >  	if (!task)
> >  		goto out_no_task;
> > +
> > +	/* A task may only write its own attributes. */
> > +	length = -EACCES;
> > +	if (current != task)
> > +		goto out;
> > +
> >  	if (count > PAGE_SIZE)
> >  		count = PAGE_SIZE;
> >  
> > @@ -2500,14 +2506,13 @@ static ssize_t proc_pid_attr_write(struct
> > file * file, const char __user * buf,
> >  	}
> >  
> >  	/* Guard against adverse ptrace interaction */
> > -	length = mutex_lock_interruptible(&task->signal-
> > > 
> > > cred_guard_mutex);
> > +	length = mutex_lock_interruptible(&current->signal-
> > > 
> > > cred_guard_mutex);
> >  	if (length < 0)
> >  		goto out_free;
> >  
> > -	length = security_setprocattr(task,
> > -				      (char*)file->f_path.dentry-
> > > 
> > > d_name.name,
> > +	length = security_setprocattr(file->f_path.dentry-
> > > 
> > > d_name.name,
> >  				      page, count);
> > -	mutex_unlock(&task->signal->cred_guard_mutex);
> > +	mutex_unlock(&current->signal->cred_guard_mutex);
> >  out_free:
> >  	kfree(page);
> >  out:
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 558adfa..0dde959 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -1547,8 +1547,7 @@ union security_list_options {
> >  	void (*d_instantiate)(struct dentry *dentry, struct inode
> > *inode);
> >  
> >  	int (*getprocattr)(struct task_struct *p, char *name, char
> > **value);
> > -	int (*setprocattr)(struct task_struct *p, char *name, void
> > *value,
> > -				size_t size);
> > +	int (*setprocattr)(const char *name, void *value, size_t
> > size);
> >  	int (*ismaclabel)(const char *name);
> >  	int (*secid_to_secctx)(u32 secid, char **secdata, u32
> > *seclen);
> >  	int (*secctx_to_secid)(const char *secdata, u32 seclen,
> > u32
> > *secid);
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index c2125e9..f4ebac1 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -361,7 +361,7 @@ int security_sem_semop(struct sem_array *sma,
> > struct sembuf *sops,
> >  			unsigned nsops, int alter);
> >  void security_d_instantiate(struct dentry *dentry, struct inode
> > *inode);
> >  int security_getprocattr(struct task_struct *p, char *name, char
> > **value);
> > -int security_setprocattr(struct task_struct *p, char *name, void
> > *value, size_t size);
> > +int security_setprocattr(const char *name, void *value, size_t
> > size);
> >  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> >  int security_ismaclabel(const char *name);
> >  int security_secid_to_secctx(u32 secid, char **secdata, u32
> > *seclen);
> > @@ -1106,7 +1106,7 @@ static inline int security_getprocattr(struct
> > task_struct *p, char *name, char *
> >  	return -EINVAL;
> >  }
> >  
> > -static inline int security_setprocattr(struct task_struct *p, char
> > *name, void *value, size_t size)
> > +static inline int security_setprocattr(char *name, void *value,
> > size_t size)
> >  {
> >  	return -EINVAL;
> >  }
> > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > index 41b8cb1..8202e55 100644
> > --- a/security/apparmor/lsm.c
> > +++ b/security/apparmor/lsm.c
> > @@ -495,8 +495,8 @@ static int apparmor_getprocattr(struct
> > task_struct *task, char *name,
> >  	return error;
> >  }
> >  
> > -static int apparmor_setprocattr(struct task_struct *task, char
> > *name,
> > -				void *value, size_t size)
> > +static int apparmor_setprocattr(const char *name, void *value,
> > +				size_t size)
> >  {
> >  	struct common_audit_data sa;
> >  	struct apparmor_audit_data aad = {0,};
> > @@ -506,9 +506,6 @@ static int apparmor_setprocattr(struct
> > task_struct *task, char *name,
> >  
> >  	if (size == 0)
> >  		return -EINVAL;
> > -	/* task can only write its own attributes */
> > -	if (current != task)
> > -		return -EACCES;
> >  
> >  	/* AppArmor requires that the buffer must be null
> > terminated
> > atm */
> >  	if (args[size - 1] != '\0') {
> > diff --git a/security/security.c b/security/security.c
> > index f825304..32052f5 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1170,9 +1170,9 @@ int security_getprocattr(struct task_struct
> > *p,
> > char *name, char **value)
> >  	return call_int_hook(getprocattr, -EINVAL, p, name,
> > value);
> >  }
> >  
> > -int security_setprocattr(struct task_struct *p, char *name, void
> > *value, size_t size)
> > +int security_setprocattr(const char *name, void *value, size_t
> > size)
> >  {
> > -	return call_int_hook(setprocattr, -EINVAL, p, name, value,
> > size);
> > +	return call_int_hook(setprocattr, -EINVAL, name, value,
> > size);
> >  }
> >  
> >  int security_netlink_send(struct sock *sk, struct sk_buff *skb)
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 9992626..762276b 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -5859,8 +5859,7 @@ static int selinux_getprocattr(struct
> > task_struct *p,
> >  	return error;
> >  }
> >  
> > -static int selinux_setprocattr(struct task_struct *p,
> > -			       char *name, void *value, size_t
> > size)
> > +static int selinux_setprocattr(const char *name, void *value,
> > size_t
> > size)
> >  {
> >  	struct task_security_struct *tsec;
> >  	struct cred *new;
> > @@ -5868,16 +5867,6 @@ static int selinux_setprocattr(struct
> > task_struct *p,
> >  	int error;
> >  	char *str = value;
> >  
> > -	if (current != p) {
> > -		/*
> > -		 * A task may only alter its own credentials.
> > -		 * SELinux has always enforced this restriction,
> > -		 * and it is now mandated by the Linux credentials
> > -		 * infrastructure; see
> > Documentation/security/credentials.txt.
> > -		 */
> > -		return -EACCES;
> > -	}
> > -
> >  	/*
> >  	 * Basic control over ability to set these attributes at
> > all.
> >  	 */
> > diff --git a/security/smack/smack_lsm.c
> > b/security/smack/smack_lsm.c
> > index 4d90257..9bde7f8 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -3620,7 +3620,6 @@ static int smack_getprocattr(struct
> > task_struct
> > *p, char *name, char **value)
> >  
> >  /**
> >   * smack_setprocattr - Smack process attribute setting
> > - * @p: the object task
> >   * @name: the name of the attribute in /proc/.../attr
> >   * @value: the value to set
> >   * @size: the size of the value
> > @@ -3630,8 +3629,7 @@ static int smack_getprocattr(struct
> > task_struct
> > *p, char *name, char **value)
> >   *
> >   * Returns the length of the smack label or an error code
> >   */
> > -static int smack_setprocattr(struct task_struct *p, char *name,
> > -			     void *value, size_t size)
> > +static int smack_setprocattr(const char *name, void *value, size_t
> > size)
> >  {
> >  	struct task_smack *tsp = current_security();
> >  	struct cred *new;
> > @@ -3639,13 +3637,6 @@ static int smack_setprocattr(struct
> > task_struct *p, char *name,
> >  	struct smack_known_list_elem *sklep;
> >  	int rc;
> >  
> > -	/*
> > -	 * Changing another process' Smack value is too dangerous
> > -	 * and supports no sane use case.
> > -	 */
> > -	if (p != current)
> > -		return -EPERM;
> > -
> >  	if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp-
> > > 
> > > smk_relabel))
> >  		return -EPERM;
> >  
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

José Bollo Dec. 19, 2016, 3 p.m. UTC | #7

Le lundi 19 décembre 2016 à 09:33 -0500, Stephen Smalley a écrit :
> On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
> > Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen Smalley a
> > écrit :
> > > 
> > > Processes can only alter their own security attributes via
> > > /proc/pid/attr nodes.  This is presently enforced by each
> > > individual
> > > security module and is also imposed by the Linux credentials
> > > implementation, which only allows a task to alter its own
> > > credentials.
> > > Move the check enforcing this restriction from the individual
> > > security modules to proc_pid_attr_write() before calling the
> > > security
> > > hook,
> > > and drop the unnecessary task argument to the security hook since
> > > it
> > > can
> > > only ever be the current task.
> > 
> > Hi Stephen,
> > 
> > The module PTAGS that I made relies on the ability to write files
> > in
> > /proc/pid/attr/...
> > 
> > Why did I used this files? Thaere are serious reasons:
> >  - follows the life cycle of the process
> >  - implements pid namespace
> > 
> > There is absolutely no way to get these features easyly outside
> > that
> > implementation context of /proc/pid/attr/xxx.
> > 
> > For these reasons I vote against that change.

Hi,

> It is already prohibited by the credentials framework; see
> Documentation/security/credentials.txt, under ALTERING CREDENTIALS.
> We are not imposing a new restriction here, just recognizing that it
> already exists (ever since Linux moved to using cred structures). 

Perhaps that the men who wrote that documentation had not think about
other uses. It is not having eyecup or being ignorant. Who can really
imagine the future uses?

>  You might not agree with that restriction, but we didn't invent it.

In fact, it is not really a disagreement. I can understand the why and
I can agree or disagree if I had time to study the issues in deep. But,
as I wrote, I found a very convenient use of writing this files and I
explained it and why in presentations. Your patch breaks all and I
repeat, implementing something like PTAGS would be very complicated
outside of LSM.

Maybe it is not you that wrote the spec but someone invented it.

I propose to change the documentation.


> > You will probably ask me where is PTAGS used and what is is made
> > for.
> > 
> > So it is not used but the principle of its use is benefical to the
> > community. It allows a kind of user land implementation of
> > capabilities.
> > 
> > You probably know that the kernel can not do all things for a good
> > reason: it is not intended to do all things. So there is a need for
> > user land features. PTAGS is an intent to fulfill an empty gap.
> > 
> > I made a presentation at FOSDEM on the subject that may allows you
> > to
> > understand the beginning of the issue that I'm writing about [2]. 
> > 
> > [1] https://lkml.org/lkml/2016/11/23/122
> > [2] https://archive.fosdem.org/2015/schedule/event/sec_enforcement/
> > 
> > > 
> > > 
> > > Signed-off-by: Stephen Smalley <[e-mail]>
> > > ---
> > >  fs/proc/base.c             | 13 +++++++++----
> > >  include/linux/lsm_hooks.h  |  3 +--
> > >  include/linux/security.h   |  4 ++--
> > >  security/apparmor/lsm.c    |  7 ++-----
> > >  security/security.c        |  4 ++--
> > >  security/selinux/hooks.c   | 13 +------------
> > >  security/smack/smack_lsm.c | 11 +----------
> > >  7 files changed, 18 insertions(+), 37 deletions(-)
> > > 
> > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > index 6eae4d0..7b228ea 100644
> > > --- a/fs/proc/base.c
> > > +++ b/fs/proc/base.c
> > > @@ -2485,6 +2485,12 @@ static ssize_t proc_pid_attr_write(struct
> > > file
> > > * file, const char __user * buf,
> > >  	length = -ESRCH;
> > >  	if (!task)
> > >  		goto out_no_task;
> > > +
> > > +	/* A task may only write its own attributes. */
> > > +	length = -EACCES;
> > > +	if (current != task)
> > > +		goto out;
> > > +
> > >  	if (count > PAGE_SIZE)
> > >  		count = PAGE_SIZE;
> > >  
> > > @@ -2500,14 +2506,13 @@ static ssize_t proc_pid_attr_write(struct
> > > file * file, const char __user * buf,
> > >  	}
> > >  
> > >  	/* Guard against adverse ptrace interaction */
> > > -	length = mutex_lock_interruptible(&task->signal-
> > > > 
> > > > cred_guard_mutex);
> > > 
> > > +	length = mutex_lock_interruptible(&current->signal-
> > > > 
> > > > cred_guard_mutex);
> > > 
> > >  	if (length < 0)
> > >  		goto out_free;
> > >  
> > > -	length = security_setprocattr(task,
> > > -				      (char*)file-
> > > >f_path.dentry-
> > > > 
> > > > d_name.name,
> > > 
> > > +	length = security_setprocattr(file->f_path.dentry-
> > > > 
> > > > d_name.name,
> > > 
> > >  				      page, count);
> > > -	mutex_unlock(&task->signal->cred_guard_mutex);
> > > +	mutex_unlock(&current->signal->cred_guard_mutex);
> > >  out_free:
> > >  	kfree(page);
> > >  out:
> > > diff --git a/include/linux/lsm_hooks.h
> > > b/include/linux/lsm_hooks.h
> > > index 558adfa..0dde959 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -1547,8 +1547,7 @@ union security_list_options {
> > >  	void (*d_instantiate)(struct dentry *dentry, struct
> > > inode
> > > *inode);
> > >  
> > >  	int (*getprocattr)(struct task_struct *p, char *name,
> > > char
> > > **value);
> > > -	int (*setprocattr)(struct task_struct *p, char *name,
> > > void
> > > *value,
> > > -				size_t size);
> > > +	int (*setprocattr)(const char *name, void *value, size_t
> > > size);
> > >  	int (*ismaclabel)(const char *name);
> > >  	int (*secid_to_secctx)(u32 secid, char **secdata, u32
> > > *seclen);
> > >  	int (*secctx_to_secid)(const char *secdata, u32 seclen,
> > > u32
> > > *secid);
> > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > index c2125e9..f4ebac1 100644
> > > --- a/include/linux/security.h
> > > +++ b/include/linux/security.h
> > > @@ -361,7 +361,7 @@ int security_sem_semop(struct sem_array *sma,
> > > struct sembuf *sops,
> > >  			unsigned nsops, int alter);
> > >  void security_d_instantiate(struct dentry *dentry, struct inode
> > > *inode);
> > >  int security_getprocattr(struct task_struct *p, char *name, char
> > > **value);
> > > -int security_setprocattr(struct task_struct *p, char *name, void
> > > *value, size_t size);
> > > +int security_setprocattr(const char *name, void *value, size_t
> > > size);
> > >  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> > >  int security_ismaclabel(const char *name);
> > >  int security_secid_to_secctx(u32 secid, char **secdata, u32
> > > *seclen);
> > > @@ -1106,7 +1106,7 @@ static inline int
> > > security_getprocattr(struct
> > > task_struct *p, char *name, char *
> > >  	return -EINVAL;
> > >  }
> > >  
> > > -static inline int security_setprocattr(struct task_struct *p,
> > > char
> > > *name, void *value, size_t size)
> > > +static inline int security_setprocattr(char *name, void *value,
> > > size_t size)
> > >  {
> > >  	return -EINVAL;
> > >  }
> > > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > > index 41b8cb1..8202e55 100644
> > > --- a/security/apparmor/lsm.c
> > > +++ b/security/apparmor/lsm.c
> > > @@ -495,8 +495,8 @@ static int apparmor_getprocattr(struct
> > > task_struct *task, char *name,
> > >  	return error;
> > >  }
> > >  
> > > -static int apparmor_setprocattr(struct task_struct *task, char
> > > *name,
> > > -				void *value, size_t size)
> > > +static int apparmor_setprocattr(const char *name, void *value,
> > > +				size_t size)
> > >  {
> > >  	struct common_audit_data sa;
> > >  	struct apparmor_audit_data aad = {0,};
> > > @@ -506,9 +506,6 @@ static int apparmor_setprocattr(struct
> > > task_struct *task, char *name,
> > >  
> > >  	if (size == 0)
> > >  		return -EINVAL;
> > > -	/* task can only write its own attributes */
> > > -	if (current != task)
> > > -		return -EACCES;
> > >  
> > >  	/* AppArmor requires that the buffer must be null
> > > terminated
> > > atm */
> > >  	if (args[size - 1] != '\0') {
> > > diff --git a/security/security.c b/security/security.c
> > > index f825304..32052f5 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -1170,9 +1170,9 @@ int security_getprocattr(struct task_struct
> > > *p,
> > > char *name, char **value)
> > >  	return call_int_hook(getprocattr, -EINVAL, p, name,
> > > value);
> > >  }
> > >  
> > > -int security_setprocattr(struct task_struct *p, char *name, void
> > > *value, size_t size)
> > > +int security_setprocattr(const char *name, void *value, size_t
> > > size)
> > >  {
> > > -	return call_int_hook(setprocattr, -EINVAL, p, name,
> > > value,
> > > size);
> > > +	return call_int_hook(setprocattr, -EINVAL, name, value,
> > > size);
> > >  }
> > >  
> > >  int security_netlink_send(struct sock *sk, struct sk_buff *skb)
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 9992626..762276b 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -5859,8 +5859,7 @@ static int selinux_getprocattr(struct
> > > task_struct *p,
> > >  	return error;
> > >  }
> > >  
> > > -static int selinux_setprocattr(struct task_struct *p,
> > > -			       char *name, void *value, size_t
> > > size)
> > > +static int selinux_setprocattr(const char *name, void *value,
> > > size_t
> > > size)
> > >  {
> > >  	struct task_security_struct *tsec;
> > >  	struct cred *new;
> > > @@ -5868,16 +5867,6 @@ static int selinux_setprocattr(struct
> > > task_struct *p,
> > >  	int error;
> > >  	char *str = value;
> > >  
> > > -	if (current != p) {
> > > -		/*
> > > -		 * A task may only alter its own credentials.
> > > -		 * SELinux has always enforced this restriction,
> > > -		 * and it is now mandated by the Linux
> > > credentials
> > > -		 * infrastructure; see
> > > Documentation/security/credentials.txt.
> > > -		 */
> > > -		return -EACCES;
> > > -	}
> > > -
> > >  	/*
> > >  	 * Basic control over ability to set these attributes at
> > > all.
> > >  	 */
> > > diff --git a/security/smack/smack_lsm.c
> > > b/security/smack/smack_lsm.c
> > > index 4d90257..9bde7f8 100644
> > > --- a/security/smack/smack_lsm.c
> > > +++ b/security/smack/smack_lsm.c
> > > @@ -3620,7 +3620,6 @@ static int smack_getprocattr(struct
> > > task_struct
> > > *p, char *name, char **value)
> > >  
> > >  /**
> > >   * smack_setprocattr - Smack process attribute setting
> > > - * @p: the object task
> > >   * @name: the name of the attribute in /proc/.../attr
> > >   * @value: the value to set
> > >   * @size: the size of the value
> > > @@ -3630,8 +3629,7 @@ static int smack_getprocattr(struct
> > > task_struct
> > > *p, char *name, char **value)
> > >   *
> > >   * Returns the length of the smack label or an error code
> > >   */
> > > -static int smack_setprocattr(struct task_struct *p, char *name,
> > > -			     void *value, size_t size)
> > > +static int smack_setprocattr(const char *name, void *value,
> > > size_t
> > > size)
> > >  {
> > >  	struct task_smack *tsp = current_security();
> > >  	struct cred *new;
> > > @@ -3639,13 +3637,6 @@ static int smack_setprocattr(struct
> > > task_struct *p, char *name,
> > >  	struct smack_known_list_elem *sklep;
> > >  	int rc;
> > >  
> > > -	/*
> > > -	 * Changing another process' Smack value is too
> > > dangerous
> > > -	 * and supports no sane use case.
> > > -	 */
> > > -	if (p != current)
> > > -		return -EPERM;
> > > -
> > >  	if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp-
> > > > 
> > > > smk_relabel))
> > > 
> > >  		return -EPERM;
> > >  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> security-module" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

José Bollo Dec. 19, 2016, 3:41 p.m. UTC | #8

Le lundi 19 décembre 2016 à 09:33 -0500, Stephen Smalley a écrit :
> On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
> > Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen Smalley a
> > écrit :
> > > 
> > > Processes can only alter their own security attributes via
> > > /proc/pid/attr nodes.  This is presently enforced by each
> > > individual
> > > security module and is also imposed by the Linux credentials
> > > implementation, which only allows a task to alter its own
> > > credentials.
> > > Move the check enforcing this restriction from the individual
> > > security modules to proc_pid_attr_write() before calling the
> > > security
> > > hook,
> > > and drop the unnecessary task argument to the security hook since
> > > it
> > > can
> > > only ever be the current task.
> > 
> > Hi Stephen,
> > 
> > The module PTAGS that I made relies on the ability to write files
> > in
> > /proc/pid/attr/...
> > 
> > Why did I used this files? Thaere are serious reasons:
> >  - follows the life cycle of the process
> >  - implements pid namespace
> > 
> > There is absolutely no way to get these features easyly outside
> > that
> > implementation context of /proc/pid/attr/xxx.
> > 
> > For these reasons I vote against that change.
> 
> It is already prohibited by the credentials framework;

If it were REALLY prohibited, your patch shouldn't exist ;)

Best regards
José Bollo

>  see
> Documentation/security/credentials.txt, under ALTERING CREDENTIALS.
> We are not imposing a new restriction here, just recognizing that it
> already exists (ever since Linux moved to using cred structures).
>  You
> might not agree with that restriction, but we didn't invent it.
> 
> > You will probably ask me where is PTAGS used and what is is made
> > for.
> > 
> > So it is not used but the principle of its use is benefical to the
> > community. It allows a kind of user land implementation of
> > capabilities.
> > 
> > You probably know that the kernel can not do all things for a good
> > reason: it is not intended to do all things. So there is a need for
> > user land features. PTAGS is an intent to fulfill an empty gap.
> > 
> > I made a presentation at FOSDEM on the subject that may allows you
> > to
> > understand the beginning of the issue that I'm writing about [2]. 
> > 
> > [1] https://lkml.org/lkml/2016/11/23/122
> > [2] https://archive.fosdem.org/2015/schedule/event/sec_enforcement/
> > 
> > > 
> > > 
> > > Signed-off-by: Stephen Smalley <[e-mail]>
> > > ---
> > >  fs/proc/base.c             | 13 +++++++++----
> > >  include/linux/lsm_hooks.h  |  3 +--
> > >  include/linux/security.h   |  4 ++--
> > >  security/apparmor/lsm.c    |  7 ++-----
> > >  security/security.c        |  4 ++--
> > >  security/selinux/hooks.c   | 13 +------------
> > >  security/smack/smack_lsm.c | 11 +----------
> > >  7 files changed, 18 insertions(+), 37 deletions(-)
> > > 
> > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > index 6eae4d0..7b228ea 100644
> > > --- a/fs/proc/base.c
> > > +++ b/fs/proc/base.c
> > > @@ -2485,6 +2485,12 @@ static ssize_t proc_pid_attr_write(struct
> > > file
> > > * file, const char __user * buf,
> > >  	length = -ESRCH;
> > >  	if (!task)
> > >  		goto out_no_task;
> > > +
> > > +	/* A task may only write its own attributes. */
> > > +	length = -EACCES;
> > > +	if (current != task)
> > > +		goto out;
> > > +
> > >  	if (count > PAGE_SIZE)
> > >  		count = PAGE_SIZE;
> > >  
> > > @@ -2500,14 +2506,13 @@ static ssize_t proc_pid_attr_write(struct
> > > file * file, const char __user * buf,
> > >  	}
> > >  
> > >  	/* Guard against adverse ptrace interaction */
> > > -	length = mutex_lock_interruptible(&task->signal-
> > > > 
> > > > cred_guard_mutex);
> > > 
> > > +	length = mutex_lock_interruptible(&current->signal-
> > > > 
> > > > cred_guard_mutex);
> > > 
> > >  	if (length < 0)
> > >  		goto out_free;
> > >  
> > > -	length = security_setprocattr(task,
> > > -				      (char*)file-
> > > >f_path.dentry-
> > > > 
> > > > d_name.name,
> > > 
> > > +	length = security_setprocattr(file->f_path.dentry-
> > > > 
> > > > d_name.name,
> > > 
> > >  				      page, count);
> > > -	mutex_unlock(&task->signal->cred_guard_mutex);
> > > +	mutex_unlock(&current->signal->cred_guard_mutex);
> > >  out_free:
> > >  	kfree(page);
> > >  out:
> > > diff --git a/include/linux/lsm_hooks.h
> > > b/include/linux/lsm_hooks.h
> > > index 558adfa..0dde959 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -1547,8 +1547,7 @@ union security_list_options {
> > >  	void (*d_instantiate)(struct dentry *dentry, struct
> > > inode
> > > *inode);
> > >  
> > >  	int (*getprocattr)(struct task_struct *p, char *name,
> > > char
> > > **value);
> > > -	int (*setprocattr)(struct task_struct *p, char *name,
> > > void
> > > *value,
> > > -				size_t size);
> > > +	int (*setprocattr)(const char *name, void *value, size_t
> > > size);
> > >  	int (*ismaclabel)(const char *name);
> > >  	int (*secid_to_secctx)(u32 secid, char **secdata, u32
> > > *seclen);
> > >  	int (*secctx_to_secid)(const char *secdata, u32 seclen,
> > > u32
> > > *secid);
> > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > index c2125e9..f4ebac1 100644
> > > --- a/include/linux/security.h
> > > +++ b/include/linux/security.h
> > > @@ -361,7 +361,7 @@ int security_sem_semop(struct sem_array *sma,
> > > struct sembuf *sops,
> > >  			unsigned nsops, int alter);
> > >  void security_d_instantiate(struct dentry *dentry, struct inode
> > > *inode);
> > >  int security_getprocattr(struct task_struct *p, char *name, char
> > > **value);
> > > -int security_setprocattr(struct task_struct *p, char *name, void
> > > *value, size_t size);
> > > +int security_setprocattr(const char *name, void *value, size_t
> > > size);
> > >  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> > >  int security_ismaclabel(const char *name);
> > >  int security_secid_to_secctx(u32 secid, char **secdata, u32
> > > *seclen);
> > > @@ -1106,7 +1106,7 @@ static inline int
> > > security_getprocattr(struct
> > > task_struct *p, char *name, char *
> > >  	return -EINVAL;
> > >  }
> > >  
> > > -static inline int security_setprocattr(struct task_struct *p,
> > > char
> > > *name, void *value, size_t size)
> > > +static inline int security_setprocattr(char *name, void *value,
> > > size_t size)
> > >  {
> > >  	return -EINVAL;
> > >  }
> > > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > > index 41b8cb1..8202e55 100644
> > > --- a/security/apparmor/lsm.c
> > > +++ b/security/apparmor/lsm.c
> > > @@ -495,8 +495,8 @@ static int apparmor_getprocattr(struct
> > > task_struct *task, char *name,
> > >  	return error;
> > >  }
> > >  
> > > -static int apparmor_setprocattr(struct task_struct *task, char
> > > *name,
> > > -				void *value, size_t size)
> > > +static int apparmor_setprocattr(const char *name, void *value,
> > > +				size_t size)
> > >  {
> > >  	struct common_audit_data sa;
> > >  	struct apparmor_audit_data aad = {0,};
> > > @@ -506,9 +506,6 @@ static int apparmor_setprocattr(struct
> > > task_struct *task, char *name,
> > >  
> > >  	if (size == 0)
> > >  		return -EINVAL;
> > > -	/* task can only write its own attributes */
> > > -	if (current != task)
> > > -		return -EACCES;
> > >  
> > >  	/* AppArmor requires that the buffer must be null
> > > terminated
> > > atm */
> > >  	if (args[size - 1] != '\0') {
> > > diff --git a/security/security.c b/security/security.c
> > > index f825304..32052f5 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -1170,9 +1170,9 @@ int security_getprocattr(struct task_struct
> > > *p,
> > > char *name, char **value)
> > >  	return call_int_hook(getprocattr, -EINVAL, p, name,
> > > value);
> > >  }
> > >  
> > > -int security_setprocattr(struct task_struct *p, char *name, void
> > > *value, size_t size)
> > > +int security_setprocattr(const char *name, void *value, size_t
> > > size)
> > >  {
> > > -	return call_int_hook(setprocattr, -EINVAL, p, name,
> > > value,
> > > size);
> > > +	return call_int_hook(setprocattr, -EINVAL, name, value,
> > > size);
> > >  }
> > >  
> > >  int security_netlink_send(struct sock *sk, struct sk_buff *skb)
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 9992626..762276b 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -5859,8 +5859,7 @@ static int selinux_getprocattr(struct
> > > task_struct *p,
> > >  	return error;
> > >  }
> > >  
> > > -static int selinux_setprocattr(struct task_struct *p,
> > > -			       char *name, void *value, size_t
> > > size)
> > > +static int selinux_setprocattr(const char *name, void *value,
> > > size_t
> > > size)
> > >  {
> > >  	struct task_security_struct *tsec;
> > >  	struct cred *new;
> > > @@ -5868,16 +5867,6 @@ static int selinux_setprocattr(struct
> > > task_struct *p,
> > >  	int error;
> > >  	char *str = value;
> > >  
> > > -	if (current != p) {
> > > -		/*
> > > -		 * A task may only alter its own credentials.
> > > -		 * SELinux has always enforced this restriction,
> > > -		 * and it is now mandated by the Linux
> > > credentials
> > > -		 * infrastructure; see
> > > Documentation/security/credentials.txt.
> > > -		 */
> > > -		return -EACCES;
> > > -	}
> > > -
> > >  	/*
> > >  	 * Basic control over ability to set these attributes at
> > > all.
> > >  	 */
> > > diff --git a/security/smack/smack_lsm.c
> > > b/security/smack/smack_lsm.c
> > > index 4d90257..9bde7f8 100644
> > > --- a/security/smack/smack_lsm.c
> > > +++ b/security/smack/smack_lsm.c
> > > @@ -3620,7 +3620,6 @@ static int smack_getprocattr(struct
> > > task_struct
> > > *p, char *name, char **value)
> > >  
> > >  /**
> > >   * smack_setprocattr - Smack process attribute setting
> > > - * @p: the object task
> > >   * @name: the name of the attribute in /proc/.../attr
> > >   * @value: the value to set
> > >   * @size: the size of the value
> > > @@ -3630,8 +3629,7 @@ static int smack_getprocattr(struct
> > > task_struct
> > > *p, char *name, char **value)
> > >   *
> > >   * Returns the length of the smack label or an error code
> > >   */
> > > -static int smack_setprocattr(struct task_struct *p, char *name,
> > > -			     void *value, size_t size)
> > > +static int smack_setprocattr(const char *name, void *value,
> > > size_t
> > > size)
> > >  {
> > >  	struct task_smack *tsp = current_security();
> > >  	struct cred *new;
> > > @@ -3639,13 +3637,6 @@ static int smack_setprocattr(struct
> > > task_struct *p, char *name,
> > >  	struct smack_known_list_elem *sklep;
> > >  	int rc;
> > >  
> > > -	/*
> > > -	 * Changing another process' Smack value is too
> > > dangerous
> > > -	 * and supports no sane use case.
> > > -	 */
> > > -	if (p != current)
> > > -		return -EPERM;
> > > -
> > >  	if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp-
> > > > 
> > > > smk_relabel))
> > > 
> > >  		return -EPERM;
> > >  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> security-module" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Smalley Dec. 19, 2016, 3:52 p.m. UTC | #9

On Mon, 2016-12-19 at 16:41 +0100, José Bollo wrote:
> Le lundi 19 décembre 2016 à 09:33 -0500, Stephen Smalley a écrit :
> > 
> > On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
> > > 
> > > Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen Smalley a
> > > écrit :
> > > > 
> > > > 
> > > > Processes can only alter their own security attributes via
> > > > /proc/pid/attr nodes.  This is presently enforced by each
> > > > individual
> > > > security module and is also imposed by the Linux credentials
> > > > implementation, which only allows a task to alter its own
> > > > credentials.
> > > > Move the check enforcing this restriction from the individual
> > > > security modules to proc_pid_attr_write() before calling the
> > > > security
> > > > hook,
> > > > and drop the unnecessary task argument to the security hook
> > > > since
> > > > it
> > > > can
> > > > only ever be the current task.
> > > 
> > > Hi Stephen,
> > > 
> > > The module PTAGS that I made relies on the ability to write files
> > > in
> > > /proc/pid/attr/...
> > > 
> > > Why did I used this files? Thaere are serious reasons:
> > >  - follows the life cycle of the process
> > >  - implements pid namespace
> > > 
> > > There is absolutely no way to get these features easyly outside
> > > that
> > > implementation context of /proc/pid/attr/xxx.
> > > 
> > > For these reasons I vote against that change.
> > 
> > It is already prohibited by the credentials framework;
> 
> If it were REALLY prohibited, your patch shouldn't exist ;)

You can't do it safely (and your current implementation is unsafe).
 Please, read Documentation/security/credentials.txt in full, and look
at the setprocattr implementations in the upstream security modules for
reference.

> 
> Best regards
> José Bollo
> 
> > 
> >  see
> > Documentation/security/credentials.txt, under ALTERING CREDENTIALS.
> > We are not imposing a new restriction here, just recognizing that
> > it
> > already exists (ever since Linux moved to using cred structures).
> >  You
> > might not agree with that restriction, but we didn't invent it.
> > 
> > > 
> > > You will probably ask me where is PTAGS used and what is is made
> > > for.
> > > 
> > > So it is not used but the principle of its use is benefical to
> > > the
> > > community. It allows a kind of user land implementation of
> > > capabilities.
> > > 
> > > You probably know that the kernel can not do all things for a
> > > good
> > > reason: it is not intended to do all things. So there is a need
> > > for
> > > user land features. PTAGS is an intent to fulfill an empty gap.
> > > 
> > > I made a presentation at FOSDEM on the subject that may allows
> > > you
> > > to
> > > understand the beginning of the issue that I'm writing about
> > > [2]. 
> > > 
> > > [1] https://lkml.org/lkml/2016/11/23/122
> > > [2] https://archive.fosdem.org/2015/schedule/event/sec_enforcemen
> > > t/
> > > 
> > > > 
> > > > 
> > > > 
> > > > Signed-off-by: Stephen Smalley <[e-mail]>
> > > > ---
> > > >  fs/proc/base.c             | 13 +++++++++----
> > > >  include/linux/lsm_hooks.h  |  3 +--
> > > >  include/linux/security.h   |  4 ++--
> > > >  security/apparmor/lsm.c    |  7 ++-----
> > > >  security/security.c        |  4 ++--
> > > >  security/selinux/hooks.c   | 13 +------------
> > > >  security/smack/smack_lsm.c | 11 +----------
> > > >  7 files changed, 18 insertions(+), 37 deletions(-)
> > > > 
> > > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > > index 6eae4d0..7b228ea 100644
> > > > --- a/fs/proc/base.c
> > > > +++ b/fs/proc/base.c
> > > > @@ -2485,6 +2485,12 @@ static ssize_t
> > > > proc_pid_attr_write(struct
> > > > file
> > > > * file, const char __user * buf,
> > > >  	length = -ESRCH;
> > > >  	if (!task)
> > > >  		goto out_no_task;
> > > > +
> > > > +	/* A task may only write its own attributes. */
> > > > +	length = -EACCES;
> > > > +	if (current != task)
> > > > +		goto out;
> > > > +
> > > >  	if (count > PAGE_SIZE)
> > > >  		count = PAGE_SIZE;
> > > >  
> > > > @@ -2500,14 +2506,13 @@ static ssize_t
> > > > proc_pid_attr_write(struct
> > > > file * file, const char __user * buf,
> > > >  	}
> > > >  
> > > >  	/* Guard against adverse ptrace interaction */
> > > > -	length = mutex_lock_interruptible(&task->signal-
> > > > > 
> > > > > 
> > > > > cred_guard_mutex);
> > > > 
> > > > +	length = mutex_lock_interruptible(&current->signal-
> > > > > 
> > > > > 
> > > > > cred_guard_mutex);
> > > > 
> > > >  	if (length < 0)
> > > >  		goto out_free;
> > > >  
> > > > -	length = security_setprocattr(task,
> > > > -				      (char*)file-
> > > > > 
> > > > > f_path.dentry-
> > > > > 
> > > > > d_name.name,
> > > > 
> > > > +	length = security_setprocattr(file->f_path.dentry-
> > > > > 
> > > > > 
> > > > > d_name.name,
> > > > 
> > > >  				      page, count);
> > > > -	mutex_unlock(&task->signal->cred_guard_mutex);
> > > > +	mutex_unlock(&current->signal->cred_guard_mutex);
> > > >  out_free:
> > > >  	kfree(page);
> > > >  out:
> > > > diff --git a/include/linux/lsm_hooks.h
> > > > b/include/linux/lsm_hooks.h
> > > > index 558adfa..0dde959 100644
> > > > --- a/include/linux/lsm_hooks.h
> > > > +++ b/include/linux/lsm_hooks.h
> > > > @@ -1547,8 +1547,7 @@ union security_list_options {
> > > >  	void (*d_instantiate)(struct dentry *dentry, struct
> > > > inode
> > > > *inode);
> > > >  
> > > >  	int (*getprocattr)(struct task_struct *p, char *name,
> > > > char
> > > > **value);
> > > > -	int (*setprocattr)(struct task_struct *p, char *name,
> > > > void
> > > > *value,
> > > > -				size_t size);
> > > > +	int (*setprocattr)(const char *name, void *value,
> > > > size_t
> > > > size);
> > > >  	int (*ismaclabel)(const char *name);
> > > >  	int (*secid_to_secctx)(u32 secid, char **secdata, u32
> > > > *seclen);
> > > >  	int (*secctx_to_secid)(const char *secdata, u32
> > > > seclen,
> > > > u32
> > > > *secid);
> > > > diff --git a/include/linux/security.h
> > > > b/include/linux/security.h
> > > > index c2125e9..f4ebac1 100644
> > > > --- a/include/linux/security.h
> > > > +++ b/include/linux/security.h
> > > > @@ -361,7 +361,7 @@ int security_sem_semop(struct sem_array
> > > > *sma,
> > > > struct sembuf *sops,
> > > >  			unsigned nsops, int alter);
> > > >  void security_d_instantiate(struct dentry *dentry, struct
> > > > inode
> > > > *inode);
> > > >  int security_getprocattr(struct task_struct *p, char *name,
> > > > char
> > > > **value);
> > > > -int security_setprocattr(struct task_struct *p, char *name,
> > > > void
> > > > *value, size_t size);
> > > > +int security_setprocattr(const char *name, void *value, size_t
> > > > size);
> > > >  int security_netlink_send(struct sock *sk, struct sk_buff
> > > > *skb);
> > > >  int security_ismaclabel(const char *name);
> > > >  int security_secid_to_secctx(u32 secid, char **secdata, u32
> > > > *seclen);
> > > > @@ -1106,7 +1106,7 @@ static inline int
> > > > security_getprocattr(struct
> > > > task_struct *p, char *name, char *
> > > >  	return -EINVAL;
> > > >  }
> > > >  
> > > > -static inline int security_setprocattr(struct task_struct *p,
> > > > char
> > > > *name, void *value, size_t size)
> > > > +static inline int security_setprocattr(char *name, void
> > > > *value,
> > > > size_t size)
> > > >  {
> > > >  	return -EINVAL;
> > > >  }
> > > > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > > > index 41b8cb1..8202e55 100644
> > > > --- a/security/apparmor/lsm.c
> > > > +++ b/security/apparmor/lsm.c
> > > > @@ -495,8 +495,8 @@ static int apparmor_getprocattr(struct
> > > > task_struct *task, char *name,
> > > >  	return error;
> > > >  }
> > > >  
> > > > -static int apparmor_setprocattr(struct task_struct *task, char
> > > > *name,
> > > > -				void *value, size_t size)
> > > > +static int apparmor_setprocattr(const char *name, void *value,
> > > > +				size_t size)
> > > >  {
> > > >  	struct common_audit_data sa;
> > > >  	struct apparmor_audit_data aad = {0,};
> > > > @@ -506,9 +506,6 @@ static int apparmor_setprocattr(struct
> > > > task_struct *task, char *name,
> > > >  
> > > >  	if (size == 0)
> > > >  		return -EINVAL;
> > > > -	/* task can only write its own attributes */
> > > > -	if (current != task)
> > > > -		return -EACCES;
> > > >  
> > > >  	/* AppArmor requires that the buffer must be null
> > > > terminated
> > > > atm */
> > > >  	if (args[size - 1] != '\0') {
> > > > diff --git a/security/security.c b/security/security.c
> > > > index f825304..32052f5 100644
> > > > --- a/security/security.c
> > > > +++ b/security/security.c
> > > > @@ -1170,9 +1170,9 @@ int security_getprocattr(struct
> > > > task_struct
> > > > *p,
> > > > char *name, char **value)
> > > >  	return call_int_hook(getprocattr, -EINVAL, p, name,
> > > > value);
> > > >  }
> > > >  
> > > > -int security_setprocattr(struct task_struct *p, char *name,
> > > > void
> > > > *value, size_t size)
> > > > +int security_setprocattr(const char *name, void *value, size_t
> > > > size)
> > > >  {
> > > > -	return call_int_hook(setprocattr, -EINVAL, p, name,
> > > > value,
> > > > size);
> > > > +	return call_int_hook(setprocattr, -EINVAL, name,
> > > > value,
> > > > size);
> > > >  }
> > > >  
> > > >  int security_netlink_send(struct sock *sk, struct sk_buff
> > > > *skb)
> > > > diff --git a/security/selinux/hooks.c
> > > > b/security/selinux/hooks.c
> > > > index 9992626..762276b 100644
> > > > --- a/security/selinux/hooks.c
> > > > +++ b/security/selinux/hooks.c
> > > > @@ -5859,8 +5859,7 @@ static int selinux_getprocattr(struct
> > > > task_struct *p,
> > > >  	return error;
> > > >  }
> > > >  
> > > > -static int selinux_setprocattr(struct task_struct *p,
> > > > -			       char *name, void *value, size_t
> > > > size)
> > > > +static int selinux_setprocattr(const char *name, void *value,
> > > > size_t
> > > > size)
> > > >  {
> > > >  	struct task_security_struct *tsec;
> > > >  	struct cred *new;
> > > > @@ -5868,16 +5867,6 @@ static int selinux_setprocattr(struct
> > > > task_struct *p,
> > > >  	int error;
> > > >  	char *str = value;
> > > >  
> > > > -	if (current != p) {
> > > > -		/*
> > > > -		 * A task may only alter its own credentials.
> > > > -		 * SELinux has always enforced this
> > > > restriction,
> > > > -		 * and it is now mandated by the Linux
> > > > credentials
> > > > -		 * infrastructure; see
> > > > Documentation/security/credentials.txt.
> > > > -		 */
> > > > -		return -EACCES;
> > > > -	}
> > > > -
> > > >  	/*
> > > >  	 * Basic control over ability to set these attributes
> > > > at
> > > > all.
> > > >  	 */
> > > > diff --git a/security/smack/smack_lsm.c
> > > > b/security/smack/smack_lsm.c
> > > > index 4d90257..9bde7f8 100644
> > > > --- a/security/smack/smack_lsm.c
> > > > +++ b/security/smack/smack_lsm.c
> > > > @@ -3620,7 +3620,6 @@ static int smack_getprocattr(struct
> > > > task_struct
> > > > *p, char *name, char **value)
> > > >  
> > > >  /**
> > > >   * smack_setprocattr - Smack process attribute setting
> > > > - * @p: the object task
> > > >   * @name: the name of the attribute in /proc/.../attr
> > > >   * @value: the value to set
> > > >   * @size: the size of the value
> > > > @@ -3630,8 +3629,7 @@ static int smack_getprocattr(struct
> > > > task_struct
> > > > *p, char *name, char **value)
> > > >   *
> > > >   * Returns the length of the smack label or an error code
> > > >   */
> > > > -static int smack_setprocattr(struct task_struct *p, char
> > > > *name,
> > > > -			     void *value, size_t size)
> > > > +static int smack_setprocattr(const char *name, void *value,
> > > > size_t
> > > > size)
> > > >  {
> > > >  	struct task_smack *tsp = current_security();
> > > >  	struct cred *new;
> > > > @@ -3639,13 +3637,6 @@ static int smack_setprocattr(struct
> > > > task_struct *p, char *name,
> > > >  	struct smack_known_list_elem *sklep;
> > > >  	int rc;
> > > >  
> > > > -	/*
> > > > -	 * Changing another process' Smack value is too
> > > > dangerous
> > > > -	 * and supports no sane use case.
> > > > -	 */
> > > > -	if (p != current)
> > > > -		return -EPERM;
> > > > -
> > > >  	if (!smack_privileged(CAP_MAC_ADMIN) &&
> > > > list_empty(&tsp-
> > > > > 
> > > > > 
> > > > > smk_relabel))
> > > > 
> > > >  		return -EPERM;
> > > >  
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > security-module" in
> > the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler Dec. 19, 2016, 4:32 p.m. UTC | #10

On 12/19/2016 7:52 AM, Stephen Smalley wrote:
> On Mon, 2016-12-19 at 16:41 +0100, José Bollo wrote:
>> Le lundi 19 décembre 2016 à 09:33 -0500, Stephen Smalley a écrit :
>>> On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
>>>> Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen Smalley a
>>>> écrit :
>>>>>
>>>>> Processes can only alter their own security attributes via
>>>>> /proc/pid/attr nodes.  This is presently enforced by each
>>>>> individual
>>>>> security module and is also imposed by the Linux credentials
>>>>> implementation, which only allows a task to alter its own
>>>>> credentials.
>>>>> Move the check enforcing this restriction from the individual
>>>>> security modules to proc_pid_attr_write() before calling the
>>>>> security
>>>>> hook,
>>>>> and drop the unnecessary task argument to the security hook
>>>>> since
>>>>> it
>>>>> can
>>>>> only ever be the current task.
>>>> Hi Stephen,
>>>>
>>>> The module PTAGS that I made relies on the ability to write files
>>>> in
>>>> /proc/pid/attr/...
>>>>
>>>> Why did I used this files? Thaere are serious reasons:
>>>>  - follows the life cycle of the process
>>>>  - implements pid namespace
>>>>
>>>> There is absolutely no way to get these features easyly outside
>>>> that
>>>> implementation context of /proc/pid/attr/xxx.
>>>>
>>>> For these reasons I vote against that change.
>>> It is already prohibited by the credentials framework;
>> If it were REALLY prohibited, your patch shouldn't exist ;)
> You can't do it safely (and your current implementation is unsafe).
>  Please, read Documentation/security/credentials.txt in full, and look
> at the setprocattr implementations in the upstream security modules for
> reference.

I just reviewed the referenced documentation (thanks David)
and it does clearly identify why, from the viewpoint of the
credential implementation, modifying another task's credentials
is unsafe. Both Smack and SELinux view modification of task
credentials as unsafe from a security viewpoint, so neither of
these modules would force the credential implementation to
allow the behavior.

This, unfortunately, leaves Jose in a bit of a bind. It looks
to me like there are options, so let's not just tell him to
go away. The options I see include:

 - Fix the credential code so modifying another task is safe
	I don't advocate this approach because the existing
	implementation is only reasonable because of the
	restriction.
 - Revert shared credentials
	I confess to not being a fan of shared credentials,
	for reason of complexity. I would not expect a
	proposal to go back to embedding credentials in
	their containing structures to be considered seriously.
	Nonetheless, it would solve the problem.
 - Add a security blob for the task
	It is unfortunate that when shared credentials were
	introduced the task blob was not renamed a cred blob.
	It would be simple and consistent to add a security
	blob to the task in addition to the blob in the cred.
	I would add a t_security field to the task structure
	that associates security data with the task. This
	blob would be used for data that does not meet intent
	of a credential, which I believe may be the case for
	Jose's PTAGS.

I know which I would prefer, but as I expect someone to shout
that none can possibly be acceptable I think I'll stop here and
let the discussion go a while.

("Light fuse. Stand back. :))

>
>> Best regards
>> José Bollo
>>
>>>  see
>>> Documentation/security/credentials.txt, under ALTERING CREDENTIALS.
>>> We are not imposing a new restriction here, just recognizing that
>>> it
>>> already exists (ever since Linux moved to using cred structures).
>>>  You
>>> might not agree with that restriction, but we didn't invent it.
>>>
>>>> You will probably ask me where is PTAGS used and what is is made
>>>> for.
>>>>
>>>> So it is not used but the principle of its use is benefical to
>>>> the
>>>> community. It allows a kind of user land implementation of
>>>> capabilities.
>>>>
>>>> You probably know that the kernel can not do all things for a
>>>> good
>>>> reason: it is not intended to do all things. So there is a need
>>>> for
>>>> user land features. PTAGS is an intent to fulfill an empty gap.
>>>>
>>>> I made a presentation at FOSDEM on the subject that may allows
>>>> you
>>>> to
>>>> understand the beginning of the issue that I'm writing about
>>>> [2]. 
>>>>
>>>> [1] https://lkml.org/lkml/2016/11/23/122
>>>> [2] https://archive.fosdem.org/2015/schedule/event/sec_enforcemen
>>>> t/
>>>>
>>>>>
>>>>>
>>>>> Signed-off-by: Stephen Smalley <[e-mail]>
>>>>> ---
>>>>>  fs/proc/base.c             | 13 +++++++++----
>>>>>  include/linux/lsm_hooks.h  |  3 +--
>>>>>  include/linux/security.h   |  4 ++--
>>>>>  security/apparmor/lsm.c    |  7 ++-----
>>>>>  security/security.c        |  4 ++--
>>>>>  security/selinux/hooks.c   | 13 +------------
>>>>>  security/smack/smack_lsm.c | 11 +----------
>>>>>  7 files changed, 18 insertions(+), 37 deletions(-)
>>>>>
>>>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>>>> index 6eae4d0..7b228ea 100644
>>>>> --- a/fs/proc/base.c
>>>>> +++ b/fs/proc/base.c
>>>>> @@ -2485,6 +2485,12 @@ static ssize_t
>>>>> proc_pid_attr_write(struct
>>>>> file
>>>>> * file, const char __user * buf,
>>>>>  	length = -ESRCH;
>>>>>  	if (!task)
>>>>>  		goto out_no_task;
>>>>> +
>>>>> +	/* A task may only write its own attributes. */
>>>>> +	length = -EACCES;
>>>>> +	if (current != task)
>>>>> +		goto out;
>>>>> +
>>>>>  	if (count > PAGE_SIZE)
>>>>>  		count = PAGE_SIZE;
>>>>>  
>>>>> @@ -2500,14 +2506,13 @@ static ssize_t
>>>>> proc_pid_attr_write(struct
>>>>> file * file, const char __user * buf,
>>>>>  	}
>>>>>  
>>>>>  	/* Guard against adverse ptrace interaction */
>>>>> -	length = mutex_lock_interruptible(&task->signal-
>>>>>>
>>>>>> cred_guard_mutex);
>>>>> +	length = mutex_lock_interruptible(&current->signal-
>>>>>>
>>>>>> cred_guard_mutex);
>>>>>  	if (length < 0)
>>>>>  		goto out_free;
>>>>>  
>>>>> -	length = security_setprocattr(task,
>>>>> -				      (char*)file-
>>>>>> f_path.dentry-
>>>>>>
>>>>>> d_name.name,
>>>>> +	length = security_setprocattr(file->f_path.dentry-
>>>>>>
>>>>>> d_name.name,
>>>>>  				      page, count);
>>>>> -	mutex_unlock(&task->signal->cred_guard_mutex);
>>>>> +	mutex_unlock(&current->signal->cred_guard_mutex);
>>>>>  out_free:
>>>>>  	kfree(page);
>>>>>  out:
>>>>> diff --git a/include/linux/lsm_hooks.h
>>>>> b/include/linux/lsm_hooks.h
>>>>> index 558adfa..0dde959 100644
>>>>> --- a/include/linux/lsm_hooks.h
>>>>> +++ b/include/linux/lsm_hooks.h
>>>>> @@ -1547,8 +1547,7 @@ union security_list_options {
>>>>>  	void (*d_instantiate)(struct dentry *dentry, struct
>>>>> inode
>>>>> *inode);
>>>>>  
>>>>>  	int (*getprocattr)(struct task_struct *p, char *name,
>>>>> char
>>>>> **value);
>>>>> -	int (*setprocattr)(struct task_struct *p, char *name,
>>>>> void
>>>>> *value,
>>>>> -				size_t size);
>>>>> +	int (*setprocattr)(const char *name, void *value,
>>>>> size_t
>>>>> size);
>>>>>  	int (*ismaclabel)(const char *name);
>>>>>  	int (*secid_to_secctx)(u32 secid, char **secdata, u32
>>>>> *seclen);
>>>>>  	int (*secctx_to_secid)(const char *secdata, u32
>>>>> seclen,
>>>>> u32
>>>>> *secid);
>>>>> diff --git a/include/linux/security.h
>>>>> b/include/linux/security.h
>>>>> index c2125e9..f4ebac1 100644
>>>>> --- a/include/linux/security.h
>>>>> +++ b/include/linux/security.h
>>>>> @@ -361,7 +361,7 @@ int security_sem_semop(struct sem_array
>>>>> *sma,
>>>>> struct sembuf *sops,
>>>>>  			unsigned nsops, int alter);
>>>>>  void security_d_instantiate(struct dentry *dentry, struct
>>>>> inode
>>>>> *inode);
>>>>>  int security_getprocattr(struct task_struct *p, char *name,
>>>>> char
>>>>> **value);
>>>>> -int security_setprocattr(struct task_struct *p, char *name,
>>>>> void
>>>>> *value, size_t size);
>>>>> +int security_setprocattr(const char *name, void *value, size_t
>>>>> size);
>>>>>  int security_netlink_send(struct sock *sk, struct sk_buff
>>>>> *skb);
>>>>>  int security_ismaclabel(const char *name);
>>>>>  int security_secid_to_secctx(u32 secid, char **secdata, u32
>>>>> *seclen);
>>>>> @@ -1106,7 +1106,7 @@ static inline int
>>>>> security_getprocattr(struct
>>>>> task_struct *p, char *name, char *
>>>>>  	return -EINVAL;
>>>>>  }
>>>>>  
>>>>> -static inline int security_setprocattr(struct task_struct *p,
>>>>> char
>>>>> *name, void *value, size_t size)
>>>>> +static inline int security_setprocattr(char *name, void
>>>>> *value,
>>>>> size_t size)
>>>>>  {
>>>>>  	return -EINVAL;
>>>>>  }
>>>>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>>>>> index 41b8cb1..8202e55 100644
>>>>> --- a/security/apparmor/lsm.c
>>>>> +++ b/security/apparmor/lsm.c
>>>>> @@ -495,8 +495,8 @@ static int apparmor_getprocattr(struct
>>>>> task_struct *task, char *name,
>>>>>  	return error;
>>>>>  }
>>>>>  
>>>>> -static int apparmor_setprocattr(struct task_struct *task, char
>>>>> *name,
>>>>> -				void *value, size_t size)
>>>>> +static int apparmor_setprocattr(const char *name, void *value,
>>>>> +				size_t size)
>>>>>  {
>>>>>  	struct common_audit_data sa;
>>>>>  	struct apparmor_audit_data aad = {0,};
>>>>> @@ -506,9 +506,6 @@ static int apparmor_setprocattr(struct
>>>>> task_struct *task, char *name,
>>>>>  
>>>>>  	if (size == 0)
>>>>>  		return -EINVAL;
>>>>> -	/* task can only write its own attributes */
>>>>> -	if (current != task)
>>>>> -		return -EACCES;
>>>>>  
>>>>>  	/* AppArmor requires that the buffer must be null
>>>>> terminated
>>>>> atm */
>>>>>  	if (args[size - 1] != '\0') {
>>>>> diff --git a/security/security.c b/security/security.c
>>>>> index f825304..32052f5 100644
>>>>> --- a/security/security.c
>>>>> +++ b/security/security.c
>>>>> @@ -1170,9 +1170,9 @@ int security_getprocattr(struct
>>>>> task_struct
>>>>> *p,
>>>>> char *name, char **value)
>>>>>  	return call_int_hook(getprocattr, -EINVAL, p, name,
>>>>> value);
>>>>>  }
>>>>>  
>>>>> -int security_setprocattr(struct task_struct *p, char *name,
>>>>> void
>>>>> *value, size_t size)
>>>>> +int security_setprocattr(const char *name, void *value, size_t
>>>>> size)
>>>>>  {
>>>>> -	return call_int_hook(setprocattr, -EINVAL, p, name,
>>>>> value,
>>>>> size);
>>>>> +	return call_int_hook(setprocattr, -EINVAL, name,
>>>>> value,
>>>>> size);
>>>>>  }
>>>>>  
>>>>>  int security_netlink_send(struct sock *sk, struct sk_buff
>>>>> *skb)
>>>>> diff --git a/security/selinux/hooks.c
>>>>> b/security/selinux/hooks.c
>>>>> index 9992626..762276b 100644
>>>>> --- a/security/selinux/hooks.c
>>>>> +++ b/security/selinux/hooks.c
>>>>> @@ -5859,8 +5859,7 @@ static int selinux_getprocattr(struct
>>>>> task_struct *p,
>>>>>  	return error;
>>>>>  }
>>>>>  
>>>>> -static int selinux_setprocattr(struct task_struct *p,
>>>>> -			       char *name, void *value, size_t
>>>>> size)
>>>>> +static int selinux_setprocattr(const char *name, void *value,
>>>>> size_t
>>>>> size)
>>>>>  {
>>>>>  	struct task_security_struct *tsec;
>>>>>  	struct cred *new;
>>>>> @@ -5868,16 +5867,6 @@ static int selinux_setprocattr(struct
>>>>> task_struct *p,
>>>>>  	int error;
>>>>>  	char *str = value;
>>>>>  
>>>>> -	if (current != p) {
>>>>> -		/*
>>>>> -		 * A task may only alter its own credentials.
>>>>> -		 * SELinux has always enforced this
>>>>> restriction,
>>>>> -		 * and it is now mandated by the Linux
>>>>> credentials
>>>>> -		 * infrastructure; see
>>>>> Documentation/security/credentials.txt.
>>>>> -		 */
>>>>> -		return -EACCES;
>>>>> -	}
>>>>> -
>>>>>  	/*
>>>>>  	 * Basic control over ability to set these attributes
>>>>> at
>>>>> all.
>>>>>  	 */
>>>>> diff --git a/security/smack/smack_lsm.c
>>>>> b/security/smack/smack_lsm.c
>>>>> index 4d90257..9bde7f8 100644
>>>>> --- a/security/smack/smack_lsm.c
>>>>> +++ b/security/smack/smack_lsm.c
>>>>> @@ -3620,7 +3620,6 @@ static int smack_getprocattr(struct
>>>>> task_struct
>>>>> *p, char *name, char **value)
>>>>>  
>>>>>  /**
>>>>>   * smack_setprocattr - Smack process attribute setting
>>>>> - * @p: the object task
>>>>>   * @name: the name of the attribute in /proc/.../attr
>>>>>   * @value: the value to set
>>>>>   * @size: the size of the value
>>>>> @@ -3630,8 +3629,7 @@ static int smack_getprocattr(struct
>>>>> task_struct
>>>>> *p, char *name, char **value)
>>>>>   *
>>>>>   * Returns the length of the smack label or an error code
>>>>>   */
>>>>> -static int smack_setprocattr(struct task_struct *p, char
>>>>> *name,
>>>>> -			     void *value, size_t size)
>>>>> +static int smack_setprocattr(const char *name, void *value,
>>>>> size_t
>>>>> size)
>>>>>  {
>>>>>  	struct task_smack *tsp = current_security();
>>>>>  	struct cred *new;
>>>>> @@ -3639,13 +3637,6 @@ static int smack_setprocattr(struct
>>>>> task_struct *p, char *name,
>>>>>  	struct smack_known_list_elem *sklep;
>>>>>  	int rc;
>>>>>  
>>>>> -	/*
>>>>> -	 * Changing another process' Smack value is too
>>>>> dangerous
>>>>> -	 * and supports no sane use case.
>>>>> -	 */
>>>>> -	if (p != current)
>>>>> -		return -EPERM;
>>>>> -
>>>>>  	if (!smack_privileged(CAP_MAC_ADMIN) &&
>>>>> list_empty(&tsp-
>>>>>>
>>>>>> smk_relabel))
>>>>>  		return -EPERM;
>>>>>  
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-
>>> security-module" in
>>> the body of a message to [e-mail]
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Smalley Dec. 19, 2016, 5:09 p.m. UTC | #11

On Mon, 2016-12-19 at 08:32 -0800, Casey Schaufler wrote:
> On 12/19/2016 7:52 AM, Stephen Smalley wrote:
> > 
> > On Mon, 2016-12-19 at 16:41 +0100, José Bollo wrote:
> > > 
> > > Le lundi 19 décembre 2016 à 09:33 -0500, Stephen Smalley a écrit
> > > :
> > > > 
> > > > On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
> > > > > 
> > > > > Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen Smalley a
> > > > > écrit :
> > > > > > 
> > > > > > 
> > > > > > Processes can only alter their own security attributes via
> > > > > > /proc/pid/attr nodes.  This is presently enforced by each
> > > > > > individual
> > > > > > security module and is also imposed by the Linux
> > > > > > credentials
> > > > > > implementation, which only allows a task to alter its own
> > > > > > credentials.
> > > > > > Move the check enforcing this restriction from the
> > > > > > individual
> > > > > > security modules to proc_pid_attr_write() before calling
> > > > > > the
> > > > > > security
> > > > > > hook,
> > > > > > and drop the unnecessary task argument to the security hook
> > > > > > since
> > > > > > it
> > > > > > can
> > > > > > only ever be the current task.
> > > > > Hi Stephen,
> > > > > 
> > > > > The module PTAGS that I made relies on the ability to write
> > > > > files
> > > > > in
> > > > > /proc/pid/attr/...
> > > > > 
> > > > > Why did I used this files? Thaere are serious reasons:
> > > > >  - follows the life cycle of the process
> > > > >  - implements pid namespace
> > > > > 
> > > > > There is absolutely no way to get these features easyly
> > > > > outside
> > > > > that
> > > > > implementation context of /proc/pid/attr/xxx.
> > > > > 
> > > > > For these reasons I vote against that change.
> > > > It is already prohibited by the credentials framework;
> > > If it were REALLY prohibited, your patch shouldn't exist ;)
> > You can't do it safely (and your current implementation is unsafe).
> >  Please, read Documentation/security/credentials.txt in full, and
> > look
> > at the setprocattr implementations in the upstream security modules
> > for
> > reference.
> 
> I just reviewed the referenced documentation (thanks David)
> and it does clearly identify why, from the viewpoint of the
> credential implementation, modifying another task's credentials
> is unsafe. Both Smack and SELinux view modification of task
> credentials as unsafe from a security viewpoint, so neither of
> these modules would force the credential implementation to
> allow the behavior.
> 
> This, unfortunately, leaves Jose in a bit of a bind. It looks
> to me like there are options, so let's not just tell him to
> go away. The options I see include:
> 
>  - Fix the credential code so modifying another task is safe
> 	I don't advocate this approach because the existing
> 	implementation is only reasonable because of the
> 	restriction.
>  - Revert shared credentials
> 	I confess to not being a fan of shared credentials,
> 	for reason of complexity. I would not expect a
> 	proposal to go back to embedding credentials in
> 	their containing structures to be considered seriously.
> 	Nonetheless, it would solve the problem.
>  - Add a security blob for the task
> 	It is unfortunate that when shared credentials were
> 	introduced the task blob was not renamed a cred blob.
> 	It would be simple and consistent to add a security
> 	blob to the task in addition to the blob in the cred.
> 	I would add a t_security field to the task structure
> 	that associates security data with the task. This
> 	blob would be used for data that does not meet intent
> 	of a credential, which I believe may be the case for
> 	Jose's PTAGS.
> 
> I know which I would prefer, but as I expect someone to shout
> that none can possibly be acceptable I think I'll stop here and
> let the discussion go a while.
> 
> ("Light fuse. Stand back. :))

Why not:

- Reconsider the approach used in PTAGS in light of the security
implications of allowing one task to change another task's security
attributes, and rework PTAGS to use a more secure approach.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler Dec. 19, 2016, 6 p.m. UTC | #12

On 12/19/2016 9:09 AM, Stephen Smalley wrote:
> On Mon, 2016-12-19 at 08:32 -0800, Casey Schaufler wrote:
>> On 12/19/2016 7:52 AM, Stephen Smalley wrote:
>>> On Mon, 2016-12-19 at 16:41 +0100, José Bollo wrote:
>>>> Le lundi 19 décembre 2016 à 09:33 -0500, Stephen Smalley a écrit
>>>> :
>>>>> On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
>>>>>> Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen Smalley a
>>>>>> écrit :
>>>>>>>
>>>>>>> Processes can only alter their own security attributes via
>>>>>>> /proc/pid/attr nodes.  This is presently enforced by each
>>>>>>> individual
>>>>>>> security module and is also imposed by the Linux
>>>>>>> credentials
>>>>>>> implementation, which only allows a task to alter its own
>>>>>>> credentials.
>>>>>>> Move the check enforcing this restriction from the
>>>>>>> individual
>>>>>>> security modules to proc_pid_attr_write() before calling
>>>>>>> the
>>>>>>> security
>>>>>>> hook,
>>>>>>> and drop the unnecessary task argument to the security hook
>>>>>>> since
>>>>>>> it
>>>>>>> can
>>>>>>> only ever be the current task.
>>>>>> Hi Stephen,
>>>>>>
>>>>>> The module PTAGS that I made relies on the ability to write
>>>>>> files
>>>>>> in
>>>>>> /proc/pid/attr/...
>>>>>>
>>>>>> Why did I used this files? Thaere are serious reasons:
>>>>>>  - follows the life cycle of the process
>>>>>>  - implements pid namespace
>>>>>>
>>>>>> There is absolutely no way to get these features easyly
>>>>>> outside
>>>>>> that
>>>>>> implementation context of /proc/pid/attr/xxx.
>>>>>>
>>>>>> For these reasons I vote against that change.
>>>>> It is already prohibited by the credentials framework;
>>>> If it were REALLY prohibited, your patch shouldn't exist ;)
>>> You can't do it safely (and your current implementation is unsafe).
>>>  Please, read Documentation/security/credentials.txt in full, and
>>> look
>>> at the setprocattr implementations in the upstream security modules
>>> for
>>> reference.
>> I just reviewed the referenced documentation (thanks David)
>> and it does clearly identify why, from the viewpoint of the
>> credential implementation, modifying another task's credentials
>> is unsafe. Both Smack and SELinux view modification of task
>> credentials as unsafe from a security viewpoint, so neither of
>> these modules would force the credential implementation to
>> allow the behavior.
>>
>> This, unfortunately, leaves Jose in a bit of a bind. It looks
>> to me like there are options, so let's not just tell him to
>> go away. The options I see include:
>>
>>  - Fix the credential code so modifying another task is safe
>> 	I don't advocate this approach because the existing
>> 	implementation is only reasonable because of the
>> 	restriction.
>>  - Revert shared credentials
>> 	I confess to not being a fan of shared credentials,
>> 	for reason of complexity. I would not expect a
>> 	proposal to go back to embedding credentials in
>> 	their containing structures to be considered seriously.
>> 	Nonetheless, it would solve the problem.
>>  - Add a security blob for the task
>> 	It is unfortunate that when shared credentials were
>> 	introduced the task blob was not renamed a cred blob.
>> 	It would be simple and consistent to add a security
>> 	blob to the task in addition to the blob in the cred.
>> 	I would add a t_security field to the task structure
>> 	that associates security data with the task. This
>> 	blob would be used for data that does not meet intent
>> 	of a credential, which I believe may be the case for
>> 	Jose's PTAGS.
>>
>> I know which I would prefer, but as I expect someone to shout
>> that none can possibly be acceptable I think I'll stop here and
>> let the discussion go a while.
>>
>> ("Light fuse. Stand back. :))
> Why not:
>
> - Reconsider the approach used in PTAGS in light of the security
> implications of allowing one task to change another task's security
> attributes, and rework PTAGS to use a more secure approach.

Not every approach to security relies on being unable to change
another process' attributes. I know of people who believe that
SELinux would be more secure if an application launcher could set
the context on another process. While I do not agree with these
people I understand the logic they use and why it does not apply
to SELinux or Smack.

Consider a module that does statistical analysis on program behavior
and tags (using PTAGS or something similar) processes that are
behaving in suspect ways. You probably don't want the math running
in the kernel. The kernel decides to start denying the suspect
process access once some tagging threshold is exceeded. Sure, you
might be able to come up with some mechanism to get the process to
tag itself, but it's kind of silly to make it hard when it doesn't
have to be.

If you can suggest a mechanism that achieves Jose's goals (which
may not match your goals) that does not require allowing a process
to change another's security attributes I'm sure we'd all be
interested.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

José Bollo Dec. 19, 2016, 6:12 p.m. UTC | #13

Le lundi 19 décembre 2016 à 12:09 -0500, Stephen Smalley a écrit :
> On Mon, 2016-12-19 at 08:32 -0800, Casey Schaufler wrote:

(snip)

> > I just reviewed the referenced documentation (thanks David)
> > and it does clearly identify why, from the viewpoint of the
> > credential implementation, modifying another task's credentials
> > is unsafe. Both Smack and SELinux view modification of task
> > credentials as unsafe from a security viewpoint, so neither of
> > these modules would force the credential implementation to
> > allow the behavior.
> > 
> > This, unfortunately, leaves Jose in a bit of a bind. It looks
> > to me like there are options, so let's not just tell him to
> > go away. The options I see include:
> > 
> >  - Fix the credential code so modifying another task is safe
> > 	I don't advocate this approach because the existing
> > 	implementation is only reasonable because of the
> > 	restriction.
> >  - Revert shared credentials
> > 	I confess to not being a fan of shared credentials,
> > 	for reason of complexity. I would not expect a
> > 	proposal to go back to embedding credentials in
> > 	their containing structures to be considered seriously.
> > 	Nonetheless, it would solve the problem.

Hi Casey,

Thank you for this review of the possibilities because it opens my eyes
on the fact that I probably missed this "sharing credentials". Now I
understand better the issue (but probably only in surface): avoiding 
duplication not needed. I understand that it complicates things. For
example, the current implementation of PTAGS copies every thing even
for threads. It is more direct but it implies many unneeded
duplications.

When were shared credentials introduced?

> >  - Add a security blob for the task
> > 	It is unfortunate that when shared credentials were
> > 	introduced the task blob was not renamed a cred blob.
> > 	It would be simple and consistent to add a security
> > 	blob to the task in addition to the blob in the cred.
> > 	I would add a t_security field to the task structure
> > 	that associates security data with the task. This
> > 	blob would be used for data that does not meet intent
> > 	of a credential, which I believe may be the case for
> > 	Jose's PTAGS.
> > 
> > I know which I would prefer, but as I expect someone to shout
> > that none can possibly be acceptable I think I'll stop here and
> > let the discussion go a while.
> > 
> > ("Light fuse. Stand back. :))
> 
> Why not:
> 
> - Reconsider the approach used in PTAGS in light of the security
> implications of allowing one task to change another task's security
> attributes, and rework PTAGS to use a more secure approach.

PTAGS has not much things to do with kernel internals. It is only
intended to be available outside of the kernel (*).

Enforcing a task to only modify its attributes, not the one of other
tasks, has strong implication on the architecture of the system that
you build on top of PTAGS. It would make launchers mandatory, would
prohibit revocations, would forbid dynamic permissions and security
cookies. It could be possible but it would limit the possibilities.

The main idea of PTAGS is to attach data to tasks. How can it be done?
To follow life of tasks, LSM hooks is great and stacking is formidable.
To interface task's attributes, taking care of namespaces PID and user,
/proc/pid/attr is a very good place.

Best regards
José

(*) I asked me whether it were valuable provide kernel API for using
PTAGS internally but frankly it is a bad idea. Kernel has capabilities
and that's all. PTAGS isn't here to duplicate this behaviour.



--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

José Bollo Dec. 19, 2016, 6:18 p.m. UTC | #14

Le lundi 19 décembre 2016 à 10:00 -0800, Casey Schaufler a écrit :

(snip)

> Not every approach to security relies on being unable to change
> another process' attributes. I know of people who believe that
> SELinux would be more secure if an application launcher could set
> the context on another process. While I do not agree with these
> people I understand the logic they use and why it does not apply
> to SELinux or Smack.
> 
> Consider a module that does statistical analysis on program behavior
> and tags (using PTAGS or something similar) processes that are
> behaving in suspect ways. You probably don't want the math running
> in the kernel. The kernel decides to start denying the suspect
> process access once some tagging threshold is exceeded. Sure, you
> might be able to come up with some mechanism to get the process to
> tag itself, but it's kind of silly to make it hard when it doesn't
> have to be.

That is a very example of what can be achieved using PTAGS. Thank you.

> If you can suggest a mechanism that achieves Jose's goals (which
> may not match your goals) that does not require allowing a process
> to change another's security attributes I'm sure we'd all be
> interested.

Yes. And what would imply the ability to write to /proc/pid/attr
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

John Johansen Dec. 19, 2016, 8:36 p.m. UTC | #15

On 12/19/2016 08:32 AM, Casey Schaufler wrote:
> On 12/19/2016 7:52 AM, Stephen Smalley wrote:
>> On Mon, 2016-12-19 at 16:41 +0100, José Bollo wrote:
>>> Le lundi 19 décembre 2016 à 09:33 -0500, Stephen Smalley a écrit :
>>>> On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
>>>>> Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen Smalley a
>>>>> écrit :
>>>>>>
>>>>>> Processes can only alter their own security attributes via
>>>>>> /proc/pid/attr nodes.  This is presently enforced by each
>>>>>> individual
>>>>>> security module and is also imposed by the Linux credentials
>>>>>> implementation, which only allows a task to alter its own
>>>>>> credentials.
>>>>>> Move the check enforcing this restriction from the individual
>>>>>> security modules to proc_pid_attr_write() before calling the
>>>>>> security
>>>>>> hook,
>>>>>> and drop the unnecessary task argument to the security hook
>>>>>> since
>>>>>> it
>>>>>> can
>>>>>> only ever be the current task.
>>>>> Hi Stephen,
>>>>>
>>>>> The module PTAGS that I made relies on the ability to write files
>>>>> in
>>>>> /proc/pid/attr/...
>>>>>
>>>>> Why did I used this files? Thaere are serious reasons:
>>>>>  - follows the life cycle of the process
>>>>>  - implements pid namespace
>>>>>
>>>>> There is absolutely no way to get these features easyly outside
>>>>> that
>>>>> implementation context of /proc/pid/attr/xxx.
>>>>>
>>>>> For these reasons I vote against that change.
>>>> It is already prohibited by the credentials framework;
>>> If it were REALLY prohibited, your patch shouldn't exist ;)
>> You can't do it safely (and your current implementation is unsafe).
>>  Please, read Documentation/security/credentials.txt in full, and look
>> at the setprocattr implementations in the upstream security modules for
>> reference.
> 
> I just reviewed the referenced documentation (thanks David)
> and it does clearly identify why, from the viewpoint of the
> credential implementation, modifying another task's credentials
> is unsafe. Both Smack and SELinux view modification of task
> credentials as unsafe from a security viewpoint, so neither of
> these modules would force the credential implementation to
> allow the behavior.
> 
> This, unfortunately, leaves Jose in a bit of a bind. It looks
> to me like there are options, so let's not just tell him to
> go away. The options I see include:
> 
>  - Fix the credential code so modifying another task is safe
> 	I don't advocate this approach because the existing
> 	implementation is only reasonable because of the
> 	restriction.
>  - Revert shared credentials
> 	I confess to not being a fan of shared credentials,
> 	for reason of complexity. I would not expect a
> 	proposal to go back to embedding credentials in
> 	their containing structures to be considered seriously.
> 	Nonetheless, it would solve the problem.
>  - Add a security blob for the task
> 	It is unfortunate that when shared credentials were
> 	introduced the task blob was not renamed a cred blob.
> 	It would be simple and consistent to add a security
> 	blob to the task in addition to the blob in the cred.
> 	I would add a t_security field to the task structure
> 	that associates security data with the task. This
> 	blob would be used for data that does not meet intent
> 	of a credential, which I believe may be the case for
> 	Jose's PTAGS.
> 
> I know which I would prefer, but as I expect someone to shout
> that none can possibly be acceptable I think I'll stop here and
> let the discussion go a while.
> 
> ("Light fuse. Stand back. :))
> 

Tetsuo has been asking for the return of the task security blob
for years. It does makes sense as not everything fits the cred
model, eg. seccomp rules aren't part of the cred, and I don't think
it makes sense for PTAGS either.

AppArmor could make use of it as well, as currently we abuse
the cred a little bit to store the change_hat, and setexeccon info.


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler Dec. 19, 2016, 9:25 p.m. UTC | #16

On 12/19/2016 12:36 PM, John Johansen wrote:
> On 12/19/2016 08:32 AM, Casey Schaufler wrote:
>> On 12/19/2016 7:52 AM, Stephen Smalley wrote:
>>> On Mon, 2016-12-19 at 16:41 +0100, José Bollo wrote:
>>>> Le lundi 19 décembre 2016 à 09:33 -0500, Stephen Smalley a écrit :
>>>>> On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
>>>>>> Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen Smalley a
>>>>>> écrit :
>>>>>>> Processes can only alter their own security attributes via
>>>>>>> /proc/pid/attr nodes.  This is presently enforced by each
>>>>>>> individual
>>>>>>> security module and is also imposed by the Linux credentials
>>>>>>> implementation, which only allows a task to alter its own
>>>>>>> credentials.
>>>>>>> Move the check enforcing this restriction from the individual
>>>>>>> security modules to proc_pid_attr_write() before calling the
>>>>>>> security
>>>>>>> hook,
>>>>>>> and drop the unnecessary task argument to the security hook
>>>>>>> since
>>>>>>> it
>>>>>>> can
>>>>>>> only ever be the current task.
>>>>>> Hi Stephen,
>>>>>>
>>>>>> The module PTAGS that I made relies on the ability to write files
>>>>>> in
>>>>>> /proc/pid/attr/...
>>>>>>
>>>>>> Why did I used this files? Thaere are serious reasons:
>>>>>>  - follows the life cycle of the process
>>>>>>  - implements pid namespace
>>>>>>
>>>>>> There is absolutely no way to get these features easyly outside
>>>>>> that
>>>>>> implementation context of /proc/pid/attr/xxx.
>>>>>>
>>>>>> For these reasons I vote against that change.
>>>>> It is already prohibited by the credentials framework;
>>>> If it were REALLY prohibited, your patch shouldn't exist ;)
>>> You can't do it safely (and your current implementation is unsafe).
>>>  Please, read Documentation/security/credentials.txt in full, and look
>>> at the setprocattr implementations in the upstream security modules for
>>> reference.
>> I just reviewed the referenced documentation (thanks David)
>> and it does clearly identify why, from the viewpoint of the
>> credential implementation, modifying another task's credentials
>> is unsafe. Both Smack and SELinux view modification of task
>> credentials as unsafe from a security viewpoint, so neither of
>> these modules would force the credential implementation to
>> allow the behavior.
>>
>> This, unfortunately, leaves Jose in a bit of a bind. It looks
>> to me like there are options, so let's not just tell him to
>> go away. The options I see include:
>>
>>  - Fix the credential code so modifying another task is safe
>> 	I don't advocate this approach because the existing
>> 	implementation is only reasonable because of the
>> 	restriction.
>>  - Revert shared credentials
>> 	I confess to not being a fan of shared credentials,
>> 	for reason of complexity. I would not expect a
>> 	proposal to go back to embedding credentials in
>> 	their containing structures to be considered seriously.
>> 	Nonetheless, it would solve the problem.
>>  - Add a security blob for the task
>> 	It is unfortunate that when shared credentials were
>> 	introduced the task blob was not renamed a cred blob.
>> 	It would be simple and consistent to add a security
>> 	blob to the task in addition to the blob in the cred.
>> 	I would add a t_security field to the task structure
>> 	that associates security data with the task. This
>> 	blob would be used for data that does not meet intent
>> 	of a credential, which I believe may be the case for
>> 	Jose's PTAGS.
>>
>> I know which I would prefer, but as I expect someone to shout
>> that none can possibly be acceptable I think I'll stop here and
>> let the discussion go a while.
>>
>> ("Light fuse. Stand back. :))
>>
> Tetsuo has been asking for the return of the task security blob
> for years. It does makes sense as not everything fits the cred
> model, eg. seccomp rules aren't part of the cred, and I don't think
> it makes sense for PTAGS either.
>
> AppArmor could make use of it as well, as currently we abuse
> the cred a little bit to store the change_hat, and setexeccon info.

A brief look at the existing modules leads me to believe that
everyone ought to be happier if we moved the LSM task blob out
of the cred structure. SELinux keeps a small (6xu32) task blob
that has no reason to share and refcount. Smack has a couple of
lists in the task blob that really shouldn't be in the cred.
There would have to be some rework of the task_alloc and task_free
hooks to get the life of the blobs correct, but I think on the
whole it would be lots cleaner.


> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tetsuo Handa Dec. 19, 2016, 9:46 p.m. UTC | #17

Casey Schaufler wrote:
> On 12/19/2016 12:36 PM, John Johansen wrote:
> > On 12/19/2016 08:32 AM, Casey Schaufler wrote:
> >> This, unfortunately, leaves Jose in a bit of a bind. It looks
> >> to me like there are options, so let's not just tell him to
> >> go away. The options I see include:
> >>
> >>  - Fix the credential code so modifying another task is safe
> >> 	I don't advocate this approach because the existing
> >> 	implementation is only reasonable because of the
> >> 	restriction.
> >>  - Revert shared credentials
> >> 	I confess to not being a fan of shared credentials,
> >> 	for reason of complexity. I would not expect a
> >> 	proposal to go back to embedding credentials in
> >> 	their containing structures to be considered seriously.
> >> 	Nonetheless, it would solve the problem.
> >>  - Add a security blob for the task
> >> 	It is unfortunate that when shared credentials were
> >> 	introduced the task blob was not renamed a cred blob.
> >> 	It would be simple and consistent to add a security
> >> 	blob to the task in addition to the blob in the cred.
> >> 	I would add a t_security field to the task structure
> >> 	that associates security data with the task. This
> >> 	blob would be used for data that does not meet intent
> >> 	of a credential, which I believe may be the case for
> >> 	Jose's PTAGS.
> >>
> >> I know which I would prefer, but as I expect someone to shout
> >> that none can possibly be acceptable I think I'll stop here and
> >> let the discussion go a while.
> >>
> >> ("Light fuse. Stand back. :))
> >>
> > Tetsuo has been asking for the return of the task security blob
> > for years. It does makes sense as not everything fits the cred
> > model, eg. seccomp rules aren't part of the cred, and I don't think
> > it makes sense for PTAGS either.
> >
> > AppArmor could make use of it as well, as currently we abuse
> > the cred a little bit to store the change_hat, and setexeccon info.
> 
> A brief look at the existing modules leads me to believe that
> everyone ought to be happier if we moved the LSM task blob out
> of the cred structure. SELinux keeps a small (6xu32) task blob
> that has no reason to share and refcount. Smack has a couple of
> lists in the task blob that really shouldn't be in the cred.
> There would have to be some rework of the task_alloc and task_free
> hooks to get the life of the blobs correct, but I think on the
> whole it would be lots cleaner.

Yes, I appreciate if the task security blob is revived.
According to TOMOYO's security context definition, use of per
task_struct attribute management (i.e. no cred override mechanism)
is correct. Currently I'm using very tricky implementation in AKARI
in order to emulate per task_struct attribute management.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Smalley Dec. 19, 2016, 9:50 p.m. UTC | #18

On Mon, 2016-12-19 at 13:25 -0800, Casey Schaufler wrote:
> On 12/19/2016 12:36 PM, John Johansen wrote:
> > 
> > On 12/19/2016 08:32 AM, Casey Schaufler wrote:
> > > 
> > > On 12/19/2016 7:52 AM, Stephen Smalley wrote:
> > > > 
> > > > On Mon, 2016-12-19 at 16:41 +0100, José Bollo wrote:
> > > > > 
> > > > > Le lundi 19 décembre 2016 à 09:33 -0500, Stephen Smalley a
> > > > > écrit :
> > > > > > 
> > > > > > On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
> > > > > > > 
> > > > > > > Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen
> > > > > > > Smalley a
> > > > > > > écrit :
> > > > > > > > 
> > > > > > > > Processes can only alter their own security attributes
> > > > > > > > via
> > > > > > > > /proc/pid/attr nodes.  This is presently enforced by
> > > > > > > > each
> > > > > > > > individual
> > > > > > > > security module and is also imposed by the Linux
> > > > > > > > credentials
> > > > > > > > implementation, which only allows a task to alter its
> > > > > > > > own
> > > > > > > > credentials.
> > > > > > > > Move the check enforcing this restriction from the
> > > > > > > > individual
> > > > > > > > security modules to proc_pid_attr_write() before
> > > > > > > > calling the
> > > > > > > > security
> > > > > > > > hook,
> > > > > > > > and drop the unnecessary task argument to the security
> > > > > > > > hook
> > > > > > > > since
> > > > > > > > it
> > > > > > > > can
> > > > > > > > only ever be the current task.
> > > > > > > Hi Stephen,
> > > > > > > 
> > > > > > > The module PTAGS that I made relies on the ability to
> > > > > > > write files
> > > > > > > in
> > > > > > > /proc/pid/attr/...
> > > > > > > 
> > > > > > > Why did I used this files? Thaere are serious reasons:
> > > > > > >  - follows the life cycle of the process
> > > > > > >  - implements pid namespace
> > > > > > > 
> > > > > > > There is absolutely no way to get these features easyly
> > > > > > > outside
> > > > > > > that
> > > > > > > implementation context of /proc/pid/attr/xxx.
> > > > > > > 
> > > > > > > For these reasons I vote against that change.
> > > > > > It is already prohibited by the credentials framework;
> > > > > If it were REALLY prohibited, your patch shouldn't exist ;)
> > > > You can't do it safely (and your current implementation is
> > > > unsafe).
> > > >  Please, read Documentation/security/credentials.txt in full,
> > > > and look
> > > > at the setprocattr implementations in the upstream security
> > > > modules for
> > > > reference.
> > > I just reviewed the referenced documentation (thanks David)
> > > and it does clearly identify why, from the viewpoint of the
> > > credential implementation, modifying another task's credentials
> > > is unsafe. Both Smack and SELinux view modification of task
> > > credentials as unsafe from a security viewpoint, so neither of
> > > these modules would force the credential implementation to
> > > allow the behavior.
> > > 
> > > This, unfortunately, leaves Jose in a bit of a bind. It looks
> > > to me like there are options, so let's not just tell him to
> > > go away. The options I see include:
> > > 
> > >  - Fix the credential code so modifying another task is safe
> > > 	I don't advocate this approach because the existing
> > > 	implementation is only reasonable because of the
> > > 	restriction.
> > >  - Revert shared credentials
> > > 	I confess to not being a fan of shared credentials,
> > > 	for reason of complexity. I would not expect a
> > > 	proposal to go back to embedding credentials in
> > > 	their containing structures to be considered seriously.
> > > 	Nonetheless, it would solve the problem.
> > >  - Add a security blob for the task
> > > 	It is unfortunate that when shared credentials were
> > > 	introduced the task blob was not renamed a cred blob.
> > > 	It would be simple and consistent to add a security
> > > 	blob to the task in addition to the blob in the cred.
> > > 	I would add a t_security field to the task structure
> > > 	that associates security data with the task. This
> > > 	blob would be used for data that does not meet intent
> > > 	of a credential, which I believe may be the case for
> > > 	Jose's PTAGS.
> > > 
> > > I know which I would prefer, but as I expect someone to shout
> > > that none can possibly be acceptable I think I'll stop here and
> > > let the discussion go a while.
> > > 
> > > ("Light fuse. Stand back. :))
> > > 
> > Tetsuo has been asking for the return of the task security blob
> > for years. It does makes sense as not everything fits the cred
> > model, eg. seccomp rules aren't part of the cred, and I don't think
> > it makes sense for PTAGS either.
> > 
> > AppArmor could make use of it as well, as currently we abuse
> > the cred a little bit to store the change_hat, and setexeccon info.
> 
> A brief look at the existing modules leads me to believe that
> everyone ought to be happier if we moved the LSM task blob out
> of the cred structure. SELinux keeps a small (6xu32) task blob
> that has no reason to share and refcount. Smack has a couple of
> lists in the task blob that really shouldn't be in the cred.
> There would have to be some rework of the task_alloc and task_free
> hooks to get the life of the blobs correct, but I think on the
> whole it would be lots cleaner.

Moving everything back to per-task security blob is inadvisable; the
kernel now passes around cred structures, including to some of the
security hooks, and you don't always want to check the credentials/sid
of the current task (that's one of the reasons creds were introduced).

You could perhaps move the SIDs other than the current one (e.g.
exec_sid, create_sid, ...) to the per-task security blob without
harm/confusion, but it is unclear what you gain from doing so; then we
would need to look in two places to find all the information we need
for certain hooks rather than one.  And certainly we'd end up consuming
more memory that way.

No, I don't think we want SELinux changed in this regard.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler Dec. 19, 2016, 10:31 p.m. UTC | #19

On 12/19/2016 1:50 PM, Stephen Smalley wrote:
> On Mon, 2016-12-19 at 13:25 -0800, Casey Schaufler wrote:
>> On 12/19/2016 12:36 PM, John Johansen wrote:
>>> On 12/19/2016 08:32 AM, Casey Schaufler wrote:
>>>> On 12/19/2016 7:52 AM, Stephen Smalley wrote:
>>>>> On Mon, 2016-12-19 at 16:41 +0100, José Bollo wrote:
>>>>>> Le lundi 19 décembre 2016 à 09:33 -0500, Stephen Smalley a
>>>>>> écrit :
>>>>>>> On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
>>>>>>>> Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen
>>>>>>>> Smalley a
>>>>>>>> écrit :
>>>>>>>>> Processes can only alter their own security attributes
>>>>>>>>> via
>>>>>>>>> /proc/pid/attr nodes.  This is presently enforced by
>>>>>>>>> each
>>>>>>>>> individual
>>>>>>>>> security module and is also imposed by the Linux
>>>>>>>>> credentials
>>>>>>>>> implementation, which only allows a task to alter its
>>>>>>>>> own
>>>>>>>>> credentials.
>>>>>>>>> Move the check enforcing this restriction from the
>>>>>>>>> individual
>>>>>>>>> security modules to proc_pid_attr_write() before
>>>>>>>>> calling the
>>>>>>>>> security
>>>>>>>>> hook,
>>>>>>>>> and drop the unnecessary task argument to the security
>>>>>>>>> hook
>>>>>>>>> since
>>>>>>>>> it
>>>>>>>>> can
>>>>>>>>> only ever be the current task.
>>>>>>>> Hi Stephen,
>>>>>>>>
>>>>>>>> The module PTAGS that I made relies on the ability to
>>>>>>>> write files
>>>>>>>> in
>>>>>>>> /proc/pid/attr/...
>>>>>>>>
>>>>>>>> Why did I used this files? Thaere are serious reasons:
>>>>>>>>  - follows the life cycle of the process
>>>>>>>>  - implements pid namespace
>>>>>>>>
>>>>>>>> There is absolutely no way to get these features easyly
>>>>>>>> outside
>>>>>>>> that
>>>>>>>> implementation context of /proc/pid/attr/xxx.
>>>>>>>>
>>>>>>>> For these reasons I vote against that change.
>>>>>>> It is already prohibited by the credentials framework;
>>>>>> If it were REALLY prohibited, your patch shouldn't exist ;)
>>>>> You can't do it safely (and your current implementation is
>>>>> unsafe).
>>>>>  Please, read Documentation/security/credentials.txt in full,
>>>>> and look
>>>>> at the setprocattr implementations in the upstream security
>>>>> modules for
>>>>> reference.
>>>> I just reviewed the referenced documentation (thanks David)
>>>> and it does clearly identify why, from the viewpoint of the
>>>> credential implementation, modifying another task's credentials
>>>> is unsafe. Both Smack and SELinux view modification of task
>>>> credentials as unsafe from a security viewpoint, so neither of
>>>> these modules would force the credential implementation to
>>>> allow the behavior.
>>>>
>>>> This, unfortunately, leaves Jose in a bit of a bind. It looks
>>>> to me like there are options, so let's not just tell him to
>>>> go away. The options I see include:
>>>>
>>>>  - Fix the credential code so modifying another task is safe
>>>> 	I don't advocate this approach because the existing
>>>> 	implementation is only reasonable because of the
>>>> 	restriction.
>>>>  - Revert shared credentials
>>>> 	I confess to not being a fan of shared credentials,
>>>> 	for reason of complexity. I would not expect a
>>>> 	proposal to go back to embedding credentials in
>>>> 	their containing structures to be considered seriously.
>>>> 	Nonetheless, it would solve the problem.
>>>>  - Add a security blob for the task
>>>> 	It is unfortunate that when shared credentials were
>>>> 	introduced the task blob was not renamed a cred blob.
>>>> 	It would be simple and consistent to add a security
>>>> 	blob to the task in addition to the blob in the cred.
>>>> 	I would add a t_security field to the task structure
>>>> 	that associates security data with the task. This
>>>> 	blob would be used for data that does not meet intent
>>>> 	of a credential, which I believe may be the case for
>>>> 	Jose's PTAGS.
>>>>
>>>> I know which I would prefer, but as I expect someone to shout
>>>> that none can possibly be acceptable I think I'll stop here and
>>>> let the discussion go a while.
>>>>
>>>> ("Light fuse. Stand back. :))
>>>>
>>> Tetsuo has been asking for the return of the task security blob
>>> for years. It does makes sense as not everything fits the cred
>>> model, eg. seccomp rules aren't part of the cred, and I don't think
>>> it makes sense for PTAGS either.
>>>
>>> AppArmor could make use of it as well, as currently we abuse
>>> the cred a little bit to store the change_hat, and setexeccon info.
>> A brief look at the existing modules leads me to believe that
>> everyone ought to be happier if we moved the LSM task blob out
>> of the cred structure. SELinux keeps a small (6xu32) task blob
>> that has no reason to share and refcount. Smack has a couple of
>> lists in the task blob that really shouldn't be in the cred.
>> There would have to be some rework of the task_alloc and task_free
>> hooks to get the life of the blobs correct, but I think on the
>> whole it would be lots cleaner.
> Moving everything back to per-task security blob is inadvisable; the
> kernel now passes around cred structures,

No, it passes around pointers to cred structures.

>  including to some of the security hooks,

Most of which are related to the shared, copy on write
nature of credentials. From a little bit of poking about
it seems that in general if you have access to cred->security
you're going to have access to task->security just as
readily.

> and you don't always want to check the credentials/sid
> of the current task (that's one of the reasons creds were introduced).

Who said anything about the current task?
Sure, you've got outliers like dentry_create_files_as,
but there's no reason they can't be corrected.

> You could perhaps move the SIDs other than the current one (e.g.
> exec_sid, create_sid, ...) to the per-task security blob without
> harm/confusion, but it is unclear what you gain from doing so;

SELinux, I'm sure to noone's surprise, is the best match for the
current implementation. Even at that, I don't see that the change
would have real impact beyond the regrettable code churn.

> then we
> would need to look in two places to find all the information we need
> for certain hooks rather than one.  And certainly we'd end up consuming
> more memory that way.

Which is one reason why I didn't and wouldn't suggest it.

> No, I don't think we want SELinux changed in this regard.

I'm looking at what would be best for the LSM infrastructure.
So far what I hear is that the proposed change would be better
for three of the four existing "major" modules and required for
two modules that are out, but targeting mainline. Moving the
complete infrastructure from cred->security to task->security
(NOT the hybrid you've suggested) appears generally beneficial.
There does not appear to be harmful to SELinux. In fact, I suspect
you'll see a minor performance improvement out of the deal.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

John Johansen Dec. 19, 2016, 10:45 p.m. UTC | #20

On 12/19/2016 01:50 PM, Stephen Smalley wrote:
> On Mon, 2016-12-19 at 13:25 -0800, Casey Schaufler wrote:
>> On 12/19/2016 12:36 PM, John Johansen wrote:
>>>
>>> On 12/19/2016 08:32 AM, Casey Schaufler wrote:
>>>>
>>>> On 12/19/2016 7:52 AM, Stephen Smalley wrote:
>>>>>
>>>>> On Mon, 2016-12-19 at 16:41 +0100, José Bollo wrote:
>>>>>>
>>>>>> Le lundi 19 décembre 2016 à 09:33 -0500, Stephen Smalley a
>>>>>> écrit :
>>>>>>>
>>>>>>> On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
>>>>>>>>
>>>>>>>> Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen
>>>>>>>> Smalley a
>>>>>>>> écrit :
>>>>>>>>>
>>>>>>>>> Processes can only alter their own security attributes
>>>>>>>>> via
>>>>>>>>> /proc/pid/attr nodes.  This is presently enforced by
>>>>>>>>> each
>>>>>>>>> individual
>>>>>>>>> security module and is also imposed by the Linux
>>>>>>>>> credentials
>>>>>>>>> implementation, which only allows a task to alter its
>>>>>>>>> own
>>>>>>>>> credentials.
>>>>>>>>> Move the check enforcing this restriction from the
>>>>>>>>> individual
>>>>>>>>> security modules to proc_pid_attr_write() before
>>>>>>>>> calling the
>>>>>>>>> security
>>>>>>>>> hook,
>>>>>>>>> and drop the unnecessary task argument to the security
>>>>>>>>> hook
>>>>>>>>> since
>>>>>>>>> it
>>>>>>>>> can
>>>>>>>>> only ever be the current task.
>>>>>>>> Hi Stephen,
>>>>>>>>
>>>>>>>> The module PTAGS that I made relies on the ability to
>>>>>>>> write files
>>>>>>>> in
>>>>>>>> /proc/pid/attr/...
>>>>>>>>
>>>>>>>> Why did I used this files? Thaere are serious reasons:
>>>>>>>>  - follows the life cycle of the process
>>>>>>>>  - implements pid namespace
>>>>>>>>
>>>>>>>> There is absolutely no way to get these features easyly
>>>>>>>> outside
>>>>>>>> that
>>>>>>>> implementation context of /proc/pid/attr/xxx.
>>>>>>>>
>>>>>>>> For these reasons I vote against that change.
>>>>>>> It is already prohibited by the credentials framework;
>>>>>> If it were REALLY prohibited, your patch shouldn't exist ;)
>>>>> You can't do it safely (and your current implementation is
>>>>> unsafe).
>>>>>  Please, read Documentation/security/credentials.txt in full,
>>>>> and look
>>>>> at the setprocattr implementations in the upstream security
>>>>> modules for
>>>>> reference.
>>>> I just reviewed the referenced documentation (thanks David)
>>>> and it does clearly identify why, from the viewpoint of the
>>>> credential implementation, modifying another task's credentials
>>>> is unsafe. Both Smack and SELinux view modification of task
>>>> credentials as unsafe from a security viewpoint, so neither of
>>>> these modules would force the credential implementation to
>>>> allow the behavior.
>>>>
>>>> This, unfortunately, leaves Jose in a bit of a bind. It looks
>>>> to me like there are options, so let's not just tell him to
>>>> go away. The options I see include:
>>>>
>>>>  - Fix the credential code so modifying another task is safe
>>>> 	I don't advocate this approach because the existing
>>>> 	implementation is only reasonable because of the
>>>> 	restriction.
>>>>  - Revert shared credentials
>>>> 	I confess to not being a fan of shared credentials,
>>>> 	for reason of complexity. I would not expect a
>>>> 	proposal to go back to embedding credentials in
>>>> 	their containing structures to be considered seriously.
>>>> 	Nonetheless, it would solve the problem.
>>>>  - Add a security blob for the task
>>>> 	It is unfortunate that when shared credentials were
>>>> 	introduced the task blob was not renamed a cred blob.
>>>> 	It would be simple and consistent to add a security
>>>> 	blob to the task in addition to the blob in the cred.
>>>> 	I would add a t_security field to the task structure
>>>> 	that associates security data with the task. This
>>>> 	blob would be used for data that does not meet intent
>>>> 	of a credential, which I believe may be the case for
>>>> 	Jose's PTAGS.
>>>>
>>>> I know which I would prefer, but as I expect someone to shout
>>>> that none can possibly be acceptable I think I'll stop here and
>>>> let the discussion go a while.
>>>>
>>>> ("Light fuse. Stand back. :))
>>>>
>>> Tetsuo has been asking for the return of the task security blob
>>> for years. It does makes sense as not everything fits the cred
>>> model, eg. seccomp rules aren't part of the cred, and I don't think
>>> it makes sense for PTAGS either.
>>>
>>> AppArmor could make use of it as well, as currently we abuse
>>> the cred a little bit to store the change_hat, and setexeccon info.
>>
>> A brief look at the existing modules leads me to believe that
>> everyone ought to be happier if we moved the LSM task blob out
>> of the cred structure. SELinux keeps a small (6xu32) task blob
>> that has no reason to share and refcount. Smack has a couple of
>> lists in the task blob that really shouldn't be in the cred.
>> There would have to be some rework of the task_alloc and task_free
>> hooks to get the life of the blobs correct, but I think on the
>> whole it would be lots cleaner.
> 
> Moving everything back to per-task security blob is inadvisable; the
> kernel now passes around cred structures, including to some of the
> security hooks, and you don't always want to check the credentials/sid
> of the current task (that's one of the reasons creds were introduced).
> 

Unwinding the the cred, security relationship would be interesting.
I'm not sure its even advisable, I would want to see a patch attempting
it before I would make a call either way. I'm not convinced that they
should be split but I do think it wouldn't hurt to have a t_security
field for the ancillary data, and other models could benefit from it.

> You could perhaps move the SIDs other than the current one (e.g.
> exec_sid, create_sid, ...) to the per-task security blob without
> harm/confusion, but it is unclear what you gain from doing so; then we
> would need to look in two places to find all the information we need
> for certain hooks rather than one.  And certainly we'd end up consuming
> more memory that way.
> 
In apparmor's case it would actually save memory as we could share the
cred more often (instead of making new creds with none cred related
info), and we wouldn't allocate the ancillary information unless it was
needed.

It would certainly introduce a little bit of extra overhead to access
the extra info when it is needed, but the overhead would be small for
apparmor's use cases.

> No, I don't think we want SELinux changed in this regard.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler Dec. 19, 2016, 10:49 p.m. UTC | #21

On 12/19/2016 2:45 PM, John Johansen wrote:
> On 12/19/2016 01:50 PM, Stephen Smalley wrote:
>> On Mon, 2016-12-19 at 13:25 -0800, Casey Schaufler wrote:
>>> On 12/19/2016 12:36 PM, John Johansen wrote:
>>>> On 12/19/2016 08:32 AM, Casey Schaufler wrote:
>>>>> On 12/19/2016 7:52 AM, Stephen Smalley wrote:
>>>>>> On Mon, 2016-12-19 at 16:41 +0100, José Bollo wrote:
>>>>>>> Le lundi 19 décembre 2016 à 09:33 -0500, Stephen Smalley a
>>>>>>> écrit :
>>>>>>>> On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
>>>>>>>>> Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen
>>>>>>>>> Smalley a
>>>>>>>>> écrit :
>>>>>>>>>> Processes can only alter their own security attributes
>>>>>>>>>> via
>>>>>>>>>> /proc/pid/attr nodes.  This is presently enforced by
>>>>>>>>>> each
>>>>>>>>>> individual
>>>>>>>>>> security module and is also imposed by the Linux
>>>>>>>>>> credentials
>>>>>>>>>> implementation, which only allows a task to alter its
>>>>>>>>>> own
>>>>>>>>>> credentials.
>>>>>>>>>> Move the check enforcing this restriction from the
>>>>>>>>>> individual
>>>>>>>>>> security modules to proc_pid_attr_write() before
>>>>>>>>>> calling the
>>>>>>>>>> security
>>>>>>>>>> hook,
>>>>>>>>>> and drop the unnecessary task argument to the security
>>>>>>>>>> hook
>>>>>>>>>> since
>>>>>>>>>> it
>>>>>>>>>> can
>>>>>>>>>> only ever be the current task.
>>>>>>>>> Hi Stephen,
>>>>>>>>>
>>>>>>>>> The module PTAGS that I made relies on the ability to
>>>>>>>>> write files
>>>>>>>>> in
>>>>>>>>> /proc/pid/attr/...
>>>>>>>>>
>>>>>>>>> Why did I used this files? Thaere are serious reasons:
>>>>>>>>>  - follows the life cycle of the process
>>>>>>>>>  - implements pid namespace
>>>>>>>>>
>>>>>>>>> There is absolutely no way to get these features easyly
>>>>>>>>> outside
>>>>>>>>> that
>>>>>>>>> implementation context of /proc/pid/attr/xxx.
>>>>>>>>>
>>>>>>>>> For these reasons I vote against that change.
>>>>>>>> It is already prohibited by the credentials framework;
>>>>>>> If it were REALLY prohibited, your patch shouldn't exist ;)
>>>>>> You can't do it safely (and your current implementation is
>>>>>> unsafe).
>>>>>>  Please, read Documentation/security/credentials.txt in full,
>>>>>> and look
>>>>>> at the setprocattr implementations in the upstream security
>>>>>> modules for
>>>>>> reference.
>>>>> I just reviewed the referenced documentation (thanks David)
>>>>> and it does clearly identify why, from the viewpoint of the
>>>>> credential implementation, modifying another task's credentials
>>>>> is unsafe. Both Smack and SELinux view modification of task
>>>>> credentials as unsafe from a security viewpoint, so neither of
>>>>> these modules would force the credential implementation to
>>>>> allow the behavior.
>>>>>
>>>>> This, unfortunately, leaves Jose in a bit of a bind. It looks
>>>>> to me like there are options, so let's not just tell him to
>>>>> go away. The options I see include:
>>>>>
>>>>>  - Fix the credential code so modifying another task is safe
>>>>> 	I don't advocate this approach because the existing
>>>>> 	implementation is only reasonable because of the
>>>>> 	restriction.
>>>>>  - Revert shared credentials
>>>>> 	I confess to not being a fan of shared credentials,
>>>>> 	for reason of complexity. I would not expect a
>>>>> 	proposal to go back to embedding credentials in
>>>>> 	their containing structures to be considered seriously.
>>>>> 	Nonetheless, it would solve the problem.
>>>>>  - Add a security blob for the task
>>>>> 	It is unfortunate that when shared credentials were
>>>>> 	introduced the task blob was not renamed a cred blob.
>>>>> 	It would be simple and consistent to add a security
>>>>> 	blob to the task in addition to the blob in the cred.
>>>>> 	I would add a t_security field to the task structure
>>>>> 	that associates security data with the task. This
>>>>> 	blob would be used for data that does not meet intent
>>>>> 	of a credential, which I believe may be the case for
>>>>> 	Jose's PTAGS.
>>>>>
>>>>> I know which I would prefer, but as I expect someone to shout
>>>>> that none can possibly be acceptable I think I'll stop here and
>>>>> let the discussion go a while.
>>>>>
>>>>> ("Light fuse. Stand back. :))
>>>>>
>>>> Tetsuo has been asking for the return of the task security blob
>>>> for years. It does makes sense as not everything fits the cred
>>>> model, eg. seccomp rules aren't part of the cred, and I don't think
>>>> it makes sense for PTAGS either.
>>>>
>>>> AppArmor could make use of it as well, as currently we abuse
>>>> the cred a little bit to store the change_hat, and setexeccon info.
>>> A brief look at the existing modules leads me to believe that
>>> everyone ought to be happier if we moved the LSM task blob out
>>> of the cred structure. SELinux keeps a small (6xu32) task blob
>>> that has no reason to share and refcount. Smack has a couple of
>>> lists in the task blob that really shouldn't be in the cred.
>>> There would have to be some rework of the task_alloc and task_free
>>> hooks to get the life of the blobs correct, but I think on the
>>> whole it would be lots cleaner.
>> Moving everything back to per-task security blob is inadvisable; the
>> kernel now passes around cred structures, including to some of the
>> security hooks, and you don't always want to check the credentials/sid
>> of the current task (that's one of the reasons creds were introduced).
>>
> Unwinding the the cred, security relationship would be interesting.
> I'm not sure its even advisable, I would want to see a patch attempting
> it before I would make a call either way. I'm not convinced that they
> should be split but I do think it wouldn't hurt to have a t_security
> field for the ancillary data, and other models could benefit from it.
>
>> You could perhaps move the SIDs other than the current one (e.g.
>> exec_sid, create_sid, ...) to the per-task security blob without
>> harm/confusion, but it is unclear what you gain from doing so; then we
>> would need to look in two places to find all the information we need
>> for certain hooks rather than one.  And certainly we'd end up consuming
>> more memory that way.
>>
> In apparmor's case it would actually save memory as we could share the
> cred more often (instead of making new creds with none cred related
> info), and we wouldn't allocate the ancillary information unless it was
> needed.
>
> It would certainly introduce a little bit of extra overhead to access
> the extra info when it is needed, but the overhead would be small for
> apparmor's use cases.
>
>> No, I don't think we want SELinux changed in this regard.
>>
>
I will also point out that SELinux and Smack predate the credential
implementation. They worked before with a blob directly off of
the task structure.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paul Moore Dec. 20, 2016, 1:23 a.m. UTC | #22

On Mon, Dec 19, 2016 at 4:50 PM, Stephen Smalley <[e-mail]> wrote:
> On Mon, 2016-12-19 at 13:25 -0800, Casey Schaufler wrote:
>> A brief look at the existing modules leads me to believe that
>> everyone ought to be happier if we moved the LSM task blob out
>> of the cred structure. SELinux keeps a small (6xu32) task blob
>> that has no reason to share and refcount. Smack has a couple of
>> lists in the task blob that really shouldn't be in the cred.
>> There would have to be some rework of the task_alloc and task_free
>> hooks to get the life of the blobs correct, but I think on the
>> whole it would be lots cleaner.
>
> Moving everything back to per-task security blob is inadvisable; the
> kernel now passes around cred structures, including to some of the
> security hooks, and you don't always want to check the credentials/sid
> of the current task (that's one of the reasons creds were introduced).

Casey, if you haven't looked at some of the stuff that we did recently
in SELinux for overlayfs, it might be a good idea to look at that,
especially with the world hurtling towards containers with wild
abandon.  I can't say I know exactly how it will affect Smack, but my
guess is that you will end up with something very similar to what we
do in SELinux.

> You could perhaps move the SIDs other than the current one (e.g.
> exec_sid, create_sid, ...) to the per-task security blob without
> harm/confusion, but it is unclear what you gain from doing so; then we
> would need to look in two places to find all the information we need
> for certain hooks rather than one.  And certainly we'd end up consuming
> more memory that way.

No thank you.

> No, I don't think we want SELinux changed in this regard.

Agreed.  I have no object if other LSMs want to add a security blob to
the task struct, I just don't want to see it replace the cred blobs.

Paul Moore Dec. 20, 2016, 1:27 a.m. UTC | #23

On Mon, Dec 19, 2016 at 5:49 PM, Casey Schaufler <[e-mail]> wrote:
> I will also point out that SELinux and Smack predate the credential
> implementation. They worked before with a blob directly off of
> the task structure.

It should also be noted that SELinux has added a some very tricky
functionality since the current credential implementation was adopted
and the changes to adopt that to a task based blob would not be
trivial.

Paul Moore Dec. 20, 2016, 1:30 a.m. UTC | #24

On Fri, Dec 16, 2016 at 5:13 PM, Casey Schaufler <[e-mail]> wrote:
> On 12/16/2016 2:06 PM, Paul Moore wrote:
>> On Fri, Dec 16, 2016 at 12:41 PM, Stephen Smalley <[e-mail]> wrote:
>>> Processes can only alter their own security attributes via
>>> /proc/pid/attr nodes.  This is presently enforced by each individual
>>> security module and is also imposed by the Linux credentials
>>> implementation, which only allows a task to alter its own credentials.
>>> Move the check enforcing this restriction from the individual
>>> security modules to proc_pid_attr_write() before calling the security hook,
>>> and drop the unnecessary task argument to the security hook since it can
>>> only ever be the current task.
>>>
>>> Signed-off-by: Stephen Smalley <[e-mail]>
>>> ---
>>>  fs/proc/base.c             | 13 +++++++++----
>>>  include/linux/lsm_hooks.h  |  3 +--
>>>  include/linux/security.h   |  4 ++--
>>>  security/apparmor/lsm.c    |  7 ++-----
>>>  security/security.c        |  4 ++--
>>>  security/selinux/hooks.c   | 13 +------------
>>>  security/smack/smack_lsm.c | 11 +----------
>>>  7 files changed, 18 insertions(+), 37 deletions(-)
>> Looks good to me.  I'm happy to pull this in via the SELinux tree
>> unless anyone else would rather take it?
>
> That works for me. It does need to go in atomically.

Even with all the discussion today, I still think this patch has value
and I don't believe it should impact the PTAGS work as there are
clearly larger issues that need to be resolved (e.g. reintroduce a
task based security blob?).  Unless I hear any objections that this
will wreck everything for years to come (very doubtful), I'll go ahead
and merge this into the SELinux tree tomorrow.

Casey Schaufler Dec. 20, 2016, 1:51 a.m. UTC | #25

On 12/19/2016 5:30 PM, Paul Moore wrote:
> On Fri, Dec 16, 2016 at 5:13 PM, Casey Schaufler <[e-mail]> wrote:
>> On 12/16/2016 2:06 PM, Paul Moore wrote:
>>> On Fri, Dec 16, 2016 at 12:41 PM, Stephen Smalley <[e-mail]> wrote:
>>>> Processes can only alter their own security attributes via
>>>> /proc/pid/attr nodes.  This is presently enforced by each individual
>>>> security module and is also imposed by the Linux credentials
>>>> implementation, which only allows a task to alter its own credentials.
>>>> Move the check enforcing this restriction from the individual
>>>> security modules to proc_pid_attr_write() before calling the security hook,
>>>> and drop the unnecessary task argument to the security hook since it can
>>>> only ever be the current task.
>>>>
>>>> Signed-off-by: Stephen Smalley <[e-mail]>
>>>> ---
>>>>  fs/proc/base.c             | 13 +++++++++----
>>>>  include/linux/lsm_hooks.h  |  3 +--
>>>>  include/linux/security.h   |  4 ++--
>>>>  security/apparmor/lsm.c    |  7 ++-----
>>>>  security/security.c        |  4 ++--
>>>>  security/selinux/hooks.c   | 13 +------------
>>>>  security/smack/smack_lsm.c | 11 +----------
>>>>  7 files changed, 18 insertions(+), 37 deletions(-)
>>> Looks good to me.  I'm happy to pull this in via the SELinux tree
>>> unless anyone else would rather take it?
>> That works for me. It does need to go in atomically.
> Even with all the discussion today, I still think this patch has value
> and I don't believe it should impact the PTAGS work as there are
> clearly larger issues that need to be resolved (e.g. reintroduce a
> task based security blob?).  Unless I hear any objections that this
> will wreck everything for years to come (very doubtful), I'll go ahead
> and merge this into the SELinux tree tomorrow.

I think this can go ahead without causing the end of western civilization.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler Dec. 20, 2016, 1:59 a.m. UTC | #26

On 12/19/2016 5:23 PM, Paul Moore wrote:
> On Mon, Dec 19, 2016 at 4:50 PM, Stephen Smalley <[e-mail]> wrote:
>> On Mon, 2016-12-19 at 13:25 -0800, Casey Schaufler wrote:
>>> A brief look at the existing modules leads me to believe that
>>> everyone ought to be happier if we moved the LSM task blob out
>>> of the cred structure. SELinux keeps a small (6xu32) task blob
>>> that has no reason to share and refcount. Smack has a couple of
>>> lists in the task blob that really shouldn't be in the cred.
>>> There would have to be some rework of the task_alloc and task_free
>>> hooks to get the life of the blobs correct, but I think on the
>>> whole it would be lots cleaner.
>> Moving everything back to per-task security blob is inadvisable; the
>> kernel now passes around cred structures, including to some of the
>> security hooks, and you don't always want to check the credentials/sid
>> of the current task (that's one of the reasons creds were introduced).
> Casey, if you haven't looked at some of the stuff that we did recently
> in SELinux for overlayfs, it might be a good idea to look at that,
> especially with the world hurtling towards containers with wild
> abandon.  I can't say I know exactly how it will affect Smack, but my
> guess is that you will end up with something very similar to what we
> do in SELinux.

We've got someone looking (verrry carefully) at overlayfs. It's ...
challenging. I wish that I had been able to track it more carefully
during implementation.


>> You could perhaps move the SIDs other than the current one (e.g.
>> exec_sid, create_sid, ...) to the per-task security blob without
>> harm/confusion, but it is unclear what you gain from doing so; then we
>> would need to look in two places to find all the information we need
>> for certain hooks rather than one.  And certainly we'd end up consuming
>> more memory that way.
> No thank you.

I don't think anyone would be happy with that.

>> No, I don't think we want SELinux changed in this regard.
> Agreed.  I have no object if other LSMs want to add a security blob to
> the task struct, I just don't want to see it replace the cred blobs.

OK, that actually make life easiest, for me at least. Jose, Tetsuo,
John, or anyone, want to propose a patch adding task->t_security and
the changes to a module to consume it? No changes to SELinux required.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

José Bollo Dec. 20, 2016, 10:36 a.m. UTC | #27

Le lundi 19 décembre 2016 à 20:30 -0500, Paul Moore a écrit :
> On Fri, Dec 16, 2016 at 5:13 PM, Casey Schaufler <casey@schaufler-ca.
> com> wrote:
> > On 12/16/2016 2:06 PM, Paul Moore wrote:
> > > On Fri, Dec 16, 2016 at 12:41 PM, Stephen Smalley <[e-mail].
> > > gov> wrote:
> > > > Processes can only alter their own security attributes via
> > > > /proc/pid/attr nodes.  This is presently enforced by each
> > > > individual
> > > > security module and is also imposed by the Linux credentials
> > > > implementation, which only allows a task to alter its own
> > > > credentials.
> > > > Move the check enforcing this restriction from the individual
> > > > security modules to proc_pid_attr_write() before calling the
> > > > security hook,
> > > > and drop the unnecessary task argument to the security hook
> > > > since it can
> > > > only ever be the current task.
> > > > 
> > > > Signed-off-by: Stephen Smalley <[e-mail]>
> > > > ---
> > > >  fs/proc/base.c             | 13 +++++++++----
> > > >  include/linux/lsm_hooks.h  |  3 +--
> > > >  include/linux/security.h   |  4 ++--
> > > >  security/apparmor/lsm.c    |  7 ++-----
> > > >  security/security.c        |  4 ++--
> > > >  security/selinux/hooks.c   | 13 +------------
> > > >  security/smack/smack_lsm.c | 11 +----------
> > > >  7 files changed, 18 insertions(+), 37 deletions(-)
> > > 
> > > Looks good to me.  I'm happy to pull this in via the SELinux tree
> > > unless anyone else would rather take it?
> > 
> > That works for me. It does need to go in atomically.
> 
> Even with all the discussion today, I still think this patch has
> value
> and I don't believe it should impact the PTAGS work as there are
> clearly larger issues that need to be resolved (e.g. reintroduce a
> task based security blob?).  Unless I hear any objections that this
> will wreck everything for years to come (very doubtful), I'll go
> ahead
> and merge this into the SELinux tree tomorrow.

Please explain first the relationship that you are supposing true:

    writing /proc/pid/attr/.. == writing creds


I'm feeling that there is here a weird logic.

I can agree that the use of creds that I made is wrong and must be
changed. But how can I agree that writing to /proc/pid/attr/... is not
good when for my purpose it is a valuable and working solution?

Best regards
José Bollo
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tetsuo Handa Dec. 20, 2016, 11:13 a.m. UTC | #28

Jose Bollo 
> Please explain first the relationship that you are supposing true:
> 
>     writing /proc/pid/attr/.. == writing creds
> 
> 
> I'm feeling that there is here a weird logic.
> 
> I can agree that the use of creds that I made is wrong and must be
> changed. But how can I agree that writing to /proc/pid/attr/... is not
> good when for my purpose it is a valuable and working solution?

Regarding TOMOYO, "writing creds" == "writing /sys/kernel/security/tomoyo/self_domain".
I didn't choose /proc/pid/attr/ interface from the beginning, for I wanted to allow
enabling multiple LSM modules concurrently and I thought existing userspace programs
will be confused when multiple concurrent LSM modules are fully supported.

You can check Casey's "LSM: module hierarchy in /proc/.../attr" patches at
http://lkml.kernel.org/r/[e-mail] .
Since PTAGS is not yet in-tree, I think it is less harder (in other words, now
is a chance) to reconsider the interface for PTAGS.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

José Bollo Dec. 20, 2016, 12:14 p.m. UTC | #29

Le mardi 20 décembre 2016 à 20:13 +0900, Tetsuo Handa a écrit :
> Jose Bollo 
> > Please explain first the relationship that you are supposing true:
> > 
> >     writing /proc/pid/attr/.. == writing creds
> > 
> > 
> > I'm feeling that there is here a weird logic.
> > 
> > I can agree that the use of creds that I made is wrong and must be
> > changed. But how can I agree that writing to /proc/pid/attr/... is
> > not
> > good when for my purpose it is a valuable and working solution?
> 
> Regarding TOMOYO, "writing creds" == "writing
> /sys/kernel/security/tomoyo/self_domain".

Hi Tetsuo,

It confirms what I wanted to point out, there is no real link between
writing creds and writing /proc/.../attr. But here it is for SELF so it
doesn't brakes the creds.

> I didn't choose /proc/pid/attr/ interface from the beginning, for I
> wanted to allow
> enabling multiple LSM modules concurrently and I thought existing
> userspace programs
> will be confused when multiple concurrent LSM modules are fully
> supported.
> 
> You can check Casey's "LSM: module hierarchy in /proc/.../attr"
> patches at
> http://lkml.kernel.org/r/00f80c77-9623-7e9e-8980-63b362a4f16c@schaufl
> er-ca.com .
> Since PTAGS is not yet in-tree, I think it is less harder (in other
> words, now
> is a chance) to reconsider the interface for PTAGS.

I agree that it can be a chance. You pointed out the limitations in
using existing /proc/.../attr implmentation and thank you for that.

But PTAGS is specifically bound to tasks.

So /proc/pid..., with its DAC and its MAC access control, with its
already existing implementation PID of namespace is the natural place
where control interface for PTAGS should occur.

I would really welcome proposal for other place but I guess that it
would be of lesser evidence and that the implmentation would be much
complicated.

I followed what Casey proposed and that you cite. It is a good proposal
in its idea. It makes things a little more complicated than what
already exist and I doubt a little that in a day (or a night ;) SELinux
 Smacks Tomoyo. However it would makes things cleaner: a command like
'id' would be easier to implement.

Best regards
José

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

José Bollo Dec. 20, 2016, 2:40 p.m. UTC | #30

Le lundi 19 décembre 2016 à 13:25 -0800, Casey Schaufler a écrit :

snip
> 
> A brief look at the existing modules leads me to believe that
> everyone ought to be happier if we moved the LSM task blob out
> of the cred structure. SELinux keeps a small (6xu32) task blob
> that has no reason to share and refcount. Smack has a couple of
> lists in the task blob that really shouldn't be in the cred.
> There would have to be some rework of the task_alloc and task_free
> hooks to get the life of the blobs correct, but I think on the
> whole it would be lots cleaner.
> 

Hi Casey,

Let suppose that creds is, in effect, the wrong place for implementing
PTAGS and let suppose that the correct way is to add a t_ptags element
in the task structure.

How to use task_alloc and task_free? There is nothing there. Is it just
possible to inherit or copy the parent attributes? no.

So there is a need to define a hook to copy or do something for the
child based on the parent. A new hook, a kind of 'task_child_init'
whose argument would be the task to init.

Is it it?

Best regards
José Bollo


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Smalley Dec. 20, 2016, 4:14 p.m. UTC | #31

On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote:
> Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen Smalley a écrit :
> > 
> > Processes can only alter their own security attributes via
> > /proc/pid/attr nodes.  This is presently enforced by each
> > individual
> > security module and is also imposed by the Linux credentials
> > implementation, which only allows a task to alter its own
> > credentials.
> > Move the check enforcing this restriction from the individual
> > security modules to proc_pid_attr_write() before calling the
> > security
> > hook,
> > and drop the unnecessary task argument to the security hook since
> > it
> > can
> > only ever be the current task.
> 
> Hi Stephen,
> 
> The module PTAGS that I made relies on the ability to write files in
> /proc/pid/attr/...
> 
> Why did I used this files? Thaere are serious reasons:
>  - follows the life cycle of the process
>  - implements pid namespace
> 
> There is absolutely no way to get these features easyly outside that
> implementation context of /proc/pid/attr/xxx.
> 
> For these reasons I vote against that change.
> 
> You will probably ask me where is PTAGS used and what is is made for.
> 
> So it is not used but the principle of its use is benefical to the
> community. It allows a kind of user land implementation of
> capabilities.
> 
> You probably know that the kernel can not do all things for a good
> reason: it is not intended to do all things. So there is a need for
> user land features. PTAGS is an intent to fulfill an empty gap.
> 
> I made a presentation at FOSDEM on the subject that may allows you to
> understand the beginning of the issue that I'm writing about [2]. 
> 
> [1] https://lkml.org/lkml/2016/11/23/122
> [2] https://archive.fosdem.org/2015/schedule/event/sec_enforcement/

Looking at your PTAGS implementation, I feel it is only fair to warn
you that your usage of /proc/pid/attr is insecure, regardless of
whether you use task security blobs or cred security blobs.

Getting the attributes of another process via /proc/pid files is
inherently racy, as the process may exit and another process with
different attributes may be created with the same pid (and no, this is
not theoretical; it has been demonstrated). Similarly, setting the
attributes of another process via /proc/pid files is likewise
inherently racy; you may end up setting the attributes on the wrong
process entirely.  Setting another process' attributes in this manner
is also prone to other kinds of races, since there is no coordination
between the process execution state and when the new tag is applied.

Again, I encourage you to reconsider your approach if you want to have
a secure solution.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler Dec. 20, 2016, 4:21 p.m. UTC | #32

On 12/20/2016 6:40 AM, José Bollo wrote:
> Le lundi 19 décembre 2016 à 13:25 -0800, Casey Schaufler a écrit :
>
> snip
>> A brief look at the existing modules leads me to believe that
>> everyone ought to be happier if we moved the LSM task blob out
>> of the cred structure. SELinux keeps a small (6xu32) task blob
>> that has no reason to share and refcount. Smack has a couple of
>> lists in the task blob that really shouldn't be in the cred.
>> There would have to be some rework of the task_alloc and task_free
>> hooks to get the life of the blobs correct, but I think on the
>> whole it would be lots cleaner.
>>
> Hi Casey,
>
> Let suppose that creds is, in effect, the wrong place for implementing
> PTAGS and let suppose that the correct way is to add a t_ptags element
> in the task structure.
>
> How to use task_alloc and task_free? There is nothing there. Is it just
> possible to inherit or copy the parent attributes? no.
>
> So there is a need to define a hook to copy or do something for the
> child based on the parent. A new hook, a kind of 'task_child_init'
> whose argument would be the task to init.
>
> Is it it?
>
> Best regards
> José Bollo

My suggestion at this point is that we introduce a "t_security"
field to task_struct (in include/linux/sched.h) under #CONFIG_SECURITY.
There is already a task_free hook that should suffice, but the existing
task_create hook will either need to be changed to get passed the
task_struct or supplemented with a task_alloc hook. A copy_task hook
may prove necessary, but we'll have to look into that. Security module
data that belongs associated with the task needs to get moved, but that
can be done by the module maintainers. It doesn't sound like there
would be resistance to this from the security side of the house, but
it needs to get run past the task management maintainers. A patch set,
used by an existing module would be the best approach for that. I
do not think that Smack is a good example as it is the one module that
would probably be best served by having some data in both places.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

José Bollo Dec. 20, 2016, 4:39 p.m. UTC | #33

Le mardi 20 décembre 2016 à 11:14 -0500, Stephen Smalley a écrit :
> 
> Looking at your PTAGS implementation, I feel it is only fair to warn
> you that your usage of /proc/pid/attr is insecure, regardless of
> whether you use task security blobs or cred security blobs.

Fair?!

> Getting the attributes of another process via /proc/pid files is
> inherently racy, as the process may exit and another process with
> different attributes may be created with the same pid (and no, this
> is not theoretical; it has been demonstrated).

I know. And I'm surprized that you dont do anything to change that.

> Similarly, setting the
> attributes of another process via /proc/pid files is likewise
> inherently racy; you may end up setting the attributes on the wrong
> process entirely.

I also know that.

> Setting another process' attributes in this manner
> is also prone to other kinds of races, since there is no coordination
> between the process execution state and when the new tag is applied.

Yes it is expected.

> Again, I encourage you to reconsider your approach if you want to
> have a secure solution.

Well I know that managing processes is not secure because there is no
kind of unique id. But please instead of thinking that it is to risky,
please hear that some risks are manageable or acceptable.

Best regards
José
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Smalley Dec. 20, 2016, 4:50 p.m. UTC | #34

On Tue, 2016-12-20 at 17:39 +0100, José Bollo wrote:
> Le mardi 20 décembre 2016 à 11:14 -0500, Stephen Smalley a écrit :
> > 
> > 
> > Looking at your PTAGS implementation, I feel it is only fair to
> > warn
> > you that your usage of /proc/pid/attr is insecure, regardless of
> > whether you use task security blobs or cred security blobs.
> 
> Fair?!
> 
> > 
> > Getting the attributes of another process via /proc/pid files is
> > inherently racy, as the process may exit and another process with
> > different attributes may be created with the same pid (and no, this
> > is not theoretical; it has been demonstrated).
> 
> I know. And I'm surprized that you dont do anything to change that.

There is a reason why SO_PEERSEC and SCM_SECURITY exist.  Again, learn
from the upstream security modules rather than re-inventing them,
badly.

> 
> > 
> > Similarly, setting the
> > attributes of another process via /proc/pid files is likewise
> > inherently racy; you may end up setting the attributes on the wrong
> > process entirely.
> 
> I also know that.
> 
> > 
> > Setting another process' attributes in this manner
> > is also prone to other kinds of races, since there is no
> > coordination
> > between the process execution state and when the new tag is
> > applied.
> 
> Yes it is expected.
> 
> > 
> > Again, I encourage you to reconsider your approach if you want to
> > have a secure solution.
> 
> Well I know that managing processes is not secure because there is no
> kind of unique id. But please instead of thinking that it is to
> risky,
> please hear that some risks are manageable or acceptable.

Even when there are known, better ways of doing things?

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler Dec. 20, 2016, 6:17 p.m. UTC | #35

On 12/20/2016 8:50 AM, Stephen Smalley wrote:
> On Tue, 2016-12-20 at 17:39 +0100, José Bollo wrote:
>> Le mardi 20 décembre 2016 à 11:14 -0500, Stephen Smalley a écrit :
>>>
>>> Looking at your PTAGS implementation, I feel it is only fair to
>>> warn
>>> you that your usage of /proc/pid/attr is insecure, regardless of
>>> whether you use task security blobs or cred security blobs.
>> Fair?!
>>
>>> Getting the attributes of another process via /proc/pid files is
>>> inherently racy, as the process may exit and another process with
>>> different attributes may be created with the same pid (and no, this
>>> is not theoretical; it has been demonstrated).
>> I know. And I'm surprized that you dont do anything to change that.
> There is a reason why SO_PEERSEC and SCM_SECURITY exist.  Again, learn
> from the upstream security modules rather than re-inventing them,
> badly.

SO_PEERSEC and SCM_SECURITY are spiffy for processes that are
sending each other messages, but they identify the attributes
associated with the message, not the process. Neither SELinux
nor Smack get the information to send off of the process, it
comes from the socket structure.

>
>>> Similarly, setting the
>>> attributes of another process via /proc/pid files is likewise
>>> inherently racy; you may end up setting the attributes on the wrong
>>> process entirely.
>> I also know that.
>>
>>> Setting another process' attributes in this manner
>>> is also prone to other kinds of races, since there is no
>>> coordination
>>> between the process execution state and when the new tag is
>>> applied.
>> Yes it is expected.
>>
>>> Again, I encourage you to reconsider your approach if you want to
>>> have a secure solution.
>> Well I know that managing processes is not secure because there is no
>> kind of unique id. But please instead of thinking that it is to
>> risky,
>> please hear that some risks are manageable or acceptable.
> Even when there are known, better ways of doing things?


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Smalley Dec. 20, 2016, 6:28 p.m. UTC | #36

On Tue, 2016-12-20 at 10:17 -0800, Casey Schaufler wrote:
> On 12/20/2016 8:50 AM, Stephen Smalley wrote:
> > 
> > On Tue, 2016-12-20 at 17:39 +0100, José Bollo wrote:
> > > 
> > > Le mardi 20 décembre 2016 à 11:14 -0500, Stephen Smalley a écrit
> > > :
> > > > 
> > > > 
> > > > Looking at your PTAGS implementation, I feel it is only fair to
> > > > warn
> > > > you that your usage of /proc/pid/attr is insecure, regardless
> > > > of
> > > > whether you use task security blobs or cred security blobs.
> > > Fair?!
> > > 
> > > > 
> > > > Getting the attributes of another process via /proc/pid files
> > > > is
> > > > inherently racy, as the process may exit and another process
> > > > with
> > > > different attributes may be created with the same pid (and no,
> > > > this
> > > > is not theoretical; it has been demonstrated).
> > > I know. And I'm surprized that you dont do anything to change
> > > that.
> > There is a reason why SO_PEERSEC and SCM_SECURITY exist.  Again,
> > learn
> > from the upstream security modules rather than re-inventing them,
> > badly.
> 
> SO_PEERSEC and SCM_SECURITY are spiffy for processes that are
> sending each other messages, but they identify the attributes
> associated with the message, not the process. Neither SELinux
> nor Smack get the information to send off of the process, it
> comes from the socket structure.

Yes, but in the SELinux case at least, the socket is labeled with the
label of the creating process (except in the rare case of using
setsockcreatecon(3), which can only be used by suitably authorized
processes).  So in general it serves quite well as a means of obtaining
the peer label, which can then be used in access control (and this is
in fact being used in a variety of applications in Linux and Android).
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler Dec. 20, 2016, 7:07 p.m. UTC | #37

On 12/20/2016 10:28 AM, Stephen Smalley wrote:
> On Tue, 2016-12-20 at 10:17 -0800, Casey Schaufler wrote:
>> On 12/20/2016 8:50 AM, Stephen Smalley wrote:
>>> On Tue, 2016-12-20 at 17:39 +0100, José Bollo wrote:
>>>> Le mardi 20 décembre 2016 à 11:14 -0500, Stephen Smalley a écrit
>>>> :
>>>>>
>>>>> Looking at your PTAGS implementation, I feel it is only fair to
>>>>> warn
>>>>> you that your usage of /proc/pid/attr is insecure, regardless
>>>>> of
>>>>> whether you use task security blobs or cred security blobs.
>>>> Fair?!
>>>>
>>>>> Getting the attributes of another process via /proc/pid files
>>>>> is
>>>>> inherently racy, as the process may exit and another process
>>>>> with
>>>>> different attributes may be created with the same pid (and no,
>>>>> this
>>>>> is not theoretical; it has been demonstrated).
>>>> I know. And I'm surprized that you dont do anything to change
>>>> that.
>>> There is a reason why SO_PEERSEC and SCM_SECURITY exist.  Again,
>>> learn
>>> from the upstream security modules rather than re-inventing them,
>>> badly.
>> SO_PEERSEC and SCM_SECURITY are spiffy for processes that are
>> sending each other messages, but they identify the attributes
>> associated with the message, not the process. Neither SELinux
>> nor Smack get the information to send off of the process, it
>> comes from the socket structure.
> Yes, but in the SELinux case at least, the socket is labeled with the
> label of the creating process (except in the rare case of using
> setsockcreatecon(3), which can only be used by suitably authorized
> processes).

Yes, it's the same with Smack. When it's not the label
of the process it's the label the system wants the peer
to think it is.

>   So in general it serves quite well as a means of obtaining
> the peer label, which can then be used in access control (and this is
> in fact being used in a variety of applications in Linux and Android).

But only between processes that are in direct, explicit communication.
There is no denying that these mechanisms work. They just aren't
applicable to Jose's use.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Smalley Dec. 20, 2016, 7:35 p.m. UTC | #38

On Tue, 2016-12-20 at 11:07 -0800, Casey Schaufler wrote:
> On 12/20/2016 10:28 AM, Stephen Smalley wrote:
> > 
> > On Tue, 2016-12-20 at 10:17 -0800, Casey Schaufler wrote:
> > > 
> > > On 12/20/2016 8:50 AM, Stephen Smalley wrote:
> > > > 
> > > > On Tue, 2016-12-20 at 17:39 +0100, José Bollo wrote:
> > > > > 
> > > > > Le mardi 20 décembre 2016 à 11:14 -0500, Stephen Smalley a
> > > > > écrit
> > > > > :
> > > > > > 
> > > > > > 
> > > > > > Looking at your PTAGS implementation, I feel it is only
> > > > > > fair to
> > > > > > warn
> > > > > > you that your usage of /proc/pid/attr is insecure,
> > > > > > regardless
> > > > > > of
> > > > > > whether you use task security blobs or cred security blobs.
> > > > > Fair?!
> > > > > 
> > > > > > 
> > > > > > Getting the attributes of another process via /proc/pid
> > > > > > files
> > > > > > is
> > > > > > inherently racy, as the process may exit and another
> > > > > > process
> > > > > > with
> > > > > > different attributes may be created with the same pid (and
> > > > > > no,
> > > > > > this
> > > > > > is not theoretical; it has been demonstrated).
> > > > > I know. And I'm surprized that you dont do anything to change
> > > > > that.
> > > > There is a reason why SO_PEERSEC and SCM_SECURITY
> > > > exist.  Again,
> > > > learn
> > > > from the upstream security modules rather than re-inventing
> > > > them,
> > > > badly.
> > > SO_PEERSEC and SCM_SECURITY are spiffy for processes that are
> > > sending each other messages, but they identify the attributes
> > > associated with the message, not the process. Neither SELinux
> > > nor Smack get the information to send off of the process, it
> > > comes from the socket structure.
> > Yes, but in the SELinux case at least, the socket is labeled with
> > the
> > label of the creating process (except in the rare case of using
> > setsockcreatecon(3), which can only be used by suitably authorized
> > processes).
> 
> Yes, it's the same with Smack. When it's not the label
> of the process it's the label the system wants the peer
> to think it is.
> 
> > 
> >   So in general it serves quite well as a means of obtaining
> > the peer label, which can then be used in access control (and this
> > is
> > in fact being used in a variety of applications in Linux and
> > Android).
> 
> But only between processes that are in direct, explicit
> communication.
> There is no denying that these mechanisms work. They just aren't
> applicable to Jose's use.

If you say so (although it is unclear to me why or what exactly his use
case is), but regardless, there is also no denying that getting and
setting another process' attributes via /proc/pid files is inherently
racy.  He even acknowledged as much.  So we are left with a "security"
module whose only purpose is to support getting and setting process
tags for security enforcement purposes, and yet does so in a known-
insecure manner.  Again, this is why I keep suggesting that he needs to
reconsider his approach, not merely figure out how to implement per-
task security blobs.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler Dec. 20, 2016, 8:03 p.m. UTC | #39

On 12/20/2016 11:35 AM, Stephen Smalley wrote:
> On Tue, 2016-12-20 at 11:07 -0800, Casey Schaufler wrote:
>> On 12/20/2016 10:28 AM, Stephen Smalley wrote:
>>> On Tue, 2016-12-20 at 10:17 -0800, Casey Schaufler wrote:
>>>> On 12/20/2016 8:50 AM, Stephen Smalley wrote:
>>>>> On Tue, 2016-12-20 at 17:39 +0100, José Bollo wrote:
>>>>>> Le mardi 20 décembre 2016 à 11:14 -0500, Stephen Smalley a
>>>>>> écrit
>>>>>> :
>>>>>>>
>>>>>>> Looking at your PTAGS implementation, I feel it is only
>>>>>>> fair to
>>>>>>> warn
>>>>>>> you that your usage of /proc/pid/attr is insecure,
>>>>>>> regardless
>>>>>>> of
>>>>>>> whether you use task security blobs or cred security blobs.
>>>>>> Fair?!
>>>>>>
>>>>>>> Getting the attributes of another process via /proc/pid
>>>>>>> files
>>>>>>> is
>>>>>>> inherently racy, as the process may exit and another
>>>>>>> process
>>>>>>> with
>>>>>>> different attributes may be created with the same pid (and
>>>>>>> no,
>>>>>>> this
>>>>>>> is not theoretical; it has been demonstrated).
>>>>>> I know. And I'm surprized that you dont do anything to change
>>>>>> that.
>>>>> There is a reason why SO_PEERSEC and SCM_SECURITY
>>>>> exist.  Again,
>>>>> learn
>>>>> from the upstream security modules rather than re-inventing
>>>>> them,
>>>>> badly.
>>>> SO_PEERSEC and SCM_SECURITY are spiffy for processes that are
>>>> sending each other messages, but they identify the attributes
>>>> associated with the message, not the process. Neither SELinux
>>>> nor Smack get the information to send off of the process, it
>>>> comes from the socket structure.
>>> Yes, but in the SELinux case at least, the socket is labeled with
>>> the
>>> label of the creating process (except in the rare case of using
>>> setsockcreatecon(3), which can only be used by suitably authorized
>>> processes).
>> Yes, it's the same with Smack. When it's not the label
>> of the process it's the label the system wants the peer
>> to think it is.
>>
>>>   So in general it serves quite well as a means of obtaining
>>> the peer label, which can then be used in access control (and this
>>> is
>>> in fact being used in a variety of applications in Linux and
>>> Android).
>> But only between processes that are in direct, explicit
>> communication.
>> There is no denying that these mechanisms work. They just aren't
>> applicable to Jose's use.
> If you say so (although it is unclear to me why or what exactly his use
> case is), but regardless, there is also no denying that getting and
> setting another process' attributes via /proc/pid files is inherently
> racy.  

You're right. How can we fix that? I have seen a gazillion cases
where system security would be much simpler and easier to enforce
and develop if we could (safely) ensure that the process under
/proc/pid wouldn't change on you without you knowing.

> He even acknowledged as much.  So we are left with a "security"
> module whose only purpose is to support getting and setting process
> tags for security enforcement purposes, and yet does so in a known-
> insecure manner.  Again, this is why I keep suggesting that he needs to
> reconsider his approach, not merely figure out how to implement per-
> task security blobs.

Whether or not Jose moves forward with PTAGS we have identified
an issue with the current cred based hooks for AppArmor, TOMOYO
and at least one other proposed module. Regardless of the issues
of /proc/pid there is work to be done.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

José Bollo Dec. 20, 2016, 9:22 p.m. UTC | #40

Le mardi 20 décembre 2016 à 12:03 -0800, Casey Schaufler a écrit :
> On 12/20/2016 11:35 AM, Stephen Smalley wrote:
> > On Tue, 2016-12-20 at 11:07 -0800, Casey Schaufler wrote:
> > > On 12/20/2016 10:28 AM, Stephen Smalley wrote:
> > > > On Tue, 2016-12-20 at 10:17 -0800, Casey Schaufler wrote:
> > > > > On 12/20/2016 8:50 AM, Stephen Smalley wrote:
> > > > > > On Tue, 2016-12-20 at 17:39 +0100, José Bollo wrote:
> > > > > > > Le mardi 20 décembre 2016 à 11:14 -0500, Stephen Smalley
> > > > > > > a
> > > > > > > écrit
> > > > > > > :
> > > > > > > > 
> > > > > > > > Looking at your PTAGS implementation, I feel it is only
> > > > > > > > fair to
> > > > > > > > warn
> > > > > > > > you that your usage of /proc/pid/attr is insecure,
> > > > > > > > regardless
> > > > > > > > of
> > > > > > > > whether you use task security blobs or cred security
> > > > > > > > blobs.
> > > > > > > 
> > > > > > > Fair?!
> > > > > > > 
> > > > > > > > Getting the attributes of another process via /proc/pid
> > > > > > > > files
> > > > > > > > is
> > > > > > > > inherently racy, as the process may exit and another
> > > > > > > > process
> > > > > > > > with
> > > > > > > > different attributes may be created with the same pid
> > > > > > > > (and
> > > > > > > > no,
> > > > > > > > this
> > > > > > > > is not theoretical; it has been demonstrated).
> > > > > > > 
> > > > > > > I know. And I'm surprized that you dont do anything to
> > > > > > > change
> > > > > > > that.
> > > > > > 
> > > > > > There is a reason why SO_PEERSEC and SCM_SECURITY
> > > > > > exist.  Again,
> > > > > > learn
> > > > > > from the upstream security modules rather than re-inventing
> > > > > > them,
> > > > > > badly.
> > > > > 
> > > > > SO_PEERSEC and SCM_SECURITY are spiffy for processes that are
> > > > > sending each other messages, but they identify the attributes
> > > > > associated with the message, not the process. Neither SELinux
> > > > > nor Smack get the information to send off of the process, it
> > > > > comes from the socket structure.
> > > > 
> > > > Yes, but in the SELinux case at least, the socket is labeled
> > > > with
> > > > the
> > > > label of the creating process (except in the rare case of using
> > > > setsockcreatecon(3), which can only be used by suitably
> > > > authorized
> > > > processes).
> > > 
> > > Yes, it's the same with Smack. When it's not the label
> > > of the process it's the label the system wants the peer
> > > to think it is.
> > > 
> > > >   So in general it serves quite well as a means of obtaining
> > > > the peer label, which can then be used in access control (and
> > > > this
> > > > is
> > > > in fact being used in a variety of applications in Linux and
> > > > Android).
> > > 
> > > But only between processes that are in direct, explicit
> > > communication.
> > > There is no denying that these mechanisms work. They just aren't
> > > applicable to Jose's use.
> > 
> > If you say so (although it is unclear to me why or what exactly his
> > use
> > case is), but regardless, there is also no denying that getting and
> > setting another process' attributes via /proc/pid files is
> > inherently
> > racy.  
> 
> You're right. How can we fix that? I have seen a gazillion cases
> where system security would be much simpler and easier to enforce
> and develop if we could (safely) ensure that the process under
> /proc/pid wouldn't change on you without you knowing.

Yes, fully agreed. It summarizes well my mind. There is here a good job
to be done. I can imagine solution to this problem. And what Stephen
thinks as a law is just a bug.

> > He even acknowledged as much.  So we are left with a "security"
> > module whose only purpose is to support getting and setting process
> > tags for security enforcement purposes, and yet does so in a known-
> > insecure manner.  Again, this is why I keep suggesting that he
> > needs to
> > reconsider his approach, not merely figure out how to implement
> > per-
> > task security blobs.
> 
> Whether or not Jose moves forward with PTAGS we have identified
> an issue with the current cred based hooks for AppArmor, TOMOYO
> and at least one other proposed module. Regardless of the issues
> of /proc/pid there is work to be done.


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Smalley Dec. 20, 2016, 9:35 p.m. UTC | #41

On Tue, 2016-12-20 at 12:03 -0800, Casey Schaufler wrote:
> On 12/20/2016 11:35 AM, Stephen Smalley wrote:
> > 
> > On Tue, 2016-12-20 at 11:07 -0800, Casey Schaufler wrote:
> > > 
> > > On 12/20/2016 10:28 AM, Stephen Smalley wrote:
> > > > 
> > > > On Tue, 2016-12-20 at 10:17 -0800, Casey Schaufler wrote:
> > > > > 
> > > > > On 12/20/2016 8:50 AM, Stephen Smalley wrote:
> > > > > > 
> > > > > > On Tue, 2016-12-20 at 17:39 +0100, José Bollo wrote:
> > > > > > > 
> > > > > > > Le mardi 20 décembre 2016 à 11:14 -0500, Stephen Smalley
> > > > > > > a
> > > > > > > écrit
> > > > > > > :
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Looking at your PTAGS implementation, I feel it is only
> > > > > > > > fair to
> > > > > > > > warn
> > > > > > > > you that your usage of /proc/pid/attr is insecure,
> > > > > > > > regardless
> > > > > > > > of
> > > > > > > > whether you use task security blobs or cred security
> > > > > > > > blobs.
> > > > > > > Fair?!
> > > > > > > 
> > > > > > > > 
> > > > > > > > Getting the attributes of another process via /proc/pid
> > > > > > > > files
> > > > > > > > is
> > > > > > > > inherently racy, as the process may exit and another
> > > > > > > > process
> > > > > > > > with
> > > > > > > > different attributes may be created with the same pid
> > > > > > > > (and
> > > > > > > > no,
> > > > > > > > this
> > > > > > > > is not theoretical; it has been demonstrated).
> > > > > > > I know. And I'm surprized that you dont do anything to
> > > > > > > change
> > > > > > > that.
> > > > > > There is a reason why SO_PEERSEC and SCM_SECURITY
> > > > > > exist.  Again,
> > > > > > learn
> > > > > > from the upstream security modules rather than re-inventing
> > > > > > them,
> > > > > > badly.
> > > > > SO_PEERSEC and SCM_SECURITY are spiffy for processes that are
> > > > > sending each other messages, but they identify the attributes
> > > > > associated with the message, not the process. Neither SELinux
> > > > > nor Smack get the information to send off of the process, it
> > > > > comes from the socket structure.
> > > > Yes, but in the SELinux case at least, the socket is labeled
> > > > with
> > > > the
> > > > label of the creating process (except in the rare case of using
> > > > setsockcreatecon(3), which can only be used by suitably
> > > > authorized
> > > > processes).
> > > Yes, it's the same with Smack. When it's not the label
> > > of the process it's the label the system wants the peer
> > > to think it is.
> > > 
> > > > 
> > > >   So in general it serves quite well as a means of obtaining
> > > > the peer label, which can then be used in access control (and
> > > > this
> > > > is
> > > > in fact being used in a variety of applications in Linux and
> > > > Android).
> > > But only between processes that are in direct, explicit
> > > communication.
> > > There is no denying that these mechanisms work. They just aren't
> > > applicable to Jose's use.
> > If you say so (although it is unclear to me why or what exactly his
> > use
> > case is), but regardless, there is also no denying that getting and
> > setting another process' attributes via /proc/pid files is
> > inherently
> > racy.  
> 
> You're right. How can we fix that? I have seen a gazillion cases
> where system security would be much simpler and easier to enforce
> and develop if we could (safely) ensure that the process under
> /proc/pid wouldn't change on you without you knowing.

I don't think that is viable.  systemd for example maintains its own
cgroup hierarchy in order to manage processes.  But in the case of
ptags, I don't even see why the kernel needs to be involved in storing
the tags, since it never uses them and isn't even providing them in a
robust manner.  Might as well just take the tag set/get interface to
userspace too.  Or re-implement it using a capability-based model (in
the classical sense) via binder or bus1 or just file/socket
descriptors.  Or use polkit or any of the other userspace access
control implementations that already exist.  What was the use case
again?  And why aren't these processes communicating with each other?

> 
> > 
> > He even acknowledged as much.  So we are left with a "security"
> > module whose only purpose is to support getting and setting process
> > tags for security enforcement purposes, and yet does so in a known-
> > insecure manner.  Again, this is why I keep suggesting that he
> > needs to
> > reconsider his approach, not merely figure out how to implement
> > per-
> > task security blobs.
> 
> Whether or not Jose moves forward with PTAGS we have identified
> an issue with the current cred based hooks for AppArmor, TOMOYO
> and at least one other proposed module. Regardless of the issues
> of /proc/pid there is work to be done.

Just be aware that any checking or processing relying on task security
blobs will not be affected by override_creds() elsewhere in the kernel,
which is used not only by overlayfs but other parts of the kernel to
switch credentials temporarily as needed.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

John Johansen Dec. 20, 2016, 9:38 p.m. UTC | #42

On 12/20/2016 12:03 PM, Casey Schaufler wrote:
> On 12/20/2016 11:35 AM, Stephen Smalley wrote:
>> On Tue, 2016-12-20 at 11:07 -0800, Casey Schaufler wrote:
>>> On 12/20/2016 10:28 AM, Stephen Smalley wrote:
>>>> On Tue, 2016-12-20 at 10:17 -0800, Casey Schaufler wrote:
>>>>> On 12/20/2016 8:50 AM, Stephen Smalley wrote:
>>>>>> On Tue, 2016-12-20 at 17:39 +0100, José Bollo wrote:
>>>>>>> Le mardi 20 décembre 2016 à 11:14 -0500, Stephen Smalley a
>>>>>>> écrit
>>>>>>> :
>>>>>>>>
>>>>>>>> Looking at your PTAGS implementation, I feel it is only
>>>>>>>> fair to
>>>>>>>> warn
>>>>>>>> you that your usage of /proc/pid/attr is insecure,
>>>>>>>> regardless
>>>>>>>> of
>>>>>>>> whether you use task security blobs or cred security blobs.
>>>>>>> Fair?!
>>>>>>>
>>>>>>>> Getting the attributes of another process via /proc/pid
>>>>>>>> files
>>>>>>>> is
>>>>>>>> inherently racy, as the process may exit and another
>>>>>>>> process
>>>>>>>> with
>>>>>>>> different attributes may be created with the same pid (and
>>>>>>>> no,
>>>>>>>> this
>>>>>>>> is not theoretical; it has been demonstrated).
>>>>>>> I know. And I'm surprized that you dont do anything to change
>>>>>>> that.
>>>>>> There is a reason why SO_PEERSEC and SCM_SECURITY
>>>>>> exist.  Again,
>>>>>> learn
>>>>>> from the upstream security modules rather than re-inventing
>>>>>> them,
>>>>>> badly.
>>>>> SO_PEERSEC and SCM_SECURITY are spiffy for processes that are
>>>>> sending each other messages, but they identify the attributes
>>>>> associated with the message, not the process. Neither SELinux
>>>>> nor Smack get the information to send off of the process, it
>>>>> comes from the socket structure.
>>>> Yes, but in the SELinux case at least, the socket is labeled with
>>>> the
>>>> label of the creating process (except in the rare case of using
>>>> setsockcreatecon(3), which can only be used by suitably authorized
>>>> processes).
>>> Yes, it's the same with Smack. When it's not the label
>>> of the process it's the label the system wants the peer
>>> to think it is.
>>>
>>>>   So in general it serves quite well as a means of obtaining
>>>> the peer label, which can then be used in access control (and this
>>>> is
>>>> in fact being used in a variety of applications in Linux and
>>>> Android).
>>> But only between processes that are in direct, explicit
>>> communication.
>>> There is no denying that these mechanisms work. They just aren't
>>> applicable to Jose's use.
>> If you say so (although it is unclear to me why or what exactly his use
>> case is), but regardless, there is also no denying that getting and
>> setting another process' attributes via /proc/pid files is inherently
>> racy.  
> 
> You're right. How can we fix that? I have seen a gazillion cases
> where system security would be much simpler and easier to enforce
> and develop if we could (safely) ensure that the process under
> /proc/pid wouldn't change on you without you knowing.
> 
locking. In this case it would have to be user acquired file lock that
would hold the entire set of pid files in place so you could do a read
for some of its info and write to it. I haven't dug into it all so
I have no idea how likely getting such a change in would be, but I
have a feeling it will meet with a fair bit of resistance.

The other way pid based interfaces have been handled is using a tuple
of pid + creation timestamp or some other monotonically increasing value,
that way you can be sure that the referenced pid is the correct one.
I'm not a fan of the creation timestamp because time isn't always
monotonically increasing but Jose could easily use a monotonic value.
This would require Jose to track the monotonic value per task and make
the expected monotonic value part of the write spec so that his module
could validate it before applying the written tags.

The monotonic value doesn't need to increase with each task, just each
reuse, but the it would probably be easiest just to use a value that
increases once per task created.

I'm sure there are probably a few other techniques that could be found
to make PTAGS safe. Whether /proc/pid/attr/ is the best interface
is a separate question.

>> He even acknowledged as much.  So we are left with a "security"
>> module whose only purpose is to support getting and setting process
>> tags for security enforcement purposes, and yet does so in a known-
>> insecure manner.  Again, this is why I keep suggesting that he needs to
>> reconsider his approach, not merely figure out how to implement per-
>> task security blobs.
> 
> Whether or not Jose moves forward with PTAGS we have identified
> an issue with the current cred based hooks for AppArmor, TOMOYO
> and at least one other proposed module. Regardless of the issues
> of /proc/pid there is work to be done.
> 
I can take a stab at it in a few weeks. While not critical for apparmor
it would certainly cleanup apparmor's cred handling.



--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paul Moore Dec. 21, 2016, 2:37 a.m. UTC | #43

On Fri, Dec 16, 2016 at 12:41 PM, Stephen Smalley <[e-mail]> wrote:
> Processes can only alter their own security attributes via
> /proc/pid/attr nodes.  This is presently enforced by each individual
> security module and is also imposed by the Linux credentials
> implementation, which only allows a task to alter its own credentials.
> Move the check enforcing this restriction from the individual
> security modules to proc_pid_attr_write() before calling the security hook,
> and drop the unnecessary task argument to the security hook since it can
> only ever be the current task.
>
> Signed-off-by: Stephen Smalley <[e-mail]>
> ---
>  fs/proc/base.c             | 13 +++++++++----
>  include/linux/lsm_hooks.h  |  3 +--
>  include/linux/security.h   |  4 ++--
>  security/apparmor/lsm.c    |  7 ++-----
>  security/security.c        |  4 ++--
>  security/selinux/hooks.c   | 13 +------------
>  security/smack/smack_lsm.c | 11 +----------
>  7 files changed, 18 insertions(+), 37 deletions(-)

Merged into the selinux#next branch.

> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6eae4d0..7b228ea 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2485,6 +2485,12 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>         length = -ESRCH;
>         if (!task)
>                 goto out_no_task;
> +
> +       /* A task may only write its own attributes. */
> +       length = -EACCES;
> +       if (current != task)
> +               goto out;
> +
>         if (count > PAGE_SIZE)
>                 count = PAGE_SIZE;
>
> @@ -2500,14 +2506,13 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>         }
>
>         /* Guard against adverse ptrace interaction */
> -       length = mutex_lock_interruptible(&task->signal->cred_guard_mutex);
> +       length = mutex_lock_interruptible(&current->signal->cred_guard_mutex);
>         if (length < 0)
>                 goto out_free;
>
> -       length = security_setprocattr(task,
> -                                     (char*)file->f_path.dentry->d_name.name,
> +       length = security_setprocattr(file->f_path.dentry->d_name.name,
>                                       page, count);
> -       mutex_unlock(&task->signal->cred_guard_mutex);
> +       mutex_unlock(&current->signal->cred_guard_mutex);
>  out_free:
>         kfree(page);
>  out:
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 558adfa..0dde959 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1547,8 +1547,7 @@ union security_list_options {
>         void (*d_instantiate)(struct dentry *dentry, struct inode *inode);
>
>         int (*getprocattr)(struct task_struct *p, char *name, char **value);
> -       int (*setprocattr)(struct task_struct *p, char *name, void *value,
> -                               size_t size);
> +       int (*setprocattr)(const char *name, void *value, size_t size);
>         int (*ismaclabel)(const char *name);
>         int (*secid_to_secctx)(u32 secid, char **secdata, u32 *seclen);
>         int (*secctx_to_secid)(const char *secdata, u32 seclen, u32 *secid);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index c2125e9..f4ebac1 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -361,7 +361,7 @@ int security_sem_semop(struct sem_array *sma, struct sembuf *sops,
>                         unsigned nsops, int alter);
>  void security_d_instantiate(struct dentry *dentry, struct inode *inode);
>  int security_getprocattr(struct task_struct *p, char *name, char **value);
> -int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size);
> +int security_setprocattr(const char *name, void *value, size_t size);
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>  int security_ismaclabel(const char *name);
>  int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> @@ -1106,7 +1106,7 @@ static inline int security_getprocattr(struct task_struct *p, char *name, char *
>         return -EINVAL;
>  }
>
> -static inline int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size)
> +static inline int security_setprocattr(char *name, void *value, size_t size)
>  {
>         return -EINVAL;
>  }
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 41b8cb1..8202e55 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -495,8 +495,8 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
>         return error;
>  }
>
> -static int apparmor_setprocattr(struct task_struct *task, char *name,
> -                               void *value, size_t size)
> +static int apparmor_setprocattr(const char *name, void *value,
> +                               size_t size)
>  {
>         struct common_audit_data sa;
>         struct apparmor_audit_data aad = {0,};
> @@ -506,9 +506,6 @@ static int apparmor_setprocattr(struct task_struct *task, char *name,
>
>         if (size == 0)
>                 return -EINVAL;
> -       /* task can only write its own attributes */
> -       if (current != task)
> -               return -EACCES;
>
>         /* AppArmor requires that the buffer must be null terminated atm */
>         if (args[size - 1] != '\0') {
> diff --git a/security/security.c b/security/security.c
> index f825304..32052f5 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1170,9 +1170,9 @@ int security_getprocattr(struct task_struct *p, char *name, char **value)
>         return call_int_hook(getprocattr, -EINVAL, p, name, value);
>  }
>
> -int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size)
> +int security_setprocattr(const char *name, void *value, size_t size)
>  {
> -       return call_int_hook(setprocattr, -EINVAL, p, name, value, size);
> +       return call_int_hook(setprocattr, -EINVAL, name, value, size);
>  }
>
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9992626..762276b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5859,8 +5859,7 @@ static int selinux_getprocattr(struct task_struct *p,
>         return error;
>  }
>
> -static int selinux_setprocattr(struct task_struct *p,
> -                              char *name, void *value, size_t size)
> +static int selinux_setprocattr(const char *name, void *value, size_t size)
>  {
>         struct task_security_struct *tsec;
>         struct cred *new;
> @@ -5868,16 +5867,6 @@ static int selinux_setprocattr(struct task_struct *p,
>         int error;
>         char *str = value;
>
> -       if (current != p) {
> -               /*
> -                * A task may only alter its own credentials.
> -                * SELinux has always enforced this restriction,
> -                * and it is now mandated by the Linux credentials
> -                * infrastructure; see Documentation/security/credentials.txt.
> -                */
> -               return -EACCES;
> -       }
> -
>         /*
>          * Basic control over ability to set these attributes at all.
>          */
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 4d90257..9bde7f8 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3620,7 +3620,6 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>
>  /**
>   * smack_setprocattr - Smack process attribute setting
> - * @p: the object task
>   * @name: the name of the attribute in /proc/.../attr
>   * @value: the value to set
>   * @size: the size of the value
> @@ -3630,8 +3629,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>   *
>   * Returns the length of the smack label or an error code
>   */
> -static int smack_setprocattr(struct task_struct *p, char *name,
> -                            void *value, size_t size)
> +static int smack_setprocattr(const char *name, void *value, size_t size)
>  {
>         struct task_smack *tsp = current_security();
>         struct cred *new;
> @@ -3639,13 +3637,6 @@ static int smack_setprocattr(struct task_struct *p, char *name,
>         struct smack_known_list_elem *sklep;
>         int rc;
>
> -       /*
> -        * Changing another process' Smack value is too dangerous
> -        * and supports no sane use case.
> -        */
> -       if (p != current)
> -               return -EPERM;
> -
>         if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel))
>                 return -EPERM;
>
> --
> 2.7.4
>

José Bollo Dec. 21, 2016, 7:04 a.m. UTC | #44

Le mardi 20 décembre 2016 à 21:37 -0500, Paul Moore a écrit :
> On Fri, Dec 16, 2016 at 12:41 PM, Stephen Smalley <[e-mail]>
> wrote:
> > Processes can only alter their own security attributes via
> > /proc/pid/attr nodes.  This is presently enforced by each
> > individual
> > security module and is also imposed by the Linux credentials
> > implementation, which only allows a task to alter its own
> > credentials.
> > Move the check enforcing this restriction from the individual
> > security modules to proc_pid_attr_write() before calling the
> > security hook,
> > and drop the unnecessary task argument to the security hook since
> > it can
> > only ever be the current task.
> > 
> > Signed-off-by: Stephen Smalley <[e-mail]>
> > ---
> >  fs/proc/base.c             | 13 +++++++++----
> >  include/linux/lsm_hooks.h  |  3 +--
> >  include/linux/security.h   |  4 ++--
> >  security/apparmor/lsm.c    |  7 ++-----
> >  security/security.c        |  4 ++--
> >  security/selinux/hooks.c   | 13 +------------
> >  security/smack/smack_lsm.c | 11 +----------
> >  7 files changed, 18 insertions(+), 37 deletions(-)
> 
> Merged into the selinux#next branch.

is it fair?


> 
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 6eae4d0..7b228ea 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -2485,6 +2485,12 @@ static ssize_t proc_pid_attr_write(struct
> > file * file, const char __user * buf,
> >         length = -ESRCH;
> >         if (!task)
> >                 goto out_no_task;
> > +
> > +       /* A task may only write its own attributes. */
> > +       length = -EACCES;
> > +       if (current != task)
> > +               goto out;
> > +
> >         if (count > PAGE_SIZE)
> >                 count = PAGE_SIZE;
> > 
> > @@ -2500,14 +2506,13 @@ static ssize_t proc_pid_attr_write(struct
> > file * file, const char __user * buf,
> >         }
> > 
> >         /* Guard against adverse ptrace interaction */
> > -       length = mutex_lock_interruptible(&task->signal-
> > >cred_guard_mutex);
> > +       length = mutex_lock_interruptible(&current->signal-
> > >cred_guard_mutex);
> >         if (length < 0)
> >                 goto out_free;
> > 
> > -       length = security_setprocattr(task,
> > -                                     (char*)file->f_path.dentry-
> > >d_name.name,
> > +       length = security_setprocattr(file->f_path.dentry-
> > >d_name.name,
> >                                       page, count);
> > -       mutex_unlock(&task->signal->cred_guard_mutex);
> > +       mutex_unlock(&current->signal->cred_guard_mutex);
> >  out_free:
> >         kfree(page);
> >  out:
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 558adfa..0dde959 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -1547,8 +1547,7 @@ union security_list_options {
> >         void (*d_instantiate)(struct dentry *dentry, struct inode
> > *inode);
> > 
> >         int (*getprocattr)(struct task_struct *p, char *name, char
> > **value);
> > -       int (*setprocattr)(struct task_struct *p, char *name, void
> > *value,
> > -                               size_t size);
> > +       int (*setprocattr)(const char *name, void *value, size_t
> > size);
> >         int (*ismaclabel)(const char *name);
> >         int (*secid_to_secctx)(u32 secid, char **secdata, u32
> > *seclen);
> >         int (*secctx_to_secid)(const char *secdata, u32 seclen, u32
> > *secid);
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index c2125e9..f4ebac1 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -361,7 +361,7 @@ int security_sem_semop(struct sem_array *sma,
> > struct sembuf *sops,
> >                         unsigned nsops, int alter);
> >  void security_d_instantiate(struct dentry *dentry, struct inode
> > *inode);
> >  int security_getprocattr(struct task_struct *p, char *name, char
> > **value);
> > -int security_setprocattr(struct task_struct *p, char *name, void
> > *value, size_t size);
> > +int security_setprocattr(const char *name, void *value, size_t
> > size);
> >  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> >  int security_ismaclabel(const char *name);
> >  int security_secid_to_secctx(u32 secid, char **secdata, u32
> > *seclen);
> > @@ -1106,7 +1106,7 @@ static inline int security_getprocattr(struct
> > task_struct *p, char *name, char *
> >         return -EINVAL;
> >  }
> > 
> > -static inline int security_setprocattr(struct task_struct *p, char
> > *name, void *value, size_t size)
> > +static inline int security_setprocattr(char *name, void *value,
> > size_t size)
> >  {
> >         return -EINVAL;
> >  }
> > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > index 41b8cb1..8202e55 100644
> > --- a/security/apparmor/lsm.c
> > +++ b/security/apparmor/lsm.c
> > @@ -495,8 +495,8 @@ static int apparmor_getprocattr(struct
> > task_struct *task, char *name,
> >         return error;
> >  }
> > 
> > -static int apparmor_setprocattr(struct task_struct *task, char
> > *name,
> > -                               void *value, size_t size)
> > +static int apparmor_setprocattr(const char *name, void *value,
> > +                               size_t size)
> >  {
> >         struct common_audit_data sa;
> >         struct apparmor_audit_data aad = {0,};
> > @@ -506,9 +506,6 @@ static int apparmor_setprocattr(struct
> > task_struct *task, char *name,
> > 
> >         if (size == 0)
> >                 return -EINVAL;
> > -       /* task can only write its own attributes */
> > -       if (current != task)
> > -               return -EACCES;
> > 
> >         /* AppArmor requires that the buffer must be null
> > terminated atm */
> >         if (args[size - 1] != '\0') {
> > diff --git a/security/security.c b/security/security.c
> > index f825304..32052f5 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1170,9 +1170,9 @@ int security_getprocattr(struct task_struct
> > *p, char *name, char **value)
> >         return call_int_hook(getprocattr, -EINVAL, p, name, value);
> >  }
> > 
> > -int security_setprocattr(struct task_struct *p, char *name, void
> > *value, size_t size)
> > +int security_setprocattr(const char *name, void *value, size_t
> > size)
> >  {
> > -       return call_int_hook(setprocattr, -EINVAL, p, name, value,
> > size);
> > +       return call_int_hook(setprocattr, -EINVAL, name, value,
> > size);
> >  }
> > 
> >  int security_netlink_send(struct sock *sk, struct sk_buff *skb)
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 9992626..762276b 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -5859,8 +5859,7 @@ static int selinux_getprocattr(struct
> > task_struct *p,
> >         return error;
> >  }
> > 
> > -static int selinux_setprocattr(struct task_struct *p,
> > -                              char *name, void *value, size_t
> > size)
> > +static int selinux_setprocattr(const char *name, void *value,
> > size_t size)
> >  {
> >         struct task_security_struct *tsec;
> >         struct cred *new;
> > @@ -5868,16 +5867,6 @@ static int selinux_setprocattr(struct
> > task_struct *p,
> >         int error;
> >         char *str = value;
> > 
> > -       if (current != p) {
> > -               /*
> > -                * A task may only alter its own credentials.
> > -                * SELinux has always enforced this restriction,
> > -                * and it is now mandated by the Linux credentials
> > -                * infrastructure; see
> > Documentation/security/credentials.txt.
> > -                */
> > -               return -EACCES;
> > -       }
> > -
> >         /*
> >          * Basic control over ability to set these attributes at
> > all.
> >          */
> > diff --git a/security/smack/smack_lsm.c
> > b/security/smack/smack_lsm.c
> > index 4d90257..9bde7f8 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -3620,7 +3620,6 @@ static int smack_getprocattr(struct
> > task_struct *p, char *name, char **value)
> > 
> >  /**
> >   * smack_setprocattr - Smack process attribute setting
> > - * @p: the object task
> >   * @name: the name of the attribute in /proc/.../attr
> >   * @value: the value to set
> >   * @size: the size of the value
> > @@ -3630,8 +3629,7 @@ static int smack_getprocattr(struct
> > task_struct *p, char *name, char **value)
> >   *
> >   * Returns the length of the smack label or an error code
> >   */
> > -static int smack_setprocattr(struct task_struct *p, char *name,
> > -                            void *value, size_t size)
> > +static int smack_setprocattr(const char *name, void *value, size_t
> > size)
> >  {
> >         struct task_smack *tsp = current_security();
> >         struct cred *new;
> > @@ -3639,13 +3637,6 @@ static int smack_setprocattr(struct
> > task_struct *p, char *name,
> >         struct smack_known_list_elem *sklep;
> >         int rc;
> > 
> > -       /*
> > -        * Changing another process' Smack value is too dangerous
> > -        * and supports no sane use case.
> > -        */
> > -       if (p != current)
> > -               return -EPERM;
> > -
> >         if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp-
> > >smk_relabel))
> >                 return -EPERM;
> > 
> > --
> > 2.7.4
> > 
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paul Moore Dec. 21, 2016, 3:15 p.m. UTC | #45

On Wed, Dec 21, 2016 at 2:04 AM, José Bollo <[e-mail]> wrote:
> Le mardi 20 décembre 2016 à 21:37 -0500, Paul Moore a écrit :
>> On Fri, Dec 16, 2016 at 12:41 PM, Stephen Smalley <[e-mail]>
>> wrote:
>> > Processes can only alter their own security attributes via
>> > /proc/pid/attr nodes.  This is presently enforced by each
>> > individual
>> > security module and is also imposed by the Linux credentials
>> > implementation, which only allows a task to alter its own
>> > credentials.
>> > Move the check enforcing this restriction from the individual
>> > security modules to proc_pid_attr_write() before calling the
>> > security hook,
>> > and drop the unnecessary task argument to the security hook since
>> > it can
>> > only ever be the current task.
>> >
>> > Signed-off-by: Stephen Smalley <[e-mail]>
>> > ---
>> >  fs/proc/base.c             | 13 +++++++++----
>> >  include/linux/lsm_hooks.h  |  3 +--
>> >  include/linux/security.h   |  4 ++--
>> >  security/apparmor/lsm.c    |  7 ++-----
>> >  security/security.c        |  4 ++--
>> >  security/selinux/hooks.c   | 13 +------------
>> >  security/smack/smack_lsm.c | 11 +----------
>> >  7 files changed, 18 insertions(+), 37 deletions(-)
>>
>> Merged into the selinux#next branch.
>
> is it fair?

I believe so, yes.  As many have already mentioned, this patch doesn't
introduce a new restriction, it simply cleans up an existing
restriction.  If/when PTAGS is merged upstream it can make any changes
needed as long as those changes do not cause a regression in the
safety or behavior of the existing LSMs and the kernel as a whole.

Patch

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6eae4d0..7b228ea 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2485,6 +2485,12 @@  static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
 	length = -ESRCH;
 	if (!task)
 		goto out_no_task;
+
+	/* A task may only write its own attributes. */
+	length = -EACCES;
+	if (current != task)
+		goto out;
+
 	if (count > PAGE_SIZE)
 		count = PAGE_SIZE;
 
@@ -2500,14 +2506,13 @@  static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
 	}
 
 	/* Guard against adverse ptrace interaction */
-	length = mutex_lock_interruptible(&task->signal->cred_guard_mutex);
+	length = mutex_lock_interruptible(&current->signal->cred_guard_mutex);
 	if (length < 0)
 		goto out_free;
 
-	length = security_setprocattr(task,
-				      (char*)file->f_path.dentry->d_name.name,
+	length = security_setprocattr(file->f_path.dentry->d_name.name,
 				      page, count);
-	mutex_unlock(&task->signal->cred_guard_mutex);
+	mutex_unlock(&current->signal->cred_guard_mutex);
 out_free:
 	kfree(page);
 out:
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 558adfa..0dde959 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1547,8 +1547,7 @@  union security_list_options {
 	void (*d_instantiate)(struct dentry *dentry, struct inode *inode);
 
 	int (*getprocattr)(struct task_struct *p, char *name, char **value);
-	int (*setprocattr)(struct task_struct *p, char *name, void *value,
-				size_t size);
+	int (*setprocattr)(const char *name, void *value, size_t size);
 	int (*ismaclabel)(const char *name);
 	int (*secid_to_secctx)(u32 secid, char **secdata, u32 *seclen);
 	int (*secctx_to_secid)(const char *secdata, u32 seclen, u32 *secid);
diff --git a/include/linux/security.h b/include/linux/security.h
index c2125e9..f4ebac1 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -361,7 +361,7 @@  int security_sem_semop(struct sem_array *sma, struct sembuf *sops,
 			unsigned nsops, int alter);
 void security_d_instantiate(struct dentry *dentry, struct inode *inode);
 int security_getprocattr(struct task_struct *p, char *name, char **value);
-int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size);
+int security_setprocattr(const char *name, void *value, size_t size);
 int security_netlink_send(struct sock *sk, struct sk_buff *skb);
 int security_ismaclabel(const char *name);
 int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
@@ -1106,7 +1106,7 @@  static inline int security_getprocattr(struct task_struct *p, char *name, char *
 	return -EINVAL;
 }
 
-static inline int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size)
+static inline int security_setprocattr(char *name, void *value, size_t size)
 {
 	return -EINVAL;
 }
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 41b8cb1..8202e55 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -495,8 +495,8 @@  static int apparmor_getprocattr(struct task_struct *task, char *name,
 	return error;
 }
 
-static int apparmor_setprocattr(struct task_struct *task, char *name,
-				void *value, size_t size)
+static int apparmor_setprocattr(const char *name, void *value,
+				size_t size)
 {
 	struct common_audit_data sa;
 	struct apparmor_audit_data aad = {0,};
@@ -506,9 +506,6 @@  static int apparmor_setprocattr(struct task_struct *task, char *name,
 
 	if (size == 0)
 		return -EINVAL;
-	/* task can only write its own attributes */
-	if (current != task)
-		return -EACCES;
 
 	/* AppArmor requires that the buffer must be null terminated atm */
 	if (args[size - 1] != '\0') {
diff --git a/security/security.c b/security/security.c
index f825304..32052f5 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1170,9 +1170,9 @@  int security_getprocattr(struct task_struct *p, char *name, char **value)
 	return call_int_hook(getprocattr, -EINVAL, p, name, value);
 }
 
-int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size)
+int security_setprocattr(const char *name, void *value, size_t size)
 {
-	return call_int_hook(setprocattr, -EINVAL, p, name, value, size);
+	return call_int_hook(setprocattr, -EINVAL, name, value, size);
 }
 
 int security_netlink_send(struct sock *sk, struct sk_buff *skb)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9992626..762276b 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5859,8 +5859,7 @@  static int selinux_getprocattr(struct task_struct *p,
 	return error;
 }
 
-static int selinux_setprocattr(struct task_struct *p,
-			       char *name, void *value, size_t size)
+static int selinux_setprocattr(const char *name, void *value, size_t size)
 {
 	struct task_security_struct *tsec;
 	struct cred *new;
@@ -5868,16 +5867,6 @@  static int selinux_setprocattr(struct task_struct *p,
 	int error;
 	char *str = value;
 
-	if (current != p) {
-		/*
-		 * A task may only alter its own credentials.
-		 * SELinux has always enforced this restriction,
-		 * and it is now mandated by the Linux credentials
-		 * infrastructure; see Documentation/security/credentials.txt.
-		 */
-		return -EACCES;
-	}
-
 	/*
 	 * Basic control over ability to set these attributes at all.
 	 */
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 4d90257..9bde7f8 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3620,7 +3620,6 @@  static int smack_getprocattr(struct task_struct *p, char *name, char **value)
 
 /**
  * smack_setprocattr - Smack process attribute setting
- * @p: the object task
  * @name: the name of the attribute in /proc/.../attr
  * @value: the value to set
  * @size: the size of the value
@@ -3630,8 +3629,7 @@  static int smack_getprocattr(struct task_struct *p, char *name, char **value)
  *
  * Returns the length of the smack label or an error code
  */
-static int smack_setprocattr(struct task_struct *p, char *name,
-			     void *value, size_t size)
+static int smack_setprocattr(const char *name, void *value, size_t size)
 {
 	struct task_smack *tsp = current_security();
 	struct cred *new;
@@ -3639,13 +3637,6 @@  static int smack_setprocattr(struct task_struct *p, char *name,
 	struct smack_known_list_elem *sklep;
 	int rc;
 
-	/*
-	 * Changing another process' Smack value is too dangerous
-	 * and supports no sane use case.
-	 */
-	if (p != current)
-		return -EPERM;
-
 	if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel))
 		return -EPERM;