[ovs-dev,v4,1/2] conntrack: fix ftp ipv4 address substitution.

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 <david.marchand@redhat.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Darrell Ball <dlu998@gmail.com>
---

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(-)

On Mon, Jan 21, 2019 at 9:27 PM Darrell Ball <dlu998@gmail.com> 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 <david.marchand@redhat.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> ---
>
> 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
>
>

It looks like you have some changes to both patches, will you post a v5?

Is this series otherwise ready to go (do you have buy-in from David for
the revisions)?

I would like to wait for David to return, so please hold off for now.

On 1/22/19, 3:58 PM, "ovs-dev-bounces@openvswitch.org on behalf of Ben Pfaff" <ovs-dev-bounces@openvswitch.org on behalf of blp@ovn.org> wrote:

    It looks like you have some changes to both patches, will you post a v5?
    
    Is this series otherwise ready to go (do you have buy-in from David for
    the revisions)?
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&amp;data=02%7C01%7Cdball%40vmware.com%7C4f0c335b794d455654a608d680c58eae%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636837983358980395&amp;sdata=vSTbqptNyV15qdBXKJpU898QODlJKF0uAo6tsXaUyBY%3D&amp;reserved=0

Hello Darrell,

On Wed, Jan 23, 2019 at 1:14 AM Darrell Ball via dev <
ovs-dev@openvswitch.org> wrote:

> I would like to wait for David to return, so please hold off for now.
>
> On 1/22/19, 3:58 PM, "ovs-dev-bounces@openvswitch.org on behalf of Ben
> Pfaff" <ovs-dev-bounces@openvswitch.org on behalf of blp@ovn.org> wrote:
>
>     It looks like you have some changes to both patches, will you post a
> v5?
>
>     Is this series otherwise ready to go (do you have buy-in from David for
>     the revisions)?
>

Directly jumped to v4, I did not look at v2 or v3.
Looking at v4, there is not much from my original patch, so I'd prefer to
have a Reported-by tag rather than a Sob.

Looked at the patch, nothing wrong to me in it, you can add my
Reviewed-by: David Marchand <david.marchand@redhat.com>

Thanks.

On Sat, Jan 26, 2019 at 2:59 AM David Marchand <david.marchand@redhat.com>
wrote:

> Hello Darrell,
>
> On Wed, Jan 23, 2019 at 1:14 AM Darrell Ball via dev <
> ovs-dev@openvswitch.org> wrote:
>
>> I would like to wait for David to return, so please hold off for now.
>>
>> On 1/22/19, 3:58 PM, "ovs-dev-bounces@openvswitch.org on behalf of Ben
>> Pfaff" <ovs-dev-bounces@openvswitch.org on behalf of blp@ovn.org> wrote:
>>
>>     It looks like you have some changes to both patches, will you post a
>> v5?
>>
>>     Is this series otherwise ready to go (do you have buy-in from David
>> for
>>     the revisions)?
>>
>
> Directly jumped to v4, I did not look at v2 or v3.
> Looking at v4, there is not much from my original patch, so I'd prefer to
> have a Reported-by tag rather than a Sob.
>

done


>
> Looked at the patch, nothing wrong to me in it, you can add my
> Reviewed-by: David Marchand <david.marchand@redhat.com>
>

thank you David

Darrell


>
> Thanks.
>
> --
> David Marchand
>