| Message ID | 20200817170729.2605279-4-tweek@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: add detailed tracepoint on audited events | expand |
On 8/17/20 1:07 PM, Thiébaud Weksteen wrote: > From: Peter Enderborg <peter.enderborg@sony.com> > > In the print out add permissions, it will look like: > <...>-1042 [007] .... 201.965142: selinux_audited: > requested=0x4000000 denied=0x4000000 audited=0x4000000 > result=-13 > scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023 > tcontext=system_u:object_r:bin_t:s0 > tclass=file permissions={ !entrypoint } > > This patch is adding the "permissions={ !entrypoint }". > The permissions preceded by "!" have been denied and the permissions > without have been accepted. > > Note that permission filtering is done on the audited, denied or > requested attributes. > > Suggested-by: Steven Rostedt <rostedt@goodmis.org> > Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> > Reviewed-by: Thiébaud Weksteen <tweek@google.com> > Signed-off-by: Peter Enderborg <peter.enderborg@sony.com> > --- Does this require a corresponding patch to userspace? Otherwise, I get the following: libtraceevent: No such file or directory [avc:selinux_audited] function avc_trace_perm_to_name not defined
On 8/17/20 1:07 PM, Thiébaud Weksteen wrote: > From: Peter Enderborg <peter.enderborg@sony.com> > > In the print out add permissions, it will look like: > <...>-1042 [007] .... 201.965142: selinux_audited: > requested=0x4000000 denied=0x4000000 audited=0x4000000 > result=-13 > scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023 > tcontext=system_u:object_r:bin_t:s0 > tclass=file permissions={ !entrypoint } > > This patch is adding the "permissions={ !entrypoint }". > The permissions preceded by "!" have been denied and the permissions > without have been accepted. > > Note that permission filtering is done on the audited, denied or > requested attributes. > > Suggested-by: Steven Rostedt <rostedt@goodmis.org> > Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> > Reviewed-by: Thiébaud Weksteen <tweek@google.com> > Signed-off-by: Peter Enderborg <peter.enderborg@sony.com> > --- > include/trace/events/avc.h | 11 +++++++++-- > security/selinux/avc.c | 36 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 45 insertions(+), 2 deletions(-) > > diff --git a/security/selinux/avc.c b/security/selinux/avc.c > index 7de5cc5169af..d585b68c2a50 100644 > --- a/security/selinux/avc.c > +++ b/security/selinux/avc.c > @@ -695,6 +695,7 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a) > audit_log_format(ab, " } for "); > } > > + > /** > * avc_audit_post_callback - SELinux specific information > * will be called by generic audit code Also, drop the spurious whitespace change above.
On Mon, 17 Aug 2020 16:13:29 -0400 Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > Does this require a corresponding patch to userspace? Otherwise, I get > the following: > > libtraceevent: No such file or directory > [avc:selinux_audited] function avc_trace_perm_to_name not defined Yes, we need to add a plugin to libtraceevent that will add that function. I could possibly write one up real quick. -- Steve
On 8/17/20 10:16 PM, Stephen Smalley wrote: > On 8/17/20 1:07 PM, Thiébaud Weksteen wrote: > >> From: Peter Enderborg <peter.enderborg@sony.com> >> >> In the print out add permissions, it will look like: >> <...>-1042 [007] .... 201.965142: selinux_audited: >> requested=0x4000000 denied=0x4000000 audited=0x4000000 >> result=-13 >> scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023 >> tcontext=system_u:object_r:bin_t:s0 >> tclass=file permissions={ !entrypoint } >> >> This patch is adding the "permissions={ !entrypoint }". >> The permissions preceded by "!" have been denied and the permissions >> without have been accepted. >> >> Note that permission filtering is done on the audited, denied or >> requested attributes. >> >> Suggested-by: Steven Rostedt <rostedt@goodmis.org> >> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> >> Reviewed-by: Thiébaud Weksteen <tweek@google.com> >> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com> >> --- >> include/trace/events/avc.h | 11 +++++++++-- >> security/selinux/avc.c | 36 ++++++++++++++++++++++++++++++++++++ >> 2 files changed, 45 insertions(+), 2 deletions(-) >> >> diff --git a/security/selinux/avc.c b/security/selinux/avc.c >> index 7de5cc5169af..d585b68c2a50 100644 >> --- a/security/selinux/avc.c >> +++ b/security/selinux/avc.c >> @@ -695,6 +695,7 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a) >> audit_log_format(ab, " } for "); >> } >> + >> /** >> * avc_audit_post_callback - SELinux specific information >> * will be called by generic audit code > > Also, drop the spurious whitespace change above. > > Is there any other things we need to fix? A part 1&2 now OK?
On Tue, Aug 18, 2020 at 4:11 AM peter enderborg <peter.enderborg@sony.com> wrote: > > On 8/17/20 10:16 PM, Stephen Smalley wrote: > > On 8/17/20 1:07 PM, Thiébaud Weksteen wrote: > > > >> From: Peter Enderborg <peter.enderborg@sony.com> > >> > >> In the print out add permissions, it will look like: > >> <...>-1042 [007] .... 201.965142: selinux_audited: > >> requested=0x4000000 denied=0x4000000 audited=0x4000000 > >> result=-13 > >> scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023 > >> tcontext=system_u:object_r:bin_t:s0 > >> tclass=file permissions={ !entrypoint } > >> > >> This patch is adding the "permissions={ !entrypoint }". > >> The permissions preceded by "!" have been denied and the permissions > >> without have been accepted. > >> > >> Note that permission filtering is done on the audited, denied or > >> requested attributes. > >> > >> Suggested-by: Steven Rostedt <rostedt@goodmis.org> > >> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> > >> Reviewed-by: Thiébaud Weksteen <tweek@google.com> > >> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com> > >> --- > >> include/trace/events/avc.h | 11 +++++++++-- > >> security/selinux/avc.c | 36 ++++++++++++++++++++++++++++++++++++ > >> 2 files changed, 45 insertions(+), 2 deletions(-) > >> > >> diff --git a/security/selinux/avc.c b/security/selinux/avc.c > >> index 7de5cc5169af..d585b68c2a50 100644 > >> --- a/security/selinux/avc.c > >> +++ b/security/selinux/avc.c > >> @@ -695,6 +695,7 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a) > >> audit_log_format(ab, " } for "); > >> } > >> + > >> /** > >> * avc_audit_post_callback - SELinux specific information > >> * will be called by generic audit code > > > > Also, drop the spurious whitespace change above. > > > > > Is there any other things we need to fix? A part 1&2 now OK? They looked ok to me, but Paul should review them.
On Mon, 17 Aug 2020 16:29:33 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Mon, 17 Aug 2020 16:13:29 -0400 > Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > > Does this require a corresponding patch to userspace? Otherwise, I get > > the following: > > > > libtraceevent: No such file or directory > > [avc:selinux_audited] function avc_trace_perm_to_name not defined > > Yes, we need to add a plugin to libtraceevent that will add that > function. > > I could possibly write one up real quick. Something like this (this is patched on top of trace-cmd, but will work for tools/lib/traceevent too). With CONFIG_TRACE_EVENT_INJECT enabled (to test events), I did the following: # echo 'utclass=1 audited=1 denied=0' > /sys/kernel/tracing/events/avc/selinux_audited/inject # trace-cmd extract # trace-cmd report cpus=8 <...>-1685 [005] 1607.612032: selinux_audited: requested=0x0 denied=0x0 audited=0x1 result=0 scontext= tcontext= tclass= permissions={ compute_av } Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- diff --git a/lib/traceevent/plugins/Makefile b/lib/traceevent/plugins/Makefile index 21e933af..13cbcb92 100644 --- a/lib/traceevent/plugins/Makefile +++ b/lib/traceevent/plugins/Makefile @@ -16,6 +16,7 @@ PLUGIN_OBJS += plugin_scsi.o PLUGIN_OBJS += plugin_cfg80211.o PLUGIN_OBJS += plugin_blk.o PLUGIN_OBJS += plugin_tlb.o +PLUGIN_OBJS += plugin_avc.o PLUGIN_OBJS := $(PLUGIN_OBJS:%.o=$(bdir)/%.o) PLUGIN_BUILD := $(PLUGIN_OBJS:$(bdir)/%.o=$(bdir)/%.so) diff --git a/lib/traceevent/plugins/plugin_avc.c b/lib/traceevent/plugins/plugin_avc.c new file mode 100644 index 00000000..76af23b9 --- /dev/null +++ b/lib/traceevent/plugins/plugin_avc.c @@ -0,0 +1,312 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <stdio.h> +#include <string.h> +#include "event-parse.h" + +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0])) + +typedef unsigned short u16; + +/* Class/perm mapping support */ +struct security_class_mapping { + const char *name; + const char *perms[sizeof(unsigned) * 8 + 1]; +}; + +#define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \ + "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append", "map" + +#define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \ + "rename", "execute", "quotaon", "mounton", "audit_access", \ + "open", "execmod", "watch", "watch_mount", "watch_sb", \ + "watch_with_perm", "watch_reads" + +#define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \ + "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \ + "sendto", "name_bind" + +#define COMMON_IPC_PERMS "create", "destroy", "getattr", "setattr", "read", \ + "write", "associate", "unix_read", "unix_write" + +#define COMMON_CAP_PERMS "chown", "dac_override", "dac_read_search", \ + "fowner", "fsetid", "kill", "setgid", "setuid", "setpcap", \ + "linux_immutable", "net_bind_service", "net_broadcast", \ + "net_admin", "net_raw", "ipc_lock", "ipc_owner", "sys_module", \ + "sys_rawio", "sys_chroot", "sys_ptrace", "sys_pacct", "sys_admin", \ + "sys_boot", "sys_nice", "sys_resource", "sys_time", \ + "sys_tty_config", "mknod", "lease", "audit_write", \ + "audit_control", "setfcap" + +#define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \ + "wake_alarm", "block_suspend", "audit_read", "perfmon", "bpf" + +/* + * Note: The name for any socket class should be suffixed by "socket", + * and doesn't contain more than one substr of "socket". + */ +struct security_class_mapping secclass_map[] = { + { "security", + { "compute_av", "compute_create", "compute_member", + "check_context", "load_policy", "compute_relabel", + "compute_user", "setenforce", "setbool", "setsecparam", + "setcheckreqprot", "read_policy", "validate_trans", NULL } }, + { "process", + { "fork", "transition", "sigchld", "sigkill", + "sigstop", "signull", "signal", "ptrace", "getsched", "setsched", + "getsession", "getpgid", "setpgid", "getcap", "setcap", "share", + "getattr", "setexec", "setfscreate", "noatsecure", "siginh", + "setrlimit", "rlimitinh", "dyntransition", "setcurrent", + "execmem", "execstack", "execheap", "setkeycreate", + "setsockcreate", "getrlimit", NULL } }, + { "process2", + { "nnp_transition", "nosuid_transition", NULL } }, + { "system", + { "ipc_info", "syslog_read", "syslog_mod", + "syslog_console", "module_request", "module_load", NULL } }, + { "capability", + { COMMON_CAP_PERMS, NULL } }, + { "filesystem", + { "mount", "remount", "unmount", "getattr", + "relabelfrom", "relabelto", "associate", "quotamod", + "quotaget", "watch", NULL } }, + { "file", + { COMMON_FILE_PERMS, + "execute_no_trans", "entrypoint", NULL } }, + { "dir", + { COMMON_FILE_PERMS, "add_name", "remove_name", + "reparent", "search", "rmdir", NULL } }, + { "fd", { "use", NULL } }, + { "lnk_file", + { COMMON_FILE_PERMS, NULL } }, + { "chr_file", + { COMMON_FILE_PERMS, NULL } }, + { "blk_file", + { COMMON_FILE_PERMS, NULL } }, + { "sock_file", + { COMMON_FILE_PERMS, NULL } }, + { "fifo_file", + { COMMON_FILE_PERMS, NULL } }, + { "socket", + { COMMON_SOCK_PERMS, NULL } }, + { "tcp_socket", + { COMMON_SOCK_PERMS, + "node_bind", "name_connect", + NULL } }, + { "udp_socket", + { COMMON_SOCK_PERMS, + "node_bind", NULL } }, + { "rawip_socket", + { COMMON_SOCK_PERMS, + "node_bind", NULL } }, + { "node", + { "recvfrom", "sendto", NULL } }, + { "netif", + { "ingress", "egress", NULL } }, + { "netlink_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "packet_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "key_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "unix_stream_socket", + { COMMON_SOCK_PERMS, "connectto", NULL } }, + { "unix_dgram_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "sem", + { COMMON_IPC_PERMS, NULL } }, + { "msg", { "send", "receive", NULL } }, + { "msgq", + { COMMON_IPC_PERMS, "enqueue", NULL } }, + { "shm", + { COMMON_IPC_PERMS, "lock", NULL } }, + { "ipc", + { COMMON_IPC_PERMS, NULL } }, + { "netlink_route_socket", + { COMMON_SOCK_PERMS, + "nlmsg_read", "nlmsg_write", NULL } }, + { "netlink_tcpdiag_socket", + { COMMON_SOCK_PERMS, + "nlmsg_read", "nlmsg_write", NULL } }, + { "netlink_nflog_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "netlink_xfrm_socket", + { COMMON_SOCK_PERMS, + "nlmsg_read", "nlmsg_write", NULL } }, + { "netlink_selinux_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "netlink_iscsi_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "netlink_audit_socket", + { COMMON_SOCK_PERMS, + "nlmsg_read", "nlmsg_write", "nlmsg_relay", "nlmsg_readpriv", + "nlmsg_tty_audit", NULL } }, + { "netlink_fib_lookup_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "netlink_connector_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "netlink_netfilter_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "netlink_dnrt_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "association", + { "sendto", "recvfrom", "setcontext", "polmatch", NULL } }, + { "netlink_kobject_uevent_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "netlink_generic_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "netlink_scsitransport_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "netlink_rdma_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "netlink_crypto_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "appletalk_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "packet", + { "send", "recv", "relabelto", "forward_in", "forward_out", NULL } }, + { "key", + { "view", "read", "write", "search", "link", "setattr", "create", + NULL } }, + { "dccp_socket", + { COMMON_SOCK_PERMS, + "node_bind", "name_connect", NULL } }, + { "memprotect", { "mmap_zero", NULL } }, + { "peer", { "recv", NULL } }, + { "capability2", + { COMMON_CAP2_PERMS, NULL } }, + { "kernel_service", { "use_as_override", "create_files_as", NULL } }, + { "tun_socket", + { COMMON_SOCK_PERMS, "attach_queue", NULL } }, + { "binder", { "impersonate", "call", "set_context_mgr", "transfer", + NULL } }, + { "cap_userns", + { COMMON_CAP_PERMS, NULL } }, + { "cap2_userns", + { COMMON_CAP2_PERMS, NULL } }, + { "sctp_socket", + { COMMON_SOCK_PERMS, + "node_bind", "name_connect", "association", NULL } }, + { "icmp_socket", + { COMMON_SOCK_PERMS, + "node_bind", NULL } }, + { "ax25_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "ipx_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "netrom_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "atmpvc_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "x25_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "rose_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "decnet_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "atmsvc_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "rds_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "irda_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "pppox_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "llc_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "can_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "tipc_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "bluetooth_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "iucv_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "rxrpc_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "isdn_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "phonet_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "ieee802154_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "caif_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "alg_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "nfc_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "vsock_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "kcm_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "qipcrtr_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "smc_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "infiniband_pkey", + { "access", NULL } }, + { "infiniband_endport", + { "manage_subnet", NULL } }, + { "bpf", + {"map_create", "map_read", "map_write", "prog_load", "prog_run"} }, + { "xdp_socket", + { COMMON_SOCK_PERMS, NULL } }, + { "perf_event", + {"open", "cpu", "kernel", "tracepoint", "read", "write"} }, + { "lockdown", + { "integrity", "confidentiality", NULL } }, + { NULL } + }; + + +static unsigned long long +avc_trace_perm_to_name(struct trace_seq *p, unsigned long long *args) +{ + u16 tclass = args[1]; + int av = args[2]; + int avdenied = args[3]; + int i, perm; + const char **perms; + + if (!tclass || tclass >= ARRAY_SIZE(secclass_map)) + return 0; + + perms = secclass_map[tclass-1].perms; + + i = 0; + perm = 1; + while (i < (sizeof(av) * 8)) { + if ((perm & av) && perms[i]) { + if (!(perm & avdenied)) + trace_seq_printf(p, " %s", perms[i]); + else + trace_seq_printf(p, " !%s", perms[i]); + av &= ~perm; + } + i++; + perm <<= 1; + } + + return 0; +} + +int TEP_PLUGIN_LOADER(struct tep_handle *tep) +{ + tep_register_print_function(tep, + avc_trace_perm_to_name, + TEP_FUNC_ARG_STRING, + "avc_trace_perm_to_name", + TEP_FUNC_ARG_PTR, + TEP_FUNC_ARG_INT, + TEP_FUNC_ARG_INT, + TEP_FUNC_ARG_INT, + TEP_FUNC_ARG_VOID); + + return 0; +} + +void TEP_PLUGIN_UNLOADER(struct tep_handle *tep) +{ + tep_unregister_print_function(tep, avc_trace_perm_to_name, + "avc_trace_perm_to_name"); +}
On 8/18/20 12:09 PM, Steven Rostedt wrote: > On Mon, 17 Aug 2020 16:29:33 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > >> On Mon, 17 Aug 2020 16:13:29 -0400 >> Stephen Smalley <stephen.smalley.work@gmail.com> wrote: >> >>> Does this require a corresponding patch to userspace? Otherwise, I get >>> the following: >>> >>> libtraceevent: No such file or directory >>> [avc:selinux_audited] function avc_trace_perm_to_name not defined >> Yes, we need to add a plugin to libtraceevent that will add that >> function. >> >> I could possibly write one up real quick. > Something like this (this is patched on top of trace-cmd, but will work > for tools/lib/traceevent too). > > With CONFIG_TRACE_EVENT_INJECT enabled (to test events), I did the following: > > # echo 'utclass=1 audited=1 denied=0' > /sys/kernel/tracing/events/avc/selinux_audited/inject > # trace-cmd extract > # trace-cmd report > cpus=8 > <...>-1685 [005] 1607.612032: selinux_audited: requested=0x0 denied=0x0 audited=0x1 result=0 scontext= tcontext= tclass= permissions={ compute_av } > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > > --- > diff --git a/lib/traceevent/plugins/Makefile b/lib/traceevent/plugins/Makefile > index 21e933af..13cbcb92 100644 > --- a/lib/traceevent/plugins/Makefile > +++ b/lib/traceevent/plugins/Makefile > @@ -16,6 +16,7 @@ PLUGIN_OBJS += plugin_scsi.o > PLUGIN_OBJS += plugin_cfg80211.o > PLUGIN_OBJS += plugin_blk.o > PLUGIN_OBJS += plugin_tlb.o > +PLUGIN_OBJS += plugin_avc.o > > PLUGIN_OBJS := $(PLUGIN_OBJS:%.o=$(bdir)/%.o) > PLUGIN_BUILD := $(PLUGIN_OBJS:$(bdir)/%.o=$(bdir)/%.so) > diff --git a/lib/traceevent/plugins/plugin_avc.c b/lib/traceevent/plugins/plugin_avc.c > new file mode 100644 > index 00000000..76af23b9 > --- /dev/null > +++ b/lib/traceevent/plugins/plugin_avc.c > @@ -0,0 +1,312 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <stdio.h> > +#include <string.h> > +#include "event-parse.h" > + > +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0])) > + > +typedef unsigned short u16; > + > +/* Class/perm mapping support */ > +struct security_class_mapping { > + const char *name; > + const char *perms[sizeof(unsigned) * 8 + 1]; > +}; > + > +#define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \ > + "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append", "map" > + > +#define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \ > + "rename", "execute", "quotaon", "mounton", "audit_access", \ > + "open", "execmod", "watch", "watch_mount", "watch_sb", \ > + "watch_with_perm", "watch_reads" > + > +#define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \ > + "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \ > + "sendto", "name_bind" > + > +#define COMMON_IPC_PERMS "create", "destroy", "getattr", "setattr", "read", \ > + "write", "associate", "unix_read", "unix_write" > + > +#define COMMON_CAP_PERMS "chown", "dac_override", "dac_read_search", \ > + "fowner", "fsetid", "kill", "setgid", "setuid", "setpcap", \ > + "linux_immutable", "net_bind_service", "net_broadcast", \ > + "net_admin", "net_raw", "ipc_lock", "ipc_owner", "sys_module", \ > + "sys_rawio", "sys_chroot", "sys_ptrace", "sys_pacct", "sys_admin", \ > + "sys_boot", "sys_nice", "sys_resource", "sys_time", \ > + "sys_tty_config", "mknod", "lease", "audit_write", \ > + "audit_control", "setfcap" > + > +#define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \ > + "wake_alarm", "block_suspend", "audit_read", "perfmon", "bpf" > + > +/* > + * Note: The name for any socket class should be suffixed by "socket", > + * and doesn't contain more than one substr of "socket". > + */ > +struct security_class_mapping secclass_map[] = { > + { "security", > + { "compute_av", "compute_create", "compute_member", > + "check_context", "load_policy", "compute_relabel", > + "compute_user", "setenforce", "setbool", "setsecparam", > + "setcheckreqprot", "read_policy", "validate_trans", NULL } }, > So we'll need to update this plugin whenever we modify security/selinux/include/classmap.h to keep them in sync. Is that a concern? I don't suppose the plugin could directly include classmap.h? I guess we'd have to export it as a public header. It isn't considered to be part of the kernel API/ABI and can change anytime (but in practice changes are not that frequent, and usually just additive in nature).
On Tue, Aug 18, 2020 at 8:14 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Tue, Aug 18, 2020 at 4:11 AM peter enderborg <peter.enderborg@sony.com> wrote: ... > > Is there any other things we need to fix? A part 1&2 now OK? > > They looked ok to me, but Paul should review them. Patches 1 and 2 look fine to me with the small nits that Stephen pointed out corrected. I'm glad to see the information in string form now, I think that will be a big help for people making use of this. Unfortunately, I'm a little concerned about patch 3 for the reason Stephen already mentioned. While changes to the class mapping are infrequent, they do happen, and I'm not very excited about adding it to the userspace kAPI via a header. Considering that the tracing tools are going to be running on the same system that is being inspected, perhaps the tracing tools could inspect /sys/fs/selinux/class at runtime to query the permission mappings? Stephen, is there a libselinux API which does this already?
On Wed, 19 Aug 2020 09:11:08 -0400 Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > So we'll need to update this plugin whenever we modify > security/selinux/include/classmap.h to keep them in sync. Is that a > concern? I don't suppose the plugin could directly include classmap.h? > I guess we'd have to export it as a public header. It isn't considered > to be part of the kernel API/ABI and can change anytime (but in practice > changes are not that frequent, and usually just additive in nature). Yes, it would require some stability between userspace and the plugin. If the value indexes don't change then that would work fine. If you add new ones, that too should be OK, just have a way to state "unknown" in the plugin. -- Steve
On 8/21/20 4:22 AM, Paul Moore wrote: > On Tue, Aug 18, 2020 at 8:14 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: >> On Tue, Aug 18, 2020 at 4:11 AM peter enderborg <peter.enderborg@sony.com> wrote: > ... > >>> Is there any other things we need to fix? A part 1&2 now OK? >> They looked ok to me, but Paul should review them. > Patches 1 and 2 look fine to me with the small nits that Stephen > pointed out corrected. I'm glad to see the information in string form > now, I think that will be a big help for people making use of this. > > Unfortunately, I'm a little concerned about patch 3 for the reason > Stephen already mentioned. While changes to the class mapping are > infrequent, they do happen, and I'm not very excited about adding it > to the userspace kAPI via a header. Considering that the tracing > tools are going to be running on the same system that is being > inspected, perhaps the tracing tools could inspect > /sys/fs/selinux/class at runtime to query the permission mappings? > Stephen, is there a libselinux API which does this already? > One way to use this trace is to write directly to a memory buffer over a time period. In the case for Android and I guess in many other embedded cases too they are moved to be some other machine to be analysed so having them locked to where it was running also have problems. So what is the problem we see with the plugin, that we have perms names that are "unknown" ?
On Thu, Aug 20, 2020 at 10:22 PM Paul Moore <paul@paul-moore.com> wrote: > > On Tue, Aug 18, 2020 at 8:14 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > On Tue, Aug 18, 2020 at 4:11 AM peter enderborg <peter.enderborg@sony.com> wrote: > > ... > > > > Is there any other things we need to fix? A part 1&2 now OK? > > > > They looked ok to me, but Paul should review them. > > Patches 1 and 2 look fine to me with the small nits that Stephen > pointed out corrected. I'm glad to see the information in string form > now, I think that will be a big help for people making use of this. > > Unfortunately, I'm a little concerned about patch 3 for the reason > Stephen already mentioned. While changes to the class mapping are > infrequent, they do happen, and I'm not very excited about adding it > to the userspace kAPI via a header. Considering that the tracing > tools are going to be running on the same system that is being > inspected, perhaps the tracing tools could inspect > /sys/fs/selinux/class at runtime to query the permission mappings? > Stephen, is there a libselinux API which does this already? There is a libselinux API but both it and the /sys/fs/selinux/class tree is exposing the policy values for classes/permissions, not the kernel-private indices. The dynamic class/perm mapping support introduced a layer of indirection between them. The tracepoint is in the avc and therefore dealing with the kernel-private values, not the policy values. The mapping occurs on entry/exit of the security server functions. So there is no way for userspace to read the kernel class/perm values. We'd just need to keep them in sync manually. And one is allowed to insert new classes or permissions before existing ones, thereby changing the values of existing ones, or even to remove them.
On Thu, Aug 20, 2020 at 10:31 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 19 Aug 2020 09:11:08 -0400 > Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > > So we'll need to update this plugin whenever we modify > > security/selinux/include/classmap.h to keep them in sync. Is that a > > concern? I don't suppose the plugin could directly include classmap.h? > > I guess we'd have to export it as a public header. It isn't considered > > to be part of the kernel API/ABI and can change anytime (but in practice > > changes are not that frequent, and usually just additive in nature). > > Yes, it would require some stability between userspace and the plugin. > If the value indexes don't change then that would work fine. If you add > new ones, that too should be OK, just have a way to state "unknown" in > the plugin. Since we introduced the dynamic class/perm mapping support, it has been possible for the values of existing classes/permissions to change, and that has happened at time, e.g. when we added watch permissions to the common file perms, that shifted the values of the class file perms like entrypoint, when we added the process2 class right after the process class, it shifted the values of all the subsequent classes in the classmap.h. So you can't rely on those values remaining stable across kernel versions.
On Fri, Aug 21, 2020 at 8:15 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Thu, Aug 20, 2020 at 10:22 PM Paul Moore <paul@paul-moore.com> wrote: > > > > On Tue, Aug 18, 2020 at 8:14 AM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > > > On Tue, Aug 18, 2020 at 4:11 AM peter enderborg <peter.enderborg@sony.com> wrote: > > > > ... > > > > > > Is there any other things we need to fix? A part 1&2 now OK? > > > > > > They looked ok to me, but Paul should review them. > > > > Patches 1 and 2 look fine to me with the small nits that Stephen > > pointed out corrected. I'm glad to see the information in string form > > now, I think that will be a big help for people making use of this. > > > > Unfortunately, I'm a little concerned about patch 3 for the reason > > Stephen already mentioned. While changes to the class mapping are > > infrequent, they do happen, and I'm not very excited about adding it > > to the userspace kAPI via a header. Considering that the tracing > > tools are going to be running on the same system that is being > > inspected, perhaps the tracing tools could inspect > > /sys/fs/selinux/class at runtime to query the permission mappings? > > Stephen, is there a libselinux API which does this already? > > There is a libselinux API but both it and the /sys/fs/selinux/class > tree is exposing the policy values for classes/permissions, not the > kernel-private indices. The dynamic class/perm mapping support > introduced a layer of indirection between them. The tracepoint is in > the avc and therefore dealing with the kernel-private values, not the > policy values. The mapping occurs on entry/exit of the security > server functions. So there is no way for userspace to read the kernel > class/perm values. We'd just need to keep them in sync manually. And > one is allowed to insert new classes or permissions before existing > ones, thereby changing the values of existing ones, or even to remove > them. Ah, okay, thanks. Can you tell I've never really had to look very closely at that code ;)
On Fri, Aug 21, 2020 at 8:29 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Thu, Aug 20, 2020 at 10:31 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 19 Aug 2020 09:11:08 -0400 > > Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > > > > So we'll need to update this plugin whenever we modify > > > security/selinux/include/classmap.h to keep them in sync. Is that a > > > concern? I don't suppose the plugin could directly include classmap.h? > > > I guess we'd have to export it as a public header. It isn't considered > > > to be part of the kernel API/ABI and can change anytime (but in practice > > > changes are not that frequent, and usually just additive in nature). > > > > Yes, it would require some stability between userspace and the plugin. > > If the value indexes don't change then that would work fine. If you add > > new ones, that too should be OK, just have a way to state "unknown" in > > the plugin. > > Since we introduced the dynamic class/perm mapping support, it has > been possible for the values of existing classes/permissions to > change, and that has happened at time, e.g. when we added watch > permissions to the common file perms, that shifted the values of the > class file perms like entrypoint, when we added the process2 class > right after the process class, it shifted the values of all the > subsequent classes in the classmap.h. So you can't rely on those > values remaining stable across kernel versions. I think it is becoming increasingly clear that generating the permission set string in userspace isn't really workable without breaking the dynamic class/permission mapping to some degree. Unfortunately I don't see these perf changes as a big enough "win" to offset the loss of the dynamic mapping loss. I'm okay with merging patches 1/3 and 2/3 wth the changes Stephen suggested, but I think we will need to leave patch 3/3 out of this for now.
On 8/21/20 3:19 PM, Paul Moore wrote: > On Fri, Aug 21, 2020 at 8:29 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: >> On Thu, Aug 20, 2020 at 10:31 PM Steven Rostedt <rostedt@goodmis.org> wrote: >>> On Wed, 19 Aug 2020 09:11:08 -0400 >>> Stephen Smalley <stephen.smalley.work@gmail.com> wrote: >>> >>>> So we'll need to update this plugin whenever we modify >>>> security/selinux/include/classmap.h to keep them in sync. Is that a >>>> concern? I don't suppose the plugin could directly include classmap.h? >>>> I guess we'd have to export it as a public header. It isn't considered >>>> to be part of the kernel API/ABI and can change anytime (but in practice >>>> changes are not that frequent, and usually just additive in nature). >>> Yes, it would require some stability between userspace and the plugin. >>> If the value indexes don't change then that would work fine. If you add >>> new ones, that too should be OK, just have a way to state "unknown" in >>> the plugin. >> Since we introduced the dynamic class/perm mapping support, it has >> been possible for the values of existing classes/permissions to >> change, and that has happened at time, e.g. when we added watch >> permissions to the common file perms, that shifted the values of the >> class file perms like entrypoint, when we added the process2 class >> right after the process class, it shifted the values of all the >> subsequent classes in the classmap.h. So you can't rely on those >> values remaining stable across kernel versions. > I think it is becoming increasingly clear that generating the > permission set string in userspace isn't really workable without > breaking the dynamic class/permission mapping to some degree. > Unfortunately I don't see these perf changes as a big enough "win" to > offset the loss of the dynamic mapping loss. > > I'm okay with merging patches 1/3 and 2/3 wth the changes Stephen > suggested, but I think we will need to leave patch 3/3 out of this for > now. > Ok.
On Fri, Aug 21, 2020 at 9:21 AM Thiébaud Weksteen <tweek@google.com> wrote: >> >> I'm okay with merging patches 1/3 and 2/3 wth the changes Stephen >> suggested, but I think we will need to leave patch 3/3 out of this for >> now. > > That works for me. Can you respin patches 1 and two with those changes and repost? I try to refrain from making non-merge-fuzz changes when merging patches.
This is other solution. I dont think needs any plugin support. It will generate one event for eatch denial and there is possible to filter on permission. .
diff --git a/include/trace/events/avc.h b/include/trace/events/avc.h index b55fda2e0773..94bca8bef8d2 100644 --- a/include/trace/events/avc.h +++ b/include/trace/events/avc.h @@ -10,6 +10,10 @@ #define _TRACE_SELINUX_H #include <linux/tracepoint.h> +#include <linux/trace_seq.h> + +extern const char *avc_trace_perm_to_name(struct trace_seq *p, u16 class, u32 audited, u32 denied); +#define __perm_to_name(class, audited, denied) avc_trace_perm_to_name(p, class, audited, denied) TRACE_EVENT(selinux_audited, @@ -29,6 +33,7 @@ TRACE_EVENT(selinux_audited, __string(scontext, scontext) __string(tcontext, tcontext) __string(tclass, tclass) + __field(u16, utclass) ), TP_fast_assign( @@ -36,14 +41,16 @@ TRACE_EVENT(selinux_audited, __entry->denied = sad->denied; __entry->audited = sad->audited; __entry->result = sad->result; + __entry->utclass = sad->tclass; __assign_str(tcontext, tcontext); __assign_str(scontext, scontext); __assign_str(tclass, tclass); ), - TP_printk("requested=0x%x denied=0x%x audited=0x%x result=%d scontext=%s tcontext=%s tclass=%s", + TP_printk("requested=0x%x denied=0x%x audited=0x%x result=%d scontext=%s tcontext=%s tclass=%s permissions={%s }", __entry->requested, __entry->denied, __entry->audited, __entry->result, - __get_str(scontext), __get_str(tcontext), __get_str(tclass) + __get_str(scontext), __get_str(tcontext), __get_str(tclass), + __perm_to_name(__entry->utclass, __entry->audited, __entry->denied) ) ); diff --git a/security/selinux/avc.c b/security/selinux/avc.c index 7de5cc5169af..d585b68c2a50 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -695,6 +695,7 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a) audit_log_format(ab, " } for "); } + /** * avc_audit_post_callback - SELinux specific information * will be called by generic audit code @@ -991,6 +992,41 @@ int avc_ss_reset(struct selinux_avc *avc, u32 seqno) return rc; } +/** + * avc_trace_perm_to_name - SELinux help function for trace + * @p pointer to output storage + * @tclass tclass for the event + * @av access vector + * @avdenied denied permissions in av format + */ +const char *avc_trace_perm_to_name(struct trace_seq *p, u16 tclass, u32 av, u32 avdenied) +{ + const char *ret = trace_seq_buffer_ptr(p); + int i, perm; + const char **perms; + + if (WARN_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map))) + return NULL; + + perms = secclass_map[tclass-1].perms; + + i = 0; + perm = 1; + while (i < (sizeof(av) * 8)) { + if ((perm & av) && perms[i]) { + if (!(perm & avdenied)) + trace_seq_printf(p, " %s", perms[i]); + else + trace_seq_printf(p, " !%s", perms[i]); + av &= ~perm; + } + i++; + perm <<= 1; + } + + return ret; +} + /* * Slow-path helper function for avc_has_perm_noaudit, * when the avc_node lookup fails. We get called with