Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

[Phil Sutter]

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

> 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@netfilter.org>

Acked-by: Phil Sutter <phil@nwl.cc>

Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

[Pablo Neira Ayuso]

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.

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@netfilter.org>
> 
> Acked-by: Phil Sutter <phil@nwl.cc>

Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

[Phil Sutter]

On Wed, Sep 06, 2023 at 01:47:34PM +0200, 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.

Here's a simple example:

| for table in t1 t2; do
| 	echo "add table $table"
| 	for chain in c1 c2 c3; do
| 		echo "add chain $table $chain"
| 		echo "add rule $table $chain counter accept"
| 		echo "add rule $table $chain counter accept"
| 		echo "add rule $table $chain counter accept"
| 	done
| done | nft -f -
| nft reset rules
| nft reset rules table t1

Before your patch, audit NETFILTER_CFG messages are:

| type=NETFILTER_CFG msg=audit(1694018800.149:1917): table=(null):5 family=0 entries=18 op=nft_reset_rule pid=6558 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694018848.399:1918): table=t1:5 family=2 entries=9 op=nft_reset_rule pid=6565 subj=kernel comm="nft"

(Note the NULL table name pointer in the first one.)

With your patch applied, these are the respective audit messages:

| type=NETFILTER_CFG msg=audit(1694020131.459:3): table=t1:2 family=2 entries=3 op=nft_reset_rule pid=3119 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020131.459:3): table=t1:2 family=2 entries=6 op=nft_reset_rule pid=3119 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020131.459:3): table=t1:2 family=2 entries=9 op=nft_reset_rule pid=3119 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020131.459:3): table=t2:2 family=2 entries=12 op=nft_reset_rule pid=3119 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020131.459:3): table=t2:2 family=2 entries=15 op=nft_reset_rule pid=3119 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020131.459:3): table=t2:2 family=2 entries=18 op=nft_reset_rule pid=3119 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020131.489:4): table=t1:2 family=2 entries=3 op=nft_reset_rule pid=3119 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020131.489:4): table=t1:2 family=2 entries=6 op=nft_reset_rule pid=3119 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020131.489:4): table=t1:2 family=2 entries=9 op=nft_reset_rule pid=3119 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020131.489:4): table=t2:2 family=2 entries=12 op=nft_reset_rule pid=3119 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020131.489:4): table=t2:2 family=2 entries=15 op=nft_reset_rule pid=3119 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020131.489:4): table=t2:2 family=2 entries=18 op=nft_reset_rule pid=3119 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020206.579:5): table=t1:2 family=2 entries=3 op=nft_reset_rule pid=3126 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020206.579:5): table=t1:2 family=2 entries=6 op=nft_reset_rule pid=3126 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020206.579:5): table=t1:2 family=2 entries=9 op=nft_reset_rule pid=3126 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020206.609:6): table=t1:2 family=2 entries=3 op=nft_reset_rule pid=3126 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020206.609:6): table=t1:2 family=2 entries=6 op=nft_reset_rule pid=3126 subj=kernel comm="nft"
| type=NETFILTER_CFG msg=audit(1694020206.609:6): table=t1:2 family=2 entries=9 op=nft_reset_rule pid=3126 subj=kernel comm="nft"

The last six come from the 'reset rules table t1' command. While on one
hand it looks like nftables fits only three rules into a single skb,
your fix seems to have a problem in that it doesn't subtract s_idx from
*idx.

Cheers, Phil

Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

[Pablo Neira Ayuso]

On Wed, Sep 06, 2023 at 07:08:40PM +0200, Phil Sutter wrote:
[...]
> The last six come from the 'reset rules table t1' command. While on one
> hand it looks like nftables fits only three rules into a single skb,
> your fix seems to have a problem in that it doesn't subtract s_idx from
> *idx.

Please, feel free to follow up to refine, thanks.

Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

[Paul Moore]

On Wed, Sep 6, 2023 at 2:46 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Sep 06, 2023 at 07:08:40PM +0200, Phil Sutter wrote:
> [...]
> > The last six come from the 'reset rules table t1' command. While on one
> > hand it looks like nftables fits only three rules into a single skb,
> > your fix seems to have a problem in that it doesn't subtract s_idx from
> > *idx.
>
> Please, feel free to follow up to refine, thanks.

Forgive me if I'm wrong, but it sounds as though Phil was pointing out
a bug and not an area of refinement, is that correct Phil?

If it is a bug, please submit a fix for this as soon as possible Pablo.

-- 
paul-moore.com

Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

[Phil Sutter]

Hi Paul,

