8:50 p.m.
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 <rgb(a)redhat.com>
---
net/netfilter/xt_AUDIT.c | 128 +++++++++++++++-------------------------------
1 files changed, 41 insertions(+), 87 deletions(-)
diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
index 4973cbd..05f7f25 100644
--- a/net/netfilter/xt_AUDIT.c
+++ b/net/netfilter/xt_AUDIT.c
@@ -31,146 +31,100 @@ 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;
- }
-}
+struct nfpkt_par {
+ int ipv;
+ const void *saddr;
+ const void *daddr;
+ u8 proto;
+};
-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;
- }
-
- audit_log_format(ab, " saddr=%pI4 daddr=%pI4 ipid=%hu proto=%hhu",
- &ih->saddr, &ih->daddr, ntohs(ih->id), 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);
+ apar->saddr = &ih->saddr;
+ apar->daddr = &ih->daddr;
+ apar->proto = ih->protocol;
}
-static void audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
+static void audit_ip6(struct audit_buffer *ab, struct sk_buff *skb, struct nfpkt_par
*apar)
{
struct ipv6hdr _ip6h;
const struct ipv6hdr *ih;
u8 nexthdr;
__be16 frag_off;
- int offset;
+ apar->ipv = 6;
ih = skb_header_pointer(skb, skb_network_offset(skb), sizeof(_ip6h), &_ip6h);
- if (!ih) {
- audit_log_format(ab, " truncated=1");
+ if (!ih)
return;
- }
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);
+ apar->saddr = &ih->saddr;
+ apar->daddr = &ih->daddr;
+ apar->proto = nexthdr;
}
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,
+ };
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);
+ audit_log_format(ab, " mark=%#x", skb->mark ?: -1);
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));
-
if (par->family == NFPROTO_BRIDGE) {
switch (eth_hdr(skb)->h_proto) {
case htons(ETH_P_IP):
- audit_ip4(ab, skb);
+ audit_ip4(ab, skb, &apar);
break;
case htons(ETH_P_IPV6):
- audit_ip6(ab, skb);
+ audit_ip6(ab, skb, &apar);
break;
}
}
}
-
+ if (apar.ipv == -1)
switch (par->family) {
case NFPROTO_IPV4:
- audit_ip4(ab, skb);
+ audit_ip4(ab, skb, &apar);
break;
case NFPROTO_IPV6:
- audit_ip6(ab, skb);
+ audit_ip6(ab, skb, &apar);
break;
}
-#ifdef CONFIG_NETWORK_SECMARK
- if (skb->secmark)
- audit_log_secctx(ab, skb->secmark);
-#endif
+ switch (apar.ipv) {
+ case 4:
+ audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu",
+ apar.saddr, apar.daddr, apar.proto);
+ break;
+ case 6:
+ audit_log_format(ab, " saddr=%pI6c daddr=%pI6c proto=%hhu",
+ apar.saddr, apar.daddr, apar.proto);
+ break;
+ default:
+ audit_log_format(ab, " saddr=? daddr=? proto=-1");
+ }
audit_log_end(ab);
--
1.7.1
11:20 p.m.
Richard Guy Briggs <rgb(a)redhat.com> wrote:
Simplify and eliminate flipping in and out of message fields, relying
on nfmark
the way we do for audit_key.
+struct nfpkt_par {
+ int ipv;
+ const void *saddr;
+ const void *daddr;
+ u8 proto;
+};
This is problematic, see below for why.
-static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
+static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb, struct nfpkt_par
*apar)
{
struct iphdr _iph;
const struct iphdr *ih;
+ apar->ipv = 4;
ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
- if (!ih) {
- audit_log_format(ab, " truncated=1");
+ if (!ih)
return;
Removing this "truncated" has the consequence that this can later log
"saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
This cannot happen for ip(6)tables because ip stack discards broken l3 headers
before the netfilter hooks get called, but its possible with NFPROTO_BRIDGE.
Perhaps you will need to change audit_ip4/6 to return "false" when it can't
get the l3 information now so we only log zero addresses when the packet
really did contain them.
- audit_log_format(ab, " saddr=%pI4 daddr=%pI4 ipid=%hu
proto=%hhu",
- &ih->saddr, &ih->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(&apar);
log(&apar);
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 = &ih->saddr;
+ apar->daddr = &ih->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 0xffffffff, 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.
9:51 a.m.
On 2017-02-23 06:20, Florian Westphal wrote:
Richard Guy Briggs <rgb(a)redhat.com> wrote:
> Simplify and eliminate flipping in and out of message fields, relying on nfmark
> the way we do for audit_key.
>
> +struct nfpkt_par {
> + int ipv;
> + const void *saddr;
> + const void *daddr;
> + u8 proto;
> +};
This is problematic, see below for why.
> -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb, struct
nfpkt_par *apar)
> {
> struct iphdr _iph;
> const struct iphdr *ih;
>
> + apar->ipv = 4;
> ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> - if (!ih) {
> - audit_log_format(ab, " truncated=1");
> + if (!ih)
> return;
Removing this "truncated" has the consequence that this can later log
"saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
This cannot happen for ip(6)tables because ip stack discards broken l3 headers
before the netfilter hooks get called, but its possible with NFPROTO_BRIDGE.
Perhaps you will need to change audit_ip4/6 to return "false" when it
can't
get the l3 information now so we only log zero addresses when the packet
really did contain them.
Ok, to clarify the implications, are you saying that handing a NULL
pointer to "saddr=%pI4" will print "0.0.0.0" rather than
"(none)" or "?"
> - audit_log_format(ab, " saddr=%pI4 daddr=%pI4 ipid=%hu
proto=%hhu",
> - &ih->saddr, &ih->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(&apar);
log(&apar);
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 = &ih->saddr;
> + apar->daddr = &ih->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 0xffffffff, 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 <rgb(a)redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
10:57 a.m.
On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2017-02-23 06:20, Florian Westphal wrote:
> Richard Guy Briggs <rgb(a)redhat.com> wrote:
> > Simplify and eliminate flipping in and out of message fields, relying on nfmark
> > the way we do for audit_key.
> >
> > +struct nfpkt_par {
> > + int ipv;
> > + const void *saddr;
> > + const void *daddr;
> > + u8 proto;
> > +};
>
> This is problematic, see below for why.
>
> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb, struct
nfpkt_par *apar)
> > {
> > struct iphdr _iph;
> > const struct iphdr *ih;
> >
> > + apar->ipv = 4;
> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> > - if (!ih) {
> > - audit_log_format(ab, " truncated=1");
> > + if (!ih)
> > return;
>
> Removing this "truncated" has the consequence that this can later log
> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
>
> This cannot happen for ip(6)tables because ip stack discards broken l3 headers
> before the netfilter hooks get called, but its possible with NFPROTO_BRIDGE.
>
> Perhaps you will need to change audit_ip4/6 to return "false" when it
can't
> get the l3 information now so we only log zero addresses when the packet
> really did contain them.
Ok, to clarify the implications, are you saying that handing a NULL
pointer to "saddr=%pI4" will print "0.0.0.0" rather than
"(none)" or "?"
My initial reaction is that if the packet is so badly
truncated/malformed that we don't have a full IP header than we should
just refrain from logging the packet; it's too malformed/garbage to
offer any useful information and the normal packet processing should
result in the packet being discarded anyway.
--
paul moore
www.paul-moore.com
11:04 a.m.
On 2017-02-23 11:57, Paul Moore wrote:
On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> On 2017-02-23 06:20, Florian Westphal wrote:
>> Richard Guy Briggs <rgb(a)redhat.com> wrote:
>> > Simplify and eliminate flipping in and out of message fields, relying on
nfmark
>> > the way we do for audit_key.
>> >
>> > +struct nfpkt_par {
>> > + int ipv;
>> > + const void *saddr;
>> > + const void *daddr;
>> > + u8 proto;
>> > +};
>>
>> This is problematic, see below for why.
>>
>> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
>> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb, struct
nfpkt_par *apar)
>> > {
>> > struct iphdr _iph;
>> > const struct iphdr *ih;
>> >
>> > + apar->ipv = 4;
>> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
>> > - if (!ih) {
>> > - audit_log_format(ab, " truncated=1");
>> > + if (!ih)
>> > return;
>>
>> Removing this "truncated" has the consequence that this can later log
>> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
>>
>> This cannot happen for ip(6)tables because ip stack discards broken l3 headers
>> before the netfilter hooks get called, but its possible with NFPROTO_BRIDGE.
>>
>> Perhaps you will need to change audit_ip4/6 to return "false" when it
can't
>> get the l3 information now so we only log zero addresses when the packet
>> really did contain them.
>
> Ok, to clarify the implications, are you saying that handing a NULL
> pointer to "saddr=%pI4" will print "0.0.0.0" rather than
"(none)" or "?"
My initial reaction is that if the packet is so badly
truncated/malformed that we don't have a full IP header than we should
just refrain from logging the packet; it's too malformed/garbage to
offer any useful information and the normal packet processing should
result in the packet being discarded anyway.
Which is why I wanted the ethertype, but that can be coded into the nfmark.
paul moore
- RGB
--
Richard Guy Briggs <rgb(a)redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
11:06 a.m.
On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2017-02-23 11:57, Paul Moore wrote:
> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs <rgb(a)redhat.com> wrote:
> > On 2017-02-23 06:20, Florian Westphal wrote:
> >> Richard Guy Briggs <rgb(a)redhat.com> wrote:
> >> > Simplify and eliminate flipping in and out of message fields, relying
on nfmark
> >> > the way we do for audit_key.
> >> >
> >> > +struct nfpkt_par {
> >> > + int ipv;
> >> > + const void *saddr;
> >> > + const void *daddr;
> >> > + u8 proto;
> >> > +};
> >>
> >> This is problematic, see below for why.
> >>
> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb,
struct nfpkt_par *apar)
> >> > {
> >> > struct iphdr _iph;
> >> > const struct iphdr *ih;
> >> >
> >> > + apar->ipv = 4;
> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> >> > - if (!ih) {
> >> > - audit_log_format(ab, " truncated=1");
> >> > + if (!ih)
> >> > return;
> >>
> >> Removing this "truncated" has the consequence that this can later
log
> >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
> >>
> >> This cannot happen for ip(6)tables because ip stack discards broken l3
headers
> >> before the netfilter hooks get called, but its possible with
NFPROTO_BRIDGE.
> >>
> >> Perhaps you will need to change audit_ip4/6 to return "false" when
it can't
> >> get the l3 information now so we only log zero addresses when the packet
> >> really did contain them.
> >
> > Ok, to clarify the implications, are you saying that handing a NULL
> > pointer to "saddr=%pI4" will print "0.0.0.0" rather than
"(none)" or "?"
>
> My initial reaction is that if the packet is so badly
> truncated/malformed that we don't have a full IP header than we should
> just refrain from logging the packet; it's too malformed/garbage to
> offer any useful information and the normal packet processing should
> result in the packet being discarded anyway.
Which is why I wanted the ethertype, but that can be coded into the nfmark.
If the packet is garbage (garbage without any payload in this case),
what does it matter? It's noise.
--
paul moore
www.paul-moore.com
11:13 a.m.
On 2017-02-23 12:06, Paul Moore wrote:
On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> On 2017-02-23 11:57, Paul Moore wrote:
>> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs <rgb(a)redhat.com>
wrote:
>> > On 2017-02-23 06:20, Florian Westphal wrote:
>> >> Richard Guy Briggs <rgb(a)redhat.com> wrote:
>> >> > Simplify and eliminate flipping in and out of message fields,
relying on nfmark
>> >> > the way we do for audit_key.
>> >> >
>> >> > +struct nfpkt_par {
>> >> > + int ipv;
>> >> > + const void *saddr;
>> >> > + const void *daddr;
>> >> > + u8 proto;
>> >> > +};
>> >>
>> >> This is problematic, see below for why.
>> >>
>> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff
*skb)
>> >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff
*skb, struct nfpkt_par *apar)
>> >> > {
>> >> > struct iphdr _iph;
>> >> > const struct iphdr *ih;
>> >> >
>> >> > + apar->ipv = 4;
>> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
>> >> > - if (!ih) {
>> >> > - audit_log_format(ab, " truncated=1");
>> >> > + if (!ih)
>> >> > return;
>> >>
>> >> Removing this "truncated" has the consequence that this can
later log
>> >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
>> >>
>> >> This cannot happen for ip(6)tables because ip stack discards broken l3
headers
>> >> before the netfilter hooks get called, but its possible with
NFPROTO_BRIDGE.
>> >>
>> >> Perhaps you will need to change audit_ip4/6 to return "false"
when it can't
>> >> get the l3 information now so we only log zero addresses when the
packet
>> >> really did contain them.
>> >
>> > Ok, to clarify the implications, are you saying that handing a NULL
>> > pointer to "saddr=%pI4" will print "0.0.0.0" rather
than "(none)" or "?"
>>
>> My initial reaction is that if the packet is so badly
>> truncated/malformed that we don't have a full IP header than we should
>> just refrain from logging the packet; it's too malformed/garbage to
>> offer any useful information and the normal packet processing should
>> result in the packet being discarded anyway.
>
> Which is why I wanted the ethertype, but that can be coded into the nfmark.
If the packet is garbage (garbage without any payload in this case),
what does it matter? It's noise.
It could be an indicator that either the logging rules or the filter
rules need honing, or even that there is a bug in the network code.
paul moore
- RGB
--
Richard Guy Briggs <rgb(a)redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
11:14 a.m.
On Thu, Feb 23, 2017 at 12:13 PM, Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2017-02-23 12:06, Paul Moore wrote:
> On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs <rgb(a)redhat.com> wrote:
> > On 2017-02-23 11:57, Paul Moore wrote:
> >> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs <rgb(a)redhat.com>
wrote:
> >> > On 2017-02-23 06:20, Florian Westphal wrote:
> >> >> Richard Guy Briggs <rgb(a)redhat.com> wrote:
> >> >> > Simplify and eliminate flipping in and out of message fields,
relying on nfmark
> >> >> > the way we do for audit_key.
> >> >> >
> >> >> > +struct nfpkt_par {
> >> >> > + int ipv;
> >> >> > + const void *saddr;
> >> >> > + const void *daddr;
> >> >> > + u8 proto;
> >> >> > +};
> >> >>
> >> >> This is problematic, see below for why.
> >> >>
> >> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff
*skb)
> >> >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff
*skb, struct nfpkt_par *apar)
> >> >> > {
> >> >> > struct iphdr _iph;
> >> >> > const struct iphdr *ih;
> >> >> >
> >> >> > + apar->ipv = 4;
> >> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> >> >> > - if (!ih) {
> >> >> > - audit_log_format(ab, " truncated=1");
> >> >> > + if (!ih)
> >> >> > return;
> >> >>
> >> >> Removing this "truncated" has the consequence that this
can later log
> >> >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
> >> >>
> >> >> This cannot happen for ip(6)tables because ip stack discards broken
l3 headers
> >> >> before the netfilter hooks get called, but its possible with
NFPROTO_BRIDGE.
> >> >>
> >> >> Perhaps you will need to change audit_ip4/6 to return
"false" when it can't
> >> >> get the l3 information now so we only log zero addresses when the
packet
> >> >> really did contain them.
> >> >
> >> > Ok, to clarify the implications, are you saying that handing a NULL
> >> > pointer to "saddr=%pI4" will print "0.0.0.0" rather
than "(none)" or "?"
> >>
> >> My initial reaction is that if the packet is so badly
> >> truncated/malformed that we don't have a full IP header than we should
> >> just refrain from logging the packet; it's too malformed/garbage to
> >> offer any useful information and the normal packet processing should
> >> result in the packet being discarded anyway.
> >
> > Which is why I wanted the ethertype, but that can be coded into the nfmark.
>
> If the packet is garbage (garbage without any payload in this case),
> what does it matter? It's noise.
It could be an indicator that either the logging rules or the filter
rules need honing, or even that there is a bug in the network code.
Elaborate on this please, I still don't see how logging the ethertype
is helpful for a malformed packet.
--
paul moore
www.paul-moore.com
11:35 a.m.
On 2017-02-23 12:14, Paul Moore wrote:
On Thu, Feb 23, 2017 at 12:13 PM, Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> On 2017-02-23 12:06, Paul Moore wrote:
>> On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs <rgb(a)redhat.com>
wrote:
>> > On 2017-02-23 11:57, Paul Moore wrote:
>> >> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs
<rgb(a)redhat.com> wrote:
>> >> > On 2017-02-23 06:20, Florian Westphal wrote:
>> >> >> Richard Guy Briggs <rgb(a)redhat.com> wrote:
>> >> >> > Simplify and eliminate flipping in and out of message
fields, relying on nfmark
>> >> >> > the way we do for audit_key.
>> >> >> >
>> >> >> > +struct nfpkt_par {
>> >> >> > + int ipv;
>> >> >> > + const void *saddr;
>> >> >> > + const void *daddr;
>> >> >> > + u8 proto;
>> >> >> > +};
>> >> >>
>> >> >> This is problematic, see below for why.
>> >> >>
>> >> >> > -static void audit_ip4(struct audit_buffer *ab, struct
sk_buff *skb)
>> >> >> > +static void audit_ip4(struct audit_buffer *ab, struct
sk_buff *skb, struct nfpkt_par *apar)
>> >> >> > {
>> >> >> > struct iphdr _iph;
>> >> >> > const struct iphdr *ih;
>> >> >> >
>> >> >> > + apar->ipv = 4;
>> >> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph),
&_iph);
>> >> >> > - if (!ih) {
>> >> >> > - audit_log_format(ab, "
truncated=1");
>> >> >> > + if (!ih)
>> >> >> > return;
>> >> >>
>> >> >> Removing this "truncated" has the consequence that
this can later log
>> >> >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
>> >> >>
>> >> >> This cannot happen for ip(6)tables because ip stack discards
broken l3 headers
>> >> >> before the netfilter hooks get called, but its possible with
NFPROTO_BRIDGE.
>> >> >>
>> >> >> Perhaps you will need to change audit_ip4/6 to return
"false" when it can't
>> >> >> get the l3 information now so we only log zero addresses when
the packet
>> >> >> really did contain them.
>> >> >
>> >> > Ok, to clarify the implications, are you saying that handing a
NULL
>> >> > pointer to "saddr=%pI4" will print "0.0.0.0"
rather than "(none)" or "?"
>> >>
>> >> My initial reaction is that if the packet is so badly
>> >> truncated/malformed that we don't have a full IP header than we
should
>> >> just refrain from logging the packet; it's too malformed/garbage
to
>> >> offer any useful information and the normal packet processing should
>> >> result in the packet being discarded anyway.
>> >
>> > Which is why I wanted the ethertype, but that can be coded into the
nfmark.
>>
>> If the packet is garbage (garbage without any payload in this case),
>> what does it matter? It's noise.
>
> It could be an indicator that either the logging rules or the filter
> rules need honing, or even that there is a bug in the network code.
Elaborate on this please, I still don't see how logging the ethertype
is helpful for a malformed packet.
Well, since we can encode it in the nfmark, it could be helpful, but not necessary.
Each bit of information we can include in the audit log message removes
something we need to code in the nf mark. That's why things like ifin,
ifout, action, hook are easy to include and help reduce the amount of
nf mark coding needed when devising netfilter rules.
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.
paul moore
- RGB
--
Richard Guy Briggs <rgb(a)redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
6:54 p.m.
On Thu, Feb 23, 2017 at 12:35 PM, Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2017-02-23 12:14, Paul Moore wrote:
> On Thu, Feb 23, 2017 at 12:13 PM, Richard Guy Briggs <rgb(a)redhat.com> wrote:
> > On 2017-02-23 12:06, Paul Moore wrote:
> >> On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs <rgb(a)redhat.com>
wrote:
> >> > On 2017-02-23 11:57, Paul Moore wrote:
> >> >> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> >> >> > On 2017-02-23 06:20, Florian Westphal wrote:
> >> >> >> Richard Guy Briggs <rgb(a)redhat.com> wrote:
> >> >> >> > Simplify and eliminate flipping in and out of message
fields, relying on nfmark
> >> >> >> > the way we do for audit_key.
> >> >> >> >
> >> >> >> > +struct nfpkt_par {
> >> >> >> > + int ipv;
> >> >> >> > + const void *saddr;
> >> >> >> > + const void *daddr;
> >> >> >> > + u8 proto;
> >> >> >> > +};
> >> >> >>
> >> >> >> This is problematic, see below for why.
> >> >> >>
> >> >> >> > -static void audit_ip4(struct audit_buffer *ab,
struct sk_buff *skb)
> >> >> >> > +static void audit_ip4(struct audit_buffer *ab,
struct sk_buff *skb, struct nfpkt_par *apar)
> >> >> >> > {
> >> >> >> > struct iphdr _iph;
> >> >> >> > const struct iphdr *ih;
> >> >> >> >
> >> >> >> > + apar->ipv = 4;
> >> >> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph),
&_iph);
> >> >> >> > - if (!ih) {
> >> >> >> > - audit_log_format(ab, "
truncated=1");
> >> >> >> > + if (!ih)
> >> >> >> > return;
> >> >> >>
> >> >> >> Removing this "truncated" has the consequence
that this can later log
> >> >> >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return
here.
> >> >> >>
> >> >> >> This cannot happen for ip(6)tables because ip stack
discards broken l3 headers
> >> >> >> before the netfilter hooks get called, but its possible
with NFPROTO_BRIDGE.
> >> >> >>
> >> >> >> Perhaps you will need to change audit_ip4/6 to return
"false" when it can't
> >> >> >> get the l3 information now so we only log zero addresses
when the packet
> >> >> >> really did contain them.
> >> >> >
> >> >> > Ok, to clarify the implications, are you saying that handing a
NULL
> >> >> > pointer to "saddr=%pI4" will print
"0.0.0.0" rather than "(none)" or "?"
> >> >>
> >> >> My initial reaction is that if the packet is so badly
> >> >> truncated/malformed that we don't have a full IP header than we
should
> >> >> just refrain from logging the packet; it's too
malformed/garbage to
> >> >> offer any useful information and the normal packet processing
should
> >> >> result in the packet being discarded anyway.
> >> >
> >> > Which is why I wanted the ethertype, but that can be coded into the
nfmark.
> >>
> >> If the packet is garbage (garbage without any payload in this case),
> >> what does it matter? It's noise.
> >
> > It could be an indicator that either the logging rules or the filter
> > rules need honing, or even that there is a bug in the network code.
>
> Elaborate on this please, I still don't see how logging the ethertype
> is helpful for a malformed packet.
Well, since we can encode it in the nfmark, it could be helpful, but not necessary.
Each bit of information we can include in the audit log message removes
something we need to code in the nf mark. That's why things like ifin,
ifout, action, hook are easy to include and help reduce the amount of
nf mark coding needed when devising netfilter rules.
... but if the packet is so badly manged it doesn't even have a
complete IP header, what's the point in logging the packet at all?
That's the point I'm trying to make.
I had another idea on how to include the sport and dport and that was
to
use the same identifier for sport/icmptype and also for dport/icmpcode,
but you've already said you are not interested.
Not at this point in time since we don't have any good requirements at
the moment. I would like us to keep this small until we have a better
idea of how people want to use this, this way we don't end up stuck
maintaining something that is ill suited for what people actually
want/use.
--
paul moore
www.paul-moore.com
7:50 p.m.
Paul Moore <paul(a)paul-moore.com> wrote:
On Thu, Feb 23, 2017 at 12:35 PM, Richard Guy Briggs
<rgb(a)redhat.com> 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.
11:06 a.m.
Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2017-02-23 11:57, Paul Moore wrote:
> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs <rgb(a)redhat.com> wrote:
> > On 2017-02-23 06:20, Florian Westphal wrote:
> >> Richard Guy Briggs <rgb(a)redhat.com> wrote:
> >> > Simplify and eliminate flipping in and out of message fields, relying
on nfmark
> >> > the way we do for audit_key.
> >> >
> >> > +struct nfpkt_par {
> >> > + int ipv;
> >> > + const void *saddr;
> >> > + const void *daddr;
> >> > + u8 proto;
> >> > +};
> >>
> >> This is problematic, see below for why.
> >>
> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb,
struct nfpkt_par *apar)
> >> > {
> >> > struct iphdr _iph;
> >> > const struct iphdr *ih;
> >> >
> >> > + apar->ipv = 4;
> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> >> > - if (!ih) {
> >> > - audit_log_format(ab, " truncated=1");
> >> > + if (!ih)
> >> > return;
> >>
> >> Removing this "truncated" has the consequence that this can later
log
> >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
> >>
> >> This cannot happen for ip(6)tables because ip stack discards broken l3
headers
> >> before the netfilter hooks get called, but its possible with
NFPROTO_BRIDGE.
> >>
> >> Perhaps you will need to change audit_ip4/6 to return "false"
when it can't
> >> get the l3 information now so we only log zero addresses when the packet
> >> really did contain them.
> >
> > Ok, to clarify the implications, are you saying that handing a NULL
> > pointer to "saddr=%pI4" will print "0.0.0.0" rather than
"(none)" or "?"
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?
11:20 a.m.
On 2017-02-23 18:06, Florian Westphal wrote:
Richard Guy Briggs <rgb(a)redhat.com> wrote:
> On 2017-02-23 11:57, Paul Moore wrote:
> > On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs <rgb(a)redhat.com>
wrote:
> > > On 2017-02-23 06:20, Florian Westphal wrote:
> > >> Richard Guy Briggs <rgb(a)redhat.com> wrote:
> > >> > Simplify and eliminate flipping in and out of message fields,
relying on nfmark
> > >> > the way we do for audit_key.
> > >> >
> > >> > +struct nfpkt_par {
> > >> > + int ipv;
> > >> > + const void *saddr;
> > >> > + const void *daddr;
> > >> > + u8 proto;
> > >> > +};
> > >>
> > >> This is problematic, see below for why.
> > >>
> > >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff
*skb)
> > >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff
*skb, struct nfpkt_par *apar)
> > >> > {
> > >> > struct iphdr _iph;
> > >> > const struct iphdr *ih;
> > >> >
> > >> > + apar->ipv = 4;
> > >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> > >> > - if (!ih) {
> > >> > - audit_log_format(ab, " truncated=1");
> > >> > + if (!ih)
> > >> > return;
> > >>
> > >> Removing this "truncated" has the consequence that this can
later log
> > >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
> > >>
> > >> This cannot happen for ip(6)tables because ip stack discards broken l3
headers
> > >> before the netfilter hooks get called, but its possible with
NFPROTO_BRIDGE.
> > >>
> > >> Perhaps you will need to change audit_ip4/6 to return
"false" when it can't
> > >> get the l3 information now so we only log zero addresses when the
packet
> > >> really did contain them.
> > >
> > > Ok, to clarify the implications, are you saying that handing a NULL
> > > pointer to "saddr=%pI4" will print "0.0.0.0" rather
than "(none)" or "?"
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 <rgb(a)redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
7:59 p.m.
Richard Guy Briggs <rgb(a)redhat.com> 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.
11:56 p.m.
On Thu, Feb 23, 2017 at 8:59 PM, Florian Westphal <fw(a)strlen.de> wrote:
Richard Guy Briggs <rgb(a)redhat.com> 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
11:20 a.m.
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 <rgb(a)redhat.com>
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
11:29 a.m.
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 <rgb(a)redhat.com>
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 <rgb(a)redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
2858
days inactive
2859
days old
linux-audit@lists.linux-audit.osci.io
16 comments
4 participants
participants (4)
-
Florian Westphal -
Paul Moore -
Richard Guy Briggs -
Steve Grubb