[PATCH V3] audit: normalize NETFILTER_PKT
Richard Guy Briggs
rgb at redhat.com
Wed Mar 1 22:34:47 UTC 2017
On 2017-03-01 17:19, Paul Moore wrote:
> On Wed, Mar 1, 2017 at 11:28 AM, Richard Guy Briggs <rgb at 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 at 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 at 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 at redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
More information about the Linux-audit
mailing list