[PATCH V2] audit: normalize NETFILTER_PKT

[Richard Guy Briggs]

On 2017-02-23 18:06, Florian Westphal wrote:
> Richard Guy Briggs <rgb at redhat.com> wrote:
> > On 2017-02-23 11:57, Paul Moore wrote:
> > > On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs <rgb at redhat.com> wrote:
> > > > On 2017-02-23 06:20, Florian Westphal wrote:
> > > >> Richard Guy Briggs <rgb at redhat.com> wrote:
> > > >> > Simplify and eliminate flipping in and out of message fields, relying on nfmark
> > > >> > the way we do for audit_key.
> > > >> >
> > > >> > +struct nfpkt_par {
> > > >> > +   int ipv;
> > > >> > +   const void *saddr;
> > > >> > +   const void *daddr;
> > > >> > +   u8 proto;
> > > >> > +};
> > > >>
> > > >> This is problematic, see below for why.
> > > >>
> > > >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> > > >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb, struct nfpkt_par *apar)
> > > >> >  {
> > > >> >     struct iphdr _iph;
> > > >> >     const struct iphdr *ih;
> > > >> >
> > > >> > +   apar->ipv = 4;
> > > >> >     ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> > > >> > -   if (!ih) {
> > > >> > -           audit_log_format(ab, " truncated=1");
> > > >> > +   if (!ih)
> > > >> >             return;
> > > >>
> > > >> Removing this "truncated" has the consequence that this can later log
> > > >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
> > > >>
> > > >> This cannot happen for ip(6)tables because ip stack discards broken l3 headers
> > > >> before the netfilter hooks get called, but its possible with NFPROTO_BRIDGE.
> > > >>
> > > >> Perhaps you will need to change audit_ip4/6 to return "false" when it can't
> > > >> get the l3 information now so we only log zero addresses when the packet
> > > >> really did contain them.
> > > >
> > > > Ok, to clarify the implications, are you saying that handing a NULL
> > > > pointer to "saddr=%pI4" will print "0.0.0.0" rather than "(none)" or "?"
> 
> No, if you pass pointers that would indeed log NULL.
> 
> > > My initial reaction is that if the packet is so badly
> > > truncated/malformed that we don't have a full IP header than we should
> > > just refrain from logging the packet; it's too malformed/garbage to
> > > offer any useful information and the normal packet processing should
> > > result in the packet being discarded anyway.
> 
> True for ip/ipv6, not sure about bridge though.
> 
> > Which is why I wanted the ethertype, but that can be coded into the nfmark.
> 
> Not following, sorry, are you saying users can/should use -j MARK
> somehow?

Part of the discussed design and rationale for stripping many of the
vanishing fields is that when setting up netfilter rules to invoke the
AUDIT target, an accompanying nf mark should be used to indicate which
rule caught that packet, since the chain name and rule number aren't
available to the audit target.  We would use the nf mark similarly to
the way we use a rule key in the audit rules (see man auditctl).

- RGB

--
Richard Guy Briggs <rgb at redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635