[PATCH V3] audit: normalize NETFILTER_PKT

[PATCH V3] audit: normalize NETFILTER_PKT

[Richard Guy Briggs]

Eliminate flipping in and out of message fields, dropping fields in the process.

Sample raw message format IPv4 UDP:
type=NETFILTER_PKT msg=audit(1487874761.386:228):  mark=0xae8a2732 saddr=127.0.0.1 daddr=127.0.0.1 proto=17^]
Sample raw message format IPv6 ICMP6:
type=NETFILTER_PKT msg=audit(1487874761.381:227):  mark=0x223894b7 saddr=::1 daddr=::1 proto=58^]

Issue: https://github.com/linux-audit/audit-kernel/issues/11
Test case: https://github.com/linux-audit/audit-testsuite/issues/43

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 net/netfilter/xt_AUDIT.c |  122 ++++++++++-----------------------------------
 1 files changed, 27 insertions(+), 95 deletions(-)

diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
index 4973cbd..945fa29 100644
--- a/net/netfilter/xt_AUDIT.c
+++ b/net/netfilter/xt_AUDIT.c
@@ -31,146 +31,78 @@ MODULE_ALIAS("ip6t_AUDIT");
 MODULE_ALIAS("ebt_AUDIT");
 MODULE_ALIAS("arpt_AUDIT");
 
-static void audit_proto(struct audit_buffer *ab, struct sk_buff *skb,
-			unsigned int proto, unsigned int offset)
-{
-	switch (proto) {
-	case IPPROTO_TCP:
-	case IPPROTO_UDP:
-	case IPPROTO_UDPLITE: {
-		const __be16 *pptr;
-		__be16 _ports[2];
-
-		pptr = skb_header_pointer(skb, offset, sizeof(_ports), _ports);
-		if (pptr == NULL) {
-			audit_log_format(ab, " truncated=1");
-			return;
-		}
-
-		audit_log_format(ab, " sport=%hu dport=%hu",
-				 ntohs(pptr[0]), ntohs(pptr[1]));
-		}
-		break;
-
-	case IPPROTO_ICMP:
-	case IPPROTO_ICMPV6: {
-		const u8 *iptr;
-		u8 _ih[2];
-
-		iptr = skb_header_pointer(skb, offset, sizeof(_ih), &_ih);
-		if (iptr == NULL) {
-			audit_log_format(ab, " truncated=1");
-			return;
-		}
-
-		audit_log_format(ab, " icmptype=%hhu icmpcode=%hhu",
-				 iptr[0], iptr[1]);
-
-		}
-		break;
-	}
-}
-
-static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
+static bool audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
 {
 	struct iphdr _iph;
 	const struct iphdr *ih;
 
 	ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
-	if (!ih) {
-		audit_log_format(ab, " truncated=1");
-		return;
-	}
+	if (!ih)
+		return false;
 
-	audit_log_format(ab, " saddr=%pI4 daddr=%pI4 ipid=%hu proto=%hhu",
-		&ih->saddr, &ih->daddr, ntohs(ih->id), ih->protocol);
+	audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu",
+			 &ih->saddr, &ih->daddr, ih->protocol);
 
-	if (ntohs(ih->frag_off) & IP_OFFSET) {
-		audit_log_format(ab, " frag=1");
-		return;
-	}
-
-	audit_proto(ab, skb, ih->protocol, ih->ihl * 4);
+	return true;
 }
 
-static void audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
+static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
 {
 	struct ipv6hdr _ip6h;
 	const struct ipv6hdr *ih;
 	u8 nexthdr;
 	__be16 frag_off;
-	int offset;
 
 	ih = skb_header_pointer(skb, skb_network_offset(skb), sizeof(_ip6h), &_ip6h);
-	if (!ih) {
-		audit_log_format(ab, " truncated=1");
-		return;
-	}
+	if (!ih)
+		return false;
 
 	nexthdr = ih->nexthdr;
-	offset = ipv6_skip_exthdr(skb, skb_network_offset(skb) + sizeof(_ip6h),
-				  &nexthdr, &frag_off);
+	ipv6_skip_exthdr(skb, skb_network_offset(skb) + sizeof(_ip6h), &nexthdr, &frag_off);
 
 	audit_log_format(ab, " saddr=%pI6c daddr=%pI6c proto=%hhu",
 			 &ih->saddr, &ih->daddr, nexthdr);
 
-	if (offset)
-		audit_proto(ab, skb, nexthdr, offset);
+	return true;
 }
 
 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;
+	int fam = -1;
 
 	if (audit_enabled == 0)
 		goto errout;
-
 	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);
-
-	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));
+	audit_log_format(ab, "mark=%#x", skb->mark ?: -1);
 
-		if (par->family == NFPROTO_BRIDGE) {
+	switch (par->family) {
+		case NFPROTO_BRIDGE: {
 			switch (eth_hdr(skb)->h_proto) {
 			case htons(ETH_P_IP):
-				audit_ip4(ab, skb);
+				fam = audit_ip4(ab, skb) ? NFPROTO_IPV4 : -1;
 				break;
-
 			case htons(ETH_P_IPV6):
-				audit_ip6(ab, skb);
+				fam = audit_ip6(ab, skb) ? NFPROTO_IPV6 : -1;
 				break;
 			}
+			break;
 		}
+		case NFPROTO_IPV4:
+			fam = audit_ip4(ab, skb) ? NFPROTO_IPV4 : -1;
+			break;
+	
+		case NFPROTO_IPV6:
+			fam = audit_ip6(ab, skb) ? NFPROTO_IPV6 : -1;
+			break;
 	}
 
-	switch (par->family) {
-	case NFPROTO_IPV4:
-		audit_ip4(ab, skb);
-		break;
-
-	case NFPROTO_IPV6:
-		audit_ip6(ab, skb);
-		break;
-	}
-
-#ifdef CONFIG_NETWORK_SECMARK
-	if (skb->secmark)
-		audit_log_secctx(ab, skb->secmark);
-#endif
+	if (fam == -1)
+		audit_log_format(ab, " saddr=? daddr=? proto=-1");
 
 	audit_log_end(ab);
 
-- 
1.7.1

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Paul Moore]

