[PATCH ghak25 v4 0/3] Address NETFILTER_CFG issues

[PATCH ghak25 v4 0/3] Address NETFILTER_CFG issues

[Richard Guy Briggs]

There were questions about the presence and cause of unsolicited syscall events
in the logs containing NETFILTER_CFG records and sometimes unaccompanied
NETFILTER_CFG records.

During testing at least the following list of events trigger NETFILTER_CFG
records and the syscalls related (There may be more events that will trigger
this message type.):
	init_module, finit_module: modprobe
	delete_module: rmmod
	setsockopt: iptables-restore, ip6tables-restore, ebtables-restore
	unshare: (h?)ostnamed, updatedb
	clone: libvirtd
	kernel threads garbage collecting empty ebtables

The syscall events unsolicited by any audit rule were found to be caused by a
missing !audit_dummy_context() check before issuing a NETFILTER_CFG
record.  In fact, since this is a configuration change it is required,
and we always want the accompanying syscall record even with no rules
present, so this has been addressed by ghak120.

The vast majority of unaccompanied records are caused by the fedora default
rule: "-a never,task" and the occasional early startup one is I believe caused
by the iptables filter table module hard linked into the kernel rather than a
loadable module.

A couple of other factors should help eliminate unaccompanied records
which include commit cb74ed278f80 ("audit: always enable syscall
auditing when supported and audit is enabled") which makes sure that
when audit is enabled, so automatically is syscall auditing, and ghak66
which addressed initializing audit before PID 1.

Ebtables module initialization to register tables doesn't generate records
because it was never hooked in to audit.  Recommend adding audit hooks to log
this covered by ghak43 covered by patch 1.

Table unregistration was never logged, which is now covered by ghak44 in
patch 2.  Unaccompanied records were caused by kernel threads
automatically unregistering empty ebtables, which necessitates adding
subject credentials covered in patch 3.

Seemingly duplicate records are not actually exact duplicates that are caused
by netfilter table initialization in different network namespaces from the same
syscall.  Recommend adding the network namespace ID (proc inode and dev)
to the record to make this obvious (address later with ghak79 after nsid
patches).

See: https://github.com/linux-audit/audit-kernel/issues/25
See: https://github.com/linux-audit/audit-kernel/issues/35
See: https://github.com/linux-audit/audit-kernel/issues/43
See: https://github.com/linux-audit/audit-kernel/issues/44

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

Richard Guy Briggs (3):
  audit: tidy and extend netfilter_cfg x_tables and ebtables logging
  netfilter: add audit table unregister actions
  audit: add subj creds to NETFILTER_CFG record to cover async
    unregister

 include/linux/audit.h           | 22 +++++++++++++++++++++
 kernel/auditsc.c                | 43 +++++++++++++++++++++++++++++++++++++++++
 net/bridge/netfilter/ebtables.c | 14 ++++++--------
 net/netfilter/x_tables.c        | 14 +++++---------
 4 files changed, 76 insertions(+), 17 deletions(-)

-- 
1.8.3.1

[PATCH ghak25 v4 1/3] audit: tidy and extend netfilter_cfg x_tables and ebtables logging

[Richard Guy Briggs]

NETFILTER_CFG record generation was inconsistent for x_tables and
ebtables configuration changes.  The call was needlessly messy and there
were supporting records missing at times while they were produced when
not requested.  Simplify the logging call into a new audit_log_nfcfg
call.  Honour the audit_enabled setting while more consistently
recording information including supporting records by tidying up dummy
checks.

Add an op= field that indicates the operation being performed (register
or replace).

Here is the enhanced sample record:
  type=NETFILTER_CFG msg=audit(1580905834.919:82970): table=filter family=2 entries=83 op=replace

Generate audit NETFILTER_CFG records on ebtables table registration.
Previously this was being done for x_tables registration and replacement
operations and ebtables table replacement only.

See: https://github.com/linux-audit/audit-kernel/issues/25
See: https://github.com/linux-audit/audit-kernel/issues/35
See: https://github.com/linux-audit/audit-kernel/issues/43

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/linux/audit.h           | 21 +++++++++++++++++++++
 kernel/auditsc.c                | 24 ++++++++++++++++++++++++
 net/bridge/netfilter/ebtables.c | 12 ++++--------
 net/netfilter/x_tables.c        | 12 +++---------
 4 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index f9ceae57ca8d..5c7f07a9776d 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -94,6 +94,11 @@ struct audit_ntp_data {
 struct audit_ntp_data {};
 #endif
 
+enum audit_nfcfgop {
+	AUDIT_XT_OP_REGISTER,
+	AUDIT_XT_OP_REPLACE,
+};
+
 extern int is_audit_feature_set(int which);
 
 extern int __init audit_register_class(int class, unsigned *list);
@@ -379,6 +384,8 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm *bprm,
 extern void __audit_fanotify(unsigned int response);
 extern void __audit_tk_injoffset(struct timespec64 offset);
 extern void __audit_ntp_log(const struct audit_ntp_data *ad);
+extern void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
+			      enum audit_nfcfgop op);
 
 static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 {
@@ -514,6 +521,14 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
 		__audit_ntp_log(ad);
 }
 
+static inline void audit_log_nfcfg(const char *name, u8 af,
+				   unsigned int nentries,
+				   enum audit_nfcfgop op)
+{
+	if (audit_enabled)
+		__audit_log_nfcfg(name, af, nentries, op);
+}
+
 extern int audit_n_rules;
 extern int audit_signals;
 #else /* CONFIG_AUDITSYSCALL */
@@ -646,6 +661,12 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
 
 static inline void audit_ptrace(struct task_struct *t)
 { }
+
+static inline void audit_log_nfcfg(const char *name, u8 af,
+				   unsigned int nentries,
+				   enum audit_nfcfgop op)
+{ }
+
 #define audit_n_rules 0
 #define audit_signals 0
 #endif /* CONFIG_AUDITSYSCALL */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 814406a35db1..705beac0ce29 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -130,6 +130,16 @@ struct audit_tree_refs {
 	struct audit_chunk *c[31];
 };
 
