Messages in this thread | | | From | Paul Moore <> | Date | Mon, 27 Jul 2020 22:14:26 -0400 | Subject | 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 <rgb@redhat.com> 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 <rgb@redhat.com> > --- > 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
| |