[PATCH V2] audit: normalize NETFILTER_PKT

[Paul Moore]

On Thu, Feb 23, 2017 at 12:35 PM, Richard Guy Briggs <rgb at redhat.com> wrote:
> On 2017-02-23 12:14, Paul Moore wrote:
>> On Thu, Feb 23, 2017 at 12:13 PM, Richard Guy Briggs <rgb at redhat.com> wrote:
>> > On 2017-02-23 12:06, Paul Moore wrote:
>> >> On Thu, Feb 23, 2017 at 12:04 PM, 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 "?"
>> >> >>
>> >> >> 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.
>> >> >
>> >> > Which is why I wanted the ethertype, but that can be coded into the nfmark.
>> >>
>> >> If the packet is garbage (garbage without any payload in this case),
>> >> what does it matter?  It's noise.
>> >
>> > It could be an indicator that either the logging rules or the filter
>> > rules need honing, or even that there is a bug in the network code.
>>
>> Elaborate on this please, I still don't see how logging the ethertype
>> is helpful for a malformed packet.
>
> Well, since we can encode it in the nfmark, it could be helpful, but not necessary.
>
> Each bit of information we can include in the audit log message removes
> something we need to code in the nf mark.  That's why things like ifin,
> ifout, action, hook are easy to include and help reduce the amount of
> nf mark coding needed when devising netfilter rules.

... but if the packet is so badly manged it doesn't even have a
complete IP header, what's the point in logging the packet at all?
That's the point I'm trying to make.

> I had another idea on how to include the sport and dport and that was to
> use the same identifier for sport/icmptype and also for dport/icmpcode,
> but you've already said you are not interested.

Not at this point in time since we don't have any good requirements at
the moment.  I would like us to keep this small until we have a better
idea of how people want to use this, this way we don't end up stuck
maintaining something that is ill suited for what people actually
want/use.

-- 
paul moore
www.paul-moore.com