On Thu, Feb 23, 2017 at 12:35 PM, Richard Guy Briggs <r...@redhat.com> wrote: > On 2017-02-23 12:14, Paul Moore wrote: >> On Thu, Feb 23, 2017 at 12:13 PM, Richard Guy Briggs <r...@redhat.com> wrote: >> > On 2017-02-23 12:06, Paul Moore wrote: >> >> On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs <r...@redhat.com> >> >> wrote: >> >> > On 2017-02-23 11:57, Paul Moore wrote: >> >> >> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs <r...@redhat.com> >> >> >> wrote: >> >> >> > On 2017-02-23 06:20, Florian Westphal wrote: >> >> >> >> Richard Guy Briggs <r...@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 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html