Re: [PATCH V2] audit: normalize NETFILTER_PKT

[Paul Moore]

On Thu, Feb 23, 2017 at 8:59 PM, Florian Westphal  wrote:
> 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

[Florian Westphal]

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.
--
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

[Florian Westphal]

Paul Moore  wrote:
> 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

[Paul Moore]

On Thu, Feb 23, 2017 at 12:35 PM, Richard Guy Briggs  wrote:
> 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

[Richard Guy Briggs]

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

[Steve Grubb]

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.

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

[Richard Guy Briggs]

On 2017-02-23 18:06, Florian Westphal wrote:
> 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 "?"
> 
> 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

[Paul Moore]

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.

-- 
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 Briggs]

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.

> 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

[Paul Moore]

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.

-- 
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

[Florian Westphal]

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 "?"

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

[Richard Guy Briggs]

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.

> 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

[Paul Moore]

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.

-- 
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 Briggs]

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 "?"

> > -   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)

[Richard Guy Briggs]

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)

[Julia Lawall]

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

[Florian Westphal]

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.

> - 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