Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH ghak25 v5] audit: add subj creds to NETFILTER_CFG record to cover async unregister
@ 2020-05-19 15:30 Richard Guy Briggs
  2020-05-19 19:18 ` Paul Moore
  0 siblings, 1 reply; 4+ messages in thread
From: Richard Guy Briggs @ 2020-05-19 15:30 UTC (permalink / raw)
  To: Linux-Audit Mailing List, LKML, netfilter-devel
  Cc: Paul Moore, sgrubb, omosnace, fw, twoerner, eparis, tgraf,
	Richard Guy Briggs

Some table unregister actions seem to be initiated by the kernel to
garbage collect unused tables that are not initiated by any userspace
actions.  It was found to be necessary to add the subject credentials to
cover this case to reveal the source of these actions.  A sample record:

The tty, ses and exe fields have not been included since they are in the
SYSCALL record and contain nothing useful in the non-user context.

  type=NETFILTER_CFG msg=audit(2020-03-11 21:25:21.491:269) : table=nat family=bridge entries=0 op=unregister pid=153 uid=root auid=unset subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
Changelog:
v5
- rebase on upstreamed ghak28 on audit/next v5.7-rc1
- remove tty, ses and exe fields as duplicates or unset
- drop upstreamed patches 1&2 from set

v4
- rebase on audit/next v5.7-rc1
- fix checkpatch.pl errors/warnings in 1/3 and 2/3

v3
- rebase on v5.6-rc1 audit/next
- change audit_nf_cfg to audit_log_nfcfg
- squash 2,3,4,5 to 1 and update patch descriptions
- add subject credentials to cover garbage collecting kernel threads

v2
- Rebase (audit/next 5.5-rc1) to get audit_context access and ebt_register_table ret code
- Split x_tables and ebtables updates
- Check audit_dummy_context
- Store struct audit_nfcfg params in audit_context, abstract to audit_nf_cfg() call
- Restore back to "table, family, entries" from "family, table, entries"
- Log unregistration of tables
- Add "op=" at the end of the AUDIT_NETFILTER_CFG record
- Defer nsid patch (ghak79) to once nsid patchset upstreamed (ghak32)
- Add ghak refs
- Ditch NETFILTER_CFGSOLO record

 kernel/auditsc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index cfe3486e5f31..a07ca529ede9 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2557,12 +2557,24 @@ void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
 		       enum audit_nfcfgop op)
 {
 	struct audit_buffer *ab;
+	const struct cred *cred;
+	struct tty_struct *tty;
+	char comm[sizeof(current->comm)];
 
 	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_NETFILTER_CFG);
 	if (!ab)
 		return;
 	audit_log_format(ab, "table=%s family=%u entries=%u op=%s",
 			 name, af, nentries, audit_nfcfgs[op].s);
+
+	cred = current_cred();
+	audit_log_format(ab, " pid=%u uid=%u auid=%u",
+			 task_pid_nr(current),
+			 from_kuid(&init_user_ns, cred->uid),
+			 from_kuid(&init_user_ns, audit_get_loginuid(current)));
+	audit_log_task_context(ab); /* subj= */
+	audit_log_format(ab, " comm=");
+	audit_log_untrustedstring(ab, get_task_comm(comm, current));
 	audit_log_end(ab);
 }
 EXPORT_SYMBOL_GPL(__audit_log_nfcfg);
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH ghak25 v5] audit: add subj creds to NETFILTER_CFG record to cover async unregister
  2020-05-19 15:30 [PATCH ghak25 v5] audit: add subj creds to NETFILTER_CFG record to cover async unregister Richard Guy Briggs
@ 2020-05-19 19:18 ` Paul Moore
  2020-05-19 19:44   ` Richard Guy Briggs
  0 siblings, 1 reply; 4+ messages in thread
From: Paul Moore @ 2020-05-19 19:18 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, netfilter-devel, sgrubb,
	Ondrej Mosnacek, fw, twoerner, Eric Paris, tgraf

On Tue, May 19, 2020 at 11:31 AM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> Some table unregister actions seem to be initiated by the kernel to
> garbage collect unused tables that are not initiated by any userspace
> actions.  It was found to be necessary to add the subject credentials to
> cover this case to reveal the source of these actions.  A sample record:
>
> The tty, ses and exe fields have not been included since they are in the
> SYSCALL record and contain nothing useful in the non-user context.
>
>   type=NETFILTER_CFG msg=audit(2020-03-11 21:25:21.491:269) : table=nat family=bridge entries=0 op=unregister pid=153 uid=root auid=unset subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2

Based on where things were left in the discussion on the previous
draft, I think it would be good if you could explain a bit why the uid
and auid fields are useful here.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH ghak25 v5] audit: add subj creds to NETFILTER_CFG record to cover async unregister
  2020-05-19 19:18 ` Paul Moore