On Sun, Feb 26, 2017 at 3:49 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Eliminate flipping in and out of message fields, dropping fields in the process.
>
> Sample raw message format IPv4 UDP:
> type=NETFILTER_PKT msg=audit(1487874761.386:228):  mark=0xae8a2732 saddr=127.0.0.1 daddr=127.0.0.1 proto=17^]
> Sample raw message format IPv6 ICMP6:
> type=NETFILTER_PKT msg=audit(1487874761.381:227):  mark=0x223894b7 saddr=::1 daddr=::1 proto=58^]
>
> Issue: https://github.com/linux-audit/audit-kernel/issues/11
> Test case: https://github.com/linux-audit/audit-testsuite/issues/43
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  net/netfilter/xt_AUDIT.c |  122 ++++++++++-----------------------------------
>  1 files changed, 27 insertions(+), 95 deletions(-)
>
> diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> index 4973cbd..945fa29 100644
> --- a/net/netfilter/xt_AUDIT.c
> +++ b/net/netfilter/xt_AUDIT.c
> @@ -31,146 +31,78 @@ MODULE_ALIAS("ip6t_AUDIT");

...

> -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> +static bool audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
>  {
>         struct iphdr _iph;
>         const struct iphdr *ih;
>
>         ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);

It seems like we should be using skb_network_offset(skb) instead of 0
above, yes?  Granted, this isn't new, but let's fix it.

> -       if (!ih) {
> -               audit_log_format(ab, " truncated=1");
> -               return;
> -       }
> +       if (!ih)
> +               return false;
>
> -       audit_log_format(ab, " saddr=%pI4 daddr=%pI4 ipid=%hu proto=%hhu",
> -               &ih->saddr, &ih->daddr, ntohs(ih->id), ih->protocol);
> +       audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu",
> +                        &ih->saddr, &ih->daddr, ih->protocol);
>
> -       if (ntohs(ih->frag_off) & IP_OFFSET) {
> -               audit_log_format(ab, " frag=1");
> -               return;
> -       }
> -
> -       audit_proto(ab, skb, ih->protocol, ih->ihl * 4);
> +       return true;
>  }

...

>  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;
> +       int fam = -1;
>
>         if (audit_enabled == 0)
>                 goto errout;
> -
>         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);
> -
> -       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));
> +       audit_log_format(ab, "mark=%#x", skb->mark ?: -1);

How do Steve's userspace tools like the unset/-1 value represented
when it is a hex value: -1 or 0xffffffff?

-- 
paul moore
www.paul-moore.com

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Richard Guy Briggs]

On 2017-02-28 17:22, Paul Moore wrote:
> On Sun, Feb 26, 2017 at 3:49 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Eliminate flipping in and out of message fields, dropping fields in the process.
> >
> > Sample raw message format IPv4 UDP:
> > type=NETFILTER_PKT msg=audit(1487874761.386:228):  mark=0xae8a2732 saddr=127.0.0.1 daddr=127.0.0.1 proto=17^]
> > Sample raw message format IPv6 ICMP6:
> > type=NETFILTER_PKT msg=audit(1487874761.381:227):  mark=0x223894b7 saddr=::1 daddr=::1 proto=58^]
> >
> > Issue: https://github.com/linux-audit/audit-kernel/issues/11
> > Test case: https://github.com/linux-audit/audit-testsuite/issues/43
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  net/netfilter/xt_AUDIT.c |  122 ++++++++++-----------------------------------
> >  1 files changed, 27 insertions(+), 95 deletions(-)
> >
> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> > index 4973cbd..945fa29 100644
> > --- a/net/netfilter/xt_AUDIT.c
> > +++ b/net/netfilter/xt_AUDIT.c
> > @@ -31,146 +31,78 @@ MODULE_ALIAS("ip6t_AUDIT");
> 
> ...
> 
> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> > +static bool audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> >  {
> >         struct iphdr _iph;
> >         const struct iphdr *ih;
> >
> >         ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> 
> It seems like we should be using skb_network_offset(skb) instead of 0
> above, yes?  Granted, this isn't new, but let's fix it.

Yes, I agree.  How does this even work now?  Maybe the MAC header hasn't
been added yet (or has already been processed and stripped off) so that
skb->data is already pointing at the network header and hence has an
offset of 0.  Can you be more explicit and elaborate to say if this what
you were thinking?

This should be a seperate bug fix patch rather than fixing it in the
noise of this substantial re-arrangement.

> > -       if (!ih) {
> > -               audit_log_format(ab, " truncated=1");
> > -               return;
> > -       }
> > +       if (!ih)
> > +               return false;
> >
> > -       audit_log_format(ab, " saddr=%pI4 daddr=%pI4 ipid=%hu proto=%hhu",
> > -               &ih->saddr, &ih->daddr, ntohs(ih->id), ih->protocol);
> > +       audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu",
> > +                        &ih->saddr, &ih->daddr, ih->protocol);
> >
> > -       if (ntohs(ih->frag_off) & IP_OFFSET) {
> > -               audit_log_format(ab, " frag=1");
> > -               return;
> > -       }
> > -
> > -       audit_proto(ab, skb, ih->protocol, ih->ihl * 4);
> > +       return true;
> >  }
> 
> ...
> 
> >  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;
> > +       int fam = -1;
> >
> >         if (audit_enabled == 0)
> >                 goto errout;
> > -
> >         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);
> > -
> > -       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));
> > +       audit_log_format(ab, "mark=%#x", skb->mark ?: -1);
> 
> How do Steve's userspace tools like the unset/-1 value represented
> when it is a hex value: -1 or 0xffffffff?

