diff mbox

[OpenWrt-Devel,firewall] zones : Redirect incoming WAN traffic only when the destination IP address matches the IP masquerading address

Message ID 1443717512-15116-1-git-send-email-dedeckeh@gmail.com
State Changes Requested
Headers show

Commit Message

Hans Dedecker Oct. 1, 2015, 4:38 p.m. UTC
This patch fixes an issue when 2 LAN network prefixes are in use :
 - the usual 192.168.0.0/24 which is masqueraded by the public IP address on the
   WAN interface
 - a public IP network prefix for those LAN devices that are excluded from NAT

Port forwarding rules introduced for 192.168.1.x devices will currently also
translate traffic addressed to the public network addresses in use on the LAN
as the destination address in the delegate prerouting rule(s) is unset.
The patch sets the destination IP address(es) in the delegate prerouting rules
equal to the IP address(es) that particular network interface has as extra descriminator

Signed-off-by: Hans Dedecker <dedeckeh@gmail.com>
Signed-off-by: Alin Nastac <alin.nastac@gmail.com>
---
 zones.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

Comments

Jo-Philipp Wich Oct. 1, 2015, 8:05 p.m. UTC | #1
Hi,

wouldn't this break port forwards to hosts not being within the range of
the on-link lan subnet?

I also read the patch description three times and still am not sure what
that change attempts to achive.

Can you further explain the problem please and provide a before/after
"fw3 print" diff so that I better understand your proposed solution?

~ Jow


