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

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 <sds@tycho.nsa.gov>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
Acked-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 8e7e61b28f31c037961c081d09a7be5f818013ef..988c5a77e8882a5583638d5b3144d7be63de68d8 100644 (file)
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2488,6 +2488,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;
 
@@ -2503,14 +2509,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 558adfa5c8a87730f0d0177a1def7570e4883596..0dde9590019682a2f86a1aa28e3c4a4b8036d17d 100644 (file)
--- 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 c2125e9093e8e51b51662ba8ce4a5faa767710b6..f4ebac117fa631f53f27c3fb9d57e7c1cbef5c15 100644 (file)
--- 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 41b8cb115801724172ce48cbb3ac6b3d96a8d6f6..8202e5583479d0a2a1791f641aa9ca56f02d5207 100644 (file)
--- 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 f825304f04a773763cad9c36ac5bc68f9e7a9aa1..32052f5c76b29991577d718ba26336dd974e00c0 100644 (file)
--- 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 262e108c36d4fe9fece9af66fe24175270ccf016..bada3cd42b9c551ebf25cf7ff4aa4853fc41b873 100644 (file)
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5862,8 +5862,7 @@ bad:
        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;
@@ -5871,16 +5870,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 94dc9d406ce33060513231c1f93727a51a106e14..8da4a6b9ca4db903210f3acb047914e56935a5a9 100644 (file)
--- 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;