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