[PATCH nf next 0/3] bridge: netfilter: fix handling of ipv4 packets w. options

[PATCH nf next 0/3] bridge: netfilter: fix handling of ipv4 packets w. options

[Florian Westphal]

David Newall reported that bridge causes bad checksums:
http://thread.gmane.org/gmane.linux.network/315705/focus=1706769

The proposal was to revert
462fb2af9788a82a5 (bridge : Sanitize skb before it enters the IP stack).

However, this has some other adverse effects since bridge netfilter
and ip stack both use skb->cb (and we thus memset skb->cb whenever
we hand skb off to the ip stack).

So, this series attemps to resolve this a bit differently.

First, lets add the inet_param padding that Eric suggested previously.
This means that any earlier setup of IPCB will be preserved inside the
bridge layer.

This is also useful for netfilter since it will preserve
IPCB(skb)->frag_max_size set up by ip defrag.

Second, this gets rid of the option parsing/memset calls in
to forward and output cases.

Third, the pre-routing path is changed to not mangle the packets
but to only validate the ip options.

This patch series is vs. next instead of net/nf tree.

This has been broken for so long that I don't think we need
to rush this.

[PATCH nf next 1/3] bridge: prepend inet_skb_param dummy to bridge cb

[Florian Westphal]

bridge can make upcalls into the ip stack, especially
when bridge netfilter is involved, we can end up calling ip_fragment().

IPv4 functions, however, may (rightfully) depend on skb->cb[]
containing the IPCB area, where eg. earlier-parsed ip options
reside.

However, since bridge has its own cb area, this has caused several
crashes in the past, and several call sites in br_netfilter since
zero ->cb again before invoking netfilter hooks.

We've tried to cure these in the past by applying memsets of skb->cb
where needed, and parsing ip options within the bridge layer.

This isn't such a great idea since we e.g. lose max fragment size
information stored there via ipv4 defrag.

Also, since 462fb2af9788a82 (bridge : Sanitize skb before it enters the IP
stack) bridge handling of received packets with ipv4 options is broken
in different ways (crash, then discarding of such packets).

This patch, originally proposed by Eric Dumazet, prepends
inet_skb_param padding so IPCB contents will be preserved (e.g.
ipv4 defrag info).

This is a first step in fixing handling of ipv4 packets with options.

br_input_skb_cb is now exactly 48 bytes.

Cc: Bandan Das <bsd@redhat.com>
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/bridge/br.c         | 2 ++
 net/bridge/br_private.h | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/net/bridge/br.c b/net/bridge/br.c
index 44425af..4ee730e 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -147,6 +147,8 @@ static int __init br_init(void)
 {
 	int err;
 
+	BUILD_BUG_ON(sizeof(struct br_input_skb_cb) > FIELD_SIZEOF(struct sk_buff, cb));
+
 	err = stp_proto_register(&br_stp_proto);
 	if (err < 0) {
 		pr_err("bridge: can't register sap for STP\n");
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index f53592f..559938f 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -19,6 +19,8 @@
 #include <linux/u64_stats_sync.h>
 #include <net/route.h>
 #include <linux/if_vlan.h>
+#include <linux/ipv6.h>
+#include <net/ip.h>
 
 #define BR_HASH_BITS 8
 #define BR_HASH_SIZE (1 << BR_HASH_BITS)
@@ -304,6 +306,10 @@ struct net_bridge
 };
 
 struct br_input_skb_cb {
+	union {
+		struct inet_skb_parm inet4_parm;
+		struct inet6_skb_parm inet6_param;
+	} inet_parm;
 	struct net_device *brdev;
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	int igmp;
-- 
2.0.4

[PATCH nf next 2/3] netfilter: bridge: don't parse ip headers in fwd and output path

[Florian Westphal]

We currently call ip_options_compile() for incoming, forwarded,
and outgoing packets.

This prevents ipv4 packets that have ip options set from being processed
by a linux bridge; both for the forward and the local delivery cases.

We first call ip_options_compile() from the pre-routing path.
This will mangle the ipv4 packet, which is already problematic since
it is a layering violation.
But this also will invalidate the ipv4 header checksum.

Since the checksum isn't fixed up, we then drop the packet in the ipv4
input path (for local delivery to the bridge) or when forwarding the
packet (since br_nf_forward_ip invokes br_parse_ip_options(), because it
has checksum error.

For the output path, this is no longer needed either:
previous change added inet_skb_param padding to br_input_skb_cb, so
bridge can no longer overwrite any IPCB data.

With this patch, such packets are not dropped by the bridge anymore,
but they are still corrupted.  This is addressed by next patch.

Cc: Bandan Das <bsd@redhat.com>
Reported-by: David Newall <davidn@davidnewall.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/bridge/br_netfilter.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index fa1270c..56c7ed8 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -718,9 +718,6 @@ static unsigned int br_nf_forward_ip(const struct nf_hook_ops *ops,
 		nf_bridge->mask |= BRNF_PKT_TYPE;
 	}
 
-	if (pf == NFPROTO_IPV4 && br_parse_ip_options(skb))
-		return NF_DROP;
-
 	/* The physdev module checks on this */
 	nf_bridge->mask |= BRNF_BRIDGED;
 	nf_bridge->physoutdev = skb->dev;
@@ -778,12 +775,9 @@ static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 
 	if (skb->protocol == htons(ETH_P_IP) &&
 	    skb->len + nf_bridge_mtu_reduction(skb) > skb->dev->mtu &&
-	    !skb_is_gso(skb)) {
-		if (br_parse_ip_options(skb))
-			/* Drop invalid packet */
-			return NF_DROP;
+	    !skb_is_gso(skb))
 		ret = ip_fragment(skb, br_dev_queue_push_xmit);
-	} else
+	else
 		ret = br_dev_queue_push_xmit(skb);
 
 	return ret;
-- 
2.0.4

[PATCH nf-next 3/3] netfilter: bridge: don't mangle ipv4 header options

[Florian Westphal]

a bridge is meant to be L3 protocol agnostic, we should not act on ipv4
header options.  Thus, ensure that skb data isn't modified when
parsing options and also remove the ip_options_rcv_srr() call.

The only purpose of this function is to do sanity tests so upcalls into
netfilter will have the same checks applied as done by the ipv4 input path.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Bandan Das <bsd@redhat.com>
Reported-by: David Newall <davidn@davidnewall.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/bridge/br_netfilter.c | 42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 56c7ed8..f5cb2ef 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -185,14 +185,17 @@ static inline void nf_bridge_save_header(struct sk_buff *skb)
 					 skb->nf_bridge->data, header_size);
 }
 