+struct audit_nfcfgop_tab {
+	enum audit_nfcfgop	op;
+	const char		*s;
+};
+
+const struct audit_nfcfgop_tab audit_nfcfgs[] = {
+	{ AUDIT_XT_OP_REGISTER,	"register"	},
+	{ AUDIT_XT_OP_REPLACE,	"replace"	},
+};
+
 static int audit_match_perm(struct audit_context *ctx, int mask)
 {
 	unsigned n;
@@ -2542,6 +2552,20 @@ void __audit_ntp_log(const struct audit_ntp_data *ad)
 	audit_log_ntp_val(ad, "adjust",	AUDIT_NTP_ADJUST);
 }
 
+void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
+		       enum audit_nfcfgop op)
+{
+	struct audit_buffer *ab;
+
+	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);
+	audit_log_end(ab);
+}
+EXPORT_SYMBOL_GPL(__audit_log_nfcfg);
+
 static void audit_log_task(struct audit_buffer *ab)
 {
 	kuid_t auid, uid;
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 78db58c7aec2..0a148e68b6e1 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1046,14 +1046,8 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
 	vfree(table);
 	vfree(counterstmp);
 
-#ifdef CONFIG_AUDIT
-	if (audit_enabled) {
-		audit_log(audit_context(), GFP_KERNEL,
-			  AUDIT_NETFILTER_CFG,
-			  "table=%s family=%u entries=%u",
-			  repl->name, AF_BRIDGE, repl->nentries);
-	}
-#endif
+	audit_log_nfcfg(repl->name, AF_BRIDGE, repl->nentries,
+			AUDIT_XT_OP_REPLACE);
 	return ret;
 
 free_unlock:
@@ -1223,6 +1217,8 @@ int ebt_register_table(struct net *net, const struct ebt_table *input_table,
 		*res = NULL;
 	}
 
+	audit_log_nfcfg(repl->name, AF_BRIDGE, repl->nentries,
+			AUDIT_XT_OP_REGISTER);
 	return ret;
 free_unlock:
 	mutex_unlock(&ebt_mutex);
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index cd2b034eef59..8f8c5dbf603d 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1408,15 +1408,9 @@ struct xt_table_info *
 		}
 	}
 
-#ifdef CONFIG_AUDIT
-	if (audit_enabled) {
-		audit_log(audit_context(), GFP_KERNEL,
-			  AUDIT_NETFILTER_CFG,
-			  "table=%s family=%u entries=%u",
-			  table->name, table->af, private->number);
-	}
-#endif
-
+	audit_log_nfcfg(table->name, table->af, private->number,
+			!private->number ? AUDIT_XT_OP_REGISTER :
+					   AUDIT_XT_OP_REPLACE);
 	return private;
 }
 EXPORT_SYMBOL_GPL(xt_replace_table);
-- 
1.8.3.1

Re: [PATCH ghak25 v4 1/3] audit: tidy and extend netfilter_cfg x_tables and ebtables logging

[Paul Moore]

On Wed, Apr 22, 2020 at 5:40 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> NETFILTER_CFG record generation was inconsistent for x_tables and
> ebtables configuration changes.  The call was needlessly messy and there
> were supporting records missing at times while they were produced when
> not requested.  Simplify the logging call into a new audit_log_nfcfg
> call.  Honour the audit_enabled setting while more consistently
> recording information including supporting records by tidying up dummy
> checks.
>
> Add an op= field that indicates the operation being performed (register
> or replace).
>
> Here is the enhanced sample record:
>   type=NETFILTER_CFG msg=audit(1580905834.919:82970): table=filter family=2 entries=83 op=replace
>
> Generate audit NETFILTER_CFG records on ebtables table registration.
> Previously this was being done for x_tables registration and replacement
> operations and ebtables table replacement only.
>
> See: https://github.com/linux-audit/audit-kernel/issues/25
> See: https://github.com/linux-audit/audit-kernel/issues/35
> See: https://github.com/linux-audit/audit-kernel/issues/43
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h           | 21 +++++++++++++++++++++
>  kernel/auditsc.c                | 24 ++++++++++++++++++++++++
>  net/bridge/netfilter/ebtables.c | 12 ++++--------
>  net/netfilter/x_tables.c        | 12 +++---------
>  4 files changed, 52 insertions(+), 17 deletions(-)

Merged into audit/next, thanks.

-- 
paul moore
www.paul-moore.com

[PATCH ghak25 v4 2/3] netfilter: add audit table unregister actions

[Richard Guy Briggs]

Audit the action of unregistering ebtables and x_tables.