@ 2020-05-19 19:44   ` Richard Guy Briggs
  2020-05-19 19:59     ` Paul Moore
  0 siblings, 1 reply; 4+ messages in thread
From: Richard Guy Briggs @ 2020-05-19 19:44 UTC (permalink / raw)
  To: Paul Moore
  Cc: Linux-Audit Mailing List, LKML, netfilter-devel, sgrubb,
	Ondrej Mosnacek, fw, twoerner, Eric Paris, tgraf

On 2020-05-19 15:18, Paul Moore wrote:
> On Tue, May 19, 2020 at 11:31 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Some table unregister actions seem to be initiated by the kernel to
> > garbage collect unused tables that are not initiated by any userspace
> > actions.  It was found to be necessary to add the subject credentials to
> > cover this case to reveal the source of these actions.  A sample record:
> >
> > The tty, ses and exe fields have not been included since they are in the
> > SYSCALL record and contain nothing useful in the non-user context.
> >
> >   type=NETFILTER_CFG msg=audit(2020-03-11 21:25:21.491:269) : table=nat family=bridge entries=0 op=unregister pid=153 uid=root auid=unset subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2
> 
> Based on where things were left in the discussion on the previous
> draft, I think it would be good if you could explain a bit why the uid
> and auid fields are useful here.

They aren't really useful here.  I was hoping to remove them given your
reasoning, but I was having trouble guessing what you wanted even after
asking for clarity.  Can you clarify what you would prefer to see in
this patch?  I was hoping to skip this extra patch revision which took
longer than hoped due to trying to guess what you wanted while working
yesterday during a public holiday to get this patch out in time for the
merge window.

A UID of 0="root" is really a bit misleading since while it is the most
trusted user running the most privileged level, the event wasn't
triggered by a user.  It is the default value of that field.  I did
think aloud that uid could be set by the kernel to run under a
particular user's id (like a daemon dropping capabilities and switching
user after setup to limit abuse), but the kernel is just a tracker for
these IDs and doesn't really know what they mean other than root.  I saw
no reply to that idea.  It was set to "root" which isn't unset or
unexpected, but granted is useless in this case.

You had offered that keeping auid was a concession to Steve so I kept it
in since I had the impression that is what you wanted to see.  That
explanation seems pretty thin to include in a patch description if what
you are getting at in your sentence above.

I am willing to purge both if that is what you would prefer to accept in
the patch.

> 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH ghak25 v5] audit: add subj creds to NETFILTER_CFG record to cover async unregister
  2020-05-19 19:44   ` Richard Guy Briggs
@ 2020-05-19 19:59     ` Paul Moore
  0 siblings, 0 replies; 4+ messages in thread
From: Paul Moore @ 2020-05-19 19:59 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, netfilter-devel, sgrubb,
	Ondrej Mosnacek, fw, twoerner, Eric Paris, tgraf

On Tue, May 19, 2020 at 3:45 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2020-05-19 15:18, Paul Moore wrote:
> > On Tue, May 19, 2020 at 11:31 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Some table unregister actions seem to be initiated by the kernel to
> > > garbage collect unused tables that are not initiated by any userspace
> > > actions.  It was found to be necessary to add the subject credentials to
> > > cover this case to reveal the source of these actions.  A sample record:
> > >
> > > The tty, ses and exe fields have not been included since they are in the
> > > SYSCALL record and contain nothing useful in the non-user context.
> > >
> > >   type=NETFILTER_CFG msg=audit(2020-03-11 21:25:21.491:269) : table=nat family=bridge entries=0 op=unregister pid=153 uid=root auid=unset subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2
> >
> > Based on where things were left in the discussion on the previous
> > draft, I think it would be good if you could explain a bit why the uid
> > and auid fields are useful here.
>
> They aren't really useful here.  I was hoping to remove them given your
> reasoning, but I was having trouble guessing what you wanted even after
> asking for clarity.  Can you clarify what you would prefer to see in
> this patch?

/me heavily rolls eyes

In my last email to you I said:

"I think it is pointless to record the subject info in this record as we
either have that info from other records in the event or there is no
valid subject info to record."

... I also said:

"As I've mentioned in the thread above, including the auid was done as
a concession to Steve, I don't think it serves any useful purpose."

If phrases like "pointless to record the subject info" and "I don't
think it serves any useful purpose" leave you unsure about what to do,
I'm at a bit of a loss.

Drop the "uid" field.  Drop the "auid" field.  Hopefully those last
two statements should remove any ambiguity from your mind.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-19 15:30 [PATCH ghak25 v5] audit: add subj creds to NETFILTER_CFG record to cover async unregister Richard Guy Briggs
2020-05-19 19:18 ` Paul Moore
2020-05-19 19:44   ` Richard Guy Briggs
2020-05-19 19:59     ` Paul Moore

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git