-/* When handing a packet over to the IP layer
- * check whether we have a skb that is in the
- * expected format
+/* When handing an ipv4 packet over to netfilter, we must
+ * first replicate the sanity tests performed in the IP stack
+ * input path.  This includes making sure that the entire ip
+ * header is in the linear skb area, ip->ihl is sane, etc.
  */
-
-static int br_parse_ip_options(struct sk_buff *skb)
+static bool br_ip_input_valid(struct sk_buff *skb)
 {
-	struct ip_options *opt;
+	struct {
+		struct ip_options opt;
+		u8 hdrdata[0xf * 4];
+	} ip_opts;
 	const struct iphdr *iph;
 	struct net_device *dev = skb->dev;
 	u32 len;
@@ -201,7 +204,6 @@ static int br_parse_ip_options(struct sk_buff *skb)
 		goto inhdr_error;
 
 	iph = ip_hdr(skb);
-	opt = &(IPCB(skb)->opt);
 
 	/* Basic sanity checks */
 	if (iph->ihl < 5 || iph->version != 4)
@@ -228,28 +230,22 @@ static int br_parse_ip_options(struct sk_buff *skb)
 
 	memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
 	if (iph->ihl == 5)
-		return 0;
-
-	opt->optlen = iph->ihl*4 - sizeof(struct iphdr);
-	if (ip_options_compile(dev_net(dev), opt, skb))
-		goto inhdr_error;
+		return true;
 
-	/* Check correct handling of SRR option */
-	if (unlikely(opt->srr)) {
-		struct in_device *in_dev = __in_dev_get_rcu(dev);
-		if (in_dev && !IN_DEV_SOURCE_ROUTE(in_dev))
-			goto drop;
+	memset(&ip_opts.opt, 0, sizeof(ip_opts.opt));
+	ip_opts.opt.optlen = iph->ihl*4 - sizeof(struct iphdr);
+	memcpy(ip_opts.hdrdata, iph + 1, ip_opts.opt.optlen);
 
-		if (ip_options_rcv_srr(skb))
-			goto drop;
-	}
+	/* We only call this to validate iph options. */
+	if (ip_options_compile(dev_net(dev), &ip_opts.opt, NULL))
+		goto inhdr_error;
 
-	return 0;
+	return true;
 
 inhdr_error:
 	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INHDRERRORS);
 drop:
-	return -1;
+	return false;
 }
 
 /* PF_BRIDGE/PRE_ROUTING *********************************************/