See: https://github.com/linux-audit/audit-kernel/issues/44
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/linux/audit.h           | 1 +
 kernel/auditsc.c                | 5 +++--
 net/bridge/netfilter/ebtables.c | 2 ++
 net/netfilter/x_tables.c        | 2 ++
 4 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 5c7f07a9776d..1a2351508211 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -97,6 +97,7 @@ struct audit_ntp_data {
 enum audit_nfcfgop {
 	AUDIT_XT_OP_REGISTER,
 	AUDIT_XT_OP_REPLACE,
+	AUDIT_XT_OP_UNREGISTER,
 };
 
 extern int is_audit_feature_set(int which);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 705beac0ce29..d281c18d1771 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -136,8 +136,9 @@ struct audit_nfcfgop_tab {
 };
 
 const struct audit_nfcfgop_tab audit_nfcfgs[] = {
-	{ AUDIT_XT_OP_REGISTER,	"register"	},
-	{ AUDIT_XT_OP_REPLACE,	"replace"	},
+	{ AUDIT_XT_OP_REGISTER,		"register"	},
+	{ AUDIT_XT_OP_REPLACE,		"replace"	},
+	{ AUDIT_XT_OP_UNREGISTER,	"unregister"	},
 };
 
 static int audit_match_perm(struct audit_context *ctx, int mask)
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 0a148e68b6e1..4778db5601b0 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1124,6 +1124,8 @@ static void __ebt_unregister_table(struct net *net, struct ebt_table *table)
 	mutex_lock(&ebt_mutex);
 	list_del(&table->list);
 	mutex_unlock(&ebt_mutex);
+	audit_log_nfcfg(table->name, AF_BRIDGE, table->private->nentries,
+			AUDIT_XT_OP_UNREGISTER);
 	EBT_ENTRY_ITERATE(table->private->entries, table->private->entries_size,
 			  ebt_cleanup_entry, net, NULL);
 	if (table->private->nentries)
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 8f8c5dbf603d..99a468be4a59 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1472,6 +1472,8 @@ void *xt_unregister_table(struct xt_table *table)
 	private = table->private;
 	list_del(&table->list);
 	mutex_unlock(&xt[table->af].mutex);
+	audit_log_nfcfg(table->name, table->af, private->number,
+			AUDIT_XT_OP_UNREGISTER);
 	kfree(table);
 
 	return private;
-- 
1.8.3.1

Re: [PATCH ghak25 v4 2/3] netfilter: add audit table unregister actions

[Paul Moore]

On Wed, Apr 22, 2020 at 5:40 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> Audit the action of unregistering ebtables and x_tables.
>
> See: https://github.com/linux-audit/audit-kernel/issues/44
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h           | 1 +
>  kernel/auditsc.c                | 5 +++--
>  net/bridge/netfilter/ebtables.c | 2 ++
>  net/netfilter/x_tables.c        | 2 ++
>  4 files changed, 8 insertions(+), 2 deletions(-)

Merged into audit/next, thanks.

-- 
paul moore
www.paul-moore.com

[PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister

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

  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 tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2 exe=(null)

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/auditsc.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index d281c18d1771..d7a45b181be0 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2557,12 +2557,30 @@ 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();
+	tty = audit_get_tty();
+	audit_log_format(ab, " pid=%u uid=%u auid=%u tty=%s ses=%u",
+			 task_pid_nr(current),
+			 from_kuid(&init_user_ns, cred->uid),
+			 from_kuid(&init_user_ns, audit_get_loginuid(current)),
+			 tty ? tty_name(tty) : "(none)",
+			 audit_get_sessionid(current));
+	audit_put_tty(tty);
+	audit_log_task_context(ab); /* subj= */
+	audit_log_format(ab, " comm=");
+	audit_log_untrustedstring(ab, get_task_comm(comm, current));
+	audit_log_d_path_exe(ab, current->mm); /* exe= */
+
 	audit_log_end(ab);
 }
 EXPORT_SYMBOL_GPL(__audit_log_nfcfg);
-- 
1.8.3.1

Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister

[Paul Moore]

On Wed, Apr 22, 2020 at 5:40 PM 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:
>
>   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 tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2 exe=(null)

[I'm going to comment up here instead of in the code because it is a
bit easier for everyone to see what the actual impact might be on the
records.]

Steve wants subject info in this case, okay, but let's try to trim out
some of the fields which simply don't make sense in this record; I'm
thinking of fields that are unset/empty in the kernel case and are
duplicates of other records in the userspace/syscall case.  I think
that means we can drop "tty", "ses", "comm", and "exe" ... yes?

While "auid" is a potential target for removal based on the
dup-or-unset criteria, I think it falls under Steve's request for
subject info here, even if it is garbage in this case.

> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/auditsc.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d281c18d1771..d7a45b181be0 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2557,12 +2557,30 @@ 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();
> +       tty = audit_get_tty();
> +       audit_log_format(ab, " pid=%u uid=%u auid=%u tty=%s ses=%u",
> +                        task_pid_nr(current),
> +                        from_kuid(&init_user_ns, cred->uid),
> +                        from_kuid(&init_user_ns, audit_get_loginuid(current)),
> +                        tty ? tty_name(tty) : "(none)",
> +                        audit_get_sessionid(current));
> +       audit_put_tty(tty);
> +       audit_log_task_context(ab); /* subj= */
> +       audit_log_format(ab, " comm=");
> +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> +       audit_log_d_path_exe(ab, current->mm); /* exe= */
> +
>         audit_log_end(ab);
>  }
>  EXPORT_SYMBOL_GPL(__audit_log_nfcfg);

-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister

[Richard Guy Briggs]

On 2020-04-28 18:25, Paul Moore wrote:
> On Wed, Apr 22, 2020 at 5:40 PM 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:
> >
> >   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 tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2 exe=(null)
> 
> [I'm going to comment up here instead of in the code because it is a
> bit easier for everyone to see what the actual impact might be on the
> records.]
> 
> Steve wants subject info in this case, okay, but let's try to trim out
> some of the fields which simply don't make sense in this record; I'm
> thinking of fields that are unset/empty in the kernel case and are
> duplicates of other records in the userspace/syscall case.  I think
> that means we can drop "tty", "ses", "comm", and "exe" ... yes?

From the ghak28 discussion, this list and order was selected due to
Steve's preference for the "kernel" record convention, so deviating from
this will create yet a new field list.  I'll defer to Steve on this.  It
also has to do with the searchability of fields if they are missing.

I do agree that some fields will be superfluous in the kernel case.
The most important field would be "subj", but then "pid" and "comm", I
would think.  Based on this contents of the "subj" field, I'd think that
"uid", "auid", "tty", "ses" and "exe" are not needed.

> While "auid" is a potential target for removal based on the
> dup-or-unset criteria, I think it falls under Steve's request for
> subject info here, even if it is garbage in this case.

If we keep auid, I'd say keep ses, since they usually go together,
though they are separated by another field in this "kernel" record field
ordering.

