[PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device

[PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device

[Linus Lüssing]

When trying to redirect bridged frames to the bridge device itself
via the ebtables nat-prerouting chain and the dnat target then this
currently fails:

The ethernet destination of the frame is dnat'ed to the MAC address of
the bridge itself just fine and the correctly altered frame can even
be captured via a tcpdump on br0 (with or without promisc mode).

However, the IP code drops it in the beginning of ip_input.c/ip_rcv()
as the dnat target did not update the skb->pkt_type. If after
dnat'ing the packet is now destined to us then the skb->pkt_type
needs to be updated from PACKET_OTHERHOST to PACKET_HOST, too.

Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
 net/bridge/br_input.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 013f2290b..ec83175 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -198,8 +198,12 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	if (dst) {
 		unsigned long now = jiffies;
 
-		if (dst->is_local)
+		if (dst->is_local) {
+			/* fix up potential DNAT mess */
+			skb->pkt_type = PACKET_HOST;
+
 			return br_pass_frame_up(skb);
+		}
 
 		if (now != dst->used)
 			dst->used = now;
-- 
2.1.4

Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device

[Florian Westphal]

Linus Lüssing <linus.luessing@c0d3.blue> wrote:
> When trying to redirect bridged frames to the bridge device itself
> via the ebtables nat-prerouting chain and the dnat target then this
> currently fails:
> 
> The ethernet destination of the frame is dnat'ed to the MAC address of
> the bridge itself just fine and the correctly altered frame can even
> be captured via a tcpdump on br0 (with or without promisc mode).
>
> However, the IP code drops it in the beginning of ip_input.c/ip_rcv()
> as the dnat target did not update the skb->pkt_type.

Right, thats the reason why ebtables also has ebt_redirect target
which does this pkt_type fixup.

> -		if (dst->is_local)
> +		if (dst->is_local) {
> +			/* fix up potential DNAT mess */
> +			skb->pkt_type = PACKET_HOST;
> +
>  			return br_pass_frame_up(skb);
> +		}

I don't mind this change though (i.e. I don't see how this would
bite us later).

Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device

[Pablo Neira Ayuso]

On Wed, Mar 15, 2017 at 11:26:08AM +0100, Florian Westphal wrote:
> Linus Lüssing <linus.luessing@c0d3.blue> wrote:
> > When trying to redirect bridged frames to the bridge device itself
> > via the ebtables nat-prerouting chain and the dnat target then this
> > currently fails:
> > 
> > The ethernet destination of the frame is dnat'ed to the MAC address of
> > the bridge itself just fine and the correctly altered frame can even
> > be captured via a tcpdump on br0 (with or without promisc mode).
> >
> > However, the IP code drops it in the beginning of ip_input.c/ip_rcv()
> > as the dnat target did not update the skb->pkt_type.
> 
> Right, thats the reason why ebtables also has ebt_redirect target
> which does this pkt_type fixup.

I'm missing then why redirect is not then just enough for Linus usecase.

Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device

[Linus Lüssing]

On Wed, Mar 15, 2017 at 11:42:11AM +0100, Pablo Neira Ayuso wrote:
> I'm missing then why redirect is not then just enough for Linus usecase.

For my usecase, the MAC address is configured by the user from a
Web-UI. It may or may not be the one from the bridge device.

Besides, found it counter intuitive that DNAT did not work here
and took me some time to find out why. At least I didn't read about
any such known limitations of the dnat target in the ebtables
manpage.

Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device

[Pablo Neira Ayuso]

On Wed, Mar 15, 2017 at 03:27:20PM +0100, Linus Lüssing wrote:
> On Wed, Mar 15, 2017 at 11:42:11AM +0100, Pablo Neira Ayuso wrote:
> > I'm missing then why redirect is not then just enough for Linus usecase.
> 
> For my usecase, the MAC address is configured by the user from a
> Web-UI. It may or may not be the one from the bridge device.
> 
> Besides, found it counter intuitive that DNAT did not work here
> and took me some time to find out why. At least I didn't read about
> any such known limitations of the dnat target in the ebtables
> manpage.

Could you update ebtables dnat to check if the ethernet address
matches the one of the input bridge interface, so we mangle the
->pkt_type accordingly from there, instead of doing this from the
core?

Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device

[Linus Lüssing]

On Wed, Mar 15, 2017 at 07:15:39PM +0100, Pablo Neira Ayuso wrote:
> Could you update ebtables dnat to check if the ethernet address
> matches the one of the input bridge interface, so we mangle the
> ->pkt_type accordingly from there, instead of doing this from the
> core?

Actually, that was the approach I thought about and went for first
(and it would probably work for me). Just checking against the
bridge device's net_device::dev_addr.

I scratched it though, as I was afraid that the issue might still
exist for people using some other upper device on top of the bridge
device. For instance, macvlan? And iterating over the
net_device::dev_addrs list seemed too costly for fast path to me.

Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device

[Pablo Neira Ayuso]

On Wed, Mar 15, 2017 at 10:16:19PM +0100, Linus Lüssing wrote:
> On Wed, Mar 15, 2017 at 07:15:39PM +0100, Pablo Neira Ayuso wrote:
> > Could you update ebtables dnat to check if the ethernet address
> > matches the one of the input bridge interface, so we mangle the
> > ->pkt_type accordingly from there, instead of doing this from the
> > core?
> 
> Actually, that was the approach I thought about and went for first
> (and it would probably work for me). Just checking against the
> bridge device's net_device::dev_addr.
> 
> I scratched it though, as I was afraid that the issue might still
> exist for people using some other upper device on top of the bridge
> device. For instance, macvlan? And iterating over the
> net_device::dev_addrs list seemed too costly for fast path to me.

I was more thinking of following the simple approach that we follow in
ebt_redirect_tg() by taking the input interface.

Anyway, I'm ok with this.

Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device

[Pablo Neira Ayuso]

On Wed, Mar 15, 2017 at 11:06:05PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Mar 15, 2017 at 10:16:19PM +0100, Linus Lüssing wrote:
> > On Wed, Mar 15, 2017 at 07:15:39PM +0100, Pablo Neira Ayuso wrote:
> > > Could you update ebtables dnat to check if the ethernet address
> > > matches the one of the input bridge interface, so we mangle the
> > > ->pkt_type accordingly from there, instead of doing this from the
> > > core?
> > 
> > Actually, that was the approach I thought about and went for first
> > (and it would probably work for me). Just checking against the
> > bridge device's net_device::dev_addr.
> > 
> > I scratched it though, as I was afraid that the issue might still
> > exist for people using some other upper device on top of the bridge
> > device. For instance, macvlan? And iterating over the
> > net_device::dev_addrs list seemed too costly for fast path to me.
> 
> I was more thinking of following the simple approach that we follow in
> ebt_redirect_tg() by taking the input interface.
> 
> Anyway, I'm ok with this.

Wait.

May this break local multicast listener that are bound to the bridge
interface? Assuming the bridge interface got an IP address, and that
there is local multicast listener.

Missing anything here?

Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device

[Linus Lüssing]

On Fri, Mar 17, 2017 at 02:10:44PM +0100, Pablo Neira Ayuso wrote:
> Wait.
> 
> May this break local multicast listener that are bound to the bridge
> interface? Assuming the bridge interface got an IP address, and that
> there is local multicast listener.
> 
> Missing anything here?

Hm, for multicast packets usually the code path a few lines
later in br_handle_frame_finish() should be taken instead.

But you might be right for IP multicast packets with a unicast MAC
destination (due to whatever reason, for instance via DNAT'ing
again).

Will check that - thanks!

Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device

[Linus Lüssing]

On Sun, Mar 19, 2017 at 05:55:06PM +0100, Linus Lüssing wrote:
> On Fri, Mar 17, 2017 at 02:10:44PM +0100, Pablo Neira Ayuso wrote:
> > Wait.
> > 
> > May this break local multicast listener that are bound to the bridge
> > interface? Assuming the bridge interface got an IP address, and that
> > there is local multicast listener.
> > 
> > Missing anything here?
> 
> Hm, for multicast packets usually the code path a few lines
> later in br_handle_frame_finish() should be taken instead.
> 
> But you might be right for IP multicast packets with a unicast MAC
> destination (due to whatever reason, for instance via DNAT'ing
> again).
> 
> Will check that - thanks!

Ok, I tested DNAT'ing an IP multicast packet to the unicast MAC address
of the bridge interface.

Both ping-ing to an IPv4 and IPv6 multicast listener on br0 worked
and was replied to fine, both with or without changing skb->pkt_type
from PACKET_MULTICAST to PACKET_HOST.
("$ ping 224.1.0.123" and "$ ping6 ff02::1:ff40:707c%in0" from a
 network namespace, tied into the bridge via veth)

Also, a DNAT'ed PACKET_BROADCAST worked, with or without changing
it to PACKET_HOST.

I also checked via tcpdump that the destination MAC was changed
successfully.


So, so far I wasn't able to find any bugs with the current
patch. But I think I like the idea of leaving the skb->pkt_type
unaltered for PACKET_MULTICAST and PACKET_BROADCAST, seems cleaner.

I'd just add an "if (skb->pkt_type == PACKET_OTHERHOST)" check
then and resend a PATCH v2.

Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device

[Pablo Neira Ayuso]

On Tue, Mar 21, 2017 at 01:09:47AM +0100, Linus Lüssing wrote:
> On Sun, Mar 19, 2017 at 05:55:06PM +0100, Linus Lüssing wrote:
> > On Fri, Mar 17, 2017 at 02:10:44PM +0100, Pablo Neira Ayuso wrote:
> > > Wait.
> > > 
> > > May this break local multicast listener that are bound to the bridge
> > > interface? Assuming the bridge interface got an IP address, and that
> > > there is local multicast listener.
> > > 
> > > Missing anything here?
> > 
> > Hm, for multicast packets usually the code path a few lines
> > later in br_handle_frame_finish() should be taken instead.
> > 
> > But you might be right for IP multicast packets with a unicast MAC
> > destination (due to whatever reason, for instance via DNAT'ing
> > again).
> > 
> > Will check that - thanks!
> 
> Ok, I tested DNAT'ing an IP multicast packet to the unicast MAC address
> of the bridge interface.
> 
> Both ping-ing to an IPv4 and IPv6 multicast listener on br0 worked
> and was replied to fine, both with or without changing skb->pkt_type
> from PACKET_MULTICAST to PACKET_HOST.
> ("$ ping 224.1.0.123" and "$ ping6 ff02::1:ff40:707c%in0" from a
>  network namespace, tied into the bridge via veth)
> 
> Also, a DNAT'ed PACKET_BROADCAST worked, with or without changing
> it to PACKET_HOST.
> 
> I also checked via tcpdump that the destination MAC was changed
> successfully.

Thanks for looking into this more in depth.

> So, so far I wasn't able to find any bugs with the current
> patch. But I think I like the idea of leaving the skb->pkt_type
> unaltered for PACKET_MULTICAST and PACKET_BROADCAST, seems cleaner.

Yes, and matching on skb->pkt_type would not break from netfilter.

> I'd just add an "if (skb->pkt_type == PACKET_OTHERHOST)" check
> then and resend a PATCH v2.

Thanks.

Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device

[Pablo Neira Ayuso]

On Wed, Mar 15, 2017 at 04:18:11AM +0100, Linus Lüssing wrote:
> When trying to redirect bridged frames to the bridge device itself
> via the ebtables nat-prerouting chain and the dnat target then this
> currently fails:
> 
> The ethernet destination of the frame is dnat'ed to the MAC address of
> the bridge itself just fine and the correctly altered frame can even
> be captured via a tcpdump on br0 (with or without promisc mode).
> 
> However, the IP code drops it in the beginning of ip_input.c/ip_rcv()
> as the dnat target did not update the skb->pkt_type. If after
> dnat'ing the packet is now destined to us then the skb->pkt_type
> needs to be updated from PACKET_OTHERHOST to PACKET_HOST, too.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
> ---
>  net/bridge/br_input.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 013f2290b..ec83175 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -198,8 +198,12 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  	if (dst) {
>  		unsigned long now = jiffies;
>  
> -		if (dst->is_local)
> +		if (dst->is_local) {
> +			/* fix up potential DNAT mess */
> +			skb->pkt_type = PACKET_HOST;

I would like to find a way to fix this from ebtables itself, so we
don't need to add this code to the bridge core path. AFAICS, from
prerouting we don't know the dst yet, so we cannot know if this packet
is local from there.