@@ -617,7 +613,7 @@ static unsigned int br_nf_pre_routing(const struct nf_hook_ops *ops,
 
 	nf_bridge_pull_encap_header_rcsum(skb);
 
-	if (br_parse_ip_options(skb))
+	if (!br_ip_input_valid(skb))
 		return NF_DROP;
 
 	nf_bridge_put(skb->nf_bridge);
-- 
2.0.4

Re: [PATCH nf next 0/3] bridge: netfilter: fix handling of ipv4 packets w. options

[Herbert Xu]

On Sat, Oct 04, 2014 at 03:04:27AM +0200, Florian Westphal wrote:
> David Newall reported that bridge causes bad checksums:
> http://thread.gmane.org/gmane.linux.network/315705/focus=1706769
> 
> The proposal was to revert
> 462fb2af9788a82a5 (bridge : Sanitize skb before it enters the IP stack).
> 
> However, this has some other adverse effects since bridge netfilter
> and ip stack both use skb->cb (and we thus memset skb->cb whenever
> we hand skb off to the ip stack).
> 
> So, this series attemps to resolve this a bit differently.
> 
> First, lets add the inet_param padding that Eric suggested previously.
> This means that any earlier setup of IPCB will be preserved inside the
> bridge layer.
> 
> This is also useful for netfilter since it will preserve
> IPCB(skb)->frag_max_size set up by ip defrag.
> 
> Second, this gets rid of the option parsing/memset calls in
> to forward and output cases.
> 
> Third, the pre-routing path is changed to not mangle the packets
> but to only validate the ip options.
> 
> This patch series is vs. next instead of net/nf tree.
> 
> This has been broken for so long that I don't think we need
> to rush this.

I'm unsure whether this is the right approach.  So if I understand
this correctly your problem is coming from packets that are

	IP stack => bridge => IP stack

in which case preserving IP options may work.

But does your patch handle packets that are

	external => bridge => IP stack

The reason I asked for the IPCB to be built is to handle exactly
that case.

In fact, even preserving IPCB in the IP stack reentry case is
a hack since if we ever change the IP stack in future such that
on exit the IPCB is no longer valid for reentry your approach
will fail.

Now as to your original problem that ip_options_compile mangles
the packet this is something I explicitly said we should fix
before we added br_parse_ip_options (point 2 in that email):

	https://lkml.org/lkml/2010/9/3/16

Unfortunately it looks like nobody actually did the audit.

So my suggestion would be to fix br_parse_ip_options so that
it never mangles the packet.

Does this fix your problem or are there other issues that I
have overlooked?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH nf next 0/3] bridge: netfilter: fix handling of ipv4 packets w. options

[Florian Westphal]

Herbert Xu <herbert@gondor.apana.org.au> wrote:

[ fix netdev mail address, sorry about that ]

> On Sat, Oct 04, 2014 at 03:04:27AM +0200, Florian Westphal wrote:
> > David Newall reported that bridge causes bad checksums:
> > http://thread.gmane.org/gmane.linux.network/315705/focus=1706769
> > 
> > The proposal was to revert
> > 462fb2af9788a82a5 (bridge : Sanitize skb before it enters the IP stack).
> > 
> > However, this has some other adverse effects since bridge netfilter
> > and ip stack both use skb->cb (and we thus memset skb->cb whenever
> > we hand skb off to the ip stack).
> > 
> > So, this series attemps to resolve this a bit differently.
> > 
> > First, lets add the inet_param padding that Eric suggested previously.
> > This means that any earlier setup of IPCB will be preserved inside the
> > bridge layer.
> > 
> > This is also useful for netfilter since it will preserve
> > IPCB(skb)->frag_max_size set up by ip defrag.
> > 
> > Second, this gets rid of the option parsing/memset calls in
> > to forward and output cases.
> > 
> > Third, the pre-routing path is changed to not mangle the packets
> > but to only validate the ip options.
> > 
> > This patch series is vs. next instead of net/nf tree.
> > 
> > This has been broken for so long that I don't think we need
> > to rush this.
> 
> I'm unsure whether this is the right approach.  So if I understand
> this correctly your problem is coming from packets that are
> 
> 	IP stack => bridge => IP stack

Just to clarify, right now this doesn't work:
ping -R <addr-of-bridge>
ping -R <addr-behind-bridge>

> in which case preserving IP options may work.
> 
> But does your patch handle packets that are
> 
> 	external => bridge => IP stack

Aside from above record-route test I also played with a bogus bridge
setup where incoming packets can exceed br0 mtu, in this case we emit
frag error without echoing/acting on the options.

IP (..  flags [DF], proto ICMP (1), length 1508, options (NOP,RR 192.168.1.1, 0.0.0.0 0.0.0.0 0.0.0.0 0.0.0.0 0.0.0.0 0.0.0.0 0.0.0.0 0.0.0.0))
192.168.1.1 > 192.168.1.16: ICMP echo request, id 26676, seq 1, length 1448
IP (..  flags [none], proto ICMP (1), length 576) 192.168.1.10 > 192.168.1.1: ICMP 192.168.1.16 unreachable - need to frag (mtu 1500), length 556