My understanding is they are set up to cope with this.

> paul moore

- RGB

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

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Pablo Neira Ayuso]

On Wed, Mar 01, 2017 at 11:28:02AM -0500, Richard Guy Briggs wrote:
> On 2017-02-28 17:22, Paul Moore wrote:
> > On Sun, Feb 26, 2017 at 3:49 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Eliminate flipping in and out of message fields, dropping fields in the process.
> > >
> > > Sample raw message format IPv4 UDP:
> > > type=NETFILTER_PKT msg=audit(1487874761.386:228):  mark=0xae8a2732 saddr=127.0.0.1 daddr=127.0.0.1 proto=17^]
> > > Sample raw message format IPv6 ICMP6:
> > > type=NETFILTER_PKT msg=audit(1487874761.381:227):  mark=0x223894b7 saddr=::1 daddr=::1 proto=58^]
> > >
> > > Issue: https://github.com/linux-audit/audit-kernel/issues/11
> > > Test case: https://github.com/linux-audit/audit-testsuite/issues/43
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >  net/netfilter/xt_AUDIT.c |  122 ++++++++++-----------------------------------
> > >  1 files changed, 27 insertions(+), 95 deletions(-)
> > >
> > > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> > > index 4973cbd..945fa29 100644
> > > --- a/net/netfilter/xt_AUDIT.c
> > > +++ b/net/netfilter/xt_AUDIT.c
> > > @@ -31,146 +31,78 @@ MODULE_ALIAS("ip6t_AUDIT");
> > 
> > ...
> > 
> > > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> > > +static bool audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> > >  {
> > >         struct iphdr _iph;
> > >         const struct iphdr *ih;
> > >
> > >         ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> > 
> > It seems like we should be using skb_network_offset(skb) instead of 0
> > above, yes?  Granted, this isn't new, but let's fix it.
> 
> Yes, I agree.  How does this even work now?  Maybe the MAC header hasn't
> been added yet (or has already been processed and stripped off) so that
> skb->data is already pointing at the network header and hence has an
> offset of 0.  Can you be more explicit and elaborate to say if this what
> you were thinking?

skb_header_pointer() takes data from skb->data and packet flowing
through netfilter are guaranteed to find the network header at
skb->data.

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Paul Moore]

On Wed, Mar 1, 2017 at 11:28 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-02-28 17:22, Paul Moore wrote:
>> On Sun, Feb 26, 2017 at 3:49 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > Eliminate flipping in and out of message fields, dropping fields in the process.
>> >
>> > Sample raw message format IPv4 UDP:
>> > type=NETFILTER_PKT msg=audit(1487874761.386:228):  mark=0xae8a2732 saddr=127.0.0.1 daddr=127.0.0.1 proto=17^]
>> > Sample raw message format IPv6 ICMP6:
>> > type=NETFILTER_PKT msg=audit(1487874761.381:227):  mark=0x223894b7 saddr=::1 daddr=::1 proto=58^]
>> >
>> > Issue: https://github.com/linux-audit/audit-kernel/issues/11
>> > Test case: https://github.com/linux-audit/audit-testsuite/issues/43
>> >
>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> > ---
>> >  net/netfilter/xt_AUDIT.c |  122 ++++++++++-----------------------------------
>> >  1 files changed, 27 insertions(+), 95 deletions(-)
>> >
>> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
>> > index 4973cbd..945fa29 100644
>> > --- a/net/netfilter/xt_AUDIT.c
>> > +++ b/net/netfilter/xt_AUDIT.c
>> > @@ -31,146 +31,78 @@ MODULE_ALIAS("ip6t_AUDIT");
>>
>> ...
>>
>> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
>> > +static bool audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
>> >  {
>> >         struct iphdr _iph;
>> >         const struct iphdr *ih;
>> >
>> >         ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
>>
>> It seems like we should be using skb_network_offset(skb) instead of 0
>> above, yes?  Granted, this isn't new, but let's fix it.
>
> Yes, I agree.  How does this even work now?  Maybe the MAC header hasn't
> been added yet (or has already been processed and stripped off) so that
> skb->data is already pointing at the network header and hence has an
> offset of 0.  Can you be more explicit and elaborate to say if this what
> you were thinking?

Unfortunately, not really, I haven't thought through of all the
situations and it has been a long time since I've had to worry about
things like this.  I think we are in agreement that it needs to
change, so let's just make the change.

> This should be a seperate bug fix patch rather than fixing it in the
> noise of this substantial re-arrangement.

That's fine, but please send it as one patchset.  Also, to get ahead
of the next likely question, no I don't think the skb_network_offset()
fix is -stable worthy at the moment.

>> > -       if (!ih) {
>> > -               audit_log_format(ab, " truncated=1");
>> > -               return;
>> > -       }
>> > +       if (!ih)
>> > +               return false;
>> >
>> > -       audit_log_format(ab, " saddr=%pI4 daddr=%pI4 ipid=%hu proto=%hhu",
>> > -               &ih->saddr, &ih->daddr, ntohs(ih->id), ih->protocol);
>> > +       audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu",
>> > +                        &ih->saddr, &ih->daddr, ih->protocol);
>> >
>> > -       if (ntohs(ih->frag_off) & IP_OFFSET) {
>> > -               audit_log_format(ab, " frag=1");
>> > -               return;
>> > -       }
>> > -
>> > -       audit_proto(ab, skb, ih->protocol, ih->ihl * 4);
>> > +       return true;
>> >  }
>>
>> ...
>>
>> >  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;
>> > +       int fam = -1;
>> >
>> >         if (audit_enabled == 0)
>> >                 goto errout;
>> > -
>> >         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);
>> > -
>> > -       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));
>> > +       audit_log_format(ab, "mark=%#x", skb->mark ?: -1);
>>
>> How do Steve's userspace tools like the unset/-1 value represented
>> when it is a hex value: -1 or 0xffffffff?
>
> My understanding is they are set up to cope with this.

