| Message ID | 20190703212538.7383-16-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | LSM: Module stacking for AppArmor | expand |
On 7/3/19 5:25 PM, Casey Schaufler wrote: > Create a new entry "display" in /proc/.../attr for controlling > which LSM security information is displayed for a process. > The name of an active LSM that supplies hooks for human readable > data may be written to "display" to set the value. The name of > the LSM currently in use can be read from "display". > At this point there can only be one LSM capable of display > active. A helper function lsm_task_display() to get the display > slot for a task_struct. As I explained previously, this is a security hole waiting to happen. It still permits a process to affect the output of audit, alter the result of reading or writing /proc/self/attr nodes even by setuid/setgid/file-caps/context-changing programs, alter the contexts generated in netlink messages delivered to other processes (I think?), and possibly other effects beyond affecting the process' own view of things. Before: $ id uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 $ su Password: su: Authentication failure syscall audit record: type=SYSCALL msg=audit(07/09/2019 11:52:49.784:365) : arch=x86_64 syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x560897e58e00 a2=O_ WRONLY a3=0x0 items=1 ppid=3258 pid=3781 auid=sds2 uid=sds2 gid=sds2 euid=root s uid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj=staff_u:staff_r:staff_t:s0-s0:c0.c1023 key=(null) After: $ id uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 $ echo apparmor > /proc/self/attr/display $ su Password: su: Authentication failure audit record: type=SYSCALL msg=audit(07/09/2019 12:05:32.402:406) : arch=x86_64 syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x556b41e1ae00 a2=O_WRONLY a3=0x0 items=1 ppid=3258 pid=9426 auid=sds2 uid=sds2 gid=sds2 euid=root suid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj==unconfined key=(null) NB The subj= field of the SYSCALL audit record is longer accurate and is potentially under the control of a process that would not be authorized to set its subject label to that value by SELinux. Now, let's play with userspace. Before: # id uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 # passwd root passwd: SELinux deny access due to security policy. audit record: type=USER_AVC msg=audit(07/09/2019 12:24:35.135:812) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='avc: denied { passwd } for scontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tcontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tclass=passwd permissive=0 exe=/usr/bin/passwd sauid=root hostname=? addr=? terminal=pts/2' type=USER_CHAUTHTOK msg=audit(07/09/2019 12:24:35.135:813) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed' After: # id uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 # echo apparmor > /proc/self/attr/display # passwd root passwd: SELinux deny access due to security policy. audit record: type=USER_CHAUTHTOK msg=audit(07/09/2019 12:28:41.349:832) : pid=13083 uid=root auid=sds2 ses=7 subj==unconfined msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed' Here we again get the wrong value for subj= in the USER_CHAUTHTOK audit record, and we further lose the USER_AVC record entirely because it didn't even reach the point of the permission check due to not being able to get the caller context. The situation gets worse if the caller can set things up such that it can set an attribute value for one security module that is valid and privileged with respect to another security module. This isn't a far-fetched scenario; AppArmor will default to running everything unconfined, so as soon as you enable it, any root process can potentially load a policy that defines contexts that look exactly like SELinux contexts. Smack is even simpler; you can set any arbitrary string you want as long as you are root (by default); no policy required. So a root process that is confined by SELinux (or by AppAmor) can suddenly forge arbitrary contexts in audit records or reads of /proc/self/attr nodes or netlink messages or ..., just by virtue of applying these patches and enabling another security module like Smack. Or consider if ptags were ever made real and merged - by design, that's all about setting arbitrary tags from userspace. Then there is the separate issue of switching display to prevent attempts by a more privileged program to set one of its attributes from taking effect. Where have we seen that before - sendmail capabilities bug anyone? And it is actually worse than that bug, because with the assistance of a friendly security module, the write may actually succeed; it just won't alter the SELinux context of the program or anything it creates! This gets a NAK from me so long as it has these issues and setting the display remains outside the control of any security module. > > Reviewed-by: Kees Cook <keescook@chromium.org> > Reviewed-by: John Johansen <john.johansen@canonical.com> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > fs/proc/base.c | 1 + > include/linux/lsm_hooks.h | 13 ++++ > security/security.c | 129 +++++++++++++++++++++++++++++++++----- > 3 files changed, 126 insertions(+), 17 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index ddef482f1334..7bf70e041315 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2618,6 +2618,7 @@ static const struct pid_entry attr_dir_stuff[] = { > ATTR(NULL, "fscreate", 0666), > ATTR(NULL, "keycreate", 0666), > ATTR(NULL, "sockcreate", 0666), > + ATTR(NULL, "display", 0666), > #ifdef CONFIG_SECURITY_SMACK > DIR("smack", 0555, > proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops), > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index fe1fb7a69ee5..88ec3f3487ae 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -2134,4 +2134,17 @@ static inline void security_delete_hooks(struct security_hook_list *hooks, > > extern int lsm_inode_alloc(struct inode *inode); > > +/** > + * lsm_task_display - the "display LSM for this task > + * @task: The task to report on > + * > + * Returns the task's display LSM slot. > + */ > +static inline int lsm_task_display(struct task_struct *task) > +{ > + int *display = task->security; > + > + return *display; > +} > + > #endif /* ! __LINUX_LSM_HOOKS_H */ > diff --git a/security/security.c b/security/security.c > index 8927508b2142..f3a293e6ef5a 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -46,7 +46,9 @@ static struct kmem_cache *lsm_file_cache; > static struct kmem_cache *lsm_inode_cache; > > char *lsm_names; > -static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init; > +static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init = { > + .lbs_task = sizeof(int), /* slot number for the "display" LSM */ > +}; > > /* Boot-time LSM user choice */ > static __initdata const char *chosen_lsm_order; > @@ -423,8 +425,10 @@ static int lsm_append(const char *new, char **result) > > /* > * Current index to use while initializing the lsmblob secid list. > + * Pointers to the LSM id structures for local use. > */ > static int lsm_slot __lsm_ro_after_init; > +static struct lsm_id *lsm_slotlist[LSMBLOB_ENTRIES]; > > /** > * security_add_hooks - Add a modules hooks to the hook lists. > @@ -444,6 +448,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count, > if (lsmid->slot == LSMBLOB_NEEDED) { > if (lsm_slot >= LSMBLOB_ENTRIES) > panic("%s Too many LSMs registered.\n", __func__); > + lsm_slotlist[lsm_slot] = lsmid; > lsmid->slot = lsm_slot++; > init_debug("%s assigned lsmblob slot %d\n", lsmid->lsm, > lsmid->slot); > @@ -564,6 +569,8 @@ int lsm_inode_alloc(struct inode *inode) > */ > static int lsm_task_alloc(struct task_struct *task) > { > + int *display; > + > if (blob_sizes.lbs_task == 0) { > task->security = NULL; > return 0; > @@ -572,6 +579,15 @@ static int lsm_task_alloc(struct task_struct *task) > task->security = kzalloc(blob_sizes.lbs_task, GFP_KERNEL); > if (task->security == NULL) > return -ENOMEM; > + > + /* > + * The start of the task blob contains the "display" LSM slot number. > + * Start with it set to the invalid slot number, indicating that the > + * default first registered LSM be displayed. > + */ > + display = task->security; > + *display = LSMBLOB_INVALID; > + > return 0; > } > > @@ -1563,14 +1579,24 @@ int security_file_open(struct file *file) > > int security_task_alloc(struct task_struct *task, unsigned long clone_flags) > { > + int *odisplay = current->security; > + int *ndisplay; > int rc = lsm_task_alloc(task); > > - if (rc) > + if (unlikely(rc)) > return rc; > + > rc = call_int_hook(task_alloc, 0, task, clone_flags); > - if (unlikely(rc)) > + if (unlikely(rc)) { > security_task_free(task); > - return rc; > + return rc; > + } > + > + ndisplay = task->security; > + if (ndisplay && odisplay) > + *ndisplay = *odisplay; > + > + return 0; > } > > void security_task_free(struct task_struct *task) > @@ -1967,10 +1993,29 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name, > char **value) > { > struct security_hook_list *hp; > + int display = lsm_task_display(current); > + int slot = 0; > + > + if (!strcmp(name, "display")) { > + /* > + * lsm_slot will be 0 if there are no displaying modules. > + */ > + if (lsm_slot == 0) > + return -EINVAL; > + if (display != LSMBLOB_INVALID) > + slot = display; > + *value = kstrdup(lsm_slotlist[slot]->lsm, GFP_KERNEL); > + if (*value) > + return strlen(*value); > + return -ENOMEM; > + } > > hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) { > if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm)) > continue; > + if (lsm == NULL && display != LSMBLOB_INVALID && > + display != hp->lsmid->slot) > + continue; > return hp->hook.getprocattr(p, name, value); > } > return -EINVAL; > @@ -1980,10 +2025,46 @@ int security_setprocattr(const char *lsm, const char *name, void *value, > size_t size) > { > struct security_hook_list *hp; > + char *term; > + char *cp; > + int *display = current->security; > + int rc = -EINVAL; > + int slot = 0; > + > + if (!strcmp(name, "display")) { > + /* > + * lsm_slot will be 0 if there are no displaying modules. > + */ > + if (lsm_slot == 0 || size == 0) > + return -EINVAL; > + cp = kzalloc(size + 1, GFP_KERNEL); > + if (cp == NULL) > + return -ENOMEM; > + memcpy(cp, value, size); > + > + term = strchr(cp, ' '); > + if (term == NULL) > + term = strchr(cp, '\n'); > + if (term != NULL) > + *term = '\0'; > + > + for (slot = 0; slot < lsm_slot; slot++) > + if (!strcmp(cp, lsm_slotlist[slot]->lsm)) { > + *display = lsm_slotlist[slot]->slot; > + rc = size; > + break; > + } > + > + kfree(cp); > + return rc; > + } > > hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) { > if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm)) > continue; > + if (lsm == NULL && *display != LSMBLOB_INVALID && > + *display != hp->lsmid->slot) > + continue; > return hp->hook.setprocattr(name, value, size); > } > return -EINVAL; > @@ -2003,15 +2084,15 @@ EXPORT_SYMBOL(security_ismaclabel); > int security_secid_to_secctx(struct lsmblob *blob, char **secdata, u32 *seclen) > { > struct security_hook_list *hp; > - int rc; > + int display = lsm_task_display(current); > > hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) { > if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot)) > continue; > - rc = hp->hook.secid_to_secctx(blob->secid[hp->lsmid->slot], > - secdata, seclen); > - if (rc != 0) > - return rc; > + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot) > + return hp->hook.secid_to_secctx( > + blob->secid[hp->lsmid->slot], > + secdata, seclen); > } > return 0; > } > @@ -2021,16 +2102,15 @@ int security_secctx_to_secid(const char *secdata, u32 seclen, > struct lsmblob *blob) > { > struct security_hook_list *hp; > - int rc; > + int display = lsm_task_display(current); > > lsmblob_init(blob, 0); > hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list) { > if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot)) > continue; > - rc = hp->hook.secctx_to_secid(secdata, seclen, > - &blob->secid[hp->lsmid->slot]); > - if (rc != 0) > - return rc; > + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot) > + return hp->hook.secctx_to_secid(secdata, seclen, > + &blob->secid[hp->lsmid->slot]); > } > return 0; > } > @@ -2038,7 +2118,15 @@ EXPORT_SYMBOL(security_secctx_to_secid); > > void security_release_secctx(char *secdata, u32 seclen) > { > - call_void_hook(release_secctx, secdata, seclen); > + struct security_hook_list *hp; > + int *display = current->security; > + > + hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list) > + if (*display == LSMBLOB_INVALID || > + *display == hp->lsmid->slot) { > + hp->hook.release_secctx(secdata, seclen); > + return; > + } > } > EXPORT_SYMBOL(security_release_secctx); > > @@ -2163,8 +2251,15 @@ EXPORT_SYMBOL(security_sock_rcv_skb); > int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, > int __user *optlen, unsigned len) > { > - return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock, > - optval, optlen, len); > + int display = lsm_task_display(current); > + struct security_hook_list *hp; > + > + hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream, > + list) > + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot) > + return hp->hook.socket_getpeersec_stream(sock, optval, > + optlen, len); > + return -ENOPROTOOPT; > } > > int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, >
On 7/9/2019 10:13 AM, Stephen Smalley wrote: > On 7/3/19 5:25 PM, Casey Schaufler wrote: >> Create a new entry "display" in /proc/.../attr for controlling >> which LSM security information is displayed for a process. >> The name of an active LSM that supplies hooks for human readable >> data may be written to "display" to set the value. The name of >> the LSM currently in use can be read from "display". >> At this point there can only be one LSM capable of display >> active. A helper function lsm_task_display() to get the display >> slot for a task_struct. > > As I explained previously, this is a security hole waiting to happen. It still permits a process to affect the output of audit, alter the result of reading or writing /proc/self/attr nodes even by setuid/setgid/file-caps/context-changing programs, alter the contexts generated in netlink messages delivered to other processes (I think?), and possibly other effects beyond affecting the process' own view of things. I would very much like some feedback regarding which of the possible formats for putting multiple subject contexts in audit records would be preferred: lsm=selinux,subj=xyzzy_t lsm=smack,subj=Xyzzy lsm=selinux,smack subj=xyzzy_t,Xyzzy subj="selinux='xyzzy_t',smack='Xyzzy'" Or something else. Free bikeshedding! I don't see how you have a problem with netlink. My look at what's in the kernel didn't expose anything, but I am willing to be educated. > > Before: > $ id > uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 > $ su > Password: > su: Authentication failure > > syscall audit record: > type=SYSCALL msg=audit(07/09/2019 11:52:49.784:365) : arch=x86_64 syscall=openat > success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x560897e58e00 a2=O_ > WRONLY a3=0x0 items=1 ppid=3258 pid=3781 auid=sds2 uid=sds2 gid=sds2 euid=root s > uid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj=staff_u:staff_r:staff_t:s0-s0:c0.c1023 key=(null) > > After: > $ id > uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 > $ echo apparmor > /proc/self/attr/display > $ su > Password: > su: Authentication failure > > audit record: > type=SYSCALL msg=audit(07/09/2019 12:05:32.402:406) : arch=x86_64 syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x556b41e1ae00 a2=O_WRONLY a3=0x0 items=1 ppid=3258 pid=9426 auid=sds2 uid=sds2 gid=sds2 euid=root suid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj==unconfined key=(null) > > NB The subj= field of the SYSCALL audit record is longer accurate and is potentially under the control of a process that would not be authorized to set its subject label to that value by SELinux. It's still accurate, it's just not complete. It's a matter of how best to complete it. > > Now, let's play with userspace. > > Before: > # id > uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 > # passwd root > passwd: SELinux deny access due to security policy. > > audit record: > type=USER_AVC msg=audit(07/09/2019 12:24:35.135:812) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='avc: denied { passwd } for scontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tcontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tclass=passwd permissive=0 exe=/usr/bin/passwd sauid=root hostname=? addr=? terminal=pts/2' > type=USER_CHAUTHTOK msg=audit(07/09/2019 12:24:35.135:813) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed' > > After: > # id > uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 > # echo apparmor > /proc/self/attr/display > # passwd root > passwd: SELinux deny access due to security policy. > > audit record: > type=USER_CHAUTHTOK msg=audit(07/09/2019 12:28:41.349:832) : pid=13083 uid=root auid=sds2 ses=7 subj==unconfined msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed' > > Here we again get the wrong value for subj= in the USER_CHAUTHTOK audit record, and we further lose the USER_AVC record entirely because it didn't even reach the point of the permission check due to not being able to get the caller context. > > The situation gets worse if the caller can set things up such that it can set an attribute value for one security module that is valid and privileged with respect to another security module. This isn't a far-fetched scenario; AppArmor will default to running everything unconfined, so as soon as you enable it, any root process can potentially load a policy that defines contexts that look exactly like SELinux contexts. Smack is even simpler; you can set any arbitrary string you want as long as you are root (by default); no policy required. So a root process that is confined by SELinux (or by AppAmor) can suddenly forge arbitrary contexts in audit records or reads of /proc/self/attr nodes or netlink messages or ..., just by virtue of applying these patches and enabling another security module like Smack. Or consider if ptags were ever made real and merged - by design, that's all about setting arbitrary tags from userspace. Then there is the separate issue of switching > display to prevent attempts by a more privileged program to set one of its attributes from taking effect. Where have we seen that before - sendmail capabilities bug anyone? And it is actually worse than that bug, because with the assistance of a friendly security module, the write may actually succeed; it just won't alter the SELinux context of the program or anything it creates! > > This gets a NAK from me so long as it has these issues and setting the display remains outside the control of any security module. The issues you've raised around audit are meritorious. Any suggestions regarding how to address them would be quite welcome. As far as the general objection to the display mechanism, I am eager to understand what you might propose as an alternative. We can't dismiss backward compatibility for any of the modules. We can't preclude any module combination. We can require user space changes for configurations that were impossible before, just as the addition of SELinux to a system required user space changes. Update libselinux to check the display before using the attr interfaces and you've addressed most of the issues.
On 7/9/19 1:51 PM, Casey Schaufler wrote: > On 7/9/2019 10:13 AM, Stephen Smalley wrote: >> On 7/3/19 5:25 PM, Casey Schaufler wrote: >>> Create a new entry "display" in /proc/.../attr for controlling >>> which LSM security information is displayed for a process. >>> The name of an active LSM that supplies hooks for human readable >>> data may be written to "display" to set the value. The name of >>> the LSM currently in use can be read from "display". >>> At this point there can only be one LSM capable of display >>> active. A helper function lsm_task_display() to get the display >>> slot for a task_struct. >> >> As I explained previously, this is a security hole waiting to happen. It still permits a process to affect the output of audit, alter the result of reading or writing /proc/self/attr nodes even by setuid/setgid/file-caps/context-changing programs, alter the contexts generated in netlink messages delivered to other processes (I think?), and possibly other effects beyond affecting the process' own view of things. > > I would very much like some feedback regarding which of the > possible formats for putting multiple subject contexts in > audit records would be preferred: > > lsm=selinux,subj=xyzzy_t lsm=smack,subj=Xyzzy > lsm=selinux,smack subj=xyzzy_t,Xyzzy > subj="selinux='xyzzy_t',smack='Xyzzy'" (cc'd linux-audit mailing list) > > Or something else. Free bikeshedding! > > I don't see how you have a problem with netlink. My look > at what's in the kernel didn't expose anything, but I am > willing to be educated. I haven't traced through it in detail, but it wasn't clear to me that the security_secid_to_secctx() call always occurs in the context of the receiving process (and hence use its display value). If not, then the display of the sender can affect what is reported to the receiver; hence, there is a forgery concern similar to the binder issue. It would be cleaner if we didn't alter the default behavior of security_secid_to_secctx() and security_secctx_to_secid() and instead introduced new hooks for any case where we truly want the display to take effect. > >> >> Before: >> $ id >> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >> $ su >> Password: >> su: Authentication failure >> >> syscall audit record: >> type=SYSCALL msg=audit(07/09/2019 11:52:49.784:365) : arch=x86_64 syscall=openat >> success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x560897e58e00 a2=O_ >> WRONLY a3=0x0 items=1 ppid=3258 pid=3781 auid=sds2 uid=sds2 gid=sds2 euid=root s >> uid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj=staff_u:staff_r:staff_t:s0-s0:c0.c1023 key=(null) >> >> After: >> $ id >> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >> $ echo apparmor > /proc/self/attr/display >> $ su >> Password: >> su: Authentication failure >> >> audit record: >> type=SYSCALL msg=audit(07/09/2019 12:05:32.402:406) : arch=x86_64 syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x556b41e1ae00 a2=O_WRONLY a3=0x0 items=1 ppid=3258 pid=9426 auid=sds2 uid=sds2 gid=sds2 euid=root suid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj==unconfined key=(null) >> >> NB The subj= field of the SYSCALL audit record is longer accurate and is potentially under the control of a process that would not be authorized to set its subject label to that value by SELinux. > > It's still accurate, it's just not complete. It's a matter > of how best to complete it. > >> >> Now, let's play with userspace. >> >> Before: >> # id >> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >> # passwd root >> passwd: SELinux deny access due to security policy. >> >> audit record: >> type=USER_AVC msg=audit(07/09/2019 12:24:35.135:812) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='avc: denied { passwd } for scontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tcontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tclass=passwd permissive=0 exe=/usr/bin/passwd sauid=root hostname=? addr=? terminal=pts/2' >> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:24:35.135:813) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed' >> >> After: >> # id >> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >> # echo apparmor > /proc/self/attr/display >> # passwd root >> passwd: SELinux deny access due to security policy. >> >> audit record: >> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:28:41.349:832) : pid=13083 uid=root auid=sds2 ses=7 subj==unconfined msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed' >> >> Here we again get the wrong value for subj= in the USER_CHAUTHTOK audit record, and we further lose the USER_AVC record entirely because it didn't even reach the point of the permission check due to not being able to get the caller context. >> >> The situation gets worse if the caller can set things up such that it can set an attribute value for one security module that is valid and privileged with respect to another security module. This isn't a far-fetched scenario; AppArmor will default to running everything unconfined, so as soon as you enable it, any root process can potentially load a policy that defines contexts that look exactly like SELinux contexts. Smack is even simpler; you can set any arbitrary string you want as long as you are root (by default); no policy required. So a root process that is confined by SELinux (or by AppAmor) can suddenly forge arbitrary contexts in audit records or reads of /proc/self/attr nodes or netlink messages or ..., just by virtue of applying these patches and enabling another security module like Smack. Or consider if ptags were ever made real and merged - by design, that's all about setting arbitrary tags from userspace. Then there is the separate issue of switching >> display to prevent attempts by a more privileged program to set one of its attributes from taking effect. Where have we seen that before - sendmail capabilities bug anyone? And it is actually worse than that bug, because with the assistance of a friendly security module, the write may actually succeed; it just won't alter the SELinux context of the program or anything it creates! >> >> This gets a NAK from me so long as it has these issues and setting the display remains outside the control of any security module. > > The issues you've raised around audit are meritorious. > Any suggestions regarding how to address them would be > quite welcome. > > As far as the general objection to the display mechanism, > I am eager to understand what you might propose as an > alternative. We can't dismiss backward compatibility for > any of the modules. We can't preclude any module combination. > > We can require user space changes for configurations that > were impossible before, just as the addition of SELinux to > a system required user space changes. Update libselinux > to check the display before using the attr interfaces and > you've addressed most of the issues. Either we ensure that setting of the display can only affect processes in the same security equivalence class (same credentials) or the security modules need to be able to control who can set the display. Or both.
On 7/9/2019 11:12 AM, Stephen Smalley wrote: > On 7/9/19 1:51 PM, Casey Schaufler wrote: >> On 7/9/2019 10:13 AM, Stephen Smalley wrote: >>> On 7/3/19 5:25 PM, Casey Schaufler wrote: >>>> Create a new entry "display" in /proc/.../attr for controlling >>>> which LSM security information is displayed for a process. >>>> The name of an active LSM that supplies hooks for human readable >>>> data may be written to "display" to set the value. The name of >>>> the LSM currently in use can be read from "display". >>>> At this point there can only be one LSM capable of display >>>> active. A helper function lsm_task_display() to get the display >>>> slot for a task_struct. >>> >>> As I explained previously, this is a security hole waiting to happen. It still permits a process to affect the output of audit, alter the result of reading or writing /proc/self/attr nodes even by setuid/setgid/file-caps/context-changing programs, alter the contexts generated in netlink messages delivered to other processes (I think?), and possibly other effects beyond affecting the process' own view of things. >> >> I would very much like some feedback regarding which of the >> possible formats for putting multiple subject contexts in >> audit records would be preferred: >> >> lsm=selinux,subj=xyzzy_t lsm=smack,subj=Xyzzy >> lsm=selinux,smack subj=xyzzy_t,Xyzzy >> subj="selinux='xyzzy_t',smack='Xyzzy'" > > (cc'd linux-audit mailing list) > >> >> Or something else. Free bikeshedding! >> >> I don't see how you have a problem with netlink. My look >> at what's in the kernel didn't expose anything, but I am >> willing to be educated. > > I haven't traced through it in detail, but it wasn't clear to me that the security_secid_to_secctx() call always occurs in the context of the receiving process (and hence use its display value). If not, then the display of the sender can affect what is reported to the receiver; hence, there is a forgery concern similar to the binder issue. It would be cleaner if we didn't alter the default behavior of security_secid_to_secctx() and security_secctx_to_secid() and instead introduced new hooks for any case where we truly want the display to take effect. If the context is generated by security_secid_to_secctx() we retain the slot number of the module that created it in lsmcontext. We have to to ensure it is released correctly. If the potential issue you're describing for netlink does in fact occur, we can check the slot in lsmcontext to verify that it is the same. security_secid_to_secctx() is called nowhere in net/netlink, at least not that grep finds. Where are you seeing this potential problem? > >> >>> Before: >>> $ id >>> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >>> $ su >>> Password: >>> su: Authentication failure >>> >>> syscall audit record: >>> type=SYSCALL msg=audit(07/09/2019 11:52:49.784:365) : arch=x86_64 syscall=openat >>> success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x560897e58e00 a2=O_ >>> WRONLY a3=0x0 items=1 ppid=3258 pid=3781 auid=sds2 uid=sds2 gid=sds2 euid=root s >>> uid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj=staff_u:staff_r:staff_t:s0-s0:c0.c1023 key=(null) >>> >>> After: >>> $ id >>> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >>> $ echo apparmor > /proc/self/attr/display >>> $ su >>> Password: >>> su: Authentication failure >>> >>> audit record: >>> type=SYSCALL msg=audit(07/09/2019 12:05:32.402:406) : arch=x86_64 syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x556b41e1ae00 a2=O_WRONLY a3=0x0 items=1 ppid=3258 pid=9426 auid=sds2 uid=sds2 gid=sds2 euid=root suid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj==unconfined key=(null) >>> >>> NB The subj= field of the SYSCALL audit record is longer accurate and is potentially under the control of a process that would not be authorized to set its subject label to that value by SELinux. >> >> It's still accurate, it's just not complete. It's a matter >> of how best to complete it. >> >>> >>> Now, let's play with userspace. >>> >>> Before: >>> # id >>> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >>> # passwd root >>> passwd: SELinux deny access due to security policy. >>> >>> audit record: >>> type=USER_AVC msg=audit(07/09/2019 12:24:35.135:812) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='avc: denied { passwd } for scontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tcontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tclass=passwd permissive=0 exe=/usr/bin/passwd sauid=root hostname=? addr=? terminal=pts/2' >>> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:24:35.135:813) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed' >>> >>> After: >>> # id >>> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >>> # echo apparmor > /proc/self/attr/display >>> # passwd root >>> passwd: SELinux deny access due to security policy. >>> >>> audit record: >>> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:28:41.349:832) : pid=13083 uid=root auid=sds2 ses=7 subj==unconfined msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed' >>> >>> Here we again get the wrong value for subj= in the USER_CHAUTHTOK audit record, and we further lose the USER_AVC record entirely because it didn't even reach the point of the permission check due to not being able to get the caller context. >>> >>> The situation gets worse if the caller can set things up such that it can set an attribute value for one security module that is valid and privileged with respect to another security module. This isn't a far-fetched scenario; AppArmor will default to running everything unconfined, so as soon as you enable it, any root process can potentially load a policy that defines contexts that look exactly like SELinux contexts. Smack is even simpler; you can set any arbitrary string you want as long as you are root (by default); no policy required. So a root process that is confined by SELinux (or by AppAmor) can suddenly forge arbitrary contexts in audit records or reads of /proc/self/attr nodes or netlink messages or ..., just by virtue of applying these patches and enabling another security module like Smack. Or consider if ptags were ever made real and merged - by design, that's all about setting arbitrary tags from userspace. Then there is the separate issue of switching >>> display to prevent attempts by a more privileged program to set one of its attributes from taking effect. Where have we seen that before - sendmail capabilities bug anyone? And it is actually worse than that bug, because with the assistance of a friendly security module, the write may actually succeed; it just won't alter the SELinux context of the program or anything it creates! >>> >>> This gets a NAK from me so long as it has these issues and setting the display remains outside the control of any security module. >> >> The issues you've raised around audit are meritorious. >> Any suggestions regarding how to address them would be >> quite welcome. >> >> As far as the general objection to the display mechanism, >> I am eager to understand what you might propose as an >> alternative. We can't dismiss backward compatibility for >> any of the modules. We can't preclude any module combination. >> >> We can require user space changes for configurations that >> were impossible before, just as the addition of SELinux to >> a system required user space changes. Update libselinux >> to check the display before using the attr interfaces and >> you've addressed most of the issues. > > Either we ensure that setting of the display can only affect processes in the same security equivalence class (same credentials) In the process of trying to argue against your point I may have come around to your thinking. There would still be the case where a privileged program sets the display and invokes an equally privileged program which is "tricked" into setting the wrong attribute, but you have to put the responsibility for use of privilege on someone, somewhere. I will propose a solution in the next round. > or the security modules need to be able to control who can set the display. That's a mechanism for a module to opt-out of stacking, and Paul has been pretty clear that he won't go for that. > Or both.
On 7/9/19 5:18 PM, Casey Schaufler wrote: > On 7/9/2019 11:12 AM, Stephen Smalley wrote: >> On 7/9/19 1:51 PM, Casey Schaufler wrote: >>> On 7/9/2019 10:13 AM, Stephen Smalley wrote: >>>> On 7/3/19 5:25 PM, Casey Schaufler wrote: >>>>> Create a new entry "display" in /proc/.../attr for controlling >>>>> which LSM security information is displayed for a process. >>>>> The name of an active LSM that supplies hooks for human readable >>>>> data may be written to "display" to set the value. The name of >>>>> the LSM currently in use can be read from "display". >>>>> At this point there can only be one LSM capable of display >>>>> active. A helper function lsm_task_display() to get the display >>>>> slot for a task_struct. >>>> >>>> As I explained previously, this is a security hole waiting to happen. It still permits a process to affect the output of audit, alter the result of reading or writing /proc/self/attr nodes even by setuid/setgid/file-caps/context-changing programs, alter the contexts generated in netlink messages delivered to other processes (I think?), and possibly other effects beyond affecting the process' own view of things. >>> >>> I would very much like some feedback regarding which of the >>> possible formats for putting multiple subject contexts in >>> audit records would be preferred: >>> >>> lsm=selinux,subj=xyzzy_t lsm=smack,subj=Xyzzy >>> lsm=selinux,smack subj=xyzzy_t,Xyzzy >>> subj="selinux='xyzzy_t',smack='Xyzzy'" >> >> (cc'd linux-audit mailing list) >> >>> >>> Or something else. Free bikeshedding! >>> >>> I don't see how you have a problem with netlink. My look >>> at what's in the kernel didn't expose anything, but I am >>> willing to be educated. >> >> I haven't traced through it in detail, but it wasn't clear to me that the security_secid_to_secctx() call always occurs in the context of the receiving process (and hence use its display value). If not, then the display of the sender can affect what is reported to the receiver; hence, there is a forgery concern similar to the binder issue. It would be cleaner if we didn't alter the default behavior of security_secid_to_secctx() and security_secctx_to_secid() and instead introduced new hooks for any case where we truly want the display to take effect. > > If the context is generated by security_secid_to_secctx() we > retain the slot number of the module that created it in lsmcontext. > We have to to ensure it is released correctly. If the potential > issue you're describing for netlink does in fact occur, we can check > the slot in lsmcontext to verify that it is the same. > > security_secid_to_secctx() is called nowhere in net/netlink, > at least not that grep finds. Where are you seeing this potential > problem? Look under net/netfilter. > >> >>> >>>> Before: >>>> $ id >>>> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >>>> $ su >>>> Password: >>>> su: Authentication failure >>>> >>>> syscall audit record: >>>> type=SYSCALL msg=audit(07/09/2019 11:52:49.784:365) : arch=x86_64 syscall=openat >>>> success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x560897e58e00 a2=O_ >>>> WRONLY a3=0x0 items=1 ppid=3258 pid=3781 auid=sds2 uid=sds2 gid=sds2 euid=root s >>>> uid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj=staff_u:staff_r:staff_t:s0-s0:c0.c1023 key=(null) >>>> >>>> After: >>>> $ id >>>> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >>>> $ echo apparmor > /proc/self/attr/display >>>> $ su >>>> Password: >>>> su: Authentication failure >>>> >>>> audit record: >>>> type=SYSCALL msg=audit(07/09/2019 12:05:32.402:406) : arch=x86_64 syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x556b41e1ae00 a2=O_WRONLY a3=0x0 items=1 ppid=3258 pid=9426 auid=sds2 uid=sds2 gid=sds2 euid=root suid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj==unconfined key=(null) >>>> >>>> NB The subj= field of the SYSCALL audit record is longer accurate and is potentially under the control of a process that would not be authorized to set its subject label to that value by SELinux. >>> >>> It's still accurate, it's just not complete. It's a matter >>> of how best to complete it. >>> >>>> >>>> Now, let's play with userspace. >>>> >>>> Before: >>>> # id >>>> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >>>> # passwd root >>>> passwd: SELinux deny access due to security policy. >>>> >>>> audit record: >>>> type=USER_AVC msg=audit(07/09/2019 12:24:35.135:812) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='avc: denied { passwd } for scontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tcontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tclass=passwd permissive=0 exe=/usr/bin/passwd sauid=root hostname=? addr=? terminal=pts/2' >>>> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:24:35.135:813) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed' >>>> >>>> After: >>>> # id >>>> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >>>> # echo apparmor > /proc/self/attr/display >>>> # passwd root >>>> passwd: SELinux deny access due to security policy. >>>> >>>> audit record: >>>> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:28:41.349:832) : pid=13083 uid=root auid=sds2 ses=7 subj==unconfined msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed' >>>> >>>> Here we again get the wrong value for subj= in the USER_CHAUTHTOK audit record, and we further lose the USER_AVC record entirely because it didn't even reach the point of the permission check due to not being able to get the caller context. >>>> >>>> The situation gets worse if the caller can set things up such that it can set an attribute value for one security module that is valid and privileged with respect to another security module. This isn't a far-fetched scenario; AppArmor will default to running everything unconfined, so as soon as you enable it, any root process can potentially load a policy that defines contexts that look exactly like SELinux contexts. Smack is even simpler; you can set any arbitrary string you want as long as you are root (by default); no policy required. So a root process that is confined by SELinux (or by AppAmor) can suddenly forge arbitrary contexts in audit records or reads of /proc/self/attr nodes or netlink messages or ..., just by virtue of applying these patches and enabling another security module like Smack. Or consider if ptags were ever made real and merged - by design, that's all about setting arbitrary tags from userspace. Then there is the separate issue of switching >>>> display to prevent attempts by a more privileged program to set one of its attributes from taking effect. Where have we seen that before - sendmail capabilities bug anyone? And it is actually worse than that bug, because with the assistance of a friendly security module, the write may actually succeed; it just won't alter the SELinux context of the program or anything it creates! >>>> >>>> This gets a NAK from me so long as it has these issues and setting the display remains outside the control of any security module. >>> >>> The issues you've raised around audit are meritorious. >>> Any suggestions regarding how to address them would be >>> quite welcome. >>> >>> As far as the general objection to the display mechanism, >>> I am eager to understand what you might propose as an >>> alternative. We can't dismiss backward compatibility for >>> any of the modules. We can't preclude any module combination. >>> >>> We can require user space changes for configurations that >>> were impossible before, just as the addition of SELinux to >>> a system required user space changes. Update libselinux >>> to check the display before using the attr interfaces and >>> you've addressed most of the issues. >> >> Either we ensure that setting of the display can only affect processes in the same security equivalence class (same credentials) > > In the process of trying to argue against your point I > may have come around to your thinking. There would still > be the case where a privileged program sets the display > and invokes an equally privileged program which is "tricked" > into setting the wrong attribute, but you have to put the > responsibility for use of privilege on someone, somewhere. > > I will propose a solution in the next round. > >> or the security modules need to be able to control who can set the display. > > That's a mechanism for a module to opt-out of stacking, > and Paul has been pretty clear that he won't go for that. It doesn't have to be used that way; it can just be used to limit the set of authorized processes that can set the display to e.g. trusted container runtimes.
On 7/9/19 2:34 PM, Stephen Smalley wrote: > On 7/9/19 5:18 PM, Casey Schaufler wrote: >> On 7/9/2019 11:12 AM, Stephen Smalley wrote: >>> On 7/9/19 1:51 PM, Casey Schaufler wrote: >>>> On 7/9/2019 10:13 AM, Stephen Smalley wrote: >>>>> On 7/3/19 5:25 PM, Casey Schaufler wrote: >>>>>> Create a new entry "display" in /proc/.../attr for controlling >>>>>> which LSM security information is displayed for a process. >>>>>> The name of an active LSM that supplies hooks for human readable >>>>>> data may be written to "display" to set the value. The name of >>>>>> the LSM currently in use can be read from "display". >>>>>> At this point there can only be one LSM capable of display >>>>>> active. A helper function lsm_task_display() to get the display >>>>>> slot for a task_struct. >>>>> >>>>> As I explained previously, this is a security hole waiting to happen. It still permits a process to affect the output of audit, alter the result of reading or writing /proc/self/attr nodes even by setuid/setgid/file-caps/context-changing programs, alter the contexts generated in netlink messages delivered to other processes (I think?), and possibly other effects beyond affecting the process' own view of things. >>>> >>>> I would very much like some feedback regarding which of the >>>> possible formats for putting multiple subject contexts in >>>> audit records would be preferred: >>>> >>>> lsm=selinux,subj=xyzzy_t lsm=smack,subj=Xyzzy >>>> lsm=selinux,smack subj=xyzzy_t,Xyzzy >>>> subj="selinux='xyzzy_t',smack='Xyzzy'" >>> >>> (cc'd linux-audit mailing list) >>> >>>> >>>> Or something else. Free bikeshedding! >>>> >>>> I don't see how you have a problem with netlink. My look >>>> at what's in the kernel didn't expose anything, but I am >>>> willing to be educated. >>> >>> I haven't traced through it in detail, but it wasn't clear to me that the security_secid_to_secctx() call always occurs in the context of the receiving process (and hence use its display value). If not, then the display of the sender can affect what is reported to the receiver; hence, there is a forgery concern similar to the binder issue. It would be cleaner if we didn't alter the default behavior of security_secid_to_secctx() and security_secctx_to_secid() and instead introduced new hooks for any case where we truly want the display to take effect. >> >> If the context is generated by security_secid_to_secctx() we >> retain the slot number of the module that created it in lsmcontext. >> We have to to ensure it is released correctly. If the potential >> issue you're describing for netlink does in fact occur, we can check >> the slot in lsmcontext to verify that it is the same. >> >> security_secid_to_secctx() is called nowhere in net/netlink, >> at least not that grep finds. Where are you seeing this potential >> problem? > > Look under net/netfilter. > >> >>> >>>> >>>>> Before: >>>>> $ id >>>>> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >>>>> $ su >>>>> Password: >>>>> su: Authentication failure >>>>> >>>>> syscall audit record: >>>>> type=SYSCALL msg=audit(07/09/2019 11:52:49.784:365) : arch=x86_64 syscall=openat >>>>> success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x560897e58e00 a2=O_ >>>>> WRONLY a3=0x0 items=1 ppid=3258 pid=3781 auid=sds2 uid=sds2 gid=sds2 euid=root s >>>>> uid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj=staff_u:staff_r:staff_t:s0-s0:c0.c1023 key=(null) >>>>> >>>>> After: >>>>> $ id >>>>> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >>>>> $ echo apparmor > /proc/self/attr/display >>>>> $ su >>>>> Password: >>>>> su: Authentication failure >>>>> >>>>> audit record: >>>>> type=SYSCALL msg=audit(07/09/2019 12:05:32.402:406) : arch=x86_64 syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x556b41e1ae00 a2=O_WRONLY a3=0x0 items=1 ppid=3258 pid=9426 auid=sds2 uid=sds2 gid=sds2 euid=root suid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj==unconfined key=(null) >>>>> >>>>> NB The subj= field of the SYSCALL audit record is longer accurate and is potentially under the control of a process that would not be authorized to set its subject label to that value by SELinux. >>>> >>>> It's still accurate, it's just not complete. It's a matter >>>> of how best to complete it. >>>> >>>>> >>>>> Now, let's play with userspace. >>>>> >>>>> Before: >>>>> # id >>>>> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >>>>> # passwd root >>>>> passwd: SELinux deny access due to security policy. >>>>> >>>>> audit record: >>>>> type=USER_AVC msg=audit(07/09/2019 12:24:35.135:812) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='avc: denied { passwd } for scontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tcontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tclass=passwd permissive=0 exe=/usr/bin/passwd sauid=root hostname=? addr=? terminal=pts/2' >>>>> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:24:35.135:813) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed' >>>>> >>>>> After: >>>>> # id >>>>> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 >>>>> # echo apparmor > /proc/self/attr/display >>>>> # passwd root >>>>> passwd: SELinux deny access due to security policy. >>>>> >>>>> audit record: >>>>> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:28:41.349:832) : pid=13083 uid=root auid=sds2 ses=7 subj==unconfined msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed' >>>>> >>>>> Here we again get the wrong value for subj= in the USER_CHAUTHTOK audit record, and we further lose the USER_AVC record entirely because it didn't even reach the point of the permission check due to not being able to get the caller context. >>>>> >>>>> The situation gets worse if the caller can set things up such that it can set an attribute value for one security module that is valid and privileged with respect to another security module. This isn't a far-fetched scenario; AppArmor will default to running everything unconfined, so as soon as you enable it, any root process can potentially load a policy that defines contexts that look exactly like SELinux contexts. Smack is even simpler; you can set any arbitrary string you want as long as you are root (by default); no policy required. So a root process that is confined by SELinux (or by AppAmor) can suddenly forge arbitrary contexts in audit records or reads of /proc/self/attr nodes or netlink messages or ..., just by virtue of applying these patches and enabling another security module like Smack. Or consider if ptags were ever made real and merged - by design, that's all about setting arbitrary tags from userspace. Then there is the separate issue of switching >>>>> display to prevent attempts by a more privileged program to set one of its attributes from taking effect. Where have we seen that before - sendmail capabilities bug anyone? And it is actually worse than that bug, because with the assistance of a friendly security module, the write may actually succeed; it just won't alter the SELinux context of the program or anything it creates! >>>>> >>>>> This gets a NAK from me so long as it has these issues and setting the display remains outside the control of any security module. >>>> >>>> The issues you've raised around audit are meritorious. >>>> Any suggestions regarding how to address them would be >>>> quite welcome. >>>> >>>> As far as the general objection to the display mechanism, >>>> I am eager to understand what you might propose as an >>>> alternative. We can't dismiss backward compatibility for >>>> any of the modules. We can't preclude any module combination. >>>> >>>> We can require user space changes for configurations that >>>> were impossible before, just as the addition of SELinux to >>>> a system required user space changes. Update libselinux >>>> to check the display before using the attr interfaces and >>>> you've addressed most of the issues. >>> >>> Either we ensure that setting of the display can only affect processes in the same security equivalence class (same credentials) >> >> In the process of trying to argue against your point I >> may have come around to your thinking. There would still >> be the case where a privileged program sets the display >> and invokes an equally privileged program which is "tricked" >> into setting the wrong attribute, but you have to put the >> responsibility for use of privilege on someone, somewhere. >> >> I will propose a solution in the next round. >> >>> or the security modules need to be able to control who can set the display. >> >> That's a mechanism for a module to opt-out of stacking, >> and Paul has been pretty clear that he won't go for that. > > It doesn't have to be used that way; it can just be used to limit the set of authorized processes that can set the display to e.g. trusted container runtimes. We need something more than just controlling which processes can set the display LSM. We needs to be able to consider the display LSM from boot and the LSM is going to have to be more involved. At a minimum we need to some cooperation and maybe virtualization to properly support existing userspace code. The current approach is sufficient for applications that are just dumping the output (top, pstree, ...). The set of privileged programs is more problematic. LSM specific privileged programs can be dealt with by mediation but non-LSM specific programs need something more. The current LSMs (selinux, smack, apparmor) that use the interfaces being virtualized by the display LSM are going to want to control the display for an applications has support for the respective LSMs. If the LSMs don't cooperate in some way we may end up in a situation where the display LSM can never be switched, and even worse userspace might still try to access the label without the display LSM being switched. So are there currently any programs where we have this problem. At least one, DBus. So lets look at dbus. It has code to support selinux, apparmor, and I have seen out-of-tree smack code as well. It can be built with support for all of them enabled (ubuntu does this), and whether a particular LSM is enabled is determined by the LSM. Each LSM has its own way of determining if it is enabled and that check is not information not under control of the LSM (is_selinux_enabled() tests whether its fs is mounted, and aa_is_enabled() is its fs and kernel parameter). This mean booting the Ubuntu deskop with both apparmor and selinux enabled gets interesting. You can try this in ubuntu 19.10, it has a previous version of the stacking patchset so you don't even need to build this particular patchset to test it. If you take a default Ubuntu 19.04 install you can do lsm=capability,yama,apparmor,selinux this will default the display lsm to apparmor and selinux will come up in permissive mode (no policy) the desktop boots successfully. However setting the display lsm to selinux lsm=capability,yama,selinux,apparmor will result in dbus apparmor failures due to selinux being the display lsm and apparmor policy being loaded and enforcing. Add selinux policy into the mix and you double the fun as it will fail to boot either way. New userspaces can be updated to use new interfaces but to successfully boot existing userspaces the none display LSMs are going to have to lie to userspace about being enabled. I should note that I have tried this in Fedora too with selinux policy enabled, a stacking kernel and apparmor as the display LSM. This fails as expected (though I haven't dug into whether its dbus or another piece of code that is the culprit). Booting with selinux as the display, with apparmor enabled with no policy boots fine.
diff --git a/fs/proc/base.c b/fs/proc/base.c index ddef482f1334..7bf70e041315 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2618,6 +2618,7 @@ static const struct pid_entry attr_dir_stuff[] = { ATTR(NULL, "fscreate", 0666), ATTR(NULL, "keycreate", 0666), ATTR(NULL, "sockcreate", 0666), + ATTR(NULL, "display", 0666), #ifdef CONFIG_SECURITY_SMACK DIR("smack", 0555, proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops), diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index fe1fb7a69ee5..88ec3f3487ae 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -2134,4 +2134,17 @@ static inline void security_delete_hooks(struct security_hook_list *hooks, extern int lsm_inode_alloc(struct inode *inode); +/** + * lsm_task_display - the "display LSM for this task + * @task: The task to report on + * + * Returns the task's display LSM slot. + */ +static inline int lsm_task_display(struct task_struct *task) +{ + int *display = task->security; + + return *display; +} + #endif /* ! __LINUX_LSM_HOOKS_H */ diff --git a/security/security.c b/security/security.c index 8927508b2142..f3a293e6ef5a 100644 --- a/security/security.c +++ b/security/security.c @@ -46,7 +46,9 @@ static struct kmem_cache *lsm_file_cache; static struct kmem_cache *lsm_inode_cache; char *lsm_names; -static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init; +static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init = { + .lbs_task = sizeof(int), /* slot number for the "display" LSM */ +}; /* Boot-time LSM user choice */ static __initdata const char *chosen_lsm_order; @@ -423,8 +425,10 @@ static int lsm_append(const char *new, char **result) /* * Current index to use while initializing the lsmblob secid list. + * Pointers to the LSM id structures for local use. */ static int lsm_slot __lsm_ro_after_init; +static struct lsm_id *lsm_slotlist[LSMBLOB_ENTRIES]; /** * security_add_hooks - Add a modules hooks to the hook lists. @@ -444,6 +448,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count, if (lsmid->slot == LSMBLOB_NEEDED) { if (lsm_slot >= LSMBLOB_ENTRIES) panic("%s Too many LSMs registered.\n", __func__); + lsm_slotlist[lsm_slot] = lsmid; lsmid->slot = lsm_slot++; init_debug("%s assigned lsmblob slot %d\n", lsmid->lsm, lsmid->slot); @@ -564,6 +569,8 @@ int lsm_inode_alloc(struct inode *inode) */ static int lsm_task_alloc(struct task_struct *task) { + int *display; + if (blob_sizes.lbs_task == 0) { task->security = NULL; return 0; @@ -572,6 +579,15 @@ static int lsm_task_alloc(struct task_struct *task) task->security = kzalloc(blob_sizes.lbs_task, GFP_KERNEL); if (task->security == NULL) return -ENOMEM; + + /* + * The start of the task blob contains the "display" LSM slot number. + * Start with it set to the invalid slot number, indicating that the + * default first registered LSM be displayed. + */ + display = task->security; + *display = LSMBLOB_INVALID; + return 0; } @@ -1563,14 +1579,24 @@ int security_file_open(struct file *file) int security_task_alloc(struct task_struct *task, unsigned long clone_flags) { + int *odisplay = current->security; + int *ndisplay; int rc = lsm_task_alloc(task); - if (rc) + if (unlikely(rc)) return rc; + rc = call_int_hook(task_alloc, 0, task, clone_flags); - if (unlikely(rc)) + if (unlikely(rc)) { security_task_free(task); - return rc; + return rc; + } + + ndisplay = task->security; + if (ndisplay && odisplay) + *ndisplay = *odisplay; + + return 0; } void security_task_free(struct task_struct *task) @@ -1967,10 +1993,29 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name, char **value) { struct security_hook_list *hp; + int display = lsm_task_display(current); + int slot = 0; + + if (!strcmp(name, "display")) { + /* + * lsm_slot will be 0 if there are no displaying modules. + */ + if (lsm_slot == 0) + return -EINVAL; + if (display != LSMBLOB_INVALID) + slot = display; + *value = kstrdup(lsm_slotlist[slot]->lsm, GFP_KERNEL); + if (*value) + return strlen(*value); + return -ENOMEM; + } hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) { if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm)) continue; + if (lsm == NULL && display != LSMBLOB_INVALID && + display != hp->lsmid->slot) + continue; return hp->hook.getprocattr(p, name, value); } return -EINVAL; @@ -1980,10 +2025,46 @@ int security_setprocattr(const char *lsm, const char *name, void *value, size_t size) { struct security_hook_list *hp; + char *term; + char *cp; + int *display = current->security; + int rc = -EINVAL; + int slot = 0; + + if (!strcmp(name, "display")) { + /* + * lsm_slot will be 0 if there are no displaying modules. + */ + if (lsm_slot == 0 || size == 0) + return -EINVAL; + cp = kzalloc(size + 1, GFP_KERNEL); + if (cp == NULL) + return -ENOMEM; + memcpy(cp, value, size); + + term = strchr(cp, ' '); + if (term == NULL) + term = strchr(cp, '\n'); + if (term != NULL) + *term = '\0'; + + for (slot = 0; slot < lsm_slot; slot++) + if (!strcmp(cp, lsm_slotlist[slot]->lsm)) { + *display = lsm_slotlist[slot]->slot; + rc = size; + break; + } + + kfree(cp); + return rc; + } hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) { if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm)) continue; + if (lsm == NULL && *display != LSMBLOB_INVALID && + *display != hp->lsmid->slot) + continue; return hp->hook.setprocattr(name, value, size); } return -EINVAL; @@ -2003,15 +2084,15 @@ EXPORT_SYMBOL(security_ismaclabel); int security_secid_to_secctx(struct lsmblob *blob, char **secdata, u32 *seclen) { struct security_hook_list *hp; - int rc; + int display = lsm_task_display(current); hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) { if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot)) continue; - rc = hp->hook.secid_to_secctx(blob->secid[hp->lsmid->slot], - secdata, seclen); - if (rc != 0) - return rc; + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot) + return hp->hook.secid_to_secctx( + blob->secid[hp->lsmid->slot], + secdata, seclen); } return 0; } @@ -2021,16 +2102,15 @@ int security_secctx_to_secid(const char *secdata, u32 seclen, struct lsmblob *blob) { struct security_hook_list *hp; - int rc; + int display = lsm_task_display(current); lsmblob_init(blob, 0); hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list) { if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot)) continue; - rc = hp->hook.secctx_to_secid(secdata, seclen, - &blob->secid[hp->lsmid->slot]); - if (rc != 0) - return rc; + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot) + return hp->hook.secctx_to_secid(secdata, seclen, + &blob->secid[hp->lsmid->slot]); } return 0; } @@ -2038,7 +2118,15 @@ EXPORT_SYMBOL(security_secctx_to_secid); void security_release_secctx(char *secdata, u32 seclen) { - call_void_hook(release_secctx, secdata, seclen); + struct security_hook_list *hp; + int *display = current->security; + + hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list) + if (*display == LSMBLOB_INVALID || + *display == hp->lsmid->slot) { + hp->hook.release_secctx(secdata, seclen); + return; + } } EXPORT_SYMBOL(security_release_secctx); @@ -2163,8 +2251,15 @@ EXPORT_SYMBOL(security_sock_rcv_skb); int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, int __user *optlen, unsigned len) { - return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock, - optval, optlen, len); + int display = lsm_task_display(current); + struct security_hook_list *hp; + + hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream, + list) + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot) + return hp->hook.socket_getpeersec_stream(sock, optval, + optlen, len); + return -ENOPROTOOPT; } int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,