[PATCH nf] netfilter: xt_nat: allow IP_CT_RELATED_REPLY in DNAT targets

[PATCH nf] netfilter: xt_nat: allow IP_CT_RELATED_REPLY in DNAT targets

[xujiahao]

The DNAT targets warn when the conntrack info is neither IP_CT_NEW nor
IP_CT_RELATED, while the corresponding SNAT targets additionally accept
IP_CT_RELATED_REPLY:

  WARN_ON(!(ct != NULL &&
           (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED)));

A TCP reset generated by nf_send_reset() on the reject path is attached
to the conntrack entry of the rejected packet and marked
IP_CT_RELATED_REPLY.  It traverses the LOCAL_OUT NAT hook, where
nf_nat_inet_fn() lets IP_CT_RELATED_REPLY packets whose destination NAT
mapping is not initialized yet run the OUTPUT NAT chain.  When that
chain holds an xtables DNAT target, the target is reached with
IP_CT_RELATED_REPLY and warns:

  WARNING: net/netfilter/xt_nat.c:154 at xt_dnat_target_v2+0x97/0xc0

The same packet is accepted by the SNAT side, so the assertion is
incomplete rather than the packet being unexpected.  Align the DNAT
targets with the SNAT targets.

Reported-by: syzbot+2e819f6641708bb6ab9d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2e819f6641708bb6ab9d
Signed-off-by: xujiahao <xujiahao@cmss.chinamobile.com>
---
 net/netfilter/xt_nat.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/xt_nat.c b/net/netfilter/xt_nat.c
index 51c7f7ce8..e565fb341 100644
--- a/net/netfilter/xt_nat.c
+++ b/net/netfilter/xt_nat.c
@@ -84,7 +84,8 @@ xt_dnat_target_v0(struct sk_buff *skb, const struct xt_action_param *par)
 
 	ct = nf_ct_get(skb, &ctinfo);
 	WARN_ON(!(ct != NULL &&
-		 (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED)));
+		 (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED ||
+		  ctinfo == IP_CT_RELATED_REPLY)));
 
 	xt_nat_convert_range(&range, &mr->range[0]);
 	return nf_nat_setup_info(ct, &range, NF_NAT_MANIP_DST);
@@ -119,7 +120,8 @@ xt_dnat_target_v1(struct sk_buff *skb, const struct xt_action_param *par)
 
 	ct = nf_ct_get(skb, &ctinfo);
 	WARN_ON(!(ct != NULL &&
-		 (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED)));
+		 (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED ||
+		  ctinfo == IP_CT_RELATED_REPLY)));
 
 	memcpy(&range, range_v1, sizeof(*range_v1));
 	memset(&range.base_proto, 0, sizeof(range.base_proto));
@@ -151,7 +153,8 @@ xt_dnat_target_v2(struct sk_buff *skb, const struct xt_action_param *par)
 
 	ct = nf_ct_get(skb, &ctinfo);
 	WARN_ON(!(ct != NULL &&
-		 (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED)));
+		 (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED ||
+		  ctinfo == IP_CT_RELATED_REPLY)));
 
 	return nf_nat_setup_info(ct, range, NF_NAT_MANIP_DST);
 }
-- 
2.43.0

Re: [PATCH nf] netfilter: xt_nat: allow IP_CT_RELATED_REPLY in DNAT targets

[Florian Westphal]

xujiahao <xujiahao@cmss.chinamobile.com> wrote:
> The DNAT targets warn when the conntrack info is neither IP_CT_NEW nor
> IP_CT_RELATED, while the corresponding SNAT targets additionally accept
> IP_CT_RELATED_REPLY:
> 
>   WARN_ON(!(ct != NULL &&
>            (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED)));
> 
> A TCP reset generated by nf_send_reset() on the reject path is attached
> to the conntrack entry of the rejected packet and marked
> IP_CT_RELATED_REPLY.

I don't think thats correct.  nf_conntrack_attach() assumes ICMP.

TCP should just set IP_CT_ESTABLISHED, we're 'emulating' a reset
packet coming from peer, so we should not create a fresh/new nat
rewrite.

I think this should be fixed in nf_conntrack_attach() to check for
TCP and then set IP_CT_ESTABLISHED state for nskb.

> It traverses the LOCAL_OUT NAT hook, where
> nf_nat_inet_fn() lets IP_CT_RELATED_REPLY packets whose destination NAT
> mapping is not initialized yet run the OUTPUT NAT chain.  When that
> chain holds an xtables DNAT target, the target is reached with
> IP_CT_RELATED_REPLY and warns:
> 
>   WARNING: net/netfilter/xt_nat.c:154 at xt_dnat_target_v2+0x97/0xc0

This warning is correct. We should not do a DNAT mapping for
IP_CT_RELATED_REPLY on TCP, it would cause packet rewrite.