How does userspace distinguish between an unset nfmark and a nfmark of
0xffffffff?

--
paul moore
www.paul-moore.com

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Richard Guy Briggs]

On 2017-03-01 17:19, Paul Moore wrote:
> On Wed, Mar 1, 2017 at 11:28 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2017-02-28 17:22, Paul Moore wrote:
> >> On Sun, Feb 26, 2017 at 3:49 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> > Eliminate flipping in and out of message fields, dropping fields in the process.
> >> >
> >> > Sample raw message format IPv4 UDP:
> >> > type=NETFILTER_PKT msg=audit(1487874761.386:228):  mark=0xae8a2732 saddr=127.0.0.1 daddr=127.0.0.1 proto=17^]
> >> > Sample raw message format IPv6 ICMP6:
> >> > type=NETFILTER_PKT msg=audit(1487874761.381:227):  mark=0x223894b7 saddr=::1 daddr=::1 proto=58^]
> >> >
> >> > Issue: https://github.com/linux-audit/audit-kernel/issues/11
> >> > Test case: https://github.com/linux-audit/audit-testsuite/issues/43
> >> >
> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> > ---
> >> >  net/netfilter/xt_AUDIT.c |  122 ++++++++++-----------------------------------
> >> >  1 files changed, 27 insertions(+), 95 deletions(-)
> >> >
> >> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> >> > index 4973cbd..945fa29 100644
> >> > --- a/net/netfilter/xt_AUDIT.c
> >> > +++ b/net/netfilter/xt_AUDIT.c
> >> > @@ -31,146 +31,78 @@ MODULE_ALIAS("ip6t_AUDIT");
> >>
> >> ...
> >>
> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> >> > +static bool audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> >> >  {
> >> >         struct iphdr _iph;
> >> >         const struct iphdr *ih;
> >> >
> >> >         ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> >>
> >> It seems like we should be using skb_network_offset(skb) instead of 0
> >> above, yes?  Granted, this isn't new, but let's fix it.
> >
> > Yes, I agree.  How does this even work now?  Maybe the MAC header hasn't
> > been added yet (or has already been processed and stripped off) so that
> > skb->data is already pointing at the network header and hence has an
> > offset of 0.  Can you be more explicit and elaborate to say if this what
> > you were thinking?
> 
> Unfortunately, not really, I haven't thought through of all the
> situations and it has been a long time since I've had to worry about
> things like this.  I think we are in agreement that it needs to
> change, so let's just make the change.

Given Pablo's assurances, this could go either way, fix audit_ip4 to use
skb_network_offset() or fix audit_ip6 to use zero.  I don't have a
strong opinion, but using zero would be more efficient while using
skb_network_offset() would remove the doubt.  Either way, the
consistency will avoid raising doubt in the future as you have
(rightfully) done.

> > This should be a seperate bug fix patch rather than fixing it in the
> > noise of this substantial re-arrangement.
> 
> That's fine, but please send it as one patchset.  Also, to get ahead
> of the next likely question, no I don't think the skb_network_offset()
> fix is -stable worthy at the moment.

It didn't look like a concern given Pablo's assurance.

> >> > -       if (!ih) {
> >> > -               audit_log_format(ab, " truncated=1");
> >> > -               return;
> >> > -       }
> >> > +       if (!ih)
> >> > +               return false;
> >> >
> >> > -       audit_log_format(ab, " saddr=%pI4 daddr=%pI4 ipid=%hu proto=%hhu",
> >> > -               &ih->saddr, &ih->daddr, ntohs(ih->id), ih->protocol);
> >> > +       audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu",
> >> > +                        &ih->saddr, &ih->daddr, ih->protocol);
> >> >
> >> > -       if (ntohs(ih->frag_off) & IP_OFFSET) {
> >> > -               audit_log_format(ab, " frag=1");
> >> > -               return;
> >> > -       }
> >> > -
> >> > -       audit_proto(ab, skb, ih->protocol, ih->ihl * 4);
> >> > +       return true;
> >> >  }
> >>
> >> ...
> >>
> >> >  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;
> >> > +       int fam = -1;
> >> >
> >> >         if (audit_enabled == 0)
> >> >                 goto errout;
> >> > -
> >> >         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);
> >> > -
> >> > -       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));
> >> > +       audit_log_format(ab, "mark=%#x", skb->mark ?: -1);
> >>
> >> How do Steve's userspace tools like the unset/-1 value represented
> >> when it is a hex value: -1 or 0xffffffff?
> >
> > My understanding is they are set up to cope with this.
> 
> How does userspace distinguish between an unset nfmark and a nfmark of
> 0xffffffff?

It never had to deal specifically with nfmark previously because it
wasn't included if it was blank.  Generally other values that are -1 are
interpreted by the audit userspace tools as unset (session id, auid,
etc...)

> paul moore

- RGB

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

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Paul Moore]