On Wed, Sep 06, 2023 at 03:56:41PM -0400, Paul Moore wrote:
> On Wed, Sep 6, 2023 at 2:46 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, Sep 06, 2023 at 07:08:40PM +0200, Phil Sutter wrote:
> > [...]
> > > The last six come from the 'reset rules table t1' command. While on one
> > > hand it looks like nftables fits only three rules into a single skb,
> > > your fix seems to have a problem in that it doesn't subtract s_idx from
> > > *idx.
> >
> > Please, feel free to follow up to refine, thanks.
> 
> Forgive me if I'm wrong, but it sounds as though Phil was pointing out
> a bug and not an area of refinement, is that correct Phil?

From my point of view, yes. Though the third parameter "nentries" to
audit_log_nfcfg() is sometimes used in rather creative ways,
nf_tables_dump_obj() for instance passes the handle of the object being
reset instead of a count. So I assume whoever parses audit logs won't
rely too much upon the 'entries=NNN' part, anyway.

> If it is a bug, please submit a fix for this as soon as possible Pablo.

Thanks for your support, but I can take over, too. The number of
notifications emitted even for a small ruleset is not ideal, also. It's
just a bit sad that I ACKed the patch already and so it went out the
door. Florian, can we still put a veto there?

Cheers, Phil

Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

[Pablo Neira Ayuso]

On Wed, Sep 06, 2023 at 11:39:41PM +0200, Phil Sutter wrote:
> On Wed, Sep 06, 2023 at 03:56:41PM -0400, Paul Moore wrote:
[...]
> > If it is a bug, please submit a fix for this as soon as possible Pablo.
> 
> Thanks for your support, but I can take over, too. The number of
> notifications emitted even for a small ruleset is not ideal, also. It's
> just a bit sad that I ACKed the patch already and so it went out the
> door. Florian, can we still put a veto there?

Phil, kernel was crashing after your patch, this was resulting in a
kernel panic when running tests here. I had to revert your patches
locally to keep running tests.

Please, just send an incremental fix to adjust the idx, revert will
leave things in worse state.

Audit does not show chains either, which is not very useful to locate
what where exactly the rules have been reset, but that can probably
discussed in net-next. Richard provided a way to extend this if audit
maintainer find it useful too.

Thanks.

Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

[Paul Moore]

On Wed, Sep 6, 2023 at 6:21 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Sep 06, 2023 at 11:39:41PM +0200, Phil Sutter wrote:
> > On Wed, Sep 06, 2023 at 03:56:41PM -0400, Paul Moore wrote:
> [...]
> > > If it is a bug, please submit a fix for this as soon as possible Pablo.
> >
> > Thanks for your support, but I can take over, too. The number of
> > notifications emitted even for a small ruleset is not ideal, also. It's
> > just a bit sad that I ACKed the patch already and so it went out the
> > door. Florian, can we still put a veto there?
>
> Phil, kernel was crashing after your patch, this was resulting in a
> kernel panic when running tests here. I had to revert your patches
> locally to keep running tests.
>
> Please, just send an incremental fix to adjust the idx, revert will
> leave things in worse state.

If we can get a fix out soon then I'm fine with that, if we can't get
a fix out soon then a revert may be wise.

> Audit does not show chains either, which is not very useful to locate
> what where exactly the rules have been reset, but that can probably
> discussed in net-next. Richard provided a way to extend this if audit
> maintainer find it useful too.

Richard was correct in saying that new fields must be added to the end
of the record.  The only correction I would make to Richard's comments
is that we tend to prefer that if a field is present in a record, it
is always present in a record; if there is no useful information to
log in that field, a "?" can be substituted for the value (e.g.
"nftfield=?").

-- 
paul-moore.com

Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

[Pablo Neira Ayuso]

On Wed, Sep 06, 2023 at 06:41:13PM -0400, Paul Moore wrote:
> On Wed, Sep 6, 2023 at 6:21 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, Sep 06, 2023 at 11:39:41PM +0200, Phil Sutter wrote:
> > > On Wed, Sep 06, 2023 at 03:56:41PM -0400, Paul Moore wrote:
> > [...]
> > > > If it is a bug, please submit a fix for this as soon as possible Pablo.
> > >
> > > Thanks for your support, but I can take over, too. The number of
> > > notifications emitted even for a small ruleset is not ideal, also. It's
> > > just a bit sad that I ACKed the patch already and so it went out the
> > > door. Florian, can we still put a veto there?
> >
> > Phil, kernel was crashing after your patch, this was resulting in a
> > kernel panic when running tests here. I had to revert your patches
> > locally to keep running tests.
> >
> > Please, just send an incremental fix to adjust the idx, revert will
> > leave things in worse state.
> 
> If we can get a fix out soon then I'm fine with that, if we can't get
> a fix out soon then a revert may be wise.

I believe it should be possible to fix this in the next -rc, which
should be quick. If Phil is busy I will jump on this and I will keep
you on Cc so you and Richard can review.

I apologize for forgetting to Cc you in first place.