I expect this orphan record to occur so infrequently that I don't think
bandwidth or space are a serious concern.

> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/auditsc.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index d281c18d1771..d7a45b181be0 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2557,12 +2557,30 @@ 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();
> > +       tty = audit_get_tty();
> > +       audit_log_format(ab, " pid=%u uid=%u auid=%u tty=%s ses=%u",
> > +                        task_pid_nr(current),
> > +                        from_kuid(&init_user_ns, cred->uid),
> > +                        from_kuid(&init_user_ns, audit_get_loginuid(current)),
> > +                        tty ? tty_name(tty) : "(none)",
> > +                        audit_get_sessionid(current));
> > +       audit_put_tty(tty);
> > +       audit_log_task_context(ab); /* subj= */
> > +       audit_log_format(ab, " comm=");
> > +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > +       audit_log_d_path_exe(ab, current->mm); /* exe= */
> > +
> >         audit_log_end(ab);
> >  }
> >  EXPORT_SYMBOL_GPL(__audit_log_nfcfg);
> 
> -- 
> paul moore
> www.paul-moore.com
> 

- 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

Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister

[Steve Grubb]

On Wednesday, April 29, 2020 10:31:46 AM EDT Richard Guy Briggs wrote:
> On 2020-04-28 18:25, Paul Moore wrote:
> > On Wed, Apr 22, 2020 at 5:40 PM 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:
> > >   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
> > >   tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0
> > >   comm=kworker/u4:2 exe=(null)> 
> > [I'm going to comment up here instead of in the code because it is a
> > bit easier for everyone to see what the actual impact might be on the
> > records.]
> > 
> > Steve wants subject info in this case, okay, but let's try to trim out
> > some of the fields which simply don't make sense in this record; I'm
> > thinking of fields that are unset/empty in the kernel case and are
> > duplicates of other records in the userspace/syscall case.  I think
> > that means we can drop "tty", "ses", "comm", and "exe" ... yes?
> 
> From the ghak28 discussion, this list and order was selected due to
> Steve's preference for the "kernel" record convention, so deviating from
> this will create yet a new field list.  I'll defer to Steve on this.  It
> also has to do with the searchability of fields if they are missing.
> 
> I do agree that some fields will be superfluous in the kernel case.
> The most important field would be "subj", but then "pid" and "comm", I
> would think.  Based on this contents of the "subj" field, I'd think that
> "uid", "auid", "tty", "ses" and "exe" are not needed.

We can't be adding deleting fields based on how its triggered. If they are 
unset, that is fine. The main issue is they have to behave the same.

> > While "auid" is a potential target for removal based on the
> > dup-or-unset criteria, I think it falls under Steve's request for
> > subject info here, even if it is garbage in this case.

auid is always unset for daemons. We do not throw it away because of that.

-Steve

> If we keep auid, I'd say keep ses, since they usually go together,
> though they are separated by another field in this "kernel" record field
> ordering.
> 
> I expect this orphan record to occur so infrequently that I don't think
> bandwidth or space are a serious concern.
> 
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > > 
> > >  kernel/auditsc.c | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index d281c18d1771..d7a45b181be0 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -2557,12 +2557,30 @@ 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();
> > > +       tty = audit_get_tty();
> > > +       audit_log_format(ab, " pid=%u uid=%u auid=%u tty=%s ses=%u",
> > > +                        task_pid_nr(current),
> > > +                        from_kuid(&init_user_ns, cred->uid),
> > > +                        from_kuid(&init_user_ns,
> > > audit_get_loginuid(current)), +                        tty ?
> > > tty_name(tty) : "(none)",
> > > +                        audit_get_sessionid(current));
> > > +       audit_put_tty(tty);
> > > +       audit_log_task_context(ab); /* subj= */
> > > +       audit_log_format(ab, " comm=");
> > > +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > > +       audit_log_d_path_exe(ab, current->mm); /* exe= */
> > > +
> > > 
> > >         audit_log_end(ab);
> > >  
> > >  }
> > >  EXPORT_SYMBOL_GPL(__audit_log_nfcfg);
> 
> - 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

Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister

[Richard Guy Briggs]

On 2020-04-29 14:47, Steve Grubb wrote:
> On Wednesday, April 29, 2020 10:31:46 AM EDT Richard Guy Briggs wrote:
> > On 2020-04-28 18:25, Paul Moore wrote:
> > > On Wed, Apr 22, 2020 at 5:40 PM 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:
> > > >   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
> > > >   tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0
> > > >   comm=kworker/u4:2 exe=(null)> 
> > > [I'm going to comment up here instead of in the code because it is a
> > > bit easier for everyone to see what the actual impact might be on the
> > > records.]
> > > 
> > > Steve wants subject info in this case, okay, but let's try to trim out
> > > some of the fields which simply don't make sense in this record; I'm
> > > thinking of fields that are unset/empty in the kernel case and are
> > > duplicates of other records in the userspace/syscall case.  I think
> > > that means we can drop "tty", "ses", "comm", and "exe" ... yes?
> > 
> > From the ghak28 discussion, this list and order was selected due to
> > Steve's preference for the "kernel" record convention, so deviating from
> > this will create yet a new field list.  I'll defer to Steve on this.  It
> > also has to do with the searchability of fields if they are missing.
> > 
> > I do agree that some fields will be superfluous in the kernel case.
> > The most important field would be "subj", but then "pid" and "comm", I
> > would think.  Based on this contents of the "subj" field, I'd think that
> > "uid", "auid", "tty", "ses" and "exe" are not needed.
> 
> We can't be adding deleting fields based on how its triggered. If they are 
> unset, that is fine. The main issue is they have to behave the same.

I don't think the intent was to have fields swing in and out depending
on trigger.  The idea is to potentially permanently not include them in
this record type only.  The justification is that where they aren't
needed for the kernel trigger situation it made sense to delete them
because if it is a user context event it will be accompanied by a
syscall record that already has that information and there would be no
sense in duplicating it.