On Wed, Mar 1, 2017 at 5:34 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-03-01 17:19, Paul Moore wrote:
>> On Wed, Mar 1, 2017 at 11:28 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > On 2017-02-28 17:22, Paul Moore wrote:
>> >> On Sun, Feb 26, 2017 at 3:49 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> >> > Eliminate flipping in and out of message fields, dropping fields in the process.
>> >> >
>> >> > Sample raw message format IPv4 UDP:
>> >> > type=NETFILTER_PKT msg=audit(1487874761.386:228):  mark=0xae8a2732 saddr=127.0.0.1 daddr=127.0.0.1 proto=17^]
>> >> > Sample raw message format IPv6 ICMP6:
>> >> > type=NETFILTER_PKT msg=audit(1487874761.381:227):  mark=0x223894b7 saddr=::1 daddr=::1 proto=58^]
>> >> >
>> >> > Issue: https://github.com/linux-audit/audit-kernel/issues/11
>> >> > Test case: https://github.com/linux-audit/audit-testsuite/issues/43
>> >> >
>> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> >> > ---
>> >> >  net/netfilter/xt_AUDIT.c |  122 ++++++++++-----------------------------------
>> >> >  1 files changed, 27 insertions(+), 95 deletions(-)
>> >> >
>> >> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
>> >> > index 4973cbd..945fa29 100644
>> >> > --- a/net/netfilter/xt_AUDIT.c
>> >> > +++ b/net/netfilter/xt_AUDIT.c
>> >> > @@ -31,146 +31,78 @@ MODULE_ALIAS("ip6t_AUDIT");
>> >>
>> >> ...
>> >>
>> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
>> >> > +static bool audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
>> >> >  {
>> >> >         struct iphdr _iph;
>> >> >         const struct iphdr *ih;
>> >> >
>> >> >         ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
>> >>
>> >> It seems like we should be using skb_network_offset(skb) instead of 0
>> >> above, yes?  Granted, this isn't new, but let's fix it.
>> >
>> > Yes, I agree.  How does this even work now?  Maybe the MAC header hasn't
>> > been added yet (or has already been processed and stripped off) so that
>> > skb->data is already pointing at the network header and hence has an
>> > offset of 0.  Can you be more explicit and elaborate to say if this what
>> > you were thinking?
>>
>> Unfortunately, not really, I haven't thought through of all the
>> situations and it has been a long time since I've had to worry about
>> things like this.  I think we are in agreement that it needs to
>> change, so let's just make the change.
>
> Given Pablo's assurances, this could go either way, fix audit_ip4 to use
> skb_network_offset() or fix audit_ip6 to use zero.  I don't have a
> strong opinion, but using zero would be more efficient while using
> skb_network_offset() would remove the doubt.  Either way, the
> consistency will avoid raising doubt in the future as you have
> (rightfully) done.

Just use skb_network_offset() as it is the safer option and there is
plenty of precedence.  Considering that we expect NETFILTER_PKT to see
limited use, I'm more concerned about it not breaking than some small
loss of performance.

>> >> >  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;
>> >> > +       int fam = -1;
>> >> >
>> >> >         if (audit_enabled == 0)
>> >> >                 goto errout;
>> >> > -
>> >> >         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);
>> >> > -
>> >> > -       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));
>> >> > +       audit_log_format(ab, "mark=%#x", skb->mark ?: -1);
>> >>
>> >> How do Steve's userspace tools like the unset/-1 value represented
>> >> when it is a hex value: -1 or 0xffffffff?
>> >
>> > My understanding is they are set up to cope with this.
>>
>> How does userspace distinguish between an unset nfmark and a nfmark of
>> 0xffffffff?
>
> It never had to deal specifically with nfmark previously because it
> wasn't included if it was blank.  Generally other values that are -1 are
> interpreted by the audit userspace tools as unset (session id, auid,
> etc...)

Yes, I know, let me get straight to the point: should we use "mark=-1"
when the nfmark is unset instead of "mark=0xffffffff"?

-- 
paul moore
www.paul-moore.com

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Richard Guy Briggs]

