[PATCH V3fix ghak120] audit: initialize context values in case of mandatory events

[PATCH V3fix ghak120] audit: initialize context values in case of mandatory events

[Richard Guy Briggs]

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 <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;
 			}
 		}
-- 
1.8.3.1

--
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

[Paul Moore]

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

--
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

[Richard Guy Briggs]

On 2020-07-27 22:14, Paul Moore wrote:
> 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;

Ok, fair enough.  I don't understand why you didn't do the revert since
it appears so trivial to you and this review and fix turned out to be
marginally more work.  I didn't understand what you wanted when you
referred to stable.

> 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.

(more below...)

> > ... 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.

Well, considering that ghak96 ended up being a bugfix (that wasn't its
intent), I figured these audit_getcwd() were also bugfixes to prevent
the same BUG under different calling conditions.

> 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.

Ok, so it sounds like rather than just fix it now, it would be better to
revert it, then submit *one* patch for ghak120 plus this fix that will
go tentatively upstream in 3 months, fully in 5.  Arguably the last
chunk above should be added to ghak96, so that should be reverted too,
then resubmitted with this added chunk (or it could be a fixup chunk
that would need to be sequenced with ghak120).  As for the middle two
chunks, they could either be resubmitted with a resubmitted ghak96, or
with a resubmitted ghak120.  As for the timing of all of these, ghak96
should be in place before the ghak120 patch, so even resubmitting one
patch for the combined ghak120 and ghak96 might make more sense.

I know you like only really minimal fixes this late, but this seemed
pretty minimal to me...

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
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

[Paul Moore]

On Tue, Jul 28, 2020 at 12:27 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2020-07-27 22:14, Paul Moore wrote:
> > 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;
>
> Ok, fair enough.  I don't understand why you didn't do the revert since
> it appears so trivial to you and this review and fix turned out to be
> marginally more work.  I didn't understand what you wanted when you
> referred to stable.

I held off on the revert because I thought you might want the chance
to submit the revert with your authorship.  I made an assumption that
it meant the same to you as it does to me; that's my mistake, I should
have known better.

I'll do the revert myself for stable-5.8 (which should trickle down to
v5.7.z with the right metadata), don't bother with it.

> > 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.
>
> (more below...)
>
> > > ... 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.
>
> Well, considering that ghak96 ended up being a bugfix (that wasn't its
> intent), I figured these audit_getcwd() were also bugfixes to prevent
> the same BUG under different calling conditions.
>
> > 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.
>
> Ok, so it sounds like rather than just fix it now, it would be better to
> revert it, then submit *one* patch for ghak120 plus this fix that will
> go tentatively upstream in 3 months, fully in 5.  Arguably the last
> chunk above should be added to ghak96, so that should be reverted too,
> then resubmitted with this added chunk (or it could be a fixup chunk
> that would need to be sequenced with ghak120).  As for the middle two
> chunks, they could either be resubmitted with a resubmitted ghak96, or
> with a resubmitted ghak120.  As for the timing of all of these, ghak96
> should be in place before the ghak120 patch, so even resubmitting one
> patch for the combined ghak120 and ghak96 might make more sense.

Sigh.

I can't even reply to that paragraph above without going to GH and
looking up all those different ghak references, which is annoying, and
right now it seems like my time is better spent cleaning up this mess.
I'm not exactly sure what you mean by "one patch", but right now we
are at -rc7 and we've/I've got broken kernels to fix; submit whatever
you want and we'll deal with it when it's posted.

> I know you like only really minimal fixes this late, but this seemed
> pretty minimal to me...

Minimal is a one (two?) line NULL check in audit_log_name(), this
patch is not that.

-- 
paul moore
www.paul-moore.com

--
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

[Richard Guy Briggs]

On 2020-07-28 14:47, Paul Moore wrote:
> On Tue, Jul 28, 2020 at 12:27 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > I know you like only really minimal fixes this late, but this seemed
> > pretty minimal to me...
> 
> Minimal is a one (two?) line NULL check in audit_log_name(), this
> patch is not that.

I didn't try and test that since I'm not sure that would have worked
because there appeared to be a low non-NULL value in it.  brauer1's trace had
0x60 and mine had 0xd0.  Or am I missing something obvious?

The patch provided the information rather than ignoring the problem
(which maybe should have been caught by WARN_ONCE?).

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
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

[Paul Moore]

On Tue, Jul 28, 2020 at 10:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> On 2020-07-28 14:47, Paul Moore wrote:
> > On Tue, Jul 28, 2020 at 12:27 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > I know you like only really minimal fixes this late, but this seemed
> > > pretty minimal to me...
> >
> > Minimal is a one (two?) line NULL check in audit_log_name(), this
> > patch is not that.
>
> I didn't try and test that since I'm not sure that would have worked
> because there appeared to be a low non-NULL value in it.  brauer1's trace had
> 0x60 and mine had 0xd0.  Or am I missing something obvious?

Well, you mentioned the obvious already: both 0x60 and 0xd0 are not
NULL.  We already have a NULL check for context->pwd elsewhere so
there is precedence for this solving a similar problem, although
without going through the git log I'm not sure what problem that
solved, or if it was precautionary.

I agree the low value looks suspicious, it almost looks like an offset
to me, ideally it would be good to understand how/why that value is
"off'.  It could be that the audit_context is not being properly
initialized, reset, or something unrelated is clobbering the value;
all things that would be nice to know.

> The patch provided the information rather than ignoring the problem ...

I disagree.

--
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit