[PATCH] ebtables: Clone xt_AUDIT to ebt_audit to return EBT_CONTINUE

[PATCH] ebtables: Clone xt_AUDIT to ebt_audit to return EBT_CONTINUE

[Thomas Graf]

Even though ebtables uses xtables it still requires targets to
return EBT_CONTINUE instead of XT_CONTINUE. This prevented
xt_AUDIT to work as ebt module.

Signed-off-by: Thomas Graf <tgraf@redhat.com>

diff --git a/net/bridge/netfilter/Kconfig b/net/bridge/netfilter/Kconfig
index ba6f73e..8b7d48f 100644
--- a/net/bridge/netfilter/Kconfig
+++ b/net/bridge/netfilter/Kconfig
@@ -144,6 +144,15 @@ config BRIDGE_EBT_ARPREPLY
 
 	  To compile it as a module, choose M here.  If unsure, say N.
 
+config BRIDGE_EBT_AUDIT
+	tristate "ebt: audit target support"
+	depends on AUDIT
+	help
+	  This option adds a 'AUDIT' target, which can be used to create
+	  audit records for packets dropped/accepted.
+
+	  To compile it as a module, choose M here. If unsure, say N.
+
 config BRIDGE_EBT_DNAT
 	tristate "ebt: dnat target support"
 	help
diff --git a/net/bridge/netfilter/Makefile b/net/bridge/netfilter/Makefile
index 0718699..a14865e 100644
--- a/net/bridge/netfilter/Makefile
+++ b/net/bridge/netfilter/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_BRIDGE_EBT_VLAN) += ebt_vlan.o
 
 # targets
 obj-$(CONFIG_BRIDGE_EBT_ARPREPLY) += ebt_arpreply.o
+obj-$(CONFIG_BRIDGE_EBT_AUDIT) += ebt_audit.o
 obj-$(CONFIG_BRIDGE_EBT_MARK_T) += ebt_mark.o
 obj-$(CONFIG_BRIDGE_EBT_DNAT) += ebt_dnat.o
 obj-$(CONFIG_BRIDGE_EBT_REDIRECT) += ebt_redirect.o
diff --git a/net/bridge/netfilter/ebt_audit.c b/net/bridge/netfilter/ebt_audit.c
new file mode 100644
index 0000000..03fe00f
--- /dev/null
+++ b/net/bridge/netfilter/ebt_audit.c
@@ -0,0 +1,201 @@
+/*
+ * Creates audit record for dropped/accepted packets
+ *
+ * (C) 2010-2011 Thomas Graf <tgraf@redhat.com>
+ * (C) 2010-2011 Red Hat, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/audit.h>
+#include <linux/module.h>
+#include <linux/skbuff.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
+#include <linux/if_arp.h>
+#include <linux/netfilter/x_tables.h>
+#include <linux/netfilter/xt_AUDIT.h>
+#include <linux/netfilter_bridge/ebtables.h>
+#include <net/ipv6.h>
+#include <net/ip.h>
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Thomas Graf <tgraf@redhat.com>");
+MODULE_DESCRIPTION("Ebtables: creates audit records for dropped/accepted packets");
+
+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)
+{
+	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;
+	}
+
+	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);
+}
+
+static void audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
+{
+	struct ipv6hdr _ip6h;
+	const struct ipv6hdr *ih;
+	u8 nexthdr;
+	int offset;
+
+	ih = skb_header_pointer(skb, skb_network_offset(skb), sizeof(_ip6h), &_ip6h);
+	if (!ih) {
+		audit_log_format(ab, " truncated=1");
+		return;
+	}
+
+	nexthdr = ih->nexthdr;
+	offset = ipv6_skip_exthdr(skb, skb_network_offset(skb) + sizeof(_ip6h),
+				  &nexthdr);
+
+	audit_log_format(ab, " saddr=%pI6c daddr=%pI6c proto=%hhu",
+			 &ih->saddr, &ih->daddr, nexthdr);
+
+	if (offset)
+		audit_proto(ab, skb, nexthdr, offset);
+}
+
+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;
+
+	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));
+
+		if (par->family == NFPROTO_BRIDGE) {
+			switch (eth_hdr(skb)->h_proto) {
+			case __constant_htons(ETH_P_IP):
+				audit_ip4(ab, skb);
+				break;
+
+			case __constant_htons(ETH_P_IPV6):
+				audit_ip6(ab, skb);
+				break;
+			}
+		}
+	}
+
+	switch (par->family) {
+	case NFPROTO_IPV4:
+		audit_ip4(ab, skb);
+		break;
+
+	case NFPROTO_IPV6:
+		audit_ip6(ab, skb);
+		break;
+	}
+
+	audit_log_end(ab);
+
+errout:
+	return EBT_CONTINUE;
+}
+
+static int audit_tg_check(const struct xt_tgchk_param *par)
+{
+	const struct xt_audit_info *info = par->targinfo;
+
+	if (info->type > XT_AUDIT_TYPE_MAX) {
+		pr_info("Audit type out of range (valid range: 0..%hhu)\n",
+			XT_AUDIT_TYPE_MAX);
+		return -ERANGE;
+	}
+
+	return 0;
+}
+
+static struct xt_target audit_tg_reg __read_mostly = {
+	.name		= "AUDIT",
+	.family		= NFPROTO_BRIDGE,
+	.target		= audit_tg,
+	.targetsize	= sizeof(struct xt_audit_info),
+	.checkentry	= audit_tg_check,
+	.me		= THIS_MODULE,
+};
+
+static int __init audit_tg_init(void)
+{
+	return xt_register_target(&audit_tg_reg);
+}
+
+static void __exit audit_tg_exit(void)
+{
+	xt_unregister_target(&audit_tg_reg);
+}
+
+module_init(audit_tg_init);
+module_exit(audit_tg_exit);
diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
index 81802d2..baa0416 100644
--- a/net/netfilter/xt_AUDIT.c
+++ b/net/netfilter/xt_AUDIT.c
@@ -27,7 +27,6 @@ MODULE_AUTHOR("Thomas Graf <tgraf@redhat.com>");
 MODULE_DESCRIPTION("Xtables: creates audit records for dropped/accepted packets");
 MODULE_ALIAS("ipt_AUDIT");
 MODULE_ALIAS("ip6t_AUDIT");
-MODULE_ALIAS("ebt_AUDIT");
 MODULE_ALIAS("arpt_AUDIT");
 
 static void audit_proto(struct audit_buffer *ab, struct sk_buff *skb,

Re: [PATCH] ebtables: Clone xt_AUDIT to ebt_audit to return EBT_CONTINUE

[Jan Engelhardt]

On Wednesday 2011-03-16 01:33, Thomas Graf wrote:

>Even though ebtables uses xtables it still requires targets to
>return EBT_CONTINUE instead of XT_CONTINUE. This prevented
>xt_AUDIT to work as ebt module.

Something that just came to mind is that you could probably do
to keep the code at a minimum:


static unsigned int ebt_audit(struct xt_target_param *par)
{
	unsigned int ret = xt_audit_tg(par);

	if (ret == XT_CONTINUE)
		return EBT_CONTINUE;
	...
}

static struct xt_target audit_tg_reg[] = {
	{
		.name = "AUDIT",
		.family = NFPROTO_UNSPEC,
		.target = xt_audit_tg,
	},
	{
		.name = "AUDIT",
		.family = NFPROTO_BRIDGE,
		.target = ebt_audit_tg,
	},
};

Re: [PATCH] ebtables: Clone xt_AUDIT to ebt_audit to return EBT_CONTINUE

[Patrick McHardy]

Am 16.03.2011 01:43, schrieb Jan Engelhardt:
> On Wednesday 2011-03-16 01:33, Thomas Graf wrote:
> 
>> Even though ebtables uses xtables it still requires targets to
>> return EBT_CONTINUE instead of XT_CONTINUE. This prevented
>> xt_AUDIT to work as ebt module.
> 
> Something that just came to mind is that you could probably do
> to keep the code at a minimum:
> 
> 
> static unsigned int ebt_audit(struct xt_target_param *par)
> {
> 	unsigned int ret = xt_audit_tg(par);
> 
> 	if (ret == XT_CONTINUE)
> 		return EBT_CONTINUE;
> 	...
> }

Seems like a good idea to me. If more modules need this (f.i.
MARK) we could also consider doing the mapping based on a target
flag in ebtables itself.

However please see Dave's mail about net-next, until the merge
window is over and -rc1 released only bugfixes will be accepted.

Re: [PATCH] ebtables: Clone xt_AUDIT to ebt_audit to return EBT_CONTINUE

[Thomas Graf]

On Wed, Mar 16, 2011 at 06:40:33AM +0100, Patrick McHardy wrote:
> Am 16.03.2011 01:43, schrieb Jan Engelhardt:
> > On Wednesday 2011-03-16 01:33, Thomas Graf wrote:
> > 
> >> Even though ebtables uses xtables it still requires targets to
> >> return EBT_CONTINUE instead of XT_CONTINUE. This prevented
> >> xt_AUDIT to work as ebt module.
> > 
> > Something that just came to mind is that you could probably do
> > to keep the code at a minimum:
> > 
> > 
> > static unsigned int ebt_audit(struct xt_target_param *par)
> > {
> > 	unsigned int ret = xt_audit_tg(par);
> > 
> > 	if (ret == XT_CONTINUE)
> > 		return EBT_CONTINUE;
> > 	...
> > }

That's a good idea, thanks Jan!

> Seems like a good idea to me. If more modules need this (f.i.
> MARK) we could also consider doing the mapping based on a target
> flag in ebtables itself.
> 
> However please see Dave's mail about net-next, until the merge
> window is over and -rc1 released only bugfixes will be accepted.

I was considering this a bugfix because the module as-is can be
loaded with ebtables, will create audit records but won't allow
for the next rule to drop/reject the packet.

Would you consider Jan's approach a bugfix or should I wait?

[PATCH] ebtables: Fix xt_AUDIT to work with ebtables, return EBT_CONTINUE if NFPROTO_BRIDGE

[Thomas Graf]

Even though ebtables uses xtables it still requires targets to
return EBT_CONTINUE instead of XT_CONTINUE. This prevented
xt_AUDIT to work as ebt module.

Upon Jan's suggestion, use a separate struct xt_target for
NFPROTO_BRIDGE having its own target callback returning
EBT_CONTINUE instead of cloning the module.

Signed-off-by: Thomas Graf <tgraf@redhat.com>

diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
index 81802d2..363a99e 100644
--- a/net/netfilter/xt_AUDIT.c
+++ b/net/netfilter/xt_AUDIT.c
@@ -19,6 +19,7 @@
 #include <linux/if_arp.h>
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_AUDIT.h>
+#include <linux/netfilter_bridge/ebtables.h>
 #include <net/ipv6.h>
 #include <net/ip.h>
 
@@ -168,6 +169,13 @@ errout:
 	return XT_CONTINUE;
 }
 
+static unsigned int
+audit_tg_ebt(struct sk_buff *skb, const struct xt_action_param *par)
+{
+	audit_tg(skb, par);
+	return EBT_CONTINUE;
+}
+
 static int audit_tg_check(const struct xt_tgchk_param *par)
 {
 	const struct xt_audit_info *info = par->targinfo;
@@ -181,23 +189,33 @@ static int audit_tg_check(const struct xt_tgchk_param *par)
 	return 0;
 }
 
-static struct xt_target audit_tg_reg __read_mostly = {
-	.name		= "AUDIT",
-	.family		= NFPROTO_UNSPEC,
-	.target		= audit_tg,
-	.targetsize	= sizeof(struct xt_audit_info),
-	.checkentry	= audit_tg_check,
-	.me		= THIS_MODULE,
+static struct xt_target audit_tg_reg[] __read_mostly = {
+	{
+		.name		= "AUDIT",
+		.family		= NFPROTO_UNSPEC,
+		.target		= audit_tg,
+		.targetsize	= sizeof(struct xt_audit_info),
+		.checkentry	= audit_tg_check,
+		.me		= THIS_MODULE,
+	},
+	{
+		.name		= "AUDIT",
+		.family		= NFPROTO_BRIDGE,
+		.target		= audit_tg_ebt,
+		.targetsize	= sizeof(struct xt_audit_info),
+		.checkentry	= audit_tg_check,
+		.me		= THIS_MODULE,
+	},
 };
 
 static int __init audit_tg_init(void)
 {
-	return xt_register_target(&audit_tg_reg);
+	return xt_register_targets(audit_tg_reg, ARRAY_SIZE(audit_tg_reg));
 }
 
 static void __exit audit_tg_exit(void)
 {
-	xt_unregister_target(&audit_tg_reg);
+	xt_unregister_targets(audit_tg_reg, ARRAY_SIZE(audit_tg_reg));
 }
 
 module_init(audit_tg_init);

Re: [PATCH] ebtables: Clone xt_AUDIT to ebt_audit to return EBT_CONTINUE

[Patrick McHardy]

On 16.03.2011 09:24, Thomas Graf wrote:
> On Wed, Mar 16, 2011 at 06:40:33AM +0100, Patrick McHardy wrote:
>> Am 16.03.2011 01:43, schrieb Jan Engelhardt:
>>> On Wednesday 2011-03-16 01:33, Thomas Graf wrote:
>>>
>>>> Even though ebtables uses xtables it still requires targets to
>>>> return EBT_CONTINUE instead of XT_CONTINUE. This prevented
>>>> xt_AUDIT to work as ebt module.
>>>
>>> Something that just came to mind is that you could probably do
>>> to keep the code at a minimum:
>>>
>>>
>>> static unsigned int ebt_audit(struct xt_target_param *par)
>>> {
>>> 	unsigned int ret = xt_audit_tg(par);
>>>
>>> 	if (ret == XT_CONTINUE)
>>> 		return EBT_CONTINUE;
>>> 	...
>>> }
> 
> That's a good idea, thanks Jan!
> 
>> Seems like a good idea to me. If more modules need this (f.i.
>> MARK) we could also consider doing the mapping based on a target
>> flag in ebtables itself.
>>
>> However please see Dave's mail about net-next, until the merge
>> window is over and -rc1 released only bugfixes will be accepted.
> 
> I was considering this a bugfix because the module as-is can be
> loaded with ebtables, will create audit records but won't allow
> for the next rule to drop/reject the packet.
> 
> Would you consider Jan's approach a bugfix or should I wait?

Yes, this qualifies as a bugfix in my opinion since we're returning
invalid verdicts to ebtables. I'll apply your patch later today.

Re: [PATCH] ebtables: Fix xt_AUDIT to work with ebtables, return EBT_CONTINUE if NFPROTO_BRIDGE

[Patrick McHardy]

On 16.03.2011 10:20, Thomas Graf wrote:
> Even though ebtables uses xtables it still requires targets to
> return EBT_CONTINUE instead of XT_CONTINUE. This prevented
> xt_AUDIT to work as ebt module.
> 
> Upon Jan's suggestion, use a separate struct xt_target for
> NFPROTO_BRIDGE having its own target callback returning
> EBT_CONTINUE instead of cloning the module.
> 

Applied, thanks Thomas.