On 2017-03-02 19:16, Paul Moore wrote:
> On Wed, Mar 1, 2017 at 5:34 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2017-03-01 17:19, Paul Moore wrote:
> >> On Wed, Mar 1, 2017 at 11:28 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> > On 2017-02-28 17:22, Paul Moore wrote:
> >> >> On Sun, Feb 26, 2017 at 3:49 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> >> > Eliminate flipping in and out of message fields, dropping fields in the process.
> >> >> >
> >> >> > Sample raw message format IPv4 UDP:
> >> >> > type=NETFILTER_PKT msg=audit(1487874761.386:228):  mark=0xae8a2732 saddr=127.0.0.1 daddr=127.0.0.1 proto=17^]
> >> >> > Sample raw message format IPv6 ICMP6:
> >> >> > type=NETFILTER_PKT msg=audit(1487874761.381:227):  mark=0x223894b7 saddr=::1 daddr=::1 proto=58^]
> >> >> >
> >> >> > Issue: https://github.com/linux-audit/audit-kernel/issues/11
> >> >> > Test case: https://github.com/linux-audit/audit-testsuite/issues/43
> >> >> >
> >> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> >> > ---
> >> >> >  net/netfilter/xt_AUDIT.c |  122 ++++++++++-----------------------------------
> >> >> >  1 files changed, 27 insertions(+), 95 deletions(-)
> >> >> >
> >> >> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> >> >> > index 4973cbd..945fa29 100644
> >> >> > --- a/net/netfilter/xt_AUDIT.c
> >> >> > +++ b/net/netfilter/xt_AUDIT.c
> >> >> > @@ -31,146 +31,78 @@ MODULE_ALIAS("ip6t_AUDIT");
> >> >>
> >> >> ...
> >> >>
> >> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> >> >> > +static bool audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> >> >> >  {
> >> >> >         struct iphdr _iph;
> >> >> >         const struct iphdr *ih;
> >> >> >
> >> >> >         ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> >> >>
> >> >> It seems like we should be using skb_network_offset(skb) instead of 0
> >> >> above, yes?  Granted, this isn't new, but let's fix it.
> >> >
> >> > Yes, I agree.  How does this even work now?  Maybe the MAC header hasn't
> >> > been added yet (or has already been processed and stripped off) so that
> >> > skb->data is already pointing at the network header and hence has an
> >> > offset of 0.  Can you be more explicit and elaborate to say if this what
> >> > you were thinking?
> >>
> >> Unfortunately, not really, I haven't thought through of all the
> >> situations and it has been a long time since I've had to worry about
> >> things like this.  I think we are in agreement that it needs to
> >> change, so let's just make the change.
> >
> > Given Pablo's assurances, this could go either way, fix audit_ip4 to use
> > skb_network_offset() or fix audit_ip6 to use zero.  I don't have a
> > strong opinion, but using zero would be more efficient while using
> > skb_network_offset() would remove the doubt.  Either way, the
> > consistency will avoid raising doubt in the future as you have
> > (rightfully) done.
> 
> Just use skb_network_offset() as it is the safer option and there is
> plenty of precedence.  Considering that we expect NETFILTER_PKT to see
> limited use, I'm more concerned about it not breaking than some small
> loss of performance.

Agreed.

> >> >> >  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;
> >> >> > +       int fam = -1;
> >> >> >
> >> >> >         if (audit_enabled == 0)
> >> >> >                 goto errout;
> >> >> > -
> >> >> >         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);
> >> >> > -
> >> >> > -       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));
> >> >> > +       audit_log_format(ab, "mark=%#x", skb->mark ?: -1);
> >> >>
> >> >> How do Steve's userspace tools like the unset/-1 value represented
> >> >> when it is a hex value: -1 or 0xffffffff?
> >> >
> >> > My understanding is they are set up to cope with this.
> >>
> >> How does userspace distinguish between an unset nfmark and a nfmark of
> >> 0xffffffff?
> >
> > It never had to deal specifically with nfmark previously because it
> > wasn't included if it was blank.  Generally other values that are -1 are
> > interpreted by the audit userspace tools as unset (session id, auid,
> > etc...)
> 
> Yes, I know, let me get straight to the point: should we use "mark=-1"
> when the nfmark is unset instead of "mark=0xffffffff"?

I'd prefer to keep the format as it was, explicitly labelled hex.  The
other fields that are printed as unset, -1, come out in the logs as
MAX_UINT: "auid=4294967295 ses=4294967295", so I don't see any reason to
change that convention.  Once that field is known by userspace tools,
they can interpret (-i) that as -1.

> paul moore

- RGB

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

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Paul Moore]

On Thu, Mar 2, 2017 at 9:00 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-03-02 19:16, Paul Moore wrote:
>> On Wed, Mar 1, 2017 at 5:34 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > On 2017-03-01 17:19, Paul Moore wrote:
>> >> On Wed, Mar 1, 2017 at 11:28 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> >> > On 2017-02-28 17:22, Paul Moore wrote:
>> >> >> On Sun, Feb 26, 2017 at 3:49 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> >> >> > Eliminate flipping in and out of message fields, dropping fields in the process.
>> >> >> >
>> >> >> > Sample raw message format IPv4 UDP:
>> >> >> > type=NETFILTER_PKT msg=audit(1487874761.386:228):  mark=0xae8a2732 saddr=127.0.0.1 daddr=127.0.0.1 proto=17^]
>> >> >> > Sample raw message format IPv6 ICMP6:
>> >> >> > type=NETFILTER_PKT msg=audit(1487874761.381:227):  mark=0x223894b7 saddr=::1 daddr=::1 proto=58^]
>> >> >> >
>> >> >> > Issue: https://github.com/linux-audit/audit-kernel/issues/11
>> >> >> > Test case: https://github.com/linux-audit/audit-testsuite/issues/43
>> >> >> >
>> >> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> >> >> > ---
>> >> >> >  net/netfilter/xt_AUDIT.c |  122 ++++++++++-----------------------------------
>> >> >> >  1 files changed, 27 insertions(+), 95 deletions(-)
>> >> >> >
>> >> >> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
>> >> >> > index 4973cbd..945fa29 100644
>> >> >> > --- a/net/netfilter/xt_AUDIT.c
>> >> >> > +++ b/net/netfilter/xt_AUDIT.c
>> >> >> > @@ -31,146 +31,78 @@ MODULE_ALIAS("ip6t_AUDIT");

...

>> >> >> >  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;
>> >> >> > +       int fam = -1;
>> >> >> >
>> >> >> >         if (audit_enabled == 0)
>> >> >> >                 goto errout;
>> >> >> > -
>> >> >> >         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);
>> >> >> > -
>> >> >> > -       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));
>> >> >> > +       audit_log_format(ab, "mark=%#x", skb->mark ?: -1);
>> >> >>
>> >> >> How do Steve's userspace tools like the unset/-1 value represented
>> >> >> when it is a hex value: -1 or 0xffffffff?
>> >> >
>> >> > My understanding is they are set up to cope with this.
>> >>
>> >> How does userspace distinguish between an unset nfmark and a nfmark of
>> >> 0xffffffff?
>> >
>> > It never had to deal specifically with nfmark previously because it
>> > wasn't included if it was blank.  Generally other values that are -1 are
>> > interpreted by the audit userspace tools as unset (session id, auid,
>> > etc...)
>>
>> Yes, I know, let me get straight to the point: should we use "mark=-1"
>> when the nfmark is unset instead of "mark=0xffffffff"?
>
> I'd prefer to keep the format as it was, explicitly labelled hex.  The
> other fields that are printed as unset, -1, come out in the logs as
> MAX_UINT: "auid=4294967295 ses=4294967295", so I don't see any reason to
> change that convention.  Once that field is known by userspace tools,
> they can interpret (-i) that as -1.

Perhaps I'm missing something here, but let me ask again, how does
userspace distinguish between an unset nfmark and a nfmark of
0xffffffff?

-- 
paul moore
www.paul-moore.com

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Richard Guy Briggs]

On 2017-03-02 21:54, Paul Moore wrote:
> On Thu, Mar 2, 2017 at 9:00 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2017-03-02 19:16, Paul Moore wrote:
> >> On Wed, Mar 1, 2017 at 5:34 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> > On 2017-03-01 17:19, Paul Moore wrote:
> >> >> On Wed, Mar 1, 2017 at 11:28 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> >> > On 2017-02-28 17:22, Paul Moore wrote:
> >> >> >> On Sun, Feb 26, 2017 at 3:49 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> >> >> > Eliminate flipping in and out of message fields, dropping fields in the process.
> >> >> >> >
> >> >> >> > Sample raw message format IPv4 UDP:
> >> >> >> > type=NETFILTER_PKT msg=audit(1487874761.386:228):  mark=0xae8a2732 saddr=127.0.0.1 daddr=127.0.0.1 proto=17^]
> >> >> >> > Sample raw message format IPv6 ICMP6:
> >> >> >> > type=NETFILTER_PKT msg=audit(1487874761.381:227):  mark=0x223894b7 saddr=::1 daddr=::1 proto=58^]
> >> >> >> >
> >> >> >> > Issue: https://github.com/linux-audit/audit-kernel/issues/11
> >> >> >> > Test case: https://github.com/linux-audit/audit-testsuite/issues/43
> >> >> >> >
> >> >> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> >> >> > ---
> >> >> >> >  net/netfilter/xt_AUDIT.c |  122 ++++++++++-----------------------------------
> >> >> >> >  1 files changed, 27 insertions(+), 95 deletions(-)
> >> >> >> >
> >> >> >> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> >> >> >> > index 4973cbd..945fa29 100644
> >> >> >> > --- a/net/netfilter/xt_AUDIT.c
> >> >> >> > +++ b/net/netfilter/xt_AUDIT.c
> >> >> >> > @@ -31,146 +31,78 @@ MODULE_ALIAS("ip6t_AUDIT");
> 
> ...
> 
> >> >> >> >  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;
> >> >> >> > +       int fam = -1;
> >> >> >> >
> >> >> >> >         if (audit_enabled == 0)
> >> >> >> >                 goto errout;
> >> >> >> > -
> >> >> >> >         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);
> >> >> >> > -
> >> >> >> > -       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));
> >> >> >> > +       audit_log_format(ab, "mark=%#x", skb->mark ?: -1);
> >> >> >>
> >> >> >> How do Steve's userspace tools like the unset/-1 value represented
> >> >> >> when it is a hex value: -1 or 0xffffffff?
> >> >> >
> >> >> > My understanding is they are set up to cope with this.
> >> >>
> >> >> How does userspace distinguish between an unset nfmark and a nfmark of
> >> >> 0xffffffff?
> >> >
> >> > It never had to deal specifically with nfmark previously because it
> >> > wasn't included if it was blank.  Generally other values that are -1 are
> >> > interpreted by the audit userspace tools as unset (session id, auid,
> >> > etc...)
> >>
> >> Yes, I know, let me get straight to the point: should we use "mark=-1"
> >> when the nfmark is unset instead of "mark=0xffffffff"?
> >
> > I'd prefer to keep the format as it was, explicitly labelled hex.  The
> > other fields that are printed as unset, -1, come out in the logs as
> > MAX_UINT: "auid=4294967295 ses=4294967295", so I don't see any reason to
> > change that convention.  Once that field is known by userspace tools,
> > they can interpret (-i) that as -1.
> 
> Perhaps I'm missing something here, but let me ask again, how does
> userspace distinguish between an unset nfmark and a nfmark of
> 0xffffffff?

It can't.

> paul moore

- RGB

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

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Florian Westphal]

Richard Guy Briggs <rgb@redhat.com> wrote:
> > Perhaps I'm missing something here, but let me ask again, how does
> > userspace distinguish between an unset nfmark and a nfmark of
> > 0xffffffff?
> 
> It can't.

It can if you log it as 0, as I asked in patch 1 review.

(You wouldn't log sk uid of 0 as -1 either, would you?)

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Paul Moore]

On Fri, Mar 3, 2017 at 7:45 AM, Florian Westphal <fw@strlen.de> wrote:
> Richard Guy Briggs <rgb@redhat.com> wrote:
>> > Perhaps I'm missing something here, but let me ask again, how does
>> > userspace distinguish between an unset nfmark and a nfmark of
>> > 0xffffffff?
>>
>> It can't.
>
> It can if you log it as 0, as I asked in patch 1 review.
>
> (You wouldn't log sk uid of 0 as -1 either, would you?)

I want to see the code able to handle the full range of nfmark values
as well as the unset case; if that means we need to tweak userspace a
bit, please work with Steve on that.

-- 
paul moore
www.paul-moore.com

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Florian Westphal]

Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Mar 3, 2017 at 7:45 AM, Florian Westphal <fw@strlen.de> wrote:
> > Richard Guy Briggs <rgb@redhat.com> wrote:
> >> > Perhaps I'm missing something here, but let me ask again, how does
> >> > userspace distinguish between an unset nfmark and a nfmark of
> >> > 0xffffffff?
> >>
> >> It can't.
> >
> > It can if you log it as 0, as I asked in patch 1 review.
> >
> > (You wouldn't log sk uid of 0 as -1 either, would you?)
> 
> I want to see the code able to handle the full range of nfmark values
> as well as the unset case; if that means we need to tweak userspace a
> bit, please work with Steve on that.

There is no 'unset nfmark'.  Its just a 32bit integer.

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Paul Moore]