> > > While "auid" is a potential target for removal based on the
> > > dup-or-unset criteria, I think it falls under Steve's request for
> > > subject info here, even if it is garbage in this case.
> 
> auid is always unset for daemons. We do not throw it away because of that.
> 
> -Steve
> 
> > If we keep auid, I'd say keep ses, since they usually go together,
> > though they are separated by another field in this "kernel" record field
> > ordering.
> > 
> > I expect this orphan record to occur so infrequently that I don't think
> > bandwidth or space are a serious concern.
> > 
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > > 
> > > >  kernel/auditsc.c | 18 ++++++++++++++++++
> > > >  1 file changed, 18 insertions(+)
> > > > 
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index d281c18d1771..d7a45b181be0 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -2557,12 +2557,30 @@ 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();
> > > > +       tty = audit_get_tty();
> > > > +       audit_log_format(ab, " pid=%u uid=%u auid=%u tty=%s ses=%u",
> > > > +                        task_pid_nr(current),
> > > > +                        from_kuid(&init_user_ns, cred->uid),
> > > > +                        from_kuid(&init_user_ns,
> > > > audit_get_loginuid(current)), +                        tty ?
> > > > tty_name(tty) : "(none)",
> > > > +                        audit_get_sessionid(current));
> > > > +       audit_put_tty(tty);
> > > > +       audit_log_task_context(ab); /* subj= */
> > > > +       audit_log_format(ab, " comm=");
> > > > +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > > > +       audit_log_d_path_exe(ab, current->mm); /* exe= */
> > > > +
> > > > 
> > > >         audit_log_end(ab);
> > > >  
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(__audit_log_nfcfg);
> > 
> > - RGB

- 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

Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister

[Paul Moore]

On Wed, Apr 29, 2020 at 5:33 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2020-04-29 14:47, Steve Grubb wrote:
> > On Wednesday, April 29, 2020 10:31:46 AM EDT Richard Guy Briggs wrote:
> > > On 2020-04-28 18:25, Paul Moore wrote:
> > > > On Wed, Apr 22, 2020 at 5:40 PM 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:
> > > > >   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
> > > > >   tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0
> > > > >   comm=kworker/u4:2 exe=(null)>
> > > > [I'm going to comment up here instead of in the code because it is a
> > > > bit easier for everyone to see what the actual impact might be on the
> > > > records.]
> > > >
> > > > Steve wants subject info in this case, okay, but let's try to trim out
> > > > some of the fields which simply don't make sense in this record; I'm
> > > > thinking of fields that are unset/empty in the kernel case and are
> > > > duplicates of other records in the userspace/syscall case.  I think
> > > > that means we can drop "tty", "ses", "comm", and "exe" ... yes?
> > >
> > > From the ghak28 discussion, this list and order was selected due to
> > > Steve's preference for the "kernel" record convention, so deviating from
> > > this will create yet a new field list.  I'll defer to Steve on this.  It
> > > also has to do with the searchability of fields if they are missing.
> > >
> > > I do agree that some fields will be superfluous in the kernel case.
> > > The most important field would be "subj", but then "pid" and "comm", I
> > > would think.  Based on this contents of the "subj" field, I'd think that
> > > "uid", "auid", "tty", "ses" and "exe" are not needed.
> >
> > We can't be adding deleting fields based on how its triggered. If they are
> > unset, that is fine. The main issue is they have to behave the same.
>
> I don't think the intent was to have fields swing in and out depending
> on trigger.  The idea is to potentially permanently not include them in
> this record type only.  The justification is that where they aren't
> needed for the kernel trigger situation it made sense to delete them
> because if it is a user context event it will be accompanied by a
> syscall record that already has that information and there would be no
> sense in duplicating it.

Yes, exactly.

-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister

[Steve Grubb]

On Wednesday, April 29, 2020 5:32:47 PM EDT Richard Guy Briggs wrote:
> On 2020-04-29 14:47, Steve Grubb wrote:
> > On Wednesday, April 29, 2020 10:31:46 AM EDT Richard Guy Briggs wrote:
> > > On 2020-04-28 18:25, Paul Moore wrote:
> > > > On Wed, Apr 22, 2020 at 5:40 PM 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:
> > > > > 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
> > > > > tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0
> > > > > comm=kworker/u4:2 exe=(null)>
> > > > 
> > > > [I'm going to comment up here instead of in the code because it is a
> > > > bit easier for everyone to see what the actual impact might be on the
> > > > records.]
> > > > 
> > > > Steve wants subject info in this case, okay, but let's try to trim
> > > > out
> > > > some of the fields which simply don't make sense in this record; I'm
> > > > thinking of fields that are unset/empty in the kernel case and are
> > > > duplicates of other records in the userspace/syscall case.  I think
> > > > that means we can drop "tty", "ses", "comm", and "exe" ... yes?
> > > 
> > > From the ghak28 discussion, this list and order was selected due to
> > > Steve's preference for the "kernel" record convention, so deviating
> > > from this will create yet a new field list.  I'll defer to Steve on
> > > this. It also has to do with the searchability of fields if they are
> > > missing.
> > > 
> > > I do agree that some fields will be superfluous in the kernel case.
> > > The most important field would be "subj", but then "pid" and "comm", I
> > > would think.  Based on this contents of the "subj" field, I'd think
> > > that "uid", "auid", "tty", "ses" and "exe" are not needed.
> > 
> > We can't be adding deleting fields based on how its triggered. If they
> > are unset, that is fine. The main issue is they have to behave the same.
> 
> I don't think the intent was to have fields swing in and out depending
> on trigger.  The idea is to potentially permanently not include them in
> this record type only.  The justification is that where they aren't
> needed for the kernel trigger situation it made sense to delete them
> because if it is a user context event it will be accompanied by a
> syscall record that already has that information and there would be no
> sense in duplicating it.