1.10 is br0 IP, 1.16 and 1.1 are on different bridge ports, 1.1 has
bogus (larger) mtu than all other hosts.

The fragment error does not echo any RR information.
Is that your concern?

> The reason I asked for the IPCB to be built is to handle exactly
> that case.

Why do we need to compile ip options, exactly?  If the packet
is locally delivered, we hand it up to the ip stack which will
compile ip options normally.

If its forwarded, it only travels through netfilter hooks.
The preserved ip_options_compile() call will make sure options
look sane (we don't preserve the built opts information in
this patch).

The only case where it can reenter in fwd case, AFAICS, is when the
skb exceeds the mtu due to nf_defrag (reenter via call to ip_fragment()).

And we used to get crash here when calling icmp_send since skb->cb
was pointing to bridge cb, which then would crash in __ip_options_echo()
because the various IPCB->opts offsets were garbage.

But, why would we want to echo options?

We're just a bridge (so yes, strictly speaking the icmp response
is already wrong, but silently tossing packets doesn't seem right
either).

Are you saying we should act like router and set the options?

> In fact, even preserving IPCB in the IP stack reentry case is
> a hack since if we ever change the IP stack in future such that
> on exit the IPCB is no longer valid for reentry your approach
> will fail.

True.  I guess in that case, we'd have to resort to less
straightforward approach, i.e. explicitly add the IPCB parts
we wish to retain to br_input_skb_cb, then translate back-and-forth
where needed.

> Now as to your original problem that ip_options_compile mangles
> the packet this is something I explicitly said we should fix
> before we added br_parse_ip_options (point 2 in that email):
> 
> 	https://lkml.org/lkml/2010/9/3/16
> 
> Unfortunately it looks like nobody actually did the audit.

Right.

> So my suggestion would be to fix br_parse_ip_options so that
> it never mangles the packet.

This patch avoids the option mangling by passing in a NULL skb.
So to do what you want all that is needed is to remember
the parsed opts result.  If we add Erics suggested inet cb pad
we can just place the parsed option struct into IPCB()->opts.

If not, we could add struct ip_options to br_input_skb_cb
and stash it there (we'd still need to re-arrange skb->cb to
what ip stack expects though when calling back into it in output
path).

Alternatively, we could call the ipv4 parsing function again
to re-construct IPCB->opts.

I'm just not yet sure if this is the right idea.
Remembering the information will cause the icmp frag error
above to list br0 ip address in the icmp frag error.

Under which circumstances would we want/need to remember the
parsed options (i.e. retain struct ip_options in ->cb[]), or
act upon them?

Thanks,
Florian

Re: [PATCH nf next 0/3] bridge: netfilter: fix handling of ipv4 packets w. options

[Herbert Xu]

On Sat, Oct 04, 2014 at 12:04:13PM +0200, Florian Westphal wrote:
>
> > The reason I asked for the IPCB to be built is to handle exactly
> > that case.
> 
> Why do we need to compile ip options, exactly?  If the packet
> is locally delivered, we hand it up to the ip stack which will
> compile ip options normally.

Good point.  I thought we added this because Bandan Das wanted
options.  But rereading the thread in question

	http://lkml.org/lkml/2010/9/3/16

it seems that he doesn't actually need options.  So what happened
appears to be a misunderstanding.  Bandan tried to improve my
original memset hack by compiling options which would have been
fine except that his approach ended up mangling the packet which
is a big no-no.

So the most straightforward solution is to go back to my original
hack and just do a straight memset zero of the cb area before
each entry into the IP stack from the bridge.

I'll try to create a patch that essentially reverts the patch
that led us here.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

bridge: Do not compile options in br_parse_ip_options

[Herbert Xu]

On Sat, Oct 04, 2014 at 09:55:08PM +0800, Herbert Xu wrote:
>
> I'll try to create a patch that essentially reverts the patch
> that led us here.

Here is a patch that's only compile-tested:

bridge: Do not compile options in br_parse_ip_options

Commit 462fb2af9788a82a534f8184abfde31574e1cfa0

	bridge : Sanitize skb before it enters the IP stack

broke when IP options are actually used because it mangles the
skb as if it entered the IP stack which is wrong because the
bridge is supposed to operate below the IP stack.

Since nobody has actually requested for parsing of IP options
this patch fixes it by simply reverting to the previous approach
of ignoring all IP options, i.e., zeroing the IPCB.

If and when somebody who uses IP options and actually needs them
to be parsed by the bridge complains then we can revisit this.

