On Mon, Jan 21, 2019 at 9:27 PM Darrell Ball <[email protected]> wrote:
> 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 doing something similar to V6 and also splicing out common
> code for better coverage and maintainability.
>
> 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: David Marchand <[email protected]>
> Signed-off-by: David Marchand <[email protected]>
> Signed-off-by: Darrell Ball <[email protected]>
> ---
>
> v4: Fix bug in call to inet_ntop() in repl_ftp_v4_addr().
> v3: Make V4 and V6 code paths more common.
> v2: Minimum alternative patch; also add tests.
>
> lib/conntrack.c | 118
> +++++++++++++++++++++++-------------------------
> tests/system-traffic.at | 74 +++++++++++++++++++++++++++---
> 2 files changed, 123 insertions(+), 69 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index f732b9e..6b66750 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -136,7 +136,7 @@ expectation_lookup(struct hmap *alg_expectations,
> const struct conn_key *key,
> static int
> repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 v4_addr_rep,
> char *ftp_data_v4_start,
> - size_t addr_offset_from_ftp_data_start);
> + size_t addr_offset_from_ftp_data_start, size_t
> addr_size);
>
> static enum ftp_ctl_pkt
> process_ftp_ctl_v4(struct conntrack *ct,
> @@ -144,7 +144,8 @@ process_ftp_ctl_v4(struct conntrack *ct,
> const struct conn *conn_for_expectation,
> ovs_be32 *v4_addr_rep,
> char **ftp_data_v4_start,
> - size_t *addr_offset_from_ftp_data_start);
> + size_t *addr_offset_from_ftp_data_start,
> + size_t *addr_size);
>
> static enum ftp_ctl_pkt
> detect_ftp_ctl_type(const struct conn_lookup_ctx *ctx,
> @@ -2771,13 +2772,6 @@ expectation_create(struct conntrack *ct, ovs_be16
> dst_port,
> ct_rwlock_unlock(&ct->resources_lock);
> }
>
> -static uint8_t
> -get_v4_byte_be(ovs_be32 v4_addr, uint8_t index)
> -{
> - uint8_t *byte_ptr = (OVS_FORCE uint8_t *) &v4_addr;
> - return byte_ptr[index];
> -}
> -
> static void
> replace_substring(char *substr, uint8_t substr_size,
> uint8_t total_size, char *rep_str,
> @@ -2788,51 +2782,56 @@ replace_substring(char *substr, uint8_t
> substr_size,
> memcpy(substr, rep_str, rep_str_size);
> }
>
> +static void
> +repl_bytes(char *str, char c1, char c2)
> +{
> + while (*str) {
> + if (*str == c1) {
> + *str = c2;
> + }
> + str++;
> + }
> +}
> +
> +static void
> +modify_packet(struct dp_packet *pkt, char *pkt_str, size_t size,
> + char *repl_str, size_t repl_size,
> + uint32_t orig_used_size)
> +{
> + replace_substring(pkt_str, size,
> + (const char *) dp_packet_tail(pkt) - pkt_str,
> + repl_str, repl_size);
> + dp_packet_set_size(pkt, orig_used_size + (int) repl_size - (int)
> size);
> +}
> +
> /* Replace IPV4 address in FTP message with NATed address. */
> static int
> repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 v4_addr_rep,
> char *ftp_data_start,
> - size_t addr_offset_from_ftp_data_start)
> + size_t addr_offset_from_ftp_data_start,
> + size_t addr_size OVS_UNUSED)
>
'OVS_UNUSED' above will be removed.
> {
> enum { MAX_FTP_V4_NAT_DELTA = 8 };
>
> /* Do conservative check for pathological MTU usage. */
> uint32_t orig_used_size = dp_packet_size(pkt);
> - uint16_t allocated_size = dp_packet_get_allocated(pkt);
> - if (orig_used_size + MAX_FTP_V4_NAT_DELTA > allocated_size) {
> + if (orig_used_size + MAX_FTP_V4_NAT_DELTA >
> + dp_packet_get_allocated(pkt)) {
> +
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> - VLOG_WARN_RL(&rl, "Unsupported effective MTU %u used with FTP",
> - allocated_size);
> + VLOG_WARN_RL(&rl, "Unsupported effective MTU %u used with FTP V4",
> + dp_packet_get_allocated(pkt));
> 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;
> -
> - /* Replace the existing IPv4 address by the new one. */
> - for (uint8_t i = 0; i < 4; i++) {
> - /* Find the end of the string for this octet. */
> - 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);
> - int replace_size = sprintf(rep_str, "%d", rep_byte);
> - replace_substring(byte_str, substr_size, remain_size,
> - rep_str, replace_size);
> - overall_delta += replace_size - substr_size;
> -
> - /* Advance past the octet and the following comma. */
> - byte_str += replace_size + 1;
> - }
> -
> - dp_packet_set_size(pkt, orig_used_size + overall_delta);
> - return overall_delta;
> + char v4_addr_str[INET_ADDRSTRLEN] = {0};
> + ovs_assert(inet_ntop(AF_INET, &v4_addr_rep, v4_addr_str,
> + sizeof v4_addr_str));
> + repl_bytes(v4_addr_str, '.', ',');
> + modify_packet(pkt, ftp_data_start + addr_offset_from_ftp_data_start,
> + addr_size, v4_addr_str, strlen(v4_addr_str),
> + orig_used_size);
> + return (int) strlen(v4_addr_str) - (int) addr_size;
> }
>
> static char *
> @@ -2901,7 +2900,8 @@ process_ftp_ctl_v4(struct conntrack *ct,
> const struct conn *conn_for_expectation,
> ovs_be32 *v4_addr_rep,
> char **ftp_data_v4_start,
> - size_t *addr_offset_from_ftp_data_start)
> + size_t *addr_offset_from_ftp_data_start,
> + size_t *addr_size)
> {
> struct tcp_header *th = dp_packet_l4(pkt);
> size_t tcp_hdr_len = TCP_OFFSET(th->tcp_ctl) * 4;
> @@ -2957,6 +2957,7 @@ process_ftp_ctl_v4(struct conntrack *ct,
> return CT_FTP_CTL_INVALID;
> }
>
> + *addr_size = ftp - ip_addr_start - 1;
> char *save_ftp = ftp;
> ftp = terminate_number_str(ftp, MAX_FTP_PORT_DGTS);
> if (!ftp) {
> @@ -3149,31 +3150,22 @@ repl_ftp_v6_addr(struct dp_packet *pkt, struct
> ct_addr v6_addr_rep,
>
> /* Do conservative check for pathological MTU usage. */
> uint32_t orig_used_size = dp_packet_size(pkt);
> - uint16_t allocated_size = dp_packet_get_allocated(pkt);
> - if (orig_used_size + MAX_FTP_V6_NAT_DELTA > allocated_size) {
> + if (orig_used_size + MAX_FTP_V6_NAT_DELTA >
> + dp_packet_get_allocated(pkt)) {
> +
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> - VLOG_WARN_RL(&rl, "Unsupported effective MTU %u used with FTP",
> - allocated_size);
> + VLOG_WARN_RL(&rl, "Unsupported effective MTU %u used with FTP V6",
> + dp_packet_get_allocated(pkt));
> return 0;
> }
>
> char v6_addr_str[IPV6_SCAN_LEN] = {0};
> ovs_assert(inet_ntop(AF_INET6, &v6_addr_rep.ipv6_aligned, v6_addr_str,
> IPV6_SCAN_LEN - 1));
> -
> - size_t replace_addr_size = strlen(v6_addr_str);
> -
> - size_t remain_size = tcp_payload_length(pkt) -
> - addr_offset_from_ftp_data_start;
> -
> - char *pkt_addr_str = ftp_data_start + addr_offset_from_ftp_data_start;
> - replace_substring(pkt_addr_str, addr_size, remain_size,
> - v6_addr_str, replace_addr_size);
> -
> - int overall_delta = (int) replace_addr_size - (int) addr_size;
> -
> - dp_packet_set_size(pkt, orig_used_size + overall_delta);
> - return overall_delta;
> + modify_packet(pkt, ftp_data_start + addr_offset_from_ftp_data_start,
> + addr_size, v6_addr_str, strlen(v6_addr_str),
> + orig_used_size);
> + return (int) strlen(v6_addr_str) - (int) addr_size;
> }
>
> /* Increment/decrement a TCP sequence number. */
> @@ -3213,7 +3205,8 @@ handle_ftp_ctl(struct conntrack *ct, const struct
> conn_lookup_ctx *ctx,
> } else {
> rc = process_ftp_ctl_v4(ct, pkt, ec,
> &v4_addr_rep, &ftp_data_start,
> - &addr_offset_from_ftp_data_start);
> + &addr_offset_from_ftp_data_start,
> + &addr_size);
> }
> if (rc == CT_FTP_CTL_INVALID) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> @@ -3240,7 +3233,8 @@ handle_ftp_ctl(struct conntrack *ct, const struct
> conn_lookup_ctx *ctx,
> if (nat) {
> seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep,
> ftp_data_start,
> - addr_offset_from_ftp_data_start);
> + addr_offset_from_ftp_data_start,
> + addr_size);
> }
> if (seq_skew) {
> ip_len = ntohs(l3_hdr->ip_tot_len) + seq_skew;
> 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