We should not be adding syscall records to anything that does not result from 
a syscall rule triggering the event. Its very wasteful. More wasteful than 
just adding the necessary fields.

I also wished we had a coding specification that put this in writing so that 
every event is not a committee decision. That anyone can look at the document 
and Do The Right Thing ™.

If I add a section to Writing-Good-Events outlining the expected ordering of 
fields, would that be enough that we do not have long discussions about event 
format? I'm thinking this would also help new people that want to contribute.

-Steve

Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister

[Richard Guy Briggs]

On 2020-05-06 17:26, Steve Grubb wrote:
> On Wednesday, April 29, 2020 5:32:47 PM EDT Richard Guy Briggs wrote:
> > On 2020-04-29 14:47, Steve Grubb wrote:
> > > On Wednesday, April 29, 2020 10:31:46 AM EDT Richard Guy Briggs wrote:
> > > > On 2020-04-28 18:25, Paul Moore wrote:
> > > > > On Wed, Apr 22, 2020 at 5:40 PM 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:
> > > > > > 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
> > > > > > tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0
> > > > > > comm=kworker/u4:2 exe=(null)>
> > > > > 
> > > > > [I'm going to comment up here instead of in the code because it is a
> > > > > bit easier for everyone to see what the actual impact might be on the
> > > > > records.]
> > > > > 
> > > > > Steve wants subject info in this case, okay, but let's try to trim
> > > > > out
> > > > > some of the fields which simply don't make sense in this record; I'm
> > > > > thinking of fields that are unset/empty in the kernel case and are
> > > > > duplicates of other records in the userspace/syscall case.  I think
> > > > > that means we can drop "tty", "ses", "comm", and "exe" ... yes?
> > > > 
> > > > From the ghak28 discussion, this list and order was selected due to
> > > > Steve's preference for the "kernel" record convention, so deviating
> > > > from this will create yet a new field list.  I'll defer to Steve on
> > > > this. It also has to do with the searchability of fields if they are
> > > > missing.
> > > > 
> > > > I do agree that some fields will be superfluous in the kernel case.
> > > > The most important field would be "subj", but then "pid" and "comm", I
> > > > would think.  Based on this contents of the "subj" field, I'd think
> > > > that "uid", "auid", "tty", "ses" and "exe" are not needed.
> > > 
> > > We can't be adding deleting fields based on how its triggered. If they
> > > are unset, that is fine. The main issue is they have to behave the same.
> > 
> > I don't think the intent was to have fields swing in and out depending
> > on trigger.  The idea is to potentially permanently not include them in
> > this record type only.  The justification is that where they aren't
> > needed for the kernel trigger situation it made sense to delete them
> > because if it is a user context event it will be accompanied by a
> > syscall record that already has that information and there would be no
> > sense in duplicating it.
> 
> We should not be adding syscall records to anything that does not result from 
> a syscall rule triggering the event. Its very wasteful. More wasteful than 
> just adding the necessary fields.

So what you are saying is you want all the fields that are being
proposed to be added to this record?

If the records are all from one event, they all should all have the same
timestamp/serial number so that the records are kept together and not
mistaken for multiple events.  One reason for having information in
seperate records is to be able to filter them either in kernel or in
userspace if you don't need certain records.

> I also wished we had a coding specification that put this in writing so that 
> every event is not a committee decision. That anyone can look at the document 
> and Do The Right Thing ™.
> 
> If I add a section to Writing-Good-Events outlining the expected ordering of 
> fields, would that be enough that we do not have long discussions about event 
> format? I'm thinking this would also help new people that want to contribute.

If you add this expected ordering of fields, can we re-factor all the
kernel code to use this order because the userspace parser won't care
what order they are in?

I realize this isn't what you are saying, but having a clear description
in that document that talks about the different classes of events and
what each one needs in terms of minimum to full subject attributes and
object attributes would help a lot.  It would also help for new records
to be able to decide if it should follow the format of an existing
related or similar record, or a new class with the expected standard order.

> -Steve

- 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

Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister

[Paul Moore]

On Wed, May 6, 2020 at 6:43 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2020-05-06 17:26, Steve Grubb wrote:
> > On Wednesday, April 29, 2020 5:32:47 PM EDT Richard Guy Briggs wrote:
> > > On 2020-04-29 14:47, Steve Grubb wrote:
> > > > On Wednesday, April 29, 2020 10:31:46 AM EDT Richard Guy Briggs wrote:
> > > > > On 2020-04-28 18:25, Paul Moore wrote:
> > > > > > On Wed, Apr 22, 2020 at 5:40 PM 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:
> > > > > > > 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
> > > > > > > tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0
> > > > > > > comm=kworker/u4:2 exe=(null)>
> > > > > >
> > > > > > [I'm going to comment up here instead of in the code because it is a
> > > > > > bit easier for everyone to see what the actual impact might be on the
> > > > > > records.]
> > > > > >
> > > > > > Steve wants subject info in this case, okay, but let's try to trim
> > > > > > out
> > > > > > some of the fields which simply don't make sense in this record; I'm
> > > > > > thinking of fields that are unset/empty in the kernel case and are
> > > > > > duplicates of other records in the userspace/syscall case.  I think
> > > > > > that means we can drop "tty", "ses", "comm", and "exe" ... yes?
> > > > >
> > > > > From the ghak28 discussion, this list and order was selected due to
> > > > > Steve's preference for the "kernel" record convention, so deviating
> > > > > from this will create yet a new field list.  I'll defer to Steve on
> > > > > this. It also has to do with the searchability of fields if they are
> > > > > missing.
> > > > >
> > > > > I do agree that some fields will be superfluous in the kernel case.
> > > > > The most important field would be "subj", but then "pid" and "comm", I
> > > > > would think.  Based on this contents of the "subj" field, I'd think
> > > > > that "uid", "auid", "tty", "ses" and "exe" are not needed.
> > > >
> > > > We can't be adding deleting fields based on how its triggered. If they
> > > > are unset, that is fine. The main issue is they have to behave the same.
> > >
> > > I don't think the intent was to have fields swing in and out depending
> > > on trigger.  The idea is to potentially permanently not include them in
> > > this record type only.  The justification is that where they aren't
> > > needed for the kernel trigger situation it made sense to delete them
> > > because if it is a user context event it will be accompanied by a
> > > syscall record that already has that information and there would be no
> > > sense in duplicating it.
> >
> > We should not be adding syscall records to anything that does not result from
> > a syscall rule triggering the event. Its very wasteful. More wasteful than
> > just adding the necessary fields.
>
> So what you are saying is you want all the fields that are being
> proposed to be added to this record?
>
> If the records are all from one event, they all should all have the same
> timestamp/serial number so that the records are kept together and not
> mistaken for multiple events.  One reason for having information in
> seperate records is to be able to filter them either in kernel or in
> userspace if you don't need certain records.

