Address NETFILTER_CFG issues

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 <[email protected]>

---
 include/linux/audit.h           | 19 +++++++++++++++++++
 kernel/auditsc.c                | 24 ++++++++++++++++++++++++
 net/bridge/netfilter/ebtables.c | 12 ++++--------
 net/netfilter/x_tables.c        | 12 +++---------
 4 files changed, 50 insertions(+), 17 deletions(-)

include/linux/audit.h +7 -0

index f9ceae57ca8d..f4aed2b9be8d 100644
@@ -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)
 {

Quoted 60 lines · 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 <[email protected]>

---
 include/linux/audit.h           | 19 +++++++++++++++++++
 kernel/auditsc.c                | 24 ++++++++++++++++++++++++
 net/bridge/netfilter/ebtables.c | 12 ++++--------
 net/netfilter/x_tables.c        | 12 +++---------
 4 files changed, 50 insertions(+), 17 deletions(-)

include/linux/audit.h +7 -0

index f9ceae57ca8d..f4aed2b9be8d 100644
@@ -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,13 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
                __audit_ntp_log(ad);
 }
 

include/linux/audit.h static inline void audit_ntp_log(const struct audit_ntp_data *ad) · line 528

Quoted 5 lines

include/linux/audit.h +5 -0

+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);

Do we want a dummy check here too? Or do we always want to generate this record (assuming audit is enabled) because it is a configuration related record?

paul moore

Quoted 61 lines · Paul Moore

Quoted 60 lines · 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 <[email protected]>

---
 include/linux/audit.h           | 19 +++++++++++++++++++
 kernel/auditsc.c                | 24 ++++++++++++++++++++++++
 net/bridge/netfilter/ebtables.c | 12 ++++--------
 net/netfilter/x_tables.c        | 12 +++---------
 4 files changed, 50 insertions(+), 17 deletions(-)

include/linux/audit.h +7 -0

index f9ceae57ca8d..f4aed2b9be8d 100644
@@ -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,13 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
                __audit_ntp_log(ad);
 }
 

Quoted 9 lines

include/linux/audit.h static inline void audit_ntp_log(const struct audit_ntp_data *ad) · line 528

Quoted 5 lines

include/linux/audit.h +5 -0

+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);

Do we want a dummy check here too? Or do we always want to generate this record (assuming audit is enabled) because it is a configuration related record?

This is an audit configuration change, so it is mandatory unless there is a rule that excludes it. I talked about this in the cover letter, but perhaps my wording wasn't as clear as it could have been.

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 <[email protected]>

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

kernel/auditsc.c +18 -0

index dbb056feccb9..6c233076dfb7 100644
@@ -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);

Signature · 6 lines

1.8.3.1

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

Quoted 5 lines · 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:

Quoted 1 line

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)

Given the precedent set by bpf unload, I'd really rather drop this patch that adds subject credentials.

Similarly with ghak25's subject credentials, but they were already present and that would change an existing record format, so it isn't quite as justifiable in that case.

Quoted 11 lines · Richard Guy Briggs

Quoted 6 lines · 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)

Given the precedent set by bpf unload, I'd really rather drop this patch that adds subject credentials.

Quoted 3 lines

Similarly with ghak25's subject credentials, but they were already present and that would change an existing record format, so it isn't quite as justifiable in that case.

Your comments have me confused - do you want this patch (v3 3/3) considered for merging or no?

paul moore

Quoted 12 lines · Paul Moore

Quoted 11 lines · Richard Guy Briggs

Quoted 6 lines · 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)

Given the precedent set by bpf unload, I'd really rather drop this patch that adds subject credentials.

Quoted 6 lines

Quoted 3 lines

Similarly with ghak25's subject credentials, but they were already present and that would change an existing record format, so it isn't quite as justifiable in that case.

Your comments have me confused - do you want this patch (v3 3/3) considered for merging or no?

I would like it considered for merging if you think it will be required to provide enough information about the event that happenned. In the bpf unload case, there is a program number to provide a link to a previous load action. In this case, we won't know for sure what caused the table to be unloaded if the number of entries was empty. I'm still trying to decide if it matters. For the sake of caution I think it should be included. I don't like it, but I think it needs to be included.

Quoted 2 lines · Richard Guy Briggs

Quoted 1 line · Paul Moore

