Re: [PATCH v19 13/23] LSM: Specify which LSM to display
On 7/27/20 1:36 PM, James Morris wrote: > On Fri, 24 Jul 2020, Casey Schaufler wrote: > >> Create a new entry "display" in the procfs attr directory for >> controlling which LSM security information is displayed for a >> process. A process can only read or write its own display value. >> >> 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() is >> provided to get the display slot for a task_struct. >> >> Setting the "display" requires that all security modules using >> setprocattr hooks allow the action. Each security module is >> responsible for defining its policy. >> >> AppArmor hook provided by John Johansen >> SELinux hook provided by Stephen Smalley >> >> Reviewed-by: Kees Cook >> Acked-by: Stephen Smalley >> Acked-by: Paul Moore >> Signed-off-by: Casey Schaufler > > jj: do you have any review/feedback on this? > yeah, I am working my way through it today -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH V3fix ghak120] audit: initialize context values in case of mandatory events
On Mon, Jul 27, 2020 at 5:30 PM Richard Guy Briggs wrote: > > Issue ghak120 enabled syscall records to accompany required records when > no rules are present to trigger the storage of syscall context. A > reported issue showed that the cwd was not always initialized. That > issue was already resolved ... Yes and no. Yes, it appears to be resolved in v5.8-rc1 and above, but the problematic commit is in v5.7 and I'm not sure backporting the fix in v5.8-rcX plus this patch is the right thing to do for a released kernel. The lowest risk fix for v5.7 at this point is to do a revert; regardless of what happens with this patch and v5.8-rcX please post a revert for the audit/stable-5.7 tree as soon as you can. > ... but a review of all other records that could > be triggered at the time of a syscall record revealed other potential > values that could be missing or misleading. Initialize them. > > The fds array is reset to -1 after the first syscall to indicate it > isn't valid any more, but was never set to -1 when the context was > allocated to indicate it wasn't yet valid. > > The audit_inode* functions can be called without going through > getname_flags() or getname_kernel() that sets audit_names and cwd, so > set the cwd if it has not already been done so due to audit_names being > valid. > > The LSM dump_common_audit_data() LSM_AUDIT_DATA_NET:AF_UNIX case was > missed with the ghak96 patch, so add that case here. > > Please see issue https://github.com/linux-audit/audit-kernel/issues/120 > Please see issue https://github.com/linux-audit/audit-kernel/issues/96 > Passes audit-testsuite. > > Signed-off-by: Richard Guy Briggs > --- > kernel/auditsc.c | 3 +++ > security/lsm_audit.c | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 6884b50069d1..2f97618e6a34 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -929,6 +929,7 @@ static inline struct audit_context > *audit_alloc_context(enum audit_state state) > context->prio = state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0; > INIT_LIST_HEAD(&context->killed_trees); > INIT_LIST_HEAD(&context->names_list); > + context->fds[0] = -1; > return context; > } > > @@ -2076,6 +2077,7 @@ void __audit_inode(struct filename *name, const struct > dentry *dentry, > } > handle_path(dentry); > audit_copy_inode(n, dentry, inode, flags & AUDIT_INODE_NOEVAL); > + _audit_getcwd(context); > } > > void __audit_file(const struct file *file) > @@ -2194,6 +2196,7 @@ void __audit_inode_child(struct inode *parent, > audit_copy_inode(found_child, dentry, inode, 0); > else > found_child->ino = AUDIT_INO_UNSET; > + _audit_getcwd(context); > } > EXPORT_SYMBOL_GPL(__audit_inode_child); > > diff --git a/security/lsm_audit.c b/security/lsm_audit.c > index 53d0d183db8f..e93077612246 100644 > --- a/security/lsm_audit.c > +++ b/security/lsm_audit.c > @@ -369,6 +369,7 @@ static void dump_common_audit_data(struct audit_buffer > *ab, > audit_log_untrustedstring(ab, p); > else > audit_log_n_hex(ab, p, len); > + audit_getcwd(); > break; > } > } I understand the "fds[0] = -1" fix in audit_alloc_context() (ironically, the kzalloc() which is supposed to help with cases like this, hurts us with this particular field), but I'm still not quite seeing why we need to sprinkle audit_getcwd() calls everywhere to fix this bug (this seems more like a feature add than a bigfix). Yes, they may fix the problem but it seems like simply adding a context->pwd test in audit_log_name() similar to what we do in audit_log_exit() is the correct fix. We are currently at -rc7 and this really needs to land before v5.8 is released, presumably this weekend; this means a small and limited bug fix patch is what is needed. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
[PATCH V3fix ghak120] audit: initialize context values in case of mandatory events
Issue ghak120 enabled syscall records to accompany required records when no rules are present to trigger the storage of syscall context. A reported issue showed that the cwd was not always initialized. That issue was already resolved, but a review of all other records that could be triggered at the time of a syscall record revealed other potential values that could be missing or misleading. Initialize them. The fds array is reset to -1 after the first syscall to indicate it isn't valid any more, but was never set to -1 when the context was allocated to indicate it wasn't yet valid. The audit_inode* functions can be called without going through getname_flags() or getname_kernel() that sets audit_names and cwd, so set the cwd if it has not already been done so due to audit_names being valid. The LSM dump_common_audit_data() LSM_AUDIT_DATA_NET:AF_UNIX case was missed with the ghak96 patch, so add that case here. Please see issue https://github.com/linux-audit/audit-kernel/issues/120 Please see issue https://github.com/linux-audit/audit-kernel/issues/96 Passes audit-testsuite. Signed-off-by: Richard Guy Briggs --- kernel/auditsc.c | 3 +++ security/lsm_audit.c | 1 + 2 files changed, 4 insertions(+) diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 6884b50069d1..2f97618e6a34 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -929,6 +929,7 @@ static inline struct audit_context *audit_alloc_context(enum audit_state state) context->prio = state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0; INIT_LIST_HEAD(&context->killed_trees); INIT_LIST_HEAD(&context->names_list); + context->fds[0] = -1; return context; } @@ -2076,6 +2077,7 @@ void __audit_inode(struct filename *name, const struct dentry *dentry, } handle_path(dentry); audit_copy_inode(n, dentry, inode, flags & AUDIT_INODE_NOEVAL); + _audit_getcwd(context); } void __audit_file(const struct file *file) @@ -2194,6 +2196,7 @@ void __audit_inode_child(struct inode *parent, audit_copy_inode(found_child, dentry, inode, 0); else found_child->ino = AUDIT_INO_UNSET; + _audit_getcwd(context); } EXPORT_SYMBOL_GPL(__audit_inode_child); diff --git a/security/lsm_audit.c b/security/lsm_audit.c index 53d0d183db8f..e93077612246 100644 --- a/security/lsm_audit.c +++ b/security/lsm_audit.c @@ -369,6 +369,7 @@ static void dump_common_audit_data(struct audit_buffer *ab, audit_log_untrustedstring(ab, p); else audit_log_n_hex(ab, p, len); + audit_getcwd(); break; } } -- 1.8.3.1 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v19 02/23] LSM: Create and manage the lsmblob data structure.
On 7/27/2020 9:12 AM, Stephen Smalley wrote: > On Fri, Jul 24, 2020 at 4:35 PM Casey Schaufler > wrote: >> When more than one security module is exporting data to >> audit and networking sub-systems a single 32 bit integer >> is no longer sufficient to represent the data. Add a >> structure to be used instead. >> >> The lsmblob structure is currently an array of >> u32 "secids". There is an entry for each of the >> security modules built into the system that would >> use secids if active. The system assigns the module >> a "slot" when it registers hooks. If modules are >> compiled in but not registered there will be unused >> slots. >> >> A new lsm_id structure, which contains the name >> of the LSM and its slot number, is created. There >> is an instance for each LSM, which assigns the name >> and passes it to the infrastructure to set the slot. >> >> The audit rules data is expanded to use an array of >> security module data rather than a single instance. >> Because IMA uses the audit rule functions it is >> affected as well. >> >> Acked-by: Stephen Smalley >> Acked-by: Paul Moore >> Signed-off-by: Casey Schaufler > With CONFIG_BPF_LSM=y: Thanks. I am surprised that this config option isn't under security. No problem, an easy fix. > > security/bpf/hooks.c: In function ‘bpf_lsm_init’: > security/bpf/hooks.c:18:63: error: passing argument 3 of > ‘security_add_hooks’ from incompatible pointer type > [-Werror=incompatible-pointer-types] >18 | security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks), "bpf"); > | ^ > | | > | char * > In file included from security/bpf/hooks.c:6: > ./include/linux/lsm_hooks.h:1592:26: note: expected ‘struct lsm_id *’ > but argument is of type ‘char *’ > 1592 | struct lsm_id *lsmid); > | ~~~^ -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v19 13/23] LSM: Specify which LSM to display
On Fri, 24 Jul 2020, Casey Schaufler wrote: > Create a new entry "display" in the procfs attr directory for > controlling which LSM security information is displayed for a > process. A process can only read or write its own display value. > > 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() is > provided to get the display slot for a task_struct. > > Setting the "display" requires that all security modules using > setprocattr hooks allow the action. Each security module is > responsible for defining its policy. > > AppArmor hook provided by John Johansen > SELinux hook provided by Stephen Smalley > > Reviewed-by: Kees Cook > Acked-by: Stephen Smalley > Acked-by: Paul Moore > Signed-off-by: Casey Schaufler jj: do you have any review/feedback on this? -- James Morris -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v19 21/23] Audit: Add a new record for multiple object LSM attributes
On Fri, 24 Jul 2020, Casey Schaufler wrote: > Create a new audit record type to contain the object information > when there are multiple security modules that require such data. > This record is emitted before the other records for the event, but > is linked with the same timestamp and serial number. > > Signed-off-by: Casey Schaufler > Cc: linux-audit@redhat.com These audit patches will need ack/review from Paul. -- James Morris -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v19 17/23] LSM: security_secid_to_secctx in netlink netfilter
On Fri, 24 Jul 2020, Casey Schaufler wrote: > Change netlink netfilter interfaces to use lsmcontext > pointers, and remove scaffolding. > > Reviewed-by: Kees Cook > Reviewed-by: John Johansen > Acked-by: Stephen Smalley > Signed-off-by: Casey Schaufler > cc: net...@vger.kernel.org I'd like to see Paul's acks on any networking related changes. -- James Morris -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v19 20/23] Audit: Add new record for multiple process LSM attributes
On 7/24/20 4:32 PM, Casey Schaufler wrote: Create a new audit record type to contain the subject information when there are multiple security modules that require such data. This record is linked with the same timestamp and serial number. The record is produced only in cases where there is more than one security module with a process "context". Before this change the only audit events that required multiple records were syscall events. Several non-syscall events include subject contexts, so the use of audit_context data has been expanded as necessary. Signed-off-by: Casey Schaufler Cc: linux-audit@redhat.com --- diff --git a/include/net/xfrm.h b/include/net/xfrm.h index c7d213c9f9d8..930432c3912e 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -672,11 +672,13 @@ static inline struct audit_buffer *xfrm_audit_start(const char *op) if (audit_enabled == AUDIT_OFF) return NULL; + audit_stamp_context(audit_context()); audit_buf = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_MAC_IPSEC_EVENT); if (audit_buf == NULL) return NULL; audit_log_format(audit_buf, "op=%s", op); + audit_log_lsm(NULL, false); Notice that the audit_log_start() call above specified GFP_ATOMIC. But your audit_log_lsm() uses GFP_KERNEL. You'll either need to always use GFP_ATOMIC in audit_log_lsm() or pass in the gfp flags there. Make sure you test with CONFIG_DEBUG_ATOMIC_SLEEP=y and check your dmesg output. -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v19 02/23] LSM: Create and manage the lsmblob data structure.
On Fri, Jul 24, 2020 at 4:35 PM Casey Schaufler wrote: > > When more than one security module is exporting data to > audit and networking sub-systems a single 32 bit integer > is no longer sufficient to represent the data. Add a > structure to be used instead. > > The lsmblob structure is currently an array of > u32 "secids". There is an entry for each of the > security modules built into the system that would > use secids if active. The system assigns the module > a "slot" when it registers hooks. If modules are > compiled in but not registered there will be unused > slots. > > A new lsm_id structure, which contains the name > of the LSM and its slot number, is created. There > is an instance for each LSM, which assigns the name > and passes it to the infrastructure to set the slot. > > The audit rules data is expanded to use an array of > security module data rather than a single instance. > Because IMA uses the audit rule functions it is > affected as well. > > Acked-by: Stephen Smalley > Acked-by: Paul Moore > Signed-off-by: Casey Schaufler With CONFIG_BPF_LSM=y: security/bpf/hooks.c: In function ‘bpf_lsm_init’: security/bpf/hooks.c:18:63: error: passing argument 3 of ‘security_add_hooks’ from incompatible pointer type [-Werror=incompatible-pointer-types] 18 | security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks), "bpf"); | ^ | | | char * In file included from security/bpf/hooks.c:6: ./include/linux/lsm_hooks.h:1592:26: note: expected ‘struct lsm_id *’ but argument is of type ‘char *’ 1592 | struct lsm_id *lsmid); | ~~~^ -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
