Ignore V2; I sent V3 with more improvements. On Mon, Jan 21, 2019 at 12:12 PM Darrell Ball <[email protected]> wrote:
> From: David Marchand <[email protected]> > > When replacing the ipv4 address in repl_ftp_v4_addr(), the remaining size > was incorrectly calculated which could lead to the wrong replacement > adjustment. > > This goes unnoticed most of the time, unless you choose carefully your > initial and replacement addresses. > > Example fail address combination with 10.1.1.200 DNAT'd to 10.1.100.1. > > Fix this by getting rid of remain_size since it is a derived variable > and use the primary variable byte_str instead. > > A test is updated to exercise different initial and replacement addresses > and another test is added. > > Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.") > Co-authored-by: Darrell Ball <[email protected]> > Signed-off-by: Darrell Ball <[email protected]> > Signed-off-by: David Marchand <[email protected]> > --- > lib/conntrack.c | 8 ++---- > tests/system-traffic.at | 74 > ++++++++++++++++++++++++++++++++++++++++++++----- > 2 files changed, 70 insertions(+), 12 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index f732b9e..f1e6ee2 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -2806,8 +2806,6 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 > v4_addr_rep, > return 0; > } > > - size_t remain_size = tcp_payload_length(pkt) - > - addr_offset_from_ftp_data_start; > int overall_delta = 0; > char *byte_str = ftp_data_start + addr_offset_from_ftp_data_start; > > @@ -2817,13 +2815,13 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 > v4_addr_rep, > char *next_delim = memchr(byte_str, ',', 4); > ovs_assert(next_delim); > int substr_size = next_delim - byte_str; > - remain_size -= substr_size; > > /* Compose the new string for this octet, and replace it. */ > - char rep_str[4]; > uint8_t rep_byte = get_v4_byte_be(v4_addr_rep, i); > + char rep_str[4]; > int replace_size = sprintf(rep_str, "%d", rep_byte); > - replace_substring(byte_str, substr_size, remain_size, > + replace_substring(byte_str, substr_size, > + (const char *) dp_packet_tail(pkt) - byte_str, > rep_str, replace_size); > overall_delta += replace_size - substr_size; > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 3a62e17..3e3b9e7 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -4652,6 +4652,66 @@ > tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src= > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([conntrack - IPv4 FTP Passive with DNAT 2]) > +AT_SKIP_IF([test $HAVE_FTP = no]) > +CHECK_CONNTRACK() > +CHECK_CONNTRACK_NAT() > +CHECK_CONNTRACK_ALG() > + > +OVS_TRAFFIC_VSWITCHD_START() > + > +ADD_NAMESPACES(at_ns0, at_ns1) > + > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/16") > +NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address e6:66:c1:11:11:11]) > +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.200 e6:66:c1:22:22:22]) > +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.100.1 e6:66:c1:22:22:22]) > + > +ADD_VETH(p1, at_ns1, br0, "10.1.100.1/16") > +NS_CHECK_EXEC([at_ns1], [ip link set dev p1 address e6:66:c1:22:22:22]) > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e6:66:c1:11:11:11]) > + > +dnl Allow any traffic from ns0->ns1. > +AT_DATA([flows.txt], [dnl > +dnl track all IPv4 traffic and NAT any established traffic. > +table=0 priority=10 ip, action=ct(nat,table=1) > +table=0 priority=0 action=drop > +dnl > +dnl Table 1 > +dnl > +dnl Allow new FTP control connections. > +table=1 in_port=1 ct_state=+new tcp nw_src=10.1.1.1 tp_dst=21 > action=ct(alg=ftp,commit,nat(dst=10.1.100.1)),2 > +dnl Allow related TCP connections from port 1. > +table=1 in_port=1 ct_state=+new+rel tcp nw_src=10.1.1.1 > action=ct(commit,nat),2 > +dnl Allow established TCP connections both ways, post-NAT match. > +table=1 in_port=1 ct_state=+est tcp nw_dst=10.1.100.1 action=2 > +table=1 in_port=2 ct_state=+est tcp nw_dst=10.1.1.1 action=1 > + > +dnl Allow ICMP both ways. > +table=1 priority=100 in_port=1 icmp, action=2 > +table=1 priority=100 in_port=2 icmp, action=1 > +table=1 priority=0, action=drop > +]) > + > +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt]) > + > +dnl Check that the stacks working to avoid races. > +OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.100.1 >/dev/null]) > + > +OVS_START_L7([at_ns1], [ftp]) > + > +dnl FTP requests from p0->p1 should work fine. > +NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.1.200 -t 3 -T 1 > --retry-connrefused -v -o wget0.log]) > + > +dnl Discards CLOSE_WAIT and CLOSING > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.200)], [0], > [dnl > > +tcp,orig=(src=10.1.1.1,dst=10.1.1.200,sport=<cleared>,dport=<cleared>),reply=(src=10.1.100.1,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>) > > +tcp,orig=(src=10.1.1.1,dst=10.1.1.200,sport=<cleared>,dport=<cleared>),reply=(src=10.1.100.1,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>),helper=ftp > +]) > + > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([conntrack - IPv4 FTP Active with DNAT]) > AT_SKIP_IF([test $HAVE_FTP = no]) > CHECK_CONNTRACK() > @@ -4722,12 +4782,12 @@ OVS_TRAFFIC_VSWITCHD_START() > > ADD_NAMESPACES(at_ns0, at_ns1) > > -ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/16") > NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address e6:66:c1:11:11:11]) > NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e6:66:c1:22:22:22]) > -NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.240 e6:66:c1:22:22:22]) > +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.120.240 e6:66:c1:22:22:22]) > > -ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/16") > NS_CHECK_EXEC([at_ns1], [ip link set dev p1 address e6:66:c1:22:22:22]) > NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e6:66:c1:11:11:11]) > > @@ -4761,12 +4821,12 @@ OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 > 10.1.1.2 >/dev/null]) > OVS_START_L7([at_ns1], [ftp]) > > dnl FTP requests from p0->p1 should work fine. > -NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.1.240 --no-passive-ftp -t 3 -T > 1 --retry-connrefused -v -o wget0.log]) > +NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.120.240 --no-passive-ftp -t 3 > -T 1 --retry-connrefused -v -o wget0.log]) > > dnl Discards CLOSE_WAIT and CLOSING > -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], > [dnl > > -tcp,orig=(src=10.1.1.1,dst=10.1.1.240,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>),helper=ftp > > -tcp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.1,dst=10.1.1.240,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>) > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.120.240)], > [0], [dnl > > +tcp,orig=(src=10.1.1.1,dst=10.1.120.240,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>),helper=ftp > > +tcp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.1,dst=10.1.120.240,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>) > ]) > > OVS_TRAFFIC_VSWITCHD_STOP > -- > 1.9.1 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