On Wed, Mar 18, 2020 at 9:12 AM Richard Guy Briggs <[email protected]>

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines · Richard Guy Briggs

Quoted 9 lines

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)

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Given the precedent set by bpf unload, I'd really rather drop this patch that adds subject credentials.

I'm in the middle of building patches 1/3 and 2/3, assuming all goes well I'll merge them into audit/next (expect mail soon), however I'm going back and forth on this patch. Like you I kinda don't like it, and with both of us not in love with this patch I have to ask if there is certification requirement for this?

I know about the generic
subj/obj requirements, but in the case where there is no associated task/syscall/etc. information it isn't like the extra fields supplied in this patch are going to have much information in that regard; it's really the *absence* of that information which is telling.

Which brings me to wonder if simply the lack of any associated records in this event is enough? Before when we weren't associating records into a single event it would have been a problem, but the way things currently are, if there are no other records (and you have configured that) then I think you have everything you need to know.

Thoughts?

Quoted 3 lines · EDT Paul Moore

Quoted 2 lines · Richard Guy Briggs

Quoted 1 line · Paul Moore

On Wed, Mar 18, 2020 at 9:12 AM Richard Guy Briggs <[email protected]>

Quoted 10 lines · wrote:

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines · Richard Guy Briggs

Quoted 9 lines

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)

If this is the kernel, why is pid not 0? And if pid is 0, then isn't exe=/boot/vmlinuz-X.Y.Z-blah?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Given the precedent set by bpf unload, I'd really rather drop this patch that adds subject credentials.

<snip>

Quoted 5 lines

I'm in the middle of building patches 1/3 and 2/3, assuming all goes well I'll merge them into audit/next (expect mail soon), however I'm going back and forth on this patch. Like you I kinda don't like it, and with both of us not in love with this patch I have to ask if there is certification requirement for this?

Yes, any change to information flow must be auditable.

Quoted 5 lines

I know about the generic
subj/obj requirements, but in the case where there is no associated task/syscall/etc. information it isn't like the extra fields supplied in this patch are going to have much information in that regard; it's really the *absence* of that information which is telling.

Exactly. But if someone does a search based on the fields, they need to be able to find this record. For example, suppose I want to know what actions have been performed by kernel_t, I can run a search and find this event.

Quoted 7 lines

Which brings me to wonder if simply the lack of any associated records in this event is enough? Before when we weren't associating records into a single event it would have been a problem, but the way things currently are, if there are no other records (and you have configured that) then I think you have everything you need to know.

Thoughts?

You can't search on the absense of information. There are some fields that have meaning. It's OK if they are unset. It happens for daemons, too. But we don't remove the fields because of it. It tells part of the story.

-Steve

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

Quoted 20 lines · Richard Guy Briggs

Quoted 12 lines · Paul Moore

Quoted 11 lines · Richard Guy Briggs

Quoted 6 lines · 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)

Given the precedent set by bpf unload, I'd really rather drop this patch that adds subject credentials.

Quoted 6 lines

Quoted 3 lines

Similarly with ghak25's subject credentials, but they were already present and that would change an existing record format, so it isn't quite as justifiable in that case.

Your comments have me confused - do you want this patch (v3 3/3) considered for merging or no?

Quoted 8 lines

I would like it considered for merging if you think it will be required to provide enough information about the event that happenned. In the bpf unload case, there is a program number to provide a link to a previous load action. In this case, we won't know for sure what caused the table to be unloaded if the number of entries was empty. I'm still trying to decide if it matters. For the sake of caution I think it should be included. I don't like it, but I think it needs to be included.

I'm in the middle of building patches 1/3 and 2/3, assuming all goes well I'll merge them into audit/next (expect mail soon), however I'm going back and forth on this patch. Like you I kinda don't like it, and with both of us not in love with this patch I have to ask if there is certification requirement for this? I know about the generic subj/obj requirements, but in the case where there is no associated task/syscall/etc. information it isn't like the extra fields supplied in this patch are going to have much information in that regard; it's really the *absence* of that information which is telling. Which brings me to wonder if simply the lack of any associated records in this event is enough? Before when we weren't associating records into a single event it would have been a problem, but the way things currently are, if there are no other records (and you have configured that) then I think you have everything you need to know.

Thoughts?

paul moore