Yes, I'm opposed to duplicating fields across records in a single
event.  If there are cases where we have a standalone record, such as
with "unregister", then there is an argument to be made about
duplicating some fields that are important in the standalone
unregister case.  However, this is *only* for those fields which make
sense in the standalone kernel unregister event; if the field isn't
useful in this unregister corner case *and* it is duplicated in
another record type which normally accompanies this record in an event
there is no reason it needs to be in this record.

> > I also wished we had a coding specification that put this in writing so that
> > every event is not a committee decision. That anyone can look at the document
> > and Do The Right Thing ™.
> >
> > If I add a section to Writing-Good-Events outlining the expected ordering of
> > fields, would that be enough that we do not have long discussions about event
> > format? I'm thinking this would also help new people that want to contribute.

To be clear, we are not changing any existing record formats; they are
part of the kernel/userspace ABI and changing them would break the
ABI.

In a perfect world both the audit kernel and userspace would have been
designed, implemented, and documented better.  Unfortunately it wasn't
and we have to live with what we have.

-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister

[Steve Grubb]

On Wednesday, May 6, 2020 6:42:33 PM EDT Richard Guy Briggs wrote:
> > > > We can't be adding deleting fields based on how its triggered. If
> > > > they are unset, that is fine. The main issue is they have to behave
> > > > the same.
> > > 
> > > I don't think the intent was to have fields swing in and out depending
> > > on trigger.  The idea is to potentially permanently not include them in
> > > this record type only.  The justification is that where they aren't
> > > needed for the kernel trigger situation it made sense to delete them
> > > because if it is a user context event it will be accompanied by a
> > > syscall record that already has that information and there would be no
> > > sense in duplicating it.
> > 
> > We should not be adding syscall records to anything that does not result
> > from a syscall rule triggering the event. Its very wasteful. More
> > wasteful than just adding the necessary fields.
> 
> So what you are saying is you want all the fields that are being
> proposed to be added to this record?

Yes.

> If the records are all from one event, they all should all have the same
> timestamp/serial number so that the records are kept together and not
> mistaken for multiple events.

But NETFILTER_CFG is a simple event known to have only 1 record.

> One reason for having information in seperate records is to be able to
> filter them either in kernel or in userspace if you don't need certain
> records.

We can't filter out SYSCALL.

-Steve

Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister

[Richard Guy Briggs]

On 2020-04-28 18:25, Paul Moore wrote:
> On Wed, Apr 22, 2020 at 5:40 PM 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:
> >
> >   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 tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2 exe=(null)
> 
> [I'm going to comment up here instead of in the code because it is a
> bit easier for everyone to see what the actual impact might be on the
> records.]
> 
> Steve wants subject info in this case, okay, but let's try to trim out
> some of the fields which simply don't make sense in this record; I'm
> thinking of fields that are unset/empty in the kernel case and are
> duplicates of other records in the userspace/syscall case.  I think
> that means we can drop "tty", "ses", "comm", and "exe" ... yes?
> 
> While "auid" is a potential target for removal based on the
> dup-or-unset criteria, I think it falls under Steve's request for
> subject info here, even if it is garbage in this case.

Can you explain why auid falls under this criteria but ses does not if
both are unset?  If auid is unset then we know ses is unset?  If subj
contains *:kernel_t:* then uid can also be dropped even though it is
set, no?  I figure if we are going to start dropping fields, might as
well drop enough to make it worth the effort, even though this is a rare
event.

As for searchability, I have solved that easily in the parser.

> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/auditsc.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index d281c18d1771..d7a45b181be0 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2557,12 +2557,30 @@ 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();
> > +       tty = audit_get_tty();
> > +       audit_log_format(ab, " pid=%u uid=%u auid=%u tty=%s ses=%u",
> > +                        task_pid_nr(current),
> > +                        from_kuid(&init_user_ns, cred->uid),
> > +                        from_kuid(&init_user_ns, audit_get_loginuid(current)),
> > +                        tty ? tty_name(tty) : "(none)",
> > +                        audit_get_sessionid(current));
> > +       audit_put_tty(tty);
> > +       audit_log_task_context(ab); /* subj= */
> > +       audit_log_format(ab, " comm=");
> > +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > +       audit_log_d_path_exe(ab, current->mm); /* exe= */
> > +
> >         audit_log_end(ab);
> >  }
> >  EXPORT_SYMBOL_GPL(__audit_log_nfcfg);
> 
> 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

Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister

[Paul Moore]

On Sun, May 17, 2020 at 10:15 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2020-04-28 18:25, Paul Moore wrote:
> > On Wed, Apr 22, 2020 at 5:40 PM 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:
> > >
> > >   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 tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2 exe=(null)
> >
> > [I'm going to comment up here instead of in the code because it is a
> > bit easier for everyone to see what the actual impact might be on the
> > records.]
> >
> > Steve wants subject info in this case, okay, but let's try to trim out
> > some of the fields which simply don't make sense in this record; I'm
> > thinking of fields that are unset/empty in the kernel case and are
> > duplicates of other records in the userspace/syscall case.  I think
> > that means we can drop "tty", "ses", "comm", and "exe" ... yes?
> >
> > While "auid" is a potential target for removal based on the
> > dup-or-unset criteria, I think it falls under Steve's request for
> > subject info here, even if it is garbage in this case.
>
> Can you explain why auid falls under this criteria but ses does not if
> both are unset?

