Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 13 Feb 2020 07:14:10 -0500
From:   Richard Guy Briggs <rgb@redhat.com>
To:     Florian Westphal <fw@strlen.de>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Linux-Audit Mailing List <linux-audit@redhat.com>,
        netfilter-devel@vger.kernel.org, ebiederm@xmission.com,
        twoerner@redhat.com, eparis@parisplace.org, tgraf@infradead.org
Subject: Re: [PATCH ghak25 v2 8/9] netfilter: add audit operation field
Message-ID: <20200213121410.b2dsh2kwg3k7xg7e@madcap2.tricolour.ca>
References: <cover.1577830902.git.rgb@redhat.com>
 <6768f7c7d9804216853e6e9c59e44f8a10f46b99.1577830902.git.rgb@redhat.com>
 <20200106202306.GO795@breakpoint.cc>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200106202306.GO795@breakpoint.cc>
User-Agent: NeoMutt/20180716
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2020-01-06 21:23, Florian Westphal wrote:
> Richard Guy Briggs <rgb@redhat.com> wrote:
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 96cabb095eed..5eab4d898c26 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -379,7 +379,7 @@ 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_nf_cfg(const char *name, u8 af, int nentries);
> > +extern void __audit_nf_cfg(const char *name, u8 af, int nentries, int op);
> 
> Consider adding an enum instead of int op.
> 
> >  	if (audit_enabled)
> > -		audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries);
> > +		audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, 1);
> 
> audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, AUDIT_XT_OP_REPLACE);
> 
> ... would be a bit more readable than '1'.
> 
> The name is just an example, you can pick something else.

Thanks Florian.

Another question occurs to me about table default policy.

I'd observed previously that if nentries was zero due to an empty table,
then it was due to table registration calls, which resulted from module
loading.  The default policy is NF_ACCEPT (because Rusty didn't want
more email, go figure...).  It occurred to me later that some table
loads took a command line parameter to be able to change the default
policy verdict from NF_ACCEPT to NF_DROP.  In particular, filter FORWARD
hook tables.  Is there a straightforward way to be able to detect this
in all the audit_nf_cfg() callers to be able to log it?  In particular,
in:
	net/bridge/netfilter/ebtables.c: ebt_register_table()
	net/bridge/netfilter/ebtables.c: do_replace_finish()
	net/bridge/netfilter/ebtables.c: __ebt_unregister_table()
	net/netfilter/x_tables.c: xt_replace_table()
	net/netfilter/x_tables.c: xt_unregister_table()

It appears to be stored in the second entry of struct ipt_replace and
struct ip6t_replace, of types struct ipt_standard and struct
ip6t_standard in target.verdict, which doesn't appear to be obvious or
easily accessible from xt_replace_table().  In ebtables, it appears to
be in the policy member of struct ebt_entries.

Both potential solutions are awkward, adding a parameter to pass that
value in, but also trying to reach into the protocol-specific entry
table to find that value.  Would you have a recommendation?  This
assumes that reporting that default policy value is even desired or
required.

- RGB

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