* [PATCH 1/2] selinux: clean up cred usage and simplify
@ 2016-12-16 17:41 Stephen Smalley
2016-12-16 17:41 ` [PATCH 2/2] proc, security: move restriction on writing /proc/pid/attr nodes to proc Stephen Smalley
2016-12-16 22:02 ` [PATCH 1/2] selinux: clean up cred usage and simplify Paul Moore
0 siblings, 2 replies; 48+ messages in thread
From: Stephen Smalley @ 2016-12-16 17:41 UTC (permalink / raw)
To: selinux
Cc: paul, james.l.morris, linux-security-module, casey,
john.johansen, Stephen Smalley
SELinux was sometimes using the task "objective" credentials when
it could/should use the "subjective" credentials. This was sometimes
hidden by the fact that we were unnecessarily passing around pointers
to the current task, making it appear as if the task could be something
other than current, so eliminate all such passing of current. Inline
various permission checking helper functions that can be reduced to a
single avc_has_perm() call.
Since the credentials infrastructure only allows a task to alter
its own credentials, we can always assume that current must be the same
as the target task in selinux_setprocattr after the check. We likely
should move this check from selinux_setprocattr() to proc_pid_attr_write()
and drop the task argument to the security hook altogether; it can only
serve to confuse things.
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
security/selinux/hooks.c | 294 +++++++++++++++-----------------------
security/selinux/include/objsec.h | 10 ++
security/selinux/selinuxfs.c | 73 +++++-----
3 files changed, 166 insertions(+), 211 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e7c5481..9992626 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -210,16 +210,6 @@ static inline u32 task_sid(const struct task_struct *task)
return sid;
}
-/*
- * get the subjective security ID of the current task
- */
-static inline u32 current_sid(void)
-{
- const struct task_security_struct *tsec = current_security();
-
- return tsec->sid;
-}
-
/* Allocate and free functions for each kind of security blob. */
static int inode_alloc_security(struct inode *inode)
@@ -1687,55 +1677,6 @@ static inline u32 signal_to_av(int sig)
return perm;
}
-/*
- * Check permission between a pair of credentials
- * fork check, ptrace check, etc.
- */
-static int cred_has_perm(const struct cred *actor,
- const struct cred *target,
- u32 perms)
-{
- u32 asid = cred_sid(actor), tsid = cred_sid(target);
-
- return avc_has_perm(asid, tsid, SECCLASS_PROCESS, perms, NULL);
-}
-
-/*
- * Check permission between a pair of tasks, e.g. signal checks,
- * fork check, ptrace check, etc.
- * tsk1 is the actor and tsk2 is the target
- * - this uses the default subjective creds of tsk1
- */
-static int task_has_perm(const struct task_struct *tsk1,
- const struct task_struct *tsk2,
- u32 perms)
-{
- const struct task_security_struct *__tsec1, *__tsec2;
- u32 sid1, sid2;
-
- rcu_read_lock();
- __tsec1 = __task_cred(tsk1)->security; sid1 = __tsec1->sid;
- __tsec2 = __task_cred(tsk2)->security; sid2 = __tsec2->sid;
- rcu_read_unlock();
- return avc_has_perm(sid1, sid2, SECCLASS_PROCESS, perms, NULL);
-}
-
-/*
- * Check permission between current and another task, e.g. signal checks,
- * fork check, ptrace check, etc.
- * current is the actor and tsk2 is the target
- * - this uses current's subjective creds
- */
-static int current_has_perm(const struct task_struct *tsk,
- u32 perms)
-{
- u32 sid, tsid;
-
- sid = current_sid();
- tsid = task_sid(tsk);
- return avc_has_perm(sid, tsid, SECCLASS_PROCESS, perms, NULL);
-}
-
#if CAP_LAST_CAP > 63
#error Fix SELinux to handle capabilities > 63.
#endif
@@ -1777,16 +1718,6 @@ static int cred_has_capability(const struct cred *cred,
return rc;
}
-/* Check whether a task is allowed to use a system operation. */
-static int task_has_system(struct task_struct *tsk,
- u32 perms)
-{
- u32 sid = task_sid(tsk);
-
- return avc_has_perm(sid, SECINITSID_KERNEL,
- SECCLASS_SYSTEM, perms, NULL);
-}
-
/* Check whether a task has a particular permission to an inode.
The 'adp' parameter is optional and allows other audit
data to be passed (e.g. the dentry). */
@@ -1958,15 +1889,6 @@ static int may_create(struct inode *dir,
FILESYSTEM__ASSOCIATE, &ad);
}
-/* Check whether a task can create a key. */
-static int may_create_key(u32 ksid,
- struct task_struct *ctx)
-{
- u32 sid = task_sid(ctx);
-
- return avc_has_perm(sid, ksid, SECCLASS_KEY, KEY__CREATE, NULL);
-}
-
#define MAY_LINK 0
#define MAY_UNLINK 1
#define MAY_RMDIR 2
@@ -2222,24 +2144,26 @@ static int selinux_binder_transfer_file(struct task_struct *from,
static int selinux_ptrace_access_check(struct task_struct *child,
unsigned int mode)
{
- if (mode & PTRACE_MODE_READ) {
- u32 sid = current_sid();
- u32 csid = task_sid(child);
+ u32 sid = current_sid();
+ u32 csid = task_sid(child);
+
+ if (mode & PTRACE_MODE_READ)
return avc_has_perm(sid, csid, SECCLASS_FILE, FILE__READ, NULL);
- }
- return current_has_perm(child, PROCESS__PTRACE);
+ return avc_has_perm(sid, csid, SECCLASS_PROCESS, PROCESS__PTRACE, NULL);
}
static int selinux_ptrace_traceme(struct task_struct *parent)
{
- return task_has_perm(parent, current, PROCESS__PTRACE);
+ return avc_has_perm(task_sid(parent), current_sid(), SECCLASS_PROCESS,
+ PROCESS__PTRACE, NULL);
}
static int selinux_capget(struct task_struct *target, kernel_cap_t *effective,
kernel_cap_t *inheritable, kernel_cap_t *permitted)
{
- return current_has_perm(target, PROCESS__GETCAP);
+ return avc_has_perm(current_sid(), task_sid(target), SECCLASS_PROCESS,
+ PROCESS__GETCAP, NULL);
}
static int selinux_capset(struct cred *new, const struct cred *old,
@@ -2247,7 +2171,8 @@ static int selinux_capset(struct cred *new, const struct cred *old,
const kernel_cap_t *inheritable,
const kernel_cap_t *permitted)
{
- return cred_has_perm(old, new, PROCESS__SETCAP);
+ return avc_has_perm(cred_sid(old), cred_sid(new), SECCLASS_PROCESS,
+ PROCESS__SETCAP, NULL);
}
/*
@@ -2303,29 +2228,22 @@ static int selinux_quota_on(struct dentry *dentry)
static int selinux_syslog(int type)
{
- int rc;
-
switch (type) {
case SYSLOG_ACTION_READ_ALL: /* Read last kernel messages */
case SYSLOG_ACTION_SIZE_BUFFER: /* Return size of the log buffer */
- rc = task_has_system(current, SYSTEM__SYSLOG_READ);
- break;
+ return avc_has_perm(current_sid(), SECINITSID_KERNEL,
+ SECCLASS_SYSTEM, SYSTEM__SYSLOG_READ, NULL);
case SYSLOG_ACTION_CONSOLE_OFF: /* Disable logging to console */
case SYSLOG_ACTION_CONSOLE_ON: /* Enable logging to console */
/* Set level of messages printed to console */
case SYSLOG_ACTION_CONSOLE_LEVEL:
- rc = task_has_system(current, SYSTEM__SYSLOG_CONSOLE);
- break;
- case SYSLOG_ACTION_CLOSE: /* Close log */
- case SYSLOG_ACTION_OPEN: /* Open log */
- case SYSLOG_ACTION_READ: /* Read from log */
- case SYSLOG_ACTION_READ_CLEAR: /* Read/clear last kernel messages */
- case SYSLOG_ACTION_CLEAR: /* Clear ring buffer */
- default:
- rc = task_has_system(current, SYSTEM__SYSLOG_MOD);
- break;
+ return avc_has_perm(current_sid(), SECINITSID_KERNEL,
+ SECCLASS_SYSTEM, SYSTEM__SYSLOG_CONSOLE,
+ NULL);
}
- return rc;
+ /* All other syslog types */
+ return avc_has_perm(current_sid(), SECINITSID_KERNEL,
+ SECCLASS_SYSTEM, SYSTEM__SYSLOG_MOD, NULL);
}
/*
@@ -2350,13 +2268,13 @@ static int selinux_vm_enough_memory(struct mm_struct *mm, long pages)
/* binprm security operations */
-static u32 ptrace_parent_sid(struct task_struct *task)
+static u32 ptrace_parent_sid(void)
{
u32 sid = 0;
struct task_struct *tracer;
rcu_read_lock();
- tracer = ptrace_parent(task);
+ tracer = ptrace_parent(current);
if (tracer)
sid = task_sid(tracer);
rcu_read_unlock();
@@ -2485,7 +2403,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
* changes its SID has the appropriate permit */
if (bprm->unsafe &
(LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
- u32 ptsid = ptrace_parent_sid(current);
+ u32 ptsid = ptrace_parent_sid();
if (ptsid != 0) {
rc = avc_has_perm(ptsid, new_tsec->sid,
SECCLASS_PROCESS,
@@ -3579,6 +3497,7 @@ static int default_noexec;
static int file_map_prot_check(struct file *file, unsigned long prot, int shared)
{
const struct cred *cred = current_cred();
+ u32 sid = cred_sid(cred);
int rc = 0;
if (default_noexec &&
@@ -3589,7 +3508,8 @@ static int file_map_prot_check(struct file *file, unsigned long prot, int shared
* private file mapping that will also be writable.
* This has an additional check.
*/
- rc = cred_has_perm(cred, cred, PROCESS__EXECMEM);
+ rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
+ PROCESS__EXECMEM, NULL);
if (rc)
goto error;
}
@@ -3640,6 +3560,7 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
unsigned long prot)
{
const struct cred *cred = current_cred();
+ u32 sid = cred_sid(cred);
if (selinux_checkreqprot)
prot = reqprot;
@@ -3649,12 +3570,14 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
int rc = 0;
if (vma->vm_start >= vma->vm_mm->start_brk &&
vma->vm_end <= vma->vm_mm->brk) {
- rc = cred_has_perm(cred, cred, PROCESS__EXECHEAP);
+ rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
+ PROCESS__EXECHEAP, NULL);
} else if (!vma->vm_file &&
((vma->vm_start <= vma->vm_mm->start_stack &&
vma->vm_end >= vma->vm_mm->start_stack) ||
vma_is_stack_for_current(vma))) {
- rc = current_has_perm(current, PROCESS__EXECSTACK);
+ rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
+ PROCESS__EXECSTACK, NULL);
} else if (vma->vm_file && vma->anon_vma) {
/*
* We are making executable a file mapping that has
@@ -3787,7 +3710,9 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
static int selinux_task_create(unsigned long clone_flags)
{
- return current_has_perm(current, PROCESS__FORK);
+ u32 sid = current_sid();
+
+ return avc_has_perm(sid, sid, SECCLASS_PROCESS, PROCESS__FORK, NULL);
}
/*
@@ -3897,15 +3822,12 @@ static int selinux_kernel_create_files_as(struct cred *new, struct inode *inode)
static int selinux_kernel_module_request(char *kmod_name)
{
- u32 sid;
struct common_audit_data ad;
- sid = task_sid(current);
-
ad.type = LSM_AUDIT_DATA_KMOD;
ad.u.kmod_name = kmod_name;
- return avc_has_perm(sid, SECINITSID_KERNEL, SECCLASS_SYSTEM,
+ return avc_has_perm(current_sid(), SECINITSID_KERNEL, SECCLASS_SYSTEM,
SYSTEM__MODULE_REQUEST, &ad);
}
@@ -3957,17 +3879,20 @@ static int selinux_kernel_read_file(struct file *file,
static int selinux_task_setpgid(struct task_struct *p, pid_t pgid)
{
- return current_has_perm(p, PROCESS__SETPGID);
+ return avc_has_perm(current_sid(), task_sid(p), SECCLASS_PROCESS,
+ PROCESS__SETPGID, NULL);
}
static int selinux_task_getpgid(struct task_struct *p)
{
- return current_has_perm(p, PROCESS__GETPGID);
+ return avc_has_perm(current_sid(), task_sid(p), SECCLASS_PROCESS,
+ PROCESS__GETPGID, NULL);
}
static int selinux_task_getsid(struct task_struct *p)
{
- return current_has_perm(p, PROCESS__GETSESSION);
+ return avc_has_perm(current_sid(), task_sid(p), SECCLASS_PROCESS,
+ PROCESS__GETSESSION, NULL);
}
static void selinux_task_getsecid(struct task_struct *p, u32 *secid)
@@ -3977,17 +3902,20 @@ static void selinux_task_getsecid(struct task_struct *p, u32 *secid)
static int selinux_task_setnice(struct task_struct *p, int nice)
{
- return current_has_perm(p, PROCESS__SETSCHED);
+ return avc_has_perm(current_sid(), task_sid(p), SECCLASS_PROCESS,
+ PROCESS__SETSCHED, NULL);
}
static int selinux_task_setioprio(struct task_struct *p, int ioprio)
{
- return current_has_perm(p, PROCESS__SETSCHED);
+ return avc_has_perm(current_sid(), task_sid(p), SECCLASS_PROCESS,
+ PROCESS__SETSCHED, NULL);
}
static int selinux_task_getioprio(struct task_struct *p)
{
- return current_has_perm(p, PROCESS__GETSCHED);
+ return avc_has_perm(current_sid(), task_sid(p), SECCLASS_PROCESS,
+ PROCESS__GETSCHED, NULL);
}
static int selinux_task_setrlimit(struct task_struct *p, unsigned int resource,
@@ -4000,47 +3928,48 @@ static int selinux_task_setrlimit(struct task_struct *p, unsigned int resource,
later be used as a safe reset point for the soft limit
upon context transitions. See selinux_bprm_committing_creds. */
if (old_rlim->rlim_max != new_rlim->rlim_max)
- return current_has_perm(p, PROCESS__SETRLIMIT);
+ return avc_has_perm(current_sid(), task_sid(p),
+ SECCLASS_PROCESS, PROCESS__SETRLIMIT, NULL);
return 0;
}
static int selinux_task_setscheduler(struct task_struct *p)
{
- return current_has_perm(p, PROCESS__SETSCHED);
+ return avc_has_perm(current_sid(), task_sid(p), SECCLASS_PROCESS,
+ PROCESS__SETSCHED, NULL);
}
static int selinux_task_getscheduler(struct task_struct *p)
{
- return current_has_perm(p, PROCESS__GETSCHED);
+ return avc_has_perm(current_sid(), task_sid(p), SECCLASS_PROCESS,
+ PROCESS__GETSCHED, NULL);
}
static int selinux_task_movememory(struct task_struct *p)
{
- return current_has_perm(p, PROCESS__SETSCHED);
+ return avc_has_perm(current_sid(), task_sid(p), SECCLASS_PROCESS,
+ PROCESS__SETSCHED, NULL);
}
static int selinux_task_kill(struct task_struct *p, struct siginfo *info,
int sig, u32 secid)
{
u32 perm;
- int rc;
if (!sig)
perm = PROCESS__SIGNULL; /* null signal; existence test */
else
perm = signal_to_av(sig);
- if (secid)
- rc = avc_has_perm(secid, task_sid(p),
- SECCLASS_PROCESS, perm, NULL);
- else
- rc = current_has_perm(p, perm);
- return rc;
+ if (!secid)
+ secid = current_sid();
+ return avc_has_perm(secid, task_sid(p), SECCLASS_PROCESS, perm, NULL);
}
static int selinux_task_wait(struct task_struct *p)
{
- return task_has_perm(p, current, PROCESS__SIGCHLD);
+ return avc_has_perm(task_sid(p), current_sid(), SECCLASS_PROCESS,
+ PROCESS__SIGCHLD, NULL);
}
static void selinux_task_to_inode(struct task_struct *p,
@@ -4330,12 +4259,11 @@ static int socket_sockcreate_sid(const struct task_security_struct *tsec,
socksid);
}
-static int sock_has_perm(struct task_struct *task, struct sock *sk, u32 perms)
+static int sock_has_perm(struct sock *sk, u32 perms)
{
struct sk_security_struct *sksec = sk->sk_security;
struct common_audit_data ad;
struct lsm_network_audit net = {0,};
- u32 tsid = task_sid(task);
if (sksec->sid == SECINITSID_KERNEL)
return 0;
@@ -4344,7 +4272,8 @@ static int sock_has_perm(struct task_struct *task, struct sock *sk, u32 perms)
ad.u.net = &net;
ad.u.net->sk = sk;
- return avc_has_perm(tsid, sksec->sid, sksec->sclass, perms, &ad);
+ return avc_has_perm(current_sid(), sksec->sid, sksec->sclass, perms,
+ &ad);
}
static int selinux_socket_create(int family, int type,
@@ -4406,7 +4335,7 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
u16 family;
int err;
- err = sock_has_perm(current, sk, SOCKET__BIND);
+ err = sock_has_perm(sk, SOCKET__BIND);
if (err)
goto out;
@@ -4505,7 +4434,7 @@ static int selinux_socket_connect(struct socket *sock, struct sockaddr *address,
struct sk_security_struct *sksec = sk->sk_security;
int err;
- err = sock_has_perm(current, sk, SOCKET__CONNECT);
+ err = sock_has_perm(sk, SOCKET__CONNECT);
if (err)
return err;
@@ -4557,7 +4486,7 @@ static int selinux_socket_connect(struct socket *sock, struct sockaddr *address,
static int selinux_socket_listen(struct socket *sock, int backlog)
{
- return sock_has_perm(current, sock->sk, SOCKET__LISTEN);
+ return sock_has_perm(sock->sk, SOCKET__LISTEN);
}
static int selinux_socket_accept(struct socket *sock, struct socket *newsock)
@@ -4568,7 +4497,7 @@ static int selinux_socket_accept(struct socket *sock, struct socket *newsock)
u16 sclass;
u32 sid;
- err = sock_has_perm(current, sock->sk, SOCKET__ACCEPT);
+ err = sock_has_perm(sock->sk, SOCKET__ACCEPT);
if (err)
return err;
@@ -4589,30 +4518,30 @@ static int selinux_socket_accept(struct socket *sock, struct socket *newsock)
static int selinux_socket_sendmsg(struct socket *sock, struct msghdr *msg,
int size)
{
- return sock_has_perm(current, sock->sk, SOCKET__WRITE);
+ return sock_has_perm(sock->sk, SOCKET__WRITE);
}
static int selinux_socket_recvmsg(struct socket *sock, struct msghdr *msg,
int size, int flags)
{
- return sock_has_perm(current, sock->sk, SOCKET__READ);
+ return sock_has_perm(sock->sk, SOCKET__READ);
}
static int selinux_socket_getsockname(struct socket *sock)
{
- return sock_has_perm(current, sock->sk, SOCKET__GETATTR);
+ return sock_has_perm(sock->sk, SOCKET__GETATTR);
}
static int selinux_socket_getpeername(struct socket *sock)
{
- return sock_has_perm(current, sock->sk, SOCKET__GETATTR);
+ return sock_has_perm(sock->sk, SOCKET__GETATTR);
}
static int selinux_socket_setsockopt(struct socket *sock, int level, int optname)
{
int err;
- err = sock_has_perm(current, sock->sk, SOCKET__SETOPT);
+ err = sock_has_perm(sock->sk, SOCKET__SETOPT);
if (err)
return err;
@@ -4622,12 +4551,12 @@ static int selinux_socket_setsockopt(struct socket *sock, int level, int optname
static int selinux_socket_getsockopt(struct socket *sock, int level,
int optname)
{
- return sock_has_perm(current, sock->sk, SOCKET__GETOPT);
+ return sock_has_perm(sock->sk, SOCKET__GETOPT);
}
static int selinux_socket_shutdown(struct socket *sock, int how)
{
- return sock_has_perm(current, sock->sk, SOCKET__SHUTDOWN);
+ return sock_has_perm(sock->sk, SOCKET__SHUTDOWN);
}
static int selinux_socket_unix_stream_connect(struct sock *sock,
@@ -5115,7 +5044,7 @@ static int selinux_nlmsg_perm(struct sock *sk, struct sk_buff *skb)
goto out;
}
- err = sock_has_perm(current, sk, perm);
+ err = sock_has_perm(sk, perm);
out:
return err;
}
@@ -5446,20 +5375,17 @@ static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb)
return selinux_nlmsg_perm(sk, skb);
}
-static int ipc_alloc_security(struct task_struct *task,
- struct kern_ipc_perm *perm,
+static int ipc_alloc_security(struct kern_ipc_perm *perm,
u16 sclass)
{
struct ipc_security_struct *isec;
- u32 sid;
isec = kzalloc(sizeof(struct ipc_security_struct), GFP_KERNEL);
if (!isec)
return -ENOMEM;
- sid = task_sid(task);
isec->sclass = sclass;
- isec->sid = sid;
+ isec->sid = current_sid();
perm->security = isec;
return 0;
@@ -5527,7 +5453,7 @@ static int selinux_msg_queue_alloc_security(struct msg_queue *msq)
u32 sid = current_sid();
int rc;
- rc = ipc_alloc_security(current, &msq->q_perm, SECCLASS_MSGQ);
+ rc = ipc_alloc_security(&msq->q_perm, SECCLASS_MSGQ);
if (rc)
return rc;
@@ -5574,7 +5500,8 @@ static int selinux_msg_queue_msgctl(struct msg_queue *msq, int cmd)
case IPC_INFO:
case MSG_INFO:
/* No specific object, just general system-wide information. */
- return task_has_system(current, SYSTEM__IPC_INFO);
+ return avc_has_perm(current_sid(), SECINITSID_KERNEL,
+ SECCLASS_SYSTEM, SYSTEM__IPC_INFO, NULL);
case IPC_STAT:
case MSG_STAT:
perms = MSGQ__GETATTR | MSGQ__ASSOCIATE;
@@ -5668,7 +5595,7 @@ static int selinux_shm_alloc_security(struct shmid_kernel *shp)
u32 sid = current_sid();
int rc;
- rc = ipc_alloc_security(current, &shp->shm_perm, SECCLASS_SHM);
+ rc = ipc_alloc_security(&shp->shm_perm, SECCLASS_SHM);
if (rc)
return rc;
@@ -5716,7 +5643,8 @@ static int selinux_shm_shmctl(struct shmid_kernel *shp, int cmd)
case IPC_INFO:
case SHM_INFO:
/* No specific object, just general system-wide information. */
- return task_has_system(current, SYSTEM__IPC_INFO);
+ return avc_has_perm(current_sid(), SECINITSID_KERNEL,
+ SECCLASS_SYSTEM, SYSTEM__IPC_INFO, NULL);
case IPC_STAT:
case SHM_STAT:
perms = SHM__GETATTR | SHM__ASSOCIATE;
@@ -5760,7 +5688,7 @@ static int selinux_sem_alloc_security(struct sem_array *sma)
u32 sid = current_sid();
int rc;
- rc = ipc_alloc_security(current, &sma->sem_perm, SECCLASS_SEM);
+ rc = ipc_alloc_security(&sma->sem_perm, SECCLASS_SEM);
if (rc)
return rc;
@@ -5808,7 +5736,8 @@ static int selinux_sem_semctl(struct sem_array *sma, int cmd)
case IPC_INFO:
case SEM_INFO:
/* No specific object, just general system-wide information. */
- return task_has_system(current, SYSTEM__IPC_INFO);
+ return avc_has_perm(current_sid(), SECINITSID_KERNEL,
+ SECCLASS_SYSTEM, SYSTEM__IPC_INFO, NULL);
case GETPID:
case GETNCNT:
case GETZCNT:
@@ -5889,15 +5818,16 @@ static int selinux_getprocattr(struct task_struct *p,
int error;
unsigned len;
+ rcu_read_lock();
+ __tsec = __task_cred(p)->security;
+
if (current != p) {
- error = current_has_perm(p, PROCESS__GETATTR);
+ error = avc_has_perm(current_sid(), __tsec->sid,
+ SECCLASS_PROCESS, PROCESS__GETATTR, NULL);
if (error)
- return error;
+ goto bad;
}
- rcu_read_lock();
- __tsec = __task_cred(p)->security;
-
if (!strcmp(name, "current"))
sid = __tsec->sid;
else if (!strcmp(name, "prev"))
@@ -5910,8 +5840,10 @@ static int selinux_getprocattr(struct task_struct *p,
sid = __tsec->keycreate_sid;
else if (!strcmp(name, "sockcreate"))
sid = __tsec->sockcreate_sid;
- else
- goto invalid;
+ else {
+ error = -EINVAL;
+ goto bad;
+ }
rcu_read_unlock();
if (!sid)
@@ -5922,9 +5854,9 @@ static int selinux_getprocattr(struct task_struct *p,
return error;
return len;
-invalid:
+bad:
rcu_read_unlock();
- return -EINVAL;
+ return error;
}
static int selinux_setprocattr(struct task_struct *p,
@@ -5932,31 +5864,38 @@ static int selinux_setprocattr(struct task_struct *p,
{
struct task_security_struct *tsec;
struct cred *new;
- u32 sid = 0, ptsid;
+ u32 mysid = current_sid(), sid = 0, ptsid;
int error;
char *str = value;
if (current != p) {
- /* SELinux only allows a process to change its own
- security attributes. */
+ /*
+ * 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.
- * current == p, but we'll pass them separately in case the
- * above restriction is ever removed.
*/
if (!strcmp(name, "exec"))
- error = current_has_perm(p, PROCESS__SETEXEC);
+ error = avc_has_perm(mysid, mysid, SECCLASS_PROCESS,
+ PROCESS__SETEXEC, NULL);
else if (!strcmp(name, "fscreate"))
- error = current_has_perm(p, PROCESS__SETFSCREATE);
+ error = avc_has_perm(mysid, mysid, SECCLASS_PROCESS,
+ PROCESS__SETFSCREATE, NULL);
else if (!strcmp(name, "keycreate"))
- error = current_has_perm(p, PROCESS__SETKEYCREATE);
+ error = avc_has_perm(mysid, mysid, SECCLASS_PROCESS,
+ PROCESS__SETKEYCREATE, NULL);
else if (!strcmp(name, "sockcreate"))
- error = current_has_perm(p, PROCESS__SETSOCKCREATE);
+ error = avc_has_perm(mysid, mysid, SECCLASS_PROCESS,
+ PROCESS__SETSOCKCREATE, NULL);
else if (!strcmp(name, "current"))
- error = current_has_perm(p, PROCESS__SETCURRENT);
+ error = avc_has_perm(mysid, mysid, SECCLASS_PROCESS,
+ PROCESS__SETCURRENT, NULL);
else
error = -EINVAL;
if (error)
@@ -6010,7 +5949,8 @@ static int selinux_setprocattr(struct task_struct *p,
} else if (!strcmp(name, "fscreate")) {
tsec->create_sid = sid;
} else if (!strcmp(name, "keycreate")) {
- error = may_create_key(sid, p);
+ error = avc_has_perm(mysid, sid, SECCLASS_KEY, KEY__CREATE,
+ NULL);
if (error)
goto abort_change;
tsec->keycreate_sid = sid;
@@ -6037,7 +5977,7 @@ static int selinux_setprocattr(struct task_struct *p,
/* Check for ptracing, and update the task SID if ok.
Otherwise, leave SID unchanged and fail. */
- ptsid = ptrace_parent_sid(p);
+ ptsid = ptrace_parent_sid();
if (ptsid != 0) {
error = avc_has_perm(ptsid, sid, SECCLASS_PROCESS,
PROCESS__PTRACE, NULL);
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index e8dab0f..c03cdcd 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -37,6 +37,16 @@ struct task_security_struct {
u32 sockcreate_sid; /* fscreate SID */
};
+/*
+ * get the subjective security ID of the current task
+ */
+static inline u32 current_sid(void)
+{
+ const struct task_security_struct *tsec = current_security();
+
+ return tsec->sid;
+}
+
enum label_initialized {
LABEL_INVALID, /* invalid or not initialized */
LABEL_INITIALIZED, /* initialized */
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 0aac402..55345f8 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -77,25 +77,6 @@ static char policy_opened;
/* global data for policy capabilities */
static struct dentry *policycap_dir;
-/* Check whether a task is allowed to use a security operation. */
-static int task_has_security(struct task_struct *tsk,
- u32 perms)
-{
- const struct task_security_struct *tsec;
- u32 sid = 0;
-
- rcu_read_lock();
- tsec = __task_cred(tsk)->security;
- if (tsec)
- sid = tsec->sid;
- rcu_read_unlock();
- if (!tsec)
- return -EACCES;
-
- return avc_has_perm(sid, SECINITSID_SECURITY,
- SECCLASS_SECURITY, perms, NULL);
-}
-
enum sel_inos {
SEL_ROOT_INO = 2,
SEL_LOAD, /* load policy */
@@ -166,7 +147,9 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf,
new_value = !!new_value;
if (new_value != selinux_enforcing) {
- length = task_has_security(current, SECURITY__SETENFORCE);
+ length = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__SETENFORCE,
+ NULL);
if (length)
goto out;
audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STATUS,
@@ -368,7 +351,8 @@ static int sel_open_policy(struct inode *inode, struct file *filp)
mutex_lock(&sel_mutex);
- rc = task_has_security(current, SECURITY__READ_POLICY);
+ rc = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__READ_POLICY, NULL);
if (rc)
goto err;
@@ -429,7 +413,8 @@ static ssize_t sel_read_policy(struct file *filp, char __user *buf,
mutex_lock(&sel_mutex);
- ret = task_has_security(current, SECURITY__READ_POLICY);
+ ret = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__READ_POLICY, NULL);
if (ret)
goto out;
@@ -499,7 +484,8 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
mutex_lock(&sel_mutex);
- length = task_has_security(current, SECURITY__LOAD_POLICY);
+ length = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__LOAD_POLICY, NULL);
if (length)
goto out;
@@ -561,7 +547,8 @@ static ssize_t sel_write_context(struct file *file, char *buf, size_t size)
u32 sid, len;
ssize_t length;
- length = task_has_security(current, SECURITY__CHECK_CONTEXT);
+ length = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__CHECK_CONTEXT, NULL);
if (length)
goto out;
@@ -604,7 +591,9 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf,
ssize_t length;
unsigned int new_value;
- length = task_has_security(current, SECURITY__SETCHECKREQPROT);
+ length = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__SETCHECKREQPROT,
+ NULL);
if (length)
return length;
@@ -645,7 +634,8 @@ static ssize_t sel_write_validatetrans(struct file *file,
u16 tclass;
int rc;
- rc = task_has_security(current, SECURITY__VALIDATE_TRANS);
+ rc = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__VALIDATE_TRANS, NULL);
if (rc)
goto out;
@@ -772,7 +762,8 @@ static ssize_t sel_write_access(struct file *file, char *buf, size_t size)
struct av_decision avd;
ssize_t length;
- length = task_has_security(current, SECURITY__COMPUTE_AV);
+ length = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__COMPUTE_AV, NULL);
if (length)
goto out;
@@ -822,7 +813,9 @@ static ssize_t sel_write_create(struct file *file, char *buf, size_t size)
u32 len;
int nargs;
- length = task_has_security(current, SECURITY__COMPUTE_CREATE);
+ length = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__COMPUTE_CREATE,
+ NULL);
if (length)
goto out;
@@ -919,7 +912,9 @@ static ssize_t sel_write_relabel(struct file *file, char *buf, size_t size)
char *newcon = NULL;
u32 len;
- length = task_has_security(current, SECURITY__COMPUTE_RELABEL);
+ length = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__COMPUTE_RELABEL,
+ NULL);
if (length)
goto out;
@@ -975,7 +970,9 @@ static ssize_t sel_write_user(struct file *file, char *buf, size_t size)
int i, rc;
u32 len, nsids;
- length = task_has_security(current, SECURITY__COMPUTE_USER);
+ length = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__COMPUTE_USER,
+ NULL);
if (length)
goto out;
@@ -1035,7 +1032,9 @@ static ssize_t sel_write_member(struct file *file, char *buf, size_t size)
char *newcon = NULL;
u32 len;
- length = task_has_security(current, SECURITY__COMPUTE_MEMBER);
+ length = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__COMPUTE_MEMBER,
+ NULL);
if (length)
goto out;
@@ -1142,7 +1141,9 @@ static ssize_t sel_write_bool(struct file *filep, const char __user *buf,
mutex_lock(&sel_mutex);
- length = task_has_security(current, SECURITY__SETBOOL);
+ length = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__SETBOOL,
+ NULL);
if (length)
goto out;
@@ -1198,7 +1199,9 @@ static ssize_t sel_commit_bools_write(struct file *filep,
mutex_lock(&sel_mutex);
- length = task_has_security(current, SECURITY__SETBOOL);
+ length = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__SETBOOL,
+ NULL);
if (length)
goto out;
@@ -1351,7 +1354,9 @@ static ssize_t sel_write_avc_cache_threshold(struct file *file,
ssize_t ret;
unsigned int new_value;
- ret = task_has_security(current, SECURITY__SETSECPARAM);
+ ret = avc_has_perm(current_sid(), SECINITSID_SECURITY,
+ SECCLASS_SECURITY, SECURITY__SETSECPARAM,
+ NULL);
if (ret)
return ret;
--
2.7.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 2/2] proc, security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-16 17:41 [PATCH 1/2] selinux: clean up cred usage and simplify Stephen Smalley
@ 2016-12-16 17:41 ` Stephen Smalley
2016-12-16 18:38 ` [PATCH 2/2] proc,security: " Casey Schaufler
` (4 more replies)
2016-12-16 22:02 ` [PATCH 1/2] selinux: clean up cred usage and simplify Paul Moore
1 sibling, 5 replies; 48+ messages in thread
From: Stephen Smalley @ 2016-12-16 17:41 UTC (permalink / raw)
To: selinux
Cc: paul, james.l.morris, linux-security-module, casey,
john.johansen, Stephen Smalley
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>
---
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(¤t->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(¤t->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
^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-16 17:41 ` [PATCH 2/2] proc, security: move restriction on writing /proc/pid/attr nodes to proc Stephen Smalley
@ 2016-12-16 18:38 ` Casey Schaufler
2016-12-16 19:06 ` John Johansen
` (3 subsequent siblings)
4 siblings, 0 replies; 48+ messages in thread
From: Casey Schaufler @ 2016-12-16 18:38 UTC (permalink / raw)
To: Stephen Smalley, selinux
Cc: paul, james.l.morris, linux-security-module, john.johansen
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 <sds@tycho.nsa.gov>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> 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(¤t->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(¤t->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;
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-16 17:41 ` [PATCH 2/2] proc, security: move restriction on writing /proc/pid/attr nodes to proc Stephen Smalley
2016-12-16 18:38 ` [PATCH 2/2] proc,security: " Casey Schaufler
@ 2016-12-16 19:06 ` John Johansen
2016-12-16 22:06 ` Paul Moore
` (2 subsequent siblings)
4 siblings, 0 replies; 48+ messages in thread
From: John Johansen @ 2016-12-16 19:06 UTC (permalink / raw)
To: Stephen Smalley, selinux
Cc: paul, james.l.morris, linux-security-module, casey
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 <sds@tycho.nsa.gov>
Acked-by: John Johansen <john.johansen@canonical.com>
> ---
> 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(¤t->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(¤t->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;
>
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/2] selinux: clean up cred usage and simplify
2016-12-16 17:41 [PATCH 1/2] selinux: clean up cred usage and simplify Stephen Smalley
2016-12-16 17:41 ` [PATCH 2/2] proc, security: move restriction on writing /proc/pid/attr nodes to proc Stephen Smalley
@ 2016-12-16 22:02 ` Paul Moore
1 sibling, 0 replies; 48+ messages in thread
From: Paul Moore @ 2016-12-16 22:02 UTC (permalink / raw)
To: Stephen Smalley
Cc: selinux, James Morris, linux-security-module, casey, john.johansen
On Fri, Dec 16, 2016 at 12:41 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> SELinux was sometimes using the task "objective" credentials when
> it could/should use the "subjective" credentials. This was sometimes
> hidden by the fact that we were unnecessarily passing around pointers
> to the current task, making it appear as if the task could be something
> other than current, so eliminate all such passing of current. Inline
> various permission checking helper functions that can be reduced to a
> single avc_has_perm() call.
>
> Since the credentials infrastructure only allows a task to alter
> its own credentials, we can always assume that current must be the same
> as the target task in selinux_setprocattr after the check. We likely
> should move this check from selinux_setprocattr() to proc_pid_attr_write()
> and drop the task argument to the security hook altogether; it can only
> serve to confuse things.
>
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/hooks.c | 294 +++++++++++++++-----------------------
> security/selinux/include/objsec.h | 10 ++
> security/selinux/selinuxfs.c | 73 +++++-----
> 3 files changed, 166 insertions(+), 211 deletions(-)
Merged, thanks.
Looking at this patch, I think getting rid of the wrappers was the
right way to go from a readability perspective.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-16 17:41 ` [PATCH 2/2] proc, security: move restriction on writing /proc/pid/attr nodes to proc Stephen Smalley
2016-12-16 18:38 ` [PATCH 2/2] proc,security: " Casey Schaufler
2016-12-16 19:06 ` John Johansen
@ 2016-12-16 22:06 ` Paul Moore
2016-12-16 22:13 ` Casey Schaufler
2016-12-19 9:44 ` José Bollo
2016-12-21 2:37 ` Paul Moore
4 siblings, 1 reply; 48+ messages in thread
From: Paul Moore @ 2016-12-16 22:06 UTC (permalink / raw)
To: Stephen Smalley, casey, john.johansen, James Morris
Cc: selinux, linux-security-module
On Fri, Dec 16, 2016 at 12:41 PM, Stephen Smalley <sds@tycho.nsa.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 <sds@tycho.nsa.gov>
> ---
> 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 <paul@paul-moore.com>
> 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(¤t->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(¤t->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
>
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-16 22:06 ` Paul Moore
@ 2016-12-16 22:13 ` Casey Schaufler
2016-12-20 1:30 ` Paul Moore
0 siblings, 1 reply; 48+ messages in thread
From: Casey Schaufler @ 2016-12-16 22:13 UTC (permalink / raw)
To: Paul Moore, Stephen Smalley, john.johansen, James Morris
Cc: selinux, linux-security-module
On 12/16/2016 2:06 PM, Paul Moore wrote:
> On Fri, Dec 16, 2016 at 12:41 PM, Stephen Smalley <sds@tycho.nsa.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 <sds@tycho.nsa.gov>
>> ---
>> 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 <paul@paul-moore.com>
>
>> 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(¤t->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(¤t->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
>>
>
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-16 17:41 ` [PATCH 2/2] proc, security: move restriction on writing /proc/pid/attr nodes to proc Stephen Smalley
` (2 preceding siblings ...)
2016-12-16 22:06 ` Paul Moore
@ 2016-12-19 9:44 ` José Bollo
2016-12-19 14:33 ` Stephen Smalley
2016-12-20 16:14 ` Stephen Smalley
2016-12-21 2:37 ` Paul Moore
4 siblings, 2 replies; 48+ messages in thread
From: José Bollo @ 2016-12-19 9:44 UTC (permalink / raw)
To: Stephen Smalley, selinux
Cc: paul, james.l.morris, linux-security-module, casey, john.johansen
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 <sds@tycho.nsa.gov>
> ---
> 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(¤t->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(¤t->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;
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 9:44 ` José Bollo
@ 2016-12-19 14:33 ` Stephen Smalley
2016-12-19 15:00 ` José Bollo
2016-12-19 15:41 ` José Bollo
2016-12-20 16:14 ` Stephen Smalley
1 sibling, 2 replies; 48+ messages in thread
From: Stephen Smalley @ 2016-12-19 14:33 UTC (permalink / raw)
To: José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module, casey, john.johansen
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 <sds@tycho.nsa.gov>
> > ---
> > 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(¤t->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(¤t->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;
> >
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 14:33 ` Stephen Smalley
@ 2016-12-19 15:00 ` José Bollo
2016-12-19 15:41 ` José Bollo
1 sibling, 0 replies; 48+ messages in thread
From: José Bollo @ 2016-12-19 15:00 UTC (permalink / raw)
To: Stephen Smalley, selinux
Cc: paul, james.l.morris, linux-security-module, casey, john.johansen
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 <sds@tycho.nsa.gov>
> > > ---
> > > 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(¤t->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(¤t->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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 14:33 ` Stephen Smalley
2016-12-19 15:00 ` José Bollo
@ 2016-12-19 15:41 ` José Bollo
2016-12-19 15:52 ` Stephen Smalley
1 sibling, 1 reply; 48+ messages in thread
From: José Bollo @ 2016-12-19 15:41 UTC (permalink / raw)
To: Stephen Smalley, selinux
Cc: paul, james.l.morris, linux-security-module, casey, john.johansen
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 <sds@tycho.nsa.gov>
> > > ---
> > > 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(¤t->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(¤t->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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 15:41 ` José Bollo
@ 2016-12-19 15:52 ` Stephen Smalley
2016-12-19 16:32 ` Casey Schaufler
0 siblings, 1 reply; 48+ messages in thread
From: Stephen Smalley @ 2016-12-19 15:52 UTC (permalink / raw)
To: José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module, casey, john.johansen
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 <sds@tycho.nsa.gov>
> > > > ---
> > > > 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(¤t->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(¤t->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 majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 15:52 ` Stephen Smalley
@ 2016-12-19 16:32 ` Casey Schaufler
2016-12-19 17:09 ` Stephen Smalley
2016-12-19 20:36 ` John Johansen
0 siblings, 2 replies; 48+ messages in thread
From: Casey Schaufler @ 2016-12-19 16:32 UTC (permalink / raw)
To: Stephen Smalley, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module, john.johansen
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 <sds@tycho.nsa.gov>
>>>>> ---
>>>>> 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(¤t->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(¤t->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 majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 16:32 ` Casey Schaufler
@ 2016-12-19 17:09 ` Stephen Smalley
2016-12-19 18:00 ` Casey Schaufler
2016-12-19 18:12 ` José Bollo
2016-12-19 20:36 ` John Johansen
1 sibling, 2 replies; 48+ messages in thread
From: Stephen Smalley @ 2016-12-19 17:09 UTC (permalink / raw)
To: Casey Schaufler, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module, john.johansen
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 17:09 ` Stephen Smalley
@ 2016-12-19 18:00 ` Casey Schaufler
2016-12-19 18:18 ` José Bollo
2016-12-19 18:12 ` José Bollo
1 sibling, 1 reply; 48+ messages in thread
From: Casey Schaufler @ 2016-12-19 18:00 UTC (permalink / raw)
To: Stephen Smalley, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module, john.johansen
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 17:09 ` Stephen Smalley
2016-12-19 18:00 ` Casey Schaufler
@ 2016-12-19 18:12 ` José Bollo
1 sibling, 0 replies; 48+ messages in thread
From: José Bollo @ 2016-12-19 18:12 UTC (permalink / raw)
To: Stephen Smalley, Casey Schaufler, selinux
Cc: paul, james.l.morris, linux-security-module, john.johansen
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 18:00 ` Casey Schaufler
@ 2016-12-19 18:18 ` José Bollo
0 siblings, 0 replies; 48+ messages in thread
From: José Bollo @ 2016-12-19 18:18 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley, selinux
Cc: paul, james.l.morris, linux-security-module, john.johansen
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
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 16:32 ` Casey Schaufler
2016-12-19 17:09 ` Stephen Smalley
@ 2016-12-19 20:36 ` John Johansen
2016-12-19 21:25 ` Casey Schaufler
1 sibling, 1 reply; 48+ messages in thread
From: John Johansen @ 2016-12-19 20:36 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 20:36 ` John Johansen
@ 2016-12-19 21:25 ` Casey Schaufler
2016-12-19 21:46 ` [PATCH 2/2] proc, security: move restriction on writing/proc/pid/attr " Tetsuo Handa
` (2 more replies)
0 siblings, 3 replies; 48+ messages in thread
From: Casey Schaufler @ 2016-12-19 21:25 UTC (permalink / raw)
To: John Johansen, Stephen Smalley, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module
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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc, security: move restriction on writing/proc/pid/attr nodes to proc
2016-12-19 21:25 ` Casey Schaufler
@ 2016-12-19 21:46 ` Tetsuo Handa
2016-12-19 21:50 ` [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr " Stephen Smalley
2016-12-20 14:40 ` José Bollo
2 siblings, 0 replies; 48+ messages in thread
From: Tetsuo Handa @ 2016-12-19 21:46 UTC (permalink / raw)
To: casey, john.johansen, sds, jobol, selinux
Cc: paul, james.l.morris, linux-security-module
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 21:25 ` Casey Schaufler
2016-12-19 21:46 ` [PATCH 2/2] proc, security: move restriction on writing/proc/pid/attr " Tetsuo Handa
@ 2016-12-19 21:50 ` Stephen Smalley
2016-12-19 22:31 ` Casey Schaufler
` (2 more replies)
2016-12-20 14:40 ` José Bollo
2 siblings, 3 replies; 48+ messages in thread
From: Stephen Smalley @ 2016-12-19 21:50 UTC (permalink / raw)
To: Casey Schaufler, John Johansen, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 21:50 ` [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr " Stephen Smalley
@ 2016-12-19 22:31 ` Casey Schaufler
2016-12-19 22:45 ` John Johansen
2016-12-20 1:23 ` Paul Moore
2 siblings, 0 replies; 48+ messages in thread
From: Casey Schaufler @ 2016-12-19 22:31 UTC (permalink / raw)
To: Stephen Smalley, John Johansen, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 21:50 ` [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr " Stephen Smalley
2016-12-19 22:31 ` Casey Schaufler
@ 2016-12-19 22:45 ` John Johansen
2016-12-19 22:49 ` Casey Schaufler
2016-12-20 1:23 ` Paul Moore
2 siblings, 1 reply; 48+ messages in thread
From: John Johansen @ 2016-12-19 22:45 UTC (permalink / raw)
To: Stephen Smalley, Casey Schaufler, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module
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.
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 22:45 ` John Johansen
@ 2016-12-19 22:49 ` Casey Schaufler
2016-12-20 1:27 ` Paul Moore
0 siblings, 1 reply; 48+ messages in thread
From: Casey Schaufler @ 2016-12-19 22:49 UTC (permalink / raw)
To: John Johansen, Stephen Smalley, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 21:50 ` [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr " Stephen Smalley
2016-12-19 22:31 ` Casey Schaufler
2016-12-19 22:45 ` John Johansen
@ 2016-12-20 1:23 ` Paul Moore
2016-12-20 1:59 ` Casey Schaufler
2 siblings, 1 reply; 48+ messages in thread
From: Paul Moore @ 2016-12-20 1:23 UTC (permalink / raw)
To: Stephen Smalley, Casey Schaufler
Cc: John Johansen, José Bollo, selinux, James Morris,
linux-security-module
On Mon, Dec 19, 2016 at 4:50 PM, Stephen Smalley <sds@tycho.nsa.gov> 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
www.paul-moore.com
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 22:49 ` Casey Schaufler
@ 2016-12-20 1:27 ` Paul Moore
0 siblings, 0 replies; 48+ messages in thread
From: Paul Moore @ 2016-12-20 1:27 UTC (permalink / raw)
To: Casey Schaufler
Cc: John Johansen, Stephen Smalley, José Bollo, selinux,
James Morris, linux-security-module
On Mon, Dec 19, 2016 at 5:49 PM, Casey Schaufler <casey@schaufler-ca.com> 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
www.paul-moore.com
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-16 22:13 ` Casey Schaufler
@ 2016-12-20 1:30 ` Paul Moore
2016-12-20 1:51 ` Casey Schaufler
2016-12-20 10:36 ` José Bollo
0 siblings, 2 replies; 48+ messages in thread
From: Paul Moore @ 2016-12-20 1:30 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley
Cc: john.johansen, James Morris, selinux, linux-security-module
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 <sds@tycho.nsa.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 <sds@tycho.nsa.gov>
>>> ---
>>> 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.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 1:30 ` Paul Moore
@ 2016-12-20 1:51 ` Casey Schaufler
2016-12-20 10:36 ` José Bollo
1 sibling, 0 replies; 48+ messages in thread
From: Casey Schaufler @ 2016-12-20 1:51 UTC (permalink / raw)
To: Paul Moore, Stephen Smalley
Cc: john.johansen, James Morris, selinux, linux-security-module
On 12/19/2016 5:30 PM, Paul Moore wrote:
> 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 <sds@tycho.nsa.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 <sds@tycho.nsa.gov>
>>>> ---
>>>> 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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 1:23 ` Paul Moore
@ 2016-12-20 1:59 ` Casey Schaufler
0 siblings, 0 replies; 48+ messages in thread
From: Casey Schaufler @ 2016-12-20 1:59 UTC (permalink / raw)
To: Paul Moore, Stephen Smalley
Cc: John Johansen, José Bollo, selinux, James Morris,
linux-security-module, José Bollo, Tetsuo Handa
On 12/19/2016 5:23 PM, Paul Moore wrote:
> On Mon, Dec 19, 2016 at 4:50 PM, Stephen Smalley <sds@tycho.nsa.gov> 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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 1:30 ` Paul Moore
2016-12-20 1:51 ` Casey Schaufler
@ 2016-12-20 10:36 ` José Bollo
2016-12-20 11:13 ` [PATCH 2/2] proc, security: " Tetsuo Handa
1 sibling, 1 reply; 48+ messages in thread
From: José Bollo @ 2016-12-20 10:36 UTC (permalink / raw)
To: Paul Moore, Casey Schaufler, Stephen Smalley
Cc: john.johansen, James Morris, selinux, linux-security-module
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 <sds@tycho.nsa.
> > > 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 <sds@tycho.nsa.gov>
> > > > ---
> > > > 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
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc, security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 10:36 ` José Bollo
@ 2016-12-20 11:13 ` Tetsuo Handa
2016-12-20 12:14 ` [PATCH 2/2] proc,security: " José Bollo
0 siblings, 1 reply; 48+ messages in thread
From: Tetsuo Handa @ 2016-12-20 11:13 UTC (permalink / raw)
To: jobol
Cc: paul, casey, sds, john.johansen, james.l.morris, selinux,
linux-security-module
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/00f80c77-9623-7e9e-8980-63b362a4f16c@schaufler-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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 11:13 ` [PATCH 2/2] proc, security: " Tetsuo Handa
@ 2016-12-20 12:14 ` José Bollo
0 siblings, 0 replies; 48+ messages in thread
From: José Bollo @ 2016-12-20 12:14 UTC (permalink / raw)
To: Tetsuo Handa
Cc: paul, casey, sds, john.johansen, james.l.morris, selinux,
linux-security-module
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é
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 21:25 ` Casey Schaufler
2016-12-19 21:46 ` [PATCH 2/2] proc, security: move restriction on writing/proc/pid/attr " Tetsuo Handa
2016-12-19 21:50 ` [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr " Stephen Smalley
@ 2016-12-20 14:40 ` José Bollo
2016-12-20 16:21 ` Casey Schaufler
2 siblings, 1 reply; 48+ messages in thread
From: José Bollo @ 2016-12-20 14:40 UTC (permalink / raw)
To: Casey Schaufler, John Johansen, Stephen Smalley, selinux
Cc: paul, james.l.morris, linux-security-module
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
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-19 9:44 ` José Bollo
2016-12-19 14:33 ` Stephen Smalley
@ 2016-12-20 16:14 ` Stephen Smalley
2016-12-20 16:39 ` José Bollo
1 sibling, 1 reply; 48+ messages in thread
From: Stephen Smalley @ 2016-12-20 16:14 UTC (permalink / raw)
To: José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module, casey, john.johansen
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 14:40 ` José Bollo
@ 2016-12-20 16:21 ` Casey Schaufler
0 siblings, 0 replies; 48+ messages in thread
From: Casey Schaufler @ 2016-12-20 16:21 UTC (permalink / raw)
To: José Bollo, John Johansen, Stephen Smalley, selinux
Cc: paul, james.l.morris, linux-security-module
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 16:14 ` Stephen Smalley
@ 2016-12-20 16:39 ` José Bollo
2016-12-20 16:50 ` Stephen Smalley
0 siblings, 1 reply; 48+ messages in thread
From: José Bollo @ 2016-12-20 16:39 UTC (permalink / raw)
To: Stephen Smalley, selinux
Cc: paul, james.l.morris, linux-security-module, casey, john.johansen
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é
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 16:39 ` José Bollo
@ 2016-12-20 16:50 ` Stephen Smalley
2016-12-20 18:17 ` Casey Schaufler
0 siblings, 1 reply; 48+ messages in thread
From: Stephen Smalley @ 2016-12-20 16:50 UTC (permalink / raw)
To: José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module, casey, john.johansen
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?
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 16:50 ` Stephen Smalley
@ 2016-12-20 18:17 ` Casey Schaufler
2016-12-20 18:28 ` Stephen Smalley
0 siblings, 1 reply; 48+ messages in thread
From: Casey Schaufler @ 2016-12-20 18:17 UTC (permalink / raw)
To: Stephen Smalley, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module, john.johansen
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?
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 18:17 ` Casey Schaufler
@ 2016-12-20 18:28 ` Stephen Smalley
2016-12-20 19:07 ` Casey Schaufler
0 siblings, 1 reply; 48+ messages in thread
From: Stephen Smalley @ 2016-12-20 18:28 UTC (permalink / raw)
To: Casey Schaufler, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module, john.johansen
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).
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 18:28 ` Stephen Smalley
@ 2016-12-20 19:07 ` Casey Schaufler
2016-12-20 19:35 ` Stephen Smalley
0 siblings, 1 reply; 48+ messages in thread
From: Casey Schaufler @ 2016-12-20 19:07 UTC (permalink / raw)
To: Stephen Smalley, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module, john.johansen
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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 19:07 ` Casey Schaufler
@ 2016-12-20 19:35 ` Stephen Smalley
2016-12-20 20:03 ` Casey Schaufler
0 siblings, 1 reply; 48+ messages in thread
From: Stephen Smalley @ 2016-12-20 19:35 UTC (permalink / raw)
To: Casey Schaufler, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module, john.johansen
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 19:35 ` Stephen Smalley
@ 2016-12-20 20:03 ` Casey Schaufler
2016-12-20 21:22 ` José Bollo
` (2 more replies)
0 siblings, 3 replies; 48+ messages in thread
From: Casey Schaufler @ 2016-12-20 20:03 UTC (permalink / raw)
To: Stephen Smalley, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module, john.johansen
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 20:03 ` Casey Schaufler
@ 2016-12-20 21:22 ` José Bollo
2016-12-20 21:35 ` Stephen Smalley
2016-12-20 21:38 ` John Johansen
2 siblings, 0 replies; 48+ messages in thread
From: José Bollo @ 2016-12-20 21:22 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley, selinux
Cc: paul, james.l.morris, linux-security-module, john.johansen
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 20:03 ` Casey Schaufler
2016-12-20 21:22 ` José Bollo
@ 2016-12-20 21:35 ` Stephen Smalley
2016-12-20 21:38 ` John Johansen
2 siblings, 0 replies; 48+ messages in thread
From: Stephen Smalley @ 2016-12-20 21:35 UTC (permalink / raw)
To: Casey Schaufler, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module, john.johansen
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-20 20:03 ` Casey Schaufler
2016-12-20 21:22 ` José Bollo
2016-12-20 21:35 ` Stephen Smalley
@ 2016-12-20 21:38 ` John Johansen
2 siblings, 0 replies; 48+ messages in thread
From: John Johansen @ 2016-12-20 21:38 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley, José Bollo, selinux
Cc: paul, james.l.morris, linux-security-module
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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-16 17:41 ` [PATCH 2/2] proc, security: move restriction on writing /proc/pid/attr nodes to proc Stephen Smalley
` (3 preceding siblings ...)
2016-12-19 9:44 ` José Bollo
@ 2016-12-21 2:37 ` Paul Moore
2016-12-21 7:04 ` José Bollo
4 siblings, 1 reply; 48+ messages in thread
From: Paul Moore @ 2016-12-21 2:37 UTC (permalink / raw)
To: Stephen Smalley
Cc: selinux, James Morris, linux-security-module, casey, john.johansen
On Fri, Dec 16, 2016 at 12:41 PM, Stephen Smalley <sds@tycho.nsa.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 <sds@tycho.nsa.gov>
> ---
> 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(¤t->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(¤t->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
>
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-21 2:37 ` Paul Moore
@ 2016-12-21 7:04 ` José Bollo
2016-12-21 15:15 ` Paul Moore
0 siblings, 1 reply; 48+ messages in thread
From: José Bollo @ 2016-12-21 7:04 UTC (permalink / raw)
To: Paul Moore, Stephen Smalley
Cc: selinux, James Morris, linux-security-module, casey, john.johansen
Le mardi 20 décembre 2016 à 21:37 -0500, Paul Moore a écrit :
> On Fri, Dec 16, 2016 at 12:41 PM, Stephen Smalley <sds@tycho.nsa.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 <sds@tycho.nsa.gov>
> > ---
> > 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(¤t->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(¤t->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
> >
>
>
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc
2016-12-21 7:04 ` José Bollo
@ 2016-12-21 15:15 ` Paul Moore
0 siblings, 0 replies; 48+ messages in thread
From: Paul Moore @ 2016-12-21 15:15 UTC (permalink / raw)
To: José Bollo
Cc: Stephen Smalley, selinux, James Morris, linux-security-module,
casey, john.johansen
On Wed, Dec 21, 2016 at 2:04 AM, José Bollo <jobol@nonadev.net> 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 <sds@tycho.nsa.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 <sds@tycho.nsa.gov>
>> > ---
>> > 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.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 48+ messages in thread
end of thread, other threads:[~2016-12-21 15:16 UTC | newest]
Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-16 17:41 [PATCH 1/2] selinux: clean up cred usage and simplify Stephen Smalley
2016-12-16 17:41 ` [PATCH 2/2] proc, security: move restriction on writing /proc/pid/attr nodes to proc Stephen Smalley
2016-12-16 18:38 ` [PATCH 2/2] proc,security: " Casey Schaufler
2016-12-16 19:06 ` John Johansen
2016-12-16 22:06 ` Paul Moore
2016-12-16 22:13 ` Casey Schaufler
2016-12-20 1:30 ` Paul Moore
2016-12-20 1:51 ` Casey Schaufler
2016-12-20 10:36 ` José Bollo
2016-12-20 11:13 ` [PATCH 2/2] proc, security: " Tetsuo Handa
2016-12-20 12:14 ` [PATCH 2/2] proc,security: " José Bollo
2016-12-19 9:44 ` José Bollo
2016-12-19 14:33 ` Stephen Smalley
2016-12-19 15:00 ` José Bollo
2016-12-19 15:41 ` José Bollo
2016-12-19 15:52 ` Stephen Smalley
2016-12-19 16:32 ` Casey Schaufler
2016-12-19 17:09 ` Stephen Smalley
2016-12-19 18:00 ` Casey Schaufler
2016-12-19 18:18 ` José Bollo
2016-12-19 18:12 ` José Bollo
2016-12-19 20:36 ` John Johansen
2016-12-19 21:25 ` Casey Schaufler
2016-12-19 21:46 ` [PATCH 2/2] proc, security: move restriction on writing/proc/pid/attr " Tetsuo Handa
2016-12-19 21:50 ` [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr " Stephen Smalley
2016-12-19 22:31 ` Casey Schaufler
2016-12-19 22:45 ` John Johansen
2016-12-19 22:49 ` Casey Schaufler
2016-12-20 1:27 ` Paul Moore
2016-12-20 1:23 ` Paul Moore
2016-12-20 1:59 ` Casey Schaufler
2016-12-20 14:40 ` José Bollo
2016-12-20 16:21 ` Casey Schaufler
2016-12-20 16:14 ` Stephen Smalley
2016-12-20 16:39 ` José Bollo
2016-12-20 16:50 ` Stephen Smalley
2016-12-20 18:17 ` Casey Schaufler
2016-12-20 18:28 ` Stephen Smalley
2016-12-20 19:07 ` Casey Schaufler
2016-12-20 19:35 ` Stephen Smalley
2016-12-20 20:03 ` Casey Schaufler
2016-12-20 21:22 ` José Bollo
2016-12-20 21:35 ` Stephen Smalley
2016-12-20 21:38 ` John Johansen
2016-12-21 2:37 ` Paul Moore
2016-12-21 7:04 ` José Bollo
2016-12-21 15:15 ` Paul Moore
2016-12-16 22:02 ` [PATCH 1/2] selinux: clean up cred usage and simplify Paul Moore
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.