| Message ID | 20180224194106.11060-1-fw@strlen.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf] netfilter: ipvs: flag ct as needing s/dnat in original direction | expand |
Cc'ing Simon and Julian. Please, ack this and I'll place this in the nf.git tree. Thanks. On Sat, Feb 24, 2018 at 08:41:06PM +0100, Florian Westphal wrote: > FTP passive mode got broken by this change: > - if (.. && nfct_nat(ct)) { > + if (.. (ct->status & IPS_NAT_MASK)) { > > The PASV reply sent by real server need to be translated to contain > the load balancers address, but they are passed unchanged. > > IPS_NAT_MASK should be true for connections where reverse > of reply tuple isn't the original tupe (i.e. connection had > its source/destination changed), but ipvs uses a different function > to replace the reply tuple address, and did not set SRC/DST NAT bit. > > Reported-by: Li Shuang <shuali@redhat.com> > Fixes: 41390895e50bc4 ("netfilter: ipvs: don't check for presence of nat extension") > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > net/netfilter/ipvs/ip_vs_nfct.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_nfct.c b/net/netfilter/ipvs/ip_vs_nfct.c > index 6cf3fd81a5ec..0ceac36819d9 100644 > --- a/net/netfilter/ipvs/ip_vs_nfct.c > +++ b/net/netfilter/ipvs/ip_vs_nfct.c > @@ -119,13 +119,17 @@ ip_vs_update_conntrack(struct sk_buff *skb, struct ip_vs_conn *cp, int outin) > if (outin) { > new_tuple.src.u3 = cp->daddr; > if (new_tuple.dst.protonum != IPPROTO_ICMP && > - new_tuple.dst.protonum != IPPROTO_ICMPV6) > + new_tuple.dst.protonum != IPPROTO_ICMPV6) { > new_tuple.src.u.tcp.port = cp->dport; > + ct->status |= IPS_SRC_NAT; > + } > } else { > new_tuple.dst.u3 = cp->vaddr; > if (new_tuple.dst.protonum != IPPROTO_ICMP && > - new_tuple.dst.protonum != IPPROTO_ICMPV6) > + new_tuple.dst.protonum != IPPROTO_ICMPV6) { > new_tuple.dst.u.tcp.port = cp->vport; > + ct->status |= IPS_DST_NAT; > + } > } > IP_VS_DBG(7, "%s: Updating conntrack ct=%p, status=0x%lX, " > "ctinfo=%d, old reply=" FMT_TUPLE > -- > 2.16.1 > > -- > To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Sat, 24 Feb 2018, Florian Westphal wrote: > FTP passive mode got broken by this change: > - if (.. && nfct_nat(ct)) { > + if (.. (ct->status & IPS_NAT_MASK)) { Looks like this check was needed to avoid crash in 2.6.35, see commit 7bcbf81a2296a8 ("ipvs: avoid oops for passive FTP"). I just tested such fix and it should be enough to fix the passive FTP: diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c index 3e17d32..58d5d05 100644 --- a/net/netfilter/ipvs/ip_vs_ftp.c +++ b/net/netfilter/ipvs/ip_vs_ftp.c @@ -260,7 +260,7 @@ static int ip_vs_ftp_out(struct ip_vs_app *app, struct ip_vs_conn *cp, buf_len = strlen(buf); ct = nf_ct_get(skb, &ctinfo); - if (ct && (ct->status & IPS_NAT_MASK)) { + if (ct) { bool mangled; /* If mangling fails this function will return 0 If it looks ok to you and if you prefer you can submit it as patch, otherwise I'll do it in the following days... > The PASV reply sent by real server need to be translated to contain > the load balancers address, but they are passed unchanged. > > IPS_NAT_MASK should be true for connections where reverse > of reply tuple isn't the original tupe (i.e. connection had > its source/destination changed), but ipvs uses a different function > to replace the reply tuple address, and did not set SRC/DST NAT bit. > > Reported-by: Li Shuang <shuali@redhat.com> > Fixes: 41390895e50bc4 ("netfilter: ipvs: don't check for presence of nat extension") > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > net/netfilter/ipvs/ip_vs_nfct.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_nfct.c b/net/netfilter/ipvs/ip_vs_nfct.c > index 6cf3fd81a5ec..0ceac36819d9 100644 > --- a/net/netfilter/ipvs/ip_vs_nfct.c > +++ b/net/netfilter/ipvs/ip_vs_nfct.c > @@ -119,13 +119,17 @@ ip_vs_update_conntrack(struct sk_buff *skb, struct ip_vs_conn *cp, int outin) > if (outin) { > new_tuple.src.u3 = cp->daddr; > if (new_tuple.dst.protonum != IPPROTO_ICMP && > - new_tuple.dst.protonum != IPPROTO_ICMPV6) > + new_tuple.dst.protonum != IPPROTO_ICMPV6) { > new_tuple.src.u.tcp.port = cp->dport; > + ct->status |= IPS_SRC_NAT; These bits should be reversed, 'outin' means 'from client towards real server', i.e. like DNAT rule in original direction: ct->status |= IPS_DST_NAT; > + } > } else { > new_tuple.dst.u3 = cp->vaddr; > if (new_tuple.dst.protonum != IPPROTO_ICMP && > - new_tuple.dst.protonum != IPPROTO_ICMPV6) > + new_tuple.dst.protonum != IPPROTO_ICMPV6) { > new_tuple.dst.u.tcp.port = cp->vport; > + ct->status |= IPS_DST_NAT; here too: ct->status |= IPS_SRC_NAT; But for now we do not need to set them. If your patch works, it was because the bits helped for the IPS_NAT_MASK check to allow the nf_nat_mangle_tcp_packet call. Regards -- Julian Anastasov <ja@ssi.bg> -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Julian Anastasov <ja@ssi.bg> wrote: > On Sat, 24 Feb 2018, Florian Westphal wrote: > > > FTP passive mode got broken by this change: > > - if (.. && nfct_nat(ct)) { > > + if (.. (ct->status & IPS_NAT_MASK)) { > > Looks like this check was needed to avoid crash in 2.6.35, > see commit 7bcbf81a2296a8 ("ipvs: avoid oops for passive FTP"). > > I just tested such fix and it should be enough > to fix the passive FTP: > > diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c > index 3e17d32..58d5d05 100644 > --- a/net/netfilter/ipvs/ip_vs_ftp.c > +++ b/net/netfilter/ipvs/ip_vs_ftp.c > @@ -260,7 +260,7 @@ static int ip_vs_ftp_out(struct ip_vs_app *app, struct ip_vs_conn *cp, > buf_len = strlen(buf); > > ct = nf_ct_get(skb, &ctinfo); > - if (ct && (ct->status & IPS_NAT_MASK)) { > + if (ct) { > bool mangled; > > /* If mangling fails this function will return 0 > > If it looks ok to you and if you prefer you can submit it > as patch, otherwise I'll do it in the following days... Looks good to me, its better to get rid of additional condition. Feel free to submit this patch officially thank you! > But for now we do not need to set them. If your patch > works, it was because the bits helped for the IPS_NAT_MASK > check to allow the nf_nat_mangle_tcp_packet call. Yes, exactly. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Sun, 25 Feb 2018, Florian Westphal wrote: > Julian Anastasov <ja@ssi.bg> wrote: > > ct = nf_ct_get(skb, &ctinfo); > > - if (ct && (ct->status & IPS_NAT_MASK)) { > > + if (ct) { > > bool mangled; > > > > /* If mangling fails this function will return 0 > > > > If it looks ok to you and if you prefer you can submit it > > as patch, otherwise I'll do it in the following days... > > Looks good to me, its better to get rid of additional condition. > Feel free to submit this patch officially thank you! Done, thanks! Regards -- Julian Anastasov <ja@ssi.bg> -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/ipvs/ip_vs_nfct.c b/net/netfilter/ipvs/ip_vs_nfct.c index 6cf3fd81a5ec..0ceac36819d9 100644 --- a/net/netfilter/ipvs/ip_vs_nfct.c +++ b/net/netfilter/ipvs/ip_vs_nfct.c @@ -119,13 +119,17 @@ ip_vs_update_conntrack(struct sk_buff *skb, struct ip_vs_conn *cp, int outin) if (outin) { new_tuple.src.u3 = cp->daddr; if (new_tuple.dst.protonum != IPPROTO_ICMP && - new_tuple.dst.protonum != IPPROTO_ICMPV6) + new_tuple.dst.protonum != IPPROTO_ICMPV6) { new_tuple.src.u.tcp.port = cp->dport; + ct->status |= IPS_SRC_NAT; + } } else { new_tuple.dst.u3 = cp->vaddr; if (new_tuple.dst.protonum != IPPROTO_ICMP && - new_tuple.dst.protonum != IPPROTO_ICMPV6) + new_tuple.dst.protonum != IPPROTO_ICMPV6) { new_tuple.dst.u.tcp.port = cp->vport; + ct->status |= IPS_DST_NAT; + } } IP_VS_DBG(7, "%s: Updating conntrack ct=%p, status=0x%lX, " "ctinfo=%d, old reply=" FMT_TUPLE
FTP passive mode got broken by this change: - if (.. && nfct_nat(ct)) { + if (.. (ct->status & IPS_NAT_MASK)) { The PASV reply sent by real server need to be translated to contain the load balancers address, but they are passed unchanged. IPS_NAT_MASK should be true for connections where reverse of reply tuple isn't the original tupe (i.e. connection had its source/destination changed), but ipvs uses a different function to replace the reply tuple address, and did not set SRC/DST NAT bit. Reported-by: Li Shuang <shuali@redhat.com> Fixes: 41390895e50bc4 ("netfilter: ipvs: don't check for presence of nat extension") Signed-off-by: Florian Westphal <fw@strlen.de> --- net/netfilter/ipvs/ip_vs_nfct.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)