The IP_CT_RELATED_REPLY warning is old, from a time when DNAT was
restricted to PRE_ROUTING hooks, which is likely why its only
in the SNAT handler (which was restricted to POST_ROUTING only).

[PATCH nf v2] netfilter: conntrack: attach generated TCP resets as ESTABLISHED

[xujiahao]

nf_conntrack_attach() marks every packet it attaches as related to the
conntrack entry of the packet which caused it: IP_CT_RELATED_REPLY when
that packet was in the original direction, IP_CT_RELATED otherwise.

That is right for the ICMP errors built by icmp.c and icmpv6.c, but not
for the TCP resets built by nf_send_reset(): a reset is a packet of the
connection itself and carries the state of the connection, just like the
equivalent reset the TCP stack emits.

A locally generated reset therefore currently leaves through the
LOCAL_OUT nat hook as IP_CT_RELATED_REPLY.  As no nat mapping is
initialized for it, nf_nat_inet_fn() evaluates the nat OUTPUT chain, so a
DNAT target reachable from that chain rewrites the reset and warns:

  WARNING: net/netfilter/xt_nat.c:154 at xt_dnat_target_v2+0x97/0xc0

Pass the protocol of the generated packet to nf_ct_attach() and mark TCP
resets as ESTABLISHED/ESTABLISHED_REPLY.  They then take the default
branch of nf_nat_inet_fn(), which only applies the mapping already
installed on the connection.  ICMP errors keep IP_CT_RELATED/_REPLY.

Passing u8 l4proto rather than a boolean keeps the call sites readable,
as nf_ct_attach(new, skb, IPPROTO_TCP) says which packet is built.

Suggested-by: Florian Westphal <fw@strlen.de>
Reported-by: syzbot+2e819f6641708bb6ab9d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2e819f6641708bb6ab9d
Signed-off-by: xujiahao <xujiahao@cmss.chinamobile.com>
---
 include/linux/netfilter.h           |  8 +++++---
 net/ipv4/icmp.c                     |  2 +-
 net/ipv4/netfilter/nf_reject_ipv4.c |  2 +-
 net/ipv6/icmp.c                     |  2 +-
 net/ipv6/netfilter/nf_reject_ipv6.c |  2 +-
 net/netfilter/core.c                |  4 ++--
 net/netfilter/nf_conntrack_core.c   | 27 +++++++++++++++++++++------
 7 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index e99afc141..c9771578f 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -454,13 +454,14 @@ nf_nat_decode_session(struct sk_buff *skb, struct flowi *fl, u_int8_t family)
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
 #include <linux/netfilter/nf_conntrack_zones_common.h>
 
-void nf_ct_attach(struct sk_buff *, const struct sk_buff *);
+void nf_ct_attach(struct sk_buff *, const struct sk_buff *, u8 l4proto);
 void nf_ct_set_closing(struct nf_conntrack *nfct);
 struct nf_conntrack_tuple;
 bool nf_ct_get_tuple_skb(struct nf_conntrack_tuple *dst_tuple,
 			 const struct sk_buff *skb);
 #else