Am 01.10.2015 um 18:38 schrieb Hans Dedecker:
> This patch fixes an issue when 2 LAN network prefixes are in use :
>  - the usual 192.168.0.0/24 which is masqueraded by the public IP address on the
>    WAN interface
>  - a public IP network prefix for those LAN devices that are excluded from NAT
> 
> Port forwarding rules introduced for 192.168.1.x devices will currently also
> translate traffic addressed to the public network addresses in use on the LAN
> as the destination address in the delegate prerouting rule(s) is unset.
> The patch sets the destination IP address(es) in the delegate prerouting rules
> equal to the IP address(es) that particular network interface has as extra descriminator
> 
> Signed-off-by: Hans Dedecker <dedeckeh@gmail.com>
> Signed-off-by: Alin Nastac <alin.nastac@gmail.com>
> ---
>  zones.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/zones.c b/zones.c
> index 2ddd7b4..8bd6673 100644
> --- a/zones.c
> +++ b/zones.c
> @@ -383,10 +383,38 @@ print_interface_rule(struct fw3_ipt_handle *handle, struct fw3_state *state,
>  	{
>  		if (has(zone->flags, handle->family, FW3_FLAG_DNAT))
>  		{
> -			r = fw3_ipt_rule_create(handle, NULL, dev, NULL, sub, NULL);
> -			fw3_ipt_rule_target(r, "zone_%s_prerouting", zone->name);
> -			fw3_ipt_rule_extra(r, zone->extra_src);
> -			fw3_ipt_rule_replace(r, "delegate_prerouting");
> +			struct list_head *addrs;
> +			struct fw3_address *addr;
> +
> +			addrs = zone->masq ? calloc(1, sizeof(*addrs)) : NULL;
> +			if (addrs)
> +			{
> +				/* redirect only the traffic towards a locally configured address */
> +				INIT_LIST_HEAD(addrs);
> +				fw3_ubus_address(addrs, dev->network);
> +
> +				list_for_each_entry(addr, addrs, list)
> +				{
> +					if (!fw3_is_family(addr, handle->family))
> +						continue;
> +					/* reset mask to its maximum value */
> +					memset(&addr->mask.v6, 0xFF, sizeof(addr->mask.v6));
> +
> +					r = fw3_ipt_rule_create(handle, NULL, dev, NULL, sub, addr);
> +					fw3_ipt_rule_target(r, "zone_%s_prerouting", zone->name);
> +					fw3_ipt_rule_extra(r, zone->extra_src);
> +					fw3_ipt_rule_replace(r, "delegate_prerouting");
> +				}
> +
> +				fw3_free_list(addrs);
> +			}
> +			else
> +			{
> +				r = fw3_ipt_rule_create(handle, NULL, dev, NULL, sub, NULL);
> +				fw3_ipt_rule_target(r, "zone_%s_prerouting", zone->name);
> +				fw3_ipt_rule_extra(r, zone->extra_src);
> +				fw3_ipt_rule_replace(r, "delegate_prerouting");
> +			}
>  		}
>  
>  		if (has(zone->flags, handle->family, FW3_FLAG_SNAT))
>
Alin Năstac Oct. 5, 2015, 7:52 a.m. UTC | #2
Here is the original description I gave to my patch (see
http://patchwork.ozlabs.org/patch/516167/):

Basically it prevents zone_wan_prerouting rules to affect traffic towards
IP addresses that are not used
for masquerading LAN private IP space and it does that by setting
destination IP address of the
delegate_prerouting rules for zone with masq enabled to whatever
address(es) that particular network
interface has.

The typical scenario this patch fixes involves 2 LAN network prefixes:
  - the usual 192.168.1.0/24 which is masqueraded by the public IP address
configured on the WAN interface
  - a public IP network prefix for those LAN devices that are supposed to
be excluded from NAT
Without this patch, port forwarding rules introduced for 192.168.1.x LAN
devices will also affect traffic
towards the 2nd prefix.


On Thu, Oct 1, 2015 at 10:05 PM, Jo-Philipp Wich <jow@openwrt.org> wrote:

> Hi,
>
> wouldn't this break port forwards to hosts not being within the range of
> the on-link lan subnet?
>
> I also read the patch description three times and still am not sure what
> that change attempts to achive.
>
> Can you further explain the problem please and provide a before/after
> "fw3 print" diff so that I better understand your proposed solution?
>
> ~ Jow
>
>
> Am 01.10.2015 um 18:38 schrieb Hans Dedecker:
> > This patch fixes an issue when 2 LAN network prefixes are in use :
> >  - the usual 192.168.0.0/24 which is masqueraded by the public IP
> address on the
> >    WAN interface
> >  - a public IP network prefix for those LAN devices that are excluded
> from NAT
> >
> > Port forwarding rules introduced for 192.168.1.x devices will currently
> also
> > translate traffic addressed to the public network addresses in use on
> the LAN
> > as the destination address in the delegate prerouting rule(s) is unset.
> > The patch sets the destination IP address(es) in the delegate prerouting
> rules
> > equal to the IP address(es) that particular network interface has as
> extra descriminator
> >
> > Signed-off-by: Hans Dedecker <dedeckeh@gmail.com>
> > Signed-off-by: Alin Nastac <alin.nastac@gmail.com>
> > ---
> >  zones.c | 36 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 32 insertions(+), 4 deletions(-)
> >
> > diff --git a/zones.c b/zones.c
> > index 2ddd7b4..8bd6673 100644
> > --- a/zones.c
> > +++ b/zones.c
> > @@ -383,10 +383,38 @@ print_interface_rule(struct fw3_ipt_handle
> *handle, struct fw3_state *state,
> >       {
> >               if (has(zone->flags, handle->family, FW3_FLAG_DNAT))
> >               {
> > -                     r = fw3_ipt_rule_create(handle, NULL, dev, NULL,
> sub, NULL);
> > -                     fw3_ipt_rule_target(r, "zone_%s_prerouting",
> zone->name);
> > -                     fw3_ipt_rule_extra(r, zone->extra_src);
> > -                     fw3_ipt_rule_replace(r, "delegate_prerouting");
> > +                     struct list_head *addrs;
> > +                     struct fw3_address *addr;
> > +
> > +                     addrs = zone->masq ? calloc(1, sizeof(*addrs)) :
> NULL;
> > +                     if (addrs)
> > +                     {
> > +                             /* redirect only the traffic towards a
> locally configured address */
> > +                             INIT_LIST_HEAD(addrs);
> > +                             fw3_ubus_address(addrs, dev->network);
> > +
> > +                             list_for_each_entry(addr, addrs, list)
> > +                             {
> > +                                     if (!fw3_is_family(addr,
> handle->family))
> > +                                             continue;
> > +                                     /* reset mask to its maximum value
> */
> > +                                     memset(&addr->mask.v6, 0xFF,
> sizeof(addr->mask.v6));
> > +
> > +                                     r = fw3_ipt_rule_create(handle,
> NULL, dev, NULL, sub, addr);
> > +                                     fw3_ipt_rule_target(r,
> "zone_%s_prerouting", zone->name);
> > +                                     fw3_ipt_rule_extra(r,
> zone->extra_src);
> > +                                     fw3_ipt_rule_replace(r,
> "delegate_prerouting");
> > +                             }
> > +
> > +                             fw3_free_list(addrs);
> > +                     }
> > +                     else
> > +                     {
> > +                             r = fw3_ipt_rule_create(handle, NULL, dev,
> NULL, sub, NULL);
> > +                             fw3_ipt_rule_target(r,
> "zone_%s_prerouting", zone->name);
> > +                             fw3_ipt_rule_extra(r, zone->extra_src);
> > +                             fw3_ipt_rule_replace(r,
> "delegate_prerouting");
> > +                     }
> >               }
> >
> >               if (has(zone->flags, handle->family, FW3_FLAG_SNAT))
> >
>
>
Hans Dedecker Oct. 6, 2015, 11:12 a.m. UTC | #3
Hi,

The problem occurs in the following scenario where two hosts A and B are in
use on the lan and connected to a router which is doing masquerade on the
wan link.
Host A has a private IP@ (eg 192.168.1.10/24) while host B has a public IP@
(eg 172.18.16.240/24); the router has a public IP@ on the wan in the same
subnet as host B (eg 172.18.16.245/24).
A redirect rule is defined on the router to forward tcp service 8080 to
host A port 80 and translates into following iptables nat rules :

Chain delegate_prerouting (1 references)
 pkts bytes target     prot opt in     out     source
destination
    2    68 prerouting_rule  all  --  *      *       0.0.0.0/0
0.0.0.0/0            /* user chain for prerouting */
    1    32 zone_lan_prerouting  all  --  br-lan *       0.0.0.0/0
   0.0.0.0/0
    0     0 zone_wan_prerouting  all  --  pppoe-wan *       0.0.0.0/0
     0.0.0.0/0

chain zone_wan_prerouting (1 references)
 pkts bytes target     prot opt in     out     source
destination
    0     0 prerouting_wan_rule  all  --  *      *       0.0.0.0/0
   0.0.0.0/0            /* user chain for prerouting */
    0     0 DNAT       tcp  --  *      *       0.0.0.0/0
0.0.0.0/0            tcp spt:8080 /* @redirect[0] */ to:192.168.1.10:80


TCP traffic on pppoe-wan interface with as destination port 172.18.16.245
will be redirected to 192.168.1.10 as expected but if host B runs a similar
tcp service on port 8080 it will be unreachable as traffic directed to
172.18.16.240 will also be re-directed to 192.168.1.10.
The patch tries to fix this issue by using the wan IP address in the
zone_wan_prerouting lookup; in this case traffic destined for 172.18.16.240
will not be redirected.
Chain delegate_prerouting (1 references)
 pkts bytes target     prot opt in     out     source
destination
    1    87 prerouting_rule  all  --  *      *       0.0.0.0/0
0.0.0.0/0            /* user chain for prerouting */
    1    87 zone_lan_prerouting  all  --  br-lan *       0.0.0.0/0
   0.0.0.0/0
    0     0 zone_wan_prerouting  all  --  pppoe-wan *       0.0.0.0/0
     172.18.16.245

Chain zone_wan_prerouting (1 references)
 pkts bytes target     prot opt in     out     source
destination
    0     0 prerouting_wan_rule  all  --  *      *       0.0.0.0/0
   0.0.0.0/0            /* user chain for prerouting */
    0     0 DNAT       tcp  --  *      *       0.0.0.0/0
0.0.0.0/0            tcp spt:8080 /* @redirect[0] */ to:192.168.1.10:80

Output of  fw3 print diff before/after the patch

< iptables -t nat -D delegate_prerouting -i pppoe-wan -j zone_wan_prerouting
< iptables -t nat -A delegate_prerouting -i pppoe-wan -j zone_wan_prerouting
---
> iptables -t nat -D delegate_prerouting -i pppoe-wan -d
172.18.16.245/255.255.255.255 -j zone_wan_prerouting
> iptables -t nat -A delegate_prerouting -i pppoe-wan -d
172.18.16.245/255.255.255.255 -j zone_wan_prerouting

Bye,
Hans

On Thu, Oct 1, 2015 at 10:05 PM, Jo-Philipp Wich <jow@openwrt.org> wrote:

> Hi,
>
> wouldn't this break port forwards to hosts not being within the range of
> the on-link lan subnet?
>
> I also read the patch description three times and still am not sure what
> that change attempts to achive.
>
> Can you further explain the problem please and provide a before/after
> "fw3 print" diff so that I better understand your proposed solution?


> ~ Jow
>
>
diff mbox

Patch

diff --git a/zones.c b/zones.c
index 2ddd7b4..8bd6673 100644
--- a/zones.c
+++ b/zones.c
@@ -383,10 +383,38 @@  print_interface_rule(struct fw3_ipt_handle *handle, struct fw3_state *state,
 	{
 		if (has(zone->flags, handle->family, FW3_FLAG_DNAT))
 		{
-			r = fw3_ipt_rule_create(handle, NULL, dev, NULL, sub, NULL);
-			fw3_ipt_rule_target(r, "zone_%s_prerouting", zone->name);
-			fw3_ipt_rule_extra(r, zone->extra_src);
-			fw3_ipt_rule_replace(r, "delegate_prerouting");
+			struct list_head *addrs;
+			struct fw3_address *addr;
+
+			addrs = zone->masq ? calloc(1, sizeof(*addrs)) : NULL;
+			if (addrs)
+			{
+				/* redirect only the traffic towards a locally configured address */
+				INIT_LIST_HEAD(addrs);
+				fw3_ubus_address(addrs, dev->network);
+
+				list_for_each_entry(addr, addrs, list)
+				{
+					if (!fw3_is_family(addr, handle->family))
+						continue;
+					/* reset mask to its maximum value */
+					memset(&addr->mask.v6, 0xFF, sizeof(addr->mask.v6));
+
+					r = fw3_ipt_rule_create(handle, NULL, dev, NULL, sub, addr);
+					fw3_ipt_rule_target(r, "zone_%s_prerouting", zone->name);
+					fw3_ipt_rule_extra(r, zone->extra_src);
+					fw3_ipt_rule_replace(r, "delegate_prerouting");
+				}
+
+				fw3_free_list(addrs);
+			}
+			else
+			{
+				r = fw3_ipt_rule_create(handle, NULL, dev, NULL, sub, NULL);
+				fw3_ipt_rule_target(r, "zone_%s_prerouting", zone->name);
+				fw3_ipt_rule_extra(r, zone->extra_src);
+				fw3_ipt_rule_replace(r, "delegate_prerouting");
+			}
 		}
 
 		if (has(zone->flags, handle->family, FW3_FLAG_SNAT))