[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).

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

[xujiahao]

Hi Florian,

You are right and my patch was wrong.  I am dropping the xt_nat change; the
check in xt_dnat_target_v2() should stay as it is.

I did reproduce the syzbot report in a netns: a reject rule makes
nf_send_reset() build the RST, which then goes ip_local_out() ->
nf_nat_ipv4_local_fn(), and the RST arrives with ctinfo IP_CT_RELATED_REPLY
(4).  nf_nat_inet_fn() therefore treats it as not initialized yet and runs the
nat chains for it, which is how xt_dnat_target_v2() is reached.  In my test
the conntrack was already confirmed, so nf_nat_setup_info() declined to
install a second mapping and the packet itself was not rewritten -- but the
nat chains should not be consulted for that packet at all.

Before I send a fix for nf_conntrack_attach(): it cannot tell an ICMP error
from a TCP RST on its own.  The icmp.c/icmpv6.c callers run inside a getfrag
callback, before the IP header is built, and the conntrack's L4 protocol is
the protocol of the original packet, which can well be TCP for an ICMP error.
So the packet type has to come from the call site.  Which shape do you prefer?

  a) nf_ct_attach(new, skb, bool tcp_rst) -- changes the exported symbol, or
  b) keep nf_ct_attach() as is and add a separate helper for the reject path?

Either way ICMP keeps IP_CT_RELATED/_REPLY, and only the RST built by
nf_reject_ipv4.c / nf_reject_ipv6.c would get IP_CT_ESTABLISHED/_REPLY.

Thanks,
xujiahao

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

[Florian Westphal]

xujiahao <xujiahao@cmss.chinamobile.com> wrote:
>   a) nf_ct_attach(new, skb, bool tcp_rst) -- changes the exported symbol, or
>   b) keep nf_ct_attach() as is and add a separate helper for the reject path?

Why not nf_ct_attach(new, skb, u8 l4proto) ?

This makes things easier to read, instead of
nf_ct_attach(new, skb, true)

readers see
nf_ct_attach(new, skb, IPPROTO_TCP).

> Either way ICMP keeps IP_CT_RELATED/_REPLY, and only the RST built by
> nf_reject_ipv4.c / nf_reject_ipv6.c would get IP_CT_ESTABLISHED/_REPLY.

Makes sense to me.

[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