> > Audit does not show chains either, which is not very useful to locate
> > what where exactly the rules have been reset, but that can probably
> > discussed in net-next. Richard provided a way to extend this if audit
> > maintainer find it useful too.
> 
> Richard was correct in saying that new fields must be added to the end
> of the record.  The only correction I would make to Richard's comments
> is that we tend to prefer that if a field is present in a record, it
> is always present in a record; if there is no useful information to
> log in that field, a "?" can be substituted for the value (e.g.
> "nftfield=?").

Thanks for clarification, hopefully this will help to explore
extensions to include chain information in the logs. I think that
might help users to understand better the kind of updated that
happened in the Netfilter subsystem.

Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

[Paul Moore]

On Wed, Sep 6, 2023 at 7:01 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Sep 06, 2023 at 06:41:13PM -0400, Paul Moore wrote:
> > On Wed, Sep 6, 2023 at 6:21 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Wed, Sep 06, 2023 at 11:39:41PM +0200, Phil Sutter wrote:
> > > > On Wed, Sep 06, 2023 at 03:56:41PM -0400, Paul Moore wrote:
> > > [...]
> > > > > If it is a bug, please submit a fix for this as soon as possible Pablo.
> > > >
> > > > Thanks for your support, but I can take over, too. The number of
> > > > notifications emitted even for a small ruleset is not ideal, also. It's
> > > > just a bit sad that I ACKed the patch already and so it went out the
> > > > door. Florian, can we still put a veto there?
> > >
> > > Phil, kernel was crashing after your patch, this was resulting in a
> > > kernel panic when running tests here. I had to revert your patches
> > > locally to keep running tests.
> > >
> > > Please, just send an incremental fix to adjust the idx, revert will
> > > leave things in worse state.
> >
> > If we can get a fix out soon then I'm fine with that, if we can't get
> > a fix out soon then a revert may be wise.
>
> I believe it should be possible to fix this in the next -rc, which
> should be quick. If Phil is busy I will jump on this and I will keep
> you on Cc so you and Richard can review.

Great, thank you.

> I apologize for forgetting to Cc you in first place.

No worries :)

> > > Audit does not show chains either, which is not very useful to locate
> > > what where exactly the rules have been reset, but that can probably
> > > discussed in net-next. Richard provided a way to extend this if audit
> > > maintainer find it useful too.
> >
> > Richard was correct in saying that new fields must be added to the end
> > of the record.  The only correction I would make to Richard's comments
> > is that we tend to prefer that if a field is present in a record, it
> > is always present in a record; if there is no useful information to
> > log in that field, a "?" can be substituted for the value (e.g.
> > "nftfield=?").
>
> Thanks for clarification, hopefully this will help to explore
> extensions to include chain information in the logs. I think that
> might help users to understand better the kind of updated that
> happened in the Netfilter subsystem.

Great, I'll look forward to the patches.

-- 
paul-moore.com

Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

[Paul Moore]

On Wed, Sep 6, 2023 at 5:39 PM Phil Sutter <phil@nwl.cc> wrote:
> On Wed, Sep 06, 2023 at 03:56:41PM -0400, Paul Moore wrote:
> > On Wed, Sep 6, 2023 at 2:46 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Wed, Sep 06, 2023 at 07:08:40PM +0200, Phil Sutter wrote:
> > > [...]
> > > > The last six come from the 'reset rules table t1' command. While on one
> > > > hand it looks like nftables fits only three rules into a single skb,
> > > > your fix seems to have a problem in that it doesn't subtract s_idx from
> > > > *idx.
> > >
> > > Please, feel free to follow up to refine, thanks.
> >
> > Forgive me if I'm wrong, but it sounds as though Phil was pointing out
> > a bug and not an area of refinement, is that correct Phil?
>
> From my point of view, yes. Though the third parameter "nentries" to
> audit_log_nfcfg() is sometimes used in rather creative ways,
> nf_tables_dump_obj() for instance passes the handle of the object being
> reset instead of a count. So I assume whoever parses audit logs won't
> rely too much upon the 'entries=NNN' part, anyway.
>
> > If it is a bug, please submit a fix for this as soon as possible Pablo.
>
> Thanks for your support, but I can take over, too.

That works too.  The only thing I really care about is making sure the
code is correct and the kernel is behaving the way one would expect.
Who gets it back to that state isn't much of a concern, so long as it
does get fixed ;)

> The number of
> notifications emitted even for a small ruleset is not ideal, also.

Understood.  Let's first make sure that the audit records are correct,
then we can work on improving the frequency.

-- 
paul-moore.com

Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

[Richard Guy Briggs]

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.

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@netfilter.org>
> > 
> > Acked-by: Phil Sutter <phil@nwl.cc>

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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

Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

[Richard Guy Briggs]

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@redhat.com 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@netfilter.org>
> > > 
> > > Acked-by: Phil Sutter <phil@nwl.cc>
> 
> - RGB

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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