On 2023-09-06 14:20, Richard Guy Briggs wrote:
> On 2023-09-06 13:47, Pablo Neira Ayuso wrote:
> > On Wed, Sep 06, 2023 at 01:32:50PM +0200, Phil Sutter wrote:
> > > On Wed, Sep 06, 2023 at 11:42:02AM +0200, Pablo Neira Ayuso wrote:
> > > > Deliver audit log from __nf_tables_dump_rules(), table dereference at
> > > > the end of the table list loop might point to the list head, leading to
> > > > this crash.
> > >
> > > Ah, of course. Sorry for the mess. I missed the fact that one may just
> > > call 'reset rules' and have it apply to all existing tables.
> > >
> > > [...]
> > > > Deliver audit log only once at the end of the rule dump+reset for
> > > > consistency with the set dump+reset.
> > >
> > > This may seem like number of audit logs is reduced, when it is actually
> > > increased: With your patch, there will be at least one notification for
> > > each chain, multiple with large chains (due to skb exhaustion).
> >
> > With your patch, this is called for each netlink_recvmsg() invocation,
> > which is more audit logs.
> >
> > @@ -3540,9 +3544,6 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
> > done:
> > rcu_read_unlock();
> >
> > - if (reset && idx > cb->args[0])
> > - audit_log_rule_reset(table, cb->seq, idx - cb->args[0]);
> > -
> > cb->args[0] = idx;
> > return skb->len;
> > }
> >
> > netlink dump is composed of a series of recvmsg() from userspace to
> > fetch the multiple chunks that represent the rules in this table.
> > If I read fine above, as the netlink dump makes progress, idx will
> > always go over cb->args[0], which evaluates true for each recvmsg()
> > call. With my patch this is less audit logs because audit log is only
> > delivered once for each chain, not for each recvmsg() call.
> >
> > So patch description is fine as it is :)
> >
> > > Not necessarily a problem, but worth mentioning. Also, I wonder if
> > > one should go the extra mile and add the chain name to log entries.
> > > I had considered to pass a string like "mytable:123 chain=mychain"
> > > to audit_log_nfcfg() for that matter, but it's quite a hack.
>
> (I think I missed the start of this thread?)
>
> Whatever you do, only ever add new fields to the *end* of the record to
> avoid confusing the userspace parser. I vaguely remember looking at
> adding a chain field and dismissed it.
And given that this could potentially affect userspace parsing tools,
the original audit mailing list linux-audit@xxxxxxxxxx should also be
copied. Sorry about that...
> So if this needs to be done, the string needs to be parsed to see if
> extra fields have been added and print them at the end of the record.
> The better approach is to add a parameter to carry that field and
> optionally append it to the record.
>
> > Agreed. Audit folks can extend this later on according to their needs
> > in case they need more fine grain reporting.
> >
> > > > Ensure audit reset access to table under rcu read side lock. The table
> > > > list iteration holds rcu read lock side, but recent audit code
> > > > dereferences table object out of the rcu read lock side.
> > > >
> > > > Fixes: ea078ae9108e ("netfilter: nf_tables: Audit log rule reset")
> > > > Fixes: 7e9be1124dbe ("netfilter: nf_tables: Audit log setelem reset")
> > > > Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> > >
> > > Acked-by: Phil Sutter <phil@xxxxxx>
>
> - RGB
- RGB
--
Richard Guy Briggs <rgb@xxxxxxxxxx>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
Upstream IRC: SunRaycer
Voice: +1.613.860 2354 SMS: +1.613.518.6570