Reported-by: David Newall <davidn@davidnewall.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index a615264..c0fdb4d 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -260,7 +260,6 @@ static inline void nf_bridge_update_protocol(struct sk_buff *skb)
 
 static int br_parse_ip_options(struct sk_buff *skb)
 {
-	struct ip_options *opt;
 	const struct iphdr *iph;
 	struct net_device *dev = skb->dev;
 	u32 len;
@@ -269,7 +268,6 @@ static int br_parse_ip_options(struct sk_buff *skb)
 		goto inhdr_error;
 
 	iph = ip_hdr(skb);
-	opt = &(IPCB(skb)->opt);
 
 	/* Basic sanity checks */
 	if (iph->ihl < 5 || iph->version != 4)
@@ -295,23 +293,11 @@ static int br_parse_ip_options(struct sk_buff *skb)
 	}
 
 	memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
-	if (iph->ihl == 5)
-		return 0;
-
-	opt->optlen = iph->ihl*4 - sizeof(struct iphdr);
-	if (ip_options_compile(dev_net(dev), opt, skb))
-		goto inhdr_error;
-
-	/* Check correct handling of SRR option */
-	if (unlikely(opt->srr)) {
-		struct in_device *in_dev = __in_dev_get_rcu(dev);
-		if (in_dev && !IN_DEV_SOURCE_ROUTE(in_dev))
-			goto drop;
-
-		if (ip_options_rcv_srr(skb))
-			goto drop;
-	}
-
+	/* We should really parse IP options here but until
+	 * somebody who actually uses IP options complains to
+	 * us we'll just silently ignore the options because
+	 * we're lazy!
+	 */
 	return 0;
 
 inhdr_error:

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: bridge: Do not compile options in br_parse_ip_options

[Florian Westphal]

Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Sat, Oct 04, 2014 at 09:55:08PM +0800, Herbert Xu wrote:
> >
> > I'll try to create a patch that essentially reverts the patch
> > that led us here.
> 
> Here is a patch that's only compile-tested:
> 
> bridge: Do not compile options in br_parse_ip_options
> 
> Commit 462fb2af9788a82a534f8184abfde31574e1cfa0
> 
> 	bridge : Sanitize skb before it enters the IP stack
> 
> broke when IP options are actually used because it mangles the
> skb as if it entered the IP stack which is wrong because the
> bridge is supposed to operate below the IP stack.
> 
> Since nobody has actually requested for parsing of IP options
> this patch fixes it by simply reverting to the previous approach
> of ignoring all IP options, i.e., zeroing the IPCB.

Fair enough.  We lose frag_max_size information from ipv4 defrag,
plus netfilter hooks are called without validating ip options.

The former has not worked ever with bridge, and the latter
evidentily isn't a problem either since this has not worked at all
for three years...

So I am fine with it, provided we rename br_parse_ip_options() --
thats not what it does after this patch (br_validate_iphdr(), for
example?)

> If and when somebody who uses IP options and actually needs them
> to be parsed by the bridge complains then we can revisit this.

Ok, fair enough.

Thanks Herbert.

bridge: Respect call-iptables sysctls everywhere

[Herbert Xu]

On Sat, Oct 04, 2014 at 08:06:47PM +0200, Florian Westphal wrote:
>
> Fair enough.  We lose frag_max_size information from ipv4 defrag,

Sigh.  Why are people still doing IP netfilter through the bridge?
It's a huge security hole because all bridge devices share the same
defrag zone so each bridge port can inject packets into any bridge
device on the system through conntrack.  It used to be an even bigger
hole when all defrag were in the same zone which meant that you could
inject packets into the IP stack itself.  At least that hole is
closed now.

So in this case what we have is a bridge packet that temporarily
enters the IP stack for filtering, then reenters the bridge for
processing, and then gets reinserted into the IP stack for filtering.

What we should do therefore is to save any necessary information
such as frag_max_size into the bridge CB area when reentering the
bridge and then copy it back upon the next reentry into the IP stack.

But really we should be printing a big warning to tell people that
this feature (specifically IP netfilter through the bridge, netfilter
through the bridge itself is fine) is insecure and shouldn't be used
until such a time that it is redesigned properly.

> plus netfilter hooks are called without validating ip options.

This was the status quo before the patch in question.  Patches are
welcome.
 
> So I am fine with it, provided we rename br_parse_ip_options() --
> thats not what it does after this patch (br_validate_iphdr(), for
> example?)

I thought about renaming it but if we ever do add option parsing
then we'll be renaming it back.  So let's just stick with the name
plus my comment in the function.

While reviewing this code it occured to me that we have a serious
bug in that call-iptables sysctls aren't even respected in FORWARD
and POST_ROUTING.  Here is a patch that fixes this.

bridge: Respect call-iptables sysctls everywhere