"While "auid" is a potential target for removal based on the
dup-or-unset criteria, I think it falls under Steve's request for
subject info here, even if it is garbage in this case."

It's a concession to Steve.  As I mentioned previously, I think the
subject info is bogus in this case; either it is valid and we get it
from the SYSCALL record or it simply isn't present in any meaningful
way.

-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister

[Richard Guy Briggs]

On 2020-05-17 17:50, Paul Moore wrote:
> On Sun, May 17, 2020 at 10:15 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2020-04-28 18:25, Paul Moore wrote:
> > > On Wed, Apr 22, 2020 at 5:40 PM 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:
> > > >
> > > >   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 tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2 exe=(null)
> > >
> > > [I'm going to comment up here instead of in the code because it is a
> > > bit easier for everyone to see what the actual impact might be on the
> > > records.]
> > >
> > > Steve wants subject info in this case, okay, but let's try to trim out
> > > some of the fields which simply don't make sense in this record; I'm
> > > thinking of fields that are unset/empty in the kernel case and are
> > > duplicates of other records in the userspace/syscall case.  I think
> > > that means we can drop "tty", "ses", "comm", and "exe" ... yes?
> > >
> > > While "auid" is a potential target for removal based on the
> > > dup-or-unset criteria, I think it falls under Steve's request for
> > > subject info here, even if it is garbage in this case.
> >
> > Can you explain why auid falls under this criteria but ses does not if
> > both are unset?
> 
> "While "auid" is a potential target for removal based on the
> dup-or-unset criteria, I think it falls under Steve's request for
> subject info here, even if it is garbage in this case."
> 
> It's a concession to Steve.  As I mentioned previously, I think the
> subject info is bogus in this case; either it is valid and we get it
> from the SYSCALL record or it simply isn't present in any meaningful
> way.

Sorry for being so dense.  I still don't follow your explanation.  You've
repeated the same paragraph that didn't make sense to me the first time.

What definition of "subject info" are you working with?  I had assumed
it was the set of fields that contain information that came from that
task's struct task_struct.  Some of those fields contain information
that isn't helpful.  Why not remove them all rather than keep one that
still contains no useful information?  Or is it a matter of keeping one
key field that contains no useful information that proves that the rest
is bogus?  Steve said that daemons leave no useful information in auid
as well, so I don't see how keeping this field helps us.  My
understanding is that the subj field's "...:kernel_t:..." is the key
here and that pid and comm give us a bit more of a clue that it is a
kernel thread.  Is that correct?  What use does including auid serve
here?

I suppose that the uid field is somewhat useful, since the kernel could
conceivably switch to a particular user to run a kernel thread.  Is that
even currently possible?

> 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

Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister

[Paul Moore]

On Sun, May 17, 2020 at 8:40 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2020-05-17 17:50, Paul Moore wrote:
> > On Sun, May 17, 2020 at 10:15 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2020-04-28 18:25, Paul Moore wrote:
> > > > On Wed, Apr 22, 2020 at 5:40 PM 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:
> > > > >
> > > > >   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 tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2 exe=(null)
> > > >
> > > > [I'm going to comment up here instead of in the code because it is a
> > > > bit easier for everyone to see what the actual impact might be on the
> > > > records.]
> > > >
> > > > Steve wants subject info in this case, okay, but let's try to trim out
> > > > some of the fields which simply don't make sense in this record; I'm
> > > > thinking of fields that are unset/empty in the kernel case and are
> > > > duplicates of other records in the userspace/syscall case.  I think
> > > > that means we can drop "tty", "ses", "comm", and "exe" ... yes?
> > > >
> > > > While "auid" is a potential target for removal based on the
> > > > dup-or-unset criteria, I think it falls under Steve's request for
> > > > subject info here, even if it is garbage in this case.
> > >
> > > Can you explain why auid falls under this criteria but ses does not if
> > > both are unset?
> >
> > "While "auid" is a potential target for removal based on the
> > dup-or-unset criteria, I think it falls under Steve's request for
> > subject info here, even if it is garbage in this case."
> >
> > It's a concession to Steve.  As I mentioned previously, I think the
> > subject info is bogus in this case; either it is valid and we get it
> > from the SYSCALL record or it simply isn't present in any meaningful
> > way.
>
> Sorry for being so dense.  I still don't follow your explanation.  You've
> repeated the same paragraph that didn't make sense to me the first time.
>
> What definition of "subject info" are you working with?

The subject is generally the task which is causing the event to occur,
"subject info" would be any such attribute which describes the
subject; examples include LSM label, the various UIDs/GIDs, etc..

Think "current->cred" and you on the right track.

> I had assumed
> it was the set of fields that contain information that came from that
> task's struct task_struct.  Some of those fields contain information
> that isn't helpful.  Why not remove them all rather than keep one that
> still contains no useful information?

Once again - and I don't know how else to explain this to you - 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.  As you state in the commit description:

  "Some table unregister actions seem to be initiated by the
   kernel to garbage collect unused tables that are not
   initiated by any userspace actions."

>  Or is it a matter of keeping one
> key field that contains no useful information that proves that the rest
> is bogus?  Steve said that daemons leave no useful information in auid
> as well, so I don't see how keeping this field helps us.  My
> understanding is that the subj field's "...:kernel_t:..." is the key
> here and that pid and comm give us a bit more of a clue that it is a
> kernel thread.  Is that correct?  What use does including auid serve
> here?

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.

-- 
paul moore
www.paul-moore.com