Re: [PATCH ghak124 v1] audit: log nftables configuration change events

[Richard Guy Briggs]

On 2020-05-27 16:53, Florian Westphal wrote:
> Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:
> > iptables, ip6tables, arptables and ebtables table registration,
> > replacement and unregistration configuration events are logged for the
> > native (legacy) iptables setsockopt api, but not for the
> > nftables netlink api which is used by the nft-variant of iptables in
> > addition to nftables itself.
> > 
> > Add calls to log the configuration actions in the nftables netlink api.
> > 
> > This uses the same NETFILTER_CFG record format.
> 
> I know little about audit records.  Does this allow the user to figure
> out that this record is created via nf_tables/netlink rather than xtables?

No, which is why I added that note below.  It shouldn't be hard to
change but I took the easy way to program it and now that I reflect on
it more, it sounds like it should be a basic requriement.

> > For further information please see issue
> > https://github.com/linux-audit/audit-kernel/issues/124
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@xxxxxxxxxx>
> > ---
> > This is an RFC patch.
> > Note: I have questions about the "entries" count.  Is there a more
> > appropriate or relevant item to report here?
> > Note: It might make sense to differentiate in the op= field that this
> > was a legacy call vs an nft call.  At the moment, legacy calls overlap
> > with nft table calls, which are similar calls.
> > 
> >  include/linux/audit.h         |  7 +++++++
> >  kernel/auditsc.c              | 12 +++++++++---
> >  net/netfilter/nf_tables_api.c | 14 ++++++++++++++
> >  3 files changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 3fcd9ee49734..b10f54103a82 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -12,6 +12,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/ptrace.h>
> >  #include <uapi/linux/audit.h>
> > +#include <uapi/linux/netfilter/nf_tables.h>
> >  
> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > @@ -98,6 +99,12 @@ enum audit_nfcfgop {
> >  	AUDIT_XT_OP_REGISTER,
> >  	AUDIT_XT_OP_REPLACE,
> >  	AUDIT_XT_OP_UNREGISTER,
> > +	AUDIT_XT_OP_CHAIN_REGISTER	= NFT_MSG_NEWCHAIN,
> 
> Hmm, this means AUDIT_XT_OP_CHAIN_REGISTER overlaps with the 4th
> audit_nfcfgop value...?

There was no 4th value, so the overlap is just the first three, which
are all table operations that more or less line up.

> > +	AUDIT_XT_OP_CHAIN_NOOP		= NFT_MSG_GETCHAIN,
> 
> GETCHAIN can't appear in the commit path (its not changing anything).
> Same for all other NFT_MSG_FOO that use ".call_rcu" action.

Again, I was a bit lazy in selecting the actions, and the GET actions
are of no interest since they don't change the configuration.

> Futhermore, I wonder what is to be logged by audit.

NEW and DEL of TABLEs, CHAINs and RULEs.

> The fact that there was 'some change'? If so, its enough to log
> the increment of the generation count during the commit phase.

Well, we are only logging "some change", so is it necessary to log the
generation count to show that?  Is the generation count of specific
interest?

> (After that, kernel can't back down anymore, i.e. all errors are
>  caught/handled beforehand).

I did think of recording all failed attempts too, but coding that would
be more effort.  It is worth doing if it is deemed important,
particularly for permission issues (as opposed to resource limits or
packet format errors.  This would be more of interest to a security
officer rather than a network technician, but the latter may find it
useful for debugging.

> If its 'any config change', then you also need to handle adds
> or delete from sets/maps, since that may allow something that wasn't
> allowed before, e.g. consider
> 
> ip saddr @trused accept
> 
> and then, later on,
> nft add element ip filter @trusted { 10.0.0.0/8, 192.168.0.1 }
> 
> This would not add a table, or chain, or set, but it does implicitly
> alter the ruleset.

Ah, ok, so yes, we would need that too.  I see family and table in
there, op is evident.  Is there a useful value we can use in the
"entries" field?

> > +		case NFT_MSG_DELRULE:
> > +			audit_log_nfcfg(trans->ctx.table->name,
> > +					trans->ctx.family,
> > +					atomic_read(&trans->ctx.table->chains_ht.ht.nelems),
> > +					trans->msg_type);
> > +			break;
> 
> Is that record format expected to emit the current number of chains?

I was aiming for a relevant value such as perhaps the new rule number or
the rule number being deleted.

> I'm not sure if that info is meaningful.

Can you suggest something meaningful?  This field may need to get
different information for each operation.

> Since table names can be anything in nf_tables (they have no special
> properties anymore), the table name is interesting from a informational
> pov, but not super interesting.

I don't think we need to be able to completely reconstruct the
tables/chains/rules from the information in the audit log, but be aware
of who is changing what when.

> This will also emit the same message/record multiple times, with only
> difference being the msg_type.  I'm not sure thats interesting.

I do think it is interesting.

> Consider a batch update that commits 100 new rules in chain x,
> this would result in 100 audit_log_nfcfg() calls, each with the
> same information.

So rule number would be a useful differentiator here.

> There are test cases in nftables.git, you could run them to see what
> kind of audit events are generated and how redundant they might be.

As a first pass, simply booting a test system and running the
audit-testsuite has provided some useful fodder.

Florian, thanks for your review and input.

- RGB

--
Richard Guy Briggs <rgb@xxxxxxxxxx>
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