Re: [PATCH V2] audit: normalize NETFILTER_PKT
On Thu, Feb 23, 2017 at 8:59 PM, Florian Westphalwrote: > Richard Guy Briggs wrote: >> > 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). > > I see. While this works, nfmark might already be used for other > purposes such as policy routing, so you might need an extra cookie > that can be passed to the AUDIT target instead. Yes, we discussed the idea that the nfmark field already serves many purposes, most of which are related to labeling traffic flows. I agree that using the nfmark may complicate some configurations, but using it in this manner seems to be in keeping with the ideas behind nfmark (from what I can tell). As for the configuration complexity, I think it is safe to say that any users of the NETFILTER_PKT record already have a sufficiently complex system configuration and the added complexity here may not be significant; in fact, the existing nfmark configuration may be helpful in identifying traffic categories such that no changes need to be made. -- 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT
Richard Guy Briggswrote: > > 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). I see. While this works, nfmark might already be used for other purposes such as policy routing, so you might need an extra cookie that can be passed to the AUDIT target instead. -- 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT
Paul Moorewrote: > On Thu, Feb 23, 2017 at 12:35 PM, Richard Guy Briggs wrote: > > 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. Right, I think people that want more info should just use NFLOG to dump the packet to userspace, extracting all the stuff in kernel is just a mess. -- 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On Thu, Feb 23, 2017 at 12:35 PM, Richard Guy Briggswrote: > On 2017-02-23 12:14, Paul Moore wrote: >> On Thu, Feb 23, 2017 at 12:13 PM, Richard Guy Briggs wrote: >> > On 2017-02-23 12:06, Paul Moore wrote: >> >> On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs >> >> wrote: >> >> > On 2017-02-23 11:57, Paul Moore wrote: >> >> >> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs >> >> >> wrote: >> >> >> > On 2017-02-23 06:20, Florian Westphal wrote: >> >> >> >> Richard Guy Briggs 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On 2017-02-23 12:20, Steve Grubb wrote: > On Wednesday, February 22, 2017 9:50:54 PM EST Richard Guy Briggs wrote: > > Simplify and eliminate flipping in and out of message fields, relying on > > nfmark the way we do for audit_key. > > > > https://github.com/linux-audit/audit-kernel/issues/11 > > > > Signed-off-by: Richard Guy Briggs> > If this is reworked, do you mind including a raw log event in the explanation > part of the patch? I need to see the resulting event to see how user space > needs to adapt. Yes, I'll make a note to remember to add that. > -Steve - RGB -- Richard Guy Briggs Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635 -- 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On Wednesday, February 22, 2017 9:50:54 PM EST Richard Guy Briggs wrote: > Simplify and eliminate flipping in and out of message fields, relying on > nfmark the way we do for audit_key. > > https://github.com/linux-audit/audit-kernel/issues/11 > > Signed-off-by: Richard Guy BriggsIf this is reworked, do you mind including a raw log event in the explanation part of the patch? I need to see the resulting event to see how user space needs to adapt. Thanks, -Steve -- 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On 2017-02-23 18:06, Florian Westphal wrote: > Richard Guy Briggswrote: > > On 2017-02-23 11:57, Paul Moore wrote: > > > On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs > > > wrote: > > > > On 2017-02-23 06:20, Florian Westphal wrote: > > > >> Richard Guy Briggs 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 Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635 -- 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On Thu, Feb 23, 2017 at 12:13 PM, Richard Guy Briggswrote: > On 2017-02-23 12:06, Paul Moore wrote: >> On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs wrote: >> > On 2017-02-23 11:57, Paul Moore wrote: >> >> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs >> >> wrote: >> >> > On 2017-02-23 06:20, Florian Westphal wrote: >> >> >> Richard Guy Briggs 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. -- 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On 2017-02-23 12:06, Paul Moore wrote: > On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggswrote: > > On 2017-02-23 11:57, Paul Moore wrote: > >> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs > >> wrote: > >> > On 2017-02-23 06:20, Florian Westphal wrote: > >> >> Richard Guy Briggs 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. > paul moore - RGB -- Richard Guy Briggs Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635 -- 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggswrote: > On 2017-02-23 11:57, Paul Moore wrote: >> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs wrote: >> > On 2017-02-23 06:20, Florian Westphal wrote: >> >> Richard Guy Briggs 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. -- 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT
Richard Guy Briggswrote: > On 2017-02-23 11:57, Paul Moore wrote: > > On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs > > wrote: > > > On 2017-02-23 06:20, Florian Westphal wrote: > > >> Richard Guy Briggs 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? -- 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On 2017-02-23 11:57, Paul Moore wrote: > On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggswrote: > > On 2017-02-23 06:20, Florian Westphal wrote: > >> Richard Guy Briggs 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. > paul moore - RGB -- Richard Guy Briggs Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635 -- 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggswrote: > On 2017-02-23 06:20, Florian Westphal wrote: >> Richard Guy Briggs 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. -- 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT
On 2017-02-23 06:20, Florian Westphal wrote: > Richard Guy Briggswrote: > > 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 "?" > > - audit_log_format(ab, " saddr=%pI4 daddr=%pI4 ipid=%hu proto=%hhu", > > - >saddr, >daddr, ntohs(ih->id), ih->protocol); > > Alternatively, one could keep this around. In fact, why is this (re)moved > in first place? This move of audit_log_format() seems to only reason > why *apar struct is required. > > AFAICS this now does: > ab = new() > log(ab, mark); > audit_ip4(); > log(); > > so might as well keep the log() call within the audit_ip4/6 function. Understood. The apar parameter was conceived for the previous patch with 20 fields and made more sense then. > > - audit_proto(ab, skb, ih->protocol, ih->ihl * 4); > > + apar->saddr = >saddr; > > + apar->daddr = >daddr; > > + apar->proto = ih->protocol; > > } > > Caution. skb_header_pointer() may copy from non-linear skb part > into _iph, which is on stack, so apar->saddr may be stale once > function returns. So if you really want to remove the audit_log_format() > of the saddr/daddr then you need to copy the ip addresses here. > > (We guarantee its linear for ip stack but not for NFPROTO_BRIDGE and this > function > is also called for the bridge version of the target). Ok, all the more reason to keep the log call in the protocol family function call. > > static unsigned int > > audit_tg(struct sk_buff *skb, const struct xt_action_param *par) > > { > > - const struct xt_audit_info *info = par->targinfo; > > struct audit_buffer *ab; > > + struct nfpkt_par apar = { > > + -1, NULL, NULL, -1, > > + }; > > I suggest to use > struct nfpkt_par apar = { > .family = par->family, > }; > > if apar is required for some reason. I did look at this originally, then realized that netfilter doesn't use the same protocol family identifiers as standard ethernet headers that are used in the bridge case or IP protocol or IPv6 next header. If I were to pick one, I might use the ethernet header conventions for next protocol (ethertype, except they are 16 bits instead of 8). > > ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT); > > if (ab == NULL) > > goto errout; > > > > - audit_log_format(ab, "action=%hhu hook=%u len=%u inif=%s outif=%s", > > -info->type, par->hooknum, skb->len, > > -par->in ? par->in->name : "?", > > -par->out ? par->out->name : "?"); > > - > > - if (skb->mark) > > - audit_log_format(ab, " mark=%#x", skb->mark); > > + audit_log_format(ab, " mark=%#x", skb->mark ?: -1); > > -1 will be logged as 0x, no? whats wrong with > audit_log_format(ab, " mark=%#x", skb->mark); ? You are correct, this was hasty. > > if (skb->dev && skb->dev->type == ARPHRD_ETHER) { > > - audit_log_format(ab, " smac=%pM dmac=%pM macproto=0x%04x", > > -eth_hdr(skb)->h_source, eth_hdr(skb)->h_dest, > > -ntohs(eth_hdr(skb)->h_proto)); > > - > > This ARPHDR_ETHER test is no longer needed after logging > of ether addresses is removed. Ok, that helps simplify the first level logic. I was really hoping to keep "macproto" to make clear what protocol family was in play, but that's when I noticed that par->family doesn't use a standard set of numbers I recognize. This isn't a problem because this can be indicated by a small number of bits in the nfmark. Thanks for your quick review. - RGB -- Richard Guy Briggs Kernel Security Engineering, Base Operating Systems,
Re: [PATCH V2] audit: normalize NETFILTER_PKT (fwd)
On 2017-02-23 08:12, Julia Lawall wrote: > Hello, > > It looks like the switch starting on line 106 should be indented more if > it is expected to be under the if in line 105. I believe that there > should also be braces around the switch. It is a single statement, but it > is a complex one. Yes, that switch statement should be indented, brace-to-brace. If the entire switch is indented including its own braces, the external braces around the switch should be unnecessary. Thanks. > thanks, > julia > > -- Forwarded message -- > Date: Thu, 23 Feb 2017 12:43:05 +0800 > From: kbuild test robot <fengguang...@intel.com> > To: kbu...@01.org > Cc: Julia Lawall <julia.law...@lip6.fr> > Subject: Re: [PATCH V2] audit: normalize NETFILTER_PKT > > CC: kbuild-...@01.org > In-Reply-To: > <9504740e9333a0b7074abe0dddfc487aeeae6cff.1487813996.git@redhat.com> > > Hi Richard, > > [auto build test WARNING on v4.9-rc8] > [cannot apply to nf-next/master next-20170222] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Richard-Guy-Briggs/audit-normalize-NETFILTER_PKT/20170223-110223 > :: branch date: 2 hours ago > :: commit date: 2 hours ago > > >> net/netfilter/xt_AUDIT.c:106:1-2: code aligned with following code on line > >> 116 > > git remote add linux-review https://github.com/0day-ci/linux > git remote update linux-review > git checkout 8f27486f1987d344c4d9b0de556dfd4209c524bf > vim +106 net/netfilter/xt_AUDIT.c > > 8f27486f Richard Guy Briggs 2017-02-22 100 > audit_ip6(ab, skb, ); > 43f393ca Thomas Graf2011-01-16 101 break; > 43f393ca Thomas Graf2011-01-16 102 } > 43f393ca Thomas Graf2011-01-16 103 } > 43f393ca Thomas Graf2011-01-16 104 } > 8f27486f Richard Guy Briggs 2017-02-22 105 if (apar.ipv == -1) > 43f393ca Thomas Graf2011-01-16 @106 switch (par->family) { > 43f393ca Thomas Graf2011-01-16 107 case NFPROTO_IPV4: > 8f27486f Richard Guy Briggs 2017-02-22 108 audit_ip4(ab, skb, > ); > 43f393ca Thomas Graf2011-01-16 109 break; > 43f393ca Thomas Graf2011-01-16 110 > 43f393ca Thomas Graf2011-01-16 111 case NFPROTO_IPV6: > 8f27486f Richard Guy Briggs 2017-02-22 112 audit_ip6(ab, skb, > ); > 43f393ca Thomas Graf2011-01-16 113 break; > 43f393ca Thomas Graf2011-01-16 114 } > 43f393ca Thomas Graf2011-01-16 115 > 8f27486f Richard Guy Briggs 2017-02-22 @116 switch (apar.ipv) { > 8f27486f Richard Guy Briggs 2017-02-22 117 case 4: > 8f27486f Richard Guy Briggs 2017-02-22 118 audit_log_format(ab, " > saddr=%pI4 daddr=%pI4 proto=%hhu", > 8f27486f Richard Guy Briggs 2017-02-22 119apar.saddr, > apar.daddr, apar.proto); > > :: The code at line 106 was first introduced by commit > :: 43f393caec0362abe03c72799d3f342af3973070 netfilter: audit target to > record accepted/dropped packets > > :: TO: Thomas Graf <tg...@infradead.org> > :: CC: Patrick McHardy <ka...@trash.net> > > --- > 0-DAY kernel test infrastructureOpen Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation - RGB -- Richard Guy Briggs <r...@redhat.com> Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635 -- 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT (fwd)
Hello, It looks like the switch starting on line 106 should be indented more if it is expected to be under the if in line 105. I believe that there should also be braces around the switch. It is a single statement, but it is a complex one. thanks, julia -- Forwarded message -- Date: Thu, 23 Feb 2017 12:43:05 +0800 From: kbuild test robot <fengguang...@intel.com> To: kbu...@01.org Cc: Julia Lawall <julia.law...@lip6.fr> Subject: Re: [PATCH V2] audit: normalize NETFILTER_PKT CC: kbuild-...@01.org In-Reply-To: <9504740e9333a0b7074abe0dddfc487aeeae6cff.1487813996.git@redhat.com> Hi Richard, [auto build test WARNING on v4.9-rc8] [cannot apply to nf-next/master next-20170222] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Richard-Guy-Briggs/audit-normalize-NETFILTER_PKT/20170223-110223 :: branch date: 2 hours ago :: commit date: 2 hours ago >> net/netfilter/xt_AUDIT.c:106:1-2: code aligned with following code on line >> 116 git remote add linux-review https://github.com/0day-ci/linux git remote update linux-review git checkout 8f27486f1987d344c4d9b0de556dfd4209c524bf vim +106 net/netfilter/xt_AUDIT.c 8f27486f Richard Guy Briggs 2017-02-22 100 audit_ip6(ab, skb, ); 43f393ca Thomas Graf2011-01-16 101 break; 43f393ca Thomas Graf2011-01-16 102 } 43f393ca Thomas Graf2011-01-16 103 } 43f393ca Thomas Graf2011-01-16 104 } 8f27486f Richard Guy Briggs 2017-02-22 105 if (apar.ipv == -1) 43f393ca Thomas Graf2011-01-16 @106 switch (par->family) { 43f393ca Thomas Graf2011-01-16 107 case NFPROTO_IPV4: 8f27486f Richard Guy Briggs 2017-02-22 108 audit_ip4(ab, skb, ); 43f393ca Thomas Graf2011-01-16 109 break; 43f393ca Thomas Graf2011-01-16 110 43f393ca Thomas Graf2011-01-16 111 case NFPROTO_IPV6: 8f27486f Richard Guy Briggs 2017-02-22 112 audit_ip6(ab, skb, ); 43f393ca Thomas Graf2011-01-16 113 break; 43f393ca Thomas Graf2011-01-16 114 } 43f393ca Thomas Graf2011-01-16 115 8f27486f Richard Guy Briggs 2017-02-22 @116 switch (apar.ipv) { 8f27486f Richard Guy Briggs 2017-02-22 117 case 4: 8f27486f Richard Guy Briggs 2017-02-22 118 audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu", 8f27486f Richard Guy Briggs 2017-02-22 119 apar.saddr, apar.daddr, apar.proto); :: The code at line 106 was first introduced by commit :: 43f393caec0362abe03c72799d3f342af3973070 netfilter: audit target to record accepted/dropped packets :: TO: Thomas Graf <tg...@infradead.org> :: CC: Patrick McHardy <ka...@trash.net> --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation -- 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
Re: [PATCH V2] audit: normalize NETFILTER_PKT
Richard Guy Briggswrote: > 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. > - audit_log_format(ab, " saddr=%pI4 daddr=%pI4 ipid=%hu proto=%hhu", > - >saddr, >daddr, ntohs(ih->id), ih->protocol); Alternatively, one could keep this around. In fact, why is this (re)moved in first place? This move of audit_log_format() seems to only reason why *apar struct is required. AFAICS this now does: ab = new() log(ab, mark); audit_ip4(); log(); so might as well keep the log() call within the audit_ip4/6 function. > - audit_proto(ab, skb, ih->protocol, ih->ihl * 4); > + apar->saddr = >saddr; > + apar->daddr = >daddr; > + apar->proto = ih->protocol; > } Caution. skb_header_pointer() may copy from non-linear skb part into _iph, which is on stack, so apar->saddr may be stale once function returns. So if you really want to remove the audit_log_format() of the saddr/daddr then you need to copy the ip addresses here. (We guarantee its linear for ip stack but not for NFPROTO_BRIDGE and this function is also called for the bridge version of the target). > static unsigned int > audit_tg(struct sk_buff *skb, const struct xt_action_param *par) > { > - const struct xt_audit_info *info = par->targinfo; > struct audit_buffer *ab; > + struct nfpkt_par apar = { > + -1, NULL, NULL, -1, > + }; I suggest to use struct nfpkt_par apar = { .family = par->family, }; if apar is required for some reason. > ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT); > if (ab == NULL) > goto errout; > > - audit_log_format(ab, "action=%hhu hook=%u len=%u inif=%s outif=%s", > - info->type, par->hooknum, skb->len, > - par->in ? par->in->name : "?", > - par->out ? par->out->name : "?"); > - > - if (skb->mark) > - audit_log_format(ab, " mark=%#x", skb->mark); > + audit_log_format(ab, " mark=%#x", skb->mark ?: -1); -1 will be logged as 0x, no? whats wrong with audit_log_format(ab, " mark=%#x", skb->mark); ? > if (skb->dev && skb->dev->type == ARPHRD_ETHER) { > - audit_log_format(ab, " smac=%pM dmac=%pM macproto=0x%04x", > - eth_hdr(skb)->h_source, eth_hdr(skb)->h_dest, > - ntohs(eth_hdr(skb)->h_proto)); > - This ARPHDR_ETHER test is no longer needed after logging of ether addresses is removed. -- 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