On Fri, Mar 3, 2017 at 8:22 AM, Florian Westphal <fw@strlen.de> wrote:
> Paul Moore <paul@paul-moore.com> wrote:
>> On Fri, Mar 3, 2017 at 7:45 AM, Florian Westphal <fw@strlen.de> wrote:
>> > Richard Guy Briggs <rgb@redhat.com> wrote:
>> >> > Perhaps I'm missing something here, but let me ask again, how does
>> >> > userspace distinguish between an unset nfmark and a nfmark of
>> >> > 0xffffffff?
>> >>
>> >> It can't.
>> >
>> > It can if you log it as 0, as I asked in patch 1 review.
>> >
>> > (You wouldn't log sk uid of 0 as -1 either, would you?)
>>
>> I want to see the code able to handle the full range of nfmark values
>> as well as the unset case; if that means we need to tweak userspace a
>> bit, please work with Steve on that.
>
> There is no 'unset nfmark'.  Its just a 32bit integer.

Yes, my apologies, this thread has dragged on so long I muddled the
details in my mind ... here is what I'm trying to get at, Richard's
latest patch (unless I've missed one in my inbox) has the following
line:

  audit_log_format(ab, "mark=%#x", skb->mark ?: -1);

... which I believe to be incorrect.  I was trying to lead Richard
along to that same realization, but it would appear I'm not having
much success, so to put it bluntly, here is what I want that line to
look like:

  audit_log_format(ab, "mark=%#x", skb->mark);

-- 
paul moore
www.paul-moore.com

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Richard Guy Briggs]

On 2017-03-03 08:56, Paul Moore wrote:
> On Fri, Mar 3, 2017 at 8:22 AM, Florian Westphal <fw@strlen.de> wrote:
> > Paul Moore <paul@paul-moore.com> wrote:
> >> On Fri, Mar 3, 2017 at 7:45 AM, Florian Westphal <fw@strlen.de> wrote:
> >> > Richard Guy Briggs <rgb@redhat.com> wrote:
> >> >> > Perhaps I'm missing something here, but let me ask again, how does
> >> >> > userspace distinguish between an unset nfmark and a nfmark of
> >> >> > 0xffffffff?
> >> >>
> >> >> It can't.
> >> >
> >> > It can if you log it as 0, as I asked in patch 1 review.
> >> >
> >> > (You wouldn't log sk uid of 0 as -1 either, would you?)
> >>
> >> I want to see the code able to handle the full range of nfmark values
> >> as well as the unset case; if that means we need to tweak userspace a
> >> bit, please work with Steve on that.
> >
> > There is no 'unset nfmark'.  Its just a 32bit integer.
> 
> Yes, my apologies, this thread has dragged on so long I muddled the
> details in my mind ... here is what I'm trying to get at, Richard's
> latest patch (unless I've missed one in my inbox) has the following
> line:
> 
>   audit_log_format(ab, "mark=%#x", skb->mark ?: -1);
> 
> ... which I believe to be incorrect.  I was trying to lead Richard
> along to that same realization, but it would appear I'm not having
> much success, so to put it bluntly, here is what I want that line to
> look like:
> 
>   audit_log_format(ab, "mark=%#x", skb->mark);

Then just say that.  Given the arguments presented, I agree.  Done.

> paul moore

- RGB

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

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Richard Guy Briggs]

On 2017-03-03 14:22, Florian Westphal wrote:
> Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Mar 3, 2017 at 7:45 AM, Florian Westphal <fw@strlen.de> wrote:
> > > Richard Guy Briggs <rgb@redhat.com> wrote:
> > >> > Perhaps I'm missing something here, but let me ask again, how does
> > >> > userspace distinguish between an unset nfmark and a nfmark of
> > >> > 0xffffffff?
> > >>
> > >> It can't.
> > >
> > > It can if you log it as 0, as I asked in patch 1 review.
> > >
> > > (You wouldn't log sk uid of 0 as -1 either, would you?)
> > 
> > I want to see the code able to handle the full range of nfmark values
> > as well as the unset case; if that means we need to tweak userspace a
> > bit, please work with Steve on that.
> 
> There is no 'unset nfmark'.  Its just a 32bit integer.

I was going to say, we'd need an out of band indicator.

- RGB

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

Re: [PATCH V3] audit: normalize NETFILTER_PKT

[Richard Guy Briggs]

On 2017-03-03 13:45, Florian Westphal wrote:
> Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Perhaps I'm missing something here, but let me ask again, how does
> > > userspace distinguish between an unset nfmark and a nfmark of
> > > 0xffffffff?
> > 
> > It can't.
> 
> It can if you log it as 0, as I asked in patch 1 review.

I'd be inclined to do that, since it will always have a value even if
its default is zero.

The proto field would actually be unset if it was a protocol family that
did not have a protocol field.

> (You wouldn't log sk uid of 0 as -1 either, would you?)

No, but you would log auid and session id as -1 if it were unset.


- RGB

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