>From the very beginning the call-iptables sysctl only prevented
the PRE_ROUTING hook from entering the IP stack.  This is very
wrong.  The sysctl is used because entering the IP stack from the
bridge has serious security ramifications so when the admin says
that we shouldn't do it, it really means no.

This patch fixes this by also checking the sysctl in FORWARD and
POST_ROUTING.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index c0fdb4d..389d1c6 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -171,20 +171,28 @@ void br_netfilter_rtable_init(struct net_bridge *br)
 	rt->dst.ops = &fake_dst_ops;
 }
 
-static inline struct rtable *bridge_parent_rtable(const struct net_device *dev)
+static inline struct net_bridge *bridge_parent(const struct net_device *dev)
 {
 	struct net_bridge_port *port;
 
 	port = br_port_get_rcu(dev);
-	return port ? &port->br->fake_rtable : NULL;
+	return port ? port->br : NULL;
 }
 
-static inline struct net_device *bridge_parent(const struct net_device *dev)
+static inline struct rtable *bridge_parent_rtable(const struct net_device *dev)
 {
-	struct net_bridge_port *port;
+	struct net_bridge *br;
 
-	port = br_port_get_rcu(dev);
-	return port ? port->br->dev : NULL;
+	br = bridge_parent(dev);
+	return br ? &br->fake_rtable : NULL;
+}
+
+static inline struct net_device *bridge_parent_dev(const struct net_device *dev)
+{
+	struct net_bridge *br;
+
+	br = bridge_parent(dev);
+	return br ? br->dev : NULL;
 }
 
 static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb)
@@ -367,7 +375,7 @@ static int br_nf_pre_routing_finish_bridge(struct sk_buff *skb)
 	struct neighbour *neigh;
 	struct dst_entry *dst;
 
-	skb->dev = bridge_parent(skb->dev);
+	skb->dev = bridge_parent_dev(skb->dev);
 	if (!skb->dev)
 		goto free_skb;
 	dst = skb_dst(skb);
