[PATCH net] ipvs: SNAT packet replies only for NATed connections

[PATCH net] ipvs: SNAT packet replies only for NATed connections

[Julian Anastasov]

We do not check if packet from real server is for NAT
connection before performing SNAT. This causes problems
for setups that use DR/TUN and allow local clients to
access the real server directly, for example:

- local client in director creates IPVS-DR/TUN connection
CIP->VIP and the request packets are routed to RIP.
Talks are finished but IPVS connection is not expired yet.

- second local client creates non-IPVS connection CIP->RIP
with same reply tuple RIP->CIP and when replies are received
on LOCAL_IN we wrongly assign them for the first client
connection because RIP->CIP matches the reply direction.
As result, IPVS SNATs replies for non-IPVS connections.

The problem is more visible to local UDP clients but in rare
cases it can happen also for TCP or remote clients when the
real server sends the reply traffic via the director.

So, better to be more precise for the reply traffic.
As replies are not expected for DR/TUN connections, better
to not touch them.

Reported-by: Nick Moriarty <nick.moriarty@york.ac.uk>
Tested-by: Nick Moriarty <nick.moriarty@york.ac.uk>
Signed-off-by: Julian Anastasov <ja@ssi.bg>
---

I know that 4.11 is to be released soon, I prefer this patch
to linger in the net tree during the 4.12-rc cycle.

 net/netfilter/ipvs/ip_vs_core.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index db40050..ee44ed5 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -849,10 +849,8 @@ static int handle_response_icmp(int af, struct sk_buff *skb,
 {
 	unsigned int verdict = NF_DROP;
 
-	if (IP_VS_FWD_METHOD(cp) != 0) {
-		pr_err("shouldn't reach here, because the box is on the "
-		       "half connection in the tun/dr module.\n");
-	}
+	if (IP_VS_FWD_METHOD(cp) != IP_VS_CONN_F_MASQ)
+		goto ignore_cp;
 
 	/* Ensure the checksum is correct */
 	if (!skb_csum_unnecessary(skb) && ip_vs_checksum_complete(skb, ihl)) {
@@ -886,6 +884,8 @@ static int handle_response_icmp(int af, struct sk_buff *skb,
 		ip_vs_notrack(skb);
 	else
 		ip_vs_update_conntrack(skb, cp, 0);
+
+ignore_cp:
 	verdict = NF_ACCEPT;
 
 out:
@@ -1385,8 +1385,11 @@ ip_vs_out(struct netns_ipvs *ipvs, unsigned int hooknum, struct sk_buff *skb, in
 	 */
 	cp = pp->conn_out_get(ipvs, af, skb, &iph);
 
-	if (likely(cp))
+	if (likely(cp)) {
+		if (IP_VS_FWD_METHOD(cp) != IP_VS_CONN_F_MASQ)
+			goto ignore_cp;
 		return handle_response(af, skb, pd, cp, &iph, hooknum);
+	}
 
 	/* Check for real-server-started requests */
 	if (atomic_read(&ipvs->conn_out_counter)) {
@@ -1444,9 +1447,15 @@ ip_vs_out(struct netns_ipvs *ipvs, unsigned int hooknum, struct sk_buff *skb, in
 			}
 		}
 	}
+
+out:
 	IP_VS_DBG_PKT(12, af, pp, skb, iph.off,
 		      "ip_vs_out: packet continues traversal as normal");
 	return NF_ACCEPT;
+
+ignore_cp:
+	__ip_vs_conn_put(cp);
+	goto out;
 }
 
 /*
-- 
2.9.3

Re: [PATCH net] ipvs: SNAT packet replies only for NATed connections

[Simon Horman]

On Sat, Apr 29, 2017 at 08:33:09PM +0300, Julian Anastasov wrote:
> We do not check if packet from real server is for NAT
> connection before performing SNAT. This causes problems
> for setups that use DR/TUN and allow local clients to
> access the real server directly, for example:
> 
> - local client in director creates IPVS-DR/TUN connection
> CIP->VIP and the request packets are routed to RIP.
> Talks are finished but IPVS connection is not expired yet.
> 
> - second local client creates non-IPVS connection CIP->RIP
> with same reply tuple RIP->CIP and when replies are received
> on LOCAL_IN we wrongly assign them for the first client
> connection because RIP->CIP matches the reply direction.
> As result, IPVS SNATs replies for non-IPVS connections.
> 
> The problem is more visible to local UDP clients but in rare
> cases it can happen also for TCP or remote clients when the
> real server sends the reply traffic via the director.
> 
> So, better to be more precise for the reply traffic.
> As replies are not expected for DR/TUN connections, better
> to not touch them.
> 
> Reported-by: Nick Moriarty <nick.moriarty@york.ac.uk>
> Tested-by: Nick Moriarty <nick.moriarty@york.ac.uk>
> Signed-off-by: Julian Anastasov <ja@ssi.bg>
> ---
> 
> I know that 4.11 is to be released soon, I prefer this patch
> to linger in the net tree during the 4.12-rc cycle.

I have no problem with queueing this up as a fix for v4.12 as you describe
but do you also want it to be considered for -stable?

> 
>  net/netfilter/ipvs/ip_vs_core.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index db40050..ee44ed5 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -849,10 +849,8 @@ static int handle_response_icmp(int af, struct sk_buff *skb,
>  {
>  	unsigned int verdict = NF_DROP;
>  
> -	if (IP_VS_FWD_METHOD(cp) != 0) {
> -		pr_err("shouldn't reach here, because the box is on the "
> -		       "half connection in the tun/dr module.\n");
> -	}
> +	if (IP_VS_FWD_METHOD(cp) != IP_VS_CONN_F_MASQ)
> +		goto ignore_cp;
>  
>  	/* Ensure the checksum is correct */
>  	if (!skb_csum_unnecessary(skb) && ip_vs_checksum_complete(skb, ihl)) {
> @@ -886,6 +884,8 @@ static int handle_response_icmp(int af, struct sk_buff *skb,
>  		ip_vs_notrack(skb);
>  	else
>  		ip_vs_update_conntrack(skb, cp, 0);
> +
> +ignore_cp:
>  	verdict = NF_ACCEPT;
>  
>  out:
> @@ -1385,8 +1385,11 @@ ip_vs_out(struct netns_ipvs *ipvs, unsigned int hooknum, struct sk_buff *skb, in
>  	 */
>  	cp = pp->conn_out_get(ipvs, af, skb, &iph);
>  
> -	if (likely(cp))
> +	if (likely(cp)) {
> +		if (IP_VS_FWD_METHOD(cp) != IP_VS_CONN_F_MASQ)
> +			goto ignore_cp;
>  		return handle_response(af, skb, pd, cp, &iph, hooknum);
> +	}
>  
>  	/* Check for real-server-started requests */
>  	if (atomic_read(&ipvs->conn_out_counter)) {
> @@ -1444,9 +1447,15 @@ ip_vs_out(struct netns_ipvs *ipvs, unsigned int hooknum, struct sk_buff *skb, in
>  			}
>  		}
>  	}
> +
> +out:
>  	IP_VS_DBG_PKT(12, af, pp, skb, iph.off,
>  		      "ip_vs_out: packet continues traversal as normal");
>  	return NF_ACCEPT;
> +
> +ignore_cp:
> +	__ip_vs_conn_put(cp);
> +	goto out;
>  }
>  
>  /*
> -- 
> 2.9.3
> 

Re: [PATCH net] ipvs: SNAT packet replies only for NATed connections

[Julian Anastasov]

	Hello,

On Mon, 1 May 2017, Simon Horman wrote:

> On Sat, Apr 29, 2017 at 08:33:09PM +0300, Julian Anastasov wrote:
> > We do not check if packet from real server is for NAT
> > connection before performing SNAT. This causes problems
> > for setups that use DR/TUN and allow local clients to
> > access the real server directly, for example:
> > 
> > - local client in director creates IPVS-DR/TUN connection
> > CIP->VIP and the request packets are routed to RIP.
> > Talks are finished but IPVS connection is not expired yet.
> > 
> > - second local client creates non-IPVS connection CIP->RIP
> > with same reply tuple RIP->CIP and when replies are received
> > on LOCAL_IN we wrongly assign them for the first client
> > connection because RIP->CIP matches the reply direction.
> > As result, IPVS SNATs replies for non-IPVS connections.
> > 
> > The problem is more visible to local UDP clients but in rare
> > cases it can happen also for TCP or remote clients when the
> > real server sends the reply traffic via the director.
> > 
> > So, better to be more precise for the reply traffic.
> > As replies are not expected for DR/TUN connections, better
> > to not touch them.
> > 
> > Reported-by: Nick Moriarty <nick.moriarty@york.ac.uk>
> > Tested-by: Nick Moriarty <nick.moriarty@york.ac.uk>
> > Signed-off-by: Julian Anastasov <ja@ssi.bg>
> > ---
> > 
> > I know that 4.11 is to be released soon, I prefer this patch
> > to linger in the net tree during the 4.12-rc cycle.
> 
> I have no problem with queueing this up as a fix for v4.12 as you describe
> but do you also want it to be considered for -stable?

	Yes, I'll post patches for stable kernels later today.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH net] ipvs: SNAT packet replies only for NATed connections

[Simon Horman]

On Mon, May 01, 2017 at 11:28:59AM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Mon, 1 May 2017, Simon Horman wrote:
> 
> > On Sat, Apr 29, 2017 at 08:33:09PM +0300, Julian Anastasov wrote:
> > > We do not check if packet from real server is for NAT
> > > connection before performing SNAT. This causes problems
> > > for setups that use DR/TUN and allow local clients to
> > > access the real server directly, for example:
> > > 
> > > - local client in director creates IPVS-DR/TUN connection
> > > CIP->VIP and the request packets are routed to RIP.
> > > Talks are finished but IPVS connection is not expired yet.
> > > 
> > > - second local client creates non-IPVS connection CIP->RIP
> > > with same reply tuple RIP->CIP and when replies are received
> > > on LOCAL_IN we wrongly assign them for the first client
> > > connection because RIP->CIP matches the reply direction.
> > > As result, IPVS SNATs replies for non-IPVS connections.
> > > 
> > > The problem is more visible to local UDP clients but in rare
> > > cases it can happen also for TCP or remote clients when the
> > > real server sends the reply traffic via the director.
> > > 
> > > So, better to be more precise for the reply traffic.
> > > As replies are not expected for DR/TUN connections, better
> > > to not touch them.
> > > 
> > > Reported-by: Nick Moriarty <nick.moriarty@york.ac.uk>
> > > Tested-by: Nick Moriarty <nick.moriarty@york.ac.uk>
> > > Signed-off-by: Julian Anastasov <ja@ssi.bg>
> > > ---
> > > 
> > > I know that 4.11 is to be released soon, I prefer this patch
> > > to linger in the net tree during the 4.12-rc cycle.
> > 
> > I have no problem with queueing this up as a fix for v4.12 as you describe
> > but do you also want it to be considered for -stable?
> 
> 	Yes, I'll post patches for stable kernels later today.

Thanks, I have pushed this patch to the ipvs tree.