-static inline void nf_ct_attach(struct sk_buff *new, struct sk_buff *skb) {}
+static inline void nf_ct_attach(struct sk_buff *new, struct sk_buff *skb,
+				u8 l4proto) {}
 static inline void nf_ct_set_closing(struct nf_conntrack *nfct) {}
 struct nf_conntrack_tuple;
 static inline bool nf_ct_get_tuple_skb(struct nf_conntrack_tuple *dst_tuple,
@@ -478,7 +479,8 @@ struct nf_ct_hook {
 	void (*destroy)(struct nf_conntrack *);
 	bool (*get_tuple_skb)(struct nf_conntrack_tuple *,
 			      const struct sk_buff *);
-	void (*attach)(struct sk_buff *nskb, const struct sk_buff *skb);
+	void (*attach)(struct sk_buff *nskb, const struct sk_buff *skb,
+		       u8 l4proto);
 	void (*set_closing)(struct nf_conntrack *nfct);
 	int (*confirm)(struct sk_buff *skb);
 	u32 (*get_id)(const struct nf_conntrack *nfct);
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 90c0e22c2..e01e32941 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -375,7 +375,7 @@ static int icmp_glue_bits(void *from, char *to, int offset, int len, int odd,
 	if (icmp_param->data.icmph.type <= NR_ICMP_TYPES &&
 	    icmp_pointers[array_index_nospec(icmp_param->data.icmph.type,
 					     NR_ICMP_TYPES + 1)].error)
-		nf_ct_attach(skb, icmp_param->skb);
+		nf_ct_attach(skb, icmp_param->skb, IPPROTO_ICMP);
 	return 0;
 }
 
diff --git a/net/ipv4/netfilter/nf_reject_ipv4.c b/net/ipv4/netfilter/nf_reject_ipv4.c
index 7cb33af67..2b7251b4a 100644
--- a/net/ipv4/netfilter/nf_reject_ipv4.c
+++ b/net/ipv4/netfilter/nf_reject_ipv4.c
@@ -308,7 +308,7 @@ void nf_send_reset(struct net *net, struct sock *sk, struct sk_buff *oldskb,
 	if (nskb->len > dst4_mtu(skb_dst(nskb)))
 		goto free_nskb;
 
-	nf_ct_attach(nskb, oldskb);
+	nf_ct_attach(nskb, oldskb, IPPROTO_TCP);
 	nf_ct_set_closing(skb_nfct(oldskb));
 
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index a95b03518..f95800d54 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -327,7 +327,7 @@ static int icmpv6_getfrag(void *from, char *to, int offset, int len, int odd, st
 				      to, len);
 	skb->csum = csum_block_add(skb->csum, csum, odd);
 	if (!(msg->type & ICMPV6_INFOMSG_MASK))
-		nf_ct_attach(skb, org_skb);
+		nf_ct_attach(skb, org_skb, IPPROTO_ICMP);
 	return 0;
 }
 
diff --git a/net/ipv6/netfilter/nf_reject_ipv6.c b/net/ipv6/netfilter/nf_reject_ipv6.c
index 5e08d8529..7c0035964 100644
--- a/net/ipv6/netfilter/nf_reject_ipv6.c
+++ b/net/ipv6/netfilter/nf_reject_ipv6.c
@@ -377,7 +377,7 @@ void nf_send_reset6(struct net *net, struct sock *sk, struct sk_buff *oldskb,
 	nf_reject_ip6hdr_put(nskb, oldskb, IPPROTO_TCP, ip6_dst_hoplimit(dst));
 	nf_reject_ip6_tcphdr_put(nskb, oldskb, otcph, otcplen);
 
-	nf_ct_attach(nskb, oldskb);
+	nf_ct_attach(nskb, oldskb, IPPROTO_TCP);
 	nf_ct_set_closing(skb_nfct(oldskb));
 
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 44c856c1e..a6ac17237 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -702,7 +702,7 @@ EXPORT_SYMBOL_GPL(nf_nat_hook);
  * tracking in use: without this, connection may not be in hash table, and hence
  * manufactured ICMP or RST packets will not be associated with it.
  */
-void nf_ct_attach(struct sk_buff *new, const struct sk_buff *skb)
+void nf_ct_attach(struct sk_buff *new, const struct sk_buff *skb, u8 l4proto)
 {
 	const struct nf_ct_hook *ct_hook;
 
@@ -710,7 +710,7 @@ void nf_ct_attach(struct sk_buff *new, const struct sk_buff *skb)
 		rcu_read_lock();
 		ct_hook = rcu_dereference(nf_ct_hook);
 		if (ct_hook)
-			ct_hook->attach(new, skb);
+			ct_hook->attach(new, skb, l4proto);
 		rcu_read_unlock();
 	}
 }
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 931b76aac..5cb068e14 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2178,17 +2178,32 @@ EXPORT_SYMBOL_GPL(nf_ct_port_nlattr_tuple_size);
 #endif
 
 /* Used by ipt_REJECT and ip6t_REJECT. */
-static void nf_conntrack_attach(struct sk_buff *nskb, const struct sk_buff *skb)
+static void nf_conntrack_attach(struct sk_buff *nskb, const struct sk_buff *skb,
+				u8 l4proto)
 {
 	struct nf_conn *ct;
 	enum ip_conntrack_info ctinfo;
 
-	/* This ICMP is in reverse direction to the packet which caused it */
 	ct = nf_ct_get(skb, &ctinfo);
-	if (CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL)
-		ctinfo = IP_CT_RELATED_REPLY;
-	else
-		ctinfo = IP_CT_RELATED;
+	if (l4proto == IPPROTO_TCP) {
+		/*
+		 * A reset we generate for an existing TCP connection belongs
+		 * to that connection, it is not an error related to it, so it
+		 * carries the state of the connection.  Attaching it as
+		 * related would let it take a fresh NAT mapping on its way
+		 * out, i.e. rewrite the reset.
+		 */
+		if (CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL)
+			ctinfo = IP_CT_ESTABLISHED_REPLY;
+		else
+			ctinfo = IP_CT_ESTABLISHED;
+	} else {
+		/* Errors are in reverse direction to the packet that caused them. */
+		if (CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL)
+			ctinfo = IP_CT_RELATED_REPLY;
+		else
+			ctinfo = IP_CT_RELATED;
+	}
 
 	/* Attach to new skbuff, and increment count */
 	nf_ct_set(nskb, ct, ctinfo);
-- 
2.43.0