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           | 21 +++++++++++++++++++++
 kernel/auditsc.c                | 24 ++++++++++++++++++++++++
 net/bridge/netfilter/ebtables.c | 12 ++++--------
 net/netfilter/x_tables.c        | 12 +++---------
 4 files changed, 52 insertions(+), 17 deletions(-)

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

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

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 d281c18d1771..d7a45b181be0 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);

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)

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

Quoted 32 lines

Signed-off-by Richard Guy Briggs <[email protected]>

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

kernel/auditsc.c +13 -0

index d281c18d1771..d7a45b181be0 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);

Quoted 8 lines

kernel/auditsc.c +5 -0

+       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 · 2 lines

paul moore
www.paul-moore.com

Quoted 1 line · Paul Moore

On Wed, Apr 22, 2020 at 5:40 PM Richard Guy Briggs <[email protected]>

Quoted 13 lines

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

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

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 49 lines

Quoted 49 lines

Signed-off-by Richard Guy Briggs <[email protected]>

---

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

kernel/auditsc.c +0 -0

index d281c18d1771..d7a45b181be0 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;

kernel/auditsc.c +10 -0

+       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)",

kernel/auditsc.c +7 -0

+                        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= */
+

Quoted 4 lines

Quoted 4 lines

        audit_log_end(ab);

}
EXPORT_SYMBOL_GPL(__audit_log_nfcfg);

- RGB

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

Quoted 2 lines · EDT Richard Guy Briggs

Quoted 1 line · Paul Moore

On Wed, Apr 22, 2020 at 5:40 PM Richard Guy Briggs <[email protected]>

Quoted 24 lines · wrote:

Quoted 13 lines

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

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

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

Quoted 3 lines

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

Quoted 62 lines

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.

Quoted 49 lines

Quoted 49 lines

Signed-off-by Richard Guy Briggs <[email protected]>

---

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

kernel/auditsc.c +0 -0

index d281c18d1771..d7a45b181be0 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;

kernel/auditsc.c +10 -0

+       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)",

kernel/auditsc.c +7 -0

+                        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= */
+

Quoted 4 lines

Quoted 4 lines

        audit_log_end(ab);

}
EXPORT_SYMBOL_GPL(__audit_log_nfcfg);

- RGB

Quoted 28 lines · Steve Grubb

Quoted 2 lines · EDT Richard Guy Briggs

Quoted 1 line · Paul Moore

On Wed, Apr 22, 2020 at 5:40 PM Richard Guy Briggs <[email protected]>

Quoted 24 lines · wrote:

Quoted 13 lines

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

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

Quoted 5 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 36 lines · Steve Grubb

Quoted 2 lines · EDT Richard Guy Briggs

Quoted 1 line · Paul Moore

On Wed, Apr 22, 2020 at 5:40 PM Richard Guy Briggs <[email protected]>

Quoted 31 lines · wrote:

Quoted 18 lines

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

Quoted 8 lines

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

Quoted 6 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 45 lines · EDT Richard Guy Briggs

Quoted 36 lines · Steve Grubb

Quoted 2 lines · EDT Richard Guy Briggs

Quoted 1 line · Paul Moore

On Wed, Apr 22, 2020 at 5:40 PM Richard Guy Briggs <[email protected]>

Quoted 31 lines · wrote:

Quoted 18 lines

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

Quoted 8 lines

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

Quoted 6 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 7 lines

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

Quoted 54 lines · Steve Grubb

Quoted 45 lines · EDT Richard Guy Briggs

Quoted 36 lines · Steve Grubb

Quoted 2 lines · EDT Richard Guy Briggs

Quoted 1 line · Paul Moore

On Wed, Apr 22, 2020 at 5:40 PM Richard Guy Briggs <[email protected]>

Quoted 31 lines · wrote:

Quoted 18 lines

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

Quoted 8 lines

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

Quoted 6 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 12 lines

Quoted 11 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

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)

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

Quoted 34 lines

Signed-off-by Richard Guy Briggs <[email protected]>

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

kernel/auditsc.c +15 -0

index d281c18d1771..d7a45b181be0 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=");

kernel/auditsc.c void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries, · line 2586

Quoted 6 lines

kernel/auditsc.c +3 -0

+       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

Quoted 17 lines · Paul Moore

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)

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

Quoted 3 lines

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.

Quoted 33 lines

Quoted 33 lines

Signed-off-by Richard Guy Briggs <[email protected]>

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

kernel/auditsc.c +14 -0

index d281c18d1771..d7a45b181be0 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= */

Quoted 8 lines

kernel/auditsc.c void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries, · line 2586

Quoted 7 lines

kernel/auditsc.c +4 -0

+       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 <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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

Quoted 18 lines · Richard Guy Briggs

Quoted 17 lines · Paul Moore

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)

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

Quoted 7 lines

Quoted 3 lines

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?

That's going to be up to the security module. SELinux may know that a task with a subj= *:kernel_t:* doesn't need an uid, but that's not going to be true with Smack, or if in the (distant?) future you have both SELinux and Smack. Creating a way for the security module to inform the audit system that it believes fields are unnecessary sounds tricky. Not to mention that it's likely to create cases where the audit user-space has to know which, if any, security modules are active.

Quoted 18 lines · Paul Moore

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)

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

Quoted 3 lines

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?

Quoted 19 lines · Richard Guy Briggs

Quoted 18 lines · Paul Moore

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)

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

Quoted 6 lines

Quoted 3 lines

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

Quoted 27 lines · Paul Moore

Quoted 19 lines · Richard Guy Briggs

Quoted 18 lines · Paul Moore

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)

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

Quoted 6 lines

Quoted 3 lines

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?

Quoted 8 lines

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