@@ -517,7 +525,7 @@ static struct net_device *brnf_get_logical_dev(struct sk_buff *skb, const struct
 {
 	struct net_device *vlan, *br;
 
-	br = bridge_parent(dev);
+	br = bridge_parent_dev(dev);
 	if (brnf_pass_vlan_indev == 0 || !vlan_tx_tag_present(skb))
 		return br;
 
@@ -763,6 +771,7 @@ static unsigned int br_nf_forward_ip(const struct nf_hook_ops *ops,
 {
 	struct nf_bridge_info *nf_bridge;
 	struct net_device *parent;
+	struct net_bridge *br;
 	u_int8_t pf;
 
 	if (!skb->nf_bridge)
@@ -773,15 +782,21 @@ static unsigned int br_nf_forward_ip(const struct nf_hook_ops *ops,
 	if (!nf_bridge_unshare(skb))
 		return NF_DROP;
 
-	parent = bridge_parent(out);
-	if (!parent)
+	br = bridge_parent(out);
+	if (!br)
 		return NF_DROP;
 
-	if (IS_IP(skb) || IS_VLAN_IP(skb) || IS_PPPOE_IP(skb))
+	parent = br->dev;
+
+	if (IS_IP(skb) || IS_VLAN_IP(skb) || IS_PPPOE_IP(skb)) {
 		pf = NFPROTO_IPV4;
-	else if (IS_IPV6(skb) || IS_VLAN_IPV6(skb) || IS_PPPOE_IPV6(skb))
+		if (!brnf_call_iptables && !br->nf_call_iptables)
+			return NF_ACCEPT;
+	} else if (IS_IPV6(skb) || IS_VLAN_IPV6(skb) || IS_PPPOE_IPV6(skb)) {
 		pf = NFPROTO_IPV6;
-	else
+		if (!brnf_call_ip6tables && !br->nf_call_ip6tables)
+			return NF_ACCEPT;
+	} else
 		return NF_ACCEPT;
 
 	nf_bridge_pull_encap_header(skb);
@@ -877,20 +892,27 @@ static unsigned int br_nf_post_routing(const struct nf_hook_ops *ops,
 				       int (*okfn)(struct sk_buff *))
 {
 	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
-	struct net_device *realoutdev = bridge_parent(skb->dev);
+	struct net_bridge *br = bridge_parent(skb->dev);
+	struct net_device *realoutdev;
 	u_int8_t pf;
 
 	if (!nf_bridge || !(nf_bridge->mask & BRNF_BRIDGED))
 		return NF_ACCEPT;
 
-	if (!realoutdev)
+	if (!br)
 		return NF_DROP;
 
-	if (IS_IP(skb) || IS_VLAN_IP(skb) || IS_PPPOE_IP(skb))
+	realoutdev = br->dev;
+
+	if (IS_IP(skb) || IS_VLAN_IP(skb) || IS_PPPOE_IP(skb)) {
 		pf = NFPROTO_IPV4;
-	else if (IS_IPV6(skb) || IS_VLAN_IPV6(skb) || IS_PPPOE_IPV6(skb))
+		if (!brnf_call_iptables && !br->nf_call_iptables)
+			return NF_ACCEPT;
+	} else if (IS_IPV6(skb) || IS_VLAN_IPV6(skb) || IS_PPPOE_IPV6(skb)) {
 		pf = NFPROTO_IPV6;
-	else
+		if (!brnf_call_ip6tables && !br->nf_call_ip6tables)
+			return NF_ACCEPT;
+	} else
 		return NF_ACCEPT;
 
 	/* We assume any code from br_dev_queue_push_xmit onwards doesn't care

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

bridge: Save frag_max_size between PRE_ROUTING and POST_ROUTING

[Herbert Xu]

On Sun, Oct 05, 2014 at 11:53:43AM +0800, Herbert Xu wrote:
> 
> What we should do therefore is to save any necessary information
> such as frag_max_size into the bridge CB area when reentering the
> bridge and then copy it back upon the next reentry into the IP stack.

Here's a patch that does just that:

bridge: Save frag_max_size between PRE_ROUTING and POST_ROUTING

As we may defragment the packet in IPv4 PRE_ROUTING and refragment
it after POST_ROUTING we should save the value of frag_max_size.

This is still very wrong as the bridge is supposed to leave the
packets intact, meaning that the right thing to do is to use the
original frag_list for fragmentation.

Unfortunately we don't currently guarantee that the frag_list is
left untouched throughout netfilter so until this changes this is
the best we can do.

There is also a spot in FORWARD where it appears that we can
forward a packet without going through fragmentation, mark it
so that we can fix it later.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 389d1c6..47fe079 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -398,6 +398,7 @@ static int br_nf_pre_routing_finish_bridge(struct sk_buff *skb)
 							 ETH_HLEN-ETH_ALEN);
 			/* tell br_dev_xmit to continue with forwarding */
 			nf_bridge->mask |= BRNF_BRIDGED_DNAT;
+			/* FIXME Need to refragment */
 			ret = neigh->output(neigh, skb);
 		}
 		neigh_release(neigh);
@@ -453,6 +454,10 @@ static int br_nf_pre_routing_finish(struct sk_buff *skb)
 	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
 	struct rtable *rt;
 	int err;
+	int frag_max_size;
+
+	frag_max_size = IPCB(skb)->frag_max_size;
+	BR_INPUT_SKB_CB(skb)->frag_max_size = frag_max_size;
 
 	if (nf_bridge->mask & BRNF_PKT_TYPE) {
 		skb->pkt_type = PACKET_OTHERHOST;
@@ -864,13 +869,19 @@ static unsigned int br_nf_forward_arp(const struct nf_hook_ops *ops,
 static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 {
 	int ret;
+	int frag_max_size;
 
+	/* This is wrong! We should preserve the original fragment
+	 * boundaries by preserving frag_list rather than refragmenting.
+	 */
 	if (skb->protocol == htons(ETH_P_IP) &&
 	    skb->len + nf_bridge_mtu_reduction(skb) > skb->dev->mtu &&
 	    !skb_is_gso(skb)) {
+		frag_max_size = BR_INPUT_SKB_CB(skb)->frag_max_size;
 		if (br_parse_ip_options(skb))
 			/* Drop invalid packet */
 			return NF_DROP;
+		IPCB(skb)->frag_max_size = frag_max_size;
 		ret = ip_fragment(skb, br_dev_queue_push_xmit);
 	} else
 		ret = br_dev_queue_push_xmit(skb);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index b6c04cb..2398369 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -305,10 +305,14 @@ struct net_bridge
 
 struct br_input_skb_cb {
 	struct net_device *brdev;
+
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	int igmp;
 	int mrouters_only;
 #endif
+
+	u16 frag_max_size;
+
 #ifdef CONFIG_BRIDGE_VLAN_FILTERING
 	bool vlan_filtered;
 #endif

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: bridge: Save frag_max_size between PRE_ROUTING and POST_ROUTING

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sun, 5 Oct 2014 12:00:22 +0800

> bridge: Save frag_max_size between PRE_ROUTING and POST_ROUTING
> 
> As we may defragment the packet in IPv4 PRE_ROUTING and refragment
> it after POST_ROUTING we should save the value of frag_max_size.
> 
> This is still very wrong as the bridge is supposed to leave the
> packets intact, meaning that the right thing to do is to use the
> original frag_list for fragmentation.
> 
> Unfortunately we don't currently guarantee that the frag_list is
> left untouched throughout netfilter so until this changes this is
> the best we can do.
> 
> There is also a spot in FORWARD where it appears that we can
> forward a packet without going through fragmentation, mark it
> so that we can fix it later.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks Herbert.

Re: bridge: Respect call-iptables sysctls everywhere

[Florian Westphal]

Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Sat, Oct 04, 2014 at 08:06:47PM +0200, Florian Westphal wrote:
> >
> > Fair enough.  We lose frag_max_size information from ipv4 defrag,
> 
> While reviewing this code it occured to me that we have a serious
> bug in that call-iptables sysctls aren't even respected in FORWARD
> and POST_ROUTING.  Here is a patch that fixes this.

Upcalls to iptables in FORWARD/POSTROUTING depend on skb->nf_bridge
being set up, which only happens when call-iptables=1.

Re: bridge: Respect call-iptables sysctls everywhere

[Herbert Xu]

On Sun, Oct 05, 2014 at 11:13:43AM +0200, Florian Westphal wrote:
> Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > On Sat, Oct 04, 2014 at 08:06:47PM +0200, Florian Westphal wrote:
> > >
> > > Fair enough.  We lose frag_max_size information from ipv4 defrag,
> > 
> > While reviewing this code it occured to me that we have a serious
> > bug in that call-iptables sysctls aren't even respected in FORWARD
> > and POST_ROUTING.  Here is a patch that fixes this.
> 
> Upcalls to iptables in FORWARD/POSTROUTING depend on skb->nf_bridge
> being set up, which only happens when call-iptables=1.

Good point.  So we can discard this patch.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: bridge: Do not compile options in br_parse_ip_options

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 4 Oct 2014 22:18:02 +0800

> bridge: Do not compile options in br_parse_ip_options
> 
> Commit 462fb2af9788a82a534f8184abfde31574e1cfa0
> 
> 	bridge : Sanitize skb before it enters the IP stack
> 
> broke when IP options are actually used because it mangles the
> skb as if it entered the IP stack which is wrong because the
> bridge is supposed to operate below the IP stack.
> 
> Since nobody has actually requested for parsing of IP options
> this patch fixes it by simply reverting to the previous approach
> of ignoring all IP options, i.e., zeroing the IPCB.
> 
> If and when somebody who uses IP options and actually needs them
> to be parsed by the bridge complains then we can revisit this.
> 
> Reported-by: David Newall <davidn@davidnewall.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Agreed that mangling the packet is definitely wrong here, and since
you preserve the CB clearing this change should be fine.

Please submit this formally after it's been tested.

Thanks.

Re: bridge: Do not compile options in br_parse_ip_options

[Florian Westphal]

Herbert Xu <herbert@gondor.apana.org.au> wrote:
> bridge: Do not compile options in br_parse_ip_options
> 
> Commit 462fb2af9788a82a534f8184abfde31574e1cfa0
> 
> 	bridge : Sanitize skb before it enters the IP stack
> 
> broke when IP options are actually used because it mangles the
> skb as if it entered the IP stack which is wrong because the
> bridge is supposed to operate below the IP stack.
> 
> Since nobody has actually requested for parsing of IP options
> this patch fixes it by simply reverting to the previous approach
> of ignoring all IP options, i.e., zeroing the IPCB.
> 
> If and when somebody who uses IP options and actually needs them
> to be parsed by the bridge complains then we can revisit this.
> 
> Reported-by: David Newall <davidn@davidnewall.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Tested-by: Florian Westphal <fw@strlen.de>

Pablo, could you please apply this?

Thanks!

Re: bridge: Do not compile options in br_parse_ip_options

[Pablo Neira Ayuso]

On Fri, Oct 24, 2014 at 12:41:49PM +0200, Florian Westphal wrote:
> Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > bridge: Do not compile options in br_parse_ip_options
> > 
> > Commit 462fb2af9788a82a534f8184abfde31574e1cfa0
> > 
> > 	bridge : Sanitize skb before it enters the IP stack
> > 
> > broke when IP options are actually used because it mangles the
> > skb as if it entered the IP stack which is wrong because the
> > bridge is supposed to operate below the IP stack.
> > 
> > Since nobody has actually requested for parsing of IP options
> > this patch fixes it by simply reverting to the previous approach
> > of ignoring all IP options, i.e., zeroing the IPCB.
> > 
> > If and when somebody who uses IP options and actually needs them
> > to be parsed by the bridge complains then we can revisit this.
> > 
> > Reported-by: David Newall <davidn@davidnewall.com>
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Tested-by: Florian Westphal <fw@strlen.de>

Applied, thanks